Re: [PATCH 1/1] arm64: dts: freescale: ls208xa: correct USB compatible strings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] arm64: dts: freescale: missing dma-coherent on ls208xa usb0 and usb1 nodes
- [High] arm64: dts: freescale: missing iommus property on ls208xa usb0 and usb1 nodes
--

commit 299ff7e71139430a2f497142fe0c4f1de05bde95
Author: Frank Li <Frank.Li@nxp.com>

arm64: dts: freescale: ls208xa: correct USB compatible strings

This commit updates the usb0 and usb1 compatible strings in
fsl-ls208xa.dtsi from "snps,dwc3" to "fsl,ls208xa-dwc3" with a
"fsl,ls1028a-dwc3" fallback so that the controller programs the
Layerscape-specific gsbuscfg0 value.

> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
> index cc1a64e63ed55..3b0a26bf609cc 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
> @@ -1288,7 +1288,7 @@ bus: bus {
>  			dma-ranges = <0x0 0x0 0x0 0x0 0x100 0x00000000>;
>  
>  			usb0: usb@3100000 {
> -				compatible = "snps,dwc3";
> +				compatible = "fsl,ls208xa-dwc3", "fsl,ls1028a-dwc3";
>  				reg = <0x0 0x3100000 0x0 0x10000>;
>  				interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>;
>  				dr_mode = "host";
> @@ -1299,7 +1299,7 @@ usb0: usb@3100000 {
>  			};
>  
>  			usb1: usb@3110000 {
> -				compatible = "snps,dwc3";
> +				compatible = "fsl,ls208xa-dwc3", "fsl,ls1028a-dwc3";
>  				reg = <0x0 0x3110000 0x0 0x10000>;
>  				interrupts = <GIC_SPI 81 IRQ_TYPE_LEVEL_HIGH>;
>  				dr_mode = "host";

[Severity: High]
Should usb0 (usb@3100000) and usb1 (usb@3110000) in
arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi also have the dma-coherent
property when switching to "fsl,ls1028a-dwc3"?

When usb0 and usb1 match "fsl,ls1028a-dwc3" in dwc3_generic_driver
(drivers/usb/dwc3/dwc3-generic-plat.c), they select fsl_ls1028_dwc3:

drivers/usb/dwc3/dwc3-generic-plat.c:
    static const struct dwc3_generic_config fsl_ls1028_dwc3 = {
        .properties.gsbuscfg0_reqinfo = 0x2222,
    };

During probe via dwc3_core_probe() -> dwc3_get_software_properties() ->
dwc3_core_init() -> dwc3_config_soc_bus() in drivers/usb/dwc3/core.c, the
driver writes 0x2222 to DWC3_GSBUSCFG0:

drivers/usb/dwc3/core.c:dwc3_config_soc_bus()
    if (dwc->gsbuscfg0_reqinfo != DWC3_GSBUSCFG0_REQINFO_UNSPECIFIED) {
        u32 reg;

        reg = dwc3_readl(dwc, DWC3_GSBUSCFG0);
        reg &= ~DWC3_GSBUSCFG0_REQINFO(~0);
        reg |= DWC3_GSBUSCFG0_REQINFO(dwc->gsbuscfg0_reqinfo);
        dwc3_writel(dwc, DWC3_GSBUSCFG0, reg);
    }

As noted in commit 1c97fc901fb6 ("usb: dwc3: dwc3-generic-plat: Add
layerscape dwc3 support"), this configures the controller to issue
cache-coherent snooping transactions.

However, unlike the other Layerscape device trees updated in commit
7b204755ebc214 ("arm64: dts: layerscape: add dma-coherent for usb node")
(fsl-ls1012a.dtsi, fsl-ls1028a.dtsi, fsl-ls1043a.dtsi, fsl-ls1046a.dtsi,
fsl-ls1088a.dtsi, and fsl-lx216x.dtsi) and sibling masters in
fsl-ls208xa.dtsi (sata0, sata1, pcie1..4), neither usb0, usb1, nor their
parent bus or soc nodes have dma-coherent.

As a result, of_dma_is_coherent() in drivers/of/address.c returns false for
the dwc3 sysdev and the child xhci-hcd device:

drivers/of/address.c:of_dma_is_coherent()
    while (node) {
        if (of_property_read_bool(node, "dma-coherent"))
            return true;

        if (of_property_read_bool(node, "dma-noncoherent"))
            return false;

        node = of_get_next_dma_parent(node);
    }
    return dma_default_coherent;

Won't this cause a coherency mismatch where the controller snoops caches
while Linux maps coherent buffers (such as dwc3 event buffers and xhci
rings) as Normal Non-Cacheable and omits IOMMU_CACHE in the stage-1 page
tables?

[Severity: High]
Could removing "snps,dwc3" from usb0 (usb@3100000) and usb1 (usb@3110000)
without adding iommus properties also break USB transfers when booting with
U-Boot?

In arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi, the arm,mmu-500 node
(smmu: iommu@5000000) is enabled:

arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi:
    smmu: iommu@5000000 {
        compatible = "arm,mmu-500";
        reg = <0 0x5000000 0 0x800000>;
        #global-interrupts = <12>;
        #iommu-cells = <1>;
        stream-match-mask = <0x7c00>;
        dma-coherent;

On Layerscape platforms with an smmu, U-Boot programs the hardware stream
IDs for the USB controllers so their traffic routes through iommu@5000000,
and U-Boot's device tree fixup dynamically injects the iommus property by
searching for nodes matching compatible = "snps,dwc3".

When commit 7b204755ebc214 ("arm64: dts: layerscape: add dma-coherent for
usb node") removed "snps,dwc3" from the other smmu-equipped Layerscape
device trees (fsl-ls1028a.dtsi, fsl-ls1088a.dtsi, and fsl-lx216x.dtsi), it
added iommus = <&smmu 1> to usb0 and iommus = <&smmu 2> to usb1:

arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi:
    usb0: usb@3100000 {
        compatible = "fsl,ls1088a-dwc3", "fsl,ls1028a-dwc3";
        reg = <0x0 0x3100000 0x0 0x10000>;
        interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>;
        dr_mode = "host";
        iommus = <&smmu 1>;
        dma-coherent;

Without "snps,dwc3" or explicit iommus = <&smmu 1> and iommus = <&smmu 2>
properties on usb0 and usb1 in fsl-ls208xa.dtsi, won't U-Boot's fixup skip
both nodes, leaving them unattached to smmu in Linux while hardware still
routes their stream IDs through iommu@5000000 and triggers unmatched-stream
faults?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021204516.4031644-1-Frank.Li@oss.nxp.com?part=1