1. Add clock controller driver for S4 SOC.
Yu Tu (3):
dt-bindings: clk: meson: add S4 SoC clock controller bindings
arm64: dts: meson: add S4 Soc clock controller in DT
clk: meson: s4: add s4 SoC clock controller driver
V1 -> V2: Change format as discussed in the email.
Link:https://lore.kernel.org/linux-amlogic/[email protected]/
.../bindings/clock/amlogic,gxbb-clkc.txt | 1 +
MAINTAINERS | 1 +
arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 11 +
drivers/clk/meson/Kconfig | 15 +
drivers/clk/meson/Makefile | 1 +
drivers/clk/meson/s4.c | 4732 +++++++++++++++++
drivers/clk/meson/s4.h | 296 ++
include/dt-bindings/clock/s4-clkc.h | 146 +
8 files changed, 5203 insertions(+)
create mode 100644 drivers/clk/meson/s4.c
create mode 100644 drivers/clk/meson/s4.h
create mode 100644 include/dt-bindings/clock/s4-clkc.h
base-commit: b293bc9286ee21824e93f0fcfed3b78fdfee01e6
--
2.33.1
Added information about the S4 SOC Clock controller in DT.
Signed-off-by: Yu Tu <[email protected]>
---
arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
index ff213618a598..3add7fcaffa9 100644
--- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
@@ -6,6 +6,7 @@
#include <dt-bindings/interrupt-controller/irq.h>
#include <dt-bindings/interrupt-controller/arm-gic.h>
#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/clock/s4-clkc.h>
/ {
cpus {
@@ -118,6 +119,16 @@ gpio_intc: interrupt-controller@4080 {
<10 11 12 13 14 15 16 17 18 19 20 21>;
};
+ clkc: clock-controller {
+ compatible = "amlogic,s4-clkc";
+ reg = <0x0 0x0 0x0 0x49c>,
+ <0x0 0x8000 0x0 0x348>;
+ reg-names = "basic", "pll";
+ #clock-cells = <1>;
+ clocks = <&xtal>;
+ clock-names = "xtal";
+ };
+
uart_B: serial@7a000 {
compatible = "amlogic,meson-s4-uart",
"amlogic,meson-ao-uart";
--
2.33.1
On Thu 28 Jul 2022 at 13:41, Yu Tu <[email protected]> wrote:
> 1. Add clock controller driver for S4 SOC.
>
> Yu Tu (3):
> dt-bindings: clk: meson: add S4 SoC clock controller bindings
> arm64: dts: meson: add S4 Soc clock controller in DT
> clk: meson: s4: add s4 SoC clock controller driver
>
> V1 -> V2: Change format as discussed in the email.
>
> Link:https://lore.kernel.org/linux-amlogic/[email protected]/
>
> .../bindings/clock/amlogic,gxbb-clkc.txt | 1 +
> MAINTAINERS | 1 +
> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 11 +
> drivers/clk/meson/Kconfig | 15 +
> drivers/clk/meson/Makefile | 1 +
> drivers/clk/meson/s4.c | 4732 +++++++++++++++++
> drivers/clk/meson/s4.h | 296 ++
> include/dt-bindings/clock/s4-clkc.h | 146 +
> 8 files changed, 5203 insertions(+)
> create mode 100644 drivers/clk/meson/s4.c
> create mode 100644 drivers/clk/meson/s4.h
> create mode 100644 include/dt-bindings/clock/s4-clkc.h
>
>
> base-commit: b293bc9286ee21824e93f0fcfed3b78fdfee01e6
Please don't post until you have addressed *ALL* the comments from the
previous version.
At first glance, I can see that this is still a single driver for
what is obviously 2 controllers with 2 register spaces. Simple comments
like the "<< 2" in the register declaration have not been addressed either.
Seeing that, I have not reviewed this version further.
I won't until all the comments from v1 are either addressed or answered.
Regards
Jerome
Hi JB，
On 2022/7/28 15:08, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
>
> On Thu 28 Jul 2022 at 13:41, Yu Tu <[email protected]> wrote:
>
>> 1. Add clock controller driver for S4 SOC.
>>
>> Yu Tu (3):
>> dt-bindings: clk: meson: add S4 SoC clock controller bindings
>> arm64: dts: meson: add S4 Soc clock controller in DT
>> clk: meson: s4: add s4 SoC clock controller driver
>>
>> V1 -> V2: Change format as discussed in the email.
>>
>> Link:https://lore.kernel.org/linux-amlogic/[email protected]/
>>
>> .../bindings/clock/amlogic,gxbb-clkc.txt | 1 +
>> MAINTAINERS | 1 +
>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 11 +
>> drivers/clk/meson/Kconfig | 15 +
>> drivers/clk/meson/Makefile | 1 +
>> drivers/clk/meson/s4.c | 4732 +++++++++++++++++
>> drivers/clk/meson/s4.h | 296 ++
>> include/dt-bindings/clock/s4-clkc.h | 146 +
>> 8 files changed, 5203 insertions(+)
>> create mode 100644 drivers/clk/meson/s4.c
>> create mode 100644 drivers/clk/meson/s4.h
>> create mode 100644 include/dt-bindings/clock/s4-clkc.h
>>
>>
>> base-commit: b293bc9286ee21824e93f0fcfed3b78fdfee01e6
>
> Please don't post until you have addressed *ALL* the comments from the
> previous version.
The last email asked you to adopt A1 method, but you did not reply?
>
> At first glance, I can see that this is still a single driver for
> what is obviously 2 controllers with 2 register spaces. Simple comments
> like the "<< 2" in the register declaration have not been addressed either.
I understand that this should be a controller, just two address
descriptions. One is the various PLL registers and one is the clock for
the peripherals. And PLL is to provide a clock source for various
peripheral clocks. So a clock controller is reasonable. I think you got
it wrong.
Ok, if you insist on using two clock controllers,, please provide your
the reason and example code?
>
> Seeing that, I have not reviewed this version further.
> I won't until all the comments from v1 are either addressed or answer
>
> Regards
> Jerome
>
> .
Hi JB,
We suspect that you are not looking carefully. You only see two base
addresses for controllers, so you think you should use two clock
controllers. But you're missing their direct connection.
On 2022/7/28 16:06, Yu Tu wrote:
> Hi JB，
>
> On 2022/7/28 15:08, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>>
>> On Thu 28 Jul 2022 at 13:41, Yu Tu <[email protected]> wrote:
>>
>>> 1. Add clock controller driver for S4 SOC.
>>>
>>> Yu Tu (3):
>>> dt-bindings: clk: meson: add S4 SoC clock controller bindings
>>> arm64: dts: meson: add S4 Soc clock controller in DT
>>> clk: meson: s4: add s4 SoC clock controller driver
>>>
>>> V1 -> V2: Change format as discussed in the email.
>>>
>>> Link:https://lore.kernel.org/linux-amlogic/[email protected]/
>>>
>>>
>>> .../bindings/clock/amlogic,gxbb-clkc.txt | 1 +
>>> MAINTAINERS | 1 +
>>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 11 +
>>> drivers/clk/meson/Kconfig | 15 +
>>> drivers/clk/meson/Makefile | 1 +
>>> drivers/clk/meson/s4.c | 4732 +++++++++++++++++
>>> drivers/clk/meson/s4.h | 296 ++
>>> include/dt-bindings/clock/s4-clkc.h | 146 +
>>> 8 files changed, 5203 insertions(+)
>>> create mode 100644 drivers/clk/meson/s4.c
>>> create mode 100644 drivers/clk/meson/s4.h
>>> create mode 100644 include/dt-bindings/clock/s4-clkc.h
>>>
>>>
>>> base-commit: b293bc9286ee21824e93f0fcfed3b78fdfee01e6
>>
>> Please don't post until you have addressed *ALL* the comments from the
>> previous version.
> The last email asked you to adopt A1 method, but you did not reply?
>
>>
>> At first glance, I can see that this is still a single driver for
>> what is obviously 2 controllers with 2 register spaces. Simple comments
>> like the "<< 2" in the register declaration have not been addressed
>> either.
> I understand that this should be a controller, just two address
> descriptions. One is the various PLL registers and one is the clock for
> the peripherals. And PLL is to provide a clock source for various
> peripheral clocks. So a clock controller is reasonable. I think you got
> it wrong.
>
> Ok, if you insist on using two clock controllers,, please provide your
> the reason and example code?
>
>>
>> Seeing that, I have not reviewed this version further.
>> I won't until all the comments from v1 are either addressed or answer
>>
>> Regards
>> Jerome
>>
>> .
On Thu 28 Jul 2022 at 16:06, Yu Tu <[email protected]> wrote:
> Hi JB，
>
> On 2022/7/28 15:08, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On Thu 28 Jul 2022 at 13:41, Yu Tu <[email protected]> wrote:
>>
>>> 1. Add clock controller driver for S4 SOC.
>>>
>>> Yu Tu (3):
>>> dt-bindings: clk: meson: add S4 SoC clock controller bindings
>>> arm64: dts: meson: add S4 Soc clock controller in DT
>>> clk: meson: s4: add s4 SoC clock controller driver
>>>
>>> V1 -> V2: Change format as discussed in the email.
>>>
>>> Link:https://lore.kernel.org/linux-amlogic/[email protected]/
>>>
>>> .../bindings/clock/amlogic,gxbb-clkc.txt | 1 +
>>> MAINTAINERS | 1 +
>>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 11 +
>>> drivers/clk/meson/Kconfig | 15 +
>>> drivers/clk/meson/Makefile | 1 +
>>> drivers/clk/meson/s4.c | 4732 +++++++++++++++++
>>> drivers/clk/meson/s4.h | 296 ++
>>> include/dt-bindings/clock/s4-clkc.h | 146 +
>>> 8 files changed, 5203 insertions(+)
>>> create mode 100644 drivers/clk/meson/s4.c
>>> create mode 100644 drivers/clk/meson/s4.h
>>> create mode 100644 include/dt-bindings/clock/s4-clkc.h
>>>
>>>
>>> base-commit: b293bc9286ee21824e93f0fcfed3b78fdfee01e6
>> Please don't post until you have addressed *ALL* the comments from the
>> previous version.
> The last email asked you to adopt A1 method, but you did not reply?
>
>> At first glance, I can see that this is still a single driver for
>> what is obviously 2 controllers with 2 register spaces. Simple comments
>> like the "<< 2" in the register declaration have not been addressed either.
> I understand that this should be a controller, just two address
> descriptions. One is the various PLL registers and one is the clock for
> the peripherals. And PLL is to provide a clock source for various
> peripheral clocks. So a clock controller is reasonable. I think you got
> it wrong.
I don't think I do. This looks exactly like the A1.
The post of that controller are still in the archive and I am sure your
colleagues can give you the history.
You clearly have register regions providing clock, separated by
0x8000. Claiming that as one big region is bad design.
There has been several remarks about using a big syscon on V1,
unaddressed too.
CCF has everything necessary in place to handle each register region
separately, properly and pass clock around.
You can handle it as a single controller, claiming the two regions
individually but:
# 1 - handling 2 different regmaps in the controller is going to be
bigger mess than you think
# 2 - I am far from convinced there is any reason to do so
>
> Ok, if you insist on using two clock controllers,, please provide your the
> reason and example code?
>
>> Seeing that, I have not reviewed this version further.
>> I won't until all the comments from v1 are either addressed or answer
>> Regards
>> Jerome
>> .
Hi Jerome,
Thanks for your reply and explanation.
On 2022/7/28 16:27, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
>
> On Thu 28 Jul 2022 at 16:06, Yu Tu <[email protected]> wrote:
>
>> Hi JB，
>>
>> On 2022/7/28 15:08, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> On Thu 28 Jul 2022 at 13:41, Yu Tu <[email protected]> wrote:
>>>
>>>> 1. Add clock controller driver for S4 SOC.
>>>>
>>>> Yu Tu (3):
>>>> dt-bindings: clk: meson: add S4 SoC clock controller bindings
>>>> arm64: dts: meson: add S4 Soc clock controller in DT
>>>> clk: meson: s4: add s4 SoC clock controller driver
>>>>
>>>> V1 -> V2: Change format as discussed in the email.
>>>>
>>>> Link:https://lore.kernel.org/linux-amlogic/[email protected]/
>>>>
>>>> .../bindings/clock/amlogic,gxbb-clkc.txt | 1 +
>>>> MAINTAINERS | 1 +
>>>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 11 +
>>>> drivers/clk/meson/Kconfig | 15 +
>>>> drivers/clk/meson/Makefile | 1 +
>>>> drivers/clk/meson/s4.c | 4732 +++++++++++++++++
>>>> drivers/clk/meson/s4.h | 296 ++
>>>> include/dt-bindings/clock/s4-clkc.h | 146 +
>>>> 8 files changed, 5203 insertions(+)
>>>> create mode 100644 drivers/clk/meson/s4.c
>>>> create mode 100644 drivers/clk/meson/s4.h
>>>> create mode 100644 include/dt-bindings/clock/s4-clkc.h
>>>>
>>>>
>>>> base-commit: b293bc9286ee21824e93f0fcfed3b78fdfee01e6
>>> Please don't post until you have addressed *ALL* the comments from the
>>> previous version.
>> The last email asked you to adopt A1 method, but you did not reply?
>>
>>> At first glance, I can see that this is still a single driver for
>>> what is obviously 2 controllers with 2 register spaces. Simple comments
>>> like the "<< 2" in the register declaration have not been addressed either.
>> I understand that this should be a controller, just two address
>> descriptions. One is the various PLL registers and one is the clock for
>> the peripherals. And PLL is to provide a clock source for various
>> peripheral clocks. So a clock controller is reasonable. I think you got
>> it wrong.
>
> I don't think I do. This looks exactly like the A1.
> The post of that controller are still in the archive and I am sure your
> colleagues can give you the history.
>
> You clearly have register regions providing clock, separated by
> 0x8000. Claiming that as one big region is bad design.
>
> There has been several remarks about using a big syscon on V1,
> unaddressed too.
>
> CCF has everything necessary in place to handle each register region
> separately, properly and pass clock around.
>
> You can handle it as a single controller, claiming the two regions
> individually but:
> # 1 - handling 2 different regmaps in the controller is going to be
> bigger mess than you think
> # 2 - I am far from convinced there is any reason to do so
>
It makes sense, as you say, to separate the two controllers. But I think
the only thing that was forced apart was that the digital designers
didn't put these registers together when they were designing the chips.
I'm going to separate the two controllers like you said.
>
>>
>> Ok, if you insist on using two clock controllers,, please provide your the
>> reason and example code?
>>
>>> Seeing that, I have not reviewed this version further.
>>> I won't until all the comments from v1 are either addressed or answer
>>> Regards
>>> Jerome
>>> .
>
> .
On Thu 28 Jul 2022 at 16:55, Yu Tu <[email protected]> wrote:
> Hi Jerome,
> Thanks for your reply and explanation.
>
> On 2022/7/28 16:27, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On Thu 28 Jul 2022 at 16:06, Yu Tu <[email protected]> wrote:
>>
>>> Hi JB，
>>>
>>> On 2022/7/28 15:08, Jerome Brunet wrote:
>>>> [ EXTERNAL EMAIL ]
>>>>
>>>> On Thu 28 Jul 2022 at 13:41, Yu Tu <[email protected]> wrote:
>>>>
>>>>> 1. Add clock controller driver for S4 SOC.
>>>>>
>>>>> Yu Tu (3):
>>>>> dt-bindings: clk: meson: add S4 SoC clock controller bindings
>>>>> arm64: dts: meson: add S4 Soc clock controller in DT
>>>>> clk: meson: s4: add s4 SoC clock controller driver
>>>>>
>>>>> V1 -> V2: Change format as discussed in the email.
>>>>>
>>>>> Link:https://lore.kernel.org/linux-amlogic/[email protected]/
>>>>>
>>>>> .../bindings/clock/amlogic,gxbb-clkc.txt | 1 +
>>>>> MAINTAINERS | 1 +
>>>>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 11 +
>>>>> drivers/clk/meson/Kconfig | 15 +
>>>>> drivers/clk/meson/Makefile | 1 +
>>>>> drivers/clk/meson/s4.c | 4732 +++++++++++++++++
>>>>> drivers/clk/meson/s4.h | 296 ++
>>>>> include/dt-bindings/clock/s4-clkc.h | 146 +
>>>>> 8 files changed, 5203 insertions(+)
>>>>> create mode 100644 drivers/clk/meson/s4.c
>>>>> create mode 100644 drivers/clk/meson/s4.h
>>>>> create mode 100644 include/dt-bindings/clock/s4-clkc.h
>>>>>
>>>>>
>>>>> base-commit: b293bc9286ee21824e93f0fcfed3b78fdfee01e6
>>>> Please don't post until you have addressed *ALL* the comments from the
>>>> previous version.
>>> The last email asked you to adopt A1 method, but you did not reply?
>>>
>>>> At first glance, I can see that this is still a single driver for
>>>> what is obviously 2 controllers with 2 register spaces. Simple comments
>>>> like the "<< 2" in the register declaration have not been addressed either.
>>> I understand that this should be a controller, just two address
>>> descriptions. One is the various PLL registers and one is the clock for
>>> the peripherals. And PLL is to provide a clock source for various
>>> peripheral clocks. So a clock controller is reasonable. I think you got
>>> it wrong.
>> I don't think I do. This looks exactly like the A1.
>> The post of that controller are still in the archive and I am sure your
>> colleagues can give you the history.
>> You clearly have register regions providing clock, separated by
>> 0x8000. Claiming that as one big region is bad design.
>> There has been several remarks about using a big syscon on V1,
>> unaddressed too.
>> CCF has everything necessary in place to handle each register region
>> separately, properly and pass clock around.
>> You can handle it as a single controller, claiming the two regions
>> individually but:
>> # 1 - handling 2 different regmaps in the controller is going to be
>> bigger mess than you think
>> # 2 - I am far from convinced there is any reason to do so
>>
> It makes sense, as you say, to separate the two controllers. But I think
> the only thing that was forced apart was that the digital designers
> didn't put these registers together when they were designing the chips.
>
One controller is providing all the base PLLs
The other is providing most (if not all) the devices clocks.
This does not look like coincidence or mistake to me.
> I'm going to separate the two controllers like you said.
>
>>
>>>
>>> Ok, if you insist on using two clock controllers,, please provide your the
>>> reason and example code?
>>>
>>>> Seeing that, I have not reviewed this version further.
>>>> I won't until all the comments from v1 are either addressed or answer
>>>> Regards
>>>> Jerome
>>>> .
>> .
On 2022/7/28 17:03, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
>
> On Thu 28 Jul 2022 at 16:55, Yu Tu <[email protected]> wrote:
>
>> Hi Jerome,
>> Thanks for your reply and explanation.
>>
>> On 2022/7/28 16:27, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> On Thu 28 Jul 2022 at 16:06, Yu Tu <[email protected]> wrote:
>>>
>>>> Hi JB，
>>>>
>>>> On 2022/7/28 15:08, Jerome Brunet wrote:
>>>>> [ EXTERNAL EMAIL ]
>>>>>
>>>>> On Thu 28 Jul 2022 at 13:41, Yu Tu <[email protected]> wrote:
>>>>>
>>>>>> 1. Add clock controller driver for S4 SOC.
>>>>>>
>>>>>> Yu Tu (3):
>>>>>> dt-bindings: clk: meson: add S4 SoC clock controller bindings
>>>>>> arm64: dts: meson: add S4 Soc clock controller in DT
>>>>>> clk: meson: s4: add s4 SoC clock controller driver
>>>>>>
>>>>>> V1 -> V2: Change format as discussed in the email.
>>>>>>
>>>>>> Link:https://lore.kernel.org/linux-amlogic/[email protected]/
>>>>>>
>>>>>> .../bindings/clock/amlogic,gxbb-clkc.txt | 1 +
>>>>>> MAINTAINERS | 1 +
>>>>>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 11 +
>>>>>> drivers/clk/meson/Kconfig | 15 +
>>>>>> drivers/clk/meson/Makefile | 1 +
>>>>>> drivers/clk/meson/s4.c | 4732 +++++++++++++++++
>>>>>> drivers/clk/meson/s4.h | 296 ++
>>>>>> include/dt-bindings/clock/s4-clkc.h | 146 +
>>>>>> 8 files changed, 5203 insertions(+)
>>>>>> create mode 100644 drivers/clk/meson/s4.c
>>>>>> create mode 100644 drivers/clk/meson/s4.h
>>>>>> create mode 100644 include/dt-bindings/clock/s4-clkc.h
>>>>>>
>>>>>>
>>>>>> base-commit: b293bc9286ee21824e93f0fcfed3b78fdfee01e6
>>>>> Please don't post until you have addressed *ALL* the comments from the
>>>>> previous version.
>>>> The last email asked you to adopt A1 method, but you did not reply?
>>>>
>>>>> At first glance, I can see that this is still a single driver for
>>>>> what is obviously 2 controllers with 2 register spaces. Simple comments
>>>>> like the "<< 2" in the register declaration have not been addressed either.
>>>> I understand that this should be a controller, just two address
>>>> descriptions. One is the various PLL registers and one is the clock for
>>>> the peripherals. And PLL is to provide a clock source for various
>>>> peripheral clocks. So a clock controller is reasonable. I think you got
>>>> it wrong.
>>> I don't think I do. This looks exactly like the A1.
>>> The post of that controller are still in the archive and I am sure your
>>> colleagues can give you the history.
>>> You clearly have register regions providing clock, separated by
>>> 0x8000. Claiming that as one big region is bad design.
>>> There has been several remarks about using a big syscon on V1,
>>> unaddressed too.
>>> CCF has everything necessary in place to handle each register region
>>> separately, properly and pass clock around.
>>> You can handle it as a single controller, claiming the two regions
>>> individually but:
>>> # 1 - handling 2 different regmaps in the controller is going to be
>>> bigger mess than you think
>>> # 2 - I am far from convinced there is any reason to do so
>>>
>> It makes sense, as you say, to separate the two controllers. But I think
>> the only thing that was forced apart was that the digital designers
>> didn't put these registers together when they were designing the chips.
>>
>
> One controller is providing all the base PLLs
> The other is providing most (if not all) the devices clocks.
> This does not look like coincidence or mistake to me.
Thanks for your reply. Looks like I got it wrong. However, I will talk
to the chip designer about whether it is possible to put these registers
in the later chip so that it may be easier for our software to process.
>
>> I'm going to separate the two controllers like you said.
>>
>>>
>>>>
>>>> Ok, if you insist on using two clock controllers,, please provide your the
>>>> reason and example code?
>>>>
>>>>> Seeing that, I have not reviewed this version further.
>>>>> I won't until all the comments from v1 are either addressed or answer
>>>>> Regards
>>>>> Jerome
>>>>> .
>>> .
>
> .