[U-Boot] [PATCH v3 1/8] spi: add support of SPI flash commands

[Wenyou.Yang at microchip.com <Wenyou.Yang@microchip.com>]

Hi Jagan,

Thank you for your comments.

I will rework this patch set according to your advice. Thank you!


Best Regards,
Wenyou Yang

> -----Original Message-----
> From: Jagan Teki [mailto:jagannadh.teki at gmail.com]
> Sent: 2017年8月30日 21:50
> To: Wenyou Yang - A41535 <Wenyou.Yang@microchip.com>
> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Marek Vasut <marex@denx.de>;
> Cyrille Pitchen <cyrille.pitchen@atmel.com>; Jagan Teki <jagan@openedev.com>
> Subject: Re: [U-Boot] [PATCH v3 1/8] spi: add support of SPI flash commands
> 
> On Tue, Jul 25, 2017 at 12:30 PM, Wenyou Yang <wenyou.yang@microchip.com>
> wrote:
> > From: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> >
> > This patch introduces 'struct spi_flash_command' and functions
> > spi_is_flash_command_supported() / spi_exec_flash_command().
> >
> 
> Answer for why this shouldn't be part of SPI area.
> 
> [snip]
> 
> drivers/spi and include/spi.h is Generic SPI area code - this can deal all
> connected slave devices like EEPROM, SPI-NOR etc drivers/mtd/spi and
> include/spi_flash.h is SPI-Flash or SPI-NOR code - this can handle only SPI
> flashes.
> 
> >
> > +bool dm_spi_is_flash_command_supported(struct udevice *dev,
> > +                                      const struct spi_flash_command
> > +*cmd) {
> > +       struct udevice *bus = dev->parent;
> > +       struct dm_spi_ops *ops = spi_get_ops(bus);
> > +
> > +       if (ops->is_flash_command_supported)
> > +               return ops->is_flash_command_supported(dev, cmd);
> > +
> > +       return false;
> > +}
> > +
> > +int dm_spi_exec_flash_command(struct udevice *dev,
> > +                             const struct spi_flash_command *cmd) {
> > +       struct udevice *bus = dev->parent;
> > +       struct dm_spi_ops *ops = spi_get_ops(bus);
> > +
> > +       if (ops->exec_flash_command)
> > +               return ops->exec_flash_command(dev, cmd);
> > +
> > +       return -EINVAL;
> > +}
> > +
> >  int spi_claim_bus(struct spi_slave *slave)  {
> >         return dm_spi_claim_bus(slave->dev); @@ -107,6 +131,18 @@ int
> > spi_xfer(struct spi_slave *slave, unsigned int bitlen,
> >         return dm_spi_xfer(slave->dev, bitlen, dout, din, flags);  }
> >
> > +bool spi_is_flash_command_supported(struct spi_slave *slave,
> > +                                   const struct spi_flash_command
> > +*cmd) {
> > +       return dm_spi_is_flash_command_supported(slave->dev, cmd); }
> > +
> > +int spi_exec_flash_command(struct spi_slave *slave,
> > +                          const struct spi_flash_command *cmd) {
> > +       return dm_spi_exec_flash_command(slave->dev, cmd); }
> 
> Handling spi-flash stuff in spi core is not proper way of dealing, and I saw these
> functions are not-at-all needed for generic controller drivers in drivers/spi except
> the Atmel QSPI driver (in v3,8/8).
> 
> > +
> >  #if !CONFIG_IS_ENABLED(OF_PLATDATA)
> >  static int spi_child_post_bind(struct udevice *dev)  { @@ -144,6
> > +180,10 @@ static int spi_post_probe(struct udevice *bus)
> >                 ops->set_mode += gd->reloc_off;
> >         if (ops->cs_info)
> >                 ops->cs_info += gd->reloc_off;
> > +       if (ops->is_flash_command_supported)
> > +               ops->is_flash_command_supported += gd->reloc_off;
> > +       if (ops->exec_flash_command)
> > +               ops->exec_flash_command += gd->reloc_off;
> >  #endif
> >
> >         return 0;
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index
> > 7d81fbd7f8..e47acdc9e4 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -5,6 +5,7 @@
> >   */
> >
> >  #include <common.h>
> > +#include <errno.h>
> >  #include <fdtdec.h>
> >  #include <malloc.h>
> >  #include <spi.h>
> > @@ -58,3 +59,15 @@ struct spi_slave *spi_base_setup_slave_fdt(const void
> *blob, int busnum,
> >         return spi_setup_slave(busnum, cs, max_hz, mode);  }  #endif
> > +
> > +__weak bool spi_is_flash_command_supported(struct spi_slave *slave,
> > +                                          const struct
> > +spi_flash_command *cmd) {
> > +       return false;
> > +}
> > +
> > +__weak int spi_exec_flash_command(struct spi_slave *slave,
> > +                                 const struct spi_flash_command *cmd)
> > +{
> > +       return -EINVAL;
> > +}
> > diff --git a/include/spi.h b/include/spi.h index
> > 8c4b882c54..a090266b52 100644
> > --- a/include/spi.h
> > +++ b/include/spi.h
> > @@ -10,6 +10,8 @@
> >  #ifndef _SPI_H_
> >  #define _SPI_H_
> >
> > +#include <linux/string.h> /* memset() */
> > +
> >  /* SPI mode flags */
> >  #define SPI_CPHA       BIT(0)                  /* clock phase */
> >  #define SPI_CPOL       BIT(1)                  /* clock polarity */
> > @@ -64,6 +66,116 @@ struct dm_spi_slave_platdata {  #endif /*
> > CONFIG_DM_SPI */
> >
> >  /**
> > + * enum spi_flash_protocol - SPI flash command protocol  */ #define
> > +SPI_FPROTO_INST_SHIFT  16
> > +#define SPI_FPROTO_INST_MASK   GENMASK(23, 16)
> > +#define SPI_FPROTO_INST(nbits)                                 \
> > +       ((((unsigned long)(nbits)) << SPI_FPROTO_INST_SHIFT) &  \
> > +        SPI_FPROTO_INST_MASK)
> > +
> > +#define SPI_FPROTO_ADDR_SHIFT  8
> > +#define SPI_FPROTO_ADDR_MASK   GENMASK(15, 8)
> > +#define SPI_FPROTO_ADDR(nbits)                                 \
> > +       ((((unsigned long)(nbits)) << SPI_FPROTO_ADDR_SHIFT) &  \
> > +        SPI_FPROTO_ADDR_MASK)
> > +
> > +#define SPI_FPROTO_DATA_SHIFT  0
> > +#define SPI_FPROTO_DATA_MASK   GENMASK(7, 0)
> > +#define SPI_FPROTO_DATA(nbits)                                 \
> > +       ((((unsigned long)(nbits)) << SPI_FPROTO_DATA_SHIFT) &  \
> > +        SPI_FPROTO_DATA_MASK)
> > +
> > +#define SPI_FPROTO(inst_nbits, addr_nbits, data_nbits) \
> > +       (SPI_FPROTO_INST(inst_nbits) |                  \
> > +        SPI_FPROTO_ADDR(addr_nbits) |                  \
> > +        SPI_FPROTO_DATA(data_nbits))
> > +
> > +enum spi_flash_protocol {
> > +       SPI_FPROTO_1_1_1 = SPI_FPROTO(1, 1, 1),
> > +       SPI_FPROTO_1_1_2 = SPI_FPROTO(1, 1, 2),
> > +       SPI_FPROTO_1_1_4 = SPI_FPROTO(1, 1, 4),
> > +       SPI_FPROTO_1_2_2 = SPI_FPROTO(1, 2, 2),
> > +       SPI_FPROTO_1_4_4 = SPI_FPROTO(1, 4, 4),
> > +       SPI_FPROTO_2_2_2 = SPI_FPROTO(2, 2, 2),
> > +       SPI_FPROTO_4_4_4 = SPI_FPROTO(4, 4, 4), };
> 
> these protocol also includes IO's like dual and quad and IO's which are specific for
> SPI-NOR flash and not Generic SPI protocols.
> 
> > +
> > +static inline
> > +u8 spi_flash_protocol_get_inst_nbits(enum spi_flash_protocol proto) {
> > +       return ((unsigned long)(proto & SPI_FPROTO_INST_MASK)) >>
> > +               SPI_FPROTO_INST_SHIFT; }
> > +
> > +static inline
> > +u8 spi_flash_protocol_get_addr_nbits(enum spi_flash_protocol proto) {
> > +       return ((unsigned long)(proto & SPI_FPROTO_ADDR_MASK)) >>
> > +               SPI_FPROTO_ADDR_SHIFT; }
> > +
> > +static inline
> > +u8 spi_flash_protocol_get_data_nbits(enum spi_flash_protocol proto) {
> > +       return ((unsigned long)(proto & SPI_FPROTO_DATA_MASK)) >>
> > +               SPI_FPROTO_DATA_SHIFT; }
> > +
> > +/**
> > + * struct spi_flash_command - SPI flash command structure
> > + *
> > + * @instr:             Opcode sent to the SPI slave during instr clock cycles.
> > + * @mode:              Value sent to the SPI slave during mode clock cycles.
> > + * @num_mode_cycles:   Number of mode clock cycles.
> > + * @num_wait_states:   Number of wait-state clock cycles.
> > + * @addr_len:          Number of bytes sent during address clock cycles:
> > + *                     should be 0, 3, or 4.
> > + * @addr:              Value sent to the SPI slave during address clock cycles.
> > + * @data_len:          Number of bytes to be sent during data clock cycles.
> > + * @tx_data:           Data sent to the SPI slave during data clock cycles.
> > + * @rx_data:           Data read from the SPI slave during data clock cycles.
> > + */
> > +struct spi_flash_command {
> > +       enum spi_flash_protocol proto;
> > +       u8 flags;
> > +#define SPI_FCMD_TYPE          GENMASK(2, 0)
> > +#define SPI_FCMD_READ          (0x0U << 0)
> > +#define SPI_FCMD_WRITE         (0x1U << 0)
> > +#define SPI_FCMD_ERASE         (0x2U << 0)
> > +#define SPI_FCMD_READ_REG      (0x3U << 0)
> > +#define SPI_FCMD_WRITE_REG     (0x4U << 0)
> > +
> > +       u8 inst;
> > +       u8 mode;
> > +       u8 num_mode_cycles;
> > +       u8 num_wait_states;
> > +       u8 addr_len;
> > +       u32 addr;
> > +       size_t data_len;
> > +       const void *tx_data;
> > +       void *rx_data;
> > +};
> 
> I saw from the other patches, this structure is initializing for each I/O interface
> between SPI to driver/mtd like spi_transfer, if so it shouldn't have flash attributes.
> Better to move flash contents to spi_flash structure and do the respective
> operations.
> 
> thanks!
> --
> Jagan Teki
> Free Software Engineer | www.openedev.com U-Boot, Linux | Upstream
> Maintainer Hyderabad, India.