Re: [PATCH] usb: dwc2: combine platform specific data for Intel Agilex and Stratix10

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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