LinuxLists.cc - [PATCH v3 0/4] riscv: sophgo: add clock support for Sophgo CV1800 SoCs

2023-12-08 01:14:00

by Inochi Amaoto

[permalink] [raw]

Subject: Re: [PATCH v3 3/4] riscv: dts: sophgo: add clock generator for Sophgo CV1800 series SoC

>On Thu, Dec 07, 2023 at 01:52:16PM +0100, Krzysztof Kozlowski wrote:
>> On 07/12/2023 10:42, Inochi Amaoto wrote:
>>>>> +&clk {
>>>>> + compatible = "sophgo,cv1810-clk";
>>>>> +};
>>>>> diff --git a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
>>>>> index 2d6f4a4b1e58..6ea1b2784db9 100644
>>>>> --- a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
>>>>> +++ b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
>>>>> @@ -53,6 +53,12 @@ soc {
>>>>> dma-noncoherent;
>>>>> ranges;
>>>>>
>>>>> + clk: clock-controller@3002000 {
>>>>> + reg = <0x03002000 0x1000>;
>>>>> + clocks = <&osc>;
>>>>> + #clock-cells = <1>;
>>>>
>>>> I don't find such layout readable and maintainable. I did some parts
>>>> like this long, long time ago for one of my SoCs (Exynos54xx), but I
>>>> find it over time unmaintainable approach. I strongly suggest to have
>>>> compatible and other properties in one place, so cv1800 and cv1812, even
>>>> if it duplicates the code.
>>>>
>>>
>>> Hi Krzysztof:
>>>
>>> Thanks for your advice, but I have a question about this: when I should
>>> use the DT override? The memory mapping of the CV1800 and CV1810 are
>>> almost the same (the CV1810 have more peripheral and the future SG200X
>>> have the same layout). IIRC, this is why conor suggested using DT override
>>> to make modification easier. But duplicating node seems to break thiS, so
>>> I's pretty confused.
>>
>> Go with whatever your subarchitecture and architecture maintainers
>> prefer, I just shared my opinion that I find such code difficult to read
>> and maintain.
>>
>> Extending node with supplies, pinctrl or even clocks would be readable.
>> But the compatible: no. The same applies when you need to delete
>> property or subnode: not readable/maintainable IMHO.
>
>There are apparently 3 or 4 of these SoCs that are basically identical,
>which is why the approach was taken. I do agree that it looks somewhat
>messy because I was looking for device-specific compatibles for these
>SoCs.
>

I agree that this may be messy. But it might still be acceptable if we
limit the number of devices in this format.

IIRC, only clint, plic, clk, maybe pinmux only needs different compatible.
For more complex device, such as tpu and codec, I agree with duplicating
nodes and make them SoC specific.

As for this patch, I have already adjusted the order of clock to ensure
the compatible among different SoCs. This will make the clock assignment
easier.

2023-12-08 15:02:36

>On Thu, Dec 7, 2023 at 2:37 AM Inochi Amaoto <[email protected]> wrote:
>>
>> Add driver for CV1800 series clock controller.
>>
>> Signed-off-by: Inochi Amaoto <[email protected]>
>> Link: https://github.com/milkv-duo/duo-files/blob/main/hardware/CV1800B/CV180X-Clock-v1.xlsx
>> Link: https://github.com/milkv-duo/duo-files/blob/main/hardware/CV1800B/CV1800B-CV1801B-Preliminary-Datasheet-full-en.pdf
>> ---
>> drivers/clk/Kconfig | 1 +
>> drivers/clk/Makefile | 1 +
>> drivers/clk/sophgo/Kconfig | 12 +
>> drivers/clk/sophgo/Makefile | 7 +
>> drivers/clk/sophgo/clk-cv1800.c | 1574 ++++++++++++++++++++++++
>> drivers/clk/sophgo/clk-cv1800.h | 123 ++
>> drivers/clk/sophgo/clk-cv18xx-common.c | 76 ++
>> drivers/clk/sophgo/clk-cv18xx-common.h | 85 ++
>> drivers/clk/sophgo/clk-cv18xx-ip.c | 894 ++++++++++++++
>> drivers/clk/sophgo/clk-cv18xx-ip.h | 266 ++++
>> drivers/clk/sophgo/clk-cv18xx-pll.c | 465 +++++++
>> drivers/clk/sophgo/clk-cv18xx-pll.h | 79 ++
>> 12 files changed, 3583 insertions(+)
>> create mode 100644 drivers/clk/sophgo/Kconfig
>> create mode 100644 drivers/clk/sophgo/Makefile
>> create mode 100644 drivers/clk/sophgo/clk-cv1800.c
>> create mode 100644 drivers/clk/sophgo/clk-cv1800.h
>> create mode 100644 drivers/clk/sophgo/clk-cv18xx-common.c
>> create mode 100644 drivers/clk/sophgo/clk-cv18xx-common.h
>> create mode 100644 drivers/clk/sophgo/clk-cv18xx-ip.c
>> create mode 100644 drivers/clk/sophgo/clk-cv18xx-ip.h
>> create mode 100644 drivers/clk/sophgo/clk-cv18xx-pll.c
>> create mode 100644 drivers/clk/sophgo/clk-cv18xx-pll.h
>>
>> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
>> index c30d0d396f7a..514343934fda 100644
>> --- a/drivers/clk/Kconfig
>> +++ b/drivers/clk/Kconfig
>> @@ -499,6 +499,7 @@ source "drivers/clk/rockchip/Kconfig"
>> source "drivers/clk/samsung/Kconfig"
>> source "drivers/clk/sifive/Kconfig"
>> source "drivers/clk/socfpga/Kconfig"
>> +source "drivers/clk/sophgo/Kconfig"
>> source "drivers/clk/sprd/Kconfig"
>> source "drivers/clk/starfive/Kconfig"
>> source "drivers/clk/sunxi/Kconfig"
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index ed71f2e0ee36..eeae7ae93f89 100644
>> --- a/drivers/clk/Makefile
>> +++ b/drivers/clk/Makefile
>> @@ -119,6 +119,7 @@ obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip/
>> obj-$(CONFIG_COMMON_CLK_SAMSUNG) += samsung/
>> obj-$(CONFIG_CLK_SIFIVE) += sifive/
>> obj-y += socfpga/
>> +obj-y += sophgo/
>> obj-$(CONFIG_PLAT_SPEAR) += spear/
>> obj-y += sprd/
>> obj-$(CONFIG_ARCH_STI) += st/
>> diff --git a/drivers/clk/sophgo/Kconfig b/drivers/clk/sophgo/Kconfig
>> new file mode 100644
>> index 000000000000..d67009fa749f
>> --- /dev/null
>> +++ b/drivers/clk/sophgo/Kconfig
>> @@ -0,0 +1,12 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# common clock support for SOPHGO SoC family.
>> +
>> +config CLK_SOPHGO_CV1800
>> + tristate "Support for the Sophgo CV1800 series SoCs clock controller"
>> + default m
>> + depends on ARCH_SOPHGO || COMPILE_TEST
>> + help
>> + This driver supports clock controller of Sophgo CV18XX series SoC.
>> + The driver require a 25MHz Oscillator to function generate clock.
>> + It includes PLLs, common clock function and some vendor clock for
>> + IPs of CV18XX series SoC
>> diff --git a/drivers/clk/sophgo/Makefile b/drivers/clk/sophgo/Makefile
>> new file mode 100644
>> index 000000000000..a50320764200
>> --- /dev/null
>> +++ b/drivers/clk/sophgo/Makefile
>> @@ -0,0 +1,7 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +obj-$(CONFIG_CLK_SOPHGO_CV1800) += clk-sophgo-cv1800.o
>> +
>> +clk-sophgo-cv1800-y += clk-cv1800.o
>> +clk-sophgo-cv1800-y += clk-cv18xx-common.o
>> +clk-sophgo-cv1800-y += clk-cv18xx-ip.o
>> +clk-sophgo-cv1800-y += clk-cv18xx-pll.o
>> diff --git a/drivers/clk/sophgo/clk-cv1800.c b/drivers/clk/sophgo/clk-cv1800.c
>> new file mode 100644
>> index 000000000000..8176ee672190
>> --- /dev/null
>> +++ b/drivers/clk/sophgo/clk-cv1800.c
>> @@ -0,0 +1,1574 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2023 Inochi Amaoto <[email protected]>
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/clk.h>
>> +#include <linux/clkdev.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>
>You probably don't need these 2 headers and the implicit includes it
>makes are dropped now in linux-next. Please check what you actually
>need and make them explicit.
>
>Rob
>

Thanks. I have fixed in the new version.