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@xxxxxxxxxxx> > > > > > --- > > > > > .../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.
Attachment:
signature.asc
Description: PGP signature
