Re: [U-Boot] [PATCH v3] x86: ich-spi: Convert driver to spi-mem ops

On Mon, Jul 29, 2019 at 6:43 PM Bernhard Messerklinger bernhard.messerklinger@br-automation.com wrote:

With the introduction of the new spi-mem model operations changed slightly. The new spi-mem operations make things a bit easier to handle for ich-spi flash interface. This patch converts the ich-spi driver by using spi-mem operations.

---

Changes in v2:

• make 2 routines static; update commit message a little bit

Changes in v3:

• update erase op code handling to work with latest master

spi-nor driver

The changelog should go after SoB, and below --.

Signed-off-by: Bernhard Messerklinger bernhard.messerklinger@br-automation.com

drivers/spi/ich.c | 267 +++++++++++++++++----------------------------- drivers/spi/ich.h | 9 +- 2 files changed, 100 insertions(+), 176 deletions(-)

diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c index 03531a8c0c..179f801d42 100644 --- a/drivers/spi/ich.c +++ b/drivers/spi/ich.c @@ -14,6 +14,8 @@ #include <pci_ids.h> #include <spi.h> #include <asm/io.h> +#include <spi-mem.h> +#include <div64.h>

#include "ich.h"

@@ -171,18 +173,6 @@ static int ich_init_controller(struct udevice *dev, return 0; }

-static inline void spi_use_out(struct spi_trans *trans, unsigned bytes) -{

•   trans->out += bytes;
  

•   trans->bytesout -= bytes;
  

-}

-static inline void spi_use_in(struct spi_trans *trans, unsigned bytes) -{

•   trans->in += bytes;
  

•   trans->bytesin -= bytes;
  

-}

