| Message ID | 20230313201259.19998-4-ddrokosov@sberdevices.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add Amlogic A1 clock controller drivers | expand |
On 13/03/2023 21:12, Dmitry Rokosov wrote: > Add the documentation for Amlogic A1 PLL and Amlogic A1 Peripherals > clock drivers. > Introduce Amlogic A1 PLL and Amlogic A1 Peripherals device tree > bindings and include them to MAINTAINERS. > > > diff --git a/include/dt-bindings/clock/amlogic,a1-clkc.h b/include/dt-bindings/clock/amlogic,a1-clkc.h > new file mode 100644 > index 000000000000..2b16b1f1a5bf > --- /dev/null > +++ b/include/dt-bindings/clock/amlogic,a1-clkc.h > @@ -0,0 +1,100 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ ??? > +#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? > +#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? > +/* > + * 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. Best regards, Krzysztof
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? 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. > > +/* > > + * 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. 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.
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
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 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. 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) 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 as well, checkpatch behavior should be extended for dt bindings headers licence checking. > > > >>> +/* > >>> + * 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.
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
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 >
On 14/03/2023 16:33, neil.armstrong@linaro.org wrote: >> 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. So you are aware that all undocumented clock IDs are still allowed to use in DTS and they are ABI? Changing them will be an ABI break. Best regards, Krzysztof
On 14/03/2023 16:37, Krzysztof Kozlowski wrote: > On 14/03/2023 16:33, neil.armstrong@linaro.org wrote: >>> 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. > > So you are aware that all undocumented clock IDs are still allowed to > use in DTS and they are ABI? Changing them will be an ABI break. Yes of course. Neil > > Best regards, > Krzysztof >
On Tue, Mar 14, 2023 at 04:40:19PM +0100, neil.armstrong@linaro.org wrote: > On 14/03/2023 16:37, Krzysztof Kozlowski wrote: > > On 14/03/2023 16:33, neil.armstrong@linaro.org wrote: > > > > 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. > > > > So you are aware that all undocumented clock IDs are still allowed to > > use in DTS and they are ABI? Changing them will be an ABI break. > > Yes of course. > > Neil > > > > > Best regards, > > Krzysztof > > > Sorry, guys, I'm little bit confused. In the discussion pointed by Neil not-by-one-increment ID with public and private parts are acked by Krzysztof due to explicit explanation in the gxbb header. Have I to comment out my situation and stay it as is? BTW, I think changing IDs value would not affect logic, because it's not connected to driver logic 'by values', but 'by constants names'. We can expose/hide anything from device tree bindings, it will not change the clk driver logic.
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@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. > 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@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). > Hmmm, I like this way. But first, I think we need to make meson clock driver overall situation clear. [...]
On 14/03/2023 16:56, Dmitry Rokosov wrote: > On Tue, Mar 14, 2023 at 04:40:19PM +0100, neil.armstrong@linaro.org wrote: >> On 14/03/2023 16:37, Krzysztof Kozlowski wrote: >>> On 14/03/2023 16:33, neil.armstrong@linaro.org wrote: >>>>> 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. >>> >>> So you are aware that all undocumented clock IDs are still allowed to >>> use in DTS and they are ABI? Changing them will be an ABI break. >> >> Yes of course. >> >> Neil >> >>> >>> Best regards, >>> Krzysztof >>> >> > > Sorry, guys, I'm little bit confused. > In the discussion pointed by Neil not-by-one-increment ID with public and > private parts are acked by Krzysztof due to explicit explanation in the > gxbb header. Have I to comment out my situation and stay it as is? I did not NAK your solution here. I just pointed my usual remarks that it has certain outcome and minuses (undocumented ABI). But it is OK. > > BTW, I think changing IDs value would not affect logic, because > it's not connected to driver logic 'by values', but 'by constants You cannot change the IDs, neither their values nor the names (with exceptions). IDs - so the numbers - are ABI. "Constant names" - I assume you mean the names of defines - do not exist after preprocessing, so also not really relevant here... > names'. We can expose/hide anything from device tree bindings, it will > not change the clk driver logic. Best regards, Krzysztof
On Tue, Mar 14, 2023 at 05:37:04PM +0100, Krzysztof Kozlowski wrote: > On 14/03/2023 16:56, Dmitry Rokosov wrote: > > On Tue, Mar 14, 2023 at 04:40:19PM +0100, neil.armstrong@linaro.org wrote: > >> On 14/03/2023 16:37, Krzysztof Kozlowski wrote: > >>> On 14/03/2023 16:33, neil.armstrong@linaro.org wrote: > >>>>> 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. > >>> > >>> So you are aware that all undocumented clock IDs are still allowed to > >>> use in DTS and they are ABI? Changing them will be an ABI break. > >> > >> Yes of course. > >> > >> Neil > >> > >>> > >>> Best regards, > >>> Krzysztof > >>> > >> > > > > Sorry, guys, I'm little bit confused. > > In the discussion pointed by Neil not-by-one-increment ID with public and > > private parts are acked by Krzysztof due to explicit explanation in the > > gxbb header. Have I to comment out my situation and stay it as is? > > I did not NAK your solution here. I just pointed my usual remarks that > it has certain outcome and minuses (undocumented ABI). But it is OK. > Got it, thank you. > > > > BTW, I think changing IDs value would not affect logic, because > > it's not connected to driver logic 'by values', but 'by constants > > You cannot change the IDs, neither their values nor the names (with > exceptions). IDs - so the numbers - are ABI. > > "Constant names" - I assume you mean the names of defines - do not exist > after preprocessing, so also not really relevant here... > Ah, you mean the situation when dtb blob is old and module or kernel image is new, so ABI is broken. Yep, agree with you. [...]
On Tue, Mar 14, 2023 at 02:48:25PM +0300, 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? > I've changed all new source files to GPL-2.0+ except yaml, because yaml > dt bindings schemas require the following license: Why 2.0+? The kernel's default license is 2.0-only. Are you (and your lawyer) okay with GPL v4? But this is still part of the DT binding and has the same license preference: > # SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause However, the header licenses are complicated due to .dts licenses which are all over the place. The requirement is dual licensed and matching what includes it. Rob
On Fri, Mar 17, 2023 at 01:53:17PM -0500, Rob Herring wrote: > On Tue, Mar 14, 2023 at 02:48:25PM +0300, 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? > > I've changed all new source files to GPL-2.0+ except yaml, because yaml > > dt bindings schemas require the following license: > > Why 2.0+? The kernel's default license is 2.0-only. Are you (and > your lawyer) okay with GPL v4? > > But this is still part of the DT binding and has the same license > preference: > > > # SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > However, the header licenses are complicated due to .dts licenses which > are all over the place. The requirement is dual licensed and matching > what includes it. Agree with you. As we discussed with Krzysztof, checkpatch must verify such wrong license tags. I've introduced the patchset for that, please take a look: https://lore.kernel.org/all/20230317201621.15518-1-ddrokosov@sberdevices.ru/
diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml new file mode 100644 index 000000000000..cb6d8f4eb959 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml @@ -0,0 +1,73 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/clock/amlogic,a1-clkc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Amlogic Meson A/C serials Peripheral Clock Control Unit + +maintainers: + - Neil Armstrong <neil.armstrong@linaro.org> + - Jerome Brunet <jbrunet@baylibre.com> + - Jian Hu <jian.hu@jian.hu.com> + - Dmitry Rokosov <ddrokosov@sberdevices.ru> + +properties: + compatible: + const: amlogic,a1-clkc + + '#clock-cells': + const: 1 + + reg: + maxItems: 1 + + clocks: + items: + - description: input fixed pll div2 + - description: input fixed pll div3 + - description: input fixed pll div5 + - description: input fixed pll div7 + - description: input hifi pll + - description: input oscillator (usually at 24MHz) + + clock-names: + items: + - const: fclk_div2 + - const: fclk_div3 + - const: fclk_div5 + - const: fclk_div7 + - const: hifi_pll + - const: xtal + +required: + - compatible + - '#clock-cells' + - reg + - clocks + - clock-names + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/amlogic,a1-pll-clkc.h> + apb { + #address-cells = <2>; + #size-cells = <2>; + + clock-controller@800 { + compatible = "amlogic,a1-clkc"; + reg = <0 0x800 0 0x104>; + #clock-cells = <1>; + clocks = <&clkc_pll CLKID_FCLK_DIV2>, + <&clkc_pll CLKID_FCLK_DIV3>, + <&clkc_pll CLKID_FCLK_DIV5>, + <&clkc_pll CLKID_FCLK_DIV7>, + <&clkc_pll CLKID_HIFI_PLL>, + <&xtal>; + clock-names = "fclk_div2", "fclk_div3", + "fclk_div5", "fclk_div7", + "hifi_pll", "xtal"; + }; + }; diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml new file mode 100644 index 000000000000..77a13b1f9d5a --- /dev/null +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml @@ -0,0 +1,59 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/clock/amlogic,a1-pll-clkc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Amlogic Meson A/C serials PLL Clock Control Unit + +maintainers: + - Neil Armstrong <neil.armstrong@linaro.org> + - Jerome Brunet <jbrunet@baylibre.com> + - Jian Hu <jian.hu@jian.hu.com> + - Dmitry Rokosov <ddrokosov@sberdevices.ru> + +properties: + compatible: + const: amlogic,a1-pll-clkc + + '#clock-cells': + const: 1 + + reg: + maxItems: 1 + + clocks: + items: + - description: input fixpll_in + - description: input hifipll_in + + clock-names: + items: + - const: fixpll_in + - const: hifipll_in + +required: + - compatible + - '#clock-cells' + - reg + - clocks + - clock-names + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/amlogic,a1-clkc.h> + apb { + #address-cells = <2>; + #size-cells = <2>; + + clock-controller@7c80 { + compatible = "amlogic,a1-pll-clkc"; + reg = <0 0x7c80 0 0x18c>; + #clock-cells = <1>; + clocks = <&clkc_periphs CLKID_FIXPLL_IN>, + <&clkc_periphs CLKID_HIFIPLL_IN>; + clock-names = "fixpll_in", "hifipll_in"; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 39ff1a717625..8438bc9bd636 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1895,6 +1895,7 @@ L: linux-amlogic@lists.infradead.org S: Maintained F: Documentation/devicetree/bindings/clock/amlogic* F: drivers/clk/meson/ +F: include/dt-bindings/clock/a1* F: include/dt-bindings/clock/gxbb* F: include/dt-bindings/clock/meson* diff --git a/include/dt-bindings/clock/amlogic,a1-clkc.h b/include/dt-bindings/clock/amlogic,a1-clkc.h new file mode 100644 index 000000000000..2b16b1f1a5bf --- /dev/null +++ b/include/dt-bindings/clock/amlogic,a1-clkc.h @@ -0,0 +1,100 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * 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_CLKC_H +#define __A1_CLKC_H + +#define CLKID_FIXPLL_IN 1 +#define CLKID_USB_PHY_IN 2 +#define CLKID_USB_CTRL_IN 3 +#define CLKID_HIFIPLL_IN 4 +#define CLKID_SYSPLL_IN 5 +#define CLKID_DDS_IN 6 +#define CLKID_SYS 7 +#define CLKID_CLKTREE 8 +#define CLKID_RESET_CTRL 9 +#define CLKID_ANALOG_CTRL 10 +#define CLKID_PWR_CTRL 11 +#define CLKID_PAD_CTRL 12 +#define CLKID_SYS_CTRL 13 +#define CLKID_TEMP_SENSOR 14 +#define CLKID_AM2AXI_DIV 15 +#define CLKID_SPICC_B 16 +#define CLKID_SPICC_A 17 +#define CLKID_MSR 18 +#define CLKID_AUDIO 19 +#define CLKID_JTAG_CTRL 20 +#define CLKID_SARADC_EN 21 +#define CLKID_PWM_EF 22 +#define CLKID_PWM_CD 23 +#define CLKID_PWM_AB 24 +#define CLKID_CEC 25 +#define CLKID_I2C_S 26 +#define CLKID_IR_CTRL 27 +#define CLKID_I2C_M_D 28 +#define CLKID_I2C_M_C 29 +#define CLKID_I2C_M_B 30 +#define CLKID_I2C_M_A 31 +#define CLKID_ACODEC 32 +#define CLKID_OTP 33 +#define CLKID_SD_EMMC_A 34 +#define CLKID_USB_PHY 35 +#define CLKID_USB_CTRL 36 +#define CLKID_SYS_DSPB 37 +#define CLKID_SYS_DSPA 38 +#define CLKID_DMA 39 +#define CLKID_IRQ_CTRL 40 +#define CLKID_NIC 41 +#define CLKID_GIC 42 +#define CLKID_UART_C 43 +#define CLKID_UART_B 44 +#define CLKID_UART_A 45 +#define CLKID_SYS_PSRAM 46 +#define CLKID_RSA 47 +#define CLKID_CORESIGHT 48 +#define CLKID_AM2AXI_VAD 49 +#define CLKID_AUDIO_VAD 50 +#define CLKID_AXI_DMC 51 +#define CLKID_AXI_PSRAM 52 +#define CLKID_RAMB 53 +#define CLKID_RAMA 54 +#define CLKID_AXI_SPIFC 55 +#define CLKID_AXI_NIC 56 +#define CLKID_AXI_DMA 57 +#define CLKID_CPU_CTRL 58 +#define CLKID_ROM 59 +#define CLKID_PROC_I2C 60 +#define CLKID_DSPA_EN 63 +#define CLKID_DSPA_EN_NIC 64 +#define CLKID_DSPB_EN 65 +#define CLKID_DSPB_EN_NIC 66 +#define CLKID_RTC 67 +#define CLKID_CECA_32K 68 +#define CLKID_CECB_32K 69 +#define CLKID_24M 70 +#define CLKID_12M 71 +#define CLKID_FCLK_DIV2_DIVN 72 +#define CLKID_GEN 73 +#define CLKID_SARADC 75 +#define CLKID_PWM_A 76 +#define CLKID_PWM_B 77 +#define CLKID_PWM_C 78 +#define CLKID_PWM_D 79 +#define CLKID_PWM_E 80 +#define CLKID_PWM_F 81 +#define CLKID_SPICC 82 +#define CLKID_TS 83 +#define CLKID_SPIFC 84 +#define CLKID_USB_BUS 85 +#define CLKID_SD_EMMC 86 +#define CLKID_PSRAM 87 +#define CLKID_DMC 88 +#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+ */ +/* + * 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 + +#endif /* __A1_PLL_CLKC_H */