[PATCH v2 1/8] dt-bindings: clk: define additional PMC clocks

[Manikandan.M at microchip.com]

Hi Eugen and Sumit,

On 28/02/25 5:07 pm, Sumit Garg wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, 28 Feb 2025 at 16:45, Eugen Hristev <eugen.hristev at linaro.org> wrote:
>>
>>
>>
>> On 2/28/25 12:58, Sumit Garg wrote:
>>> On Fri, 28 Feb 2025 at 15:20, <Manikandan.M at microchip.com> wrote:
>>>>
>>>> Hi Eugen,
>>>>
>>>> On 27/02/25 7:48 pm, Eugen Hristev wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On 2/27/25 12:37, Manikandan.M at microchip.com wrote:
>>>>>> Hi Sumit,
>>>>>>
>>>>>> On 27/02/25 3:14 pm, Sumit Garg wrote:
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>
>>>>>>> On Thu, 27 Feb 2025 at 15:06, Manikandan Muralidharan
>>>>>>> <manikandan.m at microchip.com> wrote:
>>>>>>>>
>>>>>>>> Add PMC clock definitions for MCK and UTMI which will be required
>>>>>>>> for the sam9x7 OF_upstream DT since the clock framework is not in
>>>>>>>> sync with Linux and also include this header in 'clock/at91.h' file
>>>>>>>
>>>>>>> You should rather drop these local DT bindings headers which will
>>>>>>> allow dts/upstream/include/dt-bindings/clock/at91.h to be included
>>>>>>> automatically.
>>>>>> Other SoC DTs where OF_UPSTREAM migration is not added yet, depends on
>>>>>> the local DT bindings header, dropping this will lead to issues with
>>>>>> compilation.
>>>>>> We can drop this altogether when we sync the u-boot clock framework with
>>>>>> Linux.
>>>>>>>
>>>>>>> -Sumit
>>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Manikandan Muralidharan <manikandan.m at microchip.com>
>>>>>>>> ---
>>>>>>>>     include/dt-bindings/clk/at91.h   | 3 +++
>>>>>>>>     include/dt-bindings/clock/at91.h | 2 ++
>>>>>>>>     2 files changed, 5 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/include/dt-bindings/clk/at91.h b/include/dt-bindings/clk/at91.h
>>>>>>>> index a178b94157b..016c6e0c620 100644
>>>>>>>> --- a/include/dt-bindings/clk/at91.h
>>>>>>>> +++ b/include/dt-bindings/clk/at91.h
>>>>>>>> @@ -24,4 +24,7 @@
>>>>>>>>     #define USB_UTMI2              1
>>>>>>>>     #define USB_UTMI3              2
>>>>>>>>
>>>>>>>> +#define PMC_MCK                        1
>>>>>>>> +#define PMC_UTMI               2
>>>>>
>>>>> Where in the patch series do you need these defines ?
>>>>>
>>>>>>>> +
>>>>>>>>     #endif
>>>>>>>> diff --git a/include/dt-bindings/clock/at91.h b/include/dt-bindings/clock/at91.h
>>>>>>>> index ab3ee241d10..7235b3ba01e 100644
>>>>>>>> --- a/include/dt-bindings/clock/at91.h
>>>>>>>> +++ b/include/dt-bindings/clock/at91.h
>>>>>>>> @@ -6,6 +6,8 @@
>>>>>>>>      * Licensed under GPLv2 or later.
>>>>>>>>      */
>>>>>>>>
>>>>>>>> +#include <dt-bindings/clk/at91.h>
>>>>>>>> +
>>>>>
>>>>> I find this odd to include one at91.h in another at91.h
>>>>>
>>>>> Let's consider to remove one of these files in the future, and have just
>>>>> one that is identical with the bindings one from Linux
>>>>>
>>>>> Meanwhile, let's see where do you need the PMC_*
>>>> The PMC_MCK and PMC_UTMI are defined in the dts/upstream sam9x7 SoC DT.
>>>> since during compilation the clock/at91.h from u-boot is used and to
>>>> resolve the syntax issues I had to declare them in clk/at91.h and
>>>> include the header in clock/at91.h
>>
>> I don't understand this. So , compiling sam9x7 upstream DT with upstream
>> at91.h should work.
>> Do you have issues with sam9x7-u-boot.dtsi that fails build ?
>>
>>>>
>>>> if we drop the clock/at91.h from u-boot, the sam9x75 will pass using the
>>>> includes from upstream Linux but will break other SoC that has not
>>>> migrated to OF_UPSTREAM yet.
>>
>> So you need to include clock/at91.h from upstream for sam9x75 and the
>> other at91.h for older SoC.
>> Does that work ?
> 
> The way it works currently is you can have a single clock/at91.h
> available where preference is currently given to local U-Boot copy
> (for backwards compatibility) over what is available in upstream DT.
> So can't have different versions of clock/at91.h available.
> 
>>
>>>>
>>>> Or we can align u-boot's clock/at91.h with Linux and drop clk/at91.h,
>>>> replace it with clock/at91.h in drivers and DT.
>>>> Please let me know if that works.
>>>
>>> I would rather favor the U-Boot drivers and DT to directly use
>>> dts/upstream/include/dt-bindings/clock/at91.h instead and drop local
>>> DT bindings import. It then becomes easier for other SoCs to migrate
>>> to OF_UPSTREAM too.
>>
>> Sumit,
>>
>> The problem is that there are two at91.h with different definitions for
>> the same macros. (I know... legacy reasons..)
>> The drivers have to be reworked to cope with the new values. Meanwhile I
>> would say that at least the new SoCs should use the right macros/bindings
> 
> Since the drivers are also common for both new and older SoCs, it's
> rather better to adapt them to common bindings rather than supporting
> 2 different versions of clock/at91.h. If you want to do that as part
> of this series or later will be based on your preference.

To ensure consistency and compatibility with upstream, I'll rename the 
local clock/at91.h to clock/at91-pmc-status.h in U-Boot and update the 
relevant legacy SoC Device Trees to reflect this change.
This will allow the new sam9x7 SoC DT and driver to utilize the standard 
upstream dt-bindings/clock/at91.h without modifications.
We shall later refine at91-pmc-status.h during clock synchronization 
with Linux.
Please let me know if this sounds good

> 
> -Sumit
> 
>>
>>>
>>> -Sumit
>>>
>>>>>
>>>>>>>>     #ifndef _DT_BINDINGS_CLK_AT91_H
>>>>>>>>     #define _DT_BINDINGS_CLK_AT91_H
>>>>>>>>
>>>>>>>> --
>>>>>>>> 2.25.1
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>> --
>>>> Thanks and Regards,
>>>> Manikandan M.
>>>>
>>

-- 
Thanks and Regards,
Manikandan M.