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

[Li, Meng]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Monday, July 17, 2023 5:42 PM
> To: Li, Meng <Meng.Li@xxxxxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx;
> robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx;
> dinguyen@xxxxxxxxxx; hminas@xxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxx>
> > ---
> >  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