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@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.