On Tue, Mar 14, 2023 at 04:19:22PM +0100, 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: > >>> > >>> [...] > >>> > >>>>> +#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. > Okay, got it. So no reason to fix this difference. > > > > 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? > Other clocks bindings for another drivers. But you've explained the reason above, so skip it. > > 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). > Hmmm, I like this way. But first, I think we need to make meson clock driver overall situation clear. [...] -- Thank you, Dmitry
