[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <e7c3bb1d-3694-7aa6-901f-47aadda18a20@linaro.org>
Date: Mon, 17 Jul 2023 14:02:37 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: "Li, Meng" <Meng.Li@...driver.com>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"krzysztof.kozlowski+dt@...aro.org"
<krzysztof.kozlowski+dt@...aro.org>,
"conor+dt@...nel.org" <conor+dt@...nel.org>,
"dinguyen@...nel.org" <dinguyen@...nel.org>,
"hminas@...opsys.com" <hminas@...opsys.com>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: dwc2: combine platform specific data for Intel
Agilex and Stratix10
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
Powered by blists - more mailing lists