| Message ID | 20241025161501.485684-2-miquel.raynal@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | spi-nand/spi-mem DTR support | expand |
On 24/10/25 06:14PM, Miquel Raynal wrote: > In the spi subsystem, the bus frequency is derived as follows: > - the controller may expose a minimum and maximum operating frequency > - the hardware description, through the spi peripheral properties, > advise what is the maximum acceptable frequency from a device/wiring > point of view. > Transfers must be observed at a frequency which fits both (so in > practice, the lowest maximum). > > Actually, this second point mixes two information and already takes the > lowest frequency among: > - what the spi device is capable of (what is written in the component > datasheet) > - what the wiring allows (electromagnetic sensibility, crossovers, > terminations, antenna effect, etc). > > This logic works until spi devices are no longer capable of sustaining > their highest frequency regardless of the operation. Spi memories are > typically subject to such variation. Some devices are capable of > spitting their internally stored data (essentially in read mode) at a > very fast rate, typically up to 166MHz on Winbond SPI-NAND chips, using > "fast" commands. However, some of the low-end operations, such as > regular page read-from-cache commands, are more limited and can only be > executed at 54MHz at most. This is currently a problem in the SPI-NAND > subsystem. Another situation, even if not yet supported, will be with > DTR commands, when the data is latched on both edges of the clock. The > same chips as mentioned previously are in this case limited to > 80MHz. Yet another example might be continuous reads, which, under > certain circumstances, can also run at most at 104 or 120MHz. > > As a matter of fact, the "one frequency per chip" policy is outdated and > more fine grain configuration is needed: we need to allow per-operation > frequency limitations. So far, all datasheets I encountered advertise a > maximum default frequency, which need to be lowered for certain specific > operations. So based on the current infrastructure, we can still expect > firmware (device trees in general) to continued advertising the same > maximum speed which is a mix between the PCB limitations and the chip > maximum capability, and expect per-operation lower frequencies when this > is relevant. Hi Miquel, I met the similar problem when working on the Micron MT35XU01GBBA SPI NOR chip. The chip can work at 166MHz in SDR mode but 200MHz in DDR mode. I found Read ID failed on some platforms when using 200MHz as maximum frequency, so I have to lower the maximum frequency with some performance loss. I think the patch is useful but the SPI NOR doesn't have such vendor-specific predefined SPI_MEM_OPS like SPI NAND. Do you have any suggestion on how to handle this case? Thanks. > > Add a `struct spi_mem_op` member to carry this information. Not > providing this field explicitly from upper layers means that there is no > further constraint and the default spi device maximum speed will be > carried instead. The SPI_MEM_OP() macro is also expanded with an > optional frequency argument, because virtually all operations can be > subject to such a limitation, and this will allow for a smooth and > discrete transition. > > For controller drivers which do not implement the spi-mem interface, the > per-transfer speed is also set acordingly to a lower (than the maximum > default) speed, or 0, to comply with the current API. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/spi/spi-mem.c | 8 ++++++++ > include/linux/spi/spi-mem.h | 11 ++++++++++- > 2 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > index 17b8baf749e6..ab650ae953bb 100644 > --- a/drivers/spi/spi-mem.c > +++ b/drivers/spi/spi-mem.c > @@ -356,6 +356,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > { > unsigned int tmpbufsize, xferpos = 0, totalxferlen = 0; > struct spi_controller *ctlr = mem->spi->controller; > + unsigned int xfer_speed = op->max_freq; > struct spi_transfer xfers[4] = { }; > struct spi_message msg; > u8 *tmpbuf; > @@ -368,6 +369,9 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > if (!spi_mem_internal_supports_op(mem, op)) > return -EOPNOTSUPP; > > + if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz) > + ((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz; > + > if (ctlr->mem_ops && ctlr->mem_ops->exec_op && !spi_get_csgpiod(mem->spi, 0)) { > ret = spi_mem_access_start(mem); > if (ret) > @@ -407,6 +411,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > xfers[xferpos].tx_buf = tmpbuf; > xfers[xferpos].len = op->cmd.nbytes; > xfers[xferpos].tx_nbits = op->cmd.buswidth; > + xfers[xferpos].speed_hz = xfer_speed; > spi_message_add_tail(&xfers[xferpos], &msg); > xferpos++; > totalxferlen++; > @@ -421,6 +426,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > xfers[xferpos].tx_buf = tmpbuf + 1; > xfers[xferpos].len = op->addr.nbytes; > xfers[xferpos].tx_nbits = op->addr.buswidth; > + xfers[xferpos].speed_hz = xfer_speed; > spi_message_add_tail(&xfers[xferpos], &msg); > xferpos++; > totalxferlen += op->addr.nbytes; > @@ -432,6 +438,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > xfers[xferpos].len = op->dummy.nbytes; > xfers[xferpos].tx_nbits = op->dummy.buswidth; > xfers[xferpos].dummy_data = 1; > + xfers[xferpos].speed_hz = xfer_speed; > spi_message_add_tail(&xfers[xferpos], &msg); > xferpos++; > totalxferlen += op->dummy.nbytes; > @@ -447,6 +454,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > } > > xfers[xferpos].len = op->data.nbytes; > + xfers[xferpos].speed_hz = xfer_speed; > spi_message_add_tail(&xfers[xferpos], &msg); > xferpos++; > totalxferlen += op->data.nbytes; > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h > index f866d5c8ed32..8963f236911b 100644 > --- a/include/linux/spi/spi-mem.h > +++ b/include/linux/spi/spi-mem.h > @@ -68,6 +68,9 @@ enum spi_mem_data_dir { > SPI_MEM_DATA_OUT, > }; > > +#define SPI_MEM_OP_MAX_FREQ(__freq) \ > + .max_freq = __freq > + > /** > * struct spi_mem_op - describes a SPI memory operation > * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is > @@ -95,6 +98,9 @@ enum spi_mem_data_dir { > * operation does not involve transferring data > * @data.buf.in: input buffer (must be DMA-able) > * @data.buf.out: output buffer (must be DMA-able) > + * @max_freq: frequency limitation wrt this operation. 0 means there is no > + * specific constraint and the highest achievable frequency can be > + * attempted). > */ > struct spi_mem_op { > struct { > @@ -132,14 +138,17 @@ struct spi_mem_op { > const void *out; > } buf; > } data; > + > + unsigned int max_freq; > }; > > -#define SPI_MEM_OP(__cmd, __addr, __dummy, __data) \ > +#define SPI_MEM_OP(__cmd, __addr, __dummy, __data, ...) \ > { \ > .cmd = __cmd, \ > .addr = __addr, \ > .dummy = __dummy, \ > .data = __data, \ > + __VA_ARGS__ \ > } > > /** > -- > 2.43.0 >
On 10/30/24 8:52 PM, Han Xu wrote: > Hi Miquel, Hi! > > I met the similar problem when working on the Micron MT35XU01GBBA SPI NOR chip. > The chip can work at 166MHz in SDR mode but 200MHz in DDR mode. I found Read ID > failed on some platforms when using 200MHz as maximum frequency, so I have to > lower the maximum frequency with some performance loss. > > I think the patch is useful but the SPI NOR doesn't have such vendor-specific > predefined SPI_MEM_OPS like SPI NAND. Do you have any suggestion on how to handle > this case? Thanks. Why can't we add similar predefined SPI_MEM_OPS in SPI NOR? Cheers, ta
On 10/25/24 5:14 PM, Miquel Raynal wrote: cut > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > index 17b8baf749e6..ab650ae953bb 100644 > --- a/drivers/spi/spi-mem.c > +++ b/drivers/spi/spi-mem.c > @@ -356,6 +356,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > { > unsigned int tmpbufsize, xferpos = 0, totalxferlen = 0; > struct spi_controller *ctlr = mem->spi->controller; > + unsigned int xfer_speed = op->max_freq; be aware that for controllers that don't support SPIMEM ops, you pass the frequency from the upper layers, without adjusting it with spi->max_speed_hz. Was this intentional? > struct spi_transfer xfers[4] = { }; > struct spi_message msg; > u8 *tmpbuf; > @@ -368,6 +369,9 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > if (!spi_mem_internal_supports_op(mem, op)) > return -EOPNOTSUPP; > > + if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz) > + ((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz; not a big fan of casting the const out. How about introducing a spi_mem_adjust_op_freq()? The upper layers will use that were needed, and you'll still be able to pass a const op to spi_mem_exec_op() cut > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h > index f866d5c8ed32..8963f236911b 100644 > --- a/include/linux/spi/spi-mem.h > +++ b/include/linux/spi/spi-mem.h > @@ -68,6 +68,9 @@ enum spi_mem_data_dir { > SPI_MEM_DATA_OUT, > }; > > +#define SPI_MEM_OP_MAX_FREQ(__freq) \ > + .max_freq = __freq > + > /** > * struct spi_mem_op - describes a SPI memory operation > * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is > @@ -95,6 +98,9 @@ enum spi_mem_data_dir { > * operation does not involve transferring data > * @data.buf.in: input buffer (must be DMA-able) > * @data.buf.out: output buffer (must be DMA-able) > + * @max_freq: frequency limitation wrt this operation. 0 means there is no > + * specific constraint and the highest achievable frequency can be > + * attempted). nit: you close a parenthesis without opening one Looking good, ta
On Fri, Oct 25 2024, Miquel Raynal wrote: > In the spi subsystem, the bus frequency is derived as follows: > - the controller may expose a minimum and maximum operating frequency > - the hardware description, through the spi peripheral properties, > advise what is the maximum acceptable frequency from a device/wiring > point of view. > Transfers must be observed at a frequency which fits both (so in > practice, the lowest maximum). > > Actually, this second point mixes two information and already takes the > lowest frequency among: > - what the spi device is capable of (what is written in the component > datasheet) > - what the wiring allows (electromagnetic sensibility, crossovers, > terminations, antenna effect, etc). > > This logic works until spi devices are no longer capable of sustaining > their highest frequency regardless of the operation. Spi memories are > typically subject to such variation. Some devices are capable of > spitting their internally stored data (essentially in read mode) at a > very fast rate, typically up to 166MHz on Winbond SPI-NAND chips, using > "fast" commands. However, some of the low-end operations, such as > regular page read-from-cache commands, are more limited and can only be > executed at 54MHz at most. This is currently a problem in the SPI-NAND > subsystem. Another situation, even if not yet supported, will be with > DTR commands, when the data is latched on both edges of the clock. The > same chips as mentioned previously are in this case limited to > 80MHz. Yet another example might be continuous reads, which, under > certain circumstances, can also run at most at 104 or 120MHz. It's been a while so I don't remember the specifics anymore, but IIRC some NOR flashes had the same issue. Some commands could only run at a lower frequency. > > As a matter of fact, the "one frequency per chip" policy is outdated and > more fine grain configuration is needed: we need to allow per-operation > frequency limitations. So far, all datasheets I encountered advertise a > maximum default frequency, which need to be lowered for certain specific > operations. So based on the current infrastructure, we can still expect > firmware (device trees in general) to continued advertising the same > maximum speed which is a mix between the PCB limitations and the chip > maximum capability, and expect per-operation lower frequencies when this > is relevant. > > Add a `struct spi_mem_op` member to carry this information. Not > providing this field explicitly from upper layers means that there is no > further constraint and the default spi device maximum speed will be > carried instead. The SPI_MEM_OP() macro is also expanded with an > optional frequency argument, because virtually all operations can be > subject to such a limitation, and this will allow for a smooth and > discrete transition. > > For controller drivers which do not implement the spi-mem interface, the > per-transfer speed is also set acordingly to a lower (than the maximum > default) speed, or 0, to comply with the current API. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- [...] > /** > * struct spi_mem_op - describes a SPI memory operation > * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is > @@ -95,6 +98,9 @@ enum spi_mem_data_dir { > * operation does not involve transferring data > * @data.buf.in: input buffer (must be DMA-able) > * @data.buf.out: output buffer (must be DMA-able) > + * @max_freq: frequency limitation wrt this operation. 0 means there is no > + * specific constraint and the highest achievable frequency can be > + * attempted). > */ > struct spi_mem_op { > struct { > @@ -132,14 +138,17 @@ struct spi_mem_op { > const void *out; > } buf; > } data; > + > + unsigned int max_freq; So we limit the maximum frequency to roughly 4.2 GHz. Seems reasonable, considering the top end NOR flashes do up to 200-300 MHz. Didn't look too closely at the code but the idea seems good to me. Acked-by: Pratyush Yadav <pratyush@kernel.org>
Hello Tudor, On 11/11/2024 at 13:07:09 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > On 10/25/24 5:14 PM, Miquel Raynal wrote: > > cut > >> >> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c >> index 17b8baf749e6..ab650ae953bb 100644 >> --- a/drivers/spi/spi-mem.c >> +++ b/drivers/spi/spi-mem.c >> @@ -356,6 +356,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) >> { >> unsigned int tmpbufsize, xferpos = 0, totalxferlen = 0; >> struct spi_controller *ctlr = mem->spi->controller; >> + unsigned int xfer_speed = op->max_freq; > > be aware that for controllers that don't support SPIMEM ops, you pass > the frequency from the upper layers, without adjusting it with > spi->max_speed_hz. Was this intentional? That is exactly the opposite of what I wanted to achieve initially. That's a very good catch. >> struct spi_transfer xfers[4] = { }; >> struct spi_message msg; >> u8 *tmpbuf; >> @@ -368,6 +369,9 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) >> if (!spi_mem_internal_supports_op(mem, op)) >> return -EOPNOTSUPP; >> >> + if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz) >> + ((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz; > > not a big fan of casting the const out. How about introducing a > spi_mem_adjust_op_freq()? The upper layers will use that were needed, > and you'll still be able to pass a const op to spi_mem_exec_op() I know it is not ideal so to follow your idea I drafted the use of spi_mem_adjust_op_freq(). In order to avoid the cast, we actually need to call this function everywhere in the core and the drivers to make sure we never get out of bounds, but here is the problem: $ git grep -w spi_mem_exec_op -- drivers/ | wc -l 42 This approach requires to add a call to spi_mem_adjust_op_freq() before *every* spi_mem_exec_op(). Yes I can do that but that means to be very attentive to the fact that these two functions are always called together. I am not sure it is a good idea. What about doing the following once in spi_mem_exec_op() instead? spi_mem_adjust_op_freq(desc->mem, (struct spi_mem_op *)op); I know we still have a cast, but it feels more acceptable than the one I initially proposed and covers all cases. I would not accept that in a driver, but here we are in the core, so that sounds acceptable. Another possibility otherwise would be to drop the const from the spi_mem_op structure entirely. But I prefer the above function call. >> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h >> index f866d5c8ed32..8963f236911b 100644 >> --- a/include/linux/spi/spi-mem.h >> +++ b/include/linux/spi/spi-mem.h >> @@ -68,6 +68,9 @@ enum spi_mem_data_dir { >> SPI_MEM_DATA_OUT, >> }; >> >> +#define SPI_MEM_OP_MAX_FREQ(__freq) \ >> + .max_freq = __freq >> + >> /** >> * struct spi_mem_op - describes a SPI memory operation >> * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is >> @@ -95,6 +98,9 @@ enum spi_mem_data_dir { >> * operation does not involve transferring data >> * @data.buf.in: input buffer (must be DMA-able) >> * @data.buf.out: output buffer (must be DMA-able) >> + * @max_freq: frequency limitation wrt this operation. 0 means there is no >> + * specific constraint and the highest achievable frequency can be >> + * attempted). > > nit: you close a parenthesis without opening one Corrected! Thanks for this very useful feedback! Miquèl
On 12/13/24 10:46 AM, Miquel Raynal wrote: > Hello Tudor, > Hi! > On 11/11/2024 at 13:07:09 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >> On 10/25/24 5:14 PM, Miquel Raynal wrote: >> >> cut >> >>> >>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c >>> index 17b8baf749e6..ab650ae953bb 100644 >>> --- a/drivers/spi/spi-mem.c >>> +++ b/drivers/spi/spi-mem.c cut >>> + if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz) >>> + ((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz; >> >> not a big fan of casting the const out. How about introducing a >> spi_mem_adjust_op_freq()? The upper layers will use that were needed, >> and you'll still be able to pass a const op to spi_mem_exec_op() > > I know it is not ideal so to follow your idea I drafted the use of > spi_mem_adjust_op_freq(). In order to avoid the cast, we actually need > to call this function everywhere in the core and the drivers to make > sure we never get out of bounds, but here is the problem: > > $ git grep -w spi_mem_exec_op -- drivers/ | wc -l > 42 > > This approach requires to add a call to spi_mem_adjust_op_freq() before > *every* spi_mem_exec_op(). Yes I can do that but that means to be very > attentive to the fact that these two functions are always called > together. I am not sure it is a good idea. > > What about doing the following once in spi_mem_exec_op() instead? > > spi_mem_adjust_op_freq(desc->mem, (struct spi_mem_op *)op); > > I know we still have a cast, but it feels more acceptable than the one I > initially proposed and covers all cases. I would not accept that in a > driver, but here we are in the core, so that sounds acceptable. > > Another possibility otherwise would be to drop the const from the > spi_mem_op structure entirely. But I prefer the above function call. How about introducing a spi_nand_spimem_exec_op() where you call spi_mem_adjust_op_freq() and spi_mem_exec_op()?
On 18/12/2024 at 08:07:24 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > On 12/13/24 10:46 AM, Miquel Raynal wrote: >> Hello Tudor, >> > > Hi! > >> On 11/11/2024 at 13:07:09 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >> >>> On 10/25/24 5:14 PM, Miquel Raynal wrote: >>> >>> cut >>> >>>> >>>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c >>>> index 17b8baf749e6..ab650ae953bb 100644 >>>> --- a/drivers/spi/spi-mem.c >>>> +++ b/drivers/spi/spi-mem.c > > cut > >>>> + if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz) >>>> + ((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz; >>> >>> not a big fan of casting the const out. How about introducing a >>> spi_mem_adjust_op_freq()? The upper layers will use that were needed, >>> and you'll still be able to pass a const op to spi_mem_exec_op() >> >> I know it is not ideal so to follow your idea I drafted the use of >> spi_mem_adjust_op_freq(). In order to avoid the cast, we actually need >> to call this function everywhere in the core and the drivers to make >> sure we never get out of bounds, but here is the problem: >> >> $ git grep -w spi_mem_exec_op -- drivers/ | wc -l >> 42 >> >> This approach requires to add a call to spi_mem_adjust_op_freq() before >> *every* spi_mem_exec_op(). Yes I can do that but that means to be very >> attentive to the fact that these two functions are always called >> together. I am not sure it is a good idea. >> >> What about doing the following once in spi_mem_exec_op() instead? >> >> spi_mem_adjust_op_freq(desc->mem, (struct spi_mem_op *)op); >> >> I know we still have a cast, but it feels more acceptable than the one I >> initially proposed and covers all cases. I would not accept that in a >> driver, but here we are in the core, so that sounds acceptable. >> >> Another possibility otherwise would be to drop the const from the >> spi_mem_op structure entirely. But I prefer the above function call. > > How about introducing a spi_nand_spimem_exec_op() where you call > spi_mem_adjust_op_freq() and spi_mem_exec_op()? That would work to make the cast disappear but TBH would not be totally relevant as adjusting the frequency is typically something that would benefit to spi-nor as well (maybe in the future) and therefore would fully apply to spi memories as a whole, not just spi-nand. We can think about another naming maybe, but I find like spi_mem_exec_op() is the right location to do this. Thanks, Miquèl
On 12/18/24 9:37 AM, Miquel Raynal wrote: > On 18/12/2024 at 08:07:24 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >> On 12/13/24 10:46 AM, Miquel Raynal wrote: >>> Hello Tudor, >>> >> >> Hi! >> >>> On 11/11/2024 at 13:07:09 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >>> >>>> On 10/25/24 5:14 PM, Miquel Raynal wrote: >>>> >>>> cut >>>> >>>>> >>>>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c >>>>> index 17b8baf749e6..ab650ae953bb 100644 >>>>> --- a/drivers/spi/spi-mem.c >>>>> +++ b/drivers/spi/spi-mem.c >> >> cut >> >>>>> + if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz) >>>>> + ((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz; >>>> >>>> not a big fan of casting the const out. How about introducing a >>>> spi_mem_adjust_op_freq()? The upper layers will use that were needed, >>>> and you'll still be able to pass a const op to spi_mem_exec_op() >>> >>> I know it is not ideal so to follow your idea I drafted the use of >>> spi_mem_adjust_op_freq(). In order to avoid the cast, we actually need >>> to call this function everywhere in the core and the drivers to make >>> sure we never get out of bounds, but here is the problem: >>> >>> $ git grep -w spi_mem_exec_op -- drivers/ | wc -l >>> 42 >>> >>> This approach requires to add a call to spi_mem_adjust_op_freq() before >>> *every* spi_mem_exec_op(). Yes I can do that but that means to be very >>> attentive to the fact that these two functions are always called >>> together. I am not sure it is a good idea. >>> >>> What about doing the following once in spi_mem_exec_op() instead? >>> >>> spi_mem_adjust_op_freq(desc->mem, (struct spi_mem_op *)op); >>> >>> I know we still have a cast, but it feels more acceptable than the one I >>> initially proposed and covers all cases. I would not accept that in a >>> driver, but here we are in the core, so that sounds acceptable. >>> >>> Another possibility otherwise would be to drop the const from the >>> spi_mem_op structure entirely. But I prefer the above function call. >> >> How about introducing a spi_nand_spimem_exec_op() where you call >> spi_mem_adjust_op_freq() and spi_mem_exec_op()? > > That would work to make the cast disappear but TBH would not be totally > relevant as adjusting the frequency is typically something that would > benefit to spi-nor as well (maybe in the future) and therefore would Right, SPI NOR will benefit of this too. > fully apply to spi memories as a whole, not just spi-nand. We can think > about another naming maybe, but I find like spi_mem_exec_op() is the > right location to do this. > It's not the first time that we adjust spi_mem_op parameters, see: https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/drivers/mtd/spi-nor/core.c?h=spi-nor/next#n153 Does SPI NAND need to call spi_mem_adjust_op_size as well? I see it calls it when using dirmap, but not with a plain spi_mem_exec_op().
On 12/18/24 10:03 AM, Tudor Ambarus wrote: > > > On 12/18/24 9:37 AM, Miquel Raynal wrote: >> On 18/12/2024 at 08:07:24 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >> >>> On 12/13/24 10:46 AM, Miquel Raynal wrote: >>>> Hello Tudor, >>>> >>> >>> Hi! >>> >>>> On 11/11/2024 at 13:07:09 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >>>> >>>>> On 10/25/24 5:14 PM, Miquel Raynal wrote: >>>>> >>>>> cut >>>>> >>>>>> >>>>>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c >>>>>> index 17b8baf749e6..ab650ae953bb 100644 >>>>>> --- a/drivers/spi/spi-mem.c >>>>>> +++ b/drivers/spi/spi-mem.c >>> >>> cut >>> >>>>>> + if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz) >>>>>> + ((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz; >>>>> >>>>> not a big fan of casting the const out. How about introducing a >>>>> spi_mem_adjust_op_freq()? The upper layers will use that were needed, >>>>> and you'll still be able to pass a const op to spi_mem_exec_op() >>>> >>>> I know it is not ideal so to follow your idea I drafted the use of >>>> spi_mem_adjust_op_freq(). In order to avoid the cast, we actually need >>>> to call this function everywhere in the core and the drivers to make >>>> sure we never get out of bounds, but here is the problem: >>>> >>>> $ git grep -w spi_mem_exec_op -- drivers/ | wc -l >>>> 42 >>>> >>>> This approach requires to add a call to spi_mem_adjust_op_freq() before >>>> *every* spi_mem_exec_op(). Yes I can do that but that means to be very >>>> attentive to the fact that these two functions are always called >>>> together. I am not sure it is a good idea. >>>> >>>> What about doing the following once in spi_mem_exec_op() instead? >>>> >>>> spi_mem_adjust_op_freq(desc->mem, (struct spi_mem_op *)op); >>>> >>>> I know we still have a cast, but it feels more acceptable than the one I >>>> initially proposed and covers all cases. I would not accept that in a >>>> driver, but here we are in the core, so that sounds acceptable. >>>> >>>> Another possibility otherwise would be to drop the const from the >>>> spi_mem_op structure entirely. But I prefer the above function call. >>> >>> How about introducing a spi_nand_spimem_exec_op() where you call >>> spi_mem_adjust_op_freq() and spi_mem_exec_op()? >> >> That would work to make the cast disappear but TBH would not be totally >> relevant as adjusting the frequency is typically something that would >> benefit to spi-nor as well (maybe in the future) and therefore would > > Right, SPI NOR will benefit of this too. > >> fully apply to spi memories as a whole, not just spi-nand. We can think >> about another naming maybe, but I find like spi_mem_exec_op() is the >> right location to do this. >> > > It's not the first time that we adjust spi_mem_op parameters, see: > https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/drivers/mtd/spi-nor/core.c?h=spi-nor/next#n153 > > Does SPI NAND need to call spi_mem_adjust_op_size as well? I see it > calls it when using dirmap, but not with a plain spi_mem_exec_op(). > I ask because I'm thinking of adding in the SPIMEM core a prepare() method, and maybe rename exec_op() to exec(). And then introduce a prepare_exec() method that the upper layers would call? Similar to clk_prepare_enable.
Hello, On 18/12/2024 at 10:13:39 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > On 12/18/24 10:03 AM, Tudor Ambarus wrote: >> >> >> On 12/18/24 9:37 AM, Miquel Raynal wrote: >>> On 18/12/2024 at 08:07:24 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >>> >>>> On 12/13/24 10:46 AM, Miquel Raynal wrote: >>>>> Hello Tudor, >>>>> >>>> >>>> Hi! >>>> >>>>> On 11/11/2024 at 13:07:09 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >>>>> >>>>>> On 10/25/24 5:14 PM, Miquel Raynal wrote: >>>>>> >>>>>> cut >>>>>> >>>>>>> >>>>>>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c >>>>>>> index 17b8baf749e6..ab650ae953bb 100644 >>>>>>> --- a/drivers/spi/spi-mem.c >>>>>>> +++ b/drivers/spi/spi-mem.c >>>> >>>> cut >>>> >>>>>>> + if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz) >>>>>>> + ((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz; >>>>>> >>>>>> not a big fan of casting the const out. How about introducing a >>>>>> spi_mem_adjust_op_freq()? The upper layers will use that were needed, >>>>>> and you'll still be able to pass a const op to spi_mem_exec_op() >>>>> >>>>> I know it is not ideal so to follow your idea I drafted the use of >>>>> spi_mem_adjust_op_freq(). In order to avoid the cast, we actually need >>>>> to call this function everywhere in the core and the drivers to make >>>>> sure we never get out of bounds, but here is the problem: >>>>> >>>>> $ git grep -w spi_mem_exec_op -- drivers/ | wc -l >>>>> 42 >>>>> >>>>> This approach requires to add a call to spi_mem_adjust_op_freq() before >>>>> *every* spi_mem_exec_op(). Yes I can do that but that means to be very >>>>> attentive to the fact that these two functions are always called >>>>> together. I am not sure it is a good idea. >>>>> >>>>> What about doing the following once in spi_mem_exec_op() instead? >>>>> >>>>> spi_mem_adjust_op_freq(desc->mem, (struct spi_mem_op *)op); >>>>> >>>>> I know we still have a cast, but it feels more acceptable than the one I >>>>> initially proposed and covers all cases. I would not accept that in a >>>>> driver, but here we are in the core, so that sounds acceptable. >>>>> >>>>> Another possibility otherwise would be to drop the const from the >>>>> spi_mem_op structure entirely. But I prefer the above function call. >>>> >>>> How about introducing a spi_nand_spimem_exec_op() where you call >>>> spi_mem_adjust_op_freq() and spi_mem_exec_op()? >>> >>> That would work to make the cast disappear but TBH would not be totally >>> relevant as adjusting the frequency is typically something that would >>> benefit to spi-nor as well (maybe in the future) and therefore would >> >> Right, SPI NOR will benefit of this too. >> >>> fully apply to spi memories as a whole, not just spi-nand. We can think >>> about another naming maybe, but I find like spi_mem_exec_op() is the >>> right location to do this. >>> >> >> It's not the first time that we adjust spi_mem_op parameters, see: >> https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/drivers/mtd/spi-nor/core.c?h=spi-nor/next#n153 >> >> Does SPI NAND need to call spi_mem_adjust_op_size as well? I see it >> calls it when using dirmap, but not with a plain spi_mem_exec_op(). >> > > I ask because I'm thinking of adding in the SPIMEM core a prepare() > method, and maybe rename exec_op() to exec(). And then introduce a > prepare_exec() method that the upper layers would call? Similar to > clk_prepare_enable. Do you have something else in mind you would like to put in the prepare step? I am not at all opposed to it, but I feel like for now the spi_mem_exec_op() is a fine path for that, especially since there are very little things to "prepare" (for now). Do you mind if I keep the cast (not the one from the series, I cleaned that up to have an adjust_op function as discussed) for now, and if you ever go the prepare/exec path we would get this converted to use the new API? I'd like to make progresses on other topics in the spi-nand core and avoid being blocked by a bigger task that we need to discuss first. Cheers, Miquèl
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index 17b8baf749e6..ab650ae953bb 100644 --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -356,6 +356,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) { unsigned int tmpbufsize, xferpos = 0, totalxferlen = 0; struct spi_controller *ctlr = mem->spi->controller; + unsigned int xfer_speed = op->max_freq; struct spi_transfer xfers[4] = { }; struct spi_message msg; u8 *tmpbuf; @@ -368,6 +369,9 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) if (!spi_mem_internal_supports_op(mem, op)) return -EOPNOTSUPP; + if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz) + ((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz; + if (ctlr->mem_ops && ctlr->mem_ops->exec_op && !spi_get_csgpiod(mem->spi, 0)) { ret = spi_mem_access_start(mem); if (ret) @@ -407,6 +411,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) xfers[xferpos].tx_buf = tmpbuf; xfers[xferpos].len = op->cmd.nbytes; xfers[xferpos].tx_nbits = op->cmd.buswidth; + xfers[xferpos].speed_hz = xfer_speed; spi_message_add_tail(&xfers[xferpos], &msg); xferpos++; totalxferlen++; @@ -421,6 +426,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) xfers[xferpos].tx_buf = tmpbuf + 1; xfers[xferpos].len = op->addr.nbytes; xfers[xferpos].tx_nbits = op->addr.buswidth; + xfers[xferpos].speed_hz = xfer_speed; spi_message_add_tail(&xfers[xferpos], &msg); xferpos++; totalxferlen += op->addr.nbytes; @@ -432,6 +438,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) xfers[xferpos].len = op->dummy.nbytes; xfers[xferpos].tx_nbits = op->dummy.buswidth; xfers[xferpos].dummy_data = 1; + xfers[xferpos].speed_hz = xfer_speed; spi_message_add_tail(&xfers[xferpos], &msg); xferpos++; totalxferlen += op->dummy.nbytes; @@ -447,6 +454,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) } xfers[xferpos].len = op->data.nbytes; + xfers[xferpos].speed_hz = xfer_speed; spi_message_add_tail(&xfers[xferpos], &msg); xferpos++; totalxferlen += op->data.nbytes; diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h index f866d5c8ed32..8963f236911b 100644 --- a/include/linux/spi/spi-mem.h +++ b/include/linux/spi/spi-mem.h @@ -68,6 +68,9 @@ enum spi_mem_data_dir { SPI_MEM_DATA_OUT, }; +#define SPI_MEM_OP_MAX_FREQ(__freq) \ + .max_freq = __freq + /** * struct spi_mem_op - describes a SPI memory operation * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is @@ -95,6 +98,9 @@ enum spi_mem_data_dir { * operation does not involve transferring data * @data.buf.in: input buffer (must be DMA-able) * @data.buf.out: output buffer (must be DMA-able) + * @max_freq: frequency limitation wrt this operation. 0 means there is no + * specific constraint and the highest achievable frequency can be + * attempted). */ struct spi_mem_op { struct { @@ -132,14 +138,17 @@ struct spi_mem_op { const void *out; } buf; } data; + + unsigned int max_freq; }; -#define SPI_MEM_OP(__cmd, __addr, __dummy, __data) \ +#define SPI_MEM_OP(__cmd, __addr, __dummy, __data, ...) \ { \ .cmd = __cmd, \ .addr = __addr, \ .dummy = __dummy, \ .data = __data, \ + __VA_ARGS__ \ } /**
In the spi subsystem, the bus frequency is derived as follows: - the controller may expose a minimum and maximum operating frequency - the hardware description, through the spi peripheral properties, advise what is the maximum acceptable frequency from a device/wiring point of view. Transfers must be observed at a frequency which fits both (so in practice, the lowest maximum). Actually, this second point mixes two information and already takes the lowest frequency among: - what the spi device is capable of (what is written in the component datasheet) - what the wiring allows (electromagnetic sensibility, crossovers, terminations, antenna effect, etc). This logic works until spi devices are no longer capable of sustaining their highest frequency regardless of the operation. Spi memories are typically subject to such variation. Some devices are capable of spitting their internally stored data (essentially in read mode) at a very fast rate, typically up to 166MHz on Winbond SPI-NAND chips, using "fast" commands. However, some of the low-end operations, such as regular page read-from-cache commands, are more limited and can only be executed at 54MHz at most. This is currently a problem in the SPI-NAND subsystem. Another situation, even if not yet supported, will be with DTR commands, when the data is latched on both edges of the clock. The same chips as mentioned previously are in this case limited to 80MHz. Yet another example might be continuous reads, which, under certain circumstances, can also run at most at 104 or 120MHz. As a matter of fact, the "one frequency per chip" policy is outdated and more fine grain configuration is needed: we need to allow per-operation frequency limitations. So far, all datasheets I encountered advertise a maximum default frequency, which need to be lowered for certain specific operations. So based on the current infrastructure, we can still expect firmware (device trees in general) to continued advertising the same maximum speed which is a mix between the PCB limitations and the chip maximum capability, and expect per-operation lower frequencies when this is relevant. Add a `struct spi_mem_op` member to carry this information. Not providing this field explicitly from upper layers means that there is no further constraint and the default spi device maximum speed will be carried instead. The SPI_MEM_OP() macro is also expanded with an optional frequency argument, because virtually all operations can be subject to such a limitation, and this will allow for a smooth and discrete transition. For controller drivers which do not implement the spi-mem interface, the per-transfer speed is also set acordingly to a lower (than the maximum default) speed, or 0, to comply with the current API. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/spi/spi-mem.c | 8 ++++++++ include/linux/spi/spi-mem.h | 11 ++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-)