| Message ID | 20210701004043.18585-1-dgilmore@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] arm64: dts: rockchip: helios64: fixup USB setup | expand |
Hello, On 7/1/21 2:40 AM, Dennis Gilmore wrote: > Without the usbdrd_dwc3_1 node defined u-boot will throw an error and > reset the system. I wonder if this should better be fixed in u-boot then?! > All other rk3399 systems use this format This is true for the dwc nodes, however for the usb2 nodes there are several that use this idiom (and even repeat the label name), see for example the &u2phy0 node in arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi . In my eyes it's a bit ugly to have the two nodes as separate entities and on the same indentation level in the machine dts given that one is the parent of the other and enabling USB needs both status settings. Best regards Uwe
On 2021-07-01 01:40, Dennis Gilmore wrote: > Without the usbdrd_dwc3_1 node defined u-boot will throw an error and > reset the system. All other rk3399 systems use this format This doesn't make much sense - the usbdrd_dwc3_1 label is defined in rk3399.dtsi either way, and the compiled DTBs before and after are identical :/ If U-Boot is doing something funny with labels and symbols to give itself some kind of fragile dependency, that probably wants to be fixed in U-Boot. Robin. > Signed-off-by: Dennis Gilmore <dgilmore@redhat.com> > --- > arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts b/arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts > index 738cfd21df3e..aeedf098f67c 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts > @@ -490,9 +490,9 @@ &uart2 { > > &usbdrd3_1 { > status = "okay"; > +}; > > - usb@fe900000 { > - dr_mode = "host"; > - status = "okay"; > - }; > +&usbdrd_dwc3_1 { > + dr_mode = "host"; > + status = "okay"; > }; >
On Thu, 2021-07-01 at 11:31 +0200, Uwe Kleine-König wrote: > Hello, > > On 7/1/21 2:40 AM, Dennis Gilmore wrote: > > Without the usbdrd_dwc3_1 node defined u-boot will throw an error > > and > > reset the system. > > I wonder if this should better be fixed in u-boot then?! > > > All other rk3399 systems use this format > > This is true for the dwc nodes, however for the usb2 nodes there are > several that use this idiom (and even repeat the label name), see for > example the &u2phy0 node in > arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi . > looking at that file is where I got the idea to set it up as I have proposed, it follows the format I have submitted Dennis > In my eyes it's a bit ugly to have the two nodes as separate entities > and on the same indentation level in the machine dts given that one > is > the parent of the other and enabling USB needs both status settings. > > Best regards > Uwe >
Hello Dennis, On 7/1/21 2:59 PM, Dennis Gilmore wrote: > On Thu, 2021-07-01 at 11:31 +0200, Uwe Kleine-König wrote: >> Hello, >> >> On 7/1/21 2:40 AM, Dennis Gilmore wrote: >>> Without the usbdrd_dwc3_1 node defined u-boot will throw an error >>> and >>> reset the system. >> >> I wonder if this should better be fixed in u-boot then?! >> >>> All other rk3399 systems use this format >> >> This is true for the dwc nodes, however for the usb2 nodes there are >> several that use this idiom (and even repeat the label name), see for >> example the &u2phy0 node in >> arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi . >> > > looking at that file is where I got the idea to set it up as I have > proposed, it follows the format I have submitted I guess you didn't read exactly what I wrote and only looked at &usbdrd* but not &u2phy0. Best regards Uwe
On Thu, 2021-07-01 at 15:35 +0200, Uwe Kleine-König wrote: > Hello Dennis, > > On 7/1/21 2:59 PM, Dennis Gilmore wrote: > > On Thu, 2021-07-01 at 11:31 +0200, Uwe Kleine-König wrote: > > > Hello, > > > > > > On 7/1/21 2:40 AM, Dennis Gilmore wrote: > > > > Without the usbdrd_dwc3_1 node defined u-boot will throw an > > > > error > > > > and > > > > reset the system. > > > > > > I wonder if this should better be fixed in u-boot then?! > > > > > > > All other rk3399 systems use this format > > > > > > This is true for the dwc nodes, however for the usb2 nodes there > > > are > > > several that use this idiom (and even repeat the label name), see > > > for > > > example the &u2phy0 node in > > > arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi . > > > > > > > looking at that file is where I got the idea to set it up as I have > > proposed, it follows the format I have submitted > > I guess you didn't read exactly what I wrote and only looked at > &usbdrd* > but not &u2phy0. Hi Uwe, I did read what you pointed at, the issue is that is already defined in arch/arm64/boot/dts/rockchip/rk3399.dtsi and all that is needed it to set the status to okay and minor enablement. as DTC merges all the snippets together they end up in the right place and doing the right thing, the approach you took is different to all the other boards I looked at and seemingly causes issues in u-boot. I did not get far enough in booting to verify the state in linux as I am updating u-boot as I go and testing using u-boots dtb for simplicty sake. It quite likely is a bug in u-boot that it resets the system, at the same time I do not think that doing it differently to how the other boards are implemented is right either. There is still a lot of the hardware in the system that is not defined in the devicetree file, including a lot of the usb stack. Dennis
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts b/arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts index 738cfd21df3e..aeedf098f67c 100644 --- a/arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts +++ b/arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts @@ -490,9 +490,9 @@ &uart2 { &usbdrd3_1 { status = "okay"; +}; - usb@fe900000 { - dr_mode = "host"; - status = "okay"; - }; +&usbdrd_dwc3_1 { + dr_mode = "host"; + status = "okay"; };
Without the usbdrd_dwc3_1 node defined u-boot will throw an error and reset the system. All other rk3399 systems use this format Signed-off-by: Dennis Gilmore <dgilmore@redhat.com> --- arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)