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

[neil.armstrong@linaro.org]

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@amlogic.com>
>>>>>> + *
>>>>>> + * Copyright (c) 2023, SberDevices. All Rights Reserved.
>>>>>> + * Author: Dmitry Rokosov <ddrokosov@sberdevices.ru>
>>>>>> + */
>>>>>> +
>>>>>> +#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@linaro.org/

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

Neil

> 
> Best regards,
> Krzysztof
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic