Intel Stratix10 is very the same with Agilex platform, the DWC2 IP on
the Stratix platform also does not support clock-gating. So, based on
commit 3d8d3504d233("usb: dwc2: Add platform specific data for
Intel's Agilex"), combine platform specific data for Intel Agilex and
Stratix10 together. In additional, in order to avoid breaking the old
device tree, keep compatible string "intel,socfpga-agilex-hsotg" unchanged.
Signed-off-by: Meng Li <[email protected]>
---
Documentation/devicetree/bindings/usb/dwc2.yaml | 2 ++
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 4 ++--
drivers/usb/dwc2/params.c | 6 ++++--
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/usb/dwc2.yaml b/Documentation/devicetree/bindings/usb/dwc2.yaml
index dc4988c0009c..c98ca98d5033 100644
--- a/Documentation/devicetree/bindings/usb/dwc2.yaml
+++ b/Documentation/devicetree/bindings/usb/dwc2.yaml
@@ -51,6 +51,7 @@ properties:
- amlogic,meson-gxbb-usb
- amlogic,meson-g12a-usb
- intel,socfpga-agilex-hsotg
+ - intel,socfpga-hsotg
- const: snps,dwc2
- const: amcc,dwc-otg
- const: apm,apm82181-dwc-otg
@@ -64,6 +65,7 @@ properties:
- const: snps,dwc2
- const: samsung,s3c6400-hsotg
- const: intel,socfpga-agilex-hsotg
+ - const: intel,socfpga-hsotg
reg:
maxItems: 1
diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
index ea788a920eab..c5a51636f657 100644
--- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
+++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
@@ -490,7 +490,7 @@ usbphy0: usbphy@0 {
};
usb0: usb@ffb00000 {
- compatible = "snps,dwc2";
+ compatible = "intel,socfpga-hsotg", "snps,dwc2";
reg = <0xffb00000 0x40000>;
interrupts = <0 93 4>;
phys = <&usbphy0>;
@@ -504,7 +504,7 @@ usb0: usb@ffb00000 {
};
usb1: usb@ffb40000 {
- compatible = "snps,dwc2";
+ compatible = "intel,socfpga-hsotg", "snps,dwc2";
reg = <0xffb40000 0x40000>;
interrupts = <0 94 4>;
phys = <&usbphy0>;
diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index 8eab5f38b110..6bb27a24e9e1 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -93,7 +93,7 @@ static void dwc2_set_s3c6400_params(struct dwc2_hsotg *hsotg)
p->phy_utmi_width = 8;
}
-static void dwc2_set_socfpga_agilex_params(struct dwc2_hsotg *hsotg)
+static void dwc2_set_socfpga_params(struct dwc2_hsotg *hsotg)
{
struct dwc2_core_params *p = &hsotg->params;
@@ -266,7 +266,9 @@ const struct of_device_id dwc2_of_match_table[] = {
{ .compatible = "st,stm32mp15-hsotg",
.data = dwc2_set_stm32mp15_hsotg_params },
{ .compatible = "intel,socfpga-agilex-hsotg",
- .data = dwc2_set_socfpga_agilex_params },
+ .data = dwc2_set_socfpga_params },
+ { .compatible = "intel,socfpga-hsotg",
+ .data = dwc2_set_socfpga_params },
{},
};
MODULE_DEVICE_TABLE(of, dwc2_of_match_table);
--
2.34.1
On 17/07/2023 10:50, Meng Li wrote:
> Intel Stratix10 is very the same with Agilex platform, the DWC2 IP on
> the Stratix platform also does not support clock-gating. So, based on
> commit 3d8d3504d233("usb: dwc2: Add platform specific data for
> Intel's Agilex"), combine platform specific data for Intel Agilex and
> Stratix10 together. In additional, in order to avoid breaking the old
> device tree, keep compatible string "intel,socfpga-agilex-hsotg" unchanged.
>
> Signed-off-by: Meng Li <[email protected]>
> ---
> Documentation/devicetree/bindings/usb/dwc2.yaml | 2 ++
Bindings are always separate patch.
> arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 4 ++--
As DTS is.
> drivers/usb/dwc2/params.c | 6 ++++--
> 3 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc2.yaml b/Documentation/devicetree/bindings/usb/dwc2.yaml
> index dc4988c0009c..c98ca98d5033 100644
> --- a/Documentation/devicetree/bindings/usb/dwc2.yaml
> +++ b/Documentation/devicetree/bindings/usb/dwc2.yaml
> @@ -51,6 +51,7 @@ properties:
> - amlogic,meson-gxbb-usb
> - amlogic,meson-g12a-usb
> - intel,socfpga-agilex-hsotg
> + - intel,socfpga-hsotg
Where is SoC specific compatible?
> - const: snps,dwc2
> - const: amcc,dwc-otg
> - const: apm,apm82181-dwc-otg
> @@ -64,6 +65,7 @@ properties:
> - const: snps,dwc2
> - const: samsung,s3c6400-hsotg
> - const: intel,socfpga-agilex-hsotg
> + - const: intel,socfpga-hsotg
>
> reg:
> maxItems: 1
> diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> index ea788a920eab..c5a51636f657 100644
> --- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> +++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
...
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index 8eab5f38b110..6bb27a24e9e1 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -93,7 +93,7 @@ static void dwc2_set_s3c6400_params(struct dwc2_hsotg *hsotg)
> p->phy_utmi_width = 8;
> }
>
> -static void dwc2_set_socfpga_agilex_params(struct dwc2_hsotg *hsotg)
> +static void dwc2_set_socfpga_params(struct dwc2_hsotg *hsotg)
Why? Old name was ok...
> {
> struct dwc2_core_params *p = &hsotg->params;
>
> @@ -266,7 +266,9 @@ const struct of_device_id dwc2_of_match_table[] = {
> { .compatible = "st,stm32mp15-hsotg",
> .data = dwc2_set_stm32mp15_hsotg_params },
> { .compatible = "intel,socfpga-agilex-hsotg",
> - .data = dwc2_set_socfpga_agilex_params },
> + .data = dwc2_set_socfpga_params },
> + { .compatible = "intel,socfpga-hsotg",
> + .data = dwc2_set_socfpga_params },
Aren't they compatible? Why do you need new entry for compatible devices?
Best regards,
Krzysztof
> -----Original Message-----
> From: Krzysztof Kozlowski <[email protected]>
> Sent: Monday, July 17, 2023 5:42 PM
> To: Li, Meng <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Cc: [email protected]
> Subject: Re: [PATCH] usb: dwc2: combine platform specific data for Intel Agilex
> and Stratix10
>
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and
> know the content is safe.
>
> On 17/07/2023 10:50, Meng Li wrote:
> > Intel Stratix10 is very the same with Agilex platform, the DWC2 IP on
> > the Stratix platform also does not support clock-gating. So, based on
> > commit 3d8d3504d233("usb: dwc2: Add platform specific data for Intel's
> > Agilex"), combine platform specific data for Intel Agilex and
> > Stratix10 together. In additional, in order to avoid breaking the old
> > device tree, keep compatible string "intel,socfpga-agilex-hsotg" unchanged.
> >
> > Signed-off-by: Meng Li <[email protected]>
> > ---
> > Documentation/devicetree/bindings/usb/dwc2.yaml | 2 ++
>
> Bindings are always separate patch.
>
Got it.
> > arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 4 ++--
>
> As DTS is.
>
Got it.
> > drivers/usb/dwc2/params.c | 6 ++++--
> > 3 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/dwc2.yaml
> > b/Documentation/devicetree/bindings/usb/dwc2.yaml
> > index dc4988c0009c..c98ca98d5033 100644
> > --- a/Documentation/devicetree/bindings/usb/dwc2.yaml
> > +++ b/Documentation/devicetree/bindings/usb/dwc2.yaml
> > @@ -51,6 +51,7 @@ properties:
> > - amlogic,meson-gxbb-usb
> > - amlogic,meson-g12a-usb
> > - intel,socfpga-agilex-hsotg
> > + - intel,socfpga-hsotg
>
> Where is SoC specific compatible?
>
The socfpga is a SoC family, it includes Agilex ad Stratix10 SoCs.
In fact, we only need the compatible " intel,socfpga-hsotg " is enough.
But in order to avoid breaking the old device tree for agilex platform, I reserve the old compatible.
So, I think we don't need the Stratix10 compatible like "intel,socfpga-stratix10-hsotg "
> > - const: snps,dwc2
> > - const: amcc,dwc-otg
> > - const: apm,apm82181-dwc-otg
> > @@ -64,6 +65,7 @@ properties:
> > - const: snps,dwc2
> > - const: samsung,s3c6400-hsotg
> > - const: intel,socfpga-agilex-hsotg
> > + - const: intel,socfpga-hsotg
> >
> > reg:
> > maxItems: 1
> > diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> > b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> > index ea788a920eab..c5a51636f657 100644
> > --- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> > +++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
>
> ...
>
> > diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> > index 8eab5f38b110..6bb27a24e9e1 100644
> > --- a/drivers/usb/dwc2/params.c
> > +++ b/drivers/usb/dwc2/params.c
> > @@ -93,7 +93,7 @@ static void dwc2_set_s3c6400_params(struct
> dwc2_hsotg *hsotg)
> > p->phy_utmi_width = 8;
> > }
> >
> > -static void dwc2_set_socfpga_agilex_params(struct dwc2_hsotg *hsotg)
> > +static void dwc2_set_socfpga_params(struct dwc2_hsotg *hsotg)
>
> Why? Old name was ok...
>
Old name includes string "agilex" that represents only agilex SoC.
This patch is used to combine platform specific data for Intel Agilex and Stratix10, so create a common function name for socfpga family.
> > {
> > struct dwc2_core_params *p = &hsotg->params;
> >
> > @@ -266,7 +266,9 @@ const struct of_device_id dwc2_of_match_table[] = {
> > { .compatible = "st,stm32mp15-hsotg",
> > .data = dwc2_set_stm32mp15_hsotg_params },
> > { .compatible = "intel,socfpga-agilex-hsotg",
> > - .data = dwc2_set_socfpga_agilex_params },
> > + .data = dwc2_set_socfpga_params },
> > + { .compatible = "intel,socfpga-hsotg",
> > + .data = dwc2_set_socfpga_params },
>
> Aren't they compatible? Why do you need new entry for compatible devices?
>
In fact, the usb IP in Agilex and Sratix10 are the same. But it is not reasonable to use agilex compatible string "intel,socfpga-agilex-hsotg" in Stratix10 dts files.
So, I create a common function name and compatible string for socfpga family that includes Agilex and Stratix10 SoCs.
Thanks,
Limeng
> Best regards,
> Krzysztof
On 17/07/2023 12:13, Li, Meng wrote:
>>> diff --git a/Documentation/devicetree/bindings/usb/dwc2.yaml
>>> b/Documentation/devicetree/bindings/usb/dwc2.yaml
>>> index dc4988c0009c..c98ca98d5033 100644
>>> --- a/Documentation/devicetree/bindings/usb/dwc2.yaml
>>> +++ b/Documentation/devicetree/bindings/usb/dwc2.yaml
>>> @@ -51,6 +51,7 @@ properties:
>>> - amlogic,meson-gxbb-usb
>>> - amlogic,meson-g12a-usb
>>> - intel,socfpga-agilex-hsotg
>>> + - intel,socfpga-hsotg
>>
>> Where is SoC specific compatible?
>>
>
> The socfpga is a SoC family, it includes Agilex ad Stratix10 SoCs.
> In fact, we only need the compatible " intel,socfpga-hsotg " is enough.
You now confuse driver and bindings compatibles...
https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
> But in order to avoid breaking the old device tree for agilex platform, I reserve the old compatible.
> So, I think we don't need the Stratix10 compatible like "intel,socfpga-stratix10-hsotg "
You need. See above link.
>
>>> - const: snps,dwc2
>>> - const: amcc,dwc-otg
>>> - const: apm,apm82181-dwc-otg
>>> @@ -64,6 +65,7 @@ properties:
>>> - const: snps,dwc2
>>> - const: samsung,s3c6400-hsotg
>>> - const: intel,socfpga-agilex-hsotg
>>> + - const: intel,socfpga-hsotg
>>>
>>> reg:
>>> maxItems: 1
>>> diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
>>> b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
>>> index ea788a920eab..c5a51636f657 100644
>>> --- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
>>> +++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
>>
>> ...
>>
>>> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
>>> index 8eab5f38b110..6bb27a24e9e1 100644
>>> --- a/drivers/usb/dwc2/params.c
>>> +++ b/drivers/usb/dwc2/params.c
>>> @@ -93,7 +93,7 @@ static void dwc2_set_s3c6400_params(struct
>> dwc2_hsotg *hsotg)
>>> p->phy_utmi_width = 8;
>>> }
>>>
>>> -static void dwc2_set_socfpga_agilex_params(struct dwc2_hsotg *hsotg)
>>> +static void dwc2_set_socfpga_params(struct dwc2_hsotg *hsotg)
>>
>> Why? Old name was ok...
>>
>
> Old name includes string "agilex" that represents only agilex SoC.
No, old name was representing the first compatibility chip.
> This patch is used to combine platform specific data for Intel Agilex and Stratix10, so create a common function name for socfpga family.
And what is wrong with using the previous one? No, don't do this. It
brings no benefits.
>
>>> {
>>> struct dwc2_core_params *p = &hsotg->params;
>>>
>>> @@ -266,7 +266,9 @@ const struct of_device_id dwc2_of_match_table[] = {
>>> { .compatible = "st,stm32mp15-hsotg",
>>> .data = dwc2_set_stm32mp15_hsotg_params },
>>> { .compatible = "intel,socfpga-agilex-hsotg",
>>> - .data = dwc2_set_socfpga_agilex_params },
>>> + .data = dwc2_set_socfpga_params },
>>> + { .compatible = "intel,socfpga-hsotg",
>>> + .data = dwc2_set_socfpga_params },
>>
>> Aren't they compatible? Why do you need new entry for compatible devices?
>>
>
> In fact, the usb IP in Agilex and Sratix10 are the same.
So you do not need new entry. Don't add it.
> But it is not reasonable to use agilex compatible string "intel,socfpga-agilex-hsotg" in Stratix10 dts files.
Why? Why you impose some rule which is different than all other SoCs in
upstream? Why do I have to repeat it every now and then.... ehhh :(
https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
> So, I create a common function name and compatible string for socfpga family that includes Agilex and Stratix10 SoCs.
Nope, sorry.
Best regards,
Krzysztof