Re: [PATCH 2/4] clocksource/drivers/sun5i: add H616 hstimer support

[Andre Przywara <andre.przywara@arm.com>]

On Sun, 19 Apr 2026 14:46:08 +0200
Michal Piekos <michal.piekos@mmpsystems.pl> wrote:

Hi,

> H616 high speed timer differs from existing timer-sun5i by register base
> offset.
> 
> Add selectable register layout structures.
> Add H616 compatible string to OF match table.
> 
> Signed-off-by: Michal Piekos <michal.piekos@mmpsystems.pl>
> ---
>  drivers/clocksource/timer-sun5i.c | 56 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 50 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-sun5i.c b/drivers/clocksource/timer-sun5i.c
> index f827d3f98f60..125abc11c3c3 100644
> --- a/drivers/clocksource/timer-sun5i.c
> +++ b/drivers/clocksource/timer-sun5i.c
> @@ -21,18 +21,52 @@
>  #define TIMER_IRQ_EN_REG		0x00
>  #define TIMER_IRQ_EN(val)			BIT(val)
>  #define TIMER_IRQ_ST_REG		0x04
> -#define TIMER_CTL_REG(val)		(0x20 * (val) + 0x10)
>  #define TIMER_CTL_ENABLE			BIT(0)
>  #define TIMER_CTL_RELOAD			BIT(1)
> -#define TIMER_CTL_CLK_PRES(val)			(((val) & 0x7) << 4)
>  #define TIMER_CTL_ONESHOT			BIT(7)
> -#define TIMER_INTVAL_LO_REG(val)	(0x20 * (val) + 0x14)
> -#define TIMER_INTVAL_HI_REG(val)	(0x20 * (val) + 0x18)
> -#define TIMER_CNTVAL_LO_REG(val)	(0x20 * (val) + 0x1c)
> -#define TIMER_CNTVAL_HI_REG(val)	(0x20 * (val) + 0x20)
> +#define TIMER_CTL_CLK_PRES(val)		(((val) & 0x7) << 4)
> +#define TIMER_CTL_REG(val)		\
> +	(soc_base->stride * (val) + soc_base->ctl_base)
> +#define TIMER_INTVAL_LO_REG(val)	\
> +	(soc_base->stride * (val) + soc_base->intval_lo_base)
> +#define TIMER_INTVAL_HI_REG(val)	\
> +	(soc_base->stride * (val) + soc_base->intval_hi_base)
> +#define TIMER_CNTVAL_LO_REG(val)	\
> +	(soc_base->stride * (val) + soc_base->cntval_lo_base)
> +#define TIMER_CNTVAL_HI_REG(val)	\
> +	(soc_base->stride * (val) + soc_base->cntval_hi_base)
>  
>  #define TIMER_SYNC_TICKS	3
>  
> +struct sunxi_timer_base {
> +	u32 ctl_base;
> +	u32 intval_lo_base;
> +	u32 intval_hi_base;
> +	u32 cntval_lo_base;
> +	u32 cntval_hi_base;
> +	u32 stride;
> +};
> +
> +static const struct sunxi_timer_base sun5i_base = {
> +	.ctl_base = 0x10,
> +	.intval_lo_base = 0x14,
> +	.intval_hi_base = 0x18,
> +	.cntval_lo_base = 0x1c,
> +	.cntval_hi_base = 0x20,

Mmmh, why all these members? Aren't those all the same, just offset by
0x10? So we just need a single value reg_offs, being either 0x0 or 0x10?

> +	.stride = 0x20

What it this about? It's the same stride for both versions, so why is
this a field?

> +};
> +
> +static const struct sunxi_timer_base sun50i_base = {
> +	.ctl_base = 0x20,
> +	.intval_lo_base = 0x24,
> +	.intval_hi_base = 0x28,
> +	.cntval_lo_base = 0x2c,
> +	.cntval_hi_base = 0x30,
> +	.stride = 0x20
> +};
> +
> +static const struct sunxi_timer_base *soc_base;

This doesn't look right. Differentiating between slightly different
hardware revision via the compatible string is a common pattern, look
at for instance drivers/media/rc/sunxi-cir.c and its usage of quirks for
an example how to handle this more nicely.

Cheers,
Andre

> +
>  struct sun5i_timer {
>  	void __iomem		*base;
>  	struct clk		*clk;
> @@ -238,6 +272,7 @@ static int sun5i_setup_clockevent(struct platform_device *pdev,
>  static int sun5i_timer_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev_of_node(&pdev->dev);
>  	struct sun5i_timer *st;
>  	struct reset_control *rstc;
>  	void __iomem *timer_base;
> @@ -251,6 +286,14 @@ static int sun5i_timer_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, st);
>  
> +	if (!node)
> +		return -EINVAL;
> +
> +	if (of_device_is_compatible(node, "allwinner,sun50i-h616-hstimer"))
> +		soc_base = &sun50i_base;
> +	else
> +		soc_base = &sun5i_base;
> +
>  	timer_base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(timer_base)) {
>  		dev_err(dev, "Can't map registers\n");
> @@ -314,6 +357,7 @@ static void sun5i_timer_remove(struct platform_device *pdev)
>  static const struct of_device_id sun5i_timer_of_match[] = {
>  	{ .compatible = "allwinner,sun5i-a13-hstimer" },
>  	{ .compatible = "allwinner,sun7i-a20-hstimer" },
> +	{ .compatible = "allwinner,sun50i-h616-hstimer" },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, sun5i_timer_of_match);
>