| Message ID | 20221129072714.22880-1-amadeus@jmu.edu.cn |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/2] dt-bindings: net: rockchip-dwmac: add rk3568 xpcs compatible | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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
On 29/11/2022 08:27, Chukun Pan wrote: > From: David Wu <david.wu@rock-chips.com> > > The gmac of RK3568 supports RGMII/SGMII/QSGMII interface. > This patch adds the remaining SGMII/QSGMII support. Do not use "This commit/patch". https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > > Run-tested-on: Ariaboard Photonicat (GMAC0 SGMII) > > Signed-off-by: David Wu <david.wu@rock-chips.com> > [rebase, rewrite commit message] > Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn> > --- > .../net/ethernet/stmicro/stmmac/dwmac-rk.c | 210 +++++++++++++++++- > 1 file changed, 207 insertions(+), 3 deletions(-) > > > -static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable) > +static int rk_gmac_phy_power_on(struct rk_priv_data *bsp_priv, bool enable) > { > struct regulator *ldo = bsp_priv->regulator; > int ret; > @@ -1728,6 +1909,18 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev, > "rockchip,grf"); > bsp_priv->php_grf = syscon_regmap_lookup_by_phandle(dev->of_node, > "rockchip,php-grf"); > + bsp_priv->xpcs = syscon_regmap_lookup_by_phandle(dev->of_node, > + "rockchip,xpcs"); > + if (!IS_ERR(bsp_priv->xpcs)) { > + struct phy *comphy; > + > + comphy = devm_of_phy_get(&pdev->dev, dev->of_node, NULL); So instead of having PHY driver, you added a syscon and implemented PHY driver here. No. Make a proper PHY driver. 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
> > -static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable) > > +static int rk_gmac_phy_power_on(struct rk_priv_data *bsp_priv, bool enable) > > { > > struct regulator *ldo = bsp_priv->regulator; > > int ret; > > @@ -1728,6 +1909,18 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev, > > "rockchip,grf"); > > bsp_priv->php_grf = syscon_regmap_lookup_by_phandle(dev->of_node, > > "rockchip,php-grf"); > > + bsp_priv->xpcs = syscon_regmap_lookup_by_phandle(dev->of_node, > > + "rockchip,xpcs"); > > + if (!IS_ERR(bsp_priv->xpcs)) { > > + struct phy *comphy; > > + > > + comphy = devm_of_phy_get(&pdev->dev, dev->of_node, NULL); > > So instead of having PHY driver, you added a syscon and implemented PHY > driver here. No. Make a proper PHY driver. I'm also thinking there should be a proper pcs driver in drivers/net/pcs. Andrew
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
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. + $ref: /schemas/types.yaml#/definitions/phandle + tx_delay: description: Delay value for TXD timing. Range value is 0~0x7F, 0x30 as default. $ref: /schemas/types.yaml#/definitions/uint32
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(+)