| Message ID | 20241025161501.485684-3-miquel.raynal@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | spi-nand/spi-mem DTR support | expand |
On Fri, Oct 25, 2024 at 06:14:39PM +0200, Miquel Raynal wrote: > There are spi devices with multiple frequency limitations depending on > the invoked command. We probably do not want to afford running at the > lowest supported frequency all the time, so if we want to get the most > of our hardware, we need to allow per-operation frequency limitations. This all makes sense to me. I'll leave it a little bit to see if anyone (especially people working with the individual devices) has any thoughts and assuming no issues apply the SPI bits on a branch. Does that sound sensible?
On Fri, Oct 25, 2024 at 06:14:39PM +0200, Miquel Raynal wrote: > There are spi devices with multiple frequency limitations depending on > the invoked command. We probably do not want to afford running at the > lowest supported frequency all the time, so if we want to get the most > of our hardware, we need to allow per-operation frequency limitations. After applying this patch (I bisected the series) my Avenger96 board started failing to probe the SPI NOR flash it has: [ 3.567876] spi-nor spi0.0: probe with driver spi-nor failed with error -95 Full job: https://lava.sirena.org.uk/scheduler/job/925156 I didn't spot anything with the code on a recheck but it's late on a Friday so I've not looked too hard. My other boards are all fine though there's limited coverage.
Hi Mark, Thanks a lot for the testing and sorry for being slow. On 01/11/2024 at 20:17:33 GMT, Mark Brown <broonie@kernel.org> wrote: > On Fri, Oct 25, 2024 at 06:14:39PM +0200, Miquel Raynal wrote: >> There are spi devices with multiple frequency limitations depending on >> the invoked command. We probably do not want to afford running at the >> lowest supported frequency all the time, so if we want to get the most >> of our hardware, we need to allow per-operation frequency limitations. > > After applying this patch (I bisected the series) my Avenger96 board > started failing to probe the SPI NOR flash it has: > > [ 3.567876] spi-nor spi0.0: probe with driver spi-nor failed with > error -95 This is an EOPNOTSUPP so maybe there is a new check that is breaking your board. I checked the hardware manual, they talk about a NOR flash. Looking at the code, I believe I forgot the SPI-NOR case which currently does not (yet?) use the op->max_freq parameter. > Full job: > > https://lava.sirena.org.uk/scheduler/job/925156 > > I didn't spot anything with the code on a recheck but it's late on a > Friday so I've not looked too hard. My other boards are all fine though > there's limited coverage. Would you mind testing the series with this change on top and tell me if that fixes it? --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -184,7 +184,7 @@ bool spi_mem_default_supports_op(struct spi_mem *mem, return false; } - if (op->max_freq < mem->spi->max_speed_hz) { + if (op->max_freq && op->max_freq < mem->spi->max_speed_hz) { if (!spi_mem_controller_is_capable(ctlr, per_op_freq)) return false; } I don't know how easy it is for you to make that test with lava, let me know if you prefer me to send a fixup! patch or even resend the whole series (but it's a bit big). Thanks, Miquèl
On 07/11/2024 at 17:15:03 GMT, Mark Brown <broonie@kernel.org> wrote: > On Thu, Nov 07, 2024 at 11:40:00AM +0100, Miquel Raynal wrote: >> On 01/11/2024 at 20:17:33 GMT, Mark Brown <broonie@kernel.org> wrote: > >> > After applying this patch (I bisected the series) my Avenger96 board >> > started failing to probe the SPI NOR flash it has: > >> > [ 3.567876] spi-nor spi0.0: probe with driver spi-nor failed with >> > error -95 > >> Would you mind testing the series with this change on top and tell me if >> that fixes it? >> >> --- a/drivers/spi/spi-mem.c >> +++ b/drivers/spi/spi-mem.c >> @@ -184,7 +184,7 @@ bool spi_mem_default_supports_op(struct spi_mem *mem, >> return false; >> } >> >> - if (op->max_freq < mem->spi->max_speed_hz) { >> + if (op->max_freq && op->max_freq < mem->spi->max_speed_hz) { >> if (!spi_mem_controller_is_capable(ctlr, per_op_freq)) >> return false; >> } > > Yes, that seems to have been the issue. Great, thanks for testing. I'll soon send a v2, but I guess that's too late for this merge window. Regarding how to apply, I believe I'll have more spi-nand patches on top of that in the next cycle, so either I apply them with your Ack and share an immutable tag, or you apply it and give me one. Either ways works fine for me. It's more work to create the branch/tag so I can handle it (once we settle on the content ofc). Cheers, Miquèl
On 10/25/24 5:14 PM, Miquel Raynal wrote: cut > > Among all the spi-memory controllers, I believe all are capable of nit: SPI memory? or spi memory? but no "-" I think > changing the spi frequency on the fly. Some of the drivers do not make > any frequency setup though. And some others will derive a per-chip nit: s/per-chip/per chip? > pre-scaler value which will be used forever. nit: s/pre-scaler/prescaler? cut > index 8963f236911b..379c048b2eb4 100644 > --- a/include/linux/spi/spi-mem.h > +++ b/include/linux/spi/spi-mem.h > @@ -306,10 +306,12 @@ struct spi_controller_mem_ops { > * struct spi_controller_mem_caps - SPI memory controller capabilities > * @dtr: Supports DTR operations > * @ecc: Supports operations with error correction > + * @per_op_freq: Supports per-operation frequency switching nit: s/per-operation/per operation? If you fix the bug that you identified you can add my R-b tag, regardless if you address these nits or not: Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>
Hi Tudor, >> Among all the spi-memory controllers, I believe all are capable of > > nit: SPI memory? or spi memory? but no "-" I think ... >> changing the spi frequency on the fly. Some of the drivers do not make >> any frequency setup though. And some others will derive a per-chip > > nit: s/per-chip/per chip? ... >> pre-scaler value which will be used forever. > > nit: s/pre-scaler/prescaler? ... >> + * @per_op_freq: Supports per-operation frequency switching > > nit: s/per-operation/per operation? > > If you fix the bug that you identified you can add my R-b tag, > regardless if you address these nits or not: > > Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org> No idea why I like '-' so much. I removed them all as advised :-) Thanks! Miquèl
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index ab650ae953bb..102d351c3d04 100644 --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -184,6 +184,11 @@ bool spi_mem_default_supports_op(struct spi_mem *mem, return false; } + if (op->max_freq < mem->spi->max_speed_hz) { + if (!spi_mem_controller_is_capable(ctlr, per_op_freq)) + return false; + } + return spi_mem_check_buswidth(mem, op); } 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 8963f236911b..379c048b2eb4 100644 --- a/include/linux/spi/spi-mem.h +++ b/include/linux/spi/spi-mem.h @@ -306,10 +306,12 @@ struct spi_controller_mem_ops { * struct spi_controller_mem_caps - SPI memory controller capabilities * @dtr: Supports DTR operations * @ecc: Supports operations with error correction + * @per_op_freq: Supports per-operation frequency switching */ struct spi_controller_mem_caps { bool dtr; bool ecc; + bool per_op_freq; }; #define spi_mem_controller_is_capable(ctlr, cap) \
There are spi devices with multiple frequency limitations depending on the invoked command. We probably do not want to afford running at the lowest supported frequency all the time, so if we want to get the most of our hardware, we need to allow per-operation frequency limitations. Among all the spi-memory controllers, I believe all are capable of changing the spi frequency on the fly. Some of the drivers do not make any frequency setup though. And some others will derive a per-chip pre-scaler value which will be used forever. Actually changing the frequency on the fly is something new in Linux, so we need to carefully flag the drivers which do and do not support it. A controller capability is created for that, and the presence for this capability will always be checked before accepting such pattern. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/spi/spi-mem.c | 5 +++++ include/linux/spi/spi-mem.h | 2 ++ 2 files changed, 7 insertions(+)