| Message ID | 20200226093703.19765-1-p.yadav@ti.com |
|---|---|
| Headers | show |
| Series | mtd: spi-nor: add xSPI Octal DTR support | expand |
On Wed, 26 Feb 2020 15:06:54 +0530 Pratyush Yadav <p.yadav@ti.com> wrote: > These two DT properties express DTR receive and transmit capabilities of > a SPI flash and controller. Introduce two new mode bits: SPI_RX_DTR and > SPI_TX_DTR which correspond to the new DT properties. Set these bits > when the two corresponding properties are present in the device tree. > Also update the detection of unsupported mode bits to include the new > bits. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/spi/spi.c | 10 +++++++++- > include/linux/spi/spi.h | 2 ++ > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 38b4c78df506..25c8ed9343f9 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -1927,6 +1927,13 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, > } > } > > + /* Device DTR mode. */ > + if (of_property_read_bool(nc, "spi-tx-dtr")) > + spi->mode |= SPI_TX_DTR; > + > + if (of_property_read_bool(nc, "spi-rx-dtr")) > + spi->mode |= SPI_RX_DTR; > + If this DTR mode is only used in spi-mem, maybe we shouldn't add those flags. SPI mem devices are usually smart enough to advertise what they support, and the subsystem in charge of those devices (in this specific case, spi-nor) will check what the controller supports using spi_mem_supports_op(). The only case we might have to deal with at some point is board level limitations (disabling DTR because the routing prevents using this mode). > if (spi_controller_is_slave(ctlr)) { > if (!of_node_name_eq(nc, "slave")) { > dev_err(&ctlr->dev, "%pOF is not called 'slave'\n", > @@ -3252,7 +3259,8 @@ int spi_setup(struct spi_device *spi) > bad_bits &= ~SPI_CS_HIGH; > ugly_bits = bad_bits & > (SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL | > - SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL); > + SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL | > + SPI_TX_DTR | SPI_RX_DTR); > if (ugly_bits) { > dev_warn(&spi->dev, > "setup: ignoring unsupported mode bits %x\n", > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index 6d16ba01ff5a..bf1108318389 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -183,6 +183,8 @@ struct spi_device { > #define SPI_TX_OCTAL 0x2000 /* transmit with 8 wires */ > #define SPI_RX_OCTAL 0x4000 /* receive with 8 wires */ > #define SPI_3WIRE_HIZ 0x8000 /* high impedance turnaround */ > +#define SPI_RX_DTR 0x10000 /* receive in DTR mode */ > +#define SPI_TX_DTR 0x20000 /* transmit in DTR mode */ > int irq; > void *controller_state; > void *controller_data;
On Wed, 26 Feb 2020 15:06:55 +0530 Pratyush Yadav <p.yadav@ti.com> wrote: > Each phase is given a separate 'is_dtr' field so mixed protocols like > 4S-4D-4D can be supported. > > Also add the mode bits SPI_RX_DTR and SPI_TX_DTR so controllers can > specify whether they support DTR modes or not. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/spi/spi-mem.c | 23 +++++++++++++++++++++++ > include/linux/spi/spi-mem.h | 8 ++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > index e5a46f0eb93b..cb13e0878b95 100644 > --- a/drivers/spi/spi-mem.c > +++ b/drivers/spi/spi-mem.c > @@ -99,6 +99,16 @@ void spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr, > } > EXPORT_SYMBOL_GPL(spi_controller_dma_unmap_mem_op_data); > > +static int spi_check_dtr_req(struct spi_mem *mem, bool tx) > +{ > + u32 mode = mem->spi->mode; > + > + if ((tx && (mode & SPI_TX_DTR)) || (!tx && (mode & SPI_RX_DTR))) > + return 0; > + > + return -ENOTSUPP; > +} > + > static int spi_check_buswidth_req(struct spi_mem *mem, u8 buswidth, bool tx) > { > u32 mode = mem->spi->mode; > @@ -154,6 +164,19 @@ bool spi_mem_default_supports_op(struct spi_mem *mem, > op->data.dir == SPI_MEM_DATA_OUT)) > return false; > > + if (op->cmd.is_dtr && spi_check_dtr_req(mem, true)) > + return false; > + > + if (op->addr.is_dtr && spi_check_dtr_req(mem, true)) > + return false; > + > + if (op->dummy.is_dtr && spi_check_dtr_req(mem, true)) > + return false; > + > + if (op->data.dir != SPI_MEM_NO_DATA && op->data.is_dtr && > + spi_check_dtr_req(mem, op->data.dir == SPI_MEM_DATA_OUT)) > + return false; > + Not all controllers use spi_mem_default_supports_op(). Those should be patched to reject DTR ops too. > return true; > } > EXPORT_SYMBOL_GPL(spi_mem_default_supports_op); > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h > index af9ff2f0f1b2..4669082b4e3b 100644 > --- a/include/linux/spi/spi-mem.h > +++ b/include/linux/spi/spi-mem.h > @@ -71,6 +71,7 @@ enum spi_mem_data_dir { > * struct spi_mem_op - describes a SPI memory operation > * @cmd.buswidth: number of IO lines used to transmit the command > * @cmd.opcode: operation opcode > + * @cmd.is_dtr: whether the command opcode should be sent in DTR mode or not > * @addr.nbytes: number of address bytes to send. Can be zero if the operation > * does not need to send an address > * @addr.buswidth: number of IO lines used to transmit the address cycles > @@ -78,10 +79,13 @@ enum spi_mem_data_dir { > * Note that only @addr.nbytes are taken into account in this > * address value, so users should make sure the value fits in the > * assigned number of bytes. > + * @addr.is_dtr: whether the address should be sent in DTR mode or not > * @dummy.nbytes: number of dummy bytes to send after an opcode or address. Can > * be zero if the operation does not require dummy bytes > * @dummy.buswidth: number of IO lanes used to transmit the dummy bytes > + * @dummy.is_dtr: whether the dummy bytes should be sent in DTR mode or not > * @data.buswidth: number of IO lanes used to send/receive the data > + * @data.is_dtr: whether the data should be sent in DTR mode or not > * @data.dir: direction of the transfer > * @data.nbytes: number of data bytes to send/receive. Can be zero if the > * operation does not involve transferring data > @@ -92,21 +96,25 @@ struct spi_mem_op { > struct { > u8 buswidth; > u8 opcode; > + bool is_dtr; Hm, maybe use a bitfield here so we can pack other fields if needed. Also not convince the 'is_' prefix is useful. u8 dtr : 1; > } cmd; > > struct { > u8 nbytes; > u8 buswidth; Maybe move the dtr field here so the compiler can pack things instead of adding extra padding for the u64 alignment. u8 dtr : 1; > u64 val; > + bool is_dtr; > } addr; > > struct { > u8 nbytes; > u8 buswidth; > + bool is_dtr; > } dummy; > > struct { > u8 buswidth; > + bool is_dtr; > enum spi_mem_data_dir dir; > unsigned int nbytes; > union {
On Wed, 26 Feb 2020 15:06:56 +0530 Pratyush Yadav <p.yadav@ti.com> wrote: > 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. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/spi/spi-mem.c | 23 +++++++++++++++++++++++ > include/linux/spi/spi-mem.h | 24 ++++++++++++++++++++++++ > 2 files changed, 47 insertions(+) > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > index cb13e0878b95..3838ddc9aeec 100644 > --- a/drivers/spi/spi-mem.c > +++ b/drivers/spi/spi-mem.c > @@ -462,6 +462,29 @@ int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op) > } > EXPORT_SYMBOL_GPL(spi_mem_adjust_op_size); > > +int spi_mem_get_cmd_ext(const struct spi_mem_op *op, u8 *ext) > +{ > + switch (op->cmd.ext_type) { > + case SPI_MEM_EXT_INVERT: > + *ext = ~op->cmd.opcode; > + break; > + > + case SPI_MEM_EXT_REPEAT: > + *ext = op->cmd.opcode; > + break; > + > + case SPI_MEM_EXT_HEX: > + *ext = op->cmd.ext; > + break; > + > + default: > + return -EINVAL; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(spi_mem_get_cmd_ext); > + > static ssize_t spi_mem_no_dirmap_read(struct spi_mem_dirmap_desc *desc, > u64 offs, size_t len, void *buf) > { > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h > index 4669082b4e3b..06ccab17e4d0 100644 > --- a/include/linux/spi/spi-mem.h > +++ b/include/linux/spi/spi-mem.h > @@ -67,11 +67,31 @@ enum spi_mem_data_dir { > SPI_MEM_DATA_OUT, > }; > > +/** > + * enum spi_mem_cmd_ext - describes the command opcode extension in DTR mode > + * @SPI_MEM_EXT_NONE: no extension. This is the default, and is used in Legacy > + * SPI mode > + * @SPI_MEM_EXT_REPEAT: the extension is same as the opcode > + * @SPI_MEM_EXT_INVERT: the extension is the bitwise inverse of the opcode > + * @SPI_MEM_EXT_HEX: the extension is any hex value. The command and opcode > + * combine to form a 16-bit opcode. > + */ > +enum spi_mem_cmd_ext { > + SPI_MEM_EXT_NONE = 0, > + SPI_MEM_EXT_REPEAT, > + SPI_MEM_EXT_INVERT, > + SPI_MEM_EXT_HEX, > +}; > + > /** > * struct spi_mem_op - describes a SPI memory operation > * @cmd.buswidth: number of IO lines used to transmit the command > * @cmd.opcode: operation opcode > * @cmd.is_dtr: whether the command opcode should be sent in DTR mode or not > + * @cmd.ext_type: type of the command opcode extension in DTR mode > + * @cmd.ext: value of the command opcode extension in DTR mode. It is > + * only set when 'ext_type' is 'SPI_MEM_EXT_HEX'. In all other > + * cases, the extension can be directly derived from the opcode. > * @addr.nbytes: number of address bytes to send. Can be zero if the operation > * does not need to send an address > * @addr.buswidth: number of IO lines used to transmit the address cycles > @@ -97,6 +117,8 @@ struct spi_mem_op { > u8 buswidth; > u8 opcode; > bool is_dtr; > + enum spi_mem_cmd_ext ext_type; > + u8 ext; Could we instead make opcode an u16 (or u8[2]) and pass the number of bytes, as done for the other addr? Mode can be extracted from the opcode/nbytes values if really needed, and the caller would be responsible for filling those fields properly (which shouldn't be too hard) > } cmd; > > struct { > @@ -361,6 +383,8 @@ int spi_mem_driver_register_with_owner(struct spi_mem_driver *drv, > > void spi_mem_driver_unregister(struct spi_mem_driver *drv); > > +int spi_mem_get_cmd_ext(const struct spi_mem_op *op, u8 *ext); > + > #define spi_mem_driver_register(__drv) \ > spi_mem_driver_register_with_owner(__drv, THIS_MODULE) >
On Wed, 26 Feb 2020 15:06:58 +0530 Pratyush Yadav <p.yadav@ti.com> wrote: > Double Transfer Rate (DTR) is SPI protocol in which data is transferred > on each clock edge as opposed to on each clock cycle. Make > framework-level changes to allow supporting flashes in DTR mode. > > Right now, mixed DTR modes are not supported. So, for example a mode > like 4S-4D-4D will not work. All phases need to be either DTR or STR. Didn't go deep into the patch but at first glance you don't seem to extend the framework to support stateful modes as I tried to do here [1]. That's really something we should address before considering supporting xD-xD-xD modes, unless the SPI-NOR only supports one stateful mode. If we don't do that first, we might face all sort of unpleasant issues: * kexec not working correctly because the previous kernel left the NOR in an unknown state * suspend/resume not working properly * linux not booting properly because the bootloader left the device in its non-default mode * ... [1]https://patchwork.kernel.org/cover/10638055/
Hi Boris, On 27/02/20 05:58PM, Boris Brezillon wrote: > On Wed, 26 Feb 2020 15:06:58 +0530 > Pratyush Yadav <p.yadav@ti.com> wrote: > > > Double Transfer Rate (DTR) is SPI protocol in which data is transferred > > on each clock edge as opposed to on each clock cycle. Make > > framework-level changes to allow supporting flashes in DTR mode. > > > > Right now, mixed DTR modes are not supported. So, for example a mode > > like 4S-4D-4D will not work. All phases need to be either DTR or STR. > > Didn't go deep into the patch but at first glance you don't seem to > extend the framework to support stateful modes as I tried to do here > [1]. That's really something we should address before considering > supporting xD-xD-xD modes, unless the SPI-NOR only supports one > stateful mode. If we don't do that first, we might face all sort of > unpleasant issues: > > * kexec not working correctly because the previous kernel left the NOR > in an unknown state > * suspend/resume not working properly > * linux not booting properly because the bootloader left the device in > its non-default mode > * ... Correct. I am working on a follow-up series that takes care of these problems. The series will allow spi-nor to detect what mode the flash is in and then run the SFPD procedure in that mode (or maybe switch to single SPI mode and then go about its business as usual? I haven't figured out all the details yet). So for the context of this series, assume we are handed the flash in single SPI mode. > [1]https://patchwork.kernel.org/cover/10638055/ BTW, I took a quick look at this series but I don't see any code that tries to detect which mode the flash is in (which is the troublesome part [0]). So, for example, if the bootloader leaves the flash in 8D-8D-8D mode, how would your series handle that situation? [0] There are multiple problems to take care of when trying to detect which mode a flash is in. We can try reading SFDP in each mode and whichever mode gives us the correct "SFDP" signature is the mode the flash is in. But the problem is that even in xSPI standard Read SFDP command is optional in 8D-8D-8D mode, let alone non-xSPI flashes. Another problem is that the address bytes and dummy cycles for Read SFDP are not the same for every flash. The xSPI standard says address bytes can be 3/4 and dummy cycles can be 8/20. So, for example, Cypress s28hs/s28ht family and Micron Xccela (mt35x) family use 4 address bytes, but the Adesto ATXP032/ATXP032R flashes use 3 address bytes. Say that a flash supports Read SFDP in 8D-8D-8D mode and we try all the combinations to find out which mode the flash is in, we now have the problem of actually identifying the flash. Unfortunately, the Read ID command is not uniform across flash vendors. The Micron Xccela flashes use 8 dummy cycles and no address bytes for Read ID. The Cypress s28hs/t family uses configurable dummy cycles (defaulting to 3) and needs 4 dummy address bytes all of which are 0. If we can't find out which flash it is, we can't run its fixup hooks, and might end up running it with incorrect settings. And all this is assuming a flash even has SFDP and has it available in all modes. So, the only solution I can now think of is having the flash name in its compatible string in the device tree. This way we can skip all the Read ID ugliness and can have flash-specific hooks to make it easier to detect the mode it is in (though I wonder if it is even possible to detect the mode in a flash that doesn't have SFDP in 8D-8D-8D). Thoughts? Is there a better way to solve this problem that I didn't think of?
On 27/02/20 05:44PM, Boris Brezillon wrote: > On Wed, 26 Feb 2020 15:06:56 +0530 > Pratyush Yadav <p.yadav@ti.com> wrote: > > > 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. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > drivers/spi/spi-mem.c | 23 +++++++++++++++++++++++ > > include/linux/spi/spi-mem.h | 24 ++++++++++++++++++++++++ > > 2 files changed, 47 insertions(+) > > > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > > index cb13e0878b95..3838ddc9aeec 100644 > > --- a/drivers/spi/spi-mem.c > > +++ b/drivers/spi/spi-mem.c > > @@ -462,6 +462,29 @@ int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op) > > } > > EXPORT_SYMBOL_GPL(spi_mem_adjust_op_size); > > > > +int spi_mem_get_cmd_ext(const struct spi_mem_op *op, u8 *ext) > > +{ > > + switch (op->cmd.ext_type) { > > + case SPI_MEM_EXT_INVERT: > > + *ext = ~op->cmd.opcode; > > + break; > > + > > + case SPI_MEM_EXT_REPEAT: > > + *ext = op->cmd.opcode; > > + break; > > + > > + case SPI_MEM_EXT_HEX: > > + *ext = op->cmd.ext; > > + break; > > + > > + default: > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(spi_mem_get_cmd_ext); > > + > > static ssize_t spi_mem_no_dirmap_read(struct spi_mem_dirmap_desc *desc, > > u64 offs, size_t len, void *buf) > > { > > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h > > index 4669082b4e3b..06ccab17e4d0 100644 > > --- a/include/linux/spi/spi-mem.h > > +++ b/include/linux/spi/spi-mem.h > > @@ -67,11 +67,31 @@ enum spi_mem_data_dir { > > SPI_MEM_DATA_OUT, > > }; > > > > +/** > > + * enum spi_mem_cmd_ext - describes the command opcode extension in DTR mode > > + * @SPI_MEM_EXT_NONE: no extension. This is the default, and is used in Legacy > > + * SPI mode > > + * @SPI_MEM_EXT_REPEAT: the extension is same as the opcode > > + * @SPI_MEM_EXT_INVERT: the extension is the bitwise inverse of the opcode > > + * @SPI_MEM_EXT_HEX: the extension is any hex value. The command and opcode > > + * combine to form a 16-bit opcode. > > + */ > > +enum spi_mem_cmd_ext { > > + SPI_MEM_EXT_NONE = 0, > > + SPI_MEM_EXT_REPEAT, > > + SPI_MEM_EXT_INVERT, > > + SPI_MEM_EXT_HEX, > > +}; > > + > > /** > > * struct spi_mem_op - describes a SPI memory operation > > * @cmd.buswidth: number of IO lines used to transmit the command > > * @cmd.opcode: operation opcode > > * @cmd.is_dtr: whether the command opcode should be sent in DTR mode or not > > + * @cmd.ext_type: type of the command opcode extension in DTR mode > > + * @cmd.ext: value of the command opcode extension in DTR mode. It is > > + * only set when 'ext_type' is 'SPI_MEM_EXT_HEX'. In all other > > + * cases, the extension can be directly derived from the opcode. > > * @addr.nbytes: number of address bytes to send. Can be zero if the operation > > * does not need to send an address > > * @addr.buswidth: number of IO lines used to transmit the address cycles > > @@ -97,6 +117,8 @@ struct spi_mem_op { > > u8 buswidth; > > u8 opcode; > > bool is_dtr; > > + enum spi_mem_cmd_ext ext_type; > > + u8 ext; > > Could we instead make opcode an u16 (or u8[2]) and pass the number of > bytes, as done for the other addr? Mode can be extracted from the > opcode/nbytes values if really needed, and the caller would be > responsible for filling those fields properly (which shouldn't be too > hard) Ok. Will do. > > } cmd; > > > > struct { > > @@ -361,6 +383,8 @@ int spi_mem_driver_register_with_owner(struct spi_mem_driver *drv, > > > > void spi_mem_driver_unregister(struct spi_mem_driver *drv); > > > > +int spi_mem_get_cmd_ext(const struct spi_mem_op *op, u8 *ext); > > + > > #define spi_mem_driver_register(__drv) \ > > spi_mem_driver_register_with_owner(__drv, THIS_MODULE) > > >
On Fri, 28 Feb 2020 15:06:58 +0530 Pratyush Yadav <p.yadav@ti.com> wrote: > Hi Boris, > > On 27/02/20 05:58PM, Boris Brezillon wrote: > > On Wed, 26 Feb 2020 15:06:58 +0530 > > Pratyush Yadav <p.yadav@ti.com> wrote: > > > > > Double Transfer Rate (DTR) is SPI protocol in which data is transferred > > > on each clock edge as opposed to on each clock cycle. Make > > > framework-level changes to allow supporting flashes in DTR mode. > > > > > > Right now, mixed DTR modes are not supported. So, for example a mode > > > like 4S-4D-4D will not work. All phases need to be either DTR or STR. > > > > Didn't go deep into the patch but at first glance you don't seem to > > extend the framework to support stateful modes as I tried to do here > > [1]. That's really something we should address before considering > > supporting xD-xD-xD modes, unless the SPI-NOR only supports one > > stateful mode. If we don't do that first, we might face all sort of > > unpleasant issues: > > > > * kexec not working correctly because the previous kernel left the NOR > > in an unknown state > > * suspend/resume not working properly > > * linux not booting properly because the bootloader left the device in > > its non-default mode > > * ... > > Correct. I am working on a follow-up series that takes care of these > problems. The series will allow spi-nor to detect what mode the flash is > in and then run the SFPD procedure in that mode (or maybe switch to > single SPI mode and then go about its business as usual? I haven't > figured out all the details yet). > > So for the context of this series, assume we are handed the flash in > single SPI mode. > > > [1]https://patchwork.kernel.org/cover/10638055/ > > BTW, I took a quick look at this series but I don't see any code that > tries to detect which mode the flash is in (which is the troublesome > part [0]). So, for example, if the bootloader leaves the flash in > 8D-8D-8D mode, how would your series handle that situation? Oh, it's definitely not taking care of that, it was just paving the road for spi-nor state tracking. You'd need to extend it to support 8D-8D-8D to 1-1-1 transitions at boot time (if that's even possible). > > [0] There are multiple problems to take care of when trying to detect > which mode a flash is in. We can try reading SFDP in each mode and > whichever mode gives us the correct "SFDP" signature is the mode the > flash is in. But the problem is that even in xSPI standard Read SFDP > command is optional in 8D-8D-8D mode, let alone non-xSPI flashes. > Another problem is that the address bytes and dummy cycles for Read > SFDP are not the same for every flash. The xSPI standard says > address bytes can be 3/4 and dummy cycles can be 8/20. So, for > example, Cypress s28hs/s28ht family and Micron Xccela (mt35x) family > use 4 address bytes, but the Adesto ATXP032/ATXP032R flashes use 3 > address bytes. I'd rather go with something simpler and more widely supported than SFDP reads. Don't we have a simple command that's supported by all flashes and returns well known data. Isn't there an EXIT sequence that allows NORs to return to a single SPI state? > > Say that a flash supports Read SFDP in 8D-8D-8D mode and we try all > the combinations to find out which mode the flash is in, we now have > the problem of actually identifying the flash. Unfortunately, the > Read ID command is not uniform across flash vendors. The Micron > Xccela flashes use 8 dummy cycles and no address bytes for Read ID. > The Cypress s28hs/t family uses configurable dummy cycles > (defaulting to 3) and needs 4 dummy address bytes all of which are > 0. Yep, that's what I complained about when I tried to support the Macronix flash. They didn't plan for a reliable RETURN-TO-SINGLE-SPI sequence which would not conflict with any other existing SPI commands, and that's a real problem. > > If we can't find out which flash it is, we can't run its fixup > hooks, and might end up running it with incorrect settings. And all > this is assuming a flash even has SFDP and has it available in all > modes. Absolutely. > > So, the only solution I can now think of is having the flash name in > its compatible string in the device tree. This way we can skip all > the Read ID ugliness and can have flash-specific hooks to make it > easier to detect the mode it is in (though I wonder if it is even > possible to detect the mode in a flash that doesn't have SFDP in > 8D-8D-8D). Hm, I'd really like to avoid that if possible. > > Thoughts? Is there a better way to solve this problem that I didn't > think of? > Nope, except maybe mandate that the bootloader always put the NOR in single SPI mode before booting Linux (and Linux should do the same, which is what my series was trying to address IIRC).
On 28/02/20 11:53AM, Boris Brezillon wrote: > On Fri, 28 Feb 2020 15:06:58 +0530 > Pratyush Yadav <p.yadav@ti.com> wrote: > > > Hi Boris, > > > > On 27/02/20 05:58PM, Boris Brezillon wrote: > > > On Wed, 26 Feb 2020 15:06:58 +0530 > > > Pratyush Yadav <p.yadav@ti.com> wrote: > > > > > > > Double Transfer Rate (DTR) is SPI protocol in which data is transferred > > > > on each clock edge as opposed to on each clock cycle. Make > > > > framework-level changes to allow supporting flashes in DTR mode. > > > > > > > > Right now, mixed DTR modes are not supported. So, for example a mode > > > > like 4S-4D-4D will not work. All phases need to be either DTR or STR. > > > > > > Didn't go deep into the patch but at first glance you don't seem to > > > extend the framework to support stateful modes as I tried to do here > > > [1]. That's really something we should address before considering > > > supporting xD-xD-xD modes, unless the SPI-NOR only supports one > > > stateful mode. If we don't do that first, we might face all sort of > > > unpleasant issues: > > > > > > * kexec not working correctly because the previous kernel left the NOR > > > in an unknown state > > > * suspend/resume not working properly > > > * linux not booting properly because the bootloader left the device in > > > its non-default mode > > > * ... > > > > Correct. I am working on a follow-up series that takes care of these > > problems. The series will allow spi-nor to detect what mode the flash is > > in and then run the SFPD procedure in that mode (or maybe switch to > > single SPI mode and then go about its business as usual? I haven't > > figured out all the details yet). > > > > So for the context of this series, assume we are handed the flash in > > single SPI mode. > > > > > [1]https://patchwork.kernel.org/cover/10638055/ > > > > BTW, I took a quick look at this series but I don't see any code that > > tries to detect which mode the flash is in (which is the troublesome > > part [0]). So, for example, if the bootloader leaves the flash in > > 8D-8D-8D mode, how would your series handle that situation? > > Oh, it's definitely not taking care of that, it was just paving the > road for spi-nor state tracking. You'd need to extend it to support > 8D-8D-8D to 1-1-1 transitions at boot time (if that's even possible). > > > > > [0] There are multiple problems to take care of when trying to detect > > which mode a flash is in. We can try reading SFDP in each mode and > > whichever mode gives us the correct "SFDP" signature is the mode the > > flash is in. But the problem is that even in xSPI standard Read SFDP > > command is optional in 8D-8D-8D mode, let alone non-xSPI flashes. > > Another problem is that the address bytes and dummy cycles for Read > > SFDP are not the same for every flash. The xSPI standard says > > address bytes can be 3/4 and dummy cycles can be 8/20. So, for > > example, Cypress s28hs/s28ht family and Micron Xccela (mt35x) family > > use 4 address bytes, but the Adesto ATXP032/ATXP032R flashes use 3 > > address bytes. > > I'd rather go with something simpler and more widely supported than SFDP > reads. Don't we have a simple command that's supported by all flashes > and returns well known data. I'm not aware of any other command that would return well-known data. > Isn't there an EXIT sequence that allows NORs to return to a single > SPI state? Yes there is, but it comes with a lot of strings attached. There is a hardware reset pin on some flashes that puts the flash in Power-on-Reset (POR) mode. But that pin is not mandatory. It also might not be connected on a given board. The other option is a "Soft Reset" (also optional), which puts the flash in POR mode after it is given the soft reset command. But to send the command you need to know the mode the device is in. On top of that, the Soft Reset opcode differs between flashes. According to the xSPI spec, some flashes can have the opcode as 0xF0 and some others can have it as a two command sequence of 0x66 and 0x99. And the cherry on top is the fact that these reset operations return to a state based on the value of the non-volatile bits. So, if the non-volatile configuration is 8D-8D-8D mode, then all these resets achieve nothing. > > > > Say that a flash supports Read SFDP in 8D-8D-8D mode and we try all > > the combinations to find out which mode the flash is in, we now have > > the problem of actually identifying the flash. Unfortunately, the > > Read ID command is not uniform across flash vendors. The Micron > > Xccela flashes use 8 dummy cycles and no address bytes for Read ID. > > The Cypress s28hs/t family uses configurable dummy cycles > > (defaulting to 3) and needs 4 dummy address bytes all of which are > > 0. > > Yep, that's what I complained about when I tried to support the > Macronix flash. They didn't plan for a reliable RETURN-TO-SINGLE-SPI > sequence which would not conflict with any other existing SPI commands, > and that's a real problem. > > > > > If we can't find out which flash it is, we can't run its fixup > > hooks, and might end up running it with incorrect settings. And all > > this is assuming a flash even has SFDP and has it available in all > > modes. > > Absolutely. > > > > > So, the only solution I can now think of is having the flash name in > > its compatible string in the device tree. This way we can skip all > > the Read ID ugliness and can have flash-specific hooks to make it > > easier to detect the mode it is in (though I wonder if it is even > > possible to detect the mode in a flash that doesn't have SFDP in > > 8D-8D-8D). > > Hm, I'd really like to avoid that if possible. Unfortunately, I don't really see a better alternative. Just so I understand this better, why do you think it is something worth avoiding? > > > > Thoughts? Is there a better way to solve this problem that I didn't > > think of? > > > > Nope, except maybe mandate that the bootloader always put the NOR in > single SPI mode before booting Linux (and Linux should do the same, > which is what my series was trying to address IIRC). A simple bootloader might not even have a SPI driver. So, if the flash PORs to 8D-8D-8D, Linux would be unable to use the flash. Or, if the ROM puts the flash in 8D-8D-8D mode for better boot speed, we would have the same problem.
On Fri, 28 Feb 2020 17:37:50 +0530 Pratyush Yadav <p.yadav@ti.com> wrote: > > Isn't there an EXIT sequence that allows NORs to return to a single > > SPI state? > > Yes there is, but it comes with a lot of strings attached. There is a > hardware reset pin on some flashes that puts the flash in Power-on-Reset > (POR) mode. But that pin is not mandatory. It also might not be > connected on a given board. > > The other option is a "Soft Reset" (also optional), which puts the flash > in POR mode after it is given the soft reset command. But to send the > command you need to know the mode the device is in. On top of that, the > Soft Reset opcode differs between flashes. According to the xSPI spec, > some flashes can have the opcode as 0xF0 and some others can have it as > a two command sequence of 0x66 and 0x99. > > And the cherry on top is the fact that these reset operations return to > a state based on the value of the non-volatile bits. So, if the > non-volatile configuration is 8D-8D-8D mode, then all these resets > achieve nothing. Looks like flash vendors don't learn from their mistakes, they keep adding more features without really thinking about backward compatibility :-(. > > > > > > > > So, the only solution I can now think of is having the flash name in > > > its compatible string in the device tree. This way we can skip all > > > the Read ID ugliness and can have flash-specific hooks to make it > > > easier to detect the mode it is in (though I wonder if it is even > > > possible to detect the mode in a flash that doesn't have SFDP in > > > 8D-8D-8D). > > > > Hm, I'd really like to avoid that if possible. > > Unfortunately, I don't really see a better alternative. Just so I > understand this better, why do you think it is something worth avoiding? There are 2 main reasons: 1/ board manufacturers usually source their flashes from different vendors so they're not tied to one of them. That means you can't really make the compatible too specific or you'd have to deal with DT variants (one variant per-flash). 2/ I feel like once we start accepting specific compats, people will try to abuse it and decide that they need one for their flash too, before even trying to see if there's not a different way to detect the flash.
Hi Boris, On 27/02/20 05:23PM, Boris Brezillon wrote: > On Wed, 26 Feb 2020 15:06:54 +0530 > Pratyush Yadav <p.yadav@ti.com> wrote: > > > These two DT properties express DTR receive and transmit capabilities of > > a SPI flash and controller. Introduce two new mode bits: SPI_RX_DTR and > > SPI_TX_DTR which correspond to the new DT properties. Set these bits > > when the two corresponding properties are present in the device tree. > > Also update the detection of unsupported mode bits to include the new > > bits. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > drivers/spi/spi.c | 10 +++++++++- > > include/linux/spi/spi.h | 2 ++ > > 2 files changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > index 38b4c78df506..25c8ed9343f9 100644 > > --- a/drivers/spi/spi.c > > +++ b/drivers/spi/spi.c > > @@ -1927,6 +1927,13 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, > > } > > } > > > > + /* Device DTR mode. */ > > + if (of_property_read_bool(nc, "spi-tx-dtr")) > > + spi->mode |= SPI_TX_DTR; > > + > > + if (of_property_read_bool(nc, "spi-rx-dtr")) > > + spi->mode |= SPI_RX_DTR; > > + > > If this DTR mode is only used in spi-mem, maybe we shouldn't add those > flags. SPI mem devices are usually smart enough to advertise what they > support, and the subsystem in charge of those devices (in this specific > case, spi-nor) will check what the controller supports > using spi_mem_supports_op(). The only case we might have to deal with > at some point is board level limitations (disabling DTR because the > routing prevents using this mode). Yes, being able to handle board-level limitations is the main reason behind this change. There should be a way to over-ride the use of DTR for a given board. And IIUC, SPI allows doing the same for Rx and Tx buswidth. So I don't see why we should deviate from that model. > > if (spi_controller_is_slave(ctlr)) { > > if (!of_node_name_eq(nc, "slave")) { > > dev_err(&ctlr->dev, "%pOF is not called 'slave'\n", > > @@ -3252,7 +3259,8 @@ int spi_setup(struct spi_device *spi) > > bad_bits &= ~SPI_CS_HIGH; > > ugly_bits = bad_bits & > > (SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL | > > - SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL); > > + SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL | > > + SPI_TX_DTR | SPI_RX_DTR); > > if (ugly_bits) { > > dev_warn(&spi->dev, > > "setup: ignoring unsupported mode bits %x\n", > > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > > index 6d16ba01ff5a..bf1108318389 100644 > > --- a/include/linux/spi/spi.h > > +++ b/include/linux/spi/spi.h > > @@ -183,6 +183,8 @@ struct spi_device { > > #define SPI_TX_OCTAL 0x2000 /* transmit with 8 wires */ > > #define SPI_RX_OCTAL 0x4000 /* receive with 8 wires */ > > #define SPI_3WIRE_HIZ 0x8000 /* high impedance turnaround */ > > +#define SPI_RX_DTR 0x10000 /* receive in DTR mode */ > > +#define SPI_TX_DTR 0x20000 /* transmit in DTR mode */ > > int irq; > > void *controller_state; > > void *controller_data; >
On Mon, 2 Mar 2020 15:18:31 +0530 Pratyush Yadav <p.yadav@ti.com> wrote: > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > > index 38b4c78df506..25c8ed9343f9 100644 > > > --- a/drivers/spi/spi.c > > > +++ b/drivers/spi/spi.c > > > @@ -1927,6 +1927,13 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, > > > } > > > } > > > > > > + /* Device DTR mode. */ > > > + if (of_property_read_bool(nc, "spi-tx-dtr")) > > > + spi->mode |= SPI_TX_DTR; > > > + > > > + if (of_property_read_bool(nc, "spi-rx-dtr")) > > > + spi->mode |= SPI_RX_DTR; > > > + > > > > If this DTR mode is only used in spi-mem, maybe we shouldn't add those > > flags. SPI mem devices are usually smart enough to advertise what they > > support, and the subsystem in charge of those devices (in this specific > > case, spi-nor) will check what the controller supports > > using spi_mem_supports_op(). The only case we might have to deal with > > at some point is board level limitations (disabling DTR because the > > routing prevents using this mode). > > Yes, being able to handle board-level limitations is the main reason > behind this change. There should be a way to over-ride the use of DTR > for a given board. And IIUC, SPI allows doing the same for Rx and Tx > buswidth. So I don't see why we should deviate from that model. My point is, maybe it should be expressed as a limitation, rather than made mandatory for the non-limited case (default to supported, unless stated otherwise). I think we already had this discussion with Rob and Mark regarding the QUAD/DUAL flags, which made conversion from spi-nor to spi-mem non-backward compatible for some controllers (some spi-nor controller drivers were considering the absence of spi-{tx,rx}-width as 'use the max supported by the controller if the device supports it' while the spi subsystem goes for the more conservative 'use single SPI if spi-{tx,rx}-width is missing'). If we introduce a new property, maybe it'd be a good thing to think twice before taking this decision. FWIW, I'd vote for a 'spi-no-dtr' property to express board-level limitations. Orthogonal to this is the question of where we should put those flags, and I'm still not convinced we need that at the spi level (at least not yet).