| Message ID | 35c12a115893d324db16ec6983afb5f1951fd4c9.1695058909.git.daniel@makrotopia.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: mtk_eth_soc: add paths and SerDes modes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 9 this patch: 9 |
| netdev/cc_maintainers | success | CCed 15 of 15 maintainers |
| netdev/build_clang | success | Errors and warnings before: 9 this patch: 9 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 9 this patch: 9 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 34 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Mon, Sep 18, 2023 at 11:26:34PM +0100, Daniel Golle wrote: > Add several phandles needed for Ethernet SerDes interfaces on the > MediaTek MT7988 SoC. > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > --- > .../devicetree/bindings/net/mediatek,net.yaml | 28 +++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/mediatek,net.yaml b/Documentation/devicetree/bindings/net/mediatek,net.yaml > index e74502a0afe86..78219158b96af 100644 > --- a/Documentation/devicetree/bindings/net/mediatek,net.yaml > +++ b/Documentation/devicetree/bindings/net/mediatek,net.yaml > @@ -385,6 +385,34 @@ allOf: > minItems: 2 > maxItems: 2 > > + mediatek,toprgu: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + Phandle to the syscon representing the reset controller. Use the reset binding > + > + mediatek,usxgmiisys: > + $ref: /schemas/types.yaml#/definitions/phandle-array > + minItems: 2 > + maxItems: 2 > + items: > + maxItems: 1 > + description: > + A list of phandle to the syscon node referencing the USXGMII PCS. Use the PCS binding > + > + mediatek,xfi-pextp: > + $ref: /schemas/types.yaml#/definitions/phandle-array > + minItems: 2 > + maxItems: 2 > + items: > + maxItems: 1 > + description: > + A list of phandle to the syscon node that handles the 10GE SerDes PHY. Use the phy binding (phys, not phy-handle for ethernet PHY). > + > + mediatek,xfi-pll: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + Phandle to the syscon node handling the 10GE SerDes clock setup. Use the clock binding > + > patternProperties: > "^mac@[0-1]$": > type: object > -- > 2.42.0
Hi Rob, thank you for the review! On Tue, Sep 19, 2023 at 01:09:09PM -0500, Rob Herring wrote: > On Mon, Sep 18, 2023 at 11:26:34PM +0100, Daniel Golle wrote: > > Add several phandles needed for Ethernet SerDes interfaces on the > > MediaTek MT7988 SoC. > > > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > --- > > .../devicetree/bindings/net/mediatek,net.yaml | 28 +++++++++++++++++++ > > 1 file changed, 28 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/net/mediatek,net.yaml b/Documentation/devicetree/bindings/net/mediatek,net.yaml > > index e74502a0afe86..78219158b96af 100644 > > --- a/Documentation/devicetree/bindings/net/mediatek,net.yaml > > +++ b/Documentation/devicetree/bindings/net/mediatek,net.yaml > > @@ -385,6 +385,34 @@ allOf: > > minItems: 2 > > maxItems: 2 > > > > + mediatek,toprgu: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: > > + Phandle to the syscon representing the reset controller. > > Use the reset binding I got an alternative implementation ready which implements an actual reset controller (by extending drivers/watchdog/mtk_wdt.c to cover also MT7988 and its addition sw-reset-enable bits) and uses single phandles for each reset bit assigned to the corresponding units instead of listing them all for the ethernet controller (maybe that's one step too far though...) However, as mentioned in the cover letter, using the Linux reset controller API (which having to use is a consequence of having to use the reset bindings) doesn't allow to simultanously deassert the resets of pextp, usxgmii pcs and/or sgmii pcs which is how the vendor implementation is doing it as all reset bits are on the same 32-bit register and the Ethernet driver is the only driver needing to access that register. Asserting the resets in sequence and subsequently deasserting in sequence works for me, but it will have to be confirmed to not create any problems because it's clearly a deviation from the behavior of the reference implementation. > > > + > > + mediatek,usxgmiisys: > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > + minItems: 2 > > + maxItems: 2 > > + items: > > + maxItems: 1 > > + description: > > + A list of phandle to the syscon node referencing the USXGMII PCS. > > Use the PCS binding Ack, I will ie. implement standalone PCS driver similar to eg. pcs-rzn1-miic.c. > > > + > > + mediatek,xfi-pextp: > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > + minItems: 2 > > + maxItems: 2 > > + items: > > + maxItems: 1 > > + description: > > + A list of phandle to the syscon node that handles the 10GE SerDes PHY. > > Use the phy binding (phys, not phy-handle for ethernet PHY). Ack, this can be implemented as a standalone PHY driver using PHY bindings. I will do that instead. > > > + > > + mediatek,xfi-pll: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: > > + Phandle to the syscon node handling the 10GE SerDes clock setup. > > Use the clock binding Does that imply that I should implement a clock driver whith only a single clock offering only a single operation ('enable') which would then do the magic register writes? While one part is actually identifyable as taking care of enabling a clock, I would not know how to meaningfully abstract the other (first) part, see vendor driver: /* Register to control USXGMII XFI PLL digital */ #define XFI_PLL_DIG_GLB8 0x08 #define RG_XFI_PLL_EN BIT(31) /* Register to control USXGMII XFI PLL analog */ #define XFI_PLL_ANA_GLB8 0x108 #define RG_XFI_PLL_ANA_SWWA 0x02283248 [...] /* Add software workaround for USXGMII PLL TCL issue */ regmap_write(ss->pll, XFI_PLL_ANA_GLB8, RG_XFI_PLL_ANA_SWWA); // How would you represent the line above using the abstractions of the // common clk framework? regmap_read(ss->pll, XFI_PLL_DIG_GLB8, &val); // that looks like it val |= RG_XFI_PLL_EN; // <- could be a abstracted regmap_write(ss->pll, XFI_PLL_DIG_GLB8, val); // in a meaningful way in clock driver. ... which is all we ever do on that regmap. Ever. Thanks in advance to everybody sharing their ideas and advises.
On 19/09/2023 22:50, Daniel Golle wrote: > Hi Rob, > > thank you for the review! > > On Tue, Sep 19, 2023 at 01:09:09PM -0500, Rob Herring wrote: >> On Mon, Sep 18, 2023 at 11:26:34PM +0100, Daniel Golle wrote: >>> Add several phandles needed for Ethernet SerDes interfaces on the >>> MediaTek MT7988 SoC. >>> >>> Signed-off-by: Daniel Golle <daniel@makrotopia.org> >>> --- >>> .../devicetree/bindings/net/mediatek,net.yaml | 28 +++++++++++++++++++ >>> 1 file changed, 28 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/net/mediatek,net.yaml b/Documentation/devicetree/bindings/net/mediatek,net.yaml >>> index e74502a0afe86..78219158b96af 100644 >>> --- a/Documentation/devicetree/bindings/net/mediatek,net.yaml >>> +++ b/Documentation/devicetree/bindings/net/mediatek,net.yaml >>> @@ -385,6 +385,34 @@ allOf: >>> minItems: 2 >>> maxItems: 2 >>> >>> + mediatek,toprgu: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: >>> + Phandle to the syscon representing the reset controller. >> >> Use the reset binding > > I got an alternative implementation ready which implements an actual > reset controller (by extending drivers/watchdog/mtk_wdt.c to cover > also MT7988 and its addition sw-reset-enable bits) and uses single > phandles for each reset bit assigned to the corresponding units > instead of listing them all for the ethernet controller (maybe that's > one step too far though...) > > However, as mentioned in the cover letter, using the Linux reset > controller API (which having to use is a consequence of having to use > the reset bindings) doesn't allow to simultanously deassert the > resets of pextp, usxgmii pcs and/or sgmii pcs which is how the vendor > implementation is doing it as all reset bits are on the same 32-bit > register and the Ethernet driver is the only driver needing to access > that register. You can have reset for entire register, why not? And even if current Linux implementation had some troubles with this, you could fix it. > >> >>> + >>> + mediatek,xfi-pll: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: >>> + Phandle to the syscon node handling the 10GE SerDes clock setup. >> >> Use the clock binding > > Does that imply that I should implement a clock driver whith only a > single clock offering only a single operation ('enable') which would > then do the magic register writes? Yes > > While one part is actually identifyable as taking care of enabling a > clock, I would not know how to meaningfully abstract the other (first) > part, see vendor driver: > > /* Register to control USXGMII XFI PLL digital */ > #define XFI_PLL_DIG_GLB8 0x08 > #define RG_XFI_PLL_EN BIT(31) > > /* Register to control USXGMII XFI PLL analog */ > #define XFI_PLL_ANA_GLB8 0x108 > #define RG_XFI_PLL_ANA_SWWA 0x02283248 > > [...] > > /* Add software workaround for USXGMII PLL TCL issue */ > regmap_write(ss->pll, XFI_PLL_ANA_GLB8, RG_XFI_PLL_ANA_SWWA); > // How would you represent the line above using the abstractions of the > // common clk framework? What is above line? Please do not ask us to decode your vendor code. You know, we also have nothing to do with it. And anyway, why do you need to abstract it? Why not writing unconditionally? > > regmap_read(ss->pll, XFI_PLL_DIG_GLB8, &val); // that looks like it > val |= RG_XFI_PLL_EN; // <- could be a abstracted > regmap_write(ss->pll, XFI_PLL_DIG_GLB8, val); // in a meaningful way in > clock driver. > > ... which is all we ever do on that regmap. Ever. Not only. You will also get all Linux infrastructure associated with this clock, so proper devlinks, sysfs/debug entries, automatic gating of unused clocks etc. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/net/mediatek,net.yaml b/Documentation/devicetree/bindings/net/mediatek,net.yaml index e74502a0afe86..78219158b96af 100644 --- a/Documentation/devicetree/bindings/net/mediatek,net.yaml +++ b/Documentation/devicetree/bindings/net/mediatek,net.yaml @@ -385,6 +385,34 @@ allOf: minItems: 2 maxItems: 2 + mediatek,toprgu: + $ref: /schemas/types.yaml#/definitions/phandle + description: + Phandle to the syscon representing the reset controller. + + mediatek,usxgmiisys: + $ref: /schemas/types.yaml#/definitions/phandle-array + minItems: 2 + maxItems: 2 + items: + maxItems: 1 + description: + A list of phandle to the syscon node referencing the USXGMII PCS. + + mediatek,xfi-pextp: + $ref: /schemas/types.yaml#/definitions/phandle-array + minItems: 2 + maxItems: 2 + items: + maxItems: 1 + description: + A list of phandle to the syscon node that handles the 10GE SerDes PHY. + + mediatek,xfi-pll: + $ref: /schemas/types.yaml#/definitions/phandle + description: + Phandle to the syscon node handling the 10GE SerDes clock setup. + patternProperties: "^mac@[0-1]$": type: object
Add several phandles needed for Ethernet SerDes interfaces on the MediaTek MT7988 SoC. Signed-off-by: Daniel Golle <daniel@makrotopia.org> --- .../devicetree/bindings/net/mediatek,net.yaml | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+)