diff mbox series

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

Message ID 20230313201259.19998-4-ddrokosov@sberdevices.ru (mailing list archive)
State Superseded
Headers show
Series add Amlogic A1 clock controller drivers | expand

Commit Message

Dmitry Rokosov March 13, 2023, 8:12 p.m. UTC
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.

Signed-off-by: Jian Hu <jian.hu@amlogic.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
---
 .../bindings/clock/amlogic,a1-clkc.yaml       |  73 +++++++++++++
 .../bindings/clock/amlogic,a1-pll-clkc.yaml   |  59 +++++++++++
 MAINTAINERS                                   |   1 +
 include/dt-bindings/clock/amlogic,a1-clkc.h   | 100 ++++++++++++++++++
 .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |  20 ++++
 5 files changed, 253 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
 create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
 create mode 100644 include/dt-bindings/clock/amlogic,a1-clkc.h
 create mode 100644 include/dt-bindings/clock/amlogic,a1-pll-clkc.h

Comments

Krzysztof Kozlowski March 14, 2023, 11:28 a.m. UTC | #1
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
Dmitry Rokosov March 14, 2023, 11:48 a.m. UTC | #2
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.
Krzysztof Kozlowski March 14, 2023, 2:05 p.m. UTC | #3
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
Dmitry Rokosov March 14, 2023, 3:01 p.m. UTC | #4
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.
Krzysztof Kozlowski March 14, 2023, 3:19 p.m. UTC | #5
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
Neil Armstrong March 14, 2023, 3:33 p.m. UTC | #6
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
>
Krzysztof Kozlowski March 14, 2023, 3:37 p.m. UTC | #7
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
Neil Armstrong March 14, 2023, 3:40 p.m. UTC | #8
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
>
Dmitry Rokosov March 14, 2023, 3:56 p.m. UTC | #9
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.
Dmitry Rokosov March 14, 2023, 4:02 p.m. UTC | #10
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.

[...]
Krzysztof Kozlowski March 14, 2023, 4:37 p.m. UTC | #11
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
Dmitry Rokosov March 14, 2023, 7:28 p.m. UTC | #12
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.

[...]
Rob Herring (Arm) March 17, 2023, 6:53 p.m. UTC | #13
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
Dmitry Rokosov March 17, 2023, 8:27 p.m. UTC | #14
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 mbox series

Patch

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