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

[Christophe ROULLIER <christophe.roullier@foss.st.com>]

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
>