static void spi_lock_down(struct ich_spi_platdata *plat, void *sbase) { if (plat->ich_version == ICHV_7) { @@ -213,47 +203,12 @@ static bool spi_lock_status(struct ich_spi_platdata *plat, void *sbase) return lock != 0; }

-static void spi_setup_type(struct spi_trans *trans, int data_bytes) -{

•   trans->type = 0xFF;
  

•   /* Try to guess spi type from read/write sizes */
  

•   if (trans->bytesin == 0) {
  

•           if (trans->bytesout + data_bytes > 4)
  

•                   /*
  

•                    * If bytesin = 0 and bytesout > 4, we presume this is
  

•                    * a write data operation, which is accompanied by an
  

•                    * address.
  

•                    */
  

•                   trans->type = SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS;
  

•           else
  

•                   trans->type = SPI_OPCODE_TYPE_WRITE_NO_ADDRESS;
  

•           return;
  

•   }
  

•   if (trans->bytesout == 1) {     /* and bytesin is > 0 */
  

•           trans->type = SPI_OPCODE_TYPE_READ_NO_ADDRESS;
  

•           return;
  

•   }
  

•   if (trans->bytesout == 4)       /* and bytesin is > 0 */
  

•           trans->type = SPI_OPCODE_TYPE_READ_WITH_ADDRESS;
  

•   /* Fast read command is called with 5 bytes instead of 4 */
  

•   if (trans->out[0] == SPI_OPCODE_FAST_READ && trans->bytesout == 5) {
  

•           trans->type = SPI_OPCODE_TYPE_READ_WITH_ADDRESS;
  

•           --trans->bytesout;
  

•   }
  

-}

static int spi_setup_opcode(struct ich_spi_priv *ctlr, struct spi_trans *trans, bool lock) { uint16_t optypes; uint8_t opmenu[ctlr->menubytes];

•   trans->opcode = trans->out[0];
  

•   spi_use_out(trans, 1);
    if (!lock) {
            /* The lock is off, so just use index 0. */
            ich_writeb(ctlr, trans->opcode, ctlr->opmenu);
  

@@ -285,12 +240,7 @@ static int spi_setup_opcode(struct ich_spi_priv *ctlr, struct spi_trans *trans,

            optypes = ich_readw(ctlr, ctlr->optype);
            optype = (optypes >> (opcode_index * 2)) & 0x3;

•           if (trans->type == SPI_OPCODE_TYPE_WRITE_NO_ADDRESS &&
  

•               optype == SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS &&
  

•               trans->bytesout >= 3) {
  

•                   /* We guessed wrong earlier. Fix it up. */
  

•                   trans->type = optype;
  

•           }
  

•           if (optype != trans->type) {
                    printf("ICH SPI: Transaction doesn't fit type %d\n",
                           optype);
  

@@ -300,26 +250,6 @@ static int spi_setup_opcode(struct ich_spi_priv *ctlr, struct spi_trans *trans, } }

-static int spi_setup_offset(struct spi_trans *trans) -{

•   /* Separate the SPI address and data */
  

•   switch (trans->type) {
  

•   case SPI_OPCODE_TYPE_READ_NO_ADDRESS:
  

•   case SPI_OPCODE_TYPE_WRITE_NO_ADDRESS:
  

•           return 0;
  

•   case SPI_OPCODE_TYPE_READ_WITH_ADDRESS:
  

•   case SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS:
  

•           trans->offset = ((uint32_t)trans->out[0] << 16) |
  

•                           ((uint32_t)trans->out[1] << 8) |
  

•                           ((uint32_t)trans->out[2] << 0);
  

•           spi_use_out(trans, 3);
  

•           return 1;
  

•   default:
  

•           printf("Unrecognized SPI transaction type %#x\n", trans->type);
  

•           return -EPROTO;
  

•   }
  

-}

/*

• Wait for up to 6s til status register bit(s) turn 1 (in case wait_til_set
• below is true) or 0. In case the wait was for the bit(s) to set - write

@@ -350,7 +280,7 @@ static int ich_status_poll(struct ich_spi_priv *ctlr, u16 bitmask, return -ETIMEDOUT; }

-void ich_spi_config_opcode(struct udevice *dev) +static void ich_spi_config_opcode(struct udevice *dev) { struct ich_spi_priv *ctlr = dev_get_priv(dev);

@@ -365,72 +295,48 @@ void ich_spi_config_opcode(struct udevice *dev) ich_writel(ctlr, SPI_OPMENU_UPPER, ctlr->opmenu + sizeof(u32)); }

-static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,

•                   const void *dout, void *din, unsigned long flags)
  

+static int ich_spi_exec_op(struct spi_slave *slave, const struct spi_mem_op *op) {

•   struct udevice *bus = dev_get_parent(dev);
  

•   struct udevice *bus = dev_get_parent(slave->dev);
    struct ich_spi_platdata *plat = dev_get_platdata(bus);
    struct ich_spi_priv *ctlr = dev_get_priv(bus);
  

•   uint16_t control;
  

•   int16_t opcode_index;
  

•   int with_address;
  

•   int status;
  

•   int bytes = bitlen / 8;
    struct spi_trans *trans = &ctlr->trans;
  

•   unsigned type = flags & (SPI_XFER_BEGIN | SPI_XFER_END);
  

•   int using_cmd = 0;
  

•   u16 control;
  

•   u16 opcode_index;
  

Why changing "uint16_t control" to "u16 control", and "int16_t opcode_index" to "u16 opcode_index"?

•   int ret = 0;
  

No need to change here as "int ret" was declared below.

•   int status;
  

Nothing changes here as "int status" was there in the original codes.

    bool lock = spi_lock_status(plat, ctlr->base);

•   int ret;
  

•   /* We don't support writing partial bytes */
  

•   if (bitlen % 8) {
  

•           debug("ICH SPI: Accessing partial bytes not supported\n");
  

•           return -EPROTONOSUPPORT;
  

•   }
  

•   /* An empty end transaction can be ignored */
  

•   if (type == SPI_XFER_END && !dout && !din)
  

•           return 0;
  

•   bool with_address = 0;
  

Why? the original codes have "int with_address".

•   if (type & SPI_XFER_BEGIN)
  

•           memset(trans, '\0', sizeof(*trans));
  

•   trans->in = NULL;
  

•   trans->out = NULL;
  

•   trans->type = 0xFF;
  
  

•   /* Dp we need to come back later to finish it? */
  

•   if (dout && type == SPI_XFER_BEGIN) {
  

•           if (bytes > ICH_MAX_CMD_LEN) {
  

•                   debug("ICH SPI: Command length limit exceeded\n");
  

•                   return -ENOSPC;
  

•   if (op->data.nbytes) {
  

•           if (op->data.dir == SPI_MEM_DATA_IN) {
  

•                   trans->in = op->data.buf.in;
  

•                   trans->bytesin = op->data.nbytes;
  

•           } else {
  

•                   trans->out = op->data.buf.out;
  

•                   trans->bytesout = op->data.nbytes;
            }
  

•           memcpy(trans->cmd, dout, bytes);
  

•           trans->cmd_len = bytes;
  

•           debug_trace("ICH SPI: Saved %d bytes\n", bytes);
  

•           return 0;
    }
  
  

•   /*
  

•    * We process a 'middle' spi_xfer() call, which has no
  

•    * SPI_XFER_BEGIN/END, as an independent transaction as if it had
  

•    * an end. We therefore repeat the command. This is because ICH
  

•    * seems to have no support for this, or because interest (in digging
  

•    * out the details and creating a special case in the code) is low.
  

•    */
  

•   if (trans->cmd_len) {
  

•           trans->out = trans->cmd;
  

•           trans->bytesout = trans->cmd_len;
  

•           using_cmd = 1;
  

•           debug_trace("ICH SPI: Using %d bytes\n", trans->cmd_len);
  

•   } else {
  

•           trans->out = dout;
  

•           trans->bytesout = dout ? bytes : 0;
  

•   }
  

•   if (trans->opcode != op->cmd.opcode)
  

•           trans->opcode = op->cmd.opcode;
  
  

•   trans->in = din;
  

•   trans->bytesin = din ? bytes : 0;
  

•   if (lock && trans->opcode == SPI_OPCODE_WRDIS)
  

•           return 0;
  
  

•   /* There has to always at least be an opcode */
  

•   if (!trans->bytesout) {
  

•           debug("ICH SPI: No opcode for transfer\n");
  

•           return -EPROTO;
  

•   if (trans->opcode == SPI_OPCODE_WREN) {
  

•           /*
  

•            * Treat Write Enable as Atomic Pre-Op if possible
  

•            * in order to prevent the Management Engine from
  

•            * issuing a transaction between WREN and DATA.
  

•            */
  

•           if (!lock)
  

•                   ich_writew(ctlr, trans->opcode, ctlr->preop);
  

•           return 0;
    }
  

[snip]

Regards, Bin