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

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

[Marek Vasut]

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")

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

[Oskari Lemmelä]

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.

Oskari

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

[Marek Vasut]

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 ?

Re: 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