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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230530-nineteen-entryway-cab54d3e3624@wendy>
Date: Tue, 30 May 2023 08:22:25 +0100
From: Conor Dooley <conor.dooley@...rochip.com>
To: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
CC: Conor Dooley <conor@...nel.org>, Andrew Lunn <andrew@...n.ch>,
	"s.shtylyov@....ru" <s.shtylyov@....ru>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	"krzysztof.kozlowski+dt@...aro.org" <krzysztof.kozlowski+dt@...aro.org>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>, "geert+renesas@...der.be"
	<geert+renesas@...der.be>, "magnus.damm@...il.com" <magnus.damm@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCH net-next 1/5] dt-bindings: net: r8a779f0-ether-switch:
 Add ACLK

Hey,

On Tue, May 30, 2023 at 12:19:46AM +0000, Yoshihiro Shimoda wrote:
> > From: Conor Dooley, Sent: Tuesday, May 30, 2023 5:44 AM
> > 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@...esas.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.
> 
> I'm sorry for lacking explanation. You're correct. this is backwards
> compatibility with old DT blobs.
> 
> > 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 ;)
> 
> The following clock rates are the same (400MHz):
>  - default rate (RSWITCH_ACLK_DEFAULT) in the C code
>  - R8A779F0_CLK_S0D2_HSC from dtb
> 
> Only for backwards compatibility with old DT blobs, I added
> the RSWITCH_ACLK_DEFAULT, and got the aclk as optional.
> 
> By the way, R8A779F0_CLK_S0D2_HSC is fixed rate, and the r8a779f0-ether-switch
> only uses the rswitch driver. Therefore, the clock rate is always 400MHz.
> So, I'm thinking that the following implementation is enough.
>  - no dt-bindings change. (In other words, don't add aclk in the dt-bindings.)
>  - hardcoded the clock rate in the C code as 400MHz.
> 
> > > 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.
> 
> Thank you very much for your comments! The it works perfectly fine for
> all use cases without the second clock & just using the default rate.
> That's why I'm now thinking that adding aclk into the dt-bindings is not
> a good way...

I am biased, but I think the binding should describe the hardware &
therefore the additional clock should be added.

Cheers,
Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)