Received-SPF: pass (google.com: domain of linux-kernel+bounces-1499-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 15 Dec 2023 12:01:27 -0600
From: Rob Herring <robh@kernel.org>
To: Elad Nachman <enachman@marvell.com>
Cc: wim@linux-watchdog.org, linux@roeck-us.net, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, gregory.clement@bootlin.com, chris.packham@alliedtelesis.co.nz, andrew@lunn.ch, fu.wei@linaro.org, Suravee.Suthikulpanit@amd.com, al.stone@linaro.org, timur@codeaurora.org, linux-watchdog@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, cyuval@marvell.com
Subject: Re: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5
Message-ID: <20231215180127.GB52386-robh@kernel.org>
References: <20231214150414.1849058-1-enachman@marvell.com>
 <20231214150414.1849058-4-enachman@marvell.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231214150414.1849058-4-enachman@marvell.com>

On Thu, Dec 14, 2023 at 05:04:14PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Add support for Marvell ac5/x variant of the ARM
> sbsa global watchdog. This watchdog deviates from
> the standard driver by the following items:
> 
> 1. Registers reside in secure register section.
>    hence access is only possible via SMC calls to ATF.
> 
> 2. There are couple more registers which reside in
>    other register areas, which needs to be configured
>    in order for the watchdog to properly generate
>    reset through the SOC.
> 
> The new Marvell compatibility string differentiates between
> the original sbsa mode of operation and the Marvell mode of
> operation.
> 
> Signed-off-by: Elad Nachman <enachman@marvell.com>
> ---
>  drivers/watchdog/sbsa_gwdt.c | 247 ++++++++++++++++++++++++++++++++---

That's more than half the existing driver...

>  1 file changed, 226 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
> index 5f23913ce3b4..0bc6f53f0968 100644
> --- a/drivers/watchdog/sbsa_gwdt.c
> +++ b/drivers/watchdog/sbsa_gwdt.c
> @@ -46,10 +46,13 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/uaccess.h>
>  #include <linux/watchdog.h>
>  #include <asm/arch_timer.h>
> +#include <linux/arm-smccc.h>
>  
>  #define DRV_NAME		"sbsa-gwdt"
>  #define WATCHDOG_NAME		"SBSA Generic Watchdog"
> @@ -75,6 +78,68 @@
>  #define SBSA_GWDT_VERSION_MASK  0xF
>  #define SBSA_GWDT_VERSION_SHIFT 16
>  
> +/* Marvell AC5/X SMCs, taken from arm trusted firmware */
> +#define SMC_FID_READ_REG	0x80007FFE
> +#define SMC_FID_WRITE_REG	0x80007FFD
> +
> +/* Marvell registers offsets: */
> +#define SBSA_GWDT_MARVELL_CPU_WD_RST_EN_REG	0x30
> +#define SBSA_GWDT_MARVELL_MNG_ID_REG		0x4C
> +#define SBSA_GWDT_MARVELL_RST_CTRL_REG		0x0C
> +
> +#define SBSA_GWDT_MARVELL_ID_MASK	GENMASK(19, 12)
> +#define SBSA_GWDT_MARVELL_AC5_ID	0xB4000
> +#define SBSA_GWDT_MARVELL_AC5X_ID	0x98000
> +#define SBSA_GWDT_MARVELL_IML_ID	0xA0000
> +#define SBSA_GWDT_MARVELL_IMM_ID	0xA2000
> +
> +#define SBSA_GWDT_MARVELL_AC5_RST_UNIT_WD_BIT		BIT(6)
> +/* The following applies to AC5X, IronMan L and M: */
> +#define SBSA_GWDT_MARVELL_IRONMAN_RST_UNIT_WD_BIT	BIT(7)
> +
> +/*
> + * Action to perform after watchdog gets WS1 (watchdog signal 1) interrupt
> + * PWD = Private Watchdog, GWD - Global Watchdog, mpp - multi purpose pin
> + *
> + * 0 = Enable  1 = Disable (Default)
> + *
> + * BIT  0: CPU 0 reset by PWD 0
> + * BIT  1: CPU 1 reset by PWD 1
> + * BIT  2: CPU 0 reset by GWD
> + * BIT  3: CPU 1 reset by GWD
> + * BIT  4: PWD 0 sys reset out
> + * BIT  5: PWD 1 sys reset out
> + * BIT  6: GWD sys reset out
> + * BIT  7: Reserved
> + * BIT  8: PWD 0 mpp reset out
> + * BIT  9: PWD 1 mpp reset out
> + * BIT 10: GWD mpp reset out
> + *
> + */
> +#define SBSA_GWDT_MARVELL_RST_CPU0_BY_PWD0	BIT(0)
> +#define SBSA_GWDT_MARVELL_RST_CPU1_BY_PWD1	BIT(1)
> +#define SBSA_GWDT_MARVELL_RST_CPU0_BY_GWD	BIT(2)
> +#define SBSA_GWDT_MARVELL_RST_CPU1_BY_GWD	BIT(3)
> +#define SBSA_GWDT_MARVELL_RST_SYSRST_BY_PWD0	BIT(4)
> +#define SBSA_GWDT_MARVELL_RST_SYSRST_BY_PWD1	BIT(5)
> +#define SBSA_GWDT_MARVELL_RST_SYSRST_BY_GWD	BIT(6)
> +#define SBSA_GWDT_MARVELL_RST_RESERVED		BIT(7)
> +#define SBSA_GWDT_MARVELL_RST_MPP_BY_PWD0	BIT(8)
> +#define SBSA_GWDT_MARVELL_RST_MPP_BY_PWD1	BIT(9)
> +#define SBSA_GWDT_MARVELL_RST_MPP_BY_GWD	BIT(10)
> +
> +/**
> + * struct sbsa_gwdt_regs_ops - ops for register read/write, depending on SOC
> + * @reg_read:			register read ops function
> + * @read_write:			register write ops function
> + */
> +struct sbsa_gwdt_regs_ops {
> +	u32 (*reg_read32)(void __iomem *ptr);
> +	__u64 (*reg_read64)(void __iomem *ptr);
> +	void (*reg_write32)(u32 val, void __iomem *ptr);
> +	void (*reg_write64)(__u64 val, void __iomem *ptr);
> +};
> +
>  /**
>   * struct sbsa_gwdt - Internal representation of the SBSA GWDT
>   * @wdd:		kernel watchdog_device structure
> @@ -89,6 +154,7 @@ struct sbsa_gwdt {
>  	int			version;
>  	void __iomem		*refresh_base;
>  	void __iomem		*control_base;
> +	const struct sbsa_gwdt_regs_ops *soc_reg_ops;
>  };
>  
>  #define DEFAULT_TIMEOUT		10 /* seconds */
> @@ -116,6 +182,91 @@ MODULE_PARM_DESC(nowayout,
>  		 "Watchdog cannot be stopped once started (default="
>  		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>  
> +/*
> + * By default, have the Global watchdog cause System Reset:
> + */
> +static unsigned int reset = 0xFFFFFFFF ^ SBSA_GWDT_MARVELL_RST_SYSRST_BY_GWD;
> +module_param(reset, uint, 0);
> +MODULE_PARM_DESC(reset, "Action to perform after watchdog gets WS1 interrupt");
> +
> +/*
> + * Marvell AC5/X use SMC, while others use direct register access
> + */
> +static u32 sbsa_gwdt_smc_readl(void __iomem *addr)
> +{
> +	struct arm_smccc_res smc_res;
> +
> +	arm_smccc_smc(SMC_FID_READ_REG, (unsigned long)addr,
> +		      0, 0, 0, 0, 0, 0, &smc_res);
> +	return (u32)smc_res.a0;
> +}
> +
> +static void sbsa_gwdt_smc_writel(u32 val, void __iomem *addr)
> +{
> +	struct arm_smccc_res smc_res;
> +
> +	arm_smccc_smc(SMC_FID_WRITE_REG, (unsigned long)addr,
> +		      (unsigned long)val, 0, 0, 0, 0, 0, &smc_res);
> +}
> +
> +static inline u64 sbsa_gwdt_lo_hi_smc_readq(void __iomem *addr)
> +{
> +	u32 low, high;
> +
> +	low = sbsa_gwdt_smc_readl(addr);
> +	high = sbsa_gwdt_smc_readl(addr + 4);
> +	/* read twice, as a workaround to HW limitation */
> +	low = sbsa_gwdt_smc_readl(addr);
> +
> +	return low + ((u64)high << 32);
> +}
> +
> +static inline void sbsa_gwdt_lo_hi_smc_writeq(__u64 val, void __iomem *addr)
> +{
> +	u32 low, high;
> +
> +	low = val & 0xffffffff;
> +	high = val >> 32;
> +	sbsa_gwdt_smc_writel(low, addr);
> +	sbsa_gwdt_smc_writel(high, addr + 4);
> +	/* write twice, as a workaround to HW limitation */
> +	sbsa_gwdt_smc_writel(low, addr);
> +}
> +
> +static u32 sbsa_gwdt_direct_reg_readl(void __iomem *addr)
> +{
> +	return readl(addr);
> +}
> +
> +static void sbsa_gwdt_direct_reg_writel(u32 val, void __iomem *addr)
> +{
> +	writel(val, addr);
> +}
> +
> +static inline u64 sbsa_gwdt_lo_hi_direct_readq(void __iomem *addr)
> +{
> +	return lo_hi_readq(addr);
> +}
> +
> +static inline void sbsa_gwdt_lo_hi_direct_writeq(__u64 val, void __iomem *addr)
> +{
> +	lo_hi_writeq(val, addr);
> +}
> +
> +static const struct sbsa_gwdt_regs_ops smc_reg_ops = {
> +	.reg_read32 = sbsa_gwdt_smc_readl,
> +	.reg_read64 = sbsa_gwdt_lo_hi_smc_readq,
> +	.reg_write32 = sbsa_gwdt_smc_writel,
> +	.reg_write64 = sbsa_gwdt_lo_hi_smc_writeq
> +};
> +
> +static const struct sbsa_gwdt_regs_ops direct_reg_ops = {
> +	.reg_read32 = sbsa_gwdt_direct_reg_readl,
> +	.reg_read64 = sbsa_gwdt_lo_hi_direct_readq,
> +	.reg_write32 = sbsa_gwdt_direct_reg_writel,
> +	.reg_write64 = sbsa_gwdt_lo_hi_smc_writeq
> +};

The watchdog_ops are already practically not much more than a register 
read or write. Do we really need 2 levels of ops here?

> +
>  /*
>   * Arm Base System Architecture 1.0 introduces watchdog v1 which
>   * increases the length watchdog offset register to 48 bits.
> @@ -127,17 +278,17 @@ MODULE_PARM_DESC(nowayout,
>  static u64 sbsa_gwdt_reg_read(struct sbsa_gwdt *gwdt)
>  {
>  	if (gwdt->version == 0)
> -		return readl(gwdt->control_base + SBSA_GWDT_WOR);
> +		return gwdt->soc_reg_ops->reg_read32(gwdt->control_base + SBSA_GWDT_WOR);
>  	else
> -		return lo_hi_readq(gwdt->control_base + SBSA_GWDT_WOR);
> +		return gwdt->soc_reg_ops->reg_read64(gwdt->control_base + SBSA_GWDT_WOR);
>  }

Here we already have a different way to abstract register accesses. 
Probably should have something that works for all 3 cases...

Rob