Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e7c3bb1d-3694-7aa6-901f-47aadda18a20@linaro.org>
Date:   Mon, 17 Jul 2023 14:02:37 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH] usb: dwc2: combine platform specific data for Intel
 Agilex and Stratix10
To:     "Li, Meng" <Meng.Li@windriver.com>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "krzysztof.kozlowski+dt@linaro.org" 
        <krzysztof.kozlowski+dt@linaro.org>,
        "conor+dt@kernel.org" <conor+dt@kernel.org>,
        "dinguyen@kernel.org" <dinguyen@kernel.org>,
        "hminas@synopsys.com" <hminas@synopsys.com>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20230717085053.1075077-1-Meng.Li@windriver.com>
 <9a3dc092-7278-cda1-cd4e-c157e0e2c431@linaro.org>
 <PH0PR11MB5191AA947AAE954FF2169FCCF13BA@PH0PR11MB5191.namprd11.prod.outlook.com>
Content-Language: en-US
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <PH0PR11MB5191AA947AAE954FF2169FCCF13BA@PH0PR11MB5191.namprd11.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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