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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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:
>>>
>>> [...]
>>>
>>>>> +#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 always run checkpatch before sending the next patch series. Checkpatch
> doesn't highlight this problem:
> 
> --------------
> $ rg SPDX a1_clkc_v10/v10-0003-dt-bindings-clock-meson-add-A1-PLL-and-Periphera.patch
> 32:+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> 111:+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> 188:+/* SPDX-License-Identifier: GPL-2.0+ */
> 294:+/* SPDX-License-Identifier: GPL-2.0+ */
> 
> $ ./scripts/checkpatch.pl --strict a1_clkc_v10/v10-0003-dt-bindings-clock-meson-add-A1-PLL-and-Periphera.patch
> total: 0 errors, 0 warnings, 0 checks, 259 lines checked

Hmm, my bad, that's something to fix/improve in checkpatch.

> 
> a1_clkc_v10/v10-0003-dt-bindings-clock-meson-add-A1-PLL-and-Periphera.patch has no obvious style problems and is ready for submission.
> --------------
> 
> I've got your point, will fix in the next version.
> 
>>> 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?
>>
> 
> By 'license issue' I meant your comment for the previous version at:
> https://lore.kernel.org/all/6a950a51-fe90-9163-b73d-0a396d7187ee@linaro.org/
> 
> I thought you mentioned the problem is in two license usage in the one
> line (GPL + MIT), I've fixed it to GPL2 only, and mentioned it in the
> changelog.

The comment was for a reason why the license here is different than in
the binding. Should be the same. Binding had license:
GPL-2.0-only OR BSD-2-Clause

> 
> I didn't know about the special requirement for a dt-bindings license, I've
> just checked other clock dt-bindings and found that license is different
> in the many places:
> 
> $ grep -r "SPDX" include/dt-bindings/clock | grep -v -e "GPL-2.0.*BSD-2-Clause" | wc -l
> 291
> 
> And Tegra Car 124 as an example for different license between yaml
> schema and binding header:
> $ grep "SPDX" include/dt-bindings/clock/tegra124-car.h
> /* SPDX-License-Identifier: GPL-2.0 */
> $ grep "SPDX" Documentation/devicetree/bindings/clock/nvidia,tegra124-car.yaml
> # SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause)

checkpatch has the correct license. Many files were licensed differently
*on purpose* so I asked about purpose here.

> 
> Anyway, it's not a problem to fix the license to the same value between
> header and yaml schema, I'll fix it in the next version.
> But based on the above experiments, other clock bindings should be fixed

Your binding has a correct license. What should be fixed?

> as well, checkpatch behavior should be extended for dt bindings headers
> licence checking.

Yes.

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

Best regards,
Krzysztof