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
