Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] watchdog: bcm7038_wdt: add big endian support
@ 2021-02-22 20:03 Álvaro Fernández Rojas
  2021-02-22 21:24 ` Guenter Roeck
  2021-02-23  8:00 ` [PATCH v2] " Álvaro Fernández Rojas
  0 siblings, 2 replies; 8+ messages in thread
From: Álvaro Fernández Rojas @ 2021-02-22 20:03 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck, linux-watchdog, linux-kernel
  Cc: Álvaro Fernández Rojas

bcm7038_wdt can be used on bmips (bcm63xx) devices too.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 drivers/watchdog/bcm7038_wdt.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/bcm7038_wdt.c b/drivers/watchdog/bcm7038_wdt.c
index 979caa18d3c8..62494da1ac57 100644
--- a/drivers/watchdog/bcm7038_wdt.c
+++ b/drivers/watchdog/bcm7038_wdt.c
@@ -34,6 +34,24 @@ struct bcm7038_watchdog {
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 
+static inline void bcm7038_wdt_write(unsigned long data, void __iomem *reg)
+{
+#ifdef CONFIG_CPU_BIG_ENDIAN
+	__raw_writel(data, reg);
+#else
+	writel(data, reg);
+#endif
+}
+
+static inline unsigned long bcm7038_wdt_read(void __iomem *reg)
+{
+#ifdef CONFIG_CPU_BIG_ENDIAN
+	return __raw_readl(reg);
+#else
+	return readl(reg);
+#endif
+}
+
 static void bcm7038_wdt_set_timeout_reg(struct watchdog_device *wdog)
 {
 	struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
@@ -41,15 +59,15 @@ static void bcm7038_wdt_set_timeout_reg(struct watchdog_device *wdog)
 
 	timeout = wdt->rate * wdog->timeout;
 
-	writel(timeout, wdt->base + WDT_TIMEOUT_REG);
+	bcm7038_wdt_write(timeout, wdt->base + WDT_TIMEOUT_REG);
 }
 
 static int bcm7038_wdt_ping(struct watchdog_device *wdog)
 {
 	struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
 
-	writel(WDT_START_1, wdt->base + WDT_CMD_REG);
-	writel(WDT_START_2, wdt->base + WDT_CMD_REG);
+	bcm7038_wdt_write(WDT_START_1, wdt->base + WDT_CMD_REG);
+	bcm7038_wdt_write(WDT_START_2, wdt->base + WDT_CMD_REG);
 
 	return 0;
 }
@@ -66,8 +84,8 @@ static int bcm7038_wdt_stop(struct watchdog_device *wdog)
 {
 	struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
 
-	writel(WDT_STOP_1, wdt->base + WDT_CMD_REG);
-	writel(WDT_STOP_2, wdt->base + WDT_CMD_REG);
+	bcm7038_wdt_write(WDT_STOP_1, wdt->base + WDT_CMD_REG);
+	bcm7038_wdt_write(WDT_STOP_2, wdt->base + WDT_CMD_REG);
 
 	return 0;
 }
@@ -88,7 +106,7 @@ static unsigned int bcm7038_wdt_get_timeleft(struct watchdog_device *wdog)
 	struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
 	u32 time_left;
 
-	time_left = readl(wdt->base + WDT_CMD_REG);
+	time_left = bcm7038_wdt_read(wdt->base + WDT_CMD_REG);
 
 	return time_left / wdt->rate;
 }
-- 
2.20.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] watchdog: bcm7038_wdt: add big endian support
  2021-02-22 20:03 [PATCH] watchdog: bcm7038_wdt: add big endian support Álvaro Fernández Rojas
