On 8/22/21 6:22 PM, Marek Vasut wrote:

On 8/22/21 4:47 PM, Oskari Lemmelä wrote:

...

On 22.8.2021 14.50, Marek Vasut wrote:

...

On 8/22/21 9:41 AM, Oskari Lemmelä wrote:

...

Hi Marek,

I was bisecting SPI flash boot problem in rockpro64 board and commit e2e95e5e25421fb seems to broke it.

It seems that after speed and mode change SPL is unable to load BL31 anymore from SPI flash device. There is no errors it just hangs forever.

If I change default mode to 0 (CONFIG_SF_DEFAULT_MODE=0), loading BL31 seems to work. In that case spi_set_speed_mode is also called but only speed is changed from 1Mhz to 10Mhz.

So changing mode from 0 to 3 in SPL stage seems to be the problem.

Any idea what could be the problem?

See 8c6d8c3219 ("configs: libretech: set SPI mode to 0")

Rockchip SPI supports both SCLK polarity and phase config and mode 3 is working fine if uboot is booted from MMC. However RK3399 documentation says SPI should be disabled while modifying master settings (speed, mode and so on). So this could be rk_spi.c driver issue.

Hmm, I don't have any rockchip device, so I cannot help you with that part.

Are you sure the SPI mode 3 (default) is the correct mode in the first place ?

After further debugging I think I found the problem. struct dm_spi_bus speed and mode are not updated after new spi_set_speed_mode function call.

I tested and updating those values inside spi_set_speed_mode function fixes issues. And there is no need to duplicate code to set those after both calls.

diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c index d867b27806..fef567caad 100644 --- a/drivers/spi/spi-uclass.c +++ b/drivers/spi/spi-uclass.c @@ -25,10 +25,10 @@ DECLARE_GLOBAL_DATA_PTR;

static int spi_set_speed_mode(struct udevice *bus, int speed, int mode)  { -       struct dm_spi_ops *ops; +       struct dm_spi_ops *ops = spi_get_ops(bus); +       struct dm_spi_bus *spi = dev_get_uclass_priv(bus);         int ret;

-       ops = spi_get_ops(bus);         if (ops->set_speed)                 ret = ops->set_speed(bus, speed);         else @@ -36,6 +36,8 @@ static int spi_set_speed_mode(struct udevice *bus, int speed, int mode)         if (ret) {                 dev_err(bus, "Cannot set speed (err=%d)\n", ret);                 return ret; +       } else { +               spi->speed = speed;         }

if (ops->set_mode) @@ -45,6 +47,8 @@ static int spi_set_speed_mode(struct udevice *bus, int speed, int mode)         if (ret) {                 dev_err(bus, "Cannot set mode (err=%d)\n", ret);                 return ret; +       } else { +               spi->mode = mode;         }

return 0; @@ -75,9 +79,6 @@ int dm_spi_claim_bus(struct udevice *dev)

if (ret)                         return log_ret(ret); - -               spi->speed = speed; -               spi->mode = mode;         }

return log_ret(ops->claim_bus ? ops->claim_bus(dev) : 0);

Oskari