Message ID | 20220409075147.136187-1-linux@fw-web.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | arm64: dts: rockchip: Fix clocks for rk356x usb | expand |
Hi, > Gesendet: Samstag, 09. April 2022 um 09:51 Uhr > Von: "Frank Wunderlich" <linux@fw-web.de> > - clock-names = "ref_clk", "suspend_clk", > - "bus_clk"; > + clock-names = "ref", "suspend_clk", > + "bus_early"; > dr_mode = "host"; > phy_type = "utmi_wide"; > power-domains = <&power RK3568_PD_PIPE>; > @@ -280,8 +280,8 @@ usb_host1_xhci: usb@fd000000 { > interrupts = <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&cru CLK_USB3OTG1_REF>, <&cru CLK_USB3OTG1_SUSPEND>, > <&cru ACLK_USB3OTG1>; > - clock-names = "ref_clk", "suspend_clk", > - "bus_clk"; > + clock-names = "ref", "suspend_clk", > + "bus_early"; > dr_mode = "host"; this is the patch breaking it: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=33fb697ec7e58c4f9b6a68d2786441189cd2df92 suspend clock needs to be renamed too from "suspend_clk" to "suspend" else i get this error on poweroff xhci-hcd xhci-hcd.1.auto: Host halt failed, -110 regards Frank
Am Samstag, 9. April 2022, 12:14:28 CEST schrieb Frank Wunderlich: > Hi, > > > Gesendet: Samstag, 09. April 2022 um 09:51 Uhr > > Von: "Frank Wunderlich" <linux@fw-web.de> > > > - clock-names = "ref_clk", "suspend_clk", > > - "bus_clk"; > > + clock-names = "ref", "suspend_clk", > > + "bus_early"; > > dr_mode = "host"; > > phy_type = "utmi_wide"; > > power-domains = <&power RK3568_PD_PIPE>; > > @@ -280,8 +280,8 @@ usb_host1_xhci: usb@fd000000 { > > interrupts = <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH>; > > clocks = <&cru CLK_USB3OTG1_REF>, <&cru CLK_USB3OTG1_SUSPEND>, > > <&cru ACLK_USB3OTG1>; > > - clock-names = "ref_clk", "suspend_clk", > > - "bus_clk"; > > + clock-names = "ref", "suspend_clk", > > + "bus_early"; > > dr_mode = "host"; > > this is the patch breaking it: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=33fb697ec7e58c4f9b6a68d2786441189cd2df92 > > suspend clock needs to be renamed too from "suspend_clk" to "suspend" > > else i get this error on poweroff > > xhci-hcd xhci-hcd.1.auto: Host halt failed, -110 > > regards Frank ok, so do you want to send a v2 including that change? Alternatively I can also add this change when applying. Also for educational purposes, the format for referencing a commit you're fixing would be Fixes: 33fb697ec7e5 ("usb: dwc3: Get clocks individually") Signed-off-by: .... Heiko
> Gesendet: Samstag, 09. April 2022 um 12:23 Uhr > Von: "Heiko Stuebner" <heiko@sntech.de> > ok, so do you want to send a v2 including that change? > Alternatively I can also add this change when applying. imho the best way will be that peter includes my patch in his "arm64: dts: rockchip: add rk356x dwc3 usb3 nodes" https://patchwork.kernel.org/project/linux-rockchip/patch/20220408151237.3165046-4-pgwipeout@gmail.com/ i just posted the fix for those who want to test his series on 5.18 including himself. but of course if this is not the right way, i post a v2. > Also for educational purposes, the format for referencing a commit > you're fixing would be > > Fixes: 33fb697ec7e5 ("usb: dwc3: Get clocks individually") > Signed-off-by: .... as the patch not really broke current mainline state, i thought Fixes-tag is not right. Imho only the rk356x-usb-patch is not compatible with 5.18 due to this change, but this is not applied yet. regards Frank
Den 09.04.2022 kl. 12.32 skrev Frank Wunderlich: >> Gesendet: Samstag, 09. April 2022 um 12:23 Uhr >> Von: "Heiko Stuebner" <heiko@sntech.de> > >> ok, so do you want to send a v2 including that change? >> Alternatively I can also add this change when applying. > imho the best way will be that peter includes my patch in his > > "arm64: dts: rockchip: add rk356x dwc3 usb3 nodes" > > https://patchwork.kernel.org/project/linux-rockchip/patch/20220408151237.3165046-4-pgwipeout@gmail.com/ > > i just posted the fix for those who want to test his series on 5.18 including himself. So the issue is only with usb 3 ports, not usb 2 ports? > > but of course if this is not the right way, i post a v2. > >> Also for educational purposes, the format for referencing a commit >> you're fixing would be >> >> Fixes: 33fb697ec7e5 ("usb: dwc3: Get clocks individually") >> Signed-off-by: .... > as the patch not really broke current mainline state, i thought Fixes-tag is not right. > Imho only the rk356x-usb-patch is not compatible with 5.18 due to this change, but this is not applied yet. > > regards Frank > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
Hi > Gesendet: Samstag, 09. April 2022 um 12:40 Uhr > Von: "Dan Johansen" <strit@manjaro.org> > So the issue is only with usb 3 ports, not usb 2 ports? my board has no standalone usb2-ports. usb2 is integrated into the usb3 ports (dual phy). here both were not working. afaik rk3566 has standalone usb2 ports that may not be broken, but i have no such board for testing. regards Frank
Am Samstag, 9. April 2022, 12:57:39 CEST schrieb Frank Wunderlich: > Hi > > Gesendet: Samstag, 09. April 2022 um 12:40 Uhr > > Von: "Dan Johansen" <strit@manjaro.org> > > > So the issue is only with usb 3 ports, not usb 2 ports? > > my board has no standalone usb2-ports. usb2 is integrated into the usb3 ports (dual phy). here both were not working. > > afaik rk3566 has standalone usb2 ports that may not be broken, but i have no such board for testing. As far as I understand the issue now after checking the code, this patch actually fixes the usb3 series from Peter, right? I.e. the usb-nodes that are fixed in this patch are not yet present in the main rk356x dtsi and only get added in "arm64: dts: rockchip: add rk356x dwc3 usb3 nodes" [0] As we don't want to add broken changes, this fix should squashed into a next version of the patch adding the nodes. Heiko [0] https://lore.kernel.org/r/20220408151237.3165046-4-pgwipeout@gmail.com
> Gesendet: Samstag, 09. April 2022 um 13:01 Uhr > Von: "Heiko Stuebner" <heiko@sntech.de> > As far as I understand the issue now after checking the code, this > patch actually fixes the usb3 series from Peter, right? right, due to the change the patches from 5.17 were no more directly compatible with 5.18. for my board without having standalone usb2-ports it breaks both as usb2 is passed through xhci controller. > I.e. the usb-nodes that are fixed in this patch are not yet present > in the main rk356x dtsi and only get added in > "arm64: dts: rockchip: add rk356x dwc3 usb3 nodes" [0] > > As we don't want to add broken changes, this fix should squashed > into a next version of the patch adding the nodes. right, that was my intention, but do not forget the suspend_clk change i've mentioned in [1] but to leave all know that this bug is known and how to solve it i've posted official patch but without fixes-tag as the "breaking" commit does not really break...more likely introduce a incompatibility for the not yet applied patchset. > [0] https://lore.kernel.org/r/20220408151237.3165046-4-pgwipeout@gmail.com [1] https://patchwork.kernel.org/comment/24809714/ regards Frank
On Sat, Apr 9, 2022 at 7:01 AM Heiko Stuebner <heiko@sntech.de> wrote: > > Am Samstag, 9. April 2022, 12:57:39 CEST schrieb Frank Wunderlich: > > Hi > > > Gesendet: Samstag, 09. April 2022 um 12:40 Uhr > > > Von: "Dan Johansen" <strit@manjaro.org> > > > > > So the issue is only with usb 3 ports, not usb 2 ports? > > > > my board has no standalone usb2-ports. usb2 is integrated into the usb3 ports (dual phy). here both were not working. > > > > afaik rk3566 has standalone usb2 ports that may not be broken, but i have no such board for testing. Good Morning, > > As far as I understand the issue now after checking the code, this > patch actually fixes the usb3 series from Peter, right? > > I.e. the usb-nodes that are fixed in this patch are not yet present > in the main rk356x dtsi and only get added in > "arm64: dts: rockchip: add rk356x dwc3 usb3 nodes" [0] > > As we don't want to add broken changes, this fix should squashed > into a next version of the patch adding the nodes. Thank you for reporting this, I will squash this fix in and add your signed-off. However the offending patch is in fact the clock separation patch, and it breaks backwards compatibility with the rk3328 dtsi which is why my series also is broken. The rockchip,dwc3.yaml needs to be fixed to align with the snps,dwc3.yaml, and both the rk3328 and rk3399 clock names updated. Also the offending clock separation patch needs a fix to grab the old clock names for rk3328 backwards compatibility to be retained. This might also be a good time to look into moving rk3399 to the core dwc3 driver? This is a delightful mess. > > > Heiko > > [0] https://lore.kernel.org/r/20220408151237.3165046-4-pgwipeout@gmail.com > > Very Respectfully, Peter Geis
On Sat, Apr 9, 2022 at 7:14 AM Peter Geis <pgwipeout@gmail.com> wrote: > > On Sat, Apr 9, 2022 at 7:01 AM Heiko Stuebner <heiko@sntech.de> wrote: > > > > Am Samstag, 9. April 2022, 12:57:39 CEST schrieb Frank Wunderlich: > > > Hi > > > > Gesendet: Samstag, 09. April 2022 um 12:40 Uhr > > > > Von: "Dan Johansen" <strit@manjaro.org> > > > > > > > So the issue is only with usb 3 ports, not usb 2 ports? > > > > > > my board has no standalone usb2-ports. usb2 is integrated into the usb3 ports (dual phy). here both were not working. > > > > > > afaik rk3566 has standalone usb2 ports that may not be broken, but i have no such board for testing. > > Good Morning, > > > > > As far as I understand the issue now after checking the code, this > > patch actually fixes the usb3 series from Peter, right? > > > > I.e. the usb-nodes that are fixed in this patch are not yet present > > in the main rk356x dtsi and only get added in > > "arm64: dts: rockchip: add rk356x dwc3 usb3 nodes" [0] > > > > As we don't want to add broken changes, this fix should squashed > > into a next version of the patch adding the nodes. > > Thank you for reporting this, I will squash this fix in and add your signed-off. > > However the offending patch is in fact the clock separation patch, and > it breaks backwards compatibility with the rk3328 dtsi which is why my > series also is broken. > > The rockchip,dwc3.yaml needs to be fixed to align with the > snps,dwc3.yaml, and both the rk3328 and rk3399 clock names updated. > Also the offending clock separation patch needs a fix to grab the old > clock names for rk3328 backwards compatibility to be retained. > > This might also be a good time to look into moving rk3399 to the core > dwc3 driver? > > This is a delightful mess. In the idea of getting this series to land, if all parties agree, I'll submit a patch that fixes the clock separation patch with this series and leave the naming as is for now. The renaming of clocks and alignment of everything can be addressed in a future series once discussion on how best to handle it has happened. Do you concur with this? > > > > > > > Heiko > > > > [0] https://lore.kernel.org/r/20220408151237.3165046-4-pgwipeout@gmail.com > > > > > > Very Respectfully, > Peter Geis
Am Samstag, 9. April 2022, 13:30:44 CEST schrieb Peter Geis: > On Sat, Apr 9, 2022 at 7:14 AM Peter Geis <pgwipeout@gmail.com> wrote: > > > > On Sat, Apr 9, 2022 at 7:01 AM Heiko Stuebner <heiko@sntech.de> wrote: > > > > > > Am Samstag, 9. April 2022, 12:57:39 CEST schrieb Frank Wunderlich: > > > > Hi > > > > > Gesendet: Samstag, 09. April 2022 um 12:40 Uhr > > > > > Von: "Dan Johansen" <strit@manjaro.org> > > > > > > > > > So the issue is only with usb 3 ports, not usb 2 ports? > > > > > > > > my board has no standalone usb2-ports. usb2 is integrated into the usb3 ports (dual phy). here both were not working. > > > > > > > > afaik rk3566 has standalone usb2 ports that may not be broken, but i have no such board for testing. > > > > Good Morning, > > > > > > > > As far as I understand the issue now after checking the code, this > > > patch actually fixes the usb3 series from Peter, right? > > > > > > I.e. the usb-nodes that are fixed in this patch are not yet present > > > in the main rk356x dtsi and only get added in > > > "arm64: dts: rockchip: add rk356x dwc3 usb3 nodes" [0] > > > > > > As we don't want to add broken changes, this fix should squashed > > > into a next version of the patch adding the nodes. > > > > Thank you for reporting this, I will squash this fix in and add your signed-off. > > > > However the offending patch is in fact the clock separation patch, and > > it breaks backwards compatibility with the rk3328 dtsi which is why my > > series also is broken. > > > > The rockchip,dwc3.yaml needs to be fixed to align with the > > snps,dwc3.yaml, and both the rk3328 and rk3399 clock names updated. > > Also the offending clock separation patch needs a fix to grab the old > > clock names for rk3328 backwards compatibility to be retained. > > > > This might also be a good time to look into moving rk3399 to the core > > dwc3 driver? > > > > This is a delightful mess. > > In the idea of getting this series to land, if all parties agree, I'll > submit a patch that fixes the clock separation patch with this series > and leave the naming as is for now. > The renaming of clocks and alignment of everything can be addressed in > a future series once discussion on how best to handle it has happened. > > Do you concur with this? I'm not sure about that ... i.e. adding known-broken changes (for the rk356x) feels somewhat wrong to me. Heiko
On Sat, Apr 9, 2022 at 7:35 AM Heiko Stuebner <heiko@sntech.de> wrote: > > Am Samstag, 9. April 2022, 13:30:44 CEST schrieb Peter Geis: > > On Sat, Apr 9, 2022 at 7:14 AM Peter Geis <pgwipeout@gmail.com> wrote: > > > > > > On Sat, Apr 9, 2022 at 7:01 AM Heiko Stuebner <heiko@sntech.de> wrote: > > > > > > > > Am Samstag, 9. April 2022, 12:57:39 CEST schrieb Frank Wunderlich: > > > > > Hi > > > > > > Gesendet: Samstag, 09. April 2022 um 12:40 Uhr > > > > > > Von: "Dan Johansen" <strit@manjaro.org> > > > > > > > > > > > So the issue is only with usb 3 ports, not usb 2 ports? > > > > > > > > > > my board has no standalone usb2-ports. usb2 is integrated into the usb3 ports (dual phy). here both were not working. > > > > > > > > > > afaik rk3566 has standalone usb2 ports that may not be broken, but i have no such board for testing. > > > > > > Good Morning, > > > > > > > > > > > As far as I understand the issue now after checking the code, this > > > > patch actually fixes the usb3 series from Peter, right? > > > > > > > > I.e. the usb-nodes that are fixed in this patch are not yet present > > > > in the main rk356x dtsi and only get added in > > > > "arm64: dts: rockchip: add rk356x dwc3 usb3 nodes" [0] > > > > > > > > As we don't want to add broken changes, this fix should squashed > > > > into a next version of the patch adding the nodes. > > > > > > Thank you for reporting this, I will squash this fix in and add your signed-off. > > > > > > However the offending patch is in fact the clock separation patch, and > > > it breaks backwards compatibility with the rk3328 dtsi which is why my > > > series also is broken. > > > > > > The rockchip,dwc3.yaml needs to be fixed to align with the > > > snps,dwc3.yaml, and both the rk3328 and rk3399 clock names updated. > > > Also the offending clock separation patch needs a fix to grab the old > > > clock names for rk3328 backwards compatibility to be retained. > > > > > > This might also be a good time to look into moving rk3399 to the core > > > dwc3 driver? > > > > > > This is a delightful mess. > > > > In the idea of getting this series to land, if all parties agree, I'll > > submit a patch that fixes the clock separation patch with this series > > and leave the naming as is for now. > > The renaming of clocks and alignment of everything can be addressed in > > a future series once discussion on how best to handle it has happened. > > > > Do you concur with this? > > I'm not sure about that ... i.e. adding known-broken changes > (for the rk356x) feels somewhat wrong to me. My series as it is complies with the current dt-binding for Rockchip. It just so happens the dt-binding describes two different devices that are handled by two different drivers, and the binding itself was wrong. This becomes a whole lot more intrusive if we decide to do this now, and the offending change still needs to be fixed if we want to retain backwards compatibility with rk3328.dtsi > > Heiko > >
Hi, so to not break the binding and other boards the right Patch should be like this +++ b/drivers/usb/dwc3/core.c @@ -1691,17 +1691,17 @@ static int dwc3_probe(struct platform_device *pdev) * Clocks are optional, but new DT platforms should support all * clocks as required by the DT-binding. */ - dwc->bus_clk = devm_clk_get_optional(dev, "bus_early"); + dwc->bus_clk = devm_clk_get_optional(dev, "bus_clk"); if (IS_ERR(dwc->bus_clk)) return dev_err_probe(dev, PTR_ERR(dwc->bus_clk), "could not get bus clock\n"); - dwc->ref_clk = devm_clk_get_optional(dev, "ref"); + dwc->ref_clk = devm_clk_get_optional(dev, "ref_clk"); if (IS_ERR(dwc->ref_clk)) return dev_err_probe(dev, PTR_ERR(dwc->ref_clk), "could not get ref clock\n"); - dwc->susp_clk = devm_clk_get_optional(dev, "suspend"); + dwc->susp_clk = devm_clk_get_optional(dev, "suspend_clk"); if (IS_ERR(dwc->susp_clk)) return dev_err_probe(dev, PTR_ERR(dwc->susp_clk), "could not get suspend clock\n"); but this needs fixing dts using the new clock names this is a link to the series moving from bulk_clk to named clocks: https://patchwork.kernel.org/project/linux-usb/patch/20220127200636.1456175-3-sean.anderson@seco.com/ regards Frank
On Sat, Apr 9, 2022 at 7:56 AM Frank Wunderlich <frank-w@public-files.de> wrote: > > Hi, > > so to not break the binding and other boards the right Patch should be like this > > +++ b/drivers/usb/dwc3/core.c > @@ -1691,17 +1691,17 @@ static int dwc3_probe(struct platform_device *pdev) > * Clocks are optional, but new DT platforms should support all > * clocks as required by the DT-binding. > */ > - dwc->bus_clk = devm_clk_get_optional(dev, "bus_early"); > + dwc->bus_clk = devm_clk_get_optional(dev, "bus_clk"); > if (IS_ERR(dwc->bus_clk)) > return dev_err_probe(dev, PTR_ERR(dwc->bus_clk), > "could not get bus clock\n"); > > - dwc->ref_clk = devm_clk_get_optional(dev, "ref"); > + dwc->ref_clk = devm_clk_get_optional(dev, "ref_clk"); > if (IS_ERR(dwc->ref_clk)) > return dev_err_probe(dev, PTR_ERR(dwc->ref_clk), > "could not get ref clock\n"); > > - dwc->susp_clk = devm_clk_get_optional(dev, "suspend"); > + dwc->susp_clk = devm_clk_get_optional(dev, "suspend_clk"); > if (IS_ERR(dwc->susp_clk)) > return dev_err_probe(dev, PTR_ERR(dwc->susp_clk), > "could not get suspend clock\n"); > > but this needs fixing dts using the new clock names > > this is a link to the series moving from bulk_clk to named clocks: > > https://patchwork.kernel.org/project/linux-usb/patch/20220127200636.1456175-3-sean.anderson@seco.com/ > > regards Frank I've submitted a fix for the backwards compatibility issue. https://patchwork.kernel.org/project/linux-rockchip/patch/20220409152116.3834354-1-pgwipeout@gmail.com/ This fix is standalone and necessary no matter which route we decide to go with this series (and the rk3328/rk3399 support as well). With this patch, dwc3 is functional on the rk356x as the series was submitted, so if we decide to fix everything all at once, that is a viable option.
Am Samstag, 9. April 2022, 17:26:01 CEST schrieb Peter Geis: > On Sat, Apr 9, 2022 at 7:56 AM Frank Wunderlich <frank-w@public-files.de> wrote: > > > > Hi, > > > > so to not break the binding and other boards the right Patch should be like this > > > > +++ b/drivers/usb/dwc3/core.c > > @@ -1691,17 +1691,17 @@ static int dwc3_probe(struct platform_device *pdev) > > * Clocks are optional, but new DT platforms should support all > > * clocks as required by the DT-binding. > > */ > > - dwc->bus_clk = devm_clk_get_optional(dev, "bus_early"); > > + dwc->bus_clk = devm_clk_get_optional(dev, "bus_clk"); > > if (IS_ERR(dwc->bus_clk)) > > return dev_err_probe(dev, PTR_ERR(dwc->bus_clk), > > "could not get bus clock\n"); > > > > - dwc->ref_clk = devm_clk_get_optional(dev, "ref"); > > + dwc->ref_clk = devm_clk_get_optional(dev, "ref_clk"); > > if (IS_ERR(dwc->ref_clk)) > > return dev_err_probe(dev, PTR_ERR(dwc->ref_clk), > > "could not get ref clock\n"); > > > > - dwc->susp_clk = devm_clk_get_optional(dev, "suspend"); > > + dwc->susp_clk = devm_clk_get_optional(dev, "suspend_clk"); > > if (IS_ERR(dwc->susp_clk)) > > return dev_err_probe(dev, PTR_ERR(dwc->susp_clk), > > "could not get suspend clock\n"); > > > > but this needs fixing dts using the new clock names > > > > this is a link to the series moving from bulk_clk to named clocks: > > > > https://patchwork.kernel.org/project/linux-usb/patch/20220127200636.1456175-3-sean.anderson@seco.com/ > > > > regards Frank > > I've submitted a fix for the backwards compatibility issue. > https://patchwork.kernel.org/project/linux-rockchip/patch/20220409152116.3834354-1-pgwipeout@gmail.com/ > > This fix is standalone and necessary no matter which route we decide > to go with this series (and the rk3328/rk3399 support as well). > With this patch, dwc3 is functional on the rk356x as the series was > submitted, so if we decide to fix everything all at once, that is a > viable option. Thanks for doing that fix. As the usb-dt-series is actually following the rockchip,dwc3 binding, and "just" the driver does ignore it, I've now applied the usb series and hope for a resolution of the general problem :-) Heiko
diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi index 55e6dcb948cc..6dfe54e53be1 100644 --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi @@ -264,8 +264,8 @@ usb_host0_xhci: usb@fcc00000 { interrupts = <GIC_SPI 169 IRQ_TYPE_LEVEL_HIGH>; clocks = <&cru CLK_USB3OTG0_REF>, <&cru CLK_USB3OTG0_SUSPEND>, <&cru ACLK_USB3OTG0>; - clock-names = "ref_clk", "suspend_clk", - "bus_clk"; + clock-names = "ref", "suspend_clk", + "bus_early"; dr_mode = "host"; phy_type = "utmi_wide"; power-domains = <&power RK3568_PD_PIPE>; @@ -280,8 +280,8 @@ usb_host1_xhci: usb@fd000000 { interrupts = <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH>; clocks = <&cru CLK_USB3OTG1_REF>, <&cru CLK_USB3OTG1_SUSPEND>, <&cru ACLK_USB3OTG1>; - clock-names = "ref_clk", "suspend_clk", - "bus_clk"; + clock-names = "ref", "suspend_clk", + "bus_early"; dr_mode = "host"; phys = <&usb2phy0_host>, <&combphy1 PHY_TYPE_USB3>; phy-names = "usb2-phy", "usb3-phy";