> -----Original Message-----
> From: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> Sent: Wednesday, August 24, 2022 4:56 PM
> To: Jun Li <jun.li@xxxxxxx>; shawnguo@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx;
> kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx
> Cc: marex@xxxxxxx; devicetree@xxxxxxxxxxxxxxx; Peng Fan <peng.fan@xxxxxxx>;
> Markus.Niebel@xxxxxxxxxxxxxxx; laurent.pinchart@xxxxxxxxxxxxxxxx;
> paul.elder@xxxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>;
> aford173@xxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH] arm64: dts: imx8mp: correct usb clocks
>
> Am Mittwoch, dem 24.08.2022 um 16:10 +0800 schrieb Li Jun:
> > After commit cf7f3f4fa9e5 ("clk: imx8mp: fix usb_root_clk parent"),
> > usb_root_clk is no longer for suspend clock so update dts accordingly
> > to use right bus clock and suspend clock.
> >
> So who is keeping IMX8MP_CLK_USB_ROOT enabled after this change? What is
> clocked by this and is it safe to disable while the USB subsystem is in working
> state? I see that things may still work, due to the shared gate with the
> suspend clock, but we should really try to model the DT after the HW. Especially
> since this is a ABI breaking change it should be right this time, so a unused
> USB_ROOT clock looks very suspicious.
The related clk driver change is [1]. I should put it in one patch set.
Before my change this shared clock gate is defined only for USB bus,
it will be off while system sleep even USB wakeup is enabled, this cause
the suspend clock is gated too, so USB remote wakeup cannot work.
In case system sleep without USB wakeup enabled, the shared gate
will be really off. Shared clock gate is matching the real HW.
You mean I should just simply define this gate only for suspend clock
instead of define 2 clock gates(then share)?
[1] https://patchwork.kernel.org/project/linux-clk/patch/1661328262-3867-2-git-send-email-jun.li@xxxxxxx/
Li Jun
>
> Regards,
> Lucas
>
> > Signed-off-by: Li Jun <jun.li@xxxxxxx>
> > ---
> > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > index fe178b7d063c..2f18778a057f 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > @@ -1169,7 +1169,7 @@ usb3_0: usb@32f10100 {
> > reg = <0x32f10100 0x8>,
> > <0x381f0000 0x20>;
> > clocks = <&clk IMX8MP_CLK_HSIO_ROOT>,
> > - <&clk IMX8MP_CLK_USB_ROOT>;
> > + <&clk IMX8MP_CLK_USB_SUSP>;
> > clock-names = "hsio", "suspend";
> > interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>;
> > power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB>; @@
> -1182,9
> > +1182,9 @@ usb3_0: usb@32f10100 {
> > usb_dwc3_0: usb@38100000 {
> > compatible = "snps,dwc3";
> > reg = <0x38100000 0x10000>;
> > - clocks = <&clk IMX8MP_CLK_HSIO_AXI>,
> > + clocks = <&clk IMX8MP_CLK_USB_ROOT>,
> > <&clk IMX8MP_CLK_USB_CORE_REF>,
> > - <&clk IMX8MP_CLK_USB_ROOT>;
> > + <&clk IMX8MP_CLK_USB_SUSP>;
> > clock-names = "bus_early", "ref", "suspend";
> > interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
> > phys = <&usb3_phy0>, <&usb3_phy0>; @@ -1211,7 +1211,7 @@
> usb3_1:
> > usb@32f10108 {
> > reg = <0x32f10108 0x8>,
> > <0x382f0000 0x20>;
> > clocks = <&clk IMX8MP_CLK_HSIO_ROOT>,
> > - <&clk IMX8MP_CLK_USB_ROOT>;
> > + <&clk IMX8MP_CLK_USB_SUSP>;
> > clock-names = "hsio", "suspend";
> > interrupts = <GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>;
> > power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB>; @@
> -1224,9
> > +1224,9 @@ usb3_1: usb@32f10108 {
> > usb_dwc3_1: usb@38200000 {
> > compatible = "snps,dwc3";
> > reg = <0x38200000 0x10000>;
> > - clocks = <&clk IMX8MP_CLK_HSIO_AXI>,
> > + clocks = <&clk IMX8MP_CLK_USB_ROOT>,
> > <&clk IMX8MP_CLK_USB_CORE_REF>,
> > - <&clk IMX8MP_CLK_USB_ROOT>;
> > + <&clk IMX8MP_CLK_USB_SUSP>;
> > clock-names = "bus_early", "ref", "suspend";
> > interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
> > phys = <&usb3_phy1>, <&usb3_phy1>;
>
