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

[Sumit Garg <sumit.garg@kernel.org>]

On Mon, Mar 03, 2025 at 10:26:15AM +0000, Manikandan.M@microchip.com wrote:
> 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@linaro.org> wrote:
> >>
> >>
> >>
> >> On 2/28/25 12:58, Sumit Garg wrote:
> >>> On Fri, 28 Feb 2025 at 15:20, <Manikandan.M@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@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@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@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
>

Sounds reasonable to me.

-Sumit