Re: [PATCH RESEND 6/7] clk: spacemit: define new syscons with only resets

[Yixun Lan <dlan@gentoo.org>]

Hi Alex:

On 08:23 Sun 23 Mar     , Alex Elder wrote:
> On 3/22/25 11:42 AM, Yixun Lan wrote:
> > Hi Alex:
> > 
> > It occur to me it's a little odd to implemnt reset driver
> > for RCPU block, but after check with vendor the RCPU region can
> > be accessed both by ACPU and RCPU, then I'm fine with this.
> 
> I implemented just the resets that were found in the downstream
> code.
> 
> I first implemented a separate reset driver, very simple, which
> only implemented the resets.  I had a separate DTS binding (like
> was done for the PLLs).  I was ready to post it for review, then
> noticed that the registers used were shared with clocks.  So I
> merged all of that separate code into the clock driver, as you
> see here.
> 
ok

> > ACPU - RISC-V Main CPU, with mmu, running Linux
> > RCPU - real time CPU, without mmu, running RT-OS
> 
> I didn't realize there was a separate CPU running its
> own OS.  Is this managed as a remoteproc by the RISC-V AP?
> The reset signals, I hope, are only touched by the AP
> and not the real-time CPU.  Can you provide any further
> information about this?
> 
As far as I know, the RCPU region can be acccesed via AP and real-time CPU
from hardware perspective, there is no guarantee of isolation,
so maybe software should take care of this in case only one side can touch

