Re: [PATCH v2 02/13] arm64: dts: ti: k3-am642-phyboard-electra-rdk: fix USB clocking for compliance

[Nishanth Menon]

On 14:15-20260507, Siddharth Vadapalli wrote:
[...]

> > > diff --git a/arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts b/arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts
> > > index 793538f94942..a85d7d08bd1b 100644
> > > --- a/arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts
> > > +++ b/arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts
> > > @@ -439,12 +439,21 @@ &sdhci1 {
> > >   	status = "okay";
> > >   };
> > > +&serdes_wiz0 {
> > > +	ti,core-clk-sel = <1>;  /* Select internal reference clock */

Doesn't the binding give the info?
> > > +	ti,ssc-enable; /* Enable SSC */

That comment is what the property says

> > > +	ti,ssc-type = <1>; /* 1 for Downspread */

If it is that critical should we have a include header or if the binding
describes this, that should suffice, no?

> > > +	ti,ssc-frequency-hz = <33000>; /* 33 KHz */

33000 Hz is 33Khz.

> > > +	ti,ssc-depth-per-mil = <5>; /* 0.5% depth */

Binding should describe this?

> > 
> > I don't think the comments are very helpful. The property names already give a meaning.
> 
> The comments have been added for three reasons:
> 1. The meaning of the following properties isn't obvious:
> 	ti,core-clk-sel = <1>
> 	ti,ssc-type = <1>
> 2. For ease of 'grepping'. Grepping for '33 KHz' for example based on the
> USB 3.2 Specification's modulation rate will not show '33000' in the
> results.
> 3. Completeness / Consistency. Since some of the less obvious properties
> have been described via comments, the remaining have also been commented on,
> although it is obvious what it means (ti,ssc-enable for example).
> 
> Unless you have a strong objection to removing the comments, I would prefer
> retaining them. Please let me know.

Just keep the necessary documentation - something that we cannot
determine by bindings.. If we have to document in ever single dts, it
might mean, you need a header to make it explicit? IRQ_TYPE_ kind of
macro? IMHO (I leave it to subsystem maintainers), I feel these are
better documented in the bindings. Also are these properties expected
to be exactly same on all evms of a given SoC (wondering it they belong
to SoC.dtsi instead)?

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D
https://ti.com/opensource