[PATCH v6 5/7] spi: mxic: Add support for swapping byte

[Tudor Ambarus]

Hi, Jaime, Michael,

Allow me to try to intermediate this :).

On 12/4/23 06:44, liao jaime wrote:
> Hi Michael
> 
> 
>>
>> Hi Jaime,
>>
>>>>> Some SPI-NOR flash swap the bytes on a 16-bit boundary when
>>>>> configured in Octal DTR mode. It means data format D0 D1 D2 D3
>>>>> would be swapped to D1 D0 D3 D2. So that whether controller
>>>>> support swapping bytes should be checked before enable Octal
>>>>> DTR mode. Add swap byte support on a 16-bit boundary when
>>>>> configured in Octal DTR mode for Macronix xSPI host controller
>>>>> dirver.
>>>>>
>>>>> Signed-off-by: JaimeLiao <jaimeliao at mxic.com.tw>
>>>>> ---
>>>>>  drivers/spi/spi-mxic.c | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
>>>>> index 60c9f3048ac9..085c9037d6f5 100644
>>>>> --- a/drivers/spi/spi-mxic.c
>>>>> +++ b/drivers/spi/spi-mxic.c
>>>>> @@ -572,6 +572,7 @@ static const struct spi_controller_mem_ops
>>>>> mxic_spi_mem_ops = {
>>>>>
>>>>>  static const struct spi_controller_mem_caps mxic_spi_mem_caps = {
>>>>>       .dtr = true,
>>>>> +     .dtr_swab16 = true,
>>>>>       .ecc = true,
>>>>>  };
>>>>
>>>> I'm confused. How can this swap the bytes depending on the flashes
>>>> requirements? I.e. the controller should look at the spi-mem operation
>>>> and either swap the bytes or it should leave them as is.
>>> Byte order in 8D-8D-8D mode could get by parsing BFPT 18th Dword 31th
>>> bit.
>>> I think flash driver should notify controller driver and verify whether
>>> the
>>> controller driver support byte swap functionality. If flash requires
>>> byte swap
>>> in octal DTR mode but the controller driver doesn't support it, then
>>> octal DTR
>>> should not be enabled.
>>
>> Correct. But the above means, the controller *supports* swapping the
>> bytes
>> if the flash requires it. But where is the code in the spi-mxic driver
>> which
>> actually controls the swapping depending on the dtr_swab16 property in
>> spi_mem_op?
> I add it additionally because of controller driver should enable swap byte
> feature on special case only.
> I programed data in 1s-1s-1s mode and read back in 8d-8d-8d mode
> with controller swapped byte.
> Then check program data(in 1s-1s-1s) and read data(in 8d-8d-8d).
> In normal case, 8d-8d-8d prgram then 8d-8d-8d read, controller driver
> should disable swapping byte feature.
> So that I don't think controller driver swapping byte feature should depends
> on dtr_swab16.
> 

I assume Michael's concerns are that if we don't have a controller that
actually supports and implements dtr_swab16, then we risk that all the
SPI NOR related code to be removed on a further cleanup, as in kernel we
don't add support that's not used.

Then Michael rightly asks about the details of this patch. Why adding a
dtr_swab16 mem caps in this driver if it's not used? We expect a
register write, specifying the controller to swap bytes. You missed
that. Can this controller swap the bytes? If yes, what needs to be
configured in order to swap the bytes?

Michael,

I know for sure that mchp's sama7g5 xSPI controller can swap the bytes
back on the fly. What I did was to check the dtr_swab16 bool at runtime
in the exec_op() method and if it was true, I had to write a
configuration bit. I can't remember why I haven't posted a v2 on that
series, maybe I'll do it if I find some time or I ever get bored. I have
a board. Anyway, knowing that there's a board out there, sama7g5ek, that
contains a macronix flash that swaps bytes and also have a controller
that can swap them back, would it be acceptable to queue just the SPI
NOR patches for now?

Thanks,
ta