[1/2] dt-bindings: net: rockchip-dwmac: add rk3568 xpcs compatible

  The gmac of RK3568 supports RGMII/SGMII/QSGMII interface.
This patch adds a compatible string for the required clock.

Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
---
 Documentation/devicetree/bindings/net/rockchip-dwmac.yaml | 6 ++++++
 1 file changed, 6 insertions(+)
  

On 29/11/2022 08:27, Chukun Pan wrote:
> The gmac of RK3568 supports RGMII/SGMII/QSGMII interface.
> This patch adds a compatible string for the required clock.
> 
> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
> ---
>  Documentation/devicetree/bindings/net/rockchip-dwmac.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> index 42fb72b6909d..36b1e82212e7 100644
> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> @@ -68,6 +68,7 @@ properties:
>          - mac_clk_rx
>          - aclk_mac
>          - pclk_mac
> +        - pclk_xpcs
>          - clk_mac_ref
>          - clk_mac_refout
>          - clk_mac_speed
> @@ -90,6 +91,11 @@ properties:
>        The phandle of the syscon node for the peripheral general register file.
>      $ref: /schemas/types.yaml#/definitions/phandle
>  
> +  rockchip,xpcs:
> +    description:
> +      The phandle of the syscon node for the peripheral general register file.

You used the same description as above, so no, you cannot have two
properties which are the same. syscons for GRF are called
"rockchip,grf", aren't they?


Best regards,
Krzysztof
  

On 29/11/2022 09:49, Krzysztof Kozlowski wrote:
> On 29/11/2022 08:27, Chukun Pan wrote:
>> The gmac of RK3568 supports RGMII/SGMII/QSGMII interface.
>> This patch adds a compatible string for the required clock.
>>
>> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
>> ---
>>  Documentation/devicetree/bindings/net/rockchip-dwmac.yaml | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
>> index 42fb72b6909d..36b1e82212e7 100644
>> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
>> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
>> @@ -68,6 +68,7 @@ properties:
>>          - mac_clk_rx
>>          - aclk_mac
>>          - pclk_mac
>> +        - pclk_xpcs
>>          - clk_mac_ref
>>          - clk_mac_refout
>>          - clk_mac_speed
>> @@ -90,6 +91,11 @@ properties:
>>        The phandle of the syscon node for the peripheral general register file.
>>      $ref: /schemas/types.yaml#/definitions/phandle
>>  
>> +  rockchip,xpcs:
>> +    description:
>> +      The phandle of the syscon node for the peripheral general register file.
> 
> You used the same description as above, so no, you cannot have two
> properties which are the same. syscons for GRF are called
> "rockchip,grf", aren't they?

Also:
1. Your commit msg does not explain it at all.
2. Your driver code uses this as some phy? Don't use syscon as
workaround for missing drivers.

Best regards,
Krzysztof
  

Am Dienstag, 29. November 2022, 09:49:08 CET schrieb Krzysztof Kozlowski:
> On 29/11/2022 08:27, Chukun Pan wrote:
> > The gmac of RK3568 supports RGMII/SGMII/QSGMII interface.
> > This patch adds a compatible string for the required clock.
> > 
> > Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
> > ---
> >  Documentation/devicetree/bindings/net/rockchip-dwmac.yaml | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> > index 42fb72b6909d..36b1e82212e7 100644
> > --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> > +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> > @@ -68,6 +68,7 @@ properties:
> >          - mac_clk_rx
> >          - aclk_mac
> >          - pclk_mac
> > +        - pclk_xpcs
> >          - clk_mac_ref
> >          - clk_mac_refout
> >          - clk_mac_speed
> > @@ -90,6 +91,11 @@ properties:
> >        The phandle of the syscon node for the peripheral general register file.
> >      $ref: /schemas/types.yaml#/definitions/phandle
> >  
> > +  rockchip,xpcs:
> > +    description:
> > +      The phandle of the syscon node for the peripheral general register file.
> 
> You used the same description as above, so no, you cannot have two
> properties which are the same. syscons for GRF are called
> "rockchip,grf", aren't they?

Not necessarily :-) .

The GRF is Rockchip's way of not sorting their own invented
additional registers. (aka a bunch of registers 

While on the older models there only ever was the one GRF
as dumping ground, newer SoCs now end up with multiple ones :-)

These are still iomem areas separate from the actual device-iomem they
work with/for but SoCs like the rk3568 now have at least 13 of them.


_But_ for the patch in question I fail to see what this set does at all.
The rk3568 (only) has XPCS_CON0 and XPCS_STATUS in its PIPE_GRF syscon
(according to the TRM), but the patch2 does strange things with
offset calculations and names that do not seem to have a match in the TRM.

So definitely more explanation on what happens here would be necessary.

Heiko
  

