Hi Igor,
On 2026-09-30T18:33:24, Igor Velkov via B4 Relay
<[email protected]> wrote:
> spi: Probe a device tree device at the requested speed
>
> `sf probe <bus>:<cs> <speed>` ignores the speed for a flash that has a
> device tree node. _spi_get_bus_and_cs() probes the device, and with it
> the flash reads its JEDEC ID, at spi-max-frequency from the device tree.
> The requested speed is set only after the probe, and the slave keeps
> max_hz from the device tree, so the next claim of the bus goes back to
> that frequency.
>
> A device bound without a device tree node already gets the requested
> speed and mode before the probe. Do the same for a device from the
> device tree, for the duration of the probe.
>
> On ROCK Pi 4A the device tree sets spi-max-frequency = <108000000> and
> the flash does not answer at the resulting clock, at any requested
> speed:
>
> => sf probe 1:0 1000000
> jedec_spi_nor flash@0: unrecognized JEDEC id bytes: ff, ff, ff
>
> [...]
>
> drivers/spi/spi-uclass.c | 9 +++++++++
> test/dm/spi.c | 20 ++++++++++++++++++++
> 2 files changed, 29 insertions(+)
> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> @@ -533,8 +533,17 @@ int _spi_get_bus_and_cs(int busnum, int cs, int speed,
> int mode,
> + /* Probe at the caller's speed and mode: a flash reads its ID
> here */
> + if (speed && !created) {
> + plat->max_hz = speed;
> + plat->mode = mode;
> + }
> ret = device_probe(dev);
> + plat->max_hz = dt_max_hz;
> + plat->mode = dt_mode;
The commit message only talks about speed, but this also replaces the
mode... spi_child_pre_probe() copies plat->mode into slave->mode, so
restoring plat->mode afterwards does not undo it: slave->mode keeps
the caller's value while the device stays probed. With 'sf probe 1:0
1000000' the mode is CONFIG_SF_DEFAULT_MODE (normally 0), so the
device-tree bits are lost: SPI_RX_QUAD / SPI_TX_QUAD from
spi-rx/tx-bus-width, SPI_NO_TX / SPI_NO_RX, SPI_CS_HIGH and so on.
spi_nor_scan() checks spi->mode & SPI_RX_QUAD when picking the read
op, so a quad flash would quietly drop to 1-1-1. Before this patch,
slave->mode kept the DT value and only the controller was switched,
via spi_set_speed_mode() further down.
Please can you override just max_hz here? If the mode really must be
honoured, perhaps merge only the SPI_CPHA | SPI_CPOL bits into the DT
value. What do you think?
> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> @@ -533,8 +533,17 @@ int _spi_get_bus_and_cs(int busnum, int cs, int speed,
> int mode,
> + if (speed && !created) {
> + plat->max_hz = speed;
This affects every spi_flash_probe() caller with a DT node, not just
sf probe. Several boards and drivers pass CONFIG_SF_DEFAULT_SPEED
(default 1MHz) and CONFIG_SF_DEFAULT_MODE, e.g. drivers/net/fm/fm.c,
drivers/net/phy/cortina.c, board/gardena/... and cmd/mvebu/bubt.c.
Their flashes are now probed at that speed and stay there, since
slave->max_hz keeps it, rather than running at spi-max-frequency as
they do today. I suspect this is intended, since the caller asked for
that speed, but please can you say so in the commit message so board
maintainers are not surprised by a slower flash.
Also, plat->max_hz now holds the DT value while slave->max_hz holds
the requested one. Just to check: should a requested speed above
spi-max-frequency be honoured here, rather than capped at it?
> diff --git a/test/dm/spi.c b/test/dm/spi.c
> @@ -91,6 +91,26 @@ static int dm_test_spi_find(struct unit_test_state *uts)
> + ut_assertok(_spi_get_bus_and_cs(busnum, cs, speed, mode, NULL, NULL,
> + &bus, &slave));
> + ut_asserteq(speed, slave->max_hz);
Please can you also check that the device-tree state survives, e.g.
that plat->max_hz is still 40000000 after the call and slave->mode
keeps its DT bits? That would have caught the mode problem above.
Regards,
Simon
