Exynos850 is a bit different from SoCs already supported in WDT driver:
- AUTOMATIC_WDT_RESET_DISABLE register is removed, so its value is
always 0; .disable_auto_reset callback is not set for that reason
- MASK_WDT_RESET_REQUEST register is replaced with
CLUSTERx_NONCPU_IN_EN register; instead of masking (disabling) WDT
reset interrupt it's now enabled with the same value; .mask_reset
callback is reused for that functionality though
- To make WDT functional, WDT counter needs to be enabled in
CLUSTERx_NONCPU_OUT register; it's done using .enable_counter
callback
Also Exynos850 has two CPU clusters, each has its own dedicated WDT
instance. Different PMU registers and bits are used for each cluster. So
driver data is now modified in probe, adding needed info depending on
cluster index passed from device tree.
Signed-off-by: Sam Protsenko <[email protected]>
---
Changes in v2:
- Used single compatible for Exynos850, populating missing driver data in
probe
- Added "index" property to specify CPU cluster index
drivers/watchdog/s3c2410_wdt.c | 68 +++++++++++++++++++++++++++++++++-
1 file changed, 66 insertions(+), 2 deletions(-)
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 8fdda2ede1c3..457b725c30ac 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -56,6 +56,14 @@
#define EXYNOS5_RST_STAT_REG_OFFSET 0x0404
#define EXYNOS5_WDT_DISABLE_REG_OFFSET 0x0408
#define EXYNOS5_WDT_MASK_RESET_REG_OFFSET 0x040c
+#define EXYNOS850_CLUSTER0_NONCPU_OUT 0x1220
+#define EXYNOS850_CLUSTER0_NONCPU_INT_EN 0x1244
+#define EXYNOS850_CLUSTER1_NONCPU_OUT 0x1620
+#define EXYNOS850_CLUSTER1_NONCPU_INT_EN 0x1644
+
+#define EXYNOS850_CLUSTER0_WDTRESET_BIT 24
+#define EXYNOS850_CLUSTER1_WDTRESET_BIT 23
+
#define QUIRK_HAS_WTCLRINT_REG (1 << 0)
#define QUIRK_HAS_PMU_MASK_RESET (1 << 1)
#define QUIRK_HAS_PMU_RST_STAT (1 << 2)
@@ -171,6 +179,21 @@ static const struct s3c2410_wdt_variant drv_data_exynos7 = {
QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_AUTO_DISABLE,
};
+static const struct s3c2410_wdt_variant drv_data_exynos850 = {
+ /*
+ * Next fields will be set in probe(), based on cluster index:
+ * - .mask_reset_reg
+ * - .rst_stat_bit
+ * - .cnt_en_reg
+ */
+ .mask_reset_inv = true,
+ .mask_bit = 2,
+ .rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
+ .cnt_en_bit = 7,
+ .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | \
+ QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN,
+};
+
static const struct of_device_id s3c2410_wdt_match[] = {
{ .compatible = "samsung,s3c2410-wdt",
.data = &drv_data_s3c2410 },
@@ -182,6 +205,8 @@ static const struct of_device_id s3c2410_wdt_match[] = {
.data = &drv_data_exynos5420 },
{ .compatible = "samsung,exynos7-wdt",
.data = &drv_data_exynos7 },
+ { .compatible = "samsung,exynos850-wdt",
+ .data = &drv_data_exynos850 },
{},
};
MODULE_DEVICE_TABLE(of, s3c2410_wdt_match);
@@ -548,15 +573,51 @@ static inline const struct s3c2410_wdt_variant *
s3c2410_get_wdt_drv_data(struct platform_device *pdev)
{
const struct s3c2410_wdt_variant *variant;
+ struct s3c2410_wdt_variant *data;
+ struct device *dev = &pdev->dev;
- variant = of_device_get_match_data(&pdev->dev);
+ variant = of_device_get_match_data(dev);
if (!variant) {
/* Device matched by platform_device_id */
variant = (struct s3c2410_wdt_variant *)
platform_get_device_id(pdev)->driver_data;
}
- return variant;
+ /* Have to copy driver data over to keep its const qualifier intact */
+ data = devm_kmemdup(dev, variant, sizeof(*variant), GFP_KERNEL);
+ if (!data)
+ return NULL;
+
+ /* Populate missing fields for Exynos850 w.r.t. cluster index */
+ if (variant == &drv_data_exynos850) {
+ u32 index;
+ int err;
+
+ err = of_property_read_u32(dev->of_node, "samsung,index",
+ &index);
+ if (err) {
+ dev_err(dev, "failed to get cluster index\n");
+ return NULL;
+ }
+
+ switch (index) {
+ case 0:
+ data->mask_reset_reg = EXYNOS850_CLUSTER0_NONCPU_INT_EN;
+ data->rst_stat_bit = EXYNOS850_CLUSTER0_WDTRESET_BIT;
+ data->cnt_en_reg = EXYNOS850_CLUSTER0_NONCPU_OUT;
+ break;
+ case 1:
+ data->mask_reset_reg = EXYNOS850_CLUSTER1_NONCPU_INT_EN;
+ data->rst_stat_bit = EXYNOS850_CLUSTER1_WDTRESET_BIT;
+ data->cnt_en_reg = EXYNOS850_CLUSTER1_NONCPU_OUT;
+ break;
+ default:
+ dev_err(dev, "wrong cluster index: %u\n", index);
+ return NULL;
+ }
+ }
+
+ return data;
}
static int s3c2410wdt_probe(struct platform_device *pdev)
@@ -576,6 +637,9 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
wdt->wdt_device = s3c2410_wdd;
wdt->drv_data = s3c2410_get_wdt_drv_data(pdev);
+ if (!wdt->drv_data)
+ return -EINVAL;
+
if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
"samsung,syscon-phandle");
--
2.30.2
Hi Sam,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on groeck-staging/hwmon-next linus/master v5.15-rc7 next-20211029]
[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]
url: https://github.com/0day-ci/linux/commits/Sam-Protsenko/watchdog-s3c2410-Add-Exynos850-support/20211031-202352
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: hexagon-randconfig-r045-20211031 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 82ed106567063ea269c6d5669278b733e173a42f)
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/0day-ci/linux/commit/ff70b806cc12adc068bb54865f81d3aa29a69471
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sam-Protsenko/watchdog-s3c2410-Add-Exynos850-support/20211031-202352
git checkout ff70b806cc12adc068bb54865f81d3aa29a69471
# save the attached .config to linux build tree
mkdir build_dir
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 as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/watchdog/s3c2410_wdt.c:592:18: error: use of undeclared identifier 'drv_data_exynos850'
if (variant == &drv_data_exynos850) {
^
1 error generated.
vim +/drv_data_exynos850 +592 drivers/watchdog/s3c2410_wdt.c
571
572 static inline const struct s3c2410_wdt_variant *
573 s3c2410_get_wdt_drv_data(struct platform_device *pdev)
574 {
575 const struct s3c2410_wdt_variant *variant;
576 struct s3c2410_wdt_variant *data;
577 struct device *dev = &pdev->dev;
578
579 variant = of_device_get_match_data(dev);
580 if (!variant) {
581 /* Device matched by platform_device_id */
582 variant = (struct s3c2410_wdt_variant *)
583 platform_get_device_id(pdev)->driver_data;
584 }
585
586 /* Have to copy driver data over to keep its const qualifier intact */
587 data = devm_kmemdup(dev, variant, sizeof(*variant), GFP_KERNEL);
588 if (!data)
589 return NULL;
590
591 /* Populate missing fields for Exynos850 w.r.t. cluster index */
> 592 if (variant == &drv_data_exynos850) {
593 u32 index;
594 int err;
595
596 err = of_property_read_u32(dev->of_node, "samsung,index",
597 &index);
598 if (err) {
599 dev_err(dev, "failed to get cluster index\n");
600 return NULL;
601 }
602
603 switch (index) {
604 case 0:
605 data->mask_reset_reg = EXYNOS850_CLUSTER0_NONCPU_INT_EN;
606 data->rst_stat_bit = EXYNOS850_CLUSTER0_WDTRESET_BIT;
607 data->cnt_en_reg = EXYNOS850_CLUSTER0_NONCPU_OUT;
608 break;
609 case 1:
610 data->mask_reset_reg = EXYNOS850_CLUSTER1_NONCPU_INT_EN;
611 data->rst_stat_bit = EXYNOS850_CLUSTER1_WDTRESET_BIT;
612 data->cnt_en_reg = EXYNOS850_CLUSTER1_NONCPU_OUT;
613 break;
614 default:
615 dev_err(dev, "wrong cluster index: %u\n", index);
616 return NULL;
617 }
618 }
619
620 return data;
621 }
622
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Sam,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on groeck-staging/hwmon-next linus/master v5.15-rc7 next-20211029]
[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]
url: https://github.com/0day-ci/linux/commits/Sam-Protsenko/watchdog-s3c2410-Add-Exynos850-support/20211031-202352
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: xtensa-randconfig-r012-20211031 (attached as .config)
compiler: xtensa-linux-gcc (GCC) 11.2.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/0day-ci/linux/commit/ff70b806cc12adc068bb54865f81d3aa29a69471
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sam-Protsenko/watchdog-s3c2410-Add-Exynos850-support/20211031-202352
git checkout ff70b806cc12adc068bb54865f81d3aa29a69471
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=xtensa SHELL=/bin/bash drivers/watchdog/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/watchdog/s3c2410_wdt.c: In function 's3c2410_get_wdt_drv_data':
>> drivers/watchdog/s3c2410_wdt.c:592:25: error: 'drv_data_exynos850' undeclared (first use in this function)
592 | if (variant == &drv_data_exynos850) {
| ^~~~~~~~~~~~~~~~~~
drivers/watchdog/s3c2410_wdt.c:592:25: note: each undeclared identifier is reported only once for each function it appears in
vim +/drv_data_exynos850 +592 drivers/watchdog/s3c2410_wdt.c
571
572 static inline const struct s3c2410_wdt_variant *
573 s3c2410_get_wdt_drv_data(struct platform_device *pdev)
574 {
575 const struct s3c2410_wdt_variant *variant;
576 struct s3c2410_wdt_variant *data;
577 struct device *dev = &pdev->dev;
578
579 variant = of_device_get_match_data(dev);
580 if (!variant) {
581 /* Device matched by platform_device_id */
582 variant = (struct s3c2410_wdt_variant *)
583 platform_get_device_id(pdev)->driver_data;
584 }
585
586 /* Have to copy driver data over to keep its const qualifier intact */
587 data = devm_kmemdup(dev, variant, sizeof(*variant), GFP_KERNEL);
588 if (!data)
589 return NULL;
590
591 /* Populate missing fields for Exynos850 w.r.t. cluster index */
> 592 if (variant == &drv_data_exynos850) {
593 u32 index;
594 int err;
595
596 err = of_property_read_u32(dev->of_node, "samsung,index",
597 &index);
598 if (err) {
599 dev_err(dev, "failed to get cluster index\n");
600 return NULL;
601 }
602
603 switch (index) {
604 case 0:
605 data->mask_reset_reg = EXYNOS850_CLUSTER0_NONCPU_INT_EN;
606 data->rst_stat_bit = EXYNOS850_CLUSTER0_WDTRESET_BIT;
607 data->cnt_en_reg = EXYNOS850_CLUSTER0_NONCPU_OUT;
608 break;
609 case 1:
610 data->mask_reset_reg = EXYNOS850_CLUSTER1_NONCPU_INT_EN;
611 data->rst_stat_bit = EXYNOS850_CLUSTER1_WDTRESET_BIT;
612 data->cnt_en_reg = EXYNOS850_CLUSTER1_NONCPU_OUT;
613 break;
614 default:
615 dev_err(dev, "wrong cluster index: %u\n", index);
616 return NULL;
617 }
618 }
619
620 return data;
621 }
622
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On 31/10/2021 13:22, Sam Protsenko wrote:
> Exynos850 is a bit different from SoCs already supported in WDT driver:
> - AUTOMATIC_WDT_RESET_DISABLE register is removed, so its value is
> always 0; .disable_auto_reset callback is not set for that reason
> - MASK_WDT_RESET_REQUEST register is replaced with
> CLUSTERx_NONCPU_IN_EN register; instead of masking (disabling) WDT
> reset interrupt it's now enabled with the same value; .mask_reset
> callback is reused for that functionality though
> - To make WDT functional, WDT counter needs to be enabled in
> CLUSTERx_NONCPU_OUT register; it's done using .enable_counter
> callback
>
> Also Exynos850 has two CPU clusters, each has its own dedicated WDT
> instance. Different PMU registers and bits are used for each cluster. So
> driver data is now modified in probe, adding needed info depending on
> cluster index passed from device tree.
>
> Signed-off-by: Sam Protsenko <[email protected]>
> ---
> Changes in v2:
> - Used single compatible for Exynos850, populating missing driver data in
> probe
> - Added "index" property to specify CPU cluster index
>
> drivers/watchdog/s3c2410_wdt.c | 68 +++++++++++++++++++++++++++++++++-
> 1 file changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 8fdda2ede1c3..457b725c30ac 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -56,6 +56,14 @@
> #define EXYNOS5_RST_STAT_REG_OFFSET 0x0404
> #define EXYNOS5_WDT_DISABLE_REG_OFFSET 0x0408
> #define EXYNOS5_WDT_MASK_RESET_REG_OFFSET 0x040c
> +#define EXYNOS850_CLUSTER0_NONCPU_OUT 0x1220
> +#define EXYNOS850_CLUSTER0_NONCPU_INT_EN 0x1244
> +#define EXYNOS850_CLUSTER1_NONCPU_OUT 0x1620
> +#define EXYNOS850_CLUSTER1_NONCPU_INT_EN 0x1644
> +
> +#define EXYNOS850_CLUSTER0_WDTRESET_BIT 24
> +#define EXYNOS850_CLUSTER1_WDTRESET_BIT 23
> +
> #define QUIRK_HAS_WTCLRINT_REG (1 << 0)
> #define QUIRK_HAS_PMU_MASK_RESET (1 << 1)
> #define QUIRK_HAS_PMU_RST_STAT (1 << 2)
> @@ -171,6 +179,21 @@ static const struct s3c2410_wdt_variant drv_data_exynos7 = {
> QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_AUTO_DISABLE,
> };
>
> +static const struct s3c2410_wdt_variant drv_data_exynos850 = {
> + /*
> + * Next fields will be set in probe(), based on cluster index:
> + * - .mask_reset_reg
> + * - .rst_stat_bit
> + * - .cnt_en_reg
> + */
> + .mask_reset_inv = true,
> + .mask_bit = 2,
> + .rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
> + .cnt_en_bit = 7,
> + .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | \
> + QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN,
> +};
> +
> static const struct of_device_id s3c2410_wdt_match[] = {
> { .compatible = "samsung,s3c2410-wdt",
> .data = &drv_data_s3c2410 },
> @@ -182,6 +205,8 @@ static const struct of_device_id s3c2410_wdt_match[] = {
> .data = &drv_data_exynos5420 },
> { .compatible = "samsung,exynos7-wdt",
> .data = &drv_data_exynos7 },
> + { .compatible = "samsung,exynos850-wdt",
> + .data = &drv_data_exynos850 },
> {},
> };
> MODULE_DEVICE_TABLE(of, s3c2410_wdt_match);
> @@ -548,15 +573,51 @@ static inline const struct s3c2410_wdt_variant *
> s3c2410_get_wdt_drv_data(struct platform_device *pdev)
> {
> const struct s3c2410_wdt_variant *variant;
> + struct s3c2410_wdt_variant *data;
> + struct device *dev = &pdev->dev;
>
> - variant = of_device_get_match_data(&pdev->dev);
> + variant = of_device_get_match_data(dev);
> if (!variant) {
> /* Device matched by platform_device_id */
> variant = (struct s3c2410_wdt_variant *)
> platform_get_device_id(pdev)->driver_data;
> }
>
> - return variant;
> + /* Have to copy driver data over to keep its const qualifier intact */
> + data = devm_kmemdup(dev, variant, sizeof(*variant), GFP_KERNEL);
> + if (!data)
> + return NULL;
> +
> + /* Populate missing fields for Exynos850 w.r.t. cluster index */
> + if (variant == &drv_data_exynos850) {
> + u32 index;
> + int err;
Another approach is to:
1. Define two variants for Exynos850 (s3c2410_wdt_variants), kind of
like before,
2. if (variant == &drv_data_exynos850)
a. Read the index
b. If index is 0, return first variant,
c. If index is 1, return the second variant,
d. Else - NULL.
This way you won't need to copy the memory on the fly, just use
different const data. Benefits: less memory allocations, entire drvdata
set in one place (so nicely visible), drvdata populated safely via const.
> +
> + err = of_property_read_u32(dev->of_node, "samsung,index",
> + &index);
> + if (err) {
> + dev_err(dev, "failed to get cluster index\n");
> + return NULL;
> + }
> +
> + switch (index) {
> + case 0:
> + data->mask_reset_reg = EXYNOS850_CLUSTER0_NONCPU_INT_EN;
> + data->rst_stat_bit = EXYNOS850_CLUSTER0_WDTRESET_BIT;
> + data->cnt_en_reg = EXYNOS850_CLUSTER0_NONCPU_OUT;
> + break;
> + case 1:
> + data->mask_reset_reg = EXYNOS850_CLUSTER1_NONCPU_INT_EN;
> + data->rst_stat_bit = EXYNOS850_CLUSTER1_WDTRESET_BIT;
> + data->cnt_en_reg = EXYNOS850_CLUSTER1_NONCPU_OUT;
> + break;
> + default:
> + dev_err(dev, "wrong cluster index: %u\n", index);
> + return NULL;
> + }
> + }
> +
> + return data;
> }
>
> static int s3c2410wdt_probe(struct platform_device *pdev)
> @@ -576,6 +637,9 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> wdt->wdt_device = s3c2410_wdd;
>
> wdt->drv_data = s3c2410_get_wdt_drv_data(pdev);
> + if (!wdt->drv_data)
> + return -EINVAL;
> +
> if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> "samsung,syscon-phandle");
>
Best regards,
Krzysztof
On Tue, 2 Nov 2021 at 12:27, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 31/10/2021 13:22, Sam Protsenko wrote:
> > Exynos850 is a bit different from SoCs already supported in WDT driver:
> > - AUTOMATIC_WDT_RESET_DISABLE register is removed, so its value is
> > always 0; .disable_auto_reset callback is not set for that reason
> > - MASK_WDT_RESET_REQUEST register is replaced with
> > CLUSTERx_NONCPU_IN_EN register; instead of masking (disabling) WDT
> > reset interrupt it's now enabled with the same value; .mask_reset
> > callback is reused for that functionality though
> > - To make WDT functional, WDT counter needs to be enabled in
> > CLUSTERx_NONCPU_OUT register; it's done using .enable_counter
> > callback
> >
> > Also Exynos850 has two CPU clusters, each has its own dedicated WDT
> > instance. Different PMU registers and bits are used for each cluster. So
> > driver data is now modified in probe, adding needed info depending on
> > cluster index passed from device tree.
> >
> > Signed-off-by: Sam Protsenko <[email protected]>
> > ---
> > Changes in v2:
> > - Used single compatible for Exynos850, populating missing driver data in
> > probe
> > - Added "index" property to specify CPU cluster index
> >
> > drivers/watchdog/s3c2410_wdt.c | 68 +++++++++++++++++++++++++++++++++-
> > 1 file changed, 66 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > index 8fdda2ede1c3..457b725c30ac 100644
> > --- a/drivers/watchdog/s3c2410_wdt.c
> > +++ b/drivers/watchdog/s3c2410_wdt.c
> > @@ -56,6 +56,14 @@
> > #define EXYNOS5_RST_STAT_REG_OFFSET 0x0404
> > #define EXYNOS5_WDT_DISABLE_REG_OFFSET 0x0408
> > #define EXYNOS5_WDT_MASK_RESET_REG_OFFSET 0x040c
> > +#define EXYNOS850_CLUSTER0_NONCPU_OUT 0x1220
> > +#define EXYNOS850_CLUSTER0_NONCPU_INT_EN 0x1244
> > +#define EXYNOS850_CLUSTER1_NONCPU_OUT 0x1620
> > +#define EXYNOS850_CLUSTER1_NONCPU_INT_EN 0x1644
> > +
> > +#define EXYNOS850_CLUSTER0_WDTRESET_BIT 24
> > +#define EXYNOS850_CLUSTER1_WDTRESET_BIT 23
> > +
> > #define QUIRK_HAS_WTCLRINT_REG (1 << 0)
> > #define QUIRK_HAS_PMU_MASK_RESET (1 << 1)
> > #define QUIRK_HAS_PMU_RST_STAT (1 << 2)
> > @@ -171,6 +179,21 @@ static const struct s3c2410_wdt_variant drv_data_exynos7 = {
> > QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_AUTO_DISABLE,
> > };
> >
> > +static const struct s3c2410_wdt_variant drv_data_exynos850 = {
> > + /*
> > + * Next fields will be set in probe(), based on cluster index:
> > + * - .mask_reset_reg
> > + * - .rst_stat_bit
> > + * - .cnt_en_reg
> > + */
> > + .mask_reset_inv = true,
> > + .mask_bit = 2,
> > + .rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
> > + .cnt_en_bit = 7,
> > + .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | \
> > + QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN,
> > +};
> > +
> > static const struct of_device_id s3c2410_wdt_match[] = {
> > { .compatible = "samsung,s3c2410-wdt",
> > .data = &drv_data_s3c2410 },
> > @@ -182,6 +205,8 @@ static const struct of_device_id s3c2410_wdt_match[] = {
> > .data = &drv_data_exynos5420 },
> > { .compatible = "samsung,exynos7-wdt",
> > .data = &drv_data_exynos7 },
> > + { .compatible = "samsung,exynos850-wdt",
> > + .data = &drv_data_exynos850 },
> > {},
> > };
> > MODULE_DEVICE_TABLE(of, s3c2410_wdt_match);
> > @@ -548,15 +573,51 @@ static inline const struct s3c2410_wdt_variant *
> > s3c2410_get_wdt_drv_data(struct platform_device *pdev)
> > {
> > const struct s3c2410_wdt_variant *variant;
> > + struct s3c2410_wdt_variant *data;
> > + struct device *dev = &pdev->dev;
> >
> > - variant = of_device_get_match_data(&pdev->dev);
> > + variant = of_device_get_match_data(dev);
> > if (!variant) {
> > /* Device matched by platform_device_id */
> > variant = (struct s3c2410_wdt_variant *)
> > platform_get_device_id(pdev)->driver_data;
> > }
> >
> > - return variant;
> > + /* Have to copy driver data over to keep its const qualifier intact */
> > + data = devm_kmemdup(dev, variant, sizeof(*variant), GFP_KERNEL);
> > + if (!data)
> > + return NULL;
> > +
> > + /* Populate missing fields for Exynos850 w.r.t. cluster index */
> > + if (variant == &drv_data_exynos850) {
> > + u32 index;
> > + int err;
>
> Another approach is to:
> 1. Define two variants for Exynos850 (s3c2410_wdt_variants), kind of
> like before,
> 2. if (variant == &drv_data_exynos850)
> a. Read the index
> b. If index is 0, return first variant,
> c. If index is 1, return the second variant,
> d. Else - NULL.
>
> This way you won't need to copy the memory on the fly, just use
> different const data. Benefits: less memory allocations, entire drvdata
> set in one place (so nicely visible), drvdata populated safely via const.
>
That's definitely better. Not sure how I missed that. Anyway, thanks
for review. Will send v3 soon, addressing all your comments (except
the one about src_clk in PATCH 10/12 -- need to discuss it further, I
guess).
> > +
> > + err = of_property_read_u32(dev->of_node, "samsung,index",
> > + &index);
> > + if (err) {
> > + dev_err(dev, "failed to get cluster index\n");
> > + return NULL;
> > + }
> > +
> > + switch (index) {
> > + case 0:
> > + data->mask_reset_reg = EXYNOS850_CLUSTER0_NONCPU_INT_EN;
> > + data->rst_stat_bit = EXYNOS850_CLUSTER0_WDTRESET_BIT;
> > + data->cnt_en_reg = EXYNOS850_CLUSTER0_NONCPU_OUT;
> > + break;
> > + case 1:
> > + data->mask_reset_reg = EXYNOS850_CLUSTER1_NONCPU_INT_EN;
> > + data->rst_stat_bit = EXYNOS850_CLUSTER1_WDTRESET_BIT;
> > + data->cnt_en_reg = EXYNOS850_CLUSTER1_NONCPU_OUT;
> > + break;
> > + default:
> > + dev_err(dev, "wrong cluster index: %u\n", index);
> > + return NULL;
> > + }
> > + }
> > +
> > + return data;
> > }
> >
> > static int s3c2410wdt_probe(struct platform_device *pdev)
> > @@ -576,6 +637,9 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > wdt->wdt_device = s3c2410_wdd;
> >
> > wdt->drv_data = s3c2410_get_wdt_drv_data(pdev);
> > + if (!wdt->drv_data)
> > + return -EINVAL;
> > +
> > if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> > wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> > "samsung,syscon-phandle");
> >
>
>
> Best regards,
> Krzysztof