| Message ID | 20230717085053.1075077-1-Meng.Li@windriver.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc2: combine platform specific data for Intel Agilex and Stratix10 | expand |
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 <Meng.Li@windriver.com> > --- > 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 <krzysztof.kozlowski@linaro.org> > Sent: Monday, July 17, 2023 5:42 PM > To: Li, Meng <Meng.Li@windriver.com>; gregkh@linuxfoundation.org; > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; > dinguyen@kernel.org; hminas@synopsys.com; linux-usb@vger.kernel.org; > devicetree@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > 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 <Meng.Li@windriver.com> > > --- > > 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
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);
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 <Meng.Li@windriver.com> --- 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(-)