Re: [PATCH v10 3/5] dt-bindings: clock: meson: add A1 PLL and Peripherals clkcs bindings

[neil . armstrong]

Hi,

On 14/03/2023 16:19, Krzysztof Kozlowski wrote:
> On 14/03/2023 16:01, Dmitry Rokosov wrote:
> > On Tue, Mar 14, 2023 at 03:05:48PM +0100, Krzysztof Kozlowski wrote:
> > > On 14/03/2023 12:48, Dmitry Rokosov wrote:
> > > > On Tue, Mar 14, 2023 at 12:28:40PM +0100, Krzysztof Kozlowski wrote:
> > > > > On 13/03/2023 21:12, Dmitry Rokosov wrote:

<snip>

> > > > > > +/*
> > > > > > + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
> > > > > > + * Author: Jian Hu <jian.hu@xxxxxxxxxxx>
> > > > > > + *
> > > > > > + * Copyright (c) 2023, SberDevices. All Rights Reserved.
> > > > > > + * Author: Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxx>
> > > > > > + */
> > > > > > +
> > > > > > +#ifndef __A1_PLL_CLKC_H
> > > > > > +#define __A1_PLL_CLKC_H
> > > > > > +
> > > > > > +#define CLKID_FIXED_PLL		1
> > > > > > +#define CLKID_FCLK_DIV2		6
> > > > > > +#define CLKID_FCLK_DIV3		7
> > > > > > +#define CLKID_FCLK_DIV5		8
> > > > > > +#define CLKID_FCLK_DIV7		9
> > > > > > +#define CLKID_HIFI_PLL		10
> > > > >
> > > > >
> > > > > Probably I asked about this... why indices are not continuous? You know
> > > > > that consumers are allowed to use number 2 and it will be your ABI, even
> > > > > though you did not write it in the binding? That's a tricky and
> > > > > confusing pattern for no real gains.
> > > >
> > > > Actually, indices are continuou but splitted into two parts: public and
> > > > private. The public part is located in the dt bindings and can be included
> > > > from device tree sources. The private part is in the drivers/clk/meson
> > > > folder, and only clk drivers can use it.
> > > > I know, there is some trick when the user just inserts a digit value and
> > > > doesn't use constants.
> > >
> > > This is not a trick. This is how DTS works. You have only indices/numbers.
> > >
> > > > But I'm starting from the assumption that such
> > > > dts changes will not be approved by maintainers. In other words, the user
> > > > *must* apply defined ABI constants from dt bindings; it's a strong
> > > > restriction.
> > >
> > > But it is not correct assumption. Defines are very important, but they
> > > are just helpers. Otherwise without defines you could not use any clock?
> > > We pretty often use IDs - for DTS to allow merging via different trees,
> > > for DT binding examples to not rely on headers.
> > >
> > > Your driver implements the ABI and the driver exposes for example clock
> > > ID=2, even if it is not in the header.
> > >
> > > These IDs are unfortunately undocumented ABI and you if you change them,
> > > users are allowed to complain.
> > >
> > > Solution: don't do this. Have all exposed clock IDs and clocks in sync
> > > (and continuous).
> >
> > I see. But I don't understand how I can restrict access to private
> > clock objects. I don't want to open ability to change system clocks
> > parents, for example. Or it's under device tree developer responsibility?
> > I would appreciate any assistance in determining the best path.
>
> There are many ways - depend on your driver. For example like this:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/samsung/clk-exynos5420.c#n975
>
> The first argument is the clock ID (or ignore).
>
> BTW, quite likely the problem is generic to all Meson clock drivers.

This issue about "public" non-continuous defined was already discussed at https://lore.kernel.org/all/c088e01c-0714-82be-8347-6140daf56640@xxxxxxxxxx/

I don't see what's different with this one.

Neil

> Best regards,
> Krzysztof