Add binding documentation for the Marvell GTI system
watchdog driver.
Signed-off-by: Bharat Bhushan <[email protected]>
---
v7:
- Corrected compatible to have soc name
- Converted marvell,wdt-timer-index to optional
.../watchdog/marvell,octeontx2-wdt.yaml | 80 +++++++++++++++++++
1 file changed, 80 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml
diff --git a/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml
new file mode 100644
index 000000000000..72951b10f1f3
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/marvell,octeontx2-wdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell Global Timer (GTI) system watchdog
+
+allOf:
+ - $ref: watchdog.yaml#
+
+maintainers:
+ - Bharat Bhushan <[email protected]>
+
+properties:
+ compatible:
+ oneOf:
+ - const: marvell,octeontx2-wdt
+ - items:
+ - enum:
+ - marvell,octeontx2-95xx-wdt
+ - marvell,octeontx2-96xx-wdt
+ - marvell,octeontx2-98xx-wdt
+ - const: marvell,octeontx2-wdt
+ - const: marvell,cn10k-wdt
+ - items:
+ - enum:
+ - marvell,cn10kx-wdt
+ - marvell,cnf10kx-wdt
+ - const: marvell,cn10k-wdt
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ minItems: 1
+
+ clock-names:
+ minItems: 1
+
+ marvell,wdt-timer-index:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ minimum: 0
+ maximum: 63
+ description:
+ An SoC have many timers (up to 64), firmware can reserve one or more timer
+ for some other use case and configures one of the global timer as watchdog
+ timer. Firmware will update this field with the timer number configured
+ as watchdog timer.
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - clock-names
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ watchdog@802000040000 {
+ compatible = "marvell,octeontx2-wdt";
+ reg = <0x00008020 0x00040000 0x00000000 0x00020000>;
+ interrupts = <GIC_SPI 38 IRQ_TYPE_EDGE_RISING>;
+ clocks = <&sclk>;
+ clock-names = "ref_clk";
+ marvell,wdt-timer-index = <63>;
+ };
+ };
+
+...
--
2.17.1
This patch add support for Marvell GTI watchdog. Global timer
unit (GTI) support hardware watchdog timer. Software programs
watchdog timer to generate interrupt on first timeout, second
timeout is configured to be ignored and system reboots on
third timeout.
Signed-off-by: Bharat Bhushan <[email protected]>
---
v7:
- Converted marvell,wdt-timer-index to optional
- Corrected compatible to have soc names
drivers/watchdog/Kconfig | 13 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/marvell_gti_wdt.c | 342 +++++++++++++++++++++++++++++
3 files changed, 356 insertions(+)
create mode 100644 drivers/watchdog/marvell_gti_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index f22138709bf5..0351982b7abf 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1779,6 +1779,19 @@ config OCTEON_WDT
from the first interrupt, it is then only poked when the
device is written.
+config MARVELL_GTI_WDT
+ tristate "Marvell GTI Watchdog driver"
+ depends on ARCH_THUNDER || COMPILE_TEST
+ default y
+ select WATCHDOG_CORE
+ help
+ Marvell GTI hardware supports watchdog timer. First timeout
+ works as watchdog pretimeout and installed interrupt handler
+ will be called on first timeout. Hardware can generate interrupt
+ to SCP on second timeout but it is not enabled, So second
+ timeout is ignored. If device poke does not happen then system
+ will reboot on third timeout.
+
config BCM2835_WDT
tristate "Broadcom BCM2835 hardware watchdog"
depends on ARCH_BCM2835 || (OF && COMPILE_TEST)
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index b4c4ccf2d703..a164cd161ef3 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -98,6 +98,7 @@ obj-$(CONFIG_VISCONTI_WATCHDOG) += visconti_wdt.o
obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
obj-$(CONFIG_APPLE_WATCHDOG) += apple_wdt.o
obj-$(CONFIG_SUNPLUS_WATCHDOG) += sunplus_wdt.o
+obj-$(CONFIG_MARVELL_GTI_WDT) += marvell_gti_wdt.o
# X86 (i386 + ia64 + x86_64) Architecture
obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
diff --git a/drivers/watchdog/marvell_gti_wdt.c b/drivers/watchdog/marvell_gti_wdt.c
new file mode 100644
index 000000000000..43f368e6fb01
--- /dev/null
+++ b/drivers/watchdog/marvell_gti_wdt.c
@@ -0,0 +1,342 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Marvell GTI Watchdog driver
+ *
+ * Copyright (C) 2023 Marvell.
+ */
+
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+#include <linux/clk.h>
+
+/*
+ * Hardware supports following mode of operation:
+ * 1) Interrupt Only:
+ * This will generate the interrupt to arm core whenever timeout happens.
+ *
+ * 2) Interrupt + del3t (Interrupt to firmware (SCP processor)).
+ * This will generate interrupt to arm core on 1st timeout happens
+ * This will generate interrupt to SCP processor on 2nd timeout happens
+ *
+ * 3) Interrupt + Interrupt to SCP processor (called delt3t) + reboot.
+ * This will generate interrupt to arm core on 1st timeout happens
+ * Will generate interrupt to SCP processor on 2nd timeout happens,
+ * if interrupt is configured.
+ * Reboot on 3rd timeout.
+ *
+ * Driver will use hardware in mode-3 above so that system can reboot in case
+ * a hardware hang. Also h/w is configured not to generate SCP interrupt, so
+ * effectively 2nd timeout is ignored within hardware.
+ *
+ * First timeout is effectively watchdog pretimeout.
+ */
+
+/* GTI CWD Watchdog (GTI_CWD_WDOG) Register */
+#define GTI_CWD_WDOG(reg_offset) (0x8 * reg_offset)
+#define GTI_CWD_WDOG_MODE_INT_DEL3T_RST 0x3
+#define GTI_CWD_WDOG_MODE_MASK GENMASK_ULL(1, 0)
+#define GTI_CWD_WDOG_LEN_SHIFT 4
+#define GTI_CWD_WDOG_LEN_MASK GENMASK_ULL(19, 4)
+#define GTI_CWD_WDOG_CNT_SHIFT 20
+#define GTI_CWD_WDOG_CNT_MASK GENMASK_ULL(43, 20)
+
+/* GTI CWD Watchdog Interrupt (GTI_CWD_INT) Register */
+#define GTI_CWD_INT 0x200
+#define GTI_CWD_INT_PENDING_STATUS(bit) (1 << bit)
+
+/* GTI CWD Watchdog Interrupt Enable Clear (GTI_CWD_INT_ENA_CLR) Register */
+#define GTI_CWD_INT_ENA_CLR 0x210
+#define GTI_CWD_INT_ENA_CLR_VAL(bit) (1 << bit)
+
+/* GTI CWD Watchdog Interrupt Enable Set (GTI_CWD_INT_ENA_SET) Register */
+#define GTI_CWD_INT_ENA_SET 0x218
+#define GTI_CWD_INT_ENA_SET_VAL(bit) (1 << bit)
+
+/* GTI CWD Watchdog Poke (GTI_CWD_POKE) Registers */
+#define GTI_CWD_POKE(reg_offset) (0x10000 + 0x8 * reg_offset)
+#define GTI_CWD_POKE_VAL 1
+
+struct gti_match_data {
+ u32 gti_num_timers;
+};
+
+static const struct gti_match_data match_data_octeontx2 = {
+ .gti_num_timers = 54,
+};
+
+static const struct gti_match_data match_data_cn10k = {
+ .gti_num_timers = 64,
+};
+
+struct gti_wdt_priv {
+ struct watchdog_device wdev;
+ void __iomem *base;
+ u32 clock_freq;
+ struct clk *sclk;
+ /* wdt_timer_idx used for timer to be used for system watchdog */
+ u32 wdt_timer_idx;
+ const struct gti_match_data *data;
+};
+
+static irqreturn_t gti_wdt_interrupt(int irq, void *data)
+{
+ struct watchdog_device *wdev = data;
+ struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+ /* Clear Interrupt Pending Status */
+ writeq(GTI_CWD_INT_PENDING_STATUS(priv->wdt_timer_idx),
+ priv->base + GTI_CWD_INT);
+
+ watchdog_notify_pretimeout(wdev);
+
+ return IRQ_HANDLED;
+}
+
+static int gti_wdt_ping(struct watchdog_device *wdev)
+{
+ struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+ writeq(GTI_CWD_POKE_VAL,
+ priv->base + GTI_CWD_POKE(priv->wdt_timer_idx));
+
+ return 0;
+}
+
+static int gti_wdt_start(struct watchdog_device *wdev)
+{
+ struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
+ u64 regval;
+
+ if (!wdev->pretimeout)
+ return -EINVAL;
+
+ set_bit(WDOG_HW_RUNNING, &wdev->status);
+
+ /* Clear any pending interrupt */
+ writeq(GTI_CWD_INT_PENDING_STATUS(priv->wdt_timer_idx),
+ priv->base + GTI_CWD_INT);
+
+ /* Enable Interrupt */
+ writeq(GTI_CWD_INT_ENA_SET_VAL(priv->wdt_timer_idx),
+ priv->base + GTI_CWD_INT_ENA_SET);
+
+ /* Set (Interrupt + SCP interrupt (DEL3T) + core domain reset) Mode */
+ regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
+ regval |= GTI_CWD_WDOG_MODE_INT_DEL3T_RST;
+ writeq(regval, priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
+
+ return 0;
+}
+
+static int gti_wdt_stop(struct watchdog_device *wdev)
+{
+ struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
+ u64 regval;
+
+ /* Disable Interrupt */
+ writeq(GTI_CWD_INT_ENA_CLR_VAL(priv->wdt_timer_idx),
+ priv->base + GTI_CWD_INT_ENA_CLR);
+
+ /* Set GTI_CWD_WDOG.Mode = 0 to stop the timer */
+ regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
+ regval &= ~GTI_CWD_WDOG_MODE_MASK;
+ writeq(regval, priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
+
+ return 0;
+}
+
+static int gti_wdt_settimeout(struct watchdog_device *wdev,
+ unsigned int timeout)
+{
+ struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
+ u64 timeout_wdog, regval;
+
+ /* Update new timeout */
+ wdev->timeout = timeout;
+
+ /* Pretimeout is 1/3 of timeout */
+ wdev->pretimeout = timeout / 3;
+
+ /* Get clock cycles from pretimeout */
+ timeout_wdog = (u64)priv->clock_freq * wdev->pretimeout;
+
+ /* Watchdog counts in 1024 cycle steps */
+ timeout_wdog = timeout_wdog >> 10;
+
+ /* GTI_CWD_WDOG.CNT: reload counter is 16-bit */
+ timeout_wdog = (timeout_wdog + 0xff) >> 8;
+ if (timeout_wdog >= 0x10000)
+ timeout_wdog = 0xffff;
+
+ /*
+ * GTI_CWD_WDOG.LEN is 24bit, lower 8-bits should be zero and
+ * upper 16-bits are same as GTI_CWD_WDOG.CNT
+ */
+ regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
+ regval &= GTI_CWD_WDOG_MODE_MASK;
+ regval |= (timeout_wdog << (GTI_CWD_WDOG_CNT_SHIFT + 8)) |
+ (timeout_wdog << GTI_CWD_WDOG_LEN_SHIFT);
+ writeq(regval, priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
+
+ return 0;
+}
+
+static int gti_wdt_set_pretimeout(struct watchdog_device *wdev,
+ unsigned int timeout)
+{
+ struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
+ struct watchdog_device *wdog_dev = &priv->wdev;
+
+ /* pretimeout should 1/3 of max_timeout */
+ if (timeout * 3 <= wdog_dev->max_timeout)
+ return gti_wdt_settimeout(wdev, timeout * 3);
+
+ return -EINVAL;
+}
+
+static void gti_clk_disable_unprepare(void *data)
+{
+ clk_disable_unprepare(data);
+}
+
+static int gti_wdt_get_cntfrq(struct platform_device *pdev,
+ struct gti_wdt_priv *priv)
+{
+ int err;
+
+ priv->sclk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(priv->sclk))
+ return PTR_ERR(priv->sclk);
+
+ err = clk_prepare_enable(priv->sclk);
+ if (err)
+ return err;
+
+ err = devm_add_action_or_reset(&pdev->dev,
+ gti_clk_disable_unprepare, priv->sclk);
+ if (err)
+ return err;
+
+ priv->clock_freq = clk_get_rate(priv->sclk);
+ if (!priv->clock_freq)
+ return -EINVAL;
+
+ return 0;
+}
+
+static const struct watchdog_info gti_wdt_ident = {
+ .identity = "Marvell GTI watchdog",
+ .options = WDIOF_SETTIMEOUT | WDIOF_PRETIMEOUT | WDIOF_KEEPALIVEPING |
+ WDIOF_MAGICCLOSE | WDIOF_CARDRESET,
+};
+
+static const struct watchdog_ops gti_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = gti_wdt_start,
+ .stop = gti_wdt_stop,
+ .ping = gti_wdt_ping,
+ .set_timeout = gti_wdt_settimeout,
+ .set_pretimeout = gti_wdt_set_pretimeout,
+};
+
+static int gti_wdt_probe(struct platform_device *pdev)
+{
+ struct gti_wdt_priv *priv;
+ struct device *dev = &pdev->dev;
+ struct watchdog_device *wdog_dev;
+ u64 max_pretimeout;
+ u32 wdt_idx;
+ int irq;
+ int err;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(priv->base))
+ return dev_err_probe(&pdev->dev, PTR_ERR(priv->base),
+ "reg property not valid/found\n");
+
+ err = gti_wdt_get_cntfrq(pdev, priv);
+ if (err)
+ return dev_err_probe(&pdev->dev, err,
+ "GTI clock frequency not valid/found");
+
+ priv->data = of_device_get_match_data(dev);
+
+ /* default use last timer for watchdog */
+ priv->wdt_timer_idx = priv->data->gti_num_timers - 1;
+
+ err = of_property_read_u32(dev->of_node, "marvell,wdt-timer-index",
+ &wdt_idx);
+ if (!err) {
+ if (wdt_idx >= priv->data->gti_num_timers)
+ return dev_err_probe(&pdev->dev, err,
+ "GTI wdog timer index not valid");
+
+ priv->wdt_timer_idx = wdt_idx;
+ }
+
+ wdog_dev = &priv->wdev;
+ wdog_dev->info = >i_wdt_ident,
+ wdog_dev->ops = >i_wdt_ops,
+ wdog_dev->parent = dev;
+ /*
+ * Watchdog counter is 24 bit where lower 8 bits are zeros
+ * This counter decrements every 1024 clock cycles.
+ */
+ max_pretimeout = (GTI_CWD_WDOG_CNT_MASK >> GTI_CWD_WDOG_CNT_SHIFT);
+ max_pretimeout &= ~0xFFUL;
+ max_pretimeout = (max_pretimeout * 1024) / priv->clock_freq;
+ wdog_dev->pretimeout = max_pretimeout;
+
+ /* Maximum timeout is 3 times the pretimeout */
+ wdog_dev->max_timeout = max_pretimeout * 3;
+ /* Minimum first timeout (pretimeout) is 1, so min_timeout as 3 */
+ wdog_dev->min_timeout = 3;
+ wdog_dev->timeout = wdog_dev->pretimeout;
+
+ watchdog_set_drvdata(wdog_dev, priv);
+ platform_set_drvdata(pdev, priv);
+ gti_wdt_settimeout(wdog_dev, wdog_dev->timeout);
+ watchdog_stop_on_reboot(wdog_dev);
+ watchdog_stop_on_unregister(wdog_dev);
+
+ err = devm_watchdog_register_device(dev, wdog_dev);
+ if (err)
+ return err;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return dev_err_probe(&pdev->dev, irq, "IRQ resource not found\n");
+
+ err = devm_request_irq(dev, irq, gti_wdt_interrupt, 0,
+ pdev->name, &priv->wdev);
+ if (err)
+ return dev_err_probe(dev, err, "Failed to register interrupt handler\n");
+
+ dev_info(dev, "Watchdog enabled (timeout=%d sec)\n", wdog_dev->timeout);
+ return 0;
+}
+
+static const struct of_device_id gti_wdt_of_match[] = {
+ { .compatible = "marvell,octeontx2-wdt", .data = &match_data_octeontx2},
+ { .compatible = "marvell,cn10k-wdt", .data = &match_data_cn10k},
+ { },
+};
+MODULE_DEVICE_TABLE(of, gti_wdt_of_match);
+
+static struct platform_driver gti_wdt_driver = {
+ .driver = {
+ .name = "gti-wdt",
+ .of_match_table = gti_wdt_of_match,
+ },
+ .probe = gti_wdt_probe,
+};
+module_platform_driver(gti_wdt_driver);
+
+MODULE_AUTHOR("Bharat Bhushan <[email protected]>");
+MODULE_DESCRIPTION("Marvell GTI watchdog driver");
--
2.17.1
Hi Bharat,
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.4-rc1 next-20230508]
[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/Bharat-Bhushan/Watchdog-Add-marvell-GTI-watchdog-driver/20230508-211645
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230508131515.19403-2-bbhushan2%40marvell.com
patch subject: [PATCH 2/2 v7] Watchdog: Add marvell GTI watchdog driver
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230509/[email protected]/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/0b1fbcf987da442c837b205256c6400adf6d298e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Bharat-Bhushan/Watchdog-Add-marvell-GTI-watchdog-driver/20230508-211645
git checkout 0b1fbcf987da442c837b205256c6400adf6d298e
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
drivers/watchdog/marvell_gti_wdt.c: In function 'gti_wdt_interrupt':
>> drivers/watchdog/marvell_gti_wdt.c:89:9: error: implicit declaration of function 'writeq' [-Werror=implicit-function-declaration]
89 | writeq(GTI_CWD_INT_PENDING_STATUS(priv->wdt_timer_idx),
| ^~~~~~
drivers/watchdog/marvell_gti_wdt.c: In function 'gti_wdt_start':
>> drivers/watchdog/marvell_gti_wdt.c:126:18: error: implicit declaration of function 'readq' [-Werror=implicit-function-declaration]
126 | regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
| ^~~~~
cc1: some warnings being treated as errors
vim +/writeq +89 drivers/watchdog/marvell_gti_wdt.c
82
83 static irqreturn_t gti_wdt_interrupt(int irq, void *data)
84 {
85 struct watchdog_device *wdev = data;
86 struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
87
88 /* Clear Interrupt Pending Status */
> 89 writeq(GTI_CWD_INT_PENDING_STATUS(priv->wdt_timer_idx),
90 priv->base + GTI_CWD_INT);
91
92 watchdog_notify_pretimeout(wdev);
93
94 return IRQ_HANDLED;
95 }
96
97 static int gti_wdt_ping(struct watchdog_device *wdev)
98 {
99 struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
100
101 writeq(GTI_CWD_POKE_VAL,
102 priv->base + GTI_CWD_POKE(priv->wdt_timer_idx));
103
104 return 0;
105 }
106
107 static int gti_wdt_start(struct watchdog_device *wdev)
108 {
109 struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
110 u64 regval;
111
112 if (!wdev->pretimeout)
113 return -EINVAL;
114
115 set_bit(WDOG_HW_RUNNING, &wdev->status);
116
117 /* Clear any pending interrupt */
118 writeq(GTI_CWD_INT_PENDING_STATUS(priv->wdt_timer_idx),
119 priv->base + GTI_CWD_INT);
120
121 /* Enable Interrupt */
122 writeq(GTI_CWD_INT_ENA_SET_VAL(priv->wdt_timer_idx),
123 priv->base + GTI_CWD_INT_ENA_SET);
124
125 /* Set (Interrupt + SCP interrupt (DEL3T) + core domain reset) Mode */
> 126 regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
127 regval |= GTI_CWD_WDOG_MODE_INT_DEL3T_RST;
128 writeq(regval, priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
129
130 return 0;
131 }
132
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On 08/05/2023 15:15, Bharat Bhushan wrote:
> Add binding documentation for the Marvell GTI system
> watchdog driver.
>
> Signed-off-by: Bharat Bhushan <[email protected]>
> ---
> v7:
> - Corrected compatible to have soc name
> - Converted marvell,wdt-timer-index to optional
>
> .../watchdog/marvell,octeontx2-wdt.yaml | 80 +++++++++++++++++++
> 1 file changed, 80 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml
>
> diff --git a/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml
> new file mode 100644
> index 000000000000..72951b10f1f3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/marvell,octeontx2-wdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell Global Timer (GTI) system watchdog
> +
> +allOf:
> + - $ref: watchdog.yaml#
Put allOf after maintainers:.
> +
> +maintainers:
> + - Bharat Bhushan <[email protected]>
> +
> +properties:
> + compatible:
> + oneOf:
> + - const: marvell,octeontx2-wdt
Why is this alone? Judging by the enum below, octeontx2 is not specific.
> + - items:
> + - enum:
> + - marvell,octeontx2-95xx-wdt
> + - marvell,octeontx2-96xx-wdt
> + - marvell,octeontx2-98xx-wdt
We don't allow wildcards in general
> + - const: marvell,octeontx2-wdt
> + - const: marvell,cn10k-wdt
Same question - why is this alone?
Second question - it should be rather part of enum with the first one if
accepted.
> + - items:
> + - enum:
> + - marvell,cn10kx-wdt
> + - marvell,cnf10kx-wdt
> + - const: marvell,cn10k-wdt
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + clocks:
> + minItems: 1
maxItems instead. You see it is different than above properties?
> +
> + clock-names:
> + minItems: 1
Need to define names.
Best regards,
Krzysztof
> -----Original Message-----
> From: Krzysztof Kozlowski <[email protected]>
> Sent: Tuesday, May 9, 2023 12:23 PM
> To: Bharat Bhushan <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; Sunil Kovvuri Goutham <[email protected]>
> Subject: [EXT] Re: [PATCH 1/2 v7] dt-bindings: watchdog: marvell GTI system
> watchdog driver
>
> External Email
>
> ----------------------------------------------------------------------
> On 08/05/2023 15:15, Bharat Bhushan wrote:
> > Add binding documentation for the Marvell GTI system watchdog driver.
> >
> > Signed-off-by: Bharat Bhushan <[email protected]>
> > ---
> > v7:
> > - Corrected compatible to have soc name
> > - Converted marvell,wdt-timer-index to optional
> >
> > .../watchdog/marvell,octeontx2-wdt.yaml | 80 +++++++++++++++++++
> > 1 file changed, 80 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yam
> > l
> > b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yam
> > l
> > new file mode 100644
> > index 000000000000..72951b10f1f3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt
> > +++ .yaml
> > @@ -0,0 +1,80 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://urldefense.proofpoint.com/v2/url?u=http-3A__devicetree.org_sc
> > +hemas_watchdog_marvell-2Cocteontx2-2Dwdt.yaml-
> 23&d=DwICaQ&c=nKjWec2b6
> >
> +R0mOyPaz7xtfQ&r=PAAlWswPe7d8gHlGbCLmy2YezyK7O3Hv_t2heGnouBw&m
> =4PmICMz
> >
> +jaooxZnBaJszNihWBPjS99OeVsnJTSsOxNyAhC83AsKQuNVano9_YP2Oj&s=s1E00
> h2dU
> > +roSz4xdrUQMO3_19ren2IoeshKFZjDWKrw&e=
> > +$schema:
> > +https://urldefense.proofpoint.com/v2/url?u=http-3A__devicetree.org_me
> > +ta-2Dschemas_core.yaml-
> 23&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=PAAlWsw
> >
> +Pe7d8gHlGbCLmy2YezyK7O3Hv_t2heGnouBw&m=4PmICMzjaooxZnBaJszNihW
> BPjS99O
> >
> +eVsnJTSsOxNyAhC83AsKQuNVano9_YP2Oj&s=MFuqeS8S00OEgao3hV2RkzNGX
> 2p0Jycp
> > +nZo_sLygcbk&e=
> > +
> > +title: Marvell Global Timer (GTI) system watchdog
> > +
> > +allOf:
> > + - $ref: watchdog.yaml#
>
> Put allOf after maintainers:.
Ok
>
> > +
> > +maintainers:
> > + - Bharat Bhushan <[email protected]>
> > +
> > +properties:
> > + compatible:
> > + oneOf:
> > + - const: marvell,octeontx2-wdt
>
> Why is this alone? Judging by the enum below, octeontx2 is not specific.
>
> > + - items:
> > + - enum:
> > + - marvell,octeontx2-95xx-wdt
> > + - marvell,octeontx2-96xx-wdt
> > + - marvell,octeontx2-98xx-wdt
>
> We don't allow wildcards in general
Marvell have octeontx2 series of processor which have watchdog timer.
In 95xx,98xx,96xx are the processors in octeontx2 series of processor. So octeontx2-95xx is on soc, octeontx2-96xx is another and so on.
>
> > + - const: marvell,octeontx2-wdt
> > + - const: marvell,cn10k-wdt
>
> Same question - why is this alone?
Same here, Marvell have cn10k series of processors and cn10kx and cnf10kx are the processor in this series.
One of the difference between octeontx2 and cn10k series processor is number of timers available. Which within the available set of timers one of the timer is programmed to be watchdog timer.
Can you please propose how you want these compatible to be defined?
>
> Second question - it should be rather part of enum with the first one if accepted.
>
> > + - items:
> > + - enum:
> > + - marvell,cn10kx-wdt
> > + - marvell,cnf10kx-wdt
> > + - const: marvell,cn10k-wdt
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + clocks:
> > + minItems: 1
>
> maxItems instead. You see it is different than above properties?
Okay,
>
> > +
> > + clock-names:
> > + minItems: 1
>
> Need to define names.
Okay,
Thanks
-Bharat
>
> Best regards,
> Krzysztof
On 09/05/2023 09:26, Bharat Bhushan wrote:
>>> +properties:
>>> + compatible:
>>> + oneOf:
>>> + - const: marvell,octeontx2-wdt
>>
>> Why is this alone? Judging by the enum below, octeontx2 is not specific.
>>
>>> + - items:
>>> + - enum:
>>> + - marvell,octeontx2-95xx-wdt
>>> + - marvell,octeontx2-96xx-wdt
>>> + - marvell,octeontx2-98xx-wdt
>>
>> We don't allow wildcards in general
>
> Marvell have octeontx2 series of processor which have watchdog timer.
> In 95xx,98xx,96xx are the processors in octeontx2 series of processor. So octeontx2-95xx is on soc, octeontx2-96xx is another and so on.
No, 95xx is not a processor. Otherwise please point me to exact product
datasheet. Hint: I checked it.
>
>>
>>> + - const: marvell,octeontx2-wdt
>>> + - const: marvell,cn10k-wdt
>>
>> Same question - why is this alone?
> Same here, Marvell have cn10k series of processors and cn10kx and cnf10kx are the processor in this series.
I don't understand how does it explain my concern. This is alone because
there are series of processors? How is that related?
>
> One of the difference between octeontx2 and cn10k series processor is number of timers available. Which within the available set of timers one of the timer is programmed to be watchdog timer.
Wrap your replies. It's difficult to read.
>
> Can you please propose how you want these compatible to be defined?
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml#L19
Best regards,
Krzysztof
> -----Original Message-----
> From: Krzysztof Kozlowski <[email protected]>
> Sent: Tuesday, May 9, 2023 1:38 PM
> To: Bharat Bhushan <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; Sunil Kovvuri Goutham <[email protected]>
> Subject: Re: [EXT] Re: [PATCH 1/2 v7] dt-bindings: watchdog: marvell GTI system
> watchdog driver
>
> On 09/05/2023 09:26, Bharat Bhushan wrote:
>
>
> >>> +properties:
> >>> + compatible:
> >>> + oneOf:
> >>> + - const: marvell,octeontx2-wdt
> >>
> >> Why is this alone? Judging by the enum below, octeontx2 is not specific.
> >>
> >>> + - items:
> >>> + - enum:
> >>> + - marvell,octeontx2-95xx-wdt
> >>> + - marvell,octeontx2-96xx-wdt
> >>> + - marvell,octeontx2-98xx-wdt
> >>
> >> We don't allow wildcards in general
> >
> > Marvell have octeontx2 series of processor which have watchdog timer.
> > In 95xx,98xx,96xx are the processors in octeontx2 series of processor. So
> octeontx2-95xx is on soc, octeontx2-96xx is another and so on.
>
> No, 95xx is not a processor. Otherwise please point me to exact product
> datasheet. Hint: I checked it.
Looks like 95xx data sheet is not public, will remove in that case.
>
> >
> >>
> >>> + - const: marvell,octeontx2-wdt
> >>> + - const: marvell,cn10k-wdt
> >>
> >> Same question - why is this alone?
> > Same here, Marvell have cn10k series of processors and cn10kx and cnf10kx are
> the processor in this series.
>
> I don't understand how does it explain my concern. This is alone because there
> are series of processors? How is that related?
Tried to make it look like other drivers. Let's keep it simple, we want to enable this only for below ones
properties:
compatible:
enum:
- marvell,cn10k-wdt
- marvell,octeontx2-wdt
Are you good with that?
Thanks
-Bharat
>
> >
> > One of the difference between octeontx2 and cn10k series processor is
> number of timers available. Which within the available set of timers one of the
> timer is programmed to be watchdog timer.
>
> Wrap your replies. It's difficult to read.
>
> >
> > Can you please propose how you want these compatible to be defined?
>
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__elixir.bootlin.com_linux_v6.4-
> 2Drc1_source_Documentation_devicetree_bindings_soc_qcom_qcom-
> 2Ceud.yaml-
> 23L19&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=PAAlWswPe7d8gHlGbCLmy
> 2YezyK7O3Hv_t2heGnouBw&m=sbDC9A17UO1l_M7xW5546TQhAMxoejy6M_sv
> PitOn_9sOxb0ru3H7X9eEW00Gqna&s=tQrCoVSHNEOd9CkEeu6leJbmP0rtbL5Vd
> WlE9GQ-GTI&e=
>
> Best regards,
> Krzysztof
Hi Bharat,
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.4-rc1 next-20230509]
[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/Bharat-Bhushan/Watchdog-Add-marvell-GTI-watchdog-driver/20230508-211645
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230508131515.19403-2-bbhushan2%40marvell.com
patch subject: [PATCH 2/2 v7] Watchdog: Add marvell GTI watchdog driver
config: hexagon-randconfig-r015-20230508 (https://download.01.org/0day-ci/archive/20230509/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project b0fb98227c90adf2536c9ad644a74d5e92961111)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/0b1fbcf987da442c837b205256c6400adf6d298e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Bharat-Bhushan/Watchdog-Add-marvell-GTI-watchdog-driver/20230508-211645
git checkout 0b1fbcf987da442c837b205256c6400adf6d298e
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/watchdog/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
In file included from drivers/watchdog/marvell_gti_wdt.c:8:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from drivers/watchdog/marvell_gti_wdt.c:8:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from drivers/watchdog/marvell_gti_wdt.c:8:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
>> drivers/watchdog/marvell_gti_wdt.c:89:2: error: call to undeclared function 'writeq'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
writeq(GTI_CWD_INT_PENDING_STATUS(priv->wdt_timer_idx),
^
drivers/watchdog/marvell_gti_wdt.c:101:2: error: call to undeclared function 'writeq'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
writeq(GTI_CWD_POKE_VAL,
^
drivers/watchdog/marvell_gti_wdt.c:118:2: error: call to undeclared function 'writeq'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
writeq(GTI_CWD_INT_PENDING_STATUS(priv->wdt_timer_idx),
^
>> drivers/watchdog/marvell_gti_wdt.c:126:11: error: call to undeclared function 'readq'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
^
drivers/watchdog/marvell_gti_wdt.c:139:2: error: call to undeclared function 'writeq'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
writeq(GTI_CWD_INT_ENA_CLR_VAL(priv->wdt_timer_idx),
^
drivers/watchdog/marvell_gti_wdt.c:143:11: error: call to undeclared function 'readq'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
^
drivers/watchdog/marvell_gti_wdt.c:177:11: error: call to undeclared function 'readq'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
^
drivers/watchdog/marvell_gti_wdt.c:181:2: error: call to undeclared function 'writeq'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
writeq(regval, priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
^
6 warnings and 8 errors generated.
vim +/writeq +89 drivers/watchdog/marvell_gti_wdt.c
82
83 static irqreturn_t gti_wdt_interrupt(int irq, void *data)
84 {
85 struct watchdog_device *wdev = data;
86 struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
87
88 /* Clear Interrupt Pending Status */
> 89 writeq(GTI_CWD_INT_PENDING_STATUS(priv->wdt_timer_idx),
90 priv->base + GTI_CWD_INT);
91
92 watchdog_notify_pretimeout(wdev);
93
94 return IRQ_HANDLED;
95 }
96
97 static int gti_wdt_ping(struct watchdog_device *wdev)
98 {
99 struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
100
101 writeq(GTI_CWD_POKE_VAL,
102 priv->base + GTI_CWD_POKE(priv->wdt_timer_idx));
103
104 return 0;
105 }
106
107 static int gti_wdt_start(struct watchdog_device *wdev)
108 {
109 struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
110 u64 regval;
111
112 if (!wdev->pretimeout)
113 return -EINVAL;
114
115 set_bit(WDOG_HW_RUNNING, &wdev->status);
116
117 /* Clear any pending interrupt */
118 writeq(GTI_CWD_INT_PENDING_STATUS(priv->wdt_timer_idx),
119 priv->base + GTI_CWD_INT);
120
121 /* Enable Interrupt */
122 writeq(GTI_CWD_INT_ENA_SET_VAL(priv->wdt_timer_idx),
123 priv->base + GTI_CWD_INT_ENA_SET);
124
125 /* Set (Interrupt + SCP interrupt (DEL3T) + core domain reset) Mode */
> 126 regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
127 regval |= GTI_CWD_WDOG_MODE_INT_DEL3T_RST;
128 writeq(regval, priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
129
130 return 0;
131 }
132
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On 09/05/2023 11:01, Bharat Bhushan wrote:
>
>
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[email protected]>
>> Sent: Tuesday, May 9, 2023 1:38 PM
>> To: Bharat Bhushan <[email protected]>; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; linux-
>> [email protected]; Sunil Kovvuri Goutham <[email protected]>
>> Subject: Re: [EXT] Re: [PATCH 1/2 v7] dt-bindings: watchdog: marvell GTI system
>> watchdog driver
>>
>> On 09/05/2023 09:26, Bharat Bhushan wrote:
>>
>>
>>>>> +properties:
>>>>> + compatible:
>>>>> + oneOf:
>>>>> + - const: marvell,octeontx2-wdt
>>>>
>>>> Why is this alone? Judging by the enum below, octeontx2 is not specific.
>>>>
>>>>> + - items:
>>>>> + - enum:
>>>>> + - marvell,octeontx2-95xx-wdt
>>>>> + - marvell,octeontx2-96xx-wdt
>>>>> + - marvell,octeontx2-98xx-wdt
>>>>
>>>> We don't allow wildcards in general
>>>
>>> Marvell have octeontx2 series of processor which have watchdog timer.
>>> In 95xx,98xx,96xx are the processors in octeontx2 series of processor. So
>> octeontx2-95xx is on soc, octeontx2-96xx is another and so on.
>>
>> No, 95xx is not a processor. Otherwise please point me to exact product
>> datasheet. Hint: I checked it.
>
> Looks like 95xx data sheet is not public, will remove in that case.
We can talk about 96xx. Can you point me to the SoC named exactly like
this? Hint: I checked it.
>
>>
>>>
>>>>
>>>>> + - const: marvell,octeontx2-wdt
>>>>> + - const: marvell,cn10k-wdt
>>>>
>>>> Same question - why is this alone?
>>> Same here, Marvell have cn10k series of processors and cn10kx and cnf10kx are
>> the processor in this series.
>>
>> I don't understand how does it explain my concern. This is alone because there
>> are series of processors? How is that related?
>
> Tried to make it look like other drivers. Let's keep it simple, we want to enable this only for below ones
Enable what? None of these explains why do you need this entry alone,
since it is covered by list further.
>
> properties:
> compatible:
> enum:
> - marvell,cn10k-wdt
> - marvell,octeontx2-wdt
>
> Are you good with that?
Not sure, it sounds like it ignores our entire discussion. What's the
name of the SoC? I see "OcteonTx2 CN9130", not "OcteonTx2". I linked
your previously guide how to write bindings. Did you read it?
Best regards,
Krzysztof
> -----Original Message-----
> From: Krzysztof Kozlowski <[email protected]>
> Sent: Tuesday, May 9, 2023 6:36 PM
> To: Bharat Bhushan <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; Sunil Kovvuri Goutham <[email protected]>
> Subject: Re: [EXT] Re: [PATCH 1/2 v7] dt-bindings: watchdog: marvell GTI system
> watchdog driver
>
> On 09/05/2023 11:01, Bharat Bhushan wrote:
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <[email protected]>
> >> Sent: Tuesday, May 9, 2023 1:38 PM
> >> To: Bharat Bhushan <[email protected]>; [email protected];
> >> [email protected]; [email protected];
> >> [email protected];
> >> [email protected]; [email protected]; linux-
> >> [email protected]; Sunil Kovvuri Goutham <[email protected]>
> >> Subject: Re: [EXT] Re: [PATCH 1/2 v7] dt-bindings: watchdog: marvell
> >> GTI system watchdog driver
> >>
> >> On 09/05/2023 09:26, Bharat Bhushan wrote:
> >>
> >>
> >>>>> +properties:
> >>>>> + compatible:
> >>>>> + oneOf:
> >>>>> + - const: marvell,octeontx2-wdt
> >>>>
> >>>> Why is this alone? Judging by the enum below, octeontx2 is not specific.
> >>>>
> >>>>> + - items:
> >>>>> + - enum:
> >>>>> + - marvell,octeontx2-95xx-wdt
> >>>>> + - marvell,octeontx2-96xx-wdt
> >>>>> + - marvell,octeontx2-98xx-wdt
> >>>>
> >>>> We don't allow wildcards in general
> >>>
> >>> Marvell have octeontx2 series of processor which have watchdog timer.
> >>> In 95xx,98xx,96xx are the processors in octeontx2 series of
> >>> processor. So
> >> octeontx2-95xx is on soc, octeontx2-96xx is another and so on.
> >>
> >> No, 95xx is not a processor. Otherwise please point me to exact
> >> product datasheet. Hint: I checked it.
> >
> > Looks like 95xx data sheet is not public, will remove in that case.
>
> We can talk about 96xx. Can you point me to the SoC named exactly like this?
> Hint: I checked it.
To recap what Bharat mentioned before along with references to individual processors.
OcteonTx2 is a family of processors
https://www.marvell.com/products/data-processing-units.html
Please check for "OCTEON TX2 DPUs"
CN96xx and CN98xx are two silicon variants in this family.
https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-infrastructure-processors-octeon-tx2-cn92xx-cn96xx-cn98xx-product-brief-2020-02.pdf
And CNF95xx is another silicon variant in the same family.
https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-infrastructure-processors-octeon-fusion-cnf95xx-product-brief.pdf
Since the HW block is same in all the variants of silicons in this family, we would like to use a
generic string instead of different compatible string for each one. ie
- const: marvell,octeontx2-wdt
Hope this is okay.
Same with CN10K or Octeon10 family of silicons.
https://www.marvell.com/products/data-processing-units.html
Please check for "OCTEON 10"
CN103xx and CN106xx are two silicons in this family.
https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-octeon-10-dpu-platform-product-brief.pdf
Same with CNF105xx
https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-octeon-10-fusion-cnf105xx-product-brief.pdf
For this family we would like to use
- const: marvell,cn10k-wdt
as the compatible string, as it represents all silicon variants in this family.
Thanks,
Sunil.
On 16/05/2023 12:06, Sunil Kovvuri Goutham wrote:
>>>>> Marvell have octeontx2 series of processor which have watchdog timer.
>>>>> In 95xx,98xx,96xx are the processors in octeontx2 series of
>>>>> processor. So
>>>> octeontx2-95xx is on soc, octeontx2-96xx is another and so on.
>>>>
>>>> No, 95xx is not a processor. Otherwise please point me to exact
>>>> product datasheet. Hint: I checked it.
>>>
>>> Looks like 95xx data sheet is not public, will remove in that case.
>>
>> We can talk about 96xx. Can you point me to the SoC named exactly like this?
>> Hint: I checked it.
>
> To recap what Bharat mentioned before along with references to individual processors.
> OcteonTx2 is a family of processors
> https://www.marvell.com/products/data-processing-units.html
> Please check for "OCTEON TX2 DPUs"
> CN96xx and CN98xx are two silicon variants in this family.
> https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-infrastructure-processors-octeon-tx2-cn92xx-cn96xx-cn98xx-product-brief-2020-02.pdf
This is a product brief which further suggests CN96xx is a family (or
sub-family).
"xx" is pretty often used as family, not as product. Otherwise how one
product CN92XX can come with 12-18 cores *in the same time*?
https://www.marvell.com/company/newsroom/marvell-announces-octeon-tx2-family-of-multi-core-infrastructure-processors.html
"Marvell’s CN91xx, CN92xx, CN96xx, and CN98xx processor families include:"
https://www.marvell.com/products/data-processing-units.html
> And CNF95xx is another silicon variant in the same family.
> https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-infrastructure-processors-octeon-fusion-cnf95xx-product-brief.pdf
Again, unspecific product brief. Your other briefs specify them clearer,
e.g. CN9130, CN9131
>
> Since the HW block is same in all the variants of silicons in this family, we would like to use a
> generic string instead of different compatible string for each one. ie
> - const: marvell,octeontx2-wdt
> Hope this is okay.
https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
>
> Same with CN10K or Octeon10 family of silicons.
> https://www.marvell.com/products/data-processing-units.html
> Please check for "OCTEON 10"
>
> CN103xx and CN106xx are two silicons in this family.
Are they? "Up to 8" cores, so how this can be one specific silicon? One
customer buys CN10300 with 8 cores, second buys exactly the same CN10300
and has 4 cores?
You are mixing families and specific devices.
Best regards,
Krzysztof
> -----Original Message-----
> From: Krzysztof Kozlowski <[email protected]>
> Sent: Tuesday, May 16, 2023 3:55 PM
> To: Sunil Kovvuri Goutham <[email protected]>; Bharat Bhushan
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [EXT] Re: [PATCH 1/2 v7] dt-bindings: watchdog: marvell GTI system
> watchdog driver
>
> On 16/05/2023 12:06, Sunil Kovvuri Goutham wrote:
>
>
> >>>>> Marvell have octeontx2 series of processor which have watchdog timer.
> >>>>> In 95xx,98xx,96xx are the processors in octeontx2 series of
> >>>>> processor. So
> >>>> octeontx2-95xx is on soc, octeontx2-96xx is another and so on.
> >>>>
> >>>> No, 95xx is not a processor. Otherwise please point me to exact
> >>>> product datasheet. Hint: I checked it.
> >>>
> >>> Looks like 95xx data sheet is not public, will remove in that case.
> >>
> >> We can talk about 96xx. Can you point me to the SoC named exactly like this?
> >> Hint: I checked it.
> >
> > To recap what Bharat mentioned before along with references to individual
> processors.
> > OcteonTx2 is a family of processors
> > https://www.marvell.com/products/data-processing-units.html
> > Please check for "OCTEON TX2 DPUs"
> > CN96xx and CN98xx are two silicon variants in this family.
> > https://www.marvell.com/content/dam/marvell/en/public-collateral/embed
> > ded-processors/marvell-infrastructure-processors-octeon-tx2-cn92xx-cn9
> > 6xx-cn98xx-product-brief-2020-02.pdf
>
> This is a product brief which further suggests CN96xx is a family (or sub-family).
>
> "xx" is pretty often used as family, not as product. Otherwise how one product
> CN92XX can come with 12-18 cores *in the same time*?
"xx" here suggests skews, some 92xx may have 18 cores and some may have
few cores fused out resulting in 12cores.
>
> https://www.marvell.com/company/newsroom/marvell-announces-octeon-
> tx2-family-of-multi-core-infrastructure-processors.html
> "Marvell’s CN91xx, CN92xx, CN96xx, and CN98xx processor families include:"
>
> https://www.marvell.com/products/data-processing-units.html
>
>
> > And CNF95xx is another silicon variant in the same family.
> > https://www.marvell.com/content/dam/marvell/en/public-collateral/embed
> > ded-processors/marvell-infrastructure-processors-octeon-fusion-cnf95xx
> > -product-brief.pdf
>
> Again, unspecific product brief. Your other briefs specify them clearer, e.g.
> CN9130, CN9131
Not sure what you are looking for in the product brief, I pointed to the link just to show proof
of 95xx being an official product.
>
> >
> > Since the HW block is same in all the variants of silicons in this
> > family, we would like to use a generic string instead of different
> > compatible string for each one. ie
> > - const: marvell,octeontx2-wdt
> > Hope this is okay.
>
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__elixir.bootlin.com_linux_v6.1-
> 2Drc1_source_Documentation_devicetree_bindings_writing-2Dbindings.rst-
> 23L42&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=q3VKxXQKiboRw_F01ggTz
> HuhwawxR1P9_tMCN2FODU4&m=GGfz5QI8ldHRqao5OsrfuHZQso5LLNBeBxCZr
> Ai7Zow-
> qoKl_S1Yw90OWnSZ3FFx&s=kM0VFY1b15BYvp2EUapQjZ6Eb96aZ_yAE76EKCmA
> aEQ&e=
>
> >
> > Same with CN10K or Octeon10 family of silicons.
> > https://www.marvell.com/products/data-processing-units.html
> > Please check for "OCTEON 10"
> >
> > CN103xx and CN106xx are two silicons in this family.
>
> Are they? "Up to 8" cores, so how this can be one specific silicon? One customer
> buys CN10300 with 8 cores, second buys exactly the same CN10300 and has 4
> cores?
>
> You are mixing families and specific devices.
I was making it simple to understand.
OcteonTx2 and Octeon10 (CN10K) are two generations of Octeon processors from Marvell.
Within each generation there are multiple silicon variants.
Again for each variant there are multiple skews.
Since the watchdog hardware block functionality is same across all of above
generations / variants / families / skews, is it okay to use below compatible strings ?
- const: marvell,octeontx2-wdt
- const: marvell,cn10k-wdt
Also this is the same naming we have been using in other drivers as well.
drivers/net/ethernet/marvell/octeontx2
drivers/net/ethernet/marvell/octeontx2/af/rvu_cn10k.c
drivers/perf/marvell_cn10k_ddr_pmu.c
static const struct of_device_id cn10k_ddr_pmu_of_match[] = {
{ .compatible = "marvell,cn10k-ddr-pmu", },
{ },
};
Thanks,
Sunil.
On 16/05/2023 13:24, Sunil Kovvuri Goutham wrote:
>
>
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[email protected]>
>> Sent: Tuesday, May 16, 2023 3:55 PM
>> To: Sunil Kovvuri Goutham <[email protected]>; Bharat Bhushan
>> <[email protected]>; [email protected]; [email protected];
>> [email protected]; [email protected]; linux-
>> [email protected]; [email protected]; linux-
>> [email protected]
>> Subject: Re: [EXT] Re: [PATCH 1/2 v7] dt-bindings: watchdog: marvell GTI system
>> watchdog driver
>>
>> On 16/05/2023 12:06, Sunil Kovvuri Goutham wrote:
>>
>>
>>>>>>> Marvell have octeontx2 series of processor which have watchdog timer.
>>>>>>> In 95xx,98xx,96xx are the processors in octeontx2 series of
>>>>>>> processor. So
>>>>>> octeontx2-95xx is on soc, octeontx2-96xx is another and so on.
>>>>>>
>>>>>> No, 95xx is not a processor. Otherwise please point me to exact
>>>>>> product datasheet. Hint: I checked it.
>>>>>
>>>>> Looks like 95xx data sheet is not public, will remove in that case.
>>>>
>>>> We can talk about 96xx. Can you point me to the SoC named exactly like this?
>>>> Hint: I checked it.
>>>
>>> To recap what Bharat mentioned before along with references to individual
>> processors.
>>> OcteonTx2 is a family of processors
>>> https://www.marvell.com/products/data-processing-units.html
>>> Please check for "OCTEON TX2 DPUs"
>>> CN96xx and CN98xx are two silicon variants in this family.
>>> https://www.marvell.com/content/dam/marvell/en/public-collateral/embed
>>> ded-processors/marvell-infrastructure-processors-octeon-tx2-cn92xx-cn9
>>> 6xx-cn98xx-product-brief-2020-02.pdf
>>
>> This is a product brief which further suggests CN96xx is a family (or sub-family).
>>
>> "xx" is pretty often used as family, not as product. Otherwise how one product
>> CN92XX can come with 12-18 cores *in the same time*?
>
> "xx" here suggests skews, some 92xx may have 18 cores and some may have
> few cores fused out resulting in 12cores.
Is the DTSI for them the same? IOW, 12-core and 18-core SoCs have
exactly the same DTSI with all properties being the same, valid and not
customized by firmware per skew? If we talk about ARM architecture, DTS
expects CPUs there, so I really wonder how do you write one DTS which
has in the same time 12 and 18 enabled CPUs. Remember - the same time
and not changed by firmware.
>
>>
>>>
>>> Since the HW block is same in all the variants of silicons in this
>>> family, we would like to use a generic string instead of different
>>> compatible string for each one. ie
>>> - const: marvell,octeontx2-wdt
>>> Hope this is okay.
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-
>> 3A__elixir.bootlin.com_linux_v6.1-
>> 2Drc1_source_Documentation_devicetree_bindings_writing-2Dbindings.rst-
>> 23L42&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=q3VKxXQKiboRw_F01ggTz
>> HuhwawxR1P9_tMCN2FODU4&m=GGfz5QI8ldHRqao5OsrfuHZQso5LLNBeBxCZr
>> Ai7Zow-
>> qoKl_S1Yw90OWnSZ3FFx&s=kM0VFY1b15BYvp2EUapQjZ6Eb96aZ_yAE76EKCmA
>> aEQ&e=
>>
>>>
>>> Same with CN10K or Octeon10 family of silicons.
>>> https://www.marvell.com/products/data-processing-units.html
>>> Please check for "OCTEON 10"
>>>
>>> CN103xx and CN106xx are two silicons in this family.
>>
>> Are they? "Up to 8" cores, so how this can be one specific silicon? One customer
>> buys CN10300 with 8 cores, second buys exactly the same CN10300 and has 4
>> cores?
>>
>> You are mixing families and specific devices.
>
> I was making it simple to understand.
>
> OcteonTx2 and Octeon10 (CN10K) are two generations of Octeon processors from Marvell.
I know. I don't think we talk about the same thing...
> Within each generation there are multiple silicon variants.
> Again for each variant there are multiple skews.
>
> Since the watchdog hardware block functionality is same across all of above
> generations / variants / families / skews, is it okay to use below compatible strings ?
You got the link which explains it.
We had this discussions for thousands times. Just a few references from
bookmarks:
https://lore.kernel.org/all/[email protected]/
https://lore.kernel.org/all/[email protected]/
https://lore.kernel.org/all/[email protected]/
https://lore.kernel.org/all/[email protected]/
Explain me how is this different. Please do not repeat the same
arguments as everywhere else, because we covered them.
>
> - const: marvell,octeontx2-wdt
> - const: marvell,cn10k-wdt
>
> Also this is the same naming we have been using in other drivers as well.
> drivers/net/ethernet/marvell/octeontx2
> drivers/net/ethernet/marvell/octeontx2/af/rvu_cn10k.c
Ah, the argument "others did it" or "in the past we did". If the
approach is buggy, does it mean you should duplicate the same buggy
approach to new bindings?
Best regards,
Krzysztof
On 16/05/2023 13:24, Sunil Kovvuri Goutham wrote:
>
> Also this is the same naming we have been using in other drivers as well.
> drivers/net/ethernet/marvell/octeontx2
> drivers/net/ethernet/marvell/octeontx2/af/rvu_cn10k.c
>
> drivers/perf/marvell_cn10k_ddr_pmu.c
> static const struct of_device_id cn10k_ddr_pmu_of_match[] = {
> { .compatible = "marvell,cn10k-ddr-pmu", },
> { },
BTW, I don't understand this part. We do not talk about fallback
compatible, so what does it prove? Of course driver will look like that,
but we established it some time ago, didn't we?
Best regards,
Krzysztof