for remoteproc, I haven't checked, and it's unrelated to this discussion
(doesn't change shared resource fact whether remoteproc supported or not)

> > On 10:18 Fri 21 Mar     , Alex Elder wrote:
> >> Enable support for three additional syscon CCUs which support reset
> >> controls but no clocks:  ARCPU, RCPU2, and APBC2.
> >>
> >> Signed-off-by: Alex Elder <elder@riscstar.com>
> >> ---
> >>   drivers/clk/spacemit/ccu-k1.c | 106 ++++++++++++++++++++++++++++++++++
> >>   1 file changed, 106 insertions(+)
> >>
> >> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
> >> index 17e321c25959a..bf5a3e2048619 100644
> >> --- a/drivers/clk/spacemit/ccu-k1.c
> >> +++ b/drivers/clk/spacemit/ccu-k1.c
> >> @@ -130,6 +130,37 @@
> >>   #define APMU_EMAC0_CLK_RES_CTRL		0x3e4
> >>   #define APMU_EMAC1_CLK_RES_CTRL		0x3ec
> >>   
> >> +/* RCPU register offsets */
> >> +#define RCPU_SSP0_CLK_RST		0x0028
> >> +#define RCPU_I2C0_CLK_RST		0x0030
> >> +#define RCPU_UART1_CLK_RST		0x003c
> >> +#define RCPU_CAN_CLK_RST		0x0048
> >> +#define RCPU_IR_CLK_RST			0x004c
> >> +#define RCPU_UART0_CLK_RST		0x00d8
> >> +/* XXX Next one is part of the AUD_AUDCLOCK region @ 0xc0882000 */
> > this comment looks odd, XXX?
> 
> Yeah, I meant to remove that before sending but I forgot.
> 
> The downstream code treats this one register as being
> part of the RCPU memory region, and extends that region
> to be 0x2048 bytes to "fit" it.
> 
> The hardware documentation actually defines a different
> "RCPU Audio Clock" memory region, and it might be more
> correct (though less convenient) to define that as a
> distinct region of memory.
> 
> What do you think?
> 
I'm not sure, but from DT perspective, is it an independent device?
if yes, then need to describe as a distinct region..
> 					-Alex
> 
> >> +#define AUDIO_HDMI_CLK_CTRL		0x2044
> >> +
> >> +/* RCPU2 register offsets */
> >> +#define RCPU2_PWM0_CLK_RST		0x0000
> >> +#define RCPU2_PWM1_CLK_RST		0x0004
> >> +#define RCPU2_PWM2_CLK_RST		0x0008
> >> +#define RCPU2_PWM3_CLK_RST		0x000c
> >> +#define RCPU2_PWM4_CLK_RST		0x0010
> >> +#define RCPU2_PWM5_CLK_RST		0x0014
> >> +#define RCPU2_PWM6_CLK_RST		0x0018
> >> +#define RCPU2_PWM7_CLK_RST		0x001c
> >> +#define RCPU2_PWM8_CLK_RST		0x0020
> >> +#define RCPU2_PWM9_CLK_RST		0x0024
> >> +
> >> +/* APBC2 register offsets */
> >> +#define APBC2_UART1_CLK_RST		0x0000
> >> +#define APBC2_SSP2_CLK_RST		0x0004
> >> +#define APBC2_TWSI3_CLK_RST		0x0008
> >> +#define APBC2_RTC_CLK_RST		0x000c
> >> +#define APBC2_TIMERS0_CLK_RST		0x0010
> >> +#define APBC2_KPC_CLK_RST		0x0014
> >> +#define APBC2_GPIO_CLK_RST		0x001c
> >> +
> >>   struct spacemit_ccu_clk {
> >>   	int id;
> >>   	struct clk_hw *hw;
> >> @@ -1781,6 +1812,69 @@ static const struct k1_ccu_data k1_ccu_apmu_data = {
> >>   	.rst_data	= &apmu_reset_controller_data,
> >>   };
> >>   
> >> +static const struct ccu_reset_data rcpu_reset_data[] = {
> >> +	[RST_RCPU_SSP0]		= RST_DATA(RCPU_SSP0_CLK_RST,	0, BIT(0)),
> >> +	[RST_RCPU_I2C0]		= RST_DATA(RCPU_I2C0_CLK_RST,	0, BIT(0)),
> >> +	[RST_RCPU_UART1]	= RST_DATA(RCPU_UART1_CLK_RST,	0, BIT(0)),
> >> +	[RST_RCPU_IR]		= RST_DATA(RCPU_CAN_CLK_RST,	0, BIT(0)),
> >> +	[RST_RCPU_CAN]		= RST_DATA(RCPU_IR_CLK_RST,	0, BIT(0)),
> >> +	[RST_RCPU_UART0]	= RST_DATA(RCPU_UART0_CLK_RST,	0, BIT(0)),
> >> +	[RST_RCPU_HDMI_AUDIO]	= RST_DATA(AUDIO_HDMI_CLK_CTRL,	0, BIT(0)),
> >> +};
> >> +
> >> +static const struct ccu_reset_controller_data rcpu_reset_controller_data = {
> >> +	.count		= ARRAY_SIZE(rcpu_reset_data),
> >> +	.data		= rcpu_reset_data,
> >> +};
> >> +
> >> +static struct k1_ccu_data k1_ccu_rcpu_data = {
> >> +	/* No clocks in the RCPU CCU */
> >> +	.rst_data	= &rcpu_reset_controller_data,
> >> +};
> >> +
> >> +static const struct ccu_reset_data rcpu2_reset_data[] = {
> >> +	[RST_RCPU2_PWM0]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
> >> +	[RST_RCPU2_PWM1]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
> >> +	[RST_RCPU2_PWM2]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
> >> +	[RST_RCPU2_PWM3]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
> >> +	[RST_RCPU2_PWM4]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
> >> +	[RST_RCPU2_PWM5]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
> >> +	[RST_RCPU2_PWM6]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
> >> +	[RST_RCPU2_PWM7]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
> >> +	[RST_RCPU2_PWM8]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
> >> +	[RST_RCPU2_PWM9]	= RST_DATA(RCPU2_PWM9_CLK_RST,	BIT(2), BIT(0)),
> >> +};
> >> +
> >> +static const struct ccu_reset_controller_data rcpu2_reset_controller_data = {
> >> +	.count		= ARRAY_SIZE(rcpu2_reset_data),
> >> +	.data		= rcpu2_reset_data,
> >> +};
> >> +
> >> +static struct k1_ccu_data k1_ccu_rcpu2_data = {
> >> +	/* No clocks in the RCPU2 CCU */
> >> +	.rst_data	= &rcpu2_reset_controller_data,
> >> +};
> >> +
> >> +static const struct ccu_reset_data apbc2_reset_data[] = {
> >> +	[RST_APBC2_UART1]	= RST_DATA(APBC2_UART1_CLK_RST,	BIT(2), (0)),
> >> +	[RST_APBC2_SSP2]	= RST_DATA(APBC2_SSP2_CLK_RST,	BIT(2), (0)),
> >> +	[RST_APBC2_TWSI3]	= RST_DATA(APBC2_TWSI3_CLK_RST,	BIT(2), (0)),
> >> +	[RST_APBC2_RTC]		= RST_DATA(APBC2_RTC_CLK_RST,	BIT(2), (0)),
> >> +	[RST_APBC2_TIMERS0]	= RST_DATA(APBC2_TIMERS0_CLK_RST, BIT(2), (0)),
> >> +	[RST_APBC2_KPC]		= RST_DATA(APBC2_KPC_CLK_RST,	BIT(2), (0)),
> >> +	[RST_APBC2_GPIO]	= RST_DATA(APBC2_GPIO_CLK_RST,	BIT(2), (0)),
> >> +};
> >> +
> >> +static const struct ccu_reset_controller_data apbc2_reset_controller_data = {
> >> +	.count		= ARRAY_SIZE(apbc2_reset_data),
> >> +	.data		= apbc2_reset_data,
> >> +};
> >> +
> >> +static struct k1_ccu_data k1_ccu_apbc2_data = {
> >> +	/* No clocks in the RCPU2 CCU */
> >> +	.rst_data	= &apbc2_reset_controller_data,
> >> +};
> >> +
> >>   static struct ccu_reset_controller *
> >>   rcdev_to_controller(struct reset_controller_dev *rcdev)
> >>   {
> >> @@ -1959,6 +2053,18 @@ static const struct of_device_id of_k1_ccu_match[] = {
> >>   		.compatible	= "spacemit,k1-syscon-apmu",
> >>   		.data		= &k1_ccu_apmu_data,
> >>   	},
> >> +	{
> >> +		.compatible	= "spacemit,k1-syscon-rcpu",
> >> +		.data		= &k1_ccu_rcpu_data,
> >> +	},
> >> +	{
> >> +		.compatible	= "spacemit,k1-syscon-rcpu2",
> >> +		.data		= &k1_ccu_rcpu2_data,
> >> +	},
> >> +	{
> >> +		.compatible	= "spacemit,k1-syscon-apbc2",
> >> +		.data		= &k1_ccu_apbc2_data,
> >> +	},
> >>   	{ }
> >>   };
> >>   MODULE_DEVICE_TABLE(of, of_k1_ccu_match);
> >> -- 
> >> 2.43.0
> >>
> > 
> 

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55