arm64: dts: rockchip: Fix clocks for rk356x usb
Subsystems (2)
PATCH 1/1 arm64: dts: rockchip: Fix clocks for rk356x usb
From: Frank Wunderlich <[email protected]>
after these 2 commit different clock names are needed compared to 5.17
commit 5114c3ee2487 ("usb: dwc3: Calculate REFCLKPER based on reference clock") commit 33fb697ec7e5 ("usb: dwc3: Get clocks individually")
change them in new rk356x usb support
---
this is a fix for this series not yet applied
https://patchwork.kernel.org/project/linux-rockchip/list/?series=630470
@peter
after testing you can squash it into your series and add a co-developed or similar
--- arch/arm64/boot/dts/rockchip/rk356x.dtsi | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
arch/arm64/boot/dts/rockchip/rk356x.dtsi +4 -4
@@ -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";
Signature · 1 line
2.25.1
Hi,
Quoted 11 lines
Gesendet: Samstag, 09. April 2022 um 09:51 Uhr
Von: "Frank Wunderlich" <[email protected]>arch/arm64/boot/dts/rockchip/rk356x.dtsi +2
-2- 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>;
arch/arm64/boot/dts/rockchip/rk356x.dtsi usb_host1_xhci: usb@fd000000 { · line 285
Quoted 8 lines
arch/arm64/boot/dts/rockchip/rk356x.dtsi +2
-2@@ -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:
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
Heiko Stuebner
maintainerAm Samstag, 9. April 2022, 12:14:28 CEST schrieb Frank Wunderlich:
Quoted 26 lines
Hi,
Quoted 11 lines
Gesendet: Samstag, 09. April 2022 um 09:51 Uhr
Von: "Frank Wunderlich" <[email protected]>arch/arm64/boot/dts/rockchip/rk356x.dtsi +2
-2- 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>;arch/arm64/boot/dts/rockchip/rk356x.dtsi usb_host1_xhci: usb@fd000000 { · line 285
Quoted 8 lines
arch/arm64/boot/dts/rockchip/rk356x.dtsi +2
-2@@ -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:
Quoted 7 lines
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
Heiko
_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
Quoted 6 lines
Gesendet: Samstag, 09. April 2022 um 12:23 Uhr
Von: "Heiko Stuebner" <[email protected]>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/[email protected]/
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.
Quoted 5 lines
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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
Den 09.04.2022 kl. 12.32 skrev Frank Wunderlich:
Quoted 5 lines
Quoted 5 lines
Gesendet: Samstag, 09. April 2022 um 12:23 Uhr
Von: "Heiko Stuebner" <[email protected]>ok, so do you want to send a v2 including that change? Alternatively I can also add this change when applying.
Quoted 7 lines
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/[email protected]/
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?
Quoted 17 lines
but of course if this is not the right way, i post a v2.
Quoted 5 lines
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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
Signature · 9 lines
Kind regards *Dan Johansen* Project lead of the *Manjaro ARM* project Manjaro-ARM <https://manjaro.org> _______________________________________________ Linux-rockchip mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-rockchip
Hi
Quoted 4 lines
Gesendet: Samstag, 09. April 2022 um 12:40 Uhr
Von: "Dan Johansen" <[email protected]>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
_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
Heiko Stuebner
maintainerAm Samstag, 9. April 2022, 12:57:39 CEST schrieb Frank Wunderlich:
Quoted 9 lines
Hi
Quoted 4 lines
Gesendet: Samstag, 09. April 2022 um 12:40 Uhr
Von: "Dan Johansen" <[email protected]>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
Quoted 11 lines · Heiko Stuebner
Am Samstag, 9. April 2022, 12:57:39 CEST schrieb Frank Wunderlich:
Quoted 9 lines
Hi
Quoted 4 lines
Gesendet: Samstag, 09. April 2022 um 12:40 Uhr
Von: "Dan Johansen" <[email protected]>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,
Quoted 10 lines
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.
Quoted 7 lines
Heiko
Very Respectfully,
Peter Geis
Quoted 38 lines · Peter Geis
Quoted 11 lines · Heiko Stuebner
Am Samstag, 9. April 2022, 12:57:39 CEST schrieb Frank Wunderlich:
Quoted 9 lines
Hi
Quoted 4 lines
Gesendet: Samstag, 09. April 2022 um 12:40 Uhr
Von: "Dan Johansen" <[email protected]>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,
Quoted 10 lines
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.
Quoted 4 lines
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?
Quoted 11 lines
Quoted 8 lines
Heiko
Very Respectfully,
Peter Geis
Heiko Stuebner
maintainerAm Samstag, 9. April 2022, 13:30:44 CEST schrieb Peter Geis:
Quoted 44 lines · Peter Geis
Quoted 38 lines · Heiko Stuebner
Quoted 11 lines
Am Samstag, 9. April 2022, 12:57:39 CEST schrieb Frank Wunderlich:
Quoted 9 lines
Hi
Quoted 4 lines
Gesendet: Samstag, 09. April 2022 um 12:40 Uhr
Von: "Dan Johansen" <[email protected]>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,
Quoted 10 lines
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.
Quoted 4 lines
This might also be a good time to look into moving rk3399 to the core dwc3 driver?
This is a delightful mess.
Quoted 7 lines
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
Quoted 52 lines · Heiko Stuebner
Am Samstag, 9. April 2022, 13:30:44 CEST schrieb Peter Geis:
Quoted 50 lines · Peter Geis
Quoted 38 lines · Heiko Stuebner
Quoted 11 lines
Am Samstag, 9. April 2022, 12:57:39 CEST schrieb Frank Wunderlich:
Quoted 9 lines
Hi
Quoted 4 lines
Gesendet: Samstag, 09. April 2022 um 12:40 Uhr
Von: "Dan Johansen" <[email protected]>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,
Quoted 10 lines
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.
Quoted 4 lines
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.
Quoted 4 lines
Quoted 1 line
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
Quoted 4 lines
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/[email protected]/
regards Frank
Quoted 28 lines · Frank Wunderlich
Hi,
so to not break the binding and other boards the right Patch should be like this
+++
b/drivers/usb/dwc3/core.c(quoted diff) +3
-3@@ -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");
Quoted 7 lines
but this needs fixing dts using the new clock names
this is a link to the series moving from
bulk_clkto named clocks:https://patchwork.kernel.org/project/linux-usb/patch/[email protected]/
regards Frank
I've submitted a fix for the backwards compatibility issue.
https://patchwork.kernel.org/project/linux-rockchip/patch/[email protected]/
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.
Heiko Stuebner
maintainerAm Samstag, 9. April 2022, 17:26:01 CEST schrieb Peter Geis:
Quoted 40 lines · Frank Wunderlich
Quoted 28 lines
Hi,
so to not break the binding and other boards the right Patch should be like this
+++
b/drivers/usb/dwc3/core.c(quoted diff) +3
-3@@ -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");Quoted 7 lines
but this needs fixing dts using the new clock names
this is a link to the series moving from
bulk_clkto named clocks:https://patchwork.kernel.org/project/linux-usb/patch/[email protected]/
regards Frank
I've submitted a fix for the backwards compatibility issue.
https://patchwork.kernel.org/project/linux-rockchip/patch/[email protected]/
Quoted 5 lines
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
Quoted 5 lines
Gesendet: Samstag, 09. April 2022 um 13:01 Uhr
Von: "Heiko Stuebner" <[email protected]>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.
Quoted 6 lines
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.
Quoted 1 line
[1] https://patchwork.kernel.org/comment/24809714/
regards Frank