@ 2021-02-22 21:24 ` Guenter Roeck
  2021-02-22 21:48   ` Álvaro Fernández Rojas
  2021-02-23  8:00 ` [PATCH v2] " Álvaro Fernández Rojas
  1 sibling, 1 reply; 8+ messages in thread
From: Guenter Roeck @ 2021-02-22 21:24 UTC (permalink / raw)
  To: Álvaro Fernández Rojas, Wim Van Sebroeck,
	linux-watchdog, linux-kernel

On 2/22/21 12:03 PM, Álvaro Fernández Rojas wrote:
> bcm7038_wdt can be used on bmips (bcm63xx) devices too.
> 
It might make sense to actually enable it for BCM63XX.

> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> ---
>  drivers/watchdog/bcm7038_wdt.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/bcm7038_wdt.c b/drivers/watchdog/bcm7038_wdt.c
> index 979caa18d3c8..62494da1ac57 100644
> --- a/drivers/watchdog/bcm7038_wdt.c
> +++ b/drivers/watchdog/bcm7038_wdt.c
> @@ -34,6 +34,24 @@ struct bcm7038_watchdog {                                 
>  
>  static bool nowayout = WATCHDOG_NOWAYOUT;
>  
> +static inline void bcm7038_wdt_write(unsigned long data, void __iomem *reg)
> +{
> +#ifdef CONFIG_CPU_BIG_ENDIAN
> +	__raw_writel(data, reg);
> +#else
> +	writel(data, reg);
> +#endif
> +}
> +
> +static inline unsigned long bcm7038_wdt_read(void __iomem *reg)
> +{
> +#ifdef CONFIG_CPU_BIG_ENDIAN
> +	return __raw_readl(reg);
> +#else
> +	return readl(reg);
> +#endif
> +}
> +

This needs further explanation. Why not just use __raw_writel() and
__raw_readl() unconditionally ? Also, is it known for sure that,
say, bmips_be_defconfig otherwise uses the wrong endianness
(vs. bmips_stb_defconfig which is a little endian configuration) ?

Thanks,
Guenter

>  static void bcm7038_wdt_set_timeout_reg(struct watchdog_device *wdog)
>  {
>  	struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
> @@ -41,15 +59,15 @@ static void bcm7038_wdt_set_timeout_reg(struct watchdog_device *wdog)
>  
>  	timeout = wdt->rate * wdog->timeout;
>  
> -	writel(timeout, wdt->base + WDT_TIMEOUT_REG);
> +	bcm7038_wdt_write(timeout, wdt->base + WDT_TIMEOUT_REG);
>  }
>  
>  static int bcm7038_wdt_ping(struct watchdog_device *wdog)
>  {
>  	struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
>  
> -	writel(WDT_START_1, wdt->base + WDT_CMD_REG);
> -	writel(WDT_START_2, wdt->base + WDT_CMD_REG);
> +	bcm7038_wdt_write(WDT_START_1, wdt->base + WDT_CMD_REG);
> +	bcm7038_wdt_write(WDT_START_2, wdt->base + WDT_CMD_REG);
>  
>  	return 0;
>  }
> @@ -66,8 +84,8 @@ static int bcm7038_wdt_stop(struct watchdog_device *wdog)
>  {
>  	struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
>  
> -	writel(WDT_STOP_1, wdt->base + WDT_CMD_REG);
> -	writel(WDT_STOP_2, wdt->base + WDT_CMD_REG);
> +	bcm7038_wdt_write(WDT_STOP_1, wdt->base + WDT_CMD_REG);
> +	bcm7038_wdt_write(WDT_STOP_2, wdt->base + WDT_CMD_REG);
>  
>  	return 0;
>  }
> @@ -88,7 +106,7 @@ static unsigned int bcm7038_wdt_get_timeleft(struct watchdog_device *wdog)
>  	struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
>  	u32 time_left;
>  
> -	time_left = readl(wdt->base + WDT_CMD_REG);
> +	time_left = bcm7038_wdt_read(wdt->base + WDT_CMD_REG);
>  
>  	return time_left / wdt->rate;
>  }
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] watchdog: bcm7038_wdt: add big endian support
  2021-02-22 21:24 ` Guenter Roeck
