Re: [PATCH net-next 1/5] dt-bindings: net: r8a779f0-ether-switch: Add ACLK

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2576 bytes --]

On Mon, May 29, 2023 at 10:11:12PM +0200, Andrew Lunn wrote:
> On Mon, May 29, 2023 at 07:36:03PM +0100, Conor Dooley wrote:
> > On Mon, May 29, 2023 at 05:08:36PM +0900, Yoshihiro Shimoda wrote:
> > > Add ACLK of GWCA which needs to calculate registers' values for
> > > rate limiter feature.
> > > 
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > ---
> > >  .../bindings/net/renesas,r8a779f0-ether-switch.yaml    | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml b/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > > index e933a1e48d67..cbe05fdcadaf 100644
> > > --- a/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > > +++ b/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > > @@ -75,7 +75,12 @@ properties:
> > >        - const: rmac2_phy
> > >  
> > >    clocks:
> > > -    maxItems: 1
> > > +    maxItems: 2
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: fck
> > > +      - const: aclk
> > 
> > Since having both clocks is now required, please add some detail in the
> > commit message about why that is the case. Reading it sounds like this
> > is an optional new feature & not something that is required.
> 
> This is something i wondered about, backwards compatibility with old
> DT blobs. In the C code it is optional, and has a default clock rate
> if the clock is not present.

Yeah, I did the cursory check of the code to make sure that an old dtb
would still function, which is part of why I am asking for the
explanation of the enforcement here. I'm not clear on what the
consequences of getting the default rate is. Perhaps if I read the whole
series and understood the code I would be, but this commit should
explain the why anyway & save me the trouble ;)

> So the yaml should not enforce an aclk member.

This however I could go either way on. If the thing isn't going to
function properly with the fallback rate, but would just limp on on
in whatever broken way it has always done, I would agree with making
the second clock required so that no new devicetrees are written in a
way that would put the hardware into that broken state.
On the other hand, if it works perfectly fine for some use cases without
the second clock & just using the default rathe then I don't think the
presence of the second clock should be enforced.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]