Re: [PATCH v2 2/2] dt-bindings: net: add new property st,ext-phyclk in documentation for stm32

[Christophe ROULLIER]

Hi

On 3/14/24 16:25, Krzysztof Kozlowski wrote:
> On 14/03/2024 16:10, Christophe ROULLIER wrote:
> > Hi,
> >
> > On 3/13/24 14:06, Krzysztof Kozlowski wrote:
> > > On 13/03/2024 11:39, Christophe ROULLIER wrote:
> > > > On 3/8/24 09:28, Krzysztof Kozlowski wrote:
> > > > > On 07/03/2024 14:59, Christophe Roullier wrote:
> > > > > > Add property st,ext-phyclk to manage cases when PHY have no cristal/quartz
> > > > > > This property can be used with RMII phy without cristal 50Mhz and when we
> > > > > > want to select RCC clock instead of ETH_REF_CLK
> > > > > > Can be used also with RGMII phy with no cristal and we select RCC clock
> > > > > > instead of ETH_CLK125
> > > > > Nothing improved here. You say you add new property (wrote it explicitly
> > > > > in the subject), but where is it? Where is the user?
> > > > >
> > > > > I think we talked about this. Rob also asked quite clear:
> > > > >
> > > > > > That is obvious from the diff. What is not obvious is why we need a new
> > > > > > property and what is the problem with the existing ones.
> > > > > How did you solve it?
> > > > Hi,
> > > >
> > > > I do not understand your questions.
> > > OK, I will clarify some questions, but are you sure that this question:
> > > "How did you solve it?"
> > > needs clarification?
> > >
> > > If so, then let me clarify:
> > > Rob pointed issue. How did you resolve Rob's comment? How did you
> > > address it? What changed in your patch, that Rob's comment should be
> > > considered as addressed/resolved/done?
> > This property was introduced in 2020 in order to simplify management of
> > all STM32 platforms without Ethernet cristal/quartz PHY.
> I fail to see how this answers how did you resolve the comment. You now
> described some sort of history, but I am asking: what did you change in
> your patches, so Rob's comment is considered resolved?

Concerning Rob's comment, in V2 I finally remove deprecated fields put 
in V1 to keep existing properties, which have no pb and can be used.

And I explained the meaning to add existing property in yaml.

> > > Now about my other question:
> > > "but where is it? Where is the user?"
> > >
> > > Your subject and commit message claim you add new property. This means
> > > such property was not existing so far in the Linux kernel. If you add
> > > new property in the binding, then I expect adding the user of that
> > > binding, thus my question: where is the user of that binding?
> > I'm preparing glue and DTS to upstream for STM32MP13 platform, this
> > platform will use with property.
> >
> > Since 2020, this property is available in the driver in kernel.org, so
> > it is also possible that someone who has not upstreamed their
> This should be explained in commit msg (although not kernel.org, website
> does not matter here).
ok I will add this in V3.
> > code also uses it.
> >
> > > > That I would like to do, it is property "st,ext-phyclk" was introduced
> > > > in driver
> > > >
> > > > "drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c" in 2020, and YAML
> > > > was not updated at the time.
> > > Are you saying you document existing property or add a new one?
> > Yes, existing property, since 2020 in kernel.org.
> Drop the website. We talk here about Linux kernel.
>
> Commit msg fails to explain it in a clear way.

ok I will add this in V3.

Thanks

> > > > Goal of this patch it is to update YAML to avoid dtbs check issue if
> > > > someone use this property :
> > > >
> > > >     dtbs check issue : views/kernel/upstream/net-next/arch/arm/boot/dts/st/stm32mp157c-dk2.dtb:
> > > > ethernet@5800a000: Unevaluated properties are not allowed
> > > > ('st,ext-phyclk' was unexpected)
> > > So DTS uses it?
> > Here it was example, if someone wants to use this property, but today
> > this property is not yet present in DTS in kernel.org
>
> Best regards,
> Krzysztof