On 29/11/2022 10:56, Heiko Stübner wrote:
> Am Dienstag, 29. November 2022, 09:49:08 CET schrieb Krzysztof Kozlowski:
>> On 29/11/2022 08:27, Chukun Pan wrote:
>>> The gmac of RK3568 supports RGMII/SGMII/QSGMII interface.
>>> This patch adds a compatible string for the required clock.
>>>
>>> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
>>> ---
>>>  Documentation/devicetree/bindings/net/rockchip-dwmac.yaml | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
>>> index 42fb72b6909d..36b1e82212e7 100644
>>> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
>>> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
>>> @@ -68,6 +68,7 @@ properties:
>>>          - mac_clk_rx
>>>          - aclk_mac
>>>          - pclk_mac
>>> +        - pclk_xpcs
>>>          - clk_mac_ref
>>>          - clk_mac_refout
>>>          - clk_mac_speed
>>> @@ -90,6 +91,11 @@ properties:
>>>        The phandle of the syscon node for the peripheral general register file.
>>>      $ref: /schemas/types.yaml#/definitions/phandle
>>>  
>>> +  rockchip,xpcs:
>>> +    description:
>>> +      The phandle of the syscon node for the peripheral general register file.
>>
>> You used the same description as above, so no, you cannot have two
>> properties which are the same. syscons for GRF are called
>> "rockchip,grf", aren't they?
> 
> Not necessarily :-) .

OK, then description should have something like "...GRF for foo bar".


Best regards,
Krzysztof
  

Am Dienstag, 29. November 2022, 10:59:34 CET schrieb Krzysztof Kozlowski:
> On 29/11/2022 10:56, Heiko Stübner wrote:
> > Am Dienstag, 29. November 2022, 09:49:08 CET schrieb Krzysztof Kozlowski:
> >> On 29/11/2022 08:27, Chukun Pan wrote:
> >>> The gmac of RK3568 supports RGMII/SGMII/QSGMII interface.
> >>> This patch adds a compatible string for the required clock.
> >>>
> >>> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
> >>> ---
> >>>  Documentation/devicetree/bindings/net/rockchip-dwmac.yaml | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> >>> index 42fb72b6909d..36b1e82212e7 100644
> >>> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> >>> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> >>> @@ -68,6 +68,7 @@ properties:
> >>>          - mac_clk_rx
> >>>          - aclk_mac
> >>>          - pclk_mac
> >>> +        - pclk_xpcs
> >>>          - clk_mac_ref
> >>>          - clk_mac_refout
> >>>          - clk_mac_speed
> >>> @@ -90,6 +91,11 @@ properties:
> >>>        The phandle of the syscon node for the peripheral general register file.
> >>>      $ref: /schemas/types.yaml#/definitions/phandle
> >>>  
> >>> +  rockchip,xpcs:
> >>> +    description:
> >>> +      The phandle of the syscon node for the peripheral general register file.
> >>
> >> You used the same description as above, so no, you cannot have two
> >> properties which are the same. syscons for GRF are called
> >> "rockchip,grf", aren't they?
> > 
> > Not necessarily :-) .
> 
> OK, then description should have something like "...GRF for foo bar".

Actually looking deeper in the TRM, having these registers "just" written
to from the dwmac-glue-layer feels quite a bit like a hack.

The "pcs" thingy referenced in patch2 actually looks more like a real device
with its own section in the TRM and own iomem area. This pcs device then
itself has some more settings stored in said pipe-grf.

So this looks more like it wants to be an actual phy-driver.


@Chukun Pan: plase take a look at something like
https://elixir.bootlin.com/linux/latest/source/drivers/phy/mscc/phy-ocelot-serdes.c#L398
on how phy-drivers for ethernets could look like.

Aquiring such a phy from the dwmac-glue and calling phy_set_mode after
moving the xpcs_setup to a phy-driver shouldn't be too hard I think.


The qsgmii/sgmii_pcs list of registers in the TRM alone already takes up
4 A4 pages, so while using the PCS as syscon and just writing some values
into it might work now, this doesn't feel at all like a future-save handling.


Heiko
  