@ 2021-02-22 21:48   ` Álvaro Fernández Rojas
  2021-02-22 22:24     ` Guenter Roeck
  0 siblings, 1 reply; 8+ messages in thread
From: Álvaro Fernández Rojas @ 2021-02-22 21:48 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Wim Van Sebroeck, linux-watchdog, linux-kernel

Hi Guenter,

> El 22 feb 2021, a las 22:24, Guenter Roeck <linux@roeck-us.net> escribió:
> 
> On 2/22/21 12:03 PM, Álvaro Fernández Rojas wrote:
>> bcm7038_wdt can be used on bmips (bcm63xx) devices too.
>> 
> It might make sense to actually enable it for BCM63XX.

bcm63xx SoCs are supported in bcm63xx and bmips.
bcm63xx doesn’t have device tree support, but bmips does and this watchdog is already enabled for bmips.

> 
>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>> ---
>> drivers/watchdog/bcm7038_wdt.c | 30 ++++++++++++++++++++++++------
>> 1 file changed, 24 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/watchdog/bcm7038_wdt.c b/drivers/watchdog/bcm7038_wdt.c
>> index 979caa18d3c8..62494da1ac57 100644
>> --- a/drivers/watchdog/bcm7038_wdt.c
>> +++ b/drivers/watchdog/bcm7038_wdt.c
>> @@ -34,6 +34,24 @@ struct bcm7038_watchdog {                                 
>> 
>> static bool nowayout = WATCHDOG_NOWAYOUT;
>> 
>> +static inline void bcm7038_wdt_write(unsigned long data, void __iomem *reg)
>> +{
>> +#ifdef CONFIG_CPU_BIG_ENDIAN
>> +    __raw_writel(data, reg);
>> +#else
>> +    writel(data, reg);
>> +#endif
>> +}
>> +
>> +static inline unsigned long bcm7038_wdt_read(void __iomem *reg)
>> +{
>> +#ifdef CONFIG_CPU_BIG_ENDIAN
>> +    return __raw_readl(reg);
>> +#else
>> +    return readl(reg);
>> +#endif
>> +}
>> +
> 
> This needs further explanation. Why not just use __raw_writel() and
> __raw_readl() unconditionally ? Also, is it known for sure that,
> say, bmips_be_defconfig otherwise uses the wrong endianness
> (vs. bmips_stb_defconfig which is a little endian configuration) ?

Because __raw_writel() doesn’t have memory barriers and writel() does.
Those configs use the correct endiannes, so I don’t know what you mean...

> 
> Thanks,
> Guenter
> 
>> static void bcm7038_wdt_set_timeout_reg(struct watchdog_device *wdog)
>> {
>>    struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
>> @@ -41,15 +59,15 @@ static void bcm7038_wdt_set_timeout_reg(struct watchdog_device *wdog)
>> 
>>    timeout = wdt->rate * wdog->timeout;
>> 
>> -    writel(timeout, wdt->base + WDT_TIMEOUT_REG);
>> +    bcm7038_wdt_write(timeout, wdt->base + WDT_TIMEOUT_REG);
>> }
>> 
>> static int bcm7038_wdt_ping(struct watchdog_device *wdog)
>> {
>>    struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
>> 
>> -    writel(WDT_START_1, wdt->base + WDT_CMD_REG);
>> -    writel(WDT_START_2, wdt->base + WDT_CMD_REG);
>> +    bcm7038_wdt_write(WDT_START_1, wdt->base + WDT_CMD_REG);
>> +    bcm7038_wdt_write(WDT_START_2, wdt->base + WDT_CMD_REG);
>> 
>>    return 0;
>> }
>> @@ -66,8 +84,8 @@ static int bcm7038_wdt_stop(struct watchdog_device *wdog)
>> {
>>    struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
>> 
>> -    writel(WDT_STOP_1, wdt->base + WDT_CMD_REG);
>> -    writel(WDT_STOP_2, wdt->base + WDT_CMD_REG);
>> +    bcm7038_wdt_write(WDT_STOP_1, wdt->base + WDT_CMD_REG);
>> +    bcm7038_wdt_write(WDT_STOP_2, wdt->base + WDT_CMD_REG);
>> 
>>    return 0;
>> }
>> @@ -88,7 +106,7 @@ static unsigned int bcm7038_wdt_get_timeleft(struct watchdog_device *wdog)
>>    struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
>>    u32 time_left;
>> 
>> -    time_left = readl(wdt->base + WDT_CMD_REG);
>> +    time_left = bcm7038_wdt_read(wdt->base + WDT_CMD_REG);
>> 
>>    return time_left / wdt->rate;
>> }
>> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] watchdog: bcm7038_wdt: add big endian support
  2021-02-22 21:48   ` Álvaro Fernández Rojas
@ 2021-02-22 22:24     ` Guenter Roeck
  2021-02-23  3:41       ` Florian Fainelli
  0 siblings, 1 reply; 8+ messages in thread
From: Guenter Roeck @ 2021-02-22 22:24 UTC (permalink / raw)
  To: Álvaro Fernández Rojas
  Cc: Wim Van Sebroeck, linux-watchdog, linux-kernel

On Mon, Feb 22, 2021 at 10:48:09PM +0100, Álvaro Fernández Rojas wrote:
> Hi Guenter,
> 
> > El 22 feb 2021, a las 22:24, Guenter Roeck <linux@roeck-us.net> escribió:
> > 
> > On 2/22/21 12:03 PM, Álvaro Fernández Rojas wrote:
> >> bcm7038_wdt can be used on bmips (bcm63xx) devices too.
> >> 
> > It might make sense to actually enable it for BCM63XX.
> 
> bcm63xx SoCs are supported in bcm63xx and bmips.
> bcm63xx doesn’t have device tree support, but bmips does and this watchdog is already enabled for bmips.
> 

Maybe add a note saying that this will only be supported for devicetree
based systems.

> > 
> >> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> >> ---
> >> drivers/watchdog/bcm7038_wdt.c | 30 ++++++++++++++++++++++++------
> >> 1 file changed, 24 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/drivers/watchdog/bcm7038_wdt.c b/drivers/watchdog/bcm7038_wdt.c
> >> index 979caa18d3c8..62494da1ac57 100644
> >> --- a/drivers/watchdog/bcm7038_wdt.c
> >> +++ b/drivers/watchdog/bcm7038_wdt.c
> >> @@ -34,6 +34,24 @@ struct bcm7038_watchdog {                                 
> >> 
> >> static bool nowayout = WATCHDOG_NOWAYOUT;
> >> 
> >> +static inline void bcm7038_wdt_write(unsigned long data, void __iomem *reg)
> >> +{
> >> +#ifdef CONFIG_CPU_BIG_ENDIAN
> >> +    __raw_writel(data, reg);
> >> +#else
> >> +    writel(data, reg);
> >> +#endif
> >> +}
> >> +
> >> +static inline unsigned long bcm7038_wdt_read(void __iomem *reg)
> >> +{
> >> +#ifdef CONFIG_CPU_BIG_ENDIAN
> >> +    return __raw_readl(reg);
> >> +#else
> >> +    return readl(reg);
> >> +#endif
> >> +}
> >> +
> > 
> > This needs further explanation. Why not just use __raw_writel() and
> > __raw_readl() unconditionally ? Also, is it known for sure that,
> > say, bmips_be_defconfig otherwise uses the wrong endianness
> > (vs. bmips_stb_defconfig which is a little endian configuration) ?
> 
> Because __raw_writel() doesn’t have memory barriers and writel() does.
> Those configs use the correct endiannes, so I don’t know what you mean...
> 
So are you saying that it already works with bmips_stb_defconfig 
(because it is little endian), that bmips_stb_defconfig needs memory
barriers, and that bmips_be_defconfig doesn't need memory barriers ?
Odd, but I'll take you by your word. And other code does something
similar, so I guess there must be a reason for it.

Anyway, after looking into that other code, please use something like

        if (IS_ENABLED(CONFIG_MIPS) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
                __raw_writel(value, reg);
        else
                writel(value, reg);

Thanks,
Guenter

> > 
> > Thanks,
> > Guenter
> > 
> >> static void bcm7038_wdt_set_timeout_reg(struct watchdog_device *wdog)
> >> {
> >>    struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
> >> @@ -41,15 +59,15 @@ static void bcm7038_wdt_set_timeout_reg(struct watchdog_device *wdog)
> >> 
> >>    timeout = wdt->rate * wdog->timeout;
> >> 
> >> -    writel(timeout, wdt->base + WDT_TIMEOUT_REG);
> >> +    bcm7038_wdt_write(timeout, wdt->base + WDT_TIMEOUT_REG);
> >> }
> >> 
> >> static int bcm7038_wdt_ping(struct watchdog_device *wdog)
> >> {
> >>    struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
> >> 
> >> -    writel(WDT_START_1, wdt->base + WDT_CMD_REG);
> >> -    writel(WDT_START_2, wdt->base + WDT_CMD_REG);
> >> +    bcm7038_wdt_write(WDT_START_1, wdt->base + WDT_CMD_REG);
> >> +    bcm7038_wdt_write(WDT_START_2, wdt->base + WDT_CMD_REG);
> >> 
> >>    return 0;
> >> }
> >> @@ -66,8 +84,8 @@ static int bcm7038_wdt_stop(struct watchdog_device *wdog)
> >> {
> >>    struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
> >> 
> >> -    writel(WDT_STOP_1, wdt->base + WDT_CMD_REG);
> >> -    writel(WDT_STOP_2, wdt->base + WDT_CMD_REG);
> >> +    bcm7038_wdt_write(WDT_STOP_1, wdt->base + WDT_CMD_REG);
> >> +    bcm7038_wdt_write(WDT_STOP_2, wdt->base + WDT_CMD_REG);
> >> 
> >>    return 0;
> >> }
> >> @@ -88,7 +106,7 @@ static unsigned int bcm7038_wdt_get_timeleft(struct watchdog_device *wdog)
> >>    struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
> >>    u32 time_left;
> >> 
> >> -    time_left = readl(wdt->base + WDT_CMD_REG);
> >> +    time_left = bcm7038_wdt_read(wdt->base + WDT_CMD_REG);
> >> 
> >>    return time_left / wdt->rate;
> >> }
> >> 
> > 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] watchdog: bcm7038_wdt: add big endian support
  2021-02-22 22:24     ` Guenter Roeck
