Re: [PATCH 1/6] ARM: dts: sam9x60: Add OHCI and EHCI DT nodes
On 1/4/23 11:11, eugen.hris...@microchip.com wrote:
On 1/4/23 11:38, Marek Vasut wrote:
On 1/4/23 08:30, eugen.hris...@microchip.com wrote:
On 1/3/23 01:12, Marek Vasut wrote:
On 12/23/22 13:33, Sergiu Moga wrote:
Add the OHCI and EHCI DT nodes for the sam9x60 boards.
Signed-off-by: Sergiu Moga
---
arch/arm/dts/at91-sam9x60_curiosity.dts | 21 +
arch/arm/dts/sam9x60.dtsi | 18 ++
Board and SoC DT changes should be in separate patches.
arch/arm/dts/sam9x60ek.dts | 21 +
3 files changed, 60 insertions(+)
[...]
diff --git a/arch/arm/dts/sam9x60.dtsi b/arch/arm/dts/sam9x60.dtsi
index 17224ef771..e36a540f78 100644
--- a/arch/arm/dts/sam9x60.dtsi
+++ b/arch/arm/dts/sam9x60.dtsi
@@ -69,6 +69,24 @@
#size-cells = <1>;
ranges;
+ usb1: ohci@60 {
This should be usb@ instead of ohci@ , if you run "make dtbs_check" on
this DT in Linux (please do), you would likely get a warning , see Linux
Documentation/devicetree/bindings/usb/usb.yaml .
If in Linux we have ohci@ , then we need to have the same in U-boot.
You should update the Linux DT to usb@ too to avoid dtbs_check warnings.
We can accept the change to usb@ , if there is a pending patch in Linux.
I can make the same argument about Linux, since DTs are OS agnostic. My
comment is OS agnostic too and does not apply specifically to U-Boot or
Linux, it applies to DT.
Definitely you can comment in Linux , for sure.
U-boot is not the place to review the devicetree.
I strongly disagree with this statement.
There are a few decisions behind this statement.
First, the fact that we no longer take bindings into Uboot, and rely
solely on bindings in Linux. This means the bindings were reviewed and
validated in Linux.
Linux
1e5f532c27371 ("ARM: dts: at91: sam9x60: add device tree for soc and board")
which added these ohci@ nodes has been accepted after Linux
14ec072a19ad7 ("dt-bindings: usb: Convert USB HCD generic binding to YAML")
was already in place.
Therefore, nobody ran the dtbs_check at that point, which is somewhat
understandable, since the dtbs_check at that point was in very early stages.
This however shows the review process is not flawless and subsequent
updates may be necessary.
One such update example is Linux:
979813d2ab705 ("ARM: dts: at91: use generic name for reset controller")
which does:
- reset_controller: rstc@fe00 {
+ reset_controller: reset-controller@fe00 {
or another one:
6a743ea387e63 ("ARM: dts: at91: Use the generic "rtc" node name for the
rtt IPs")
- rtt: rtt@fe20 {
+ rtt: rtc@fe20 {
or another one:
4b6140b96dfe4 ("ARM: dts: at91: Use the generic "crypto" node name for
the crypto IPs")
- sha: sha@f002c000 {
+ sha: crypto@f002c000 {
Sure there are a few exceptions but not the usual rule.
Second, we rely on devicetree kernel.org mailing list to review all the
DTs and binding compliance in Linux.
This means that the DT has been validated and checked as ABI in Linux.
I think the ABI argument does not work, your own engineers would've
caused multiple ABI breaks by now, see above.
Hence there is no place to question that in Uboot or any other project.
As you can see above, Linux kernel review process is not perfect and the
code is constantly evolving rather than begin set in stone. I believe
other projects and their reviewers are no worse than Linux kernel ones,
and can provide valid review feedback too.
The DT must be the same and ABI across all software that uses the DT. If
this was validated and agreed upon in Linux, we can't question that here.
So if you disagree with this statement, it's your choice of course.
I strongly disagree.
What does it matter where the review feedback came from ?
What does matter is that you can improve the DT based on that feedback,
if the feedback is valid.
I agree with that, but if we want to improve things, we need to patch
things either in Linux first, or patch projects together.
The fact that you are against some patches while they have the same
identical node in Linux, is not a reason to not allow devicetree nodes
to be added to Uboot.
DT syncing with Linux is always a must, and that is part of what Sergiu
is doing with these patches.
See below.
The devicetree must be in sync with Linux.
I agree with this statement.
Please update the Linux DT too.
That is one approach but as I said, not a reason to reject a node that
is identical with Linux.
Ultimately, I am not the at91 maintainer, so I have no say in this.
I provided review feedback, that the DT checker would warn on the node
name, and explained how to deal with it.
Whether the feedback is accepted and the DT is improved , or , the
feedback is discarded and the DT checker keeps producing
Re: [PATCH 1/6] ARM: dts: sam9x60: Add OHCI and EHCI DT nodes
On 1/4/23 11:38, Marek Vasut wrote:
> On 1/4/23 08:30, eugen.hris...@microchip.com wrote:
>> On 1/3/23 01:12, Marek Vasut wrote:
>>> On 12/23/22 13:33, Sergiu Moga wrote:
Add the OHCI and EHCI DT nodes for the sam9x60 boards.
Signed-off-by: Sergiu Moga
---
arch/arm/dts/at91-sam9x60_curiosity.dts | 21 +
arch/arm/dts/sam9x60.dtsi | 18 ++
>>>
>>> Board and SoC DT changes should be in separate patches.
>>>
arch/arm/dts/sam9x60ek.dts | 21 +
3 files changed, 60 insertions(+)
>>>
>>>
>>> [...]
>>>
diff --git a/arch/arm/dts/sam9x60.dtsi b/arch/arm/dts/sam9x60.dtsi
index 17224ef771..e36a540f78 100644
--- a/arch/arm/dts/sam9x60.dtsi
+++ b/arch/arm/dts/sam9x60.dtsi
@@ -69,6 +69,24 @@
#size-cells = <1>;
ranges;
+ usb1: ohci@60 {
>>>
>>> This should be usb@ instead of ohci@ , if you run "make dtbs_check" on
>>> this DT in Linux (please do), you would likely get a warning , see Linux
>>> Documentation/devicetree/bindings/usb/usb.yaml .
>>
>>
>> If in Linux we have ohci@ , then we need to have the same in U-boot.
>
> You should update the Linux DT to usb@ too to avoid dtbs_check warnings.
>
>> We can accept the change to usb@ , if there is a pending patch in Linux.
>
> I can make the same argument about Linux, since DTs are OS agnostic. My
> comment is OS agnostic too and does not apply specifically to U-Boot or
> Linux, it applies to DT.
Definitely you can comment in Linux , for sure.
>
>> U-boot is not the place to review the devicetree.
>
> I strongly disagree with this statement.
There are a few decisions behind this statement.
First, the fact that we no longer take bindings into Uboot, and rely
solely on bindings in Linux. This means the bindings were reviewed and
validated in Linux.
Sure there are a few exceptions but not the usual rule.
Second, we rely on devicetree kernel.org mailing list to review all the
DTs and binding compliance in Linux.
This means that the DT has been validated and checked as ABI in Linux.
Hence there is no place to question that in Uboot or any other project.
The DT must be the same and ABI across all software that uses the DT. If
this was validated and agreed upon in Linux, we can't question that here.
So if you disagree with this statement, it's your choice of course.
>
> What does it matter where the review feedback came from ?
> What does matter is that you can improve the DT based on that feedback,
> if the feedback is valid.
I agree with that, but if we want to improve things, we need to patch
things either in Linux first, or patch projects together.
The fact that you are against some patches while they have the same
identical node in Linux, is not a reason to not allow devicetree nodes
to be added to Uboot.
DT syncing with Linux is always a must, and that is part of what Sergiu
is doing with these patches.
>
>> The devicetree must be in sync with Linux.
>
> I agree with this statement.
>
> Please update the Linux DT too.
That is one approach but as I said, not a reason to reject a node that
is identical with Linux.
Re: [PATCH 1/6] ARM: dts: sam9x60: Add OHCI and EHCI DT nodes
On 1/4/23 08:30, eugen.hris...@microchip.com wrote:
On 1/3/23 01:12, Marek Vasut wrote:
On 12/23/22 13:33, Sergiu Moga wrote:
Add the OHCI and EHCI DT nodes for the sam9x60 boards.
Signed-off-by: Sergiu Moga
---
arch/arm/dts/at91-sam9x60_curiosity.dts | 21 +
arch/arm/dts/sam9x60.dtsi | 18 ++
Board and SoC DT changes should be in separate patches.
arch/arm/dts/sam9x60ek.dts | 21 +
3 files changed, 60 insertions(+)
[...]
diff --git a/arch/arm/dts/sam9x60.dtsi b/arch/arm/dts/sam9x60.dtsi
index 17224ef771..e36a540f78 100644
--- a/arch/arm/dts/sam9x60.dtsi
+++ b/arch/arm/dts/sam9x60.dtsi
@@ -69,6 +69,24 @@
#size-cells = <1>;
ranges;
+ usb1: ohci@60 {
This should be usb@ instead of ohci@ , if you run "make dtbs_check" on
this DT in Linux (please do), you would likely get a warning , see Linux
Documentation/devicetree/bindings/usb/usb.yaml .
If in Linux we have ohci@ , then we need to have the same in U-boot.
You should update the Linux DT to usb@ too to avoid dtbs_check warnings.
We can accept the change to usb@ , if there is a pending patch in Linux.
I can make the same argument about Linux, since DTs are OS agnostic. My
comment is OS agnostic too and does not apply specifically to U-Boot or
Linux, it applies to DT.
U-boot is not the place to review the devicetree.
I strongly disagree with this statement.
What does it matter where the review feedback came from ?
What does matter is that you can improve the DT based on that feedback,
if the feedback is valid.
The devicetree must be in sync with Linux.
I agree with this statement.
Please update the Linux DT too.
Re: [PATCH 1/6] ARM: dts: sam9x60: Add OHCI and EHCI DT nodes
On 1/3/23 01:12, Marek Vasut wrote:
> On 12/23/22 13:33, Sergiu Moga wrote:
>> Add the OHCI and EHCI DT nodes for the sam9x60 boards.
>>
>> Signed-off-by: Sergiu Moga
>> ---
>> arch/arm/dts/at91-sam9x60_curiosity.dts | 21 +
>> arch/arm/dts/sam9x60.dtsi | 18 ++
>
> Board and SoC DT changes should be in separate patches.
>
>> arch/arm/dts/sam9x60ek.dts | 21 +
>> 3 files changed, 60 insertions(+)
>
>
> [...]
>
>> diff --git a/arch/arm/dts/sam9x60.dtsi b/arch/arm/dts/sam9x60.dtsi
>> index 17224ef771..e36a540f78 100644
>> --- a/arch/arm/dts/sam9x60.dtsi
>> +++ b/arch/arm/dts/sam9x60.dtsi
>> @@ -69,6 +69,24 @@
>> #size-cells = <1>;
>> ranges;
>>
>> + usb1: ohci@60 {
>
> This should be usb@ instead of ohci@ , if you run "make dtbs_check" on
> this DT in Linux (please do), you would likely get a warning , see Linux
> Documentation/devicetree/bindings/usb/usb.yaml .
If in Linux we have ohci@ , then we need to have the same in U-boot.
We can accept the change to usb@ , if there is a pending patch in Linux.
U-boot is not the place to review the devicetree. The devicetree must be
in sync with Linux.
Eugen
>
>> + compatible = "atmel,at91rm9200-ohci", "usb-ohci";
>> + reg = <0x0060 0x10>;
>> + clocks = < PMC_TYPE_PERIPHERAL 22>, <
>> PMC_TYPE_PERIPHERAL 22>, < PMC_TYPE_SYSTEM 21>;
>> + clock-names = "ohci_clk", "hclk", "uhpck";
>> + status = "disabled";
>> + };
>
> [...]
>
Re: [PATCH 1/6] ARM: dts: sam9x60: Add OHCI and EHCI DT nodes
On 12/23/22 13:33, Sergiu Moga wrote:
Add the OHCI and EHCI DT nodes for the sam9x60 boards.
Signed-off-by: Sergiu Moga
---
arch/arm/dts/at91-sam9x60_curiosity.dts | 21 +
arch/arm/dts/sam9x60.dtsi | 18 ++
Board and SoC DT changes should be in separate patches.
arch/arm/dts/sam9x60ek.dts | 21 +
3 files changed, 60 insertions(+)
[...]
diff --git a/arch/arm/dts/sam9x60.dtsi b/arch/arm/dts/sam9x60.dtsi
index 17224ef771..e36a540f78 100644
--- a/arch/arm/dts/sam9x60.dtsi
+++ b/arch/arm/dts/sam9x60.dtsi
@@ -69,6 +69,24 @@
#size-cells = <1>;
ranges;
+ usb1: ohci@60 {
This should be usb@ instead of ohci@ , if you run "make dtbs_check" on
this DT in Linux (please do), you would likely get a warning , see Linux
Documentation/devicetree/bindings/usb/usb.yaml .
+ compatible = "atmel,at91rm9200-ohci", "usb-ohci";
+ reg = <0x0060 0x10>;
+ clocks = < PMC_TYPE_PERIPHERAL 22>, < PMC_TYPE_PERIPHERAL
22>, < PMC_TYPE_SYSTEM 21>;
+ clock-names = "ohci_clk", "hclk", "uhpck";
+ status = "disabled";
+ };
[...]
[PATCH 1/6] ARM: dts: sam9x60: Add OHCI and EHCI DT nodes
Add the OHCI and EHCI DT nodes for the sam9x60 boards.
Signed-off-by: Sergiu Moga
---
arch/arm/dts/at91-sam9x60_curiosity.dts | 21 +
arch/arm/dts/sam9x60.dtsi | 18 ++
arch/arm/dts/sam9x60ek.dts | 21 +
3 files changed, 60 insertions(+)
diff --git a/arch/arm/dts/at91-sam9x60_curiosity.dts
b/arch/arm/dts/at91-sam9x60_curiosity.dts
index 7c5b6ae2b8..d6ae3d648d 100644
--- a/arch/arm/dts/at91-sam9x60_curiosity.dts
+++ b/arch/arm/dts/at91-sam9x60_curiosity.dts
@@ -49,6 +49,13 @@
atmel,pins =
;
};
+
+ usb1 {
+ pinctrl_usb_default:
usb_default {
+ atmel,pins = ;
+ };
+ };
};
};
};
@@ -89,3 +96,17 @@
phy-mode = "rmii";
status = "okay";
};
+
+ {
+ num-ports = <3>;
+ atmel,vbus-gpio = <0
+ 15 GPIO_ACTIVE_HIGH
+ 18 GPIO_ACTIVE_HIGH>;
+ pinctrl-names = "default";
+ pinctrl-0 = <_usb_default>;
+ status = "okay";
+};
+
+ {
+ status = "okay";
+};
diff --git a/arch/arm/dts/sam9x60.dtsi b/arch/arm/dts/sam9x60.dtsi
index 17224ef771..e36a540f78 100644
--- a/arch/arm/dts/sam9x60.dtsi
+++ b/arch/arm/dts/sam9x60.dtsi
@@ -69,6 +69,24 @@
#size-cells = <1>;
ranges;
+ usb1: ohci@60 {
+ compatible = "atmel,at91rm9200-ohci", "usb-ohci";
+ reg = <0x0060 0x10>;
+ clocks = < PMC_TYPE_PERIPHERAL 22>, <
PMC_TYPE_PERIPHERAL 22>, < PMC_TYPE_SYSTEM 21>;
+ clock-names = "ohci_clk", "hclk", "uhpck";
+ status = "disabled";
+ };
+
+ usb2: ehci@70 {
+ compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
+ reg = <0x0070 0x10>;
+ clocks = < PMC_TYPE_CORE 8>, <
PMC_TYPE_PERIPHERAL 22>;
+ clock-names = "usb_clk", "ehci_clk";
+ assigned-clocks = < PMC_TYPE_CORE 8>;
+ assigned-clock-rates = <48000>;
+ status = "disabled";
+ };
+
ebi: ebi@1000 {
compatible = "microchip,sam9x60-ebi";
#address-cells = <2>;
diff --git a/arch/arm/dts/sam9x60ek.dts b/arch/arm/dts/sam9x60ek.dts
index 1a02e2cb79..45e2f4cc40 100644
--- a/arch/arm/dts/sam9x60ek.dts
+++ b/arch/arm/dts/sam9x60ek.dts
@@ -139,6 +139,13 @@
;
};
+ usb1 {
+ pinctrl_usb_default: usb_default {
+ atmel,pins = ;
+ };
+ };
+
};
};
};
@@ -213,3 +220,17 @@
phy-mode = "rmii";
status = "okay";
};
+
+ {
+ num-ports = <3>;
+ atmel,vbus-gpio = <0
+ 15 GPIO_ACTIVE_HIGH
+ 16 GPIO_ACTIVE_HIGH>;
+ pinctrl-names = "default";
+ pinctrl-0 = <_usb_default>;
+ status = "okay";
+};
+
+ {
+ status = "okay";
+};
--
2.34.1
