SPL SPI boot problem after commit "spi: Update speed/mode on change"

[Oskari Lemmelä]

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