diff mbox series

[V3,7/7] arm64: dts: imx8mp: add interconnect for hsio blk ctrl

Message ID 20220703091451.1416264-8-peng.fan@oss.nxp.com
State Superseded
Headers show
Series imx: blk-ctrl: Add interconnect for i.MX8MP | expand

Commit Message

Peng Fan (OSS) July 3, 2022, 9:14 a.m. UTC 
From: Peng Fan <peng.fan@nxp.com>

Add interconnect property for hsio blk ctrl

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Greg Ungerer March 27, 2023, 4:50 a.m. UTC | #1 
On 2/3/22 17:13, Peng Fan wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Add interconnect property for hsio blk ctrl
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
 > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 5 +++++
 > 1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> index 08bd57742294..9cceeeeb26be 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -1109,6 +1109,11 @@ hsio_blk_ctrl: blk-ctrl@32f10000 {
>  						<&pgc_hsiomix>, <&pgc_pcie_phy>;
>  				power-domain-names = "bus", "usb", "usb-phy1",
>  						     "usb-phy2", "pcie", "pcie-phy";
> +				interconnects = <&noc IMX8MP_ICM_NOC_PCIE &noc IMX8MP_ICN_HSIO>,
> +						<&noc IMX8MP_ICM_USB1 &noc IMX8MP_ICN_HSIO>,
> +						<&noc IMX8MP_ICM_USB2 &noc IMX8MP_ICN_HSIO>,
> +						<&noc IMX8MP_ICM_PCIE &noc IMX8MP_ICN_HSIO>;
> +				interconnect-names = "noc-pcie", "usb1", "usb2", "pcie";
>  				#power-domain-cells = <1>;
>  			};
>  		};

This change completely breaks USB for me on a new iMX8mp platform I am
working with. Before this change normal USB probe looks good:

    xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
    xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 1
    xhci-hcd xhci-hcd.0.auto: hcc params 0x0220fe6d hci version 0x110 quirks 0x0000000000010010
    xhci-hcd xhci-hcd.0.auto: irq 206, io mem 0x38100000
    xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
    xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 2
    xhci-hcd xhci-hcd.0.auto: Host supports USB 3.0 SuperSpeed
    usb usb1: New USB device found, idVendor=1d6b, idProduct=0002, bcdDevice= 6.03
    usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1
    usb usb1: Product: xHCI Host Controller
    usb usb1: Manufacturer: Linux 6.3.0-rc4-dirty xhci-hcd
    ....

But after this commit is applied, no USB probe messages at all.

USB worked fine in 6.0 for me, but when I switched up to 6.1 USB was broken,
I bisected to this as being the offending commit. This is still broken for me
in todays 6.3-rc4. If I revert this change (and only this change) USB works
again.

Any thoughts on why this breaks USB?

Regards
greg
Ahmad Fatoum March 27, 2023, 7:16 a.m. UTC | #2 
Hello Greg,

On 27.03.23 08:27, Alexander Stein wrote:
> Am Montag, 27. März 2023, 06:50:37 CEST schrieb Greg Ungerer:
>> Any thoughts on why this breaks USB?
> 
> Maybe you are missing CONFIG_INTERCONNECT_IMX8MP?

And if that's the case, did you check /sys/kernel/debug/devices_deferred
to see if there was any indication that this is the reason?

If you didn't find any hint there, you might want to place a
dev_err_probe with a suitable message at the place where -EPROBE_DEFER
was returned.

Cheers,
Ahmad

> 
> Best regards,
> Alexander
>
Greg Ungerer March 27, 2023, 7:51 a.m. UTC | #3 
Hi Alexander,

