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@xxxxxxxxxx/ > > 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@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. Best regards, Krzysztof
