| Message ID | 20240906-sparx5-lan969x-serdes-driver-v1-8-8d630614c58a@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | phy: sparx5-serdes: add support for lan969x serdes driver | expand |
On 06/09/2024 14:52, Daniel Machon wrote: > Document lan969x in the existing Sparx5 dt-bindings. > Say something useful, not copy of subject. > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> A nit, subject: drop second/last, redundant "dt-bindings". The "dt-bindings" prefix is already stating that these are bindings. See also: https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com> > --- > .../bindings/phy/microchip,sparx5-serdes.yaml | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml b/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml > index bdbdb3bbddbe..1e07a311e8a5 100644 > --- a/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml > +++ b/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml > @@ -8,6 +8,7 @@ title: Microchip Sparx5 Serdes controller > > maintainers: > - Steen Hegelund <steen.hegelund@microchip.com> > + - Daniel Machon <daniel.machon@microchip.com> > > description: | > The Sparx5 SERDES interfaces share the same basic functionality, but > @@ -62,12 +63,17 @@ description: | > * 10.3125 Gbps (10GBASE-R/10GBASE-KR/USXGMII) > * 25.78125 Gbps (25GBASE-KR/25GBASE-CR/25GBASE-SR/25GBASE-LR/25GBASE-ER) > > + lan969x has ten SERDES10G interfaces that share the same features, operating > + modes and data rates as the equivalent Sparx5 SERDES10G interfaces. > + > properties: > $nodename: > pattern: "^serdes@[0-9a-f]+$" > > compatible: > - const: microchip,sparx5-serdes > + enum: > + - microchip,sparx5-serdes > + - microchip,lan969x-serdes It seems there is no lan969x SoC/chip. Are you sure you are using correct naming, matching what kernel is using? Maybe you just sent whatever you had in downstream (hint: that's never a good idea). > > reg: > minItems: 1 > @@ -90,11 +96,19 @@ additionalProperties: false > > examples: > - | > - serdes: serdes@10808000 { > + serdes@10808000 { > compatible = "microchip,sparx5-serdes"; > #phy-cells = <1>; > clocks = <&sys_clk>; > reg = <0x10808000 0x5d0000>; > }; > > + - | > + serdes@e3410000 { > + compatible = "microchip,lan969x-serdes"; > + #phy-cells = <1>; > + clocks = <&fabric_clk>; No differences so no need for new example. Also please follow DTS coding style in case of any DTS code. Best regards, Krzysztof
Hi Krzysztof, Thanks your comments. > On 06/09/2024 14:52, Daniel Machon wrote: > > Document lan969x in the existing Sparx5 dt-bindings. > > > > Say something useful, not copy of subject. > > > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> > > A nit, subject: drop second/last, redundant "dt-bindings". The > "dt-bindings" prefix is already stating that these are bindings. > See also: > https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 Duly noted. > > > > Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com> > > --- > > .../bindings/phy/microchip,sparx5-serdes.yaml | 18 ++++++++++++++++-- > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml b/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml > > index bdbdb3bbddbe..1e07a311e8a5 100644 > > --- a/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml > > +++ b/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml > > @@ -8,6 +8,7 @@ title: Microchip Sparx5 Serdes controller > > > > maintainers: > > - Steen Hegelund <steen.hegelund@microchip.com> > > + - Daniel Machon <daniel.machon@microchip.com> > > > > description: | > > The Sparx5 SERDES interfaces share the same basic functionality, but > > @@ -62,12 +63,17 @@ description: | > > * 10.3125 Gbps (10GBASE-R/10GBASE-KR/USXGMII) > > * 25.78125 Gbps (25GBASE-KR/25GBASE-CR/25GBASE-SR/25GBASE-LR/25GBASE-ER) > > > > + lan969x has ten SERDES10G interfaces that share the same features, operating > > + modes and data rates as the equivalent Sparx5 SERDES10G interfaces. > > + > > properties: > > $nodename: > > pattern: "^serdes@[0-9a-f]+$" > > > > compatible: > > - const: microchip,sparx5-serdes > > + enum: > > + - microchip,sparx5-serdes > > + - microchip,lan969x-serdes > > It seems there is no lan969x SoC/chip. Are you sure you are using > correct naming, matching what kernel is using? Maybe you just sent > whatever you had in downstream (hint: that's never a good idea). You are right. There is no upstream support for lan969x SoC yet. The upstreaming of the lan969x SoC has just begun, and this series is part of that upstreaming effort. The lan969x switch driver (not submitted yet) will depend on this SERDES driver, however, their functionality is really independent of each other. That is why I am also upstreaming the SERDES- and switch driver series independent of each other. If these series needs to somehow be connected, by link or whatever, then fine. If there is some preferred way to do this, then please let me know or point me in some direction. Thanks. > > > > > reg: > > minItems: 1 > > @@ -90,11 +96,19 @@ additionalProperties: false > > > > examples: > > - | > > - serdes: serdes@10808000 { > > + serdes@10808000 { > > compatible = "microchip,sparx5-serdes"; > > #phy-cells = <1>; > > clocks = <&sys_clk>; > > reg = <0x10808000 0x5d0000>; > > }; > > > > + - | > > + serdes@e3410000 { > > + compatible = "microchip,lan969x-serdes"; > > + #phy-cells = <1>; > > + clocks = <&fabric_clk>; > > No differences so no need for new example. Also please follow DTS coding > style in case of any DTS code. > Ack. > Best regards, > Krzysztof > /Daniel
On 09/09/2024 10:22, Daniel Machon wrote: > Hi Krzysztof, > > Thanks your comments. > >> On 06/09/2024 14:52, Daniel Machon wrote: >>> Document lan969x in the existing Sparx5 dt-bindings. >>> >> >> Say something useful, not copy of subject. >> >>> Signed-off-by: Daniel Machon <daniel.machon@microchip.com> >> >> A nit, subject: drop second/last, redundant "dt-bindings". The >> "dt-bindings" prefix is already stating that these are bindings. >> See also: >> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > > Duly noted. > >> >> >>> Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com> >>> --- >>> .../bindings/phy/microchip,sparx5-serdes.yaml | 18 ++++++++++++++++-- >>> 1 file changed, 16 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml b/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml >>> index bdbdb3bbddbe..1e07a311e8a5 100644 >>> --- a/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml >>> +++ b/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml >>> @@ -8,6 +8,7 @@ title: Microchip Sparx5 Serdes controller >>> >>> maintainers: >>> - Steen Hegelund <steen.hegelund@microchip.com> >>> + - Daniel Machon <daniel.machon@microchip.com> >>> >>> description: | >>> The Sparx5 SERDES interfaces share the same basic functionality, but >>> @@ -62,12 +63,17 @@ description: | >>> * 10.3125 Gbps (10GBASE-R/10GBASE-KR/USXGMII) >>> * 25.78125 Gbps (25GBASE-KR/25GBASE-CR/25GBASE-SR/25GBASE-LR/25GBASE-ER) >>> >>> + lan969x has ten SERDES10G interfaces that share the same features, operating >>> + modes and data rates as the equivalent Sparx5 SERDES10G interfaces. >>> + >>> properties: >>> $nodename: >>> pattern: "^serdes@[0-9a-f]+$" >>> >>> compatible: >>> - const: microchip,sparx5-serdes >>> + enum: >>> + - microchip,sparx5-serdes >>> + - microchip,lan969x-serdes >> >> It seems there is no lan969x SoC/chip. Are you sure you are using >> correct naming, matching what kernel is using? Maybe you just sent >> whatever you had in downstream (hint: that's never a good idea). > > You are right. There is no upstream support for lan969x SoC yet. The > upstreaming of the lan969x SoC has just begun, and this series is part > of that upstreaming effort. The lan969x switch driver (not submitted > yet) will depend on this SERDES driver, however, their functionality is > really independent of each other. That is why I am also upstreaming the > SERDES- and switch driver series independent of each other. That's not exactly my point. Becayse lan969x appears. I claim you use incorrect name, so are you sure you do not use wildcards? Best regards, Krzysztof
> >>> compatible: > >>> - const: microchip,sparx5-serdes > >>> + enum: > >>> + - microchip,sparx5-serdes > >>> + - microchip,lan969x-serdes > >> > >> It seems there is no lan969x SoC/chip. Are you sure you are using > >> correct naming, matching what kernel is using? Maybe you just sent > >> whatever you had in downstream (hint: that's never a good idea). > > > > You are right. There is no upstream support for lan969x SoC yet. The > > upstreaming of the lan969x SoC has just begun, and this series is part > > of that upstreaming effort. The lan969x switch driver (not submitted > > yet) will depend on this SERDES driver, however, their functionality is > > really independent of each other. That is why I am also upstreaming the > > SERDES- and switch driver series independent of each other. > > That's not exactly my point. Becayse lan969x appears. I claim you use > incorrect name, so are you sure you do not use wildcards? > Best regards, > Krzysztof Ahh. So the problem is the 'x' in lan969x, right? I think we have a habbit of documenting compatible strings like this in bindings. Anyway, what I can do is document the different part numbers in the bindings: lan9691, lan9692, lan9693, lan9694, lan9696 and lan9698. /Daniel
On 09/09/2024 11:43, Daniel Machon wrote: >>>>> compatible: >>>>> - const: microchip,sparx5-serdes >>>>> + enum: >>>>> + - microchip,sparx5-serdes >>>>> + - microchip,lan969x-serdes >>>> >>>> It seems there is no lan969x SoC/chip. Are you sure you are using >>>> correct naming, matching what kernel is using? Maybe you just sent >>>> whatever you had in downstream (hint: that's never a good idea). >>> >>> You are right. There is no upstream support for lan969x SoC yet. The >>> upstreaming of the lan969x SoC has just begun, and this series is part >>> of that upstreaming effort. The lan969x switch driver (not submitted >>> yet) will depend on this SERDES driver, however, their functionality is >>> really independent of each other. That is why I am also upstreaming the >>> SERDES- and switch driver series independent of each other. >> >> That's not exactly my point. Becayse lan969x appears. I claim you use >> incorrect name, so are you sure you do not use wildcards? >> Best regards, >> Krzysztof > > Ahh. > > So the problem is the 'x' in lan969x, right? I think we have a habbit of > documenting compatible strings like this in bindings. Anyway, what I can > do is document the different part numbers in the bindings: lan9691, > lan9692, lan9693, lan9694, lan9696 and lan9698. Depends. I remember I was confused about such wildcards before and for some cases wildcard was ok. For some not. I don't remember, I don't know which case is here. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml b/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml index bdbdb3bbddbe..1e07a311e8a5 100644 --- a/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml +++ b/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml @@ -8,6 +8,7 @@ title: Microchip Sparx5 Serdes controller maintainers: - Steen Hegelund <steen.hegelund@microchip.com> + - Daniel Machon <daniel.machon@microchip.com> description: | The Sparx5 SERDES interfaces share the same basic functionality, but @@ -62,12 +63,17 @@ description: | * 10.3125 Gbps (10GBASE-R/10GBASE-KR/USXGMII) * 25.78125 Gbps (25GBASE-KR/25GBASE-CR/25GBASE-SR/25GBASE-LR/25GBASE-ER) + lan969x has ten SERDES10G interfaces that share the same features, operating + modes and data rates as the equivalent Sparx5 SERDES10G interfaces. + properties: $nodename: pattern: "^serdes@[0-9a-f]+$" compatible: - const: microchip,sparx5-serdes + enum: + - microchip,sparx5-serdes + - microchip,lan969x-serdes reg: minItems: 1 @@ -90,11 +96,19 @@ additionalProperties: false examples: - | - serdes: serdes@10808000 { + serdes@10808000 { compatible = "microchip,sparx5-serdes"; #phy-cells = <1>; clocks = <&sys_clk>; reg = <0x10808000 0x5d0000>; }; + - | + serdes@e3410000 { + compatible = "microchip,lan969x-serdes"; + #phy-cells = <1>; + clocks = <&fabric_clk>; + reg = <0xe3410000 0x150000>; + }; + ...