Re: [PATCH net-next v2 1/2] dt-bindings: net: mediatek,net: add phandles for SerDes on MT7988

[Krzysztof Kozlowski]

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@xxxxxxxxxxxxxx>
>>> ---
>>>  .../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