On Tue, Nov 29, 2022 at 11:22:28AM +0100, Heiko Stübner wrote:
> Am Dienstag, 29. November 2022, 10:59:34 CET schrieb Krzysztof Kozlowski:
> > On 29/11/2022 10:56, Heiko Stübner wrote:
> > > Am Dienstag, 29. November 2022, 09:49:08 CET schrieb Krzysztof Kozlowski:
> > >> On 29/11/2022 08:27, Chukun Pan wrote:
> > >>> The gmac of RK3568 supports RGMII/SGMII/QSGMII interface.
> > >>> This patch adds a compatible string for the required clock.
> > >>>
> > >>> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
> > >>> ---
> > >>>  Documentation/devicetree/bindings/net/rockchip-dwmac.yaml | 6 ++++++
> > >>>  1 file changed, 6 insertions(+)
> > >>>
> > >>> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> > >>> index 42fb72b6909d..36b1e82212e7 100644
> > >>> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> > >>> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> > >>> @@ -68,6 +68,7 @@ properties:
> > >>>          - mac_clk_rx
> > >>>          - aclk_mac
> > >>>          - pclk_mac
> > >>> +        - pclk_xpcs
> > >>>          - clk_mac_ref
> > >>>          - clk_mac_refout
> > >>>          - clk_mac_speed
> > >>> @@ -90,6 +91,11 @@ properties:
> > >>>        The phandle of the syscon node for the peripheral general register file.
> > >>>      $ref: /schemas/types.yaml#/definitions/phandle
> > >>>  
> > >>> +  rockchip,xpcs:
> > >>> +    description:
> > >>> +      The phandle of the syscon node for the peripheral general register file.
> > >>
> > >> You used the same description as above, so no, you cannot have two
> > >> properties which are the same. syscons for GRF are called
> > >> "rockchip,grf", aren't they?
> > > 
> > > Not necessarily :-) .
> > 
> > OK, then description should have something like "...GRF for foo bar".
> 
> Actually looking deeper in the TRM, having these registers "just" written
> to from the dwmac-glue-layer feels quite a bit like a hack.
> 
> The "pcs" thingy referenced in patch2 actually looks more like a real device
> with its own section in the TRM and own iomem area. This pcs device then
> itself has some more settings stored in said pipe-grf.
> 
> So this looks more like it wants to be an actual phy-driver.

There's a PCS binding now. Seems like it should be used if there is 
also a PHY already. PCS may be part of the PHY or separate block AIUI.

Rob
  

> Actually looking deeper in the TRM, having these registers "just" written
> to from the dwmac-glue-layer feels quite a bit like a hack.

> The "pcs" thingy referenced in patch2 actually looks more like a real device
> with its own section in the TRM and own iomem area. This pcs device then
> itself has some more settings stored in said pipe-grf.

> So this looks more like it wants to be an actual phy-driver.

> @Chukun Pan: plase take a look at something like
> https://elixir.bootlin.com/linux/latest/source/drivers/phy/mscc/phy-ocelot-serdes.c#L398
> on how phy-drivers for ethernets could look like.

> Aquiring such a phy from the dwmac-glue and calling phy_set_mode after
> moving the xpcs_setup to a phy-driver shouldn't be too hard I think.

Thanks for pointing that out.
The patch2 is come from the sdk kernel of rockchip.
The sgmii-phy of RK3568 is designed on nanning combo phy.
In the sdk kernel, if we want to use sgmii mode, we need
to modify the device tree in the gmac section like this:

```
&gmac0 {
	power-domains = <&power RK3568_PD_PIPE>;
	phys = <&combphy1_usq PHY_TYPE_SGMII>;
	phy-handle = <&sgmii_phy>;
	phy-mode = "sgmii";
	rockchip,pipegrf = <&pipegrf>;
	rockchip,xpcs = <&xpcs>;
	status = "okay";
};
```

I'm not sure how to write this on the mainline kernel.
Any hint will be appreciated.

--
Thanks,
Chukun
  

On Sat, Dec 03, 2022 at 05:00:15PM +0800, Chukun Pan wrote:
> > Actually looking deeper in the TRM, having these registers "just" written
> > to from the dwmac-glue-layer feels quite a bit like a hack.
> 
> > The "pcs" thingy referenced in patch2 actually looks more like a real device
> > with its own section in the TRM and own iomem area. This pcs device then
> > itself has some more settings stored in said pipe-grf.
> 
> > So this looks more like it wants to be an actual phy-driver.
> 
> > @Chukun Pan: plase take a look at something like
> > https://elixir.bootlin.com/linux/latest/source/drivers/phy/mscc/phy-ocelot-serdes.c#L398
> > on how phy-drivers for ethernets could look like.
> 
> > Aquiring such a phy from the dwmac-glue and calling phy_set_mode after
> > moving the xpcs_setup to a phy-driver shouldn't be too hard I think.
> 
> Thanks for pointing that out.
> The patch2 is come from the sdk kernel of rockchip.
> The sgmii-phy of RK3568 is designed on nanning combo phy.
> In the sdk kernel, if we want to use sgmii mode, we need
> to modify the device tree in the gmac section like this:
> 
> ```
> &gmac0 {
> 	power-domains = <&power RK3568_PD_PIPE>;
> 	phys = <&combphy1_usq PHY_TYPE_SGMII>;
> 	phy-handle = <&sgmii_phy>;
> 	phy-mode = "sgmii";

phy-mode tells you you are using SGMII. You can tell the generic PHY
driver this which will call the PHY drivers .set_mode().

As said above, there are plenty of examples of this, mvneta and its
comphy, various mscc drivers etc.

	Andrew