* Re: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5
2023-12-14 15:04 ` [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5 Elad Nachman
@ 2023-12-14 15:16 ` Krzysztof Kozlowski
2023-12-15 1:08 ` kernel test robot
` (4 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2023-12-14 15:16 UTC (permalink / raw)
To: Elad Nachman, wim, linux, robh+dt, krzysztof.kozlowski+dt,
conor+dt, gregory.clement, chris.packham, andrew, fu.wei,
Suravee.Suthikulpanit, al.stone, timur, linux-watchdog,
devicetree, linux-kernel
Cc: cyuval
On 14/12/2023 16:04, 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>
> ---
...
> gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
> if (!gwdt)
> return -ENOMEM;
> platform_set_drvdata(pdev, gwdt);
>
> - cf_base = devm_platform_ioremap_resource(pdev, 0);
> - if (IS_ERR(cf_base))
> - return PTR_ERR(cf_base);
> -
> - rf_base = devm_platform_ioremap_resource(pdev, 1);
> - if (IS_ERR(rf_base))
> - return PTR_ERR(rf_base);
> + if (of_device_is_compatible(np, "marvell,ac5-wd")) {
No, use match data. That's its purpose, don't put comaptibles inside code.
> + marvell = true;
> + gwdt->soc_reg_ops = &smc_reg_ops;
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (IS_ERR(res))
> + return PTR_ERR(res);
> + cf_base = res->start;
Why do you use entirely different code?
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (IS_ERR(res))
> + return PTR_ERR(res);
> + rf_base = res->start;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> + if (IS_ERR(res))
> + return PTR_ERR(res);
> + cpu_ctrl_base = res->start;
> + mng_base = devm_platform_ioremap_resource(pdev, 3);
> + if (IS_ERR(mng_base))
> + return PTR_ERR(mng_base);
> + rst_ctrl_base = devm_platform_ioremap_resource(pdev, 4);
> + if (IS_ERR(rst_ctrl_base))
> + return PTR_ERR(rst_ctrl_base);
> + } else {
> + gwdt->soc_reg_ops = &direct_reg_ops;
> + cf_base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(cf_base))
> + return PTR_ERR(cf_base);
Why? This is shared.
> +
> + rf_base = devm_platform_ioremap_resource(pdev, 1);
> + if (IS_ERR(rf_base))
> + return PTR_ERR(rf_base);
Ditto
> + }
>
> /*
> * Get the frequency of system counter from the cp15 interface of ARM
> @@ -299,7 +482,7 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
> else
> wdd->max_hw_heartbeat_ms = GENMASK_ULL(47, 0) / gwdt->clk * 1000;
>
> - status = readl(cf_base + SBSA_GWDT_WCS);
> + status = gwdt->soc_reg_ops->reg_read32(cf_base + SBSA_GWDT_WCS);
> if (status & SBSA_GWDT_WCS_WS1) {
> dev_warn(dev, "System reset by WDT.\n");
> wdd->bootstatus |= WDIOF_CARDRESET;
> @@ -317,7 +500,7 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
> * In case there is a pending ws0 interrupt, just ping
> * the watchdog before registering the interrupt routine
> */
> - writel(0, rf_base + SBSA_GWDT_WRR);
> + gwdt->soc_reg_ops->reg_write32(0, rf_base + SBSA_GWDT_WRR);
> if (devm_request_irq(dev, irq, sbsa_gwdt_interrupt, 0,
> pdev->name, gwdt)) {
> action = 0;
> @@ -347,7 +530,28 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
> ret = devm_watchdog_register_device(dev, wdd);
> if (ret)
> return ret;
> -
> + /*
> + * Marvell AC5/X/IM: need to configure the watchdog
> + * HW to trigger reset on WS1 (Watchdog Signal 1):
> + *
> + * 1. Configure the watchdog signal enable (routing)
> + * according to configuration
> + * 2. Unmask the wd_rst input signal to the reset unit
> + */
> + if (marvell) {
> + gwdt->soc_reg_ops->reg_write32(reset, cpu_ctrl_base +
> + SBSA_GWDT_MARVELL_CPU_WD_RST_EN_REG);
> + id = readl(mng_base + SBSA_GWDT_MARVELL_MNG_ID_REG) &
> + SBSA_GWDT_MARVELL_ID_MASK;
> +
> + if (id == SBSA_GWDT_MARVELL_AC5_ID)
> + val = SBSA_GWDT_MARVELL_AC5_RST_UNIT_WD_BIT;
> + else
> + val = SBSA_GWDT_MARVELL_IRONMAN_RST_UNIT_WD_BIT;
> +
> + writel(readl(rst_ctrl_base + SBSA_GWDT_MARVELL_RST_CTRL_REG) & ~val,
> + rst_ctrl_base + SBSA_GWDT_MARVELL_RST_CTRL_REG);
> + }
> dev_info(dev, "Initialized with %ds timeout @ %u Hz, action=%d.%s\n",
> wdd->timeout, gwdt->clk, action,
> status & SBSA_GWDT_WCS_EN ? " [enabled]" : "");
> @@ -383,6 +587,7 @@ static const struct dev_pm_ops sbsa_gwdt_pm_ops = {
>
> static const struct of_device_id sbsa_gwdt_of_match[] = {
> { .compatible = "arm,sbsa-gwdt", },
> + { .compatible = "marvell,ac5-wd", },
> {},
> };
> MODULE_DEVICE_TABLE(of, sbsa_gwdt_of_match);
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5
2023-12-14 15:04 ` [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5 Elad Nachman
2023-12-14 15:16 ` Krzysztof Kozlowski
@ 2023-12-15 1:08 ` kernel test robot
2023-12-15 18:01 ` Rob Herring
` (3 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: kernel test robot @ 2023-12-15 1:08 UTC (permalink / raw)
To: Elad Nachman, wim, linux, robh+dt, krzysztof.kozlowski+dt,
conor+dt, gregory.clement, chris.packham, andrew, fu.wei,
Suravee.Suthikulpanit, al.stone, timur, linux-watchdog,
devicetree, linux-kernel
Cc: oe-kbuild-all, enachman, cyuval
Hi Elad,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on groeck-staging/hwmon-next linus/master v6.7-rc5 next-20231214]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Elad-Nachman/dt-bindings-watchdog-add-Marvell-AC5-watchdog/20231214-230812
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20231214150414.1849058-4-enachman%40marvell.com
patch subject: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20231215/202312150848.3mhXhH3Y-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231215/202312150848.3mhXhH3Y-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312150848.3mhXhH3Y-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/watchdog/sbsa_gwdt.c: In function 'sbsa_gwdt_probe':
>> drivers/watchdog/sbsa_gwdt.c:434:25: warning: assignment to 'void *' from 'resource_size_t' {aka 'long long unsigned int'} makes pointer from integer without a cast [-Wint-conversion]
434 | cf_base = res->start;
| ^
drivers/watchdog/sbsa_gwdt.c:439:25: warning: assignment to 'void *' from 'resource_size_t' {aka 'long long unsigned int'} makes pointer from integer without a cast [-Wint-conversion]
439 | rf_base = res->start;
| ^
drivers/watchdog/sbsa_gwdt.c:444:31: warning: assignment to 'void *' from 'resource_size_t' {aka 'long long unsigned int'} makes pointer from integer without a cast [-Wint-conversion]
444 | cpu_ctrl_base = res->start;
| ^
--
>> drivers/watchdog/sbsa_gwdt.c:141: warning: Function parameter or member 'reg_read32' not described in 'sbsa_gwdt_regs_ops'
>> drivers/watchdog/sbsa_gwdt.c:141: warning: Function parameter or member 'reg_read64' not described in 'sbsa_gwdt_regs_ops'
>> drivers/watchdog/sbsa_gwdt.c:141: warning: Function parameter or member 'reg_write32' not described in 'sbsa_gwdt_regs_ops'
>> drivers/watchdog/sbsa_gwdt.c:141: warning: Function parameter or member 'reg_write64' not described in 'sbsa_gwdt_regs_ops'
>> drivers/watchdog/sbsa_gwdt.c:141: warning: Excess struct member 'reg_read' description in 'sbsa_gwdt_regs_ops'
>> drivers/watchdog/sbsa_gwdt.c:141: warning: Excess struct member 'read_write' description in 'sbsa_gwdt_regs_ops'
>> drivers/watchdog/sbsa_gwdt.c:158: warning: Function parameter or member 'soc_reg_ops' not described in 'sbsa_gwdt'
vim +434 drivers/watchdog/sbsa_gwdt.c
408
409 static int sbsa_gwdt_probe(struct platform_device *pdev)
410 {
411 void __iomem *rf_base, *cf_base;
412 void __iomem *cpu_ctrl_base = NULL, *mng_base = NULL,
413 *rst_ctrl_base = NULL;
414 struct device *dev = &pdev->dev;
415 struct device_node *np = pdev->dev.of_node;
416 struct watchdog_device *wdd;
417 struct sbsa_gwdt *gwdt;
418 struct resource *res;
419 int ret, irq;
420 bool marvell = false;
421 u32 status, id, val;
422
423 gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
424 if (!gwdt)
425 return -ENOMEM;
426 platform_set_drvdata(pdev, gwdt);
427
428 if (of_device_is_compatible(np, "marvell,ac5-wd")) {
429 marvell = true;
430 gwdt->soc_reg_ops = &smc_reg_ops;
431 res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
432 if (IS_ERR(res))
433 return PTR_ERR(res);
> 434 cf_base = res->start;
435
436 res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
437 if (IS_ERR(res))
438 return PTR_ERR(res);
439 rf_base = res->start;
440
441 res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
442 if (IS_ERR(res))
443 return PTR_ERR(res);
444 cpu_ctrl_base = res->start;
445 mng_base = devm_platform_ioremap_resource(pdev, 3);
446 if (IS_ERR(mng_base))
447 return PTR_ERR(mng_base);
448 rst_ctrl_base = devm_platform_ioremap_resource(pdev, 4);
449 if (IS_ERR(rst_ctrl_base))
450 return PTR_ERR(rst_ctrl_base);
451 } else {
452 gwdt->soc_reg_ops = &direct_reg_ops;
453 cf_base = devm_platform_ioremap_resource(pdev, 0);
454 if (IS_ERR(cf_base))
455 return PTR_ERR(cf_base);
456
457 rf_base = devm_platform_ioremap_resource(pdev, 1);
458 if (IS_ERR(rf_base))
459 return PTR_ERR(rf_base);
460 }
461
462 /*
463 * Get the frequency of system counter from the cp15 interface of ARM
464 * Generic timer. We don't need to check it, because if it returns "0",
465 * system would panic in very early stage.
466 */
467 gwdt->clk = arch_timer_get_cntfrq();
468 gwdt->refresh_base = rf_base;
469 gwdt->control_base = cf_base;
470
471 wdd = &gwdt->wdd;
472 wdd->parent = dev;
473 wdd->info = &sbsa_gwdt_info;
474 wdd->ops = &sbsa_gwdt_ops;
475 wdd->min_timeout = 1;
476 wdd->timeout = DEFAULT_TIMEOUT;
477 watchdog_set_drvdata(wdd, gwdt);
478 watchdog_set_nowayout(wdd, nowayout);
479 sbsa_gwdt_get_version(wdd);
480 if (gwdt->version == 0)
481 wdd->max_hw_heartbeat_ms = U32_MAX / gwdt->clk * 1000;
482 else
483 wdd->max_hw_heartbeat_ms = GENMASK_ULL(47, 0) / gwdt->clk * 1000;
484
485 status = gwdt->soc_reg_ops->reg_read32(cf_base + SBSA_GWDT_WCS);
486 if (status & SBSA_GWDT_WCS_WS1) {
487 dev_warn(dev, "System reset by WDT.\n");
488 wdd->bootstatus |= WDIOF_CARDRESET;
489 }
490 if (status & SBSA_GWDT_WCS_EN)
491 set_bit(WDOG_HW_RUNNING, &wdd->status);
492
493 if (action) {
494 irq = platform_get_irq(pdev, 0);
495 if (irq < 0) {
496 action = 0;
497 dev_warn(dev, "unable to get ws0 interrupt.\n");
498 } else {
499 /*
500 * In case there is a pending ws0 interrupt, just ping
501 * the watchdog before registering the interrupt routine
502 */
503 gwdt->soc_reg_ops->reg_write32(0, rf_base + SBSA_GWDT_WRR);
504 if (devm_request_irq(dev, irq, sbsa_gwdt_interrupt, 0,
505 pdev->name, gwdt)) {
506 action = 0;
507 dev_warn(dev, "unable to request IRQ %d.\n",
508 irq);
509 }
510 }
511 if (!action)
512 dev_warn(dev, "falling back to single stage mode.\n");
513 }
514 /*
515 * In the single stage mode, The first signal (WS0) is ignored,
516 * the timeout is (WOR * 2), so the maximum timeout should be doubled.
517 */
518 if (!action)
519 wdd->max_hw_heartbeat_ms *= 2;
520
521 watchdog_init_timeout(wdd, timeout, dev);
522 /*
523 * Update timeout to WOR.
524 * Because of the explicit watchdog refresh mechanism,
525 * it's also a ping, if watchdog is enabled.
526 */
527 sbsa_gwdt_set_timeout(wdd, wdd->timeout);
528
529 watchdog_stop_on_reboot(wdd);
530 ret = devm_watchdog_register_device(dev, wdd);
531 if (ret)
532 return ret;
533 /*
534 * Marvell AC5/X/IM: need to configure the watchdog
535 * HW to trigger reset on WS1 (Watchdog Signal 1):
536 *
537 * 1. Configure the watchdog signal enable (routing)
538 * according to configuration
539 * 2. Unmask the wd_rst input signal to the reset unit
540 */
541 if (marvell) {
542 gwdt->soc_reg_ops->reg_write32(reset, cpu_ctrl_base +
543 SBSA_GWDT_MARVELL_CPU_WD_RST_EN_REG);
544 id = readl(mng_base + SBSA_GWDT_MARVELL_MNG_ID_REG) &
545 SBSA_GWDT_MARVELL_ID_MASK;
546
547 if (id == SBSA_GWDT_MARVELL_AC5_ID)
548 val = SBSA_GWDT_MARVELL_AC5_RST_UNIT_WD_BIT;
549 else
550 val = SBSA_GWDT_MARVELL_IRONMAN_RST_UNIT_WD_BIT;
551
552 writel(readl(rst_ctrl_base + SBSA_GWDT_MARVELL_RST_CTRL_REG) & ~val,
553 rst_ctrl_base + SBSA_GWDT_MARVELL_RST_CTRL_REG);
554 }
555 dev_info(dev, "Initialized with %ds timeout @ %u Hz, action=%d.%s\n",
556 wdd->timeout, gwdt->clk, action,
557 status & SBSA_GWDT_WCS_EN ? " [enabled]" : "");
558
559 return 0;
560 }
561
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5
2023-12-14 15:04 ` [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5 Elad Nachman
2023-12-14 15:16 ` Krzysztof Kozlowski
2023-12-15 1:08 ` kernel test robot
@ 2023-12-15 18:01 ` Rob Herring
2023-12-15 19:12 ` Guenter Roeck
2023-12-15 22:35 ` kernel test robot
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Rob Herring @ 2023-12-15 18:01 UTC (permalink / raw)
To: Elad Nachman
Cc: wim, linux, krzysztof.kozlowski+dt, conor+dt, gregory.clement,
chris.packham, andrew, fu.wei, Suravee.Suthikulpanit, al.stone,
timur, linux-watchdog, devicetree, linux-kernel, cyuval
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5
2023-12-15 18:01 ` Rob Herring
@ 2023-12-15 19:12 ` Guenter Roeck
0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2023-12-15 19:12 UTC (permalink / raw)
To: Rob Herring, Elad Nachman
Cc: wim, krzysztof.kozlowski+dt, conor+dt, gregory.clement,
chris.packham, andrew, fu.wei, Suravee.Suthikulpanit, al.stone,
timur, linux-watchdog, devicetree, linux-kernel, cyuval
On 12/15/23 10:01, Rob Herring wrote:
> 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...
>
... which makes me really unhappy and wonder if it is appropriate
to hack up the existing driver that much. it doesn't look like
Marvell ac5/x really implements SBSA. Given the large number of
device specific deviations, a separate driver may be more appropriate.
Guenter
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5
2023-12-14 15:04 ` [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5 Elad Nachman
` (2 preceding siblings ...)
2023-12-15 18:01 ` Rob Herring
@ 2023-12-15 22:35 ` kernel test robot
2023-12-17 3:08 ` kernel test robot
2023-12-20 14:03 ` Rob Herring
5 siblings, 0 replies; 15+ messages in thread
From: kernel test robot @ 2023-12-15 22:35 UTC (permalink / raw)
To: Elad Nachman, wim, linux, robh+dt, krzysztof.kozlowski+dt,
conor+dt, gregory.clement, chris.packham, andrew, fu.wei,
Suravee.Suthikulpanit, al.stone, timur, linux-watchdog,
devicetree, linux-kernel
Cc: llvm, oe-kbuild-all, enachman, cyuval
Hi Elad,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on groeck-staging/hwmon-next linus/master v6.7-rc5 next-20231215]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Elad-Nachman/dt-bindings-watchdog-add-Marvell-AC5-watchdog/20231214-230812
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20231214150414.1849058-4-enachman%40marvell.com
patch subject: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5
config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20231216/202312160648.h1CrZxtx-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231216/202312160648.h1CrZxtx-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312160648.h1CrZxtx-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/watchdog/sbsa_gwdt.c:434:11: error: incompatible integer to pointer conversion assigning to 'void *' from 'resource_size_t' (aka 'unsigned long long') [-Wint-conversion]
434 | cf_base = res->start;
| ^ ~~~~~~~~~~
drivers/watchdog/sbsa_gwdt.c:439:11: error: incompatible integer to pointer conversion assigning to 'void *' from 'resource_size_t' (aka 'unsigned long long') [-Wint-conversion]
439 | rf_base = res->start;
| ^ ~~~~~~~~~~
drivers/watchdog/sbsa_gwdt.c:444:17: error: incompatible integer to pointer conversion assigning to 'void *' from 'resource_size_t' (aka 'unsigned long long') [-Wint-conversion]
444 | cpu_ctrl_base = res->start;
| ^ ~~~~~~~~~~
3 errors generated.
vim +434 drivers/watchdog/sbsa_gwdt.c
408
409 static int sbsa_gwdt_probe(struct platform_device *pdev)
410 {
411 void __iomem *rf_base, *cf_base;
412 void __iomem *cpu_ctrl_base = NULL, *mng_base = NULL,
413 *rst_ctrl_base = NULL;
414 struct device *dev = &pdev->dev;
415 struct device_node *np = pdev->dev.of_node;
416 struct watchdog_device *wdd;
417 struct sbsa_gwdt *gwdt;
418 struct resource *res;
419 int ret, irq;
420 bool marvell = false;
421 u32 status, id, val;
422
423 gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
424 if (!gwdt)
425 return -ENOMEM;
426 platform_set_drvdata(pdev, gwdt);
427
428 if (of_device_is_compatible(np, "marvell,ac5-wd")) {
429 marvell = true;
430 gwdt->soc_reg_ops = &smc_reg_ops;
431 res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
432 if (IS_ERR(res))
433 return PTR_ERR(res);
> 434 cf_base = res->start;
435
436 res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
437 if (IS_ERR(res))
438 return PTR_ERR(res);
439 rf_base = res->start;
440
441 res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
442 if (IS_ERR(res))
443 return PTR_ERR(res);
444 cpu_ctrl_base = res->start;
445 mng_base = devm_platform_ioremap_resource(pdev, 3);
446 if (IS_ERR(mng_base))
447 return PTR_ERR(mng_base);
448 rst_ctrl_base = devm_platform_ioremap_resource(pdev, 4);
449 if (IS_ERR(rst_ctrl_base))
450 return PTR_ERR(rst_ctrl_base);
451 } else {
452 gwdt->soc_reg_ops = &direct_reg_ops;
453 cf_base = devm_platform_ioremap_resource(pdev, 0);
454 if (IS_ERR(cf_base))
455 return PTR_ERR(cf_base);
456
457 rf_base = devm_platform_ioremap_resource(pdev, 1);
458 if (IS_ERR(rf_base))
459 return PTR_ERR(rf_base);
460 }
461
462 /*
463 * Get the frequency of system counter from the cp15 interface of ARM
464 * Generic timer. We don't need to check it, because if it returns "0",
465 * system would panic in very early stage.
466 */
467 gwdt->clk = arch_timer_get_cntfrq();
468 gwdt->refresh_base = rf_base;
469 gwdt->control_base = cf_base;
470
471 wdd = &gwdt->wdd;
472 wdd->parent = dev;
473 wdd->info = &sbsa_gwdt_info;
474 wdd->ops = &sbsa_gwdt_ops;
475 wdd->min_timeout = 1;
476 wdd->timeout = DEFAULT_TIMEOUT;
477 watchdog_set_drvdata(wdd, gwdt);
478 watchdog_set_nowayout(wdd, nowayout);
479 sbsa_gwdt_get_version(wdd);
480 if (gwdt->version == 0)
481 wdd->max_hw_heartbeat_ms = U32_MAX / gwdt->clk * 1000;
482 else
483 wdd->max_hw_heartbeat_ms = GENMASK_ULL(47, 0) / gwdt->clk * 1000;
484
485 status = gwdt->soc_reg_ops->reg_read32(cf_base + SBSA_GWDT_WCS);
486 if (status & SBSA_GWDT_WCS_WS1) {
487 dev_warn(dev, "System reset by WDT.\n");
488 wdd->bootstatus |= WDIOF_CARDRESET;
489 }
490 if (status & SBSA_GWDT_WCS_EN)
491 set_bit(WDOG_HW_RUNNING, &wdd->status);
492
493 if (action) {
494 irq = platform_get_irq(pdev, 0);
495 if (irq < 0) {
496 action = 0;
497 dev_warn(dev, "unable to get ws0 interrupt.\n");
498 } else {
499 /*
500 * In case there is a pending ws0 interrupt, just ping
501 * the watchdog before registering the interrupt routine
502 */
503 gwdt->soc_reg_ops->reg_write32(0, rf_base + SBSA_GWDT_WRR);
504 if (devm_request_irq(dev, irq, sbsa_gwdt_interrupt, 0,
505 pdev->name, gwdt)) {
506 action = 0;
507 dev_warn(dev, "unable to request IRQ %d.\n",
508 irq);
509 }
510 }
511 if (!action)
512 dev_warn(dev, "falling back to single stage mode.\n");
513 }
514 /*
515 * In the single stage mode, The first signal (WS0) is ignored,
516 * the timeout is (WOR * 2), so the maximum timeout should be doubled.
517 */
518 if (!action)
519 wdd->max_hw_heartbeat_ms *= 2;
520
521 watchdog_init_timeout(wdd, timeout, dev);
522 /*
523 * Update timeout to WOR.
524 * Because of the explicit watchdog refresh mechanism,
525 * it's also a ping, if watchdog is enabled.
526 */
527 sbsa_gwdt_set_timeout(wdd, wdd->timeout);
528
529 watchdog_stop_on_reboot(wdd);
530 ret = devm_watchdog_register_device(dev, wdd);
531 if (ret)
532 return ret;
533 /*
534 * Marvell AC5/X/IM: need to configure the watchdog
535 * HW to trigger reset on WS1 (Watchdog Signal 1):
536 *
537 * 1. Configure the watchdog signal enable (routing)
538 * according to configuration
539 * 2. Unmask the wd_rst input signal to the reset unit
540 */
541 if (marvell) {
542 gwdt->soc_reg_ops->reg_write32(reset, cpu_ctrl_base +
543 SBSA_GWDT_MARVELL_CPU_WD_RST_EN_REG);
544 id = readl(mng_base + SBSA_GWDT_MARVELL_MNG_ID_REG) &
545 SBSA_GWDT_MARVELL_ID_MASK;
546
547 if (id == SBSA_GWDT_MARVELL_AC5_ID)
548 val = SBSA_GWDT_MARVELL_AC5_RST_UNIT_WD_BIT;
549 else
550 val = SBSA_GWDT_MARVELL_IRONMAN_RST_UNIT_WD_BIT;
551
552 writel(readl(rst_ctrl_base + SBSA_GWDT_MARVELL_RST_CTRL_REG) & ~val,
553 rst_ctrl_base + SBSA_GWDT_MARVELL_RST_CTRL_REG);
554 }
555 dev_info(dev, "Initialized with %ds timeout @ %u Hz, action=%d.%s\n",
556 wdd->timeout, gwdt->clk, action,
557 status & SBSA_GWDT_WCS_EN ? " [enabled]" : "");
558
559 return 0;
560 }
561
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5
2023-12-14 15:04 ` [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5 Elad Nachman
` (3 preceding siblings ...)
2023-12-15 22:35 ` kernel test robot
@ 2023-12-17 3:08 ` kernel test robot
2023-12-20 14:03 ` Rob Herring
5 siblings, 0 replies; 15+ messages in thread
From: kernel test robot @ 2023-12-17 3:08 UTC (permalink / raw)
To: Elad Nachman, wim, linux, robh+dt, krzysztof.kozlowski+dt,
conor+dt, gregory.clement, chris.packham, andrew, fu.wei,
Suravee.Suthikulpanit, al.stone, timur, linux-watchdog,
devicetree, linux-kernel
Cc: oe-kbuild-all, enachman, cyuval
Hi Elad,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on groeck-staging/hwmon-next linus/master v6.7-rc5 next-20231215]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Elad-Nachman/dt-bindings-watchdog-add-Marvell-AC5-watchdog/20231214-230812
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20231214150414.1849058-4-enachman%40marvell.com
patch subject: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20231217/202312171047.mHNijCmt-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231217/202312171047.mHNijCmt-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312171047.mHNijCmt-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> scripts/kernel-doc: drivers/watchdog/sbsa_gwdt.c:141: warning: Function parameter or struct member 'reg_read32' not described in 'sbsa_gwdt_regs_ops'
>> scripts/kernel-doc: drivers/watchdog/sbsa_gwdt.c:141: warning: Function parameter or struct member 'reg_read64' not described in 'sbsa_gwdt_regs_ops'
>> scripts/kernel-doc: drivers/watchdog/sbsa_gwdt.c:141: warning: Function parameter or struct member 'reg_write32' not described in 'sbsa_gwdt_regs_ops'
>> scripts/kernel-doc: drivers/watchdog/sbsa_gwdt.c:141: warning: Function parameter or struct member 'reg_write64' not described in 'sbsa_gwdt_regs_ops'
>> scripts/kernel-doc: drivers/watchdog/sbsa_gwdt.c:141: warning: Excess struct member 'reg_read' description in 'sbsa_gwdt_regs_ops'
>> scripts/kernel-doc: drivers/watchdog/sbsa_gwdt.c:141: warning: Excess struct member 'read_write' description in 'sbsa_gwdt_regs_ops'
>> scripts/kernel-doc: drivers/watchdog/sbsa_gwdt.c:141: warning: Excess struct member 'read_write' description in 'sbsa_gwdt_regs_ops'
>> scripts/kernel-doc: drivers/watchdog/sbsa_gwdt.c:141: warning: Excess struct member 'reg_read' description in 'sbsa_gwdt_regs_ops'
>> scripts/kernel-doc: drivers/watchdog/sbsa_gwdt.c:158: warning: Function parameter or struct member 'soc_reg_ops' not described in 'sbsa_gwdt'
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5
2023-12-14 15:04 ` [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5 Elad Nachman
` (4 preceding siblings ...)
2023-12-17 3:08 ` kernel test robot
@ 2023-12-20 14:03 ` Rob Herring
5 siblings, 0 replies; 15+ messages in thread
From: Rob Herring @ 2023-12-20 14:03 UTC (permalink / raw)
To: Elad Nachman
Cc: wim, linux, krzysztof.kozlowski+dt, conor+dt, gregory.clement,
chris.packham, andrew, fu.wei, Suravee.Suthikulpanit, al.stone,
timur, linux-watchdog, devicetree, linux-kernel, cyuval
On Thu, Dec 14, 2023 at 9:05 AM Elad Nachman <enachman@marvell.com> 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 ++++++++++++++++++++++++++++++++---
> 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
One more thing, these IDs are part of the Arm arch range and can't be
used. You should be using the SIP range AIUI.
Perhaps you should look at arm_smc_wdt.c and make that work on your
system. Despite the name, my understanding is it is a ChromeOS defined
watchdog, not an Arm (Ltd) one.
Rob
^ permalink raw reply [flat|nested] 15+ messages in thread