@ 2021-02-23  3:41       ` Florian Fainelli
  2021-02-23  3:58         ` Florian Fainelli
  0 siblings, 1 reply; 8+ messages in thread
From: Florian Fainelli @ 2021-02-23  3:41 UTC (permalink / raw)
  To: Guenter Roeck, Álvaro Fernández Rojas
  Cc: Wim Van Sebroeck, linux-watchdog, linux-kernel



On 2/22/2021 2:24 PM, Guenter Roeck wrote:
> On Mon, Feb 22, 2021 at 10:48:09PM +0100, Álvaro Fernández Rojas wrote:
>> Hi Guenter,
>>
>>> El 22 feb 2021, a las 22:24, Guenter Roeck <linux@roeck-us.net> escribió:
>>>
>>> On 2/22/21 12:03 PM, Álvaro Fernández Rojas wrote:
>>>> bcm7038_wdt can be used on bmips (bcm63xx) devices too.
>>>>
>>> It might make sense to actually enable it for BCM63XX.
>>
>> bcm63xx SoCs are supported in bcm63xx and bmips.
>> bcm63xx doesn’t have device tree support, but bmips does and this watchdog is already enabled for bmips.
>>
> 
> Maybe add a note saying that this will only be supported for devicetree
> based systems.
> 
>>>
>>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>>>> ---
>>>> drivers/watchdog/bcm7038_wdt.c | 30 ++++++++++++++++++++++++------
>>>> 1 file changed, 24 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/bcm7038_wdt.c b/drivers/watchdog/bcm7038_wdt.c
>>>> index 979caa18d3c8..62494da1ac57 100644
>>>> --- a/drivers/watchdog/bcm7038_wdt.c
>>>> +++ b/drivers/watchdog/bcm7038_wdt.c
>>>> @@ -34,6 +34,24 @@ struct bcm7038_watchdog {                                 
>>>>
>>>> static bool nowayout = WATCHDOG_NOWAYOUT;
>>>>
>>>> +static inline void bcm7038_wdt_write(unsigned long data, void __iomem *reg)
>>>> +{
>>>> +#ifdef CONFIG_CPU_BIG_ENDIAN
>>>> +    __raw_writel(data, reg);
>>>> +#else
>>>> +    writel(data, reg);
>>>> +#endif
>>>> +}
>>>> +
>>>> +static inline unsigned long bcm7038_wdt_read(void __iomem *reg)
>>>> +{
>>>> +#ifdef CONFIG_CPU_BIG_ENDIAN
>>>> +    return __raw_readl(reg);
>>>> +#else
>>>> +    return readl(reg);
>>>> +#endif
>>>> +}
>>>> +
>>>
>>> This needs further explanation. Why not just use __raw_writel() and
>>> __raw_readl() unconditionally ? Also, is it known for sure that,
>>> say, bmips_be_defconfig otherwise uses the wrong endianness
>>> (vs. bmips_stb_defconfig which is a little endian configuration) ?
>>
>> Because __raw_writel() doesn’t have memory barriers and writel() does.
>> Those configs use the correct endiannes, so I don’t know what you mean...
>>
> So are you saying that it already works with bmips_stb_defconfig 
> (because it is little endian), that bmips_stb_defconfig needs memory
> barriers, and that bmips_be_defconfig doesn't need memory barriers ?
> Odd, but I'll take you by your word. And other code does something
> similar, so I guess there must be a reason for it.

It would be so nice to copy people, and the author of that driver who
could give you such an answer. Neither bmips_be_defconfig nor
bmips_stb_defconfig require barrier because the bus interface towards
registers that is used on those SoCs is non-reordering non-buffered.

> 
> Anyway, after looking into that other code, please use something like
> 
>         if (IS_ENABLED(CONFIG_MIPS) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>                 __raw_writel(value, reg);
>         else
>                 writel(value, reg);

Yes please.
-- 
Florian

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] watchdog: bcm7038_wdt: add big endian support
  2021-02-23  3:41       ` Florian Fainelli
@ 2021-02-23  3:58         ` Florian Fainelli
  0 siblings, 0 replies; 8+ messages in thread
From: Florian Fainelli @ 2021-02-23  3:58 UTC (permalink / raw)
  To: Guenter Roeck, Álvaro Fernández Rojas
  Cc: Wim Van Sebroeck, linux-watchdog, linux-kernel



On 2/22/2021 7:41 PM, Florian Fainelli wrote:
> 
> 
> On 2/22/2021 2:24 PM, Guenter Roeck wrote:
>> On Mon, Feb 22, 2021 at 10:48:09PM +0100, Álvaro Fernández Rojas wrote:
>>> Hi Guenter,
>>>
>>>> El 22 feb 2021, a las 22:24, Guenter Roeck <linux@roeck-us.net> escribió:
>>>>
>>>> On 2/22/21 12:03 PM, Álvaro Fernández Rojas wrote:
>>>>> bcm7038_wdt can be used on bmips (bcm63xx) devices too.
>>>>>
>>>> It might make sense to actually enable it for BCM63XX.
>>>
>>> bcm63xx SoCs are supported in bcm63xx and bmips.
>>> bcm63xx doesn’t have device tree support, but bmips does and this watchdog is already enabled for bmips.
>>>
>>
>> Maybe add a note saying that this will only be supported for devicetree
>> based systems.
>>
>>>>
>>>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>>>>> ---
>>>>> drivers/watchdog/bcm7038_wdt.c | 30 ++++++++++++++++++++++++------
>>>>> 1 file changed, 24 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/watchdog/bcm7038_wdt.c b/drivers/watchdog/bcm7038_wdt.c
>>>>> index 979caa18d3c8..62494da1ac57 100644
>>>>> --- a/drivers/watchdog/bcm7038_wdt.c
>>>>> +++ b/drivers/watchdog/bcm7038_wdt.c
>>>>> @@ -34,6 +34,24 @@ struct bcm7038_watchdog {                                 
>>>>>
>>>>> static bool nowayout = WATCHDOG_NOWAYOUT;
>>>>>
>>>>> +static inline void bcm7038_wdt_write(unsigned long data, void __iomem *reg)
>>>>> +{
>>>>> +#ifdef CONFIG_CPU_BIG_ENDIAN
>>>>> +    __raw_writel(data, reg);
>>>>> +#else
>>>>> +    writel(data, reg);
>>>>> +#endif
>>>>> +}
>>>>> +
>>>>> +static inline unsigned long bcm7038_wdt_read(void __iomem *reg)
>>>>> +{
>>>>> +#ifdef CONFIG_CPU_BIG_ENDIAN
>>>>> +    return __raw_readl(reg);
>>>>> +#else
>>>>> +    return readl(reg);
>>>>> +#endif
>>>>> +}
>>>>> +
>>>>
>>>> This needs further explanation. Why not just use __raw_writel() and
>>>> __raw_readl() unconditionally ? Also, is it known for sure that,
>>>> say, bmips_be_defconfig otherwise uses the wrong endianness
>>>> (vs. bmips_stb_defconfig which is a little endian configuration) ?
>>>
>>> Because __raw_writel() doesn’t have memory barriers and writel() does.
>>> Those configs use the correct endiannes, so I don’t know what you mean...
>>>
>> So are you saying that it already works with bmips_stb_defconfig 
>> (because it is little endian), that bmips_stb_defconfig needs memory
>> barriers, and that bmips_be_defconfig doesn't need memory barriers ?
>> Odd, but I'll take you by your word. And other code does something
>> similar, so I guess there must be a reason for it.
> 
> It would be so nice to copy people, and the author of that driver who
> could give you such an answer. Neither bmips_be_defconfig nor
> bmips_stb_defconfig require barrier because the bus interface towards
> registers that is used on those SoCs is non-reordering non-buffered.

I should mention though that using __raw_writel() with an ARM big-endian
would not work which is why {read,write}l_relaxed() should be preferred
such that all combinations work. A good example that has been proven to
work on BMIPS BE/LE and ARM BE/LE is bcmgenet.c
-- 
Florian

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] watchdog: bcm7038_wdt: add big endian support
  2021-02-22 20:03 [PATCH] watchdog: bcm7038_wdt: add big endian support Álvaro Fernández Rojas
  2021-02-22 21:24 ` Guenter Roeck
@ 2021-02-23  8:00 ` Álvaro Fernández Rojas
  2021-02-23 15:19   ` Guenter Roeck
  1 sibling, 1 reply; 8+ messages in thread
From: Álvaro Fernández Rojas @ 2021-02-23  8:00 UTC (permalink / raw)
  To: f.fainelli, Wim Van Sebroeck, Guenter Roeck, linux-watchdog,
	linux-kernel
  Cc: Álvaro Fernández Rojas

bcm7038_wdt can be used on bmips big endian (bcm63xx) devices too.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 v2: use approach indicated by Florian (same as bcmgenet.c)

 drivers/watchdog/bcm7038_wdt.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/bcm7038_wdt.c b/drivers/watchdog/bcm7038_wdt.c
index 979caa18d3c8..acaaa0005d5b 100644
--- a/drivers/watchdog/bcm7038_wdt.c
+++ b/drivers/watchdog/bcm7038_wdt.c
@@ -34,6 +34,25 @@ struct bcm7038_watchdog {
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 
+static inline void bcm7038_wdt_write(u32 value, void __iomem *addr)
+{
+	/* MIPS chips strapped for BE will automagically configure the
+	 * peripheral registers for CPU-native byte order.
+	 */
+	if (IS_ENABLED(CONFIG_MIPS) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
+		__raw_writel(value, addr);
+	else
+		writel_relaxed(value, addr);
+}
+
+static inline u32 bcm7038_wdt_read(void __iomem *addr)
+{
+	if (IS_ENABLED(CONFIG_MIPS) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
+		return __raw_readl(addr);
+	else
+		return readl_relaxed(addr);
+}
+
 static void bcm7038_wdt_set_timeout_reg(struct watchdog_device *wdog)
 {
 	struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
@@ -41,15 +60,15 @@ static void bcm7038_wdt_set_timeout_reg(struct watchdog_device *wdog)
 
 	timeout = wdt->rate * wdog->timeout;
 
-	writel(timeout, wdt->base + WDT_TIMEOUT_REG);
+	bcm7038_wdt_write(timeout, wdt->base + WDT_TIMEOUT_REG);
 }
 
 static int bcm7038_wdt_ping(struct watchdog_device *wdog)
 {
 	struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
 
-	writel(WDT_START_1, wdt->base + WDT_CMD_REG);
-	writel(WDT_START_2, wdt->base + WDT_CMD_REG);
+	bcm7038_wdt_write(WDT_START_1, wdt->base + WDT_CMD_REG);
+	bcm7038_wdt_write(WDT_START_2, wdt->base + WDT_CMD_REG);
 
 	return 0;
 }
@@ -66,8 +85,8 @@ static int bcm7038_wdt_stop(struct watchdog_device *wdog)
 {
 	struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
 
-	writel(WDT_STOP_1, wdt->base + WDT_CMD_REG);
-	writel(WDT_STOP_2, wdt->base + WDT_CMD_REG);
+	bcm7038_wdt_write(WDT_STOP_1, wdt->base + WDT_CMD_REG);
+	bcm7038_wdt_write(WDT_STOP_2, wdt->base + WDT_CMD_REG);
 
 	return 0;
 }
@@ -88,7 +107,7 @@ static unsigned int bcm7038_wdt_get_timeleft(struct watchdog_device *wdog)
 	struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
 	u32 time_left;
 
-	time_left = readl(wdt->base + WDT_CMD_REG);
+	time_left = bcm7038_wdt_read(wdt->base + WDT_CMD_REG);
 
 	return time_left / wdt->rate;
 }
-- 
2.20.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] watchdog: bcm7038_wdt: add big endian support
  2021-02-23  8:00 ` [PATCH v2] " Álvaro Fernández Rojas
@ 2021-02-23 15:19   ` Guenter Roeck
  0 siblings, 0 replies; 8+ messages in thread
From: Guenter Roeck @ 2021-02-23 15:19 UTC (permalink / raw)
  To: Álvaro Fernández Rojas, f.fainelli, Wim Van Sebroeck,
	linux-watchdog, linux-kernel

On 2/23/21 12:00 AM, Álvaro Fernández Rojas wrote:
> bcm7038_wdt can be used on bmips big endian (bcm63xx) devices too.
> 
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

With the assumption that the driver indeed does not need memory
barriers,

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-22 20:03 [PATCH] watchdog: bcm7038_wdt: add big endian support Álvaro Fernández Rojas
2021-02-22 21:24 ` Guenter Roeck
2021-02-22 21:48   ` Álvaro Fernández Rojas
2021-02-22 22:24     ` Guenter Roeck
2021-02-23  3:41       ` Florian Fainelli
2021-02-23  3:58         ` Florian Fainelli
2021-02-23  8:00 ` [PATCH v2] " Álvaro Fernández Rojas
2021-02-23 15:19   ` Guenter Roeck

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git