Re: [PATCH v8 net-next 01/10] dt-bindings: net: dsa: dt bindings for microchip lan937x

[Prasanna Vengateshan <prasanna.vengateshan@microchip.com>]

Hi Rob/Florian,

On Wed, 2022-03-02 at 17:11 +0530, Prasanna Vengateshan wrote:
> Hi Rob and Florian,
> 
> On Fri, 2022-02-11 at 19:56 -0800, Florian Fainelli wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> > 
> > On 2/9/2022 3:58 AM, Prasanna Vengateshan wrote:
> > > On Mon, 2022-02-07 at 18:53 -0800, Florian Fainelli wrote:
> > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > > > the
> > > > content is safe
> > > > 
> > > > On 2/7/2022 9:21 AM, Prasanna Vengateshan wrote:
> > > > > Documentation in .yaml format and updates to the MAINTAINERS
> > > > > Also 'make dt_binding_check' is passed.
> > > > > 
> > > > > RGMII internal delay values for the mac is retrieved from
> > > > > rx-internal-delay-ps & tx-internal-delay-ps as per the feedback from
> > > > > v3 patch series.
> > > > > https://lore.kernel.org/netdev/20210802121550.gqgbipqdvp5x76ii@skbuf/
> > > > > 
> > > > > It supports only the delay value of 0ns and 2ns.
> > > > > 
> > > > > Signed-off-by: Prasanna Vengateshan <
> > > > > prasanna.vengateshan@microchip.com>
> > > > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > > > ---
> > > > >    .../bindings/net/dsa/microchip,lan937x.yaml   | 179
> > > > > ++++++++++++++++++
> > > > >    MAINTAINERS                                   |   1 +
> > > > >    2 files changed, 180 insertions(+)
> > > > >    create mode 100644
> > > > > Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
> > > > > 
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  mdio:
> > > > > +    $ref: /schemas/net/mdio.yaml#
> > > > > +    unevaluatedProperties: false
> > > > 
> > > > This should be moved to dsa.yaml since this is about describing the
> > > > switch's internal MDIO bus controller. This is applicable to any switch,
> > > > really.
> > > 
> > > Thanks for your review and feedback. Do you mean that 'mdio' to be added
> > > in
> > > dsa.yaml instead adding here?
> > 
> > Yes indeed, since this is a common property of all DSA switches, it can
> > be defined or not depending on whether the switch does have an internal
> > MDIO bus controller or not.
> > 
> > > 
> > > > 
> > > > > +
> > > > > +patternProperties:
> > > > > +  "^(ethernet-)?ports$":
> > > > > +    patternProperties:
> > > > > +      "^(ethernet-)?port@[0-7]+$":
> > > > > +        allOf:
> > > > > +          - if:
> > > > > +              properties:
> > > > > +                phy-mode:
> > > > > +                  contains:
> > > > > +                    enum:
> > > > > +                      - rgmii
> > > > > +                      - rgmii-rxid
> > > > > +                      - rgmii-txid
> > > > > +                      - rgmii-id
> > > > > +            then:
> > > > > +              properties:
> > > > > +                rx-internal-delay-ps:
> > > > > +                  $ref: "#/$defs/internal-delay-ps"
> > > > > +                tx-internal-delay-ps:
> > > > > +                  $ref: "#/$defs/internal-delay-ps"
> > > > 
> > > > Likewise, this should actually be changed in ethernet-controller.yaml
> > > 
> > > There is *-internal-delay-ps property defined for mac in ethernet-
> > > controller.yaml. Should that be changed like above?
> > 
> > It seems to me that these properties override whatever 'phy-mode'
> > property is defined, but in premise you are right that this is largely
> > applicable to RGMII only. I seem to recall that the QCA8K driver had
> > some sort of similar delay being applied even in SGMII mode but I am not
> > sure if we got to the bottom of this.
> > 
> > Please make sure that this does not create regressions for other DTS in
> > the tree before going with that change in ethernet-controller.yaml.
> > 
> 
> I just tried changing rx-internal-delay-ps & tx-internal-delay-ps on
> conditional
> basis like above in the ethernet-controller.yaml and it passed 'make
> dt_binding_check' as well. 
> 
> It would be like below if existing *-internal-delay-ps are removed from
> ethernet-controller.yaml.
> 
> allOf:
>   - if:
>       properties:
>         phy-mode:
>           contains:
>             enum:
>               - rgmii
>               - rgmii-rxid
>               - rgmii-txid
>               - rgmii-id
>             then:
>               properties:
>                 rx-internal-delay-ps:
>                   description:
>                     RGMII Receive Clock Delay defined in pico seconds.This is 
>                     used for controllers that have configurable RX internal 
>                     delays. If this property is present then the MAC applies 
>                     the RX delay.
>                 tx-internal-delay-ps:
>                   description:
>                     RGMII Transmit Clock Delay defined in pico seconds.This is
>                     used for controllers that have configurable TX internal
>                     delays. If this property is present then the MAC applies
>                     the TX delay.   
> 
> After the above changes, these two properties descriptions are different
> compare
> to other properties. So i just wanted to know whether i am following the right
> approach or are there any other proposal available? Thanks.
> 
> Prasanna V
> 
Is the above method looking good? Your feedback will be helpful. Thanks.

Prasanna V