Message-ID: <2d9297e9-dab7-9615-3859-79b3b2980d9a@linaro.org>
Date:   Tue, 14 Mar 2023 15:05:48 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v10 3/5] dt-bindings: clock: meson: add A1 PLL and
 Peripherals clkcs bindings
Content-Language: en-US
To:     Dmitry Rokosov <ddrokosov@sberdevices.ru>
Cc:     neil.armstrong@linaro.org, jbrunet@baylibre.com,
        mturquette@baylibre.com, sboyd@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, khilman@baylibre.com,
        martin.blumenstingl@googlemail.com, jian.hu@amlogic.com,
        kernel@sberdevices.ru, rockosov@gmail.com,
        linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
References: <20230313201259.19998-1-ddrokosov@sberdevices.ru>
 <20230313201259.19998-4-ddrokosov@sberdevices.ru>
 <ffebef1d-8447-181b-1890-3e638d399c62@linaro.org>
 <20230314114825.yiv4vcszr6b7m45w@CAB-WSD-L081021>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230314114825.yiv4vcszr6b7m45w@CAB-WSD-L081021>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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:
> 
> [...]
> 
>>> +#define CLKID_SPIFC		84
>>> +#define CLKID_USB_BUS		85
>>> +#define CLKID_SD_EMMC		86
>>> +#define CLKID_PSRAM		87
>>> +#define CLKID_DMC		88
>>
>> And what is here? Between 88 and 121?
>>
> 
> Explained below.
> 
>>> +#define CLKID_GEN_SEL		121
>>> +
>>> +#endif /* __A1_CLKC_H */
>>> diff --git a/include/dt-bindings/clock/amlogic,a1-pll-clkc.h b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
>>> new file mode 100644
>>> index 000000000000..8e97d3fb9d30
>>> --- /dev/null
>>> +++ b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
>>> @@ -0,0 +1,20 @@
>>> +/* SPDX-License-Identifier: GPL-2.0+ */
>>
>> I found in changelog:
>> "fix license issue, it's GPL-2.0+ only in the current version"
>> and I do not understand.
>>
>> The license is wrong, so what did you fix?
>>
> 
> Sorry don't get you. Why is it wrong?

Run checkpatch - it will tell you why wrong. The license is not correct.
This is part of binding and should be the same as binding.

> I've changed all new source files to GPL-2.0+ except yaml, because yaml
> dt bindings schemas require the following license:
> 
>     # SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> 
> I've pointed it in the changelog.

The only thing I found was:
"fix license issue, it's GPL-2.0+ only in the current version"

so what exactly you pointed out in changelog? What was to fix? What was
fixed? Correct license into incorrect? But why?

> 
>>> +/*
>>> + * 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).

Best regards,
Krzysztof