On 27/3/23 16:27, Alexander Stein wrote:
> Hi Greg,
> 
> Am Montag, 27. März 2023, 06:50:37 CEST schrieb Greg Ungerer:
>> On 2/3/22 17:13, Peng Fan wrote:
>>> From: Peng Fan <peng.fan@nxp.com>
>>>
>>> Add interconnect property for hsio blk ctrl
>>>
>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>> ---
>>>
>>   > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 5 +++++
>>   > 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
>>> b/arch/arm64/boot/dts/freescale/imx8mp.dtsi index
>>> 08bd57742294..9cceeeeb26be 100644
>>> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
>>> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
>>> @@ -1109,6 +1109,11 @@ hsio_blk_ctrl: blk-ctrl@32f10000 {
>>>
>>>   						<&pgc_hsiomix>,
> <&pgc_pcie_phy>;
>>>   				
>>>   				power-domain-names = "bus", "usb",
> "usb-phy1",
>>>   				
>>>   						     "usb-phy2",
> "pcie", "pcie-phy";
>>>
>>> +				interconnects = <&noc
> IMX8MP_ICM_NOC_PCIE &noc IMX8MP_ICN_HSIO>,
>>> +						<&noc
> IMX8MP_ICM_USB1 &noc IMX8MP_ICN_HSIO>,
>>> +						<&noc
> IMX8MP_ICM_USB2 &noc IMX8MP_ICN_HSIO>,
>>> +						<&noc
> IMX8MP_ICM_PCIE &noc IMX8MP_ICN_HSIO>;
>>> +				interconnect-names = "noc-pcie",
> "usb1", "usb2", "pcie";
>>>
>>>   				#power-domain-cells = <1>;
>>>   			
>>>   			};
>>>   		
>>>   		};
>>
>> This change completely breaks USB for me on a new iMX8mp platform I am
>> working with. Before this change normal USB probe looks good:
>>
>>      xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
>>      xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 1
>>      xhci-hcd xhci-hcd.0.auto: hcc params 0x0220fe6d hci version 0x110 quirks
>> 0x0000000000010010 xhci-hcd xhci-hcd.0.auto: irq 206, io mem 0x38100000
>>      xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
>>      xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 2
>>      xhci-hcd xhci-hcd.0.auto: Host supports USB 3.0 SuperSpeed
>>      usb usb1: New USB device found, idVendor=1d6b, idProduct=0002,
>> bcdDevice= 6.03 usb usb1: New USB device strings: Mfr=3, Product=2,
>> SerialNumber=1 usb usb1: Product: xHCI Host Controller
>>      usb usb1: Manufacturer: Linux 6.3.0-rc4-dirty xhci-hcd
>>      ....
>>
>> But after this commit is applied, no USB probe messages at all.
>>
>> USB worked fine in 6.0 for me, but when I switched up to 6.1 USB was broken,
>> I bisected to this as being the offending commit. This is still broken for
>> me in todays 6.3-rc4. If I revert this change (and only this change) USB
>> works again.
>>
>> Any thoughts on why this breaks USB?
> 
> Maybe you are missing CONFIG_INTERCONNECT_IMX8MP?

No, it is enabled. From my config:

     CONFIG_INTERCONNECT=y
     CONFIG_INTERCONNECT_IMX=y
     # CONFIG_INTERCONNECT_IMX8MM is not set
     # CONFIG_INTERCONNECT_IMX8MN is not set
     # CONFIG_INTERCONNECT_IMX8MQ is not set
     CONFIG_INTERCONNECT_IMX8MP=y

Regards
Greg
Greg Ungerer March 28, 2023, 2:34 p.m. UTC | #4 
Hi Marco,

