Hi all,
This series make an update in the MT7621 SoC's watchdog driver code. This
SoC already provides a system controller node to access reset status
register needed for the watchdog. Instead of using MIPS architecture
dependent operations in header 'asm/mach-ralink/ralink_regs.h' use
a phandle to the system controller node and use it through regmap APIs
from the code. Driver is also using some globals that are not needed at
all if a driver data structure is used along the code. Hence, add all
new needed stuff inside a new driver data structure. With this changes
driver can be properly compile tested.
Thanks in advance for reviewing this!
Best regards,
Sergio Paracuellos
Sergio Paracuellos (3):
dt-bindings: watchdog: mt7621-wdt: add phandle to access system
controller registers
mips: dts: ralink: mt7621: add phandle to system controller node for
watchdog
watchdog: mt7621-wdt: avoid globals and arch dependencies
.../watchdog/mediatek,mt7621-wdt.yaml | 12 +-
arch/mips/boot/dts/ralink/mt7621.dtsi | 3 +-
drivers/watchdog/Kconfig | 4 +-
drivers/watchdog/mt7621_wdt.c | 121 ++++++++++++------
4 files changed, 95 insertions(+), 45 deletions(-)
--
2.25.1
MT7621 SoC provides a system controller node for accessing to some registers.
Add a phandle to this node to avoid using MIPS related arch operations and
includes in watchdog driver code.
Signed-off-by: Sergio Paracuellos <[email protected]>
---
.../bindings/watchdog/mediatek,mt7621-wdt.yaml | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml
index b2b17fdf4..3c545065f 100644
--- a/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml
@@ -14,11 +14,18 @@ allOf:
properties:
compatible:
- const: mediatek,mt7621-wdt
+ items:
+ - const: mediatek,mt7621-wdt
+ - const: syscon
reg:
maxItems: 1
+ ralink,sysctl:
+ $ref: /schemas/types.yaml#/definitions/phandle
+ description:
+ phandle of syscon used to control system registers
+
required:
- compatible
- reg
@@ -28,6 +35,7 @@ additionalProperties: false
examples:
- |
watchdog@100 {
- compatible = "mediatek,mt7621-wdt";
+ compatible = "mediatek,mt7621-wdt", "syscon";
reg = <0x100 0x100>;
+ ralink,sysctl = <&sysc>;
};
--
2.25.1
To allow to access system controller registers from watchdog driver code
add a phandle in the watchdog 'wdt' node. This avoid using arch dependent
operations in driver code.
Signed-off-by: Sergio Paracuellos <[email protected]>
---
arch/mips/boot/dts/ralink/mt7621.dtsi | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi
index 5ca40fd21..ebee23a2b 100644
--- a/arch/mips/boot/dts/ralink/mt7621.dtsi
+++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
@@ -71,8 +71,9 @@ sysc: syscon@0 {
};
wdt: wdt@100 {
- compatible = "mediatek,mt7621-wdt";
+ compatible = "mediatek,mt7621-wdt", "syscon";
reg = <0x100 0x100>;
+ ralink,sysctl = <&sysc>;
};
gpio: gpio@600 {
--
2.25.1
MT7621 SoC has a system controller node. Watchdog need to access to reset
status register. Ralink architecture and related driver are old and from
the beggining they ar providing some architecture dependent operations
for accessing this shared registers through 'asm/mach-ralink/ralink_regs.h'
header file. However this is not ideal from a driver perspective which can
just access to the system controller registers in am arch independent way
using regmap syscon APIs. Hence, add a new structure for driver data and
use it along the code. This way architecture dependencies and global vars
are not needed anymore. Update Kconfig accordingly to select new added
dependencies and allow driver to be compile tested.
Signed-off-by: Sergio Paracuellos <[email protected]>
---
drivers/watchdog/Kconfig | 4 +-
drivers/watchdog/mt7621_wdt.c | 121 ++++++++++++++++++++++------------
2 files changed, 83 insertions(+), 42 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index b64bc49c7..cf752ad64 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1865,7 +1865,9 @@ config GXP_WATCHDOG
config MT7621_WDT
tristate "Mediatek SoC watchdog"
select WATCHDOG_CORE
- depends on SOC_MT7620 || SOC_MT7621
+ select REGMAP_MMIO
+ select MFD_SYSCON
+ depends on SOC_MT7620 || SOC_MT7621 || COMPILE_TEST
help
Hardware driver for the Mediatek/Ralink MT7621/8 SoC Watchdog Timer.
diff --git a/drivers/watchdog/mt7621_wdt.c b/drivers/watchdog/mt7621_wdt.c
index a8aa3522c..a7480fd2b 100644
--- a/drivers/watchdog/mt7621_wdt.c
+++ b/drivers/watchdog/mt7621_wdt.c
@@ -15,8 +15,8 @@
#include <linux/moduleparam.h>
#include <linux/platform_device.h>
#include <linux/mod_devicetable.h>
-
-#include <asm/mach-ralink/ralink_regs.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
#define SYSC_RSTSTAT 0x38
#define WDT_RST_CAUSE BIT(1)
@@ -31,8 +31,12 @@
#define TMR1CTL_RESTART BIT(9)
#define TMR1CTL_PRESCALE_SHIFT 16
-static void __iomem *mt7621_wdt_base;
-static struct reset_control *mt7621_wdt_reset;
+struct mt7621_wdt_data {
+ void __iomem *base;
+ struct reset_control *rst;
+ struct regmap *sysc;
+ struct watchdog_device wdt;
+};
static bool nowayout = WATCHDOG_NOWAYOUT;
module_param(nowayout, bool, 0);
@@ -40,27 +44,31 @@ MODULE_PARM_DESC(nowayout,
"Watchdog cannot be stopped once started (default="
__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
-static inline void rt_wdt_w32(unsigned reg, u32 val)
+static inline void rt_wdt_w32(void __iomem *base, unsigned reg, u32 val)
{
- iowrite32(val, mt7621_wdt_base + reg);
+ iowrite32(val, base + reg);
}
-static inline u32 rt_wdt_r32(unsigned reg)
+static inline u32 rt_wdt_r32(void __iomem *base, unsigned reg)
{
- return ioread32(mt7621_wdt_base + reg);
+ return ioread32(base + reg);
}
static int mt7621_wdt_ping(struct watchdog_device *w)
{
- rt_wdt_w32(TIMER_REG_TMRSTAT, TMR1CTL_RESTART);
+ struct mt7621_wdt_data *drvdata = watchdog_get_drvdata(w);
+
+ rt_wdt_w32(drvdata->base, TIMER_REG_TMRSTAT, TMR1CTL_RESTART);
return 0;
}
static int mt7621_wdt_set_timeout(struct watchdog_device *w, unsigned int t)
{
+ struct mt7621_wdt_data *drvdata = watchdog_get_drvdata(w);
+
w->timeout = t;
- rt_wdt_w32(TIMER_REG_TMR1LOAD, t * 1000);
+ rt_wdt_w32(drvdata->base, TIMER_REG_TMR1LOAD, t * 1000);
mt7621_wdt_ping(w);
return 0;
@@ -68,36 +76,41 @@ static int mt7621_wdt_set_timeout(struct watchdog_device *w, unsigned int t)
static int mt7621_wdt_start(struct watchdog_device *w)
{
+ struct mt7621_wdt_data *drvdata = watchdog_get_drvdata(w);
u32 t;
/* set the prescaler to 1ms == 1000us */
- rt_wdt_w32(TIMER_REG_TMR1CTL, 1000 << TMR1CTL_PRESCALE_SHIFT);
+ rt_wdt_w32(drvdata->base, TIMER_REG_TMR1CTL, 1000 << TMR1CTL_PRESCALE_SHIFT);
mt7621_wdt_set_timeout(w, w->timeout);
- t = rt_wdt_r32(TIMER_REG_TMR1CTL);
+ t = rt_wdt_r32(drvdata->base, TIMER_REG_TMR1CTL);
t |= TMR1CTL_ENABLE;
- rt_wdt_w32(TIMER_REG_TMR1CTL, t);
+ rt_wdt_w32(drvdata->base, TIMER_REG_TMR1CTL, t);
return 0;
}
static int mt7621_wdt_stop(struct watchdog_device *w)
{
+ struct mt7621_wdt_data *drvdata = watchdog_get_drvdata(w);
u32 t;
mt7621_wdt_ping(w);
- t = rt_wdt_r32(TIMER_REG_TMR1CTL);
+ t = rt_wdt_r32(drvdata->base, TIMER_REG_TMR1CTL);
t &= ~TMR1CTL_ENABLE;
- rt_wdt_w32(TIMER_REG_TMR1CTL, t);
+ rt_wdt_w32(drvdata->base, TIMER_REG_TMR1CTL, t);
return 0;
}
-static int mt7621_wdt_bootcause(void)
+static int mt7621_wdt_bootcause(struct mt7621_wdt_data *d)
{
- if (rt_sysc_r32(SYSC_RSTSTAT) & WDT_RST_CAUSE)
+ u32 val;
+
+ regmap_read(d->sysc, SYSC_RSTSTAT, &val);
+ if (val & WDT_RST_CAUSE)
return WDIOF_CARDRESET;
return 0;
@@ -105,7 +118,9 @@ static int mt7621_wdt_bootcause(void)
static int mt7621_wdt_is_running(struct watchdog_device *w)
{
- return !!(rt_wdt_r32(TIMER_REG_TMR1CTL) & TMR1CTL_ENABLE);
+ struct mt7621_wdt_data *drvdata = watchdog_get_drvdata(w);
+
+ return !!(rt_wdt_r32(drvdata->base, TIMER_REG_TMR1CTL) & TMR1CTL_ENABLE);
}
static const struct watchdog_info mt7621_wdt_info = {
@@ -121,30 +136,44 @@ static const struct watchdog_ops mt7621_wdt_ops = {
.set_timeout = mt7621_wdt_set_timeout,
};
-static struct watchdog_device mt7621_wdt_dev = {
- .info = &mt7621_wdt_info,
- .ops = &mt7621_wdt_ops,
- .min_timeout = 1,
- .max_timeout = 0xfffful / 1000,
-};
-
static int mt7621_wdt_probe(struct platform_device *pdev)
{
+ struct device_node *np = pdev->dev.of_node;
struct device *dev = &pdev->dev;
- mt7621_wdt_base = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(mt7621_wdt_base))
- return PTR_ERR(mt7621_wdt_base);
+ struct watchdog_device *mt7621_wdt;
+ struct mt7621_wdt_data *drvdata;
+ int err;
+
+ drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+ if (!drvdata)
+ return -ENOMEM;
- mt7621_wdt_reset = devm_reset_control_get_exclusive(dev, NULL);
- if (!IS_ERR(mt7621_wdt_reset))
- reset_control_deassert(mt7621_wdt_reset);
+ drvdata->sysc = syscon_regmap_lookup_by_phandle(np, "ralink,sysctl");
+ if (IS_ERR(drvdata->sysc))
+ return PTR_ERR(drvdata->sysc);
- mt7621_wdt_dev.bootstatus = mt7621_wdt_bootcause();
+ drvdata->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(drvdata->base))
+ return PTR_ERR(drvdata->base);
- watchdog_init_timeout(&mt7621_wdt_dev, mt7621_wdt_dev.max_timeout,
- dev);
- watchdog_set_nowayout(&mt7621_wdt_dev, nowayout);
- if (mt7621_wdt_is_running(&mt7621_wdt_dev)) {
+ drvdata->rst = devm_reset_control_get_exclusive(dev, NULL);
+ if (!IS_ERR(drvdata->rst))
+ reset_control_deassert(drvdata->rst);
+
+ mt7621_wdt = &drvdata->wdt;
+ mt7621_wdt->info = &mt7621_wdt_info;
+ mt7621_wdt->ops = &mt7621_wdt_ops;
+ mt7621_wdt->min_timeout = 1;
+ mt7621_wdt->max_timeout = 0xfffful / 1000;
+ mt7621_wdt->parent = dev;
+
+ mt7621_wdt->bootstatus = mt7621_wdt_bootcause(drvdata);
+
+ watchdog_init_timeout(mt7621_wdt, mt7621_wdt->max_timeout, dev);
+ watchdog_set_nowayout(mt7621_wdt, nowayout);
+ watchdog_set_drvdata(mt7621_wdt, drvdata);
+
+ if (mt7621_wdt_is_running(mt7621_wdt)) {
/*
* Make sure to apply timeout from watchdog core, taking
* the prescaler of this driver here into account (the
@@ -154,17 +183,27 @@ static int mt7621_wdt_probe(struct platform_device *pdev)
* we first disable the watchdog, set the new prescaler
* and timeout, and then re-enable the watchdog.
*/
- mt7621_wdt_stop(&mt7621_wdt_dev);
- mt7621_wdt_start(&mt7621_wdt_dev);
- set_bit(WDOG_HW_RUNNING, &mt7621_wdt_dev.status);
+ mt7621_wdt_stop(mt7621_wdt);
+ mt7621_wdt_start(mt7621_wdt);
+ set_bit(WDOG_HW_RUNNING, &mt7621_wdt->status);
}
- return devm_watchdog_register_device(dev, &mt7621_wdt_dev);
+ err = devm_watchdog_register_device(dev, &drvdata->wdt);
+ if (err) {
+ dev_err(dev, "Error registering watchdog device\n");
+ return err;
+ }
+
+ platform_set_drvdata(pdev, drvdata);
+
+ return 0;
}
static void mt7621_wdt_shutdown(struct platform_device *pdev)
{
- mt7621_wdt_stop(&mt7621_wdt_dev);
+ struct mt7621_wdt_data *drvdata = platform_get_drvdata(pdev);
+
+ mt7621_wdt_stop(&drvdata->wdt);
}
static const struct of_device_id mt7621_wdt_match[] = {
--
2.25.1
On 10/02/2023 07:56, Sergio Paracuellos wrote:
> MT7621 SoC provides a system controller node for accessing to some registers.
> Add a phandle to this node to avoid using MIPS related arch operations and
I don't understand this part. You claim you add a phandle to this node,
but your binding suggest you add here a phandle to other node.
> includes in watchdog driver code.
>
> Signed-off-by: Sergio Paracuellos <[email protected]>
> ---
> .../bindings/watchdog/mediatek,mt7621-wdt.yaml | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml
> index b2b17fdf4..3c545065f 100644
> --- a/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml
> @@ -14,11 +14,18 @@ allOf:
>
> properties:
> compatible:
> - const: mediatek,mt7621-wdt
> + items:
> + - const: mediatek,mt7621-wdt
> + - const: syscon
>
> reg:
> maxItems: 1
>
> + ralink,sysctl:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description:
> + phandle of syscon used to control system registers
This needs to be more specific - which syscon? It also does not fit your
commit msg.
Best regards,
Krzysztof
On 10/02/2023 07:56, Sergio Paracuellos wrote:
> To allow to access system controller registers from watchdog driver code
> add a phandle in the watchdog 'wdt' node. This avoid using arch dependent
??? This does not make sense.
> operations in driver code.
>
> Signed-off-by: Sergio Paracuellos <[email protected]>
> ---
> arch/mips/boot/dts/ralink/mt7621.dtsi | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi
> index 5ca40fd21..ebee23a2b 100644
> --- a/arch/mips/boot/dts/ralink/mt7621.dtsi
> +++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
> @@ -71,8 +71,9 @@ sysc: syscon@0 {
> };
>
> wdt: wdt@100 {
> - compatible = "mediatek,mt7621-wdt";
> + compatible = "mediatek,mt7621-wdt", "syscon";
Why do you need syscon?
Best regards,
Krzysztof
On 10/02/2023 07:56, Sergio Paracuellos wrote:
> MT7621 SoC has a system controller node. Watchdog need to access to reset
> status register. Ralink architecture and related driver are old and from
> the beggining they ar providing some architecture dependent operations
> for accessing this shared registers through 'asm/mach-ralink/ralink_regs.h'
> header file. However this is not ideal from a driver perspective which can
> just access to the system controller registers in am arch independent way
> using regmap syscon APIs. Hence, add a new structure for driver data and
> use it along the code. This way architecture dependencies and global vars
> are not needed anymore. Update Kconfig accordingly to select new added
> dependencies and allow driver to be compile tested.
>
> Signed-off-by: Sergio Paracuellos <[email protected]>
> ---
> drivers/watchdog/Kconfig | 4 +-
> drivers/watchdog/mt7621_wdt.c | 121 ++++++++++++++++++++++------------
> 2 files changed, 83 insertions(+), 42 deletions(-)
>
> -
> static int mt7621_wdt_probe(struct platform_device *pdev)
> {
> + struct device_node *np = pdev->dev.of_node;
> struct device *dev = &pdev->dev;
> - mt7621_wdt_base = devm_platform_ioremap_resource(pdev, 0);
> - if (IS_ERR(mt7621_wdt_base))
> - return PTR_ERR(mt7621_wdt_base);
> + struct watchdog_device *mt7621_wdt;
> + struct mt7621_wdt_data *drvdata;
> + int err;
> +
> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> + if (!drvdata)
> + return -ENOMEM;
>
> - mt7621_wdt_reset = devm_reset_control_get_exclusive(dev, NULL);
> - if (!IS_ERR(mt7621_wdt_reset))
> - reset_control_deassert(mt7621_wdt_reset);
> + drvdata->sysc = syscon_regmap_lookup_by_phandle(np, "ralink,sysctl");
> + if (IS_ERR(drvdata->sysc))
> + return PTR_ERR(drvdata->sysc);
You claim in commit title that you remove some global usage, but you add
here several new features and refactor the code significantly. You need
to split refactorings, improvements from completely new features. The
entire patch is very difficult to understand in current form.
Best regards,
Krzysztof
Hi Krzysztof,
On Fri, Feb 10, 2023 at 11:59 AM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 10/02/2023 07:56, Sergio Paracuellos wrote:
> > MT7621 SoC provides a system controller node for accessing to some registers.
> > Add a phandle to this node to avoid using MIPS related arch operations and
>
> I don't understand this part. You claim you add a phandle to this node,
> but your binding suggest you add here a phandle to other node.
Probably my English is not the best here :-). Yes, you are right, I
just want to add a phandle to the 'sysc' node in the current node.
>
> > includes in watchdog driver code.
> >
> > Signed-off-by: Sergio Paracuellos <[email protected]>
> > ---
> > .../bindings/watchdog/mediatek,mt7621-wdt.yaml | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml
> > index b2b17fdf4..3c545065f 100644
> > --- a/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml
> > +++ b/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml
> > @@ -14,11 +14,18 @@ allOf:
> >
> > properties:
> > compatible:
> > - const: mediatek,mt7621-wdt
> > + items:
> > + - const: mediatek,mt7621-wdt
> > + - const: syscon
> >
> > reg:
> > maxItems: 1
> >
> > + ralink,sysctl:
> > + $ref: /schemas/types.yaml#/definitions/phandle
> > + description:
> > + phandle of syscon used to control system registers
>
> This needs to be more specific - which syscon? It also does not fit your
> commit msg.
Sure. How about "phandle to system controller 'sysc' syscon node which
controls system registers".
>
>
> Best regards,
> Krzysztof
>
Thanks,
Sergio Paracuellos
On 10/02/2023 12:22, Sergio Paracuellos wrote:
> Hi Krzysztof,
>
> On Fri, Feb 10, 2023 at 11:59 AM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 10/02/2023 07:56, Sergio Paracuellos wrote:
>>> MT7621 SoC provides a system controller node for accessing to some registers.
>>> Add a phandle to this node to avoid using MIPS related arch operations and
>>
>> I don't understand this part. You claim you add a phandle to this node,
>> but your binding suggest you add here a phandle to other node.
>
> Probably my English is not the best here :-). Yes, you are right, I
> just want to add a phandle to the 'sysc' node in the current node.
Then why do you need syscon compatible here?
Best regards,
Krzysztof
Hi Krzysztof,
On Fri, Feb 10, 2023 at 12:00 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 10/02/2023 07:56, Sergio Paracuellos wrote:
> > To allow to access system controller registers from watchdog driver code
> > add a phandle in the watchdog 'wdt' node. This avoid using arch dependent
>
> ??? This does not make sense.
What do you mean? The commit message itself? I need the phandle to
'sysc' system controller node for accessing reset status registers
inside the watchdog driver code.
>
> > operations in driver code.
> >
> > Signed-off-by: Sergio Paracuellos <[email protected]>
> > ---
> > arch/mips/boot/dts/ralink/mt7621.dtsi | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi
> > index 5ca40fd21..ebee23a2b 100644
> > --- a/arch/mips/boot/dts/ralink/mt7621.dtsi
> > +++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
> > @@ -71,8 +71,9 @@ sysc: syscon@0 {
> > };
> >
> > wdt: wdt@100 {
> > - compatible = "mediatek,mt7621-wdt";
> > + compatible = "mediatek,mt7621-wdt", "syscon";
>
> Why do you need syscon?
True, will drop, thanks!
>
> Best regards,
> Krzysztof
>
Best regards,
Sergio Paracuellos
On 10/02/2023 12:29, Sergio Paracuellos wrote:
> Hi Krzysztof,
>
> On Fri, Feb 10, 2023 at 12:00 PM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 10/02/2023 07:56, Sergio Paracuellos wrote:
>>> To allow to access system controller registers from watchdog driver code
>>> add a phandle in the watchdog 'wdt' node. This avoid using arch dependent
>>
>> ??? This does not make sense.
>
> What do you mean? The commit message itself? I need the phandle to
> 'sysc' system controller node for accessing reset status registers
> inside the watchdog driver code.
The message makes sense. The message for the code does not make anymore.
I meant, you want to access system controller registers from watchdog,
so you add syscon to watchdog...
Best regards,
Krzysztof
Hi Krzysztof,
On Fri, Feb 10, 2023 at 12:02 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 10/02/2023 07:56, Sergio Paracuellos wrote:
> > MT7621 SoC has a system controller node. Watchdog need to access to reset
> > status register. Ralink architecture and related driver are old and from
> > the beggining they ar providing some architecture dependent operations
> > for accessing this shared registers through 'asm/mach-ralink/ralink_regs.h'
> > header file. However this is not ideal from a driver perspective which can
> > just access to the system controller registers in am arch independent way
> > using regmap syscon APIs. Hence, add a new structure for driver data and
> > use it along the code. This way architecture dependencies and global vars
> > are not needed anymore. Update Kconfig accordingly to select new added
> > dependencies and allow driver to be compile tested.
> >
> > Signed-off-by: Sergio Paracuellos <[email protected]>
> > ---
> > drivers/watchdog/Kconfig | 4 +-
> > drivers/watchdog/mt7621_wdt.c | 121 ++++++++++++++++++++++------------
> > 2 files changed, 83 insertions(+), 42 deletions(-)
> >
>
>
> > -
> > static int mt7621_wdt_probe(struct platform_device *pdev)
> > {
> > + struct device_node *np = pdev->dev.of_node;
> > struct device *dev = &pdev->dev;
> > - mt7621_wdt_base = devm_platform_ioremap_resource(pdev, 0);
> > - if (IS_ERR(mt7621_wdt_base))
> > - return PTR_ERR(mt7621_wdt_base);
> > + struct watchdog_device *mt7621_wdt;
> > + struct mt7621_wdt_data *drvdata;
> > + int err;
> > +
> > + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> > + if (!drvdata)
> > + return -ENOMEM;
> >
> > - mt7621_wdt_reset = devm_reset_control_get_exclusive(dev, NULL);
> > - if (!IS_ERR(mt7621_wdt_reset))
> > - reset_control_deassert(mt7621_wdt_reset);
> > + drvdata->sysc = syscon_regmap_lookup_by_phandle(np, "ralink,sysctl");
> > + if (IS_ERR(drvdata->sysc))
> > + return PTR_ERR(drvdata->sysc);
>
> You claim in commit title that you remove some global usage, but you add
> here several new features and refactor the code significantly. You need
> to split refactorings, improvements from completely new features. The
> entire patch is very difficult to understand in current form.
I am removing global usage and architecture dependencies using a
watchdog driver data structure so I thought the changes were easy
enough to review in this way. It seems they are not according to your
reply :). If preferred I can split this in two commits:
- Avoid globals using and introducing all the related new driver data
structure.
- Add request for regmap syscon from the phandle and remove the
architecture dependent calls and includes.
Thanks in advance for your comments.
Best regards,
Sergio Paracuellos
>
> Best regards,
> Krzysztof
>
On Fri, Feb 10, 2023 at 12:31 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 10/02/2023 12:29, Sergio Paracuellos wrote:
> > Hi Krzysztof,
> >
> > On Fri, Feb 10, 2023 at 12:00 PM Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 10/02/2023 07:56, Sergio Paracuellos wrote:
> >>> To allow to access system controller registers from watchdog driver code
> >>> add a phandle in the watchdog 'wdt' node. This avoid using arch dependent
> >>
> >> ??? This does not make sense.
> >
> > What do you mean? The commit message itself? I need the phandle to
> > 'sysc' system controller node for accessing reset status registers
> > inside the watchdog driver code.
>
> The message makes sense. The message for the code does not make anymore.
> I meant, you want to access system controller registers from watchdog,
> so you add syscon to watchdog...
I got your point now, thanks. Will remove the syscon compatible from
the watchdog node.
>
> Best regards,
> Krzysztof
>
Best regards,
Sergio Paracuellos
On Fri, Feb 10, 2023 at 12:27 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 10/02/2023 12:22, Sergio Paracuellos wrote:
> > Hi Krzysztof,
> >
> > On Fri, Feb 10, 2023 at 11:59 AM Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 10/02/2023 07:56, Sergio Paracuellos wrote:
> >>> MT7621 SoC provides a system controller node for accessing to some registers.
> >>> Add a phandle to this node to avoid using MIPS related arch operations and
> >>
> >> I don't understand this part. You claim you add a phandle to this node,
> >> but your binding suggest you add here a phandle to other node.
> >
> > Probably my English is not the best here :-). Yes, you are right, I
> > just want to add a phandle to the 'sysc' node in the current node.
>
> Then why do you need syscon compatible here?
Clear now. Will drop that in v2.
>
> Best regards,
> Krzysztof
>
Thanks,
Sergio Paracuellos
On 10/02/2023 12:35, Sergio Paracuellos wrote:
>>> - mt7621_wdt_reset = devm_reset_control_get_exclusive(dev, NULL);
>>> - if (!IS_ERR(mt7621_wdt_reset))
>>> - reset_control_deassert(mt7621_wdt_reset);
>>> + drvdata->sysc = syscon_regmap_lookup_by_phandle(np, "ralink,sysctl");
>>> + if (IS_ERR(drvdata->sysc))
>>> + return PTR_ERR(drvdata->sysc);
>>
>> You claim in commit title that you remove some global usage, but you add
>> here several new features and refactor the code significantly. You need
>> to split refactorings, improvements from completely new features. The
>> entire patch is very difficult to understand in current form.
>
> I am removing global usage and architecture dependencies using a
> watchdog driver data structure so I thought the changes were easy
> enough to review in this way. It seems they are not according to your
> reply :). If preferred I can split this in two commits:
> - Avoid globals using and introducing all the related new driver data
> structure.
> - Add request for regmap syscon from the phandle and remove the
> architecture dependent calls and includes.
>
Yes, such split sounds better. Thanks.
Best regards,
Krzysztof