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.t...@gmail.com] > Sent: 2017年8月30日 21:50 > To: Wenyou Yang - A41535 <wenyou.y...@microchip.com> > Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Marek Vasut <ma...@denx.de>; > Cyrille Pitchen <cyrille.pitc...@atmel.com>; Jagan Teki <ja...@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.y...@microchip.com> > wrote: > > From: Cyrille Pitchen <cyrille.pitc...@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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot