| Message ID | 20240828062740.1614744-1-ryan_chen@aspeedtech.com |
|---|---|
| Headers | show |
| Series | Add support for AST2700 clk driver | expand |
On 28/08/2024 08:27, Ryan Chen wrote: > Add AST2700 clock controller driver. This driver also selects MFD_SYSCON, > which provides access to system controller registers, and register the > reset controller. > > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > + clks[SCU0_CLK_GATE_EMMCCLK] = > + ast2700_clk_hw_register_gate(NULL, "emmcclk-gate", "emmcclk", > + 0, clk_base + SCU0_CLK_STOP, > + 27, 0, &ast2700_clk_lock); > + > + clks[SCU0_CLK_GATE_RVAS1CLK] = > + ast2700_clk_hw_register_gate(NULL, "rvas2clk", NULL, > + 0, clk_base + SCU0_CLK_STOP, > + 28, 0, &ast2700_clk_lock); > + > + of_clk_add_hw_provider(soc0_node, of_clk_hw_onecell_get, clk_data); > + > + return 0; > +}; > + > +CLK_OF_DECLARE_DRIVER(ast2700_soc0, "aspeed,ast2700-scu0", ast2700_soc0_clk_init); > +CLK_OF_DECLARE_DRIVER(ast2700_soc1, "aspeed,ast2700-scu1", ast2700_soc1_clk_init); Nope, this is not documented. You cannot add new compatibles without bindings. Best regards, Krzysztof
On 28/08/2024 08:27, Ryan Chen wrote: > This patch series is add clk driver for AST2700. > > AST2700 is the 8th generation of Integrated Remote Management Processor > introduced by ASPEED Technology Inc. Which is Board Management controller > (BMC) SoC family. AST2700 have two SoC connected, one is SoC0, another > is SoC1, it has it's own scu, this driver inlcude SCU0 and SCU1 driver. > > v2: > -yaml: drop 64bits address example. > -yaml: add discription about soc0 and soc1 Where? Proof read your patches before sending. Best regards, Krzysztof
> Subject: Re: [PATCH v2 3/3] clk: aspeed: add AST2700 clk driver > > On 28/08/2024 08:27, Ryan Chen wrote: > > Add AST2700 clock controller driver. This driver also selects > > MFD_SYSCON, which provides access to system controller registers, and > > register the reset controller. > > > > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > > > > + clks[SCU0_CLK_GATE_EMMCCLK] = > > + ast2700_clk_hw_register_gate(NULL, "emmcclk-gate", "emmcclk", > > + 0, clk_base + SCU0_CLK_STOP, > > + 27, 0, &ast2700_clk_lock); > > + > > + clks[SCU0_CLK_GATE_RVAS1CLK] = > > + ast2700_clk_hw_register_gate(NULL, "rvas2clk", NULL, > > + 0, clk_base + SCU0_CLK_STOP, > > + 28, 0, &ast2700_clk_lock); > > + > > + of_clk_add_hw_provider(soc0_node, of_clk_hw_onecell_get, clk_data); > > + > > + return 0; > > +}; > > + > > +CLK_OF_DECLARE_DRIVER(ast2700_soc0, "aspeed,ast2700-scu0", > > +ast2700_soc0_clk_init); CLK_OF_DECLARE_DRIVER(ast2700_soc1, > > +"aspeed,ast2700-scu1", ast2700_soc1_clk_init); > > Nope, this is not documented. > > You cannot add new compatibles without bindings. > Sorry, I miss yaml file patch. will send it in next version. > Best regards, > Krzysztof
Quoting Ryan Chen (2024-08-27 23:27:40) > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > index 983ef4f36d8c..855b65f2d6dd 100644 > --- a/drivers/clk/Kconfig > +++ b/drivers/clk/Kconfig > @@ -269,6 +269,16 @@ config COMMON_CLK_ASPEED > The G4 and G5 series, including the ast2400 and ast2500, are supported > by this driver. > > +config COMMON_CLK_AST2700 > + bool "Clock driver for AST2700 SoC" > + depends on ARCH_ASPEED || COMPILE_TEST > + select MFD_SYSCON Why is this a syscon? > + select RESET_CONTROLLER > + help > + This driver provides support for clock on AST2700 SoC. > + This driver is responsible for managing the various clocks required > + by the peripherals and cores within the AST2700. > + > config COMMON_CLK_S2MPS11 > tristate "Clock driver for S2MPS1X/S5M8767 MFD" > depends on MFD_SEC_CORE || COMPILE_TEST > diff --git a/drivers/clk/clk-ast2700.c b/drivers/clk/clk-ast2700.c > new file mode 100644 > index 000000000000..7e0466e73980 > --- /dev/null > +++ b/drivers/clk/clk-ast2700.c > @@ -0,0 +1,1198 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2024 ASPEED Technology Inc. > + * Author: Ryan Chen <ryan_chen@aspeedtech.com> > + */ > + > +#include <linux/bits.h> > +#include <linux/clk-provider.h> [...] > + > +struct ast2700_reset { > + void __iomem *base; > + struct ast2700_reset_signal const *signal; > + struct reset_controller_dev rcdev; > +}; Please move the reset controller to the drivers/reset directory by means of using an auxiliary device. There are some existing examples in there if you grep for auxiliary_device in drivers/reset to help guide. > + > +#define to_rc_data(p) container_of(p, struct ast2700_reset, rcdev) > + [...] > + > +static int ast2700_soc0_clk_init(struct device_node *soc0_node) > +{ > + struct clk_hw_onecell_data *clk_data; > + void __iomem *clk_base; [...] > + 0, clk_base + SCU0_CLK_STOP, > + 28, 0, &ast2700_clk_lock); > + > + of_clk_add_hw_provider(soc0_node, of_clk_hw_onecell_get, clk_data); > + > + return 0; > +}; > + > +CLK_OF_DECLARE_DRIVER(ast2700_soc0, "aspeed,ast2700-scu0", ast2700_soc0_clk_init); > +CLK_OF_DECLARE_DRIVER(ast2700_soc1, "aspeed,ast2700-scu1", ast2700_soc1_clk_init); Why can't this be a platform driver?
On Wed, Aug 28, 2024 at 02:27:37PM +0800, Ryan Chen wrote: > This patch series is add clk driver for AST2700. > > AST2700 is the 8th generation of Integrated Remote Management Processor > introduced by ASPEED Technology Inc. Which is Board Management controller > (BMC) SoC family. AST2700 have two SoC connected, one is SoC0, another > is SoC1, it has it's own scu, this driver inlcude SCU0 and SCU1 driver. > > v2: > -yaml: drop 64bits address example. > -yaml: add discription about soc0 and soc1 > -dt-bindings: remove (), *_NUMS, reserved. > -dt-bindings: remove dulipated define number > -clk-ast2700: drop WARN_ON, weird comment. As LKP pointed out, this fails on certain static tests. It is expected that new drvier will pass the open/free static analysis tools we use in the kernel. Please run standard kernel tools for static analysis, like coccinelle, smatch and sparse, and fix reported warnings. Also please check for warnings when building with W=1. Most of these commands (checks or W=1 build) can build specific targets, like some directory, to narrow the scope to only your code. The code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. Best regards, Krzysztof
> Subject: Re: [PATCH v2 3/3] clk: aspeed: add AST2700 clk driver > > Quoting Ryan Chen (2024-08-27 23:27:40) > > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index > > 983ef4f36d8c..855b65f2d6dd 100644 > > --- a/drivers/clk/Kconfig > > +++ b/drivers/clk/Kconfig > > @@ -269,6 +269,16 @@ config COMMON_CLK_ASPEED > > The G4 and G5 series, including the ast2400 and ast2500, are > supported > > by this driver. > > > > +config COMMON_CLK_AST2700 > > + bool "Clock driver for AST2700 SoC" > > + depends on ARCH_ASPEED || COMPILE_TEST > > + select MFD_SYSCON > > Why is this a syscon? I will remove it. > > > + select RESET_CONTROLLER > > + help > > + This driver provides support for clock on AST2700 SoC. > > + This driver is responsible for managing the various clocks > required > > + by the peripherals and cores within the AST2700. > > + > > config COMMON_CLK_S2MPS11 > > tristate "Clock driver for S2MPS1X/S5M8767 MFD" > > depends on MFD_SEC_CORE || COMPILE_TEST diff --git > > a/drivers/clk/clk-ast2700.c b/drivers/clk/clk-ast2700.c new file mode > > 100644 index 000000000000..7e0466e73980 > > --- /dev/null > > +++ b/drivers/clk/clk-ast2700.c > > @@ -0,0 +1,1198 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2024 ASPEED Technology Inc. > > + * Author: Ryan Chen <ryan_chen@aspeedtech.com> */ > > + > > +#include <linux/bits.h> > > +#include <linux/clk-provider.h> > [...] > > + > > +struct ast2700_reset { > > + void __iomem *base; > > + struct ast2700_reset_signal const *signal; > > + struct reset_controller_dev rcdev; }; > > Please move the reset controller to the drivers/reset directory by means of > using an auxiliary device. There are some existing examples in there if you > grep for auxiliary_device in drivers/reset to help guide. Do you mean to have another reset controller driver? If yes, I may need syscon for remap two drivers. > > > + > > +#define to_rc_data(p) container_of(p, struct ast2700_reset, rcdev) > > + > [...] > > + > > +static int ast2700_soc0_clk_init(struct device_node *soc0_node) { > > + struct clk_hw_onecell_data *clk_data; > > + void __iomem *clk_base; > [...] > > + 0, clk_base + > SCU0_CLK_STOP, > > + 28, 0, > > + &ast2700_clk_lock); > > + > > + of_clk_add_hw_provider(soc0_node, of_clk_hw_onecell_get, > > + clk_data); > > + > > + return 0; > > +}; > > + > > +CLK_OF_DECLARE_DRIVER(ast2700_soc0, "aspeed,ast2700-scu0", > > +ast2700_soc0_clk_init); CLK_OF_DECLARE_DRIVER(ast2700_soc1, > > +"aspeed,ast2700-scu1", ast2700_soc1_clk_init); > > Why can't this be a platform driver? Due to clk and reset will be the first driver in core. Do you think all drivers should be platform driver?
Quoting Ryan Chen (2024-08-29 00:09:12) > > Subject: Re: [PATCH v2 3/3] clk: aspeed: add AST2700 clk driver > > > > Quoting Ryan Chen (2024-08-27 23:27:40) > > > a/drivers/clk/clk-ast2700.c b/drivers/clk/clk-ast2700.c new file mode > > > 100644 index 000000000000..7e0466e73980 > > > --- /dev/null > > > +++ b/drivers/clk/clk-ast2700.c > > > @@ -0,0 +1,1198 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Copyright (c) 2024 ASPEED Technology Inc. > > > + * Author: Ryan Chen <ryan_chen@aspeedtech.com> */ > > > + > > > +#include <linux/bits.h> > > > +#include <linux/clk-provider.h> > > [...] > > > + > > > +struct ast2700_reset { > > > + void __iomem *base; > > > + struct ast2700_reset_signal const *signal; > > > + struct reset_controller_dev rcdev; }; > > > > Please move the reset controller to the drivers/reset directory by means of > > using an auxiliary device. There are some existing examples in there if you > > grep for auxiliary_device in drivers/reset to help guide. > > Do you mean to have another reset controller driver? > If yes, I may need syscon for remap two drivers. Yes. A syscon is not necessary to do that. > > > > > + > > > +#define to_rc_data(p) container_of(p, struct ast2700_reset, rcdev) > > > + > > [...] > > > + > > > +static int ast2700_soc0_clk_init(struct device_node *soc0_node) { > > > + struct clk_hw_onecell_data *clk_data; > > > + void __iomem *clk_base; > > [...] > > > + 0, clk_base + > > SCU0_CLK_STOP, > > > + 28, 0, > > > + &ast2700_clk_lock); > > > + > > > + of_clk_add_hw_provider(soc0_node, of_clk_hw_onecell_get, > > > + clk_data); > > > + > > > + return 0; > > > +}; > > > + > > > +CLK_OF_DECLARE_DRIVER(ast2700_soc0, "aspeed,ast2700-scu0", > > > +ast2700_soc0_clk_init); CLK_OF_DECLARE_DRIVER(ast2700_soc1, > > > +"aspeed,ast2700-scu1", ast2700_soc1_clk_init); > > > > Why can't this be a platform driver? > Due to clk and reset will be the first driver in core. > Do you think all drivers should be platform driver? Yes all drivers should be a platform driver.
> Subject: RE: [PATCH v2 3/3] clk: aspeed: add AST2700 clk driver > > Quoting Ryan Chen (2024-08-29 00:09:12) > > > Subject: Re: [PATCH v2 3/3] clk: aspeed: add AST2700 clk driver > > > > > > Quoting Ryan Chen (2024-08-27 23:27:40) > > > > a/drivers/clk/clk-ast2700.c b/drivers/clk/clk-ast2700.c new file > > > > mode > > > > 100644 index 000000000000..7e0466e73980 > > > > --- /dev/null > > > > +++ b/drivers/clk/clk-ast2700.c > > > > @@ -0,0 +1,1198 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +/* > > > > + * Copyright (c) 2024 ASPEED Technology Inc. > > > > + * Author: Ryan Chen <ryan_chen@aspeedtech.com> */ > > > > + > > > > +#include <linux/bits.h> > > > > +#include <linux/clk-provider.h> > > > [...] > > > > + > > > > +struct ast2700_reset { > > > > + void __iomem *base; > > > > + struct ast2700_reset_signal const *signal; > > > > + struct reset_controller_dev rcdev; }; > > > > > > Please move the reset controller to the drivers/reset directory by > > > means of using an auxiliary device. There are some existing examples > > > in there if you grep for auxiliary_device in drivers/reset to help guide. > > > > Do you mean to have another reset controller driver? > > If yes, I may need syscon for remap two drivers. > > Yes. A syscon is not necessary to do that. Thanks for point out auxiliary device. Since our SoC reset and clock share the same register region. Your recommend will use clk auxiliary_device_add for reset driver am I right? > > > > > > > > + > > > > +#define to_rc_data(p) container_of(p, struct ast2700_reset, > > > > +rcdev) > > > > + > > > [...] > > > > + > > > > +static int ast2700_soc0_clk_init(struct device_node *soc0_node) { > > > > + struct clk_hw_onecell_data *clk_data; > > > > + void __iomem *clk_base; > > > [...] > > > > + 0, clk_base + > > > SCU0_CLK_STOP, > > > > + 28, 0, > > > > + &ast2700_clk_lock); > > > > + > > > > + of_clk_add_hw_provider(soc0_node, of_clk_hw_onecell_get, > > > > + clk_data); > > > > + > > > > + return 0; > > > > +}; > > > > + > > > > +CLK_OF_DECLARE_DRIVER(ast2700_soc0, "aspeed,ast2700-scu0", > > > > +ast2700_soc0_clk_init); CLK_OF_DECLARE_DRIVER(ast2700_soc1, > > > > +"aspeed,ast2700-scu1", ast2700_soc1_clk_init); > > > > > > Why can't this be a platform driver? > > Due to clk and reset will be the first driver in core. > > Do you think all drivers should be platform driver? > > Yes all drivers should be a platform driver.
Quoting Ryan Chen (2024-08-30 00:50:21) > > Subject: RE: [PATCH v2 3/3] clk: aspeed: add AST2700 clk driver > > > > Quoting Ryan Chen (2024-08-29 00:09:12) > > > > Subject: Re: [PATCH v2 3/3] clk: aspeed: add AST2700 clk driver > > > > > > > > Quoting Ryan Chen (2024-08-27 23:27:40) > > > > > a/drivers/clk/clk-ast2700.c b/drivers/clk/clk-ast2700.c new file > > > > > mode > > > > > 100644 index 000000000000..7e0466e73980 > > > > > --- /dev/null > > > > > +++ b/drivers/clk/clk-ast2700.c > > > > > @@ -0,0 +1,1198 @@ > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > +/* > > > > > + * Copyright (c) 2024 ASPEED Technology Inc. > > > > > + * Author: Ryan Chen <ryan_chen@aspeedtech.com> */ > > > > > + > > > > > +#include <linux/bits.h> > > > > > +#include <linux/clk-provider.h> > > > > [...] > > > > > + > > > > > +struct ast2700_reset { > > > > > + void __iomem *base; > > > > > + struct ast2700_reset_signal const *signal; > > > > > + struct reset_controller_dev rcdev; }; > > > > > > > > Please move the reset controller to the drivers/reset directory by > > > > means of using an auxiliary device. There are some existing examples > > > > in there if you grep for auxiliary_device in drivers/reset to help guide. > > > > > > Do you mean to have another reset controller driver? > > > If yes, I may need syscon for remap two drivers. > > > > Yes. A syscon is not necessary to do that. > Thanks for point out auxiliary device. > Since our SoC reset and clock share the same register region. > Your recommend will use clk auxiliary_device_add for reset driver am I right? Yes. Either the clk or the reset driver can be a platform driver and the other an auxiliary driver.