Re: [PATCH V2] spi: Update speed/mode on change

[Marek Vasut <marex@denx.de>]

On 7/2/21 8:28 PM, Da Xue wrote:
> On Fri, Jul 2, 2021 at 2:10 PM Marek Vasut <marex@denx.de> wrote:
> 
>> On 7/2/21 8:03 PM, Da Xue wrote:
>>> SF: Detected gd25lq128 with page size 256 Bytes, erase size 64 KiB, total
>>> 16 MiB
>>> SPI mode: 3
>>> meson_spifc spi@8c80: Cannot set mode (err=-19)
>>> Failed to initialize SPI flash at 0:0 (error -19)
>>>
>>> I added spi-rx-bus-width = <1> and spi-tx-bus-width = <1>
>>>
>>> On Fri, Jul 2, 2021 at 1:44 PM Marek Vasut <marex@denx.de> wrote:
>>>
>>>> On 7/2/21 7:24 PM, Da Xue wrote:
>>>>> Hi Marek,
>>>>>
>>>>> This patch breaks meson_spifc:
>>>>>
>>>>> SF: Detected gd25lq128 with page size 256 Bytes, erase size 64 KiB,
>> total
>>>>> 16 MiB
>>>>> meson_spifc spi@8c80: Cannot set mode (err=-19)
>>>>> Failed to initialize SPI flash at 0:0 (error -19)
>>>>>
>>>>> Best,
>>>>>
>>>>> Da
>>>>>
>>>>> On Wed, Jun 30, 2021 at 12:49 PM Tom Rini <trini@konsulko.com> wrote:
>>>>>
>>>>>> On Thu, Jun 10, 2021 at 02:00:00PM +0200, Marek Vasut wrote:
>>>>>>
>>>>>>> The spi_get_bus_and_cs() may be called on the same bus and chipselect
>>>>>>> with different frequency or mode. This is valid usecase, but the code
>>>>>>> fails to notify the controller of such a configuration change. Call
>>>>>>> spi_set_speed_mode() in case bus frequency or bus mode changed to let
>>>>>>> the controller update the configuration.
>>>>>>>
>>>>>>> The problem can easily be triggered using the sspi command:
>>>>>>> => sspi 0:0@1000
>>>>>>> => sspi 0:0@2000
>>>>>>> Without this patch, both transfers happen at 1000 Hz. With this
>> patch,
>>>>>>> the later transfer happens correctly at 2000 Hz.
>>>>>>>
>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>> Cc: Jagan Teki <jagan@amarulasolutions.com>
>>>>>>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>>>>>
>>>>>> Applied to u-boot/master, thanks!
>>>>>>
>>>>>> --
>>>>>> Tom
>>>>
>>>> Seems like you're hitting this code in drivers/spi/meson_spifc.c
>>>>
>>>> 250 static int meson_spifc_set_mode(struct udevice *dev, uint mode)
>>>> 251 {
>>>> 252         struct meson_spifc_priv *spifc = dev_get_priv(dev);
>>>> 253
>>>> 254         if (mode & (SPI_CPHA | SPI_RX_QUAD | SPI_RX_DUAL |
>>>> 255                     SPI_TX_QUAD | SPI_TX_DUAL))
>>>> 256                 return -ENODEV;
>>>>
>>>> (the -ENODEV code doesn't look right, it should be some -EOPNOTSUP or
>> so)
>>>>
>>>> Can you check which of the mode bits is set and triggers the condition ?
>>>>
>>>> I think you might be missing something like
>>>> spi-rx-bus-width = <1>;
>>>> spi-tx-bus-width = <1>;
>>>> in your DT, but that's a guess.
>>
>> Can you check which of the mode bits is set and triggers the condition ?
>>
>> Also, where in the DT did you add spi-rx-bus-width = <1> and
>> spi-tx-bus-width = <1> ?
>>
>> Finally, please do not top-post and keep the list on CC.
>>
> 
> My apologies about the top-posting.
> 
> --- a/arch/arm/dts/meson-gxl-s805x-libretech-ac.dts
> +++ b/arch/arm/dts/meson-gxl-s805x-libretech-ac.dts
> @@ -304,6 +304,8 @@
>                  compatible = "jedec,spi-nor";
>                  reg = <0>;
>                  spi-max-frequency = <80000000>;
> +               spi-rx-bus-width = <1>;
> +               spi-tx-bus-width = <1>;
>          };
>   };

That should do the trick. Can you check which of the mode bits is set in 
meson_spifc_set_mode() and triggers the ENODEV condition ?