| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20231215180127.GB52386-robh@kernel.org>
Date: Fri, 15 Dec 2023 12:01:27 -0600
From: Rob Herring <robh@...nel.org>
To: Elad Nachman <enachman@...vell.com>
Cc: wim@...ux-watchdog.org, linux@...ck-us.net, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, gregory.clement@...tlin.com, chris.packham@...iedtelesis.co.nz, andrew@...n.ch, fu.wei@...aro.org, Suravee.Suthikulpanit@....com, al.stone@...aro.org, timur@...eaurora.org, linux-watchdog@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, cyuval@...vell.com
Subject: Re: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5
On Thu, Dec 14, 2023 at 05:04:14PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@...vell.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@...vell.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
Powered by blists - more mailing lists