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

[Krzysztof Kozlowski]

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?

>>
>> 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).

> 
> 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.

>>
>>> 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