| Message ID | 20200623183030.26591-1-p.yadav@ti.com |
|---|---|
| Headers | show |
| Series | mtd: spi-nor: add xSPI Octal DTR support | expand |
On 6/23/20 9:30 PM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > This table is indication that the flash is xSPI compliant and hence > supports octal DTR mode. Extract information like the fast read opcode, > dummy cycles, the number of dummy cycles needed for a Read Status > Register command, and the number of address bytes needed for a Read > Status Register command. > > We don't know what speed the controller is running at. Find the fast > read dummy cycles for the fastest frequency the flash can run at to be > sure we are never short of dummy cycles. If nothing is available, > default to 20. Flashes that use a different value should update it in > their fixup hooks. > > Since we want to set read settings, expose spi_nor_set_read_settings() > in core.h. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/mtd/spi-nor/core.c | 2 +- > drivers/mtd/spi-nor/core.h | 10 ++++ > drivers/mtd/spi-nor/sfdp.c | 98 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 109 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 22a3832b83a6..7d24e63fcca8 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -2355,7 +2355,7 @@ static int spi_nor_check(struct spi_nor *nor) > return 0; > } > > -static void > +void > spi_nor_set_read_settings(struct spi_nor_read_command *read, > u8 num_mode_clocks, > u8 num_wait_states, > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index de1e3917889f..7e6df8322da0 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -192,6 +192,9 @@ struct spi_nor_locking_ops { > * > * @size: the flash memory density in bytes. > * @page_size: the page size of the SPI NOR flash memory. > + * @rdsr_dummy: dummy cycles needed for Read Status Register command. > + * @rdsr_addr_nbytes: dummy address bytes needed for Read Status Register > + * command. > * @hwcaps: describes the read and page program hardware > * capabilities. > * @reads: read capabilities ordered by priority: the higher index > @@ -214,6 +217,8 @@ struct spi_nor_locking_ops { > struct spi_nor_flash_parameter { > u64 size; > u32 page_size; > + u8 rdsr_dummy; > + u8 rdsr_addr_nbytes; > > struct spi_nor_hwcaps hwcaps; > struct spi_nor_read_command reads[SNOR_CMD_READ_MAX]; > @@ -424,6 +429,11 @@ ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len, > > int spi_nor_hwcaps_read2cmd(u32 hwcaps); > u8 spi_nor_convert_3to4_read(u8 opcode); > +void spi_nor_set_read_settings(struct spi_nor_read_command *read, > + u8 num_mode_clocks, > + u8 num_wait_states, > + u8 opcode, > + enum spi_nor_protocol proto); > void spi_nor_set_pp_settings(struct spi_nor_pp_command *pp, u8 opcode, > enum spi_nor_protocol proto); > > diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c > index 3f709de5ea67..d5a24e61813c 100644 > --- a/drivers/mtd/spi-nor/sfdp.c > +++ b/drivers/mtd/spi-nor/sfdp.c > @@ -4,12 +4,15 @@ > * Copyright (C) 2014, Freescale Semiconductor, Inc. > */ > > +#include <linux/bitfield.h> > #include <linux/slab.h> > #include <linux/sort.h> > #include <linux/mtd/spi-nor.h> > > #include "core.h" > > +#define ROUND_UP_TO(x, y) (((x) + (y) - 1) / (y) * (y)) you can use round_up() > + > #define SFDP_PARAM_HEADER_ID(p) (((p)->id_msb << 8) | (p)->id_lsb) > #define SFDP_PARAM_HEADER_PTP(p) \ > (((p)->parameter_table_pointer[2] << 16) | \ > @@ -19,6 +22,7 @@ > #define SFDP_BFPT_ID 0xff00 /* Basic Flash Parameter Table */ > #define SFDP_SECTOR_MAP_ID 0xff81 /* Sector Map Table */ > #define SFDP_4BAIT_ID 0xff84 /* 4-byte Address Instruction Table */ > +#define SFDP_PROFILE1_ID 0xff05 /* xSPI Profile 1.0 table. */ > > #define SFDP_SIGNATURE 0x50444653U > > @@ -66,6 +70,16 @@ struct sfdp_bfpt_erase { > u32 shift; > }; > > +/* xSPI Profile 1.0 table (from JESD216D.01). */ > +#define PROFILE1_DWORD1_RD_FAST_CMD GENMASK(15, 8) > +#define PROFILE1_DWORD1_RDSR_DUMMY BIT(28) > +#define PROFILE1_DWORD1_RDSR_ADDR_BYTES BIT(29) > +#define PROFILE1_DWORD4_DUMMY_200MHZ GENMASK(11, 7) > +#define PROFILE1_DWORD5_DUMMY_166MHZ GENMASK(31, 27) > +#define PROFILE1_DWORD5_DUMMY_133MHZ GENMASK(21, 17) > +#define PROFILE1_DWORD5_DUMMY_100MHZ GENMASK(11, 7) we should order these macros in a consistent way. I see that previous macros are declared in order starting from MSB to LSB. > +#define PROFILE1_DUMMY_DEFAULT 20 we need to explain why the default dummy value is 20. How about declaring all these macros immediately above of spi_nor_parse_profile1()? > + > #define SMPT_CMD_ADDRESS_LEN_MASK GENMASK(23, 22) > #define SMPT_CMD_ADDRESS_LEN_0 (0x0UL << 22) > #define SMPT_CMD_ADDRESS_LEN_3 (0x1UL << 22) > @@ -1106,6 +1120,86 @@ static int spi_nor_parse_4bait(struct spi_nor *nor, > return ret; > } > > +/** > + * spi_nor_parse_profile1() - parse the xSPI Profile 1.0 table > + * @nor: pointer to a 'struct spi_nor' > + * @param_header: pointer to the 'struct sfdp_parameter_header' describing > + * the 4-Byte Address Instruction Table length and version. > + * @params: pointer to the 'struct spi_nor_flash_parameter' to be. > + * > + * Return: 0 on success, -errno otherwise. > + */ > +static int spi_nor_parse_profile1(struct spi_nor *nor, > + const struct sfdp_parameter_header *profile1_header, > + struct spi_nor_flash_parameter *params) > +{ > + u32 *table, opcode, addr; s/table/dwords? u8 opcode? > + size_t len; > + int ret, i; > + u8 dummy; > + > + len = profile1_header->length * sizeof(*table); > + table = kmalloc(len, GFP_KERNEL); > + if (!table) > + return -ENOMEM; > + > + addr = SFDP_PARAM_HEADER_PTP(profile1_header); > + ret = spi_nor_read_sfdp(nor, addr, len, table); > + if (ret) > + goto out; > + > + /* Fix endianness of the table DWORDs. */ > + for (i = 0; i < profile1_header->length; i++) > + table[i] = le32_to_cpu(table[i]); le32_to_cpu_array(table, profile1_header->length); > + > + /* Get 8D-8D-8D fast read opcode and dummy cycles. */ > + opcode = FIELD_GET(PROFILE1_DWORD1_RD_FAST_CMD, table[0]); > + > + /* > + * We don't know what speed the controller is running at. Find the > + * dummy cycles for the fastest frequency the flash can run at to be > + * sure we are never short of dummy cycles. A value of 0 means the > + * frequency is not supported. > + * > + * Default to PROFILE1_DUMMY_DEFAULT if we don't find anything, and let > + * flashes set the correct value if needed in their fixup hooks. > + */ > + dummy = FIELD_GET(PROFILE1_DWORD4_DUMMY_200MHZ, table[3]); > + if (!dummy) > + dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_166MHZ, table[4]); > + if (!dummy) > + dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_133MHZ, table[4]); > + if (!dummy) > + dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_100MHZ, table[4]); > + if (!dummy) > + dummy = PROFILE1_DUMMY_DEFAULT; > + > + /* Round up to an even value to avoid tripping controllers up. */ > + dummy = ROUND_UP_TO(dummy, 2); > + > + /* Update the fast read settings. */ > + spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_8_8_8_DTR], > + 0, dummy, opcode, > + SNOR_PROTO_8_8_8_DTR); > + > + /* > + * Set the Read Status Register dummy cycles and dummy address bytes. > + */ the comment can fit in a single line > + if (table[0] & PROFILE1_DWORD1_RDSR_DUMMY) I would move this above, where opcode is parsed from the same dword > + params->rdsr_dummy = 8; > + else > + params->rdsr_dummy = 4; > + > + if (table[0] & PROFILE1_DWORD1_RDSR_ADDR_BYTES) > + params->rdsr_addr_nbytes = 4; > + else > + params->rdsr_addr_nbytes = 0; > + > +out: > + kfree(table); > + return ret; > +} > + > /** > * spi_nor_parse_sfdp() - parse the Serial Flash Discoverable Parameters. > * @nor: pointer to a 'struct spi_nor' > @@ -1207,6 +1301,10 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, > err = spi_nor_parse_4bait(nor, param_header, params); > break; > > + case SFDP_PROFILE1_ID: > + err = spi_nor_parse_profile1(nor, param_header, params); > + break; > + > default: > break; > } > -- > 2.27.0 >
On 6/23/20 9:30 PM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > The xSPI Profile 1.0 table specifies how many dummy cycles and address > bytes are needed for the Read Status Register command in octal DTR mode. > Use that information to send the correct Read SR command. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/mtd/spi-nor/core.c | 25 +++++++++++++++++++++++-- > 1 file changed, 23 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 7d24e63fcca8..f2748f1d9957 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -357,6 +357,8 @@ int spi_nor_write_disable(struct spi_nor *nor) > static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr) > { > int ret; > + u8 addr_bytes = nor->params->rdsr_addr_nbytes; > + u8 dummy = nor->params->rdsr_dummy; no need to introduce local variables for a single dereference > > if (nor->spimem) { > struct spi_mem_op op = > @@ -365,10 +367,21 @@ static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr) > SPI_MEM_OP_NO_DUMMY, > SPI_MEM_OP_DATA_IN(1, sr, 1)); > > + if (spi_nor_protocol_is_dtr(nor->reg_proto)) { > + op.addr.nbytes = addr_bytes; > + op.addr.val = 0; isn't addr already initialized to 0? > + op.dummy.nbytes = dummy; > + } > + > + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); > + > ret = spi_mem_exec_op(nor->spimem, &op); > } else { > - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDSR, > - sr, 1); > + if (spi_nor_protocol_is_dtr(nor->reg_proto)) > + ret = -ENOTSUPP; > + else > + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDSR, > + sr, 1); > } doesn't this belong to a previous patch? > > if (ret) > @@ -388,6 +401,8 @@ static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr) > static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr) > { > int ret; > + u8 addr_bytes = nor->params->rdsr_addr_nbytes; > + u8 dummy = nor->params->rdsr_dummy; > > if (nor->spimem) { > struct spi_mem_op op = > @@ -396,6 +411,12 @@ static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr) > SPI_MEM_OP_NO_DUMMY, > SPI_MEM_OP_DATA_IN(1, fsr, 1)); > > + if (spi_nor_protocol_is_dtr(nor->reg_proto)) { > + op.addr.nbytes = addr_bytes; > + op.addr.val = 0; > + op.dummy.nbytes = dummy; > + } > + > spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); > > ret = spi_mem_exec_op(nor->spimem, &op); > -- > 2.27.0 >
On 6/23/20 9:30 PM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Each phase is given a separate 'dtr' field so mixed protocols like > 4S-4D-4D can be supported. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/spi/spi-mem.c | 3 +++ > include/linux/spi/spi-mem.h | 8 ++++++++ > 2 files changed, 11 insertions(+) >
On 6/23/20 9:30 PM, Pratyush Yadav wrote: > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > index 9a86cc27fcc0..93e255287ab9 100644 > --- a/drivers/spi/spi-mem.c > +++ b/drivers/spi/spi-mem.c > @@ -156,6 +156,9 @@ bool spi_mem_default_supports_op(struct spi_mem *mem, > op->data.dir == SPI_MEM_DATA_OUT)) > return false; > > + if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr) > + return false; I would put this check the first thing in the function to exit sooner and avoid the rest of the checks, that would become superfluous. Anyway this is just a nit.
On 6/23/20 9:30 PM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > In xSPI mode, flashes expect 2-byte opcodes. The second byte is called > the "command extension". There can be 3 types of extensions in xSPI: > repeat, invert, and hex. When the extension type is "repeat", the same > opcode is sent twice. When it is "invert", the second byte is the > inverse of the opcode. When it is "hex" an additional opcode byte based > is sent with the command whose value can be anything. > > So, make opcode a 16-bit value and add a 'nbytes', similar to how > multiple address widths are handled. > > Some places use sizeof(op->cmd.opcode). Replace them with op->cmd.nbytes > > The spi-mxic and spi-zynq-qspi drivers directly use op->cmd.opcode as a > buffer. Now that opcode is a 2-byte field, this can result in different > behaviour depending on if the machine is little endian or big endian. > Extract the opcode in a local 1-byte variable and use that as the buffer > instead. Both these drivers would reject multi-byte opcodes in their > supports_op() hook anyway, so we only need to worry about single-byte > opcodes for now. > > The above two changes are put in this commit to keep the series > bisectable. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/spi/spi-mem.c | 13 +++++++------ > drivers/spi/spi-mtk-nor.c | 4 ++-- > drivers/spi/spi-mxic.c | 3 ++- > drivers/spi/spi-zynq-qspi.c | 11 ++++++----- > include/linux/spi/spi-mem.h | 6 +++++- > 5 files changed, 22 insertions(+), 15 deletions(-) > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > index 93e255287ab9..ef53290b7d24 100644 > --- a/drivers/spi/spi-mem.c > +++ b/drivers/spi/spi-mem.c > @@ -159,6 +159,9 @@ bool spi_mem_default_supports_op(struct spi_mem *mem, > if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr) > return false; > > + if (op->cmd.nbytes != 1) > + return false; I would put this imediately before: if (spi_check_buswidth_req(mem, op->cmd.buswidth, true)) to speed up the exit and avoid the rest of the checks that would become superflous. > + > return true; > } > EXPORT_SYMBOL_GPL(spi_mem_default_supports_op); > @@ -173,7 +176,7 @@ static bool spi_mem_buswidth_is_valid(u8 buswidth) > > static int spi_mem_check_op(const struct spi_mem_op *op) > { > - if (!op->cmd.buswidth) > + if (!op->cmd.buswidth || !op->cmd.nbytes) we would be more explicit with: if (!op->cmd.buswidth || !op->cmd.nbytes || op->cmd.nbytes > 2) With these addressed: Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > return -EINVAL; > > if ((op->addr.nbytes && !op->addr.buswidth) || > @@ -309,8 +312,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > return ret; > } > > - tmpbufsize = sizeof(op->cmd.opcode) + op->addr.nbytes + > - op->dummy.nbytes; > + tmpbufsize = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes; > > /* > * Allocate a buffer to transmit the CMD, ADDR cycles with kmalloc() so > @@ -325,7 +327,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > > tmpbuf[0] = op->cmd.opcode; > xfers[xferpos].tx_buf = tmpbuf; > - xfers[xferpos].len = sizeof(op->cmd.opcode); > + xfers[xferpos].len = op->cmd.nbytes; > xfers[xferpos].tx_nbits = op->cmd.buswidth; > spi_message_add_tail(&xfers[xferpos], &msg); > xferpos++; > @@ -427,8 +429,7 @@ int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op) > return ctlr->mem_ops->adjust_op_size(mem, op); > > if (!ctlr->mem_ops || !ctlr->mem_ops->exec_op) { > - len = sizeof(op->cmd.opcode) + op->addr.nbytes + > - op->dummy.nbytes; > + len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes; > > if (len > spi_max_transfer_size(mem->spi)) > return -EINVAL; > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c > index 7bc302b50396..d5f393871619 100644 > --- a/drivers/spi/spi-mtk-nor.c > +++ b/drivers/spi/spi-mtk-nor.c > @@ -195,7 +195,7 @@ static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op) > } > } > > - len = MTK_NOR_PRG_MAX_SIZE - sizeof(op->cmd.opcode) - op->addr.nbytes - > + len = MTK_NOR_PRG_MAX_SIZE - op->cmd.nbytes - op->addr.nbytes - > op->dummy.nbytes; > if (op->data.nbytes > len) > op->data.nbytes = len; > @@ -219,7 +219,7 @@ static bool mtk_nor_supports_op(struct spi_mem *mem, > (op->dummy.buswidth == 0) && > (op->data.buswidth == 1); > } > - len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes; > + len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes; > if ((len > MTK_NOR_PRG_MAX_SIZE) || > ((op->data.nbytes) && (len == MTK_NOR_PRG_MAX_SIZE))) > return false; > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c > index 69491f3a515d..8c630acb0110 100644 > --- a/drivers/spi/spi-mxic.c > +++ b/drivers/spi/spi-mxic.c > @@ -356,6 +356,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem, > int nio = 1, i, ret; > u32 ss_ctrl; > u8 addr[8]; > + u8 opcode = op->cmd.opcode; > > ret = mxic_spi_set_freq(mxic, mem->spi->max_speed_hz); > if (ret) > @@ -393,7 +394,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem, > writel(readl(mxic->regs + HC_CFG) | HC_CFG_MAN_CS_ASSERT, > mxic->regs + HC_CFG); > > - ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, 1); > + ret = mxic_spi_data_xfer(mxic, &opcode, NULL, 1); > if (ret) > goto out; > > diff --git a/drivers/spi/spi-zynq-qspi.c b/drivers/spi/spi-zynq-qspi.c > index 17641157354d..bbf3d90561f5 100644 > --- a/drivers/spi/spi-zynq-qspi.c > +++ b/drivers/spi/spi-zynq-qspi.c > @@ -527,20 +527,21 @@ static int zynq_qspi_exec_mem_op(struct spi_mem *mem, > struct zynq_qspi *xqspi = spi_controller_get_devdata(mem->spi->master); > int err = 0, i; > u8 *tmpbuf; > + u8 opcode = op->cmd.opcode; > > dev_dbg(xqspi->dev, "cmd:%#x mode:%d.%d.%d.%d\n", > - op->cmd.opcode, op->cmd.buswidth, op->addr.buswidth, > + opcode, op->cmd.buswidth, op->addr.buswidth, > op->dummy.buswidth, op->data.buswidth); > > zynq_qspi_chipselect(mem->spi, true); > zynq_qspi_config_op(xqspi, mem->spi); > > - if (op->cmd.opcode) { > + if (op->cmd.nbytes) { > reinit_completion(&xqspi->data_completion); > - xqspi->txbuf = (u8 *)&op->cmd.opcode; > + xqspi->txbuf = &opcode; > xqspi->rxbuf = NULL; > - xqspi->tx_bytes = sizeof(op->cmd.opcode); > - xqspi->rx_bytes = sizeof(op->cmd.opcode); > + xqspi->tx_bytes = op->cmd.nbytes; > + xqspi->rx_bytes = op->cmd.nbytes; > zynq_qspi_write_op(xqspi, ZYNQ_QSPI_FIFO_DEPTH, true); > zynq_qspi_write(xqspi, ZYNQ_QSPI_IEN_OFFSET, > ZYNQ_QSPI_IXR_RXTX_MASK); > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h > index e3dcb956bf61..159463cc659c 100644 > --- a/include/linux/spi/spi-mem.h > +++ b/include/linux/spi/spi-mem.h > @@ -17,6 +17,7 @@ > { \ > .buswidth = __buswidth, \ > .opcode = __opcode, \ > + .nbytes = 1, \ > } > > #define SPI_MEM_OP_ADDR(__nbytes, __val, __buswidth) \ > @@ -69,6 +70,8 @@ enum spi_mem_data_dir { > > /** > * struct spi_mem_op - describes a SPI memory operation > + * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is > + * sent MSB-first. > * @cmd.buswidth: number of IO lines used to transmit the command > * @cmd.opcode: operation opcode > * @cmd.dtr: whether the command opcode should be sent in DTR mode or not > @@ -94,9 +97,10 @@ enum spi_mem_data_dir { > */ > struct spi_mem_op { > struct { > + u8 nbytes; > u8 buswidth; > u8 dtr : 1; > - u8 opcode; > + u16 opcode; > } cmd; > > struct { > -- > 2.27.0 >
On 6/23/20 9:30 PM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Double Transfer Rate (DTR) ops are added in spi-mem. But this controller > doesn't support DTR transactions. Since we don't use the default > supports_op(), which rejects all DTR ops, do that explicitly in our > supports_op(). > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/spi/spi-mtk-nor.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c > index d5f393871619..b08d8e9a8ee9 100644 > --- a/drivers/spi/spi-mtk-nor.c > +++ b/drivers/spi/spi-mtk-nor.c > @@ -211,6 +211,12 @@ static bool mtk_nor_supports_op(struct spi_mem *mem, > if (op->cmd.buswidth != 1) > return false; > > + /* DTR ops not supported. */ > + if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr) > + return false; > + if (op->cmd.nbytes != 1) > + return false; > + > if ((op->addr.nbytes == 3) || (op->addr.nbytes == 4)) { > if ((op->data.dir == SPI_MEM_DATA_IN) && mtk_nor_match_read(op)) > return true; > -- > 2.27.0 >
Hi, Mark, On 6/23/20 9:30 PM, Pratyush Yadav wrote: > Pratyush Yadav (17): > spi: spi-mem: allow specifying whether an op is DTR or not > spi: spi-mem: allow specifying a command's extension > spi: atmel-quadspi: reject DTR ops > spi: spi-mtk-nor: reject DTR ops These four patches are looking good, I had just few minor comments. If you too think that they are ok, would you take them through the SPI tree? If so, I would need an immutable tag on top of v5.8-rc1 preferably, so I can merge them back to SPI NOR and continue the development on top of them. Let us know if you want us to resubmit for those minor comments. Cheers, ta
On 6/23/20 9:30 PM, Pratyush Yadav wrote: > The Micron MT35XU512ABA flash does not support the quad enable bit. But > instead of programming the Quad Enable Require field to 000b ("Device > does not have a QE bit"), it is programmed to 111b ("Reserved"). > > While this is technically incorrect, it is not reason enough to abort > BFPT parsing. Instead, continue BFPT parsing and let flashes set it in > their fixup hooks. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/mtd/spi-nor/sfdp.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Applied, thanks!
On Wed, 24 Jun 2020 00:00:13 +0530, Pratyush Yadav wrote: > This series adds support for octal DTR flashes in the spi-nor framework, > and then adds hooks for the Cypress Semper and Mircom Xcella flashes to > allow running them in octal DTR mode. This series assumes that the flash > is handed to the kernel in Legacy SPI mode. > > Tested on TI J721e EVM with 1-bit ECC on the Cypress flash. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/4] spi: spi-mem: allow specifying whether an op is DTR or not commit: 4c5e2bba30e49b970a0fd07b43e0b7a3b5fd5ea7 [2/4] spi: spi-mem: allow specifying a command's extension commit: caf72df48be32c39f74287976ae843501ae06949 [3/4] spi: atmel-quadspi: reject DTR ops commit: 5c81c275582c9d9c66d2f928591a2065f2528231 [4/4] spi: spi-mtk-nor: reject DTR ops commit: 4728f073bfc66b8b555274ef0d7741d7f5a48947 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On Mon, Jul 13, 2020 at 06:34:12AM +0000, Tudor.Ambarus@microchip.com wrote: > These four patches are looking good, I had just few minor comments. > If you too think that they are ok, would you take them through the > SPI tree? If so, I would need an immutable tag on top of v5.8-rc1 > preferably, so I can merge them back to SPI NOR and continue the > development on top of them. The following changes since commit b3a9e3b9622ae10064826dccb4f7a52bd88c7407: Linux 5.8-rc1 (2020-06-14 12:45:04 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git tags/spi-mem-dtr for you to fetch changes up to 4728f073bfc66b8b555274ef0d7741d7f5a48947: spi: spi-mtk-nor: reject DTR ops (2020-07-14 17:29:40 +0100) ---------------------------------------------------------------- spi: Support for DTR ops ---------------------------------------------------------------- Pratyush Yadav (4): spi: spi-mem: allow specifying whether an op is DTR or not spi: spi-mem: allow specifying a command's extension spi: atmel-quadspi: reject DTR ops spi: spi-mtk-nor: reject DTR ops drivers/spi/atmel-quadspi.c | 6 ++++++ drivers/spi/spi-mem.c | 16 ++++++++++------ drivers/spi/spi-mtk-nor.c | 10 ++++++++-- drivers/spi/spi-mxic.c | 3 ++- drivers/spi/spi-zynq-qspi.c | 11 ++++++----- include/linux/spi/spi-mem.h | 14 +++++++++++++- 6 files changed, 45 insertions(+), 15 deletions(-)
On 7/14/20 10:19 PM, Mark Brown wrote: > On Mon, Jul 13, 2020 at 06:34:12AM +0000, Tudor.Ambarus@microchip.com wrote: > >> These four patches are looking good, I had just few minor comments. >> If you too think that they are ok, would you take them through the >> SPI tree? If so, I would need an immutable tag on top of v5.8-rc1 >> preferably, so I can merge them back to SPI NOR and continue the >> development on top of them. > > The following changes since commit b3a9e3b9622ae10064826dccb4f7a52bd88c7407: > > Linux 5.8-rc1 (2020-06-14 12:45:04 -0700) > > are available in the Git repository at: > > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git tags/spi-mem-dtr > > for you to fetch changes up to 4728f073bfc66b8b555274ef0d7741d7f5a48947: > > spi: spi-mtk-nor: reject DTR ops (2020-07-14 17:29:40 +0100) > > ---------------------------------------------------------------- > spi: Support for DTR ops > > ---------------------------------------------------------------- > Pratyush Yadav (4): > spi: spi-mem: allow specifying whether an op is DTR or not > spi: spi-mem: allow specifying a command's extension > spi: atmel-quadspi: reject DTR ops > spi: spi-mtk-nor: reject DTR ops > > drivers/spi/atmel-quadspi.c | 6 ++++++ > drivers/spi/spi-mem.c | 16 ++++++++++------ > drivers/spi/spi-mtk-nor.c | 10 ++++++++-- > drivers/spi/spi-mxic.c | 3 ++- > drivers/spi/spi-zynq-qspi.c | 11 ++++++----- > include/linux/spi/spi-mem.h | 14 +++++++++++++- > 6 files changed, 45 insertions(+), 15 deletions(-) > Merged into spi-nor/next. Thank you!
Hi Tudor, On 08/07/20 04:03PM, Tudor.Ambarus@microchip.com wrote: > On 6/23/20 9:30 PM, Pratyush Yadav wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > The xSPI Profile 1.0 table specifies how many dummy cycles and address > > bytes are needed for the Read Status Register command in octal DTR mode. > > Use that information to send the correct Read SR command. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > drivers/mtd/spi-nor/core.c | 25 +++++++++++++++++++++++-- > > 1 file changed, 23 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > > index 7d24e63fcca8..f2748f1d9957 100644 > > --- a/drivers/mtd/spi-nor/core.c > > +++ b/drivers/mtd/spi-nor/core.c > > @@ -357,6 +357,8 @@ int spi_nor_write_disable(struct spi_nor *nor) > > static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr) > > { > > int ret; > > + u8 addr_bytes = nor->params->rdsr_addr_nbytes; > > + u8 dummy = nor->params->rdsr_dummy; > > no need to introduce local variables for a single dereference Ok. > > > > if (nor->spimem) { > > struct spi_mem_op op = > > @@ -365,10 +367,21 @@ static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr) > > SPI_MEM_OP_NO_DUMMY, > > SPI_MEM_OP_DATA_IN(1, sr, 1)); > > > > + if (spi_nor_protocol_is_dtr(nor->reg_proto)) { > > + op.addr.nbytes = addr_bytes; > > + op.addr.val = 0; > > isn't addr already initialized to 0? Yes, it is. But I figured it won't hurt to be explicit about what we intend the address to be. > > + op.dummy.nbytes = dummy; > > + } > > + > > + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); > > + > > ret = spi_mem_exec_op(nor->spimem, &op); > > } else { > > - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDSR, > > - sr, 1); > > + if (spi_nor_protocol_is_dtr(nor->reg_proto)) > > + ret = -ENOTSUPP; > > + else > > + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDSR, > > + sr, 1); > > } > > doesn't this belong to a previous patch? It does. Will fix. > > > > if (ret) > > @@ -388,6 +401,8 @@ static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr) > > static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr) > > { > > int ret; > > + u8 addr_bytes = nor->params->rdsr_addr_nbytes; > > + u8 dummy = nor->params->rdsr_dummy; > > > > if (nor->spimem) { > > struct spi_mem_op op = > > @@ -396,6 +411,12 @@ static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr) > > SPI_MEM_OP_NO_DUMMY, > > SPI_MEM_OP_DATA_IN(1, fsr, 1)); > > > > + if (spi_nor_protocol_is_dtr(nor->reg_proto)) { > > + op.addr.nbytes = addr_bytes; > > + op.addr.val = 0; > > + op.dummy.nbytes = dummy; > > + } > > + > > spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); > > > > ret = spi_mem_exec_op(nor->spimem, &op); -- Regards, Pratyush Yadav
Hi Tudor, On 08/07/20 04:01PM, Tudor.Ambarus@microchip.com wrote: > On 6/23/20 9:30 PM, Pratyush Yadav wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > This table is indication that the flash is xSPI compliant and hence > > supports octal DTR mode. Extract information like the fast read opcode, > > dummy cycles, the number of dummy cycles needed for a Read Status > > Register command, and the number of address bytes needed for a Read > > Status Register command. > > > > We don't know what speed the controller is running at. Find the fast > > read dummy cycles for the fastest frequency the flash can run at to be > > sure we are never short of dummy cycles. If nothing is available, > > default to 20. Flashes that use a different value should update it in > > their fixup hooks. > > > > Since we want to set read settings, expose spi_nor_set_read_settings() > > in core.h. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > drivers/mtd/spi-nor/core.c | 2 +- > > drivers/mtd/spi-nor/core.h | 10 ++++ > > drivers/mtd/spi-nor/sfdp.c | 98 ++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 109 insertions(+), 1 deletion(-) > > [...] > > diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c > > index 3f709de5ea67..d5a24e61813c 100644 > > --- a/drivers/mtd/spi-nor/sfdp.c > > +++ b/drivers/mtd/spi-nor/sfdp.c [...] > > @@ -66,6 +70,16 @@ struct sfdp_bfpt_erase { > > u32 shift; > > }; > > > > +/* xSPI Profile 1.0 table (from JESD216D.01). */ > > +#define PROFILE1_DWORD1_RD_FAST_CMD GENMASK(15, 8) > > +#define PROFILE1_DWORD1_RDSR_DUMMY BIT(28) > > +#define PROFILE1_DWORD1_RDSR_ADDR_BYTES BIT(29) > > +#define PROFILE1_DWORD4_DUMMY_200MHZ GENMASK(11, 7) > > +#define PROFILE1_DWORD5_DUMMY_166MHZ GENMASK(31, 27) > > +#define PROFILE1_DWORD5_DUMMY_133MHZ GENMASK(21, 17) > > +#define PROFILE1_DWORD5_DUMMY_100MHZ GENMASK(11, 7) > > we should order these macros in a consistent way. I see that previous macros > are declared in order starting from MSB to LSB. > > > +#define PROFILE1_DUMMY_DEFAULT 20 > > we need to explain why the default dummy value is 20. No reason other than the fact that it is the default for the first flash that uses Profile 1.0 parsing (S28HS512T). AFAIK a similar reasoning is followed for the default being 8 for 1-1-4 or 1-1-8 modes. I can't think of any reasonable way of deciding on a default value since it varies from flash to flash. > How about declaring all these macros immediately above of spi_nor_parse_profile1()? > > > + > > #define SMPT_CMD_ADDRESS_LEN_MASK GENMASK(23, 22) > > #define SMPT_CMD_ADDRESS_LEN_0 (0x0UL << 22) > > #define SMPT_CMD_ADDRESS_LEN_3 (0x1UL << 22) > > @@ -1106,6 +1120,86 @@ static int spi_nor_parse_4bait(struct spi_nor *nor, > > return ret; > > } > > > > +/** > > + * spi_nor_parse_profile1() - parse the xSPI Profile 1.0 table > > + * @nor: pointer to a 'struct spi_nor' > > + * @param_header: pointer to the 'struct sfdp_parameter_header' describing > > + * the 4-Byte Address Instruction Table length and version. > > + * @params: pointer to the 'struct spi_nor_flash_parameter' to be. > > + * > > + * Return: 0 on success, -errno otherwise. > > + */ > > +static int spi_nor_parse_profile1(struct spi_nor *nor, > > + const struct sfdp_parameter_header *profile1_header, > > + struct spi_nor_flash_parameter *params) > > +{ > > + u32 *table, opcode, addr; > > s/table/dwords? > > u8 opcode? > > > + size_t len; > > + int ret, i; > > + u8 dummy; > > + > > + len = profile1_header->length * sizeof(*table); > > + table = kmalloc(len, GFP_KERNEL); > > + if (!table) > > + return -ENOMEM; > > + > > + addr = SFDP_PARAM_HEADER_PTP(profile1_header); > > + ret = spi_nor_read_sfdp(nor, addr, len, table); > > + if (ret) > > + goto out; > > + > > + /* Fix endianness of the table DWORDs. */ > > + for (i = 0; i < profile1_header->length; i++) > > + table[i] = le32_to_cpu(table[i]); > > le32_to_cpu_array(table, profile1_header->length); > > > + > > + /* Get 8D-8D-8D fast read opcode and dummy cycles. */ > > + opcode = FIELD_GET(PROFILE1_DWORD1_RD_FAST_CMD, table[0]); > > + > > + /* > > + * We don't know what speed the controller is running at. Find the > > + * dummy cycles for the fastest frequency the flash can run at to be > > + * sure we are never short of dummy cycles. A value of 0 means the > > + * frequency is not supported. > > + * > > + * Default to PROFILE1_DUMMY_DEFAULT if we don't find anything, and let > > + * flashes set the correct value if needed in their fixup hooks. > > + */ > > + dummy = FIELD_GET(PROFILE1_DWORD4_DUMMY_200MHZ, table[3]); > > + if (!dummy) > > + dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_166MHZ, table[4]); > > + if (!dummy) > > + dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_133MHZ, table[4]); > > + if (!dummy) > > + dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_100MHZ, table[4]); > > + if (!dummy) > > + dummy = PROFILE1_DUMMY_DEFAULT; > > + > > + /* Round up to an even value to avoid tripping controllers up. */ > > + dummy = ROUND_UP_TO(dummy, 2); > > + [...] -- Regards, Pratyush Yadav