On 28/3/23 23:51, Marco Felsch wrote:
> On 23-03-28, Marco Felsch wrote:
>> Hi Greg,
>>
>> On 23-03-28, Greg Ungerer wrote:
>>> Hi Marco,
>>>
>>> On 28/3/23 17:33, Marco Felsch wrote:
>>>> Hi Greg,
>>>>
>>>> On 23-03-27, Greg Ungerer wrote:
>>>>> Hi Ahmad,
>>>>>
>>>>> On 27/3/23 17:16, Ahmad Fatoum wrote:
>>>>>> On 27.03.23 08:27, Alexander Stein wrote:
>>>>>>> Am Montag, 27. März 2023, 06:50:37 CEST schrieb Greg Ungerer:
>>>>>>>> Any thoughts on why this breaks USB?
>>>>>>>
>>>>>>> Maybe you are missing CONFIG_INTERCONNECT_IMX8MP?
>>>>>>
>>>>>> And if that's the case, did you check /sys/kernel/debug/devices_deferred
>>>>>> to see if there was any indication that this is the reason?
>>>>>
>>>>> Yeah, it does:
>>>>>
>>>>>       # cat /sys/kernel/debug/devices_deferred
>>>>>       32f10100.usb	platform: supplier 32f10000.blk-ctrl not ready
>>>>>       32f10108.usb	platform: supplier 32f10000.blk-ctrl not ready
>>>>>       32ec0000.blk-ctrl	imx8m-blk-ctrl: failed to get noc entries
>>>>>       381f0040.usb-phy	platform: supplier 32f10000.blk-ctrl not ready
>>>>>       382f0040.usb-phy	platform: supplier 32f10000.blk-ctrl not ready
>>>>>       imx-pgc-domain.11	
>>>>>       imx-pgc-domain.12	
>>>>>       imx-pgc-domain.13	
>>>>>       38330000.blk-ctrl	platform: supplier imx-pgc-domain.11 not ready
>>>>>       32f10000.blk-ctrl	imx8mp-blk-ctrl: failed to get noc entries
>>>>>
>>>>> As far as I can tell blk-ctrl should be good:
>>>>>
>>>>>       #
>>>>>       # i.MX SoC drivers
>>>>>       #
>>>>>       CONFIG_IMX_GPCV2_PM_DOMAINS=y
>>>>>       CONFIG_SOC_IMX8M=y
>>>>>       # CONFIG_SOC_IMX9 is not set
>>>>>       CONFIG_IMX8M_BLK_CTRL=y
>>>>>       # end of i.MX SoC drivers
>>>>>
>>>>>
>>>>>> If you didn't find any hint there, you might want to place a
>>>>>> dev_err_probe with a suitable message at the place where -EPROBE_DEFER
>>>>>> was returned.
>>>>>
>>>>> I will try that.
>>>>
>>>> Can you check that CONFIG_ARM_IMX_BUS_DEVFREQ is enabled? This is the
>>>> noc/interconnect driver. This could also the problem for you vpu issue.
>>>
>>> I do not have that enabled. Enabling that fixes the USB probing.
>>> So that is good, thanks.
>>>
>>> It doesn't fix the other problem I mentioned with the vpu pgc nodes though.
>>> I do get some extra messages now with this enabled and the 6.1 kernel:
>>>
>>>      imx-pgc imx-pgc-domain.8: failed to command PGC
>>>      imx-pgc imx-pgc-domain.8: failed to command PGC
>>>      imx8m-blk-ctrl 38330000.blk-ctrl: deferred probe timeout, ignoring dependency
>>>      imx8m-blk-ctrl 38330000.blk-ctrl: error -110: failed to attach power domain "g1"
>>>      imx8m-blk-ctrl: probe of 38330000.blk-ctrl failed with error -110
>>
>> Okay, this seems more like a "real" issue not related to some missing
>> drivers. I followed the code and found a poll within the
>> imx_pgc_power_up() in gpcv2.c. Power-domain 8 is the vpumix domain which
>> is used as power-domain for the g1 power-domain. My assumption is that
>> this poll does run into the timeout. Maybe Peng can support you here
>> since I didn't had the time for to test the VPUs yet and he did the
>> integration patches.
>>
>> Just ignore the errors if you don't use the VPUs or disable the
>> blk-ctrl@38330000 node via status = "disabled".
> 
> I forgot to ask: Does your i.MX8MP have a VPU? There are i.MX8MP devices
> (don't know the name) which don't have support for certain IPs. If this

The hardware platform I have is using the MIMX8ML4CVNKZAB "i.MX 8M Plus QuadLite"
(https://www.nxp.com/part/MIMX8ML4CVNKZAB#/) which does not have the hardware
video encode/decoder module (like the "i.MX 8M Plus Quad" parts).


> is the case the bootloader will fixup your devicetree by disable the
> corresponding nodes, we call this feature-controller:
> 
> https://elixir.bootlin.com/barebox/latest/source/arch/arm/dts/imx8mp.dtsi
> 
> As you can see the imx8mp.dtsi is missing the feature bits for the VPU
> but you can check the i.mx8mm.dtsi. Here you can see that barebox will
> check the availability of the vpu:
> 
> https://elixir.bootlin.com/barebox/latest/source/arch/arm/dts/imx8mm.dtsi

Ok, thanks, I'll take a look.

Regards
Greg
Marco Felsch March 28, 2023, 3:11 p.m. UTC | #5 
Hi Greg,

On 23-03-29, Greg Ungerer wrote:
> Hi Marco,

...

> > I forgot to ask: Does your i.MX8MP have a VPU? There are i.MX8MP devices
> > (don't know the name) which don't have support for certain IPs. If this
> 
> The hardware platform I have is using the MIMX8ML4CVNKZAB "i.MX 8M Plus QuadLite"
> (https://www.nxp.com/part/MIMX8ML4CVNKZAB#/) which does not have the hardware
> video encode/decoder module (like the "i.MX 8M Plus Quad" parts).

and that's the problem :) You need to update your bootloader to a
version which support disabling the VPU nodes else you will always see
the errors.

> > is the case the bootloader will fixup your devicetree by disable the
> > corresponding nodes, we call this feature-controller:
> > 
> > https://elixir.bootlin.com/barebox/latest/source/arch/arm/dts/imx8mp.dtsi
> > 
> > As you can see the imx8mp.dtsi is missing the feature bits for the VPU
> > but you can check the i.mx8mm.dtsi. Here you can see that barebox will
> > check the availability of the vpu:
> > 
> > https://elixir.bootlin.com/barebox/latest/source/arch/arm/dts/imx8mm.dtsi
> 
> Ok, thanks, I'll take a look.

Patches are welcome if you use barebox :)

Regards,
  Marco
Greg Ungerer March 31, 2023, 5:55 a.m. UTC | #6 
Hi Marco,

On 29/3/23 01:11, Marco Felsch wrote:
> Hi Greg,
> 
> On 23-03-29, Greg Ungerer wrote:
>> Hi Marco,
> 
> ...
> 
>>> I forgot to ask: Does your i.MX8MP have a VPU? There are i.MX8MP devices
>>> (don't know the name) which don't have support for certain IPs. If this
>>
>> The hardware platform I have is using the MIMX8ML4CVNKZAB "i.MX 8M Plus QuadLite"
>> (https://www.nxp.com/part/MIMX8ML4CVNKZAB#/) which does not have the hardware
>> video encode/decoder module (like the "i.MX 8M Plus Quad" parts).
> 
> and that's the problem :) You need to update your bootloader to a
> version which support disabling the VPU nodes else you will always see
> the errors.

I agree this is the problem, I don't agree that the boot loader is the
only place to fix this :-)  I should be able to generate a working devicetree
blob from the kernel that is good, and ready to use no runtime changes
required I figure.

It is not overly difficult to break out the vpu nodes and have them
only included when you have a board that has the iMX8MP-quad with the
VPU hardware blocks.

Example patch attached.

Regards
Greg
Markus Niebel March 31, 2023, 7:45 a.m. UTC | #7 
Hi Greg,

Am Freitag, dem 31.03.2023 um 15:55 +1000 schrieb Greg Ungerer:
> Hi Marco,
> 
> On 29/3/23 01:11, Marco Felsch wrote:
> > Hi Greg,
> > 
> > On 23-03-29, Greg Ungerer wrote:
> > > Hi Marco,
> > 
> > ...
> > 
> > > > I forgot to ask: Does your i.MX8MP have a VPU? There are
> > > > i.MX8MP devices
> > > > (don't know the name) which don't have support for certain IPs.
> > > > If this
> > > 
> > > The hardware platform I have is using the MIMX8ML4CVNKZAB "i.MX
> > > 8M Plus QuadLite"
> > > (https://www.nxp.com/part/MIMX8ML4CVNKZAB#/) which does not have
> > > the hardware
> > > video encode/decoder module (like the "i.MX 8M Plus Quad" parts).
> > 
> > and that's the problem :) You need to update your bootloader to a
> > version which support disabling the VPU nodes else you will always
> > see
> > the errors.
> 
> I agree this is the problem, I don't agree that the boot loader is
> the
> only place to fix this :-)  I should be able to generate a working
> devicetree
> blob from the kernel that is good, and ready to use no runtime
> changes
> required I figure.
> 

Just to point out: the approach of run time fixing in boot loader is
used for the other i.MX8M SOC, too. If you know exactly what SOC type
is assembled, you could disable non available IP in the board part of
your tree.

> It is not overly difficult to break out the vpu nodes and have them
> only included when you have a board that has the iMX8MP-quad with the
> VPU hardware blocks.
> 

Depending on the SOC type there is more to look for than the VPU: core
count, ISP, NPU - just to mention a few. Current approach allows to
keep a single tree for all types.

Regards, Markus
Ahmad Fatoum March 31, 2023, 8:11 a.m. UTC | #8 
On 31.03.23 09:45, Markus Niebel wrote:
> Am Freitag, dem 31.03.2023 um 15:55 +1000 schrieb Greg Ungerer:
>> On 29/3/23 01:11, Marco Felsch wrote:
>>> On 23-03-29, Greg Ungerer wrote:
>> I agree this is the problem, I don't agree that the boot loader is
>> the
>> only place to fix this :-)  I should be able to generate a working
>> devicetree
>> blob from the kernel that is good, and ready to use no runtime
>> changes
>> required I figure.
>>
> 
> Just to point out: the approach of run time fixing in boot loader is
> used for the other i.MX8M SOC, too. If you know exactly what SOC type
> is assembled, you could disable non available IP in the board part of
> your tree.
> 
>> It is not overly difficult to break out the vpu nodes and have them
>> only included when you have a board that has the iMX8MP-quad with the
>> VPU hardware blocks.

This breaks out-of-tree DTs that include imx8mp.dtsi. Logic should be the
other way round: imx8mp.dtsi is full-featured SoC and any new includes
strip away, not add nodes.

> Depending on the SOC type there is more to look for than the VPU: core
> count, ISP, NPU - just to mention a few. Current approach allows to
> keep a single tree for all types.

+1.

@Greg, does your board always ship with an i.MX8MPLite? If so, just
disable VPUs in your board DT.

If it ships with either VPUs available or not and you don't want to do
bootloader fixups, you may want to check out Kbuild's ability to apply
DT overlays at build time. This would give you separate DTs for each
variant while not having an extra file for every combination.

Cheers,
Ahmad


> 
> Regards, Markus
>
Greg Ungerer March 31, 2023, 3:09 p.m. UTC | #9 
On 31/3/23 18:11, Ahmad Fatoum wrote:
> On 31.03.23 09:45, Markus Niebel wrote:
>> Am Freitag, dem 31.03.2023 um 15:55 +1000 schrieb Greg Ungerer:
>>> On 29/3/23 01:11, Marco Felsch wrote:
>>>> On 23-03-29, Greg Ungerer wrote:
>>> I agree this is the problem, I don't agree that the boot loader is
>>> the
>>> only place to fix this :-)  I should be able to generate a working
>>> devicetree
>>> blob from the kernel that is good, and ready to use no runtime
>>> changes
>>> required I figure.
>>>
>>
>> Just to point out: the approach of run time fixing in boot loader is
>> used for the other i.MX8M SOC, too. If you know exactly what SOC type
>> is assembled, you could disable non available IP in the board part of
>> your tree.
>>
>>> It is not overly difficult to break out the vpu nodes and have them
>>> only included when you have a board that has the iMX8MP-quad with the
>>> VPU hardware blocks.
> 
> This breaks out-of-tree DTs that include imx8mp.dtsi. Logic should be the
> other way round: imx8mp.dtsi is full-featured SoC and any new includes
> strip away, not add nodes.
> 
>> Depending on the SOC type there is more to look for than the VPU: core
>> count, ISP, NPU - just to mention a few. Current approach allows to
>> keep a single tree for all types.
> 
> +1.
> 
> @Greg, does your board always ship with an i.MX8MPLite? If so, just
> disable VPUs in your board DT.

Yes, it will only ever have the Lite. That is why I only want
to generate a devicetree blob without the VPU nodes.

Regards
Greg


> If it ships with either VPUs available or not and you don't want to do
> bootloader fixups, you may want to check out Kbuild's ability to apply
> DT overlays at build time. This would give you separate DTs for each
> variant while not having an extra file for every combination.
> 
> Cheers,
> Ahmad
> 
> 
>>
>> Regards, Markus
>>
>
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index 08bd57742294..9cceeeeb26be 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -1109,6 +1109,11 @@  hsio_blk_ctrl: blk-ctrl@32f10000 {
 						<&pgc_hsiomix>, <&pgc_pcie_phy>;
 				power-domain-names = "bus", "usb", "usb-phy1",
 						     "usb-phy2", "pcie", "pcie-phy";
+				interconnects = <&noc IMX8MP_ICM_NOC_PCIE &noc IMX8MP_ICN_HSIO>,
+						<&noc IMX8MP_ICM_USB1 &noc IMX8MP_ICN_HSIO>,
+						<&noc IMX8MP_ICM_USB2 &noc IMX8MP_ICN_HSIO>,
+						<&noc IMX8MP_ICM_PCIE &noc IMX8MP_ICN_HSIO>;
+				interconnect-names = "noc-pcie", "usb1", "usb2", "pcie";
 				#power-domain-cells = <1>;
 			};
 		};