Re: [PATCH 07/15] ARM: dts: Configure interconnect target module for dra7 sata

[Robin Murphy <robin.murphy@arm.com>]

On 2024-11-14 10:22 am, Romain Naour wrote:
> Hello,
> 
> Le 12/11/2024 à 15:15, Romain Naour a écrit :
>> Hello Tony, Roger, All,
>>
>> Le 26/01/2021 à 13:39, Tony Lindgren a écrit :
>>> We can now probe devices with device tree only configuration using
>>> ti-sysc interconnect target module driver. Let's configure the
>>> module, but keep the legacy "ti,hwmods" peroperty to avoid new boot
>>> time warnings. The legacy property will be removed in later patches
>>> together with the legacy platform data.
>>>
>>> Note that the old sysc register offset is wrong, the real offset is at
>>> 0x1100 as listed in TRM for SATA_SYSCONFIG register. Looks like we've been
>>> happily using sata on the bootloader configured sysconfig register and
>>> nobody noticed. Also the old register range for SATAMAC_wrapper registers
>>> is wrong at 7 while it should be 8. But that too seems harmless.
>>>
>>> There is also an L3 parent interconnect range that we don't seem to be
>>> using. That can be added as needed later on.
>>
>> Since the switch from a kernel 5.10 to 6.1, the dra7 (AM574x) sata interface
>> doesn't work as expected.
>>
>> Using a kernel 6.1 with a preformated ext4 SATA disc, any copied file will be
>> corrupted when the filesystem is umounted.
>>
>> mount /dev/sda1 /mnt
>> cp /<test_file> /mnt/
>> sync
>> sha256sum /mnt/<test_file> /<test_file>
>> <same hash>
>> umount /mnt
>>
>> mount /dev/sda1 /mnt
>> sha256sum /mnt/<test_file> /<test_file>
>> /mnt/<test_file> is corrupted.
>>
>> git bisect report 8af15365a368 ("ARM: dts: Configure interconnect target module
>> for dra7 sata") as the first bad commit [1] (merged in 5.13).
>>
>> While looking for existing SATA issue on dra7 SoC, I found this old patch:
>>
>> "On TI Platforms using LPAE, SATA breaks with 64-bit DMA. Restrict it to
>> 32-bit." [2].
>>
>> Even if it's not the correct fix, disabling 64-bit DMA allows to use the sata
>> disc correctly. The discussion about this issue seems to have stopped [3] and
>> the suggested change was never merged.
>>
>> The SATA port is unlikely not available on TI AM57 EVM boards or the beaglebone-AI.
>>
>> Any suggestion?
> 
> It seems we have to use a pseudo-bus to constrain sata dma size (see [1])

No, the target-module node already represents a parent "bus", at least 
as far as the DT abstraction cares - just add a dma-ranges property 
there. Anything which has a ranges property to represent passing MMIO 
traffic downstream can equally have dma-ranges to represent DMA traffic 
coming back upstream.

Thanks,
Robin.

> But the sata node is placed inside a "ti,sysc-omap4" node, it's not clear if we
> can do that.
> 
> target-module@40000 {			/* 0x4a140000, ap 31 06.0 */
>      compatible = "ti,sysc-omap4", "ti,sysc";
>      reg = <0x400fc 4>,
>              <0x41100 4>;
>      reg-names = "rev", "sysc";
>      ti,sysc-midle = <SYSC_IDLE_FORCE>,
>              <SYSC_IDLE_NO>,
>              <SYSC_IDLE_SMART>;
>      ti,sysc-sidle = <SYSC_IDLE_FORCE>,
>              <SYSC_IDLE_NO>,
>              <SYSC_IDLE_SMART>,
>              <SYSC_IDLE_SMART_WKUP>;
>      power-domains = <&prm_l3init>;
>      clocks = <&l3init_clkctrl DRA7_L3INIT_SATA_CLKCTRL 0>;
>      clock-names = "fck";
> 
>      #size-cells = <2>;
>      #address-cells = <2>; // [2] parent-bus-address
>      ranges = <0x0 0x0 0x40000 0x0 0x10000>;
> 
>      aux_bus: aux_bus {
>          #address-cells = <2>; // [1] child-bus-address
>          #size-cells = <2>; // [3] length
>          compatible = "simple-bus";
>          ranges;
>          dma-ranges = <0x0 0x0 0x0 0x0 0x1 0x00000000>;
>          /*           | [1]   |[2]    | [3] | */
>          /* dma-ranges = <child-bus-address, parent-bus-address, length> */
> 
>          sata: sata@0 {
>              compatible = "snps,dwc-ahci";
>              reg = <0x0 0x0 0x0 0x1100>, <0x0 0x0 0x0 0x8>;
>              interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
>              phys = <&sata_phy>;
>              phy-names = "sata-phy";
>              clocks = <&l3init_clkctrl DRA7_L3INIT_SATA_CLKCTRL 8>;
>              ports-implemented = <0x1>;
>          };
>      };
> };
> 
> Note: ti,sysc-omap4 doesn't allows anything than #address-cells = <1> and
> #size-cells = <1> before commit [2].
> 
> bus: ti-sysc: Remove open coded "ranges" parsing
> 
>      "ranges" is a standard property and we have common helper functions for
>      parsing it, so let's use them.
> 
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=2a2ab4d5206d25875e30a8a8153f0b4f3c951ee4
> 
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=41e4f807f85d02a44426b87e01ed98b191dbbf9d
> 
> Best regards,
> Romain
> 
> 
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=8af15365a36845c4c15d4c8046ddccff331d5263
>> [2] https://lore.kernel.org/all/20200206111728.6703-1-rogerq@ti.com/T/
>> [3] https://lore.kernel.org/lkml/c753a232-403d-6ed2-89fd-09476c887391@ti.com/
>>
>> Best regards,
>> Romain
>>
>>
>>>
>