diff mbox series

[1/6] ARM: ks8695: watchdog: stop using mach/*.h

Message ID 20190415202501.941196-1-arnd@arndb.de (mailing list archive)
State Changes Requested
Headers show
Series [1/6] ARM: ks8695: watchdog: stop using mach/*.h | expand

Commit Message

Arnd Bergmann April 15, 2019, 8:24 p.m. UTC
drivers should not rely on machine specific headers but
get their information from the platform device.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm/mach-ks8695/devices.c | 13 ++++++++++++-
 drivers/watchdog/Kconfig       |  2 +-
 drivers/watchdog/ks8695_wdt.c  | 30 +++++++++++++++++-------------
 3 files changed, 30 insertions(+), 15 deletions(-)

Comments

Guenter Roeck April 15, 2019, 8:54 p.m. UTC | #1
On Mon, Apr 15, 2019 at 10:24:13PM +0200, Arnd Bergmann wrote:
> drivers should not rely on machine specific headers but
> get their information from the platform device.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  arch/arm/mach-ks8695/devices.c | 13 ++++++++++++-
>  drivers/watchdog/Kconfig       |  2 +-
>  drivers/watchdog/ks8695_wdt.c  | 30 +++++++++++++++++-------------
>  3 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/mach-ks8695/devices.c b/arch/arm/mach-ks8695/devices.c
> index 61cf20beb45f..57766817d86f 100644
> --- a/arch/arm/mach-ks8695/devices.c
> +++ b/arch/arm/mach-ks8695/devices.c
> @@ -169,11 +169,22 @@ void __init ks8696_add_device_hpna(void)
>  /* --------------------------------------------------------------------
>   *  Watchdog
>   * -------------------------------------------------------------------- */
> +#define KS8695_TMR_OFFSET      (0xF0000 + 0xE400)
> +#define KS8695_TMR_PA          (KS8695_IO_PA + KS8695_TMR_OFFSET)
> +static struct resource ks8695_wdt_resources[] = {
> +	[0] = {
> +		.name	= "tmr",
> +		.start	= KS8695_TMR_PA,
> +		.end	= KS8695_TMR_PA + 0xf,
> +		.flags	= IORESOURCE_MEM,
> +	},
> +};
>  
>  static struct platform_device ks8695_wdt_device = {
>  	.name		= "ks8695_wdt",
>  	.id		= -1,
> -	.num_resources	= 0,
> +	.resource	= ks8695_wdt_resources,
> +	.num_resources	= ARRAY_SIZE(ks8695_wdt_resources),
>  };
>  
>  static void __init ks8695_add_device_watchdog(void)
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 242eea859637..046e01daef57 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -397,7 +397,7 @@ config IXP4XX_WATCHDOG
>  
>  config KS8695_WATCHDOG
>  	tristate "KS8695 watchdog"
> -	depends on ARCH_KS8695
> +	depends on ARCH_KS8695 || COMPILE_TEST

Is __raw_readl / __raw_writel really available for all architectures / platforms ?

>  	help
>  	  Watchdog timer embedded into KS8695 processor. This will reboot your
>  	  system when the timeout is reached.
> diff --git a/drivers/watchdog/ks8695_wdt.c b/drivers/watchdog/ks8695_wdt.c
> index 1e41818a44bc..87c542c2f912 100644
> --- a/drivers/watchdog/ks8695_wdt.c
> +++ b/drivers/watchdog/ks8695_wdt.c
> @@ -23,10 +23,8 @@
>  #include <linux/watchdog.h>
>  #include <linux/io.h>
>  #include <linux/uaccess.h>
> -#include <mach/hardware.h>
>  
> -#define KS8695_TMR_OFFSET	(0xF0000 + 0xE400)
> -#define KS8695_TMR_VA		(KS8695_IO_VA + KS8695_TMR_OFFSET)
> +#define KS8695_CLOCK_RATE  25000000
>  
>  /*
>   * Timer registers
> @@ -57,6 +55,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>  
>  static unsigned long ks8695wdt_busy;
>  static DEFINE_SPINLOCK(ks8695_lock);
> +static void __iomem *tmr_reg;
>  
>  /* ......................................................................... */
>  
> @@ -69,8 +68,8 @@ static inline void ks8695_wdt_stop(void)
>  
>  	spin_lock(&ks8695_lock);
>  	/* disable timer0 */
> -	tmcon = __raw_readl(KS8695_TMR_VA + KS8695_TMCON);
> -	__raw_writel(tmcon & ~TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
> +	tmcon = __raw_readl(tmr_reg + KS8695_TMCON);
> +	__raw_writel(tmcon & ~TMCON_T0EN, tmr_reg + KS8695_TMCON);
>  	spin_unlock(&ks8695_lock);
>  }
>  
> @@ -84,15 +83,15 @@ static inline void ks8695_wdt_start(void)
>  
>  	spin_lock(&ks8695_lock);
>  	/* disable timer0 */
> -	tmcon = __raw_readl(KS8695_TMR_VA + KS8695_TMCON);
> -	__raw_writel(tmcon & ~TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
> +	tmcon = __raw_readl(tmr_reg + KS8695_TMCON);
> +	__raw_writel(tmcon & ~TMCON_T0EN, tmr_reg + KS8695_TMCON);
>  
>  	/* program timer0 */
> -	__raw_writel(tval | T0TC_WATCHDOG, KS8695_TMR_VA + KS8695_T0TC);
> +	__raw_writel(tval | T0TC_WATCHDOG, tmr_reg + KS8695_T0TC);
>  
>  	/* re-enable timer0 */
> -	tmcon = __raw_readl(KS8695_TMR_VA + KS8695_TMCON);
> -	__raw_writel(tmcon | TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
> +	tmcon = __raw_readl(tmr_reg + KS8695_TMCON);
> +	__raw_writel(tmcon | TMCON_T0EN, tmr_reg + KS8695_TMCON);
>  	spin_unlock(&ks8695_lock);
>  }
>  
> @@ -105,9 +104,9 @@ static inline void ks8695_wdt_reload(void)
>  
>  	spin_lock(&ks8695_lock);
>  	/* disable, then re-enable timer0 */
> -	tmcon = __raw_readl(KS8695_TMR_VA + KS8695_TMCON);
> -	__raw_writel(tmcon & ~TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
> -	__raw_writel(tmcon | TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
> +	tmcon = __raw_readl(tmr_reg + KS8695_TMCON);
> +	__raw_writel(tmcon & ~TMCON_T0EN, tmr_reg + KS8695_TMCON);
> +	__raw_writel(tmcon | TMCON_T0EN, tmr_reg + KS8695_TMCON);
>  	spin_unlock(&ks8695_lock);
>  }
>  
> @@ -238,6 +237,11 @@ static struct miscdevice ks8695wdt_miscdev = {
>  static int ks8695wdt_probe(struct platform_device *pdev)
>  {
>  	int res;
> +	struct resource *resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	tmr_reg = devm_ioremap_resource(&pdev->dev, resource);

Please use devm_platform_ioremap_resource().

Thanks,
Guenter

> +	if (!tmr_reg)
> +		return -ENXIO;
>  
>  	if (ks8695wdt_miscdev.parent)
>  		return -EBUSY;
Arnd Bergmann April 15, 2019, 8:58 p.m. UTC | #2
On Mon, Apr 15, 2019 at 10:54 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> >
> >  config KS8695_WATCHDOG
> >       tristate "KS8695 watchdog"
> > -     depends on ARCH_KS8695
> > +     depends on ARCH_KS8695 || COMPILE_TEST
>
> Is __raw_readl / __raw_writel really available for all architectures / platforms ?

I'm fairly sure it is these days, only uml and s390 used to be the
exceptions here, but they both added this.

It's possible that something else is missing, I was hoping for the 0-day
bot to tell me if so.

> > @@ -238,6 +237,11 @@ static struct miscdevice ks8695wdt_miscdev = {
> >  static int ks8695wdt_probe(struct platform_device *pdev)
> >  {
> >       int res;
> > +     struct resource *resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +
> > +     tmr_reg = devm_ioremap_resource(&pdev->dev, resource);
>
> Please use devm_platform_ioremap_resource().

Ah, that is the function I was looking for, thanks for the hint.

      Arnd
Greg Ungerer April 20, 2019, 2:36 a.m. UTC | #3
Hi Arnd,

On 16/4/19 6:24 am, Arnd Bergmann wrote:
> drivers should not rely on machine specific headers but
> get their information from the platform device.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

I like the whole series, thanks for doing this.

I haven't looked at the KS8695 in a long time now. I am not sure
that I have any working hardware - but I will have a look around my lab
and see if I can find something.

I'll get back to you with acks and tested bys soon.

Regards
Greg



> ---
>   arch/arm/mach-ks8695/devices.c | 13 ++++++++++++-
>   drivers/watchdog/Kconfig       |  2 +-
>   drivers/watchdog/ks8695_wdt.c  | 30 +++++++++++++++++-------------
>   3 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/mach-ks8695/devices.c b/arch/arm/mach-ks8695/devices.c
> index 61cf20beb45f..57766817d86f 100644
> --- a/arch/arm/mach-ks8695/devices.c
> +++ b/arch/arm/mach-ks8695/devices.c
> @@ -169,11 +169,22 @@ void __init ks8696_add_device_hpna(void)
>   /* --------------------------------------------------------------------
>    *  Watchdog
>    * -------------------------------------------------------------------- */
> +#define KS8695_TMR_OFFSET      (0xF0000 + 0xE400)
> +#define KS8695_TMR_PA          (KS8695_IO_PA + KS8695_TMR_OFFSET)
> +static struct resource ks8695_wdt_resources[] = {
> +	[0] = {
> +		.name	= "tmr",
> +		.start	= KS8695_TMR_PA,
> +		.end	= KS8695_TMR_PA + 0xf,
> +		.flags	= IORESOURCE_MEM,
> +	},
> +};
>   
>   static struct platform_device ks8695_wdt_device = {
>   	.name		= "ks8695_wdt",
>   	.id		= -1,
> -	.num_resources	= 0,
> +	.resource	= ks8695_wdt_resources,
> +	.num_resources	= ARRAY_SIZE(ks8695_wdt_resources),
>   };
>   
>   static void __init ks8695_add_device_watchdog(void)
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 242eea859637..046e01daef57 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -397,7 +397,7 @@ config IXP4XX_WATCHDOG
>   
>   config KS8695_WATCHDOG
>   	tristate "KS8695 watchdog"
> -	depends on ARCH_KS8695
> +	depends on ARCH_KS8695 || COMPILE_TEST
>   	help
>   	  Watchdog timer embedded into KS8695 processor. This will reboot your
>   	  system when the timeout is reached.
> diff --git a/drivers/watchdog/ks8695_wdt.c b/drivers/watchdog/ks8695_wdt.c
> index 1e41818a44bc..87c542c2f912 100644
> --- a/drivers/watchdog/ks8695_wdt.c
> +++ b/drivers/watchdog/ks8695_wdt.c
> @@ -23,10 +23,8 @@
>   #include <linux/watchdog.h>
>   #include <linux/io.h>
>   #include <linux/uaccess.h>
> -#include <mach/hardware.h>
>   
> -#define KS8695_TMR_OFFSET	(0xF0000 + 0xE400)
> -#define KS8695_TMR_VA		(KS8695_IO_VA + KS8695_TMR_OFFSET)
> +#define KS8695_CLOCK_RATE  25000000
>   
>   /*
>    * Timer registers
> @@ -57,6 +55,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>   
>   static unsigned long ks8695wdt_busy;
>   static DEFINE_SPINLOCK(ks8695_lock);
> +static void __iomem *tmr_reg;
>   
>   /* ......................................................................... */
>   
> @@ -69,8 +68,8 @@ static inline void ks8695_wdt_stop(void)
>   
>   	spin_lock(&ks8695_lock);
>   	/* disable timer0 */
> -	tmcon = __raw_readl(KS8695_TMR_VA + KS8695_TMCON);
> -	__raw_writel(tmcon & ~TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
> +	tmcon = __raw_readl(tmr_reg + KS8695_TMCON);
> +	__raw_writel(tmcon & ~TMCON_T0EN, tmr_reg + KS8695_TMCON);
>   	spin_unlock(&ks8695_lock);
>   }
>   
> @@ -84,15 +83,15 @@ static inline void ks8695_wdt_start(void)
>   
>   	spin_lock(&ks8695_lock);
>   	/* disable timer0 */
> -	tmcon = __raw_readl(KS8695_TMR_VA + KS8695_TMCON);
> -	__raw_writel(tmcon & ~TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
> +	tmcon = __raw_readl(tmr_reg + KS8695_TMCON);
> +	__raw_writel(tmcon & ~TMCON_T0EN, tmr_reg + KS8695_TMCON);
>   
>   	/* program timer0 */
> -	__raw_writel(tval | T0TC_WATCHDOG, KS8695_TMR_VA + KS8695_T0TC);
> +	__raw_writel(tval | T0TC_WATCHDOG, tmr_reg + KS8695_T0TC);
>   
>   	/* re-enable timer0 */
> -	tmcon = __raw_readl(KS8695_TMR_VA + KS8695_TMCON);
> -	__raw_writel(tmcon | TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
> +	tmcon = __raw_readl(tmr_reg + KS8695_TMCON);
> +	__raw_writel(tmcon | TMCON_T0EN, tmr_reg + KS8695_TMCON);
>   	spin_unlock(&ks8695_lock);
>   }
>   
> @@ -105,9 +104,9 @@ static inline void ks8695_wdt_reload(void)
>   
>   	spin_lock(&ks8695_lock);
>   	/* disable, then re-enable timer0 */
> -	tmcon = __raw_readl(KS8695_TMR_VA + KS8695_TMCON);
> -	__raw_writel(tmcon & ~TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
> -	__raw_writel(tmcon | TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
> +	tmcon = __raw_readl(tmr_reg + KS8695_TMCON);
> +	__raw_writel(tmcon & ~TMCON_T0EN, tmr_reg + KS8695_TMCON);
> +	__raw_writel(tmcon | TMCON_T0EN, tmr_reg + KS8695_TMCON);
>   	spin_unlock(&ks8695_lock);
>   }
>   
> @@ -238,6 +237,11 @@ static struct miscdevice ks8695wdt_miscdev = {
>   static int ks8695wdt_probe(struct platform_device *pdev)
>   {
>   	int res;
> +	struct resource *resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	tmr_reg = devm_ioremap_resource(&pdev->dev, resource);
> +	if (!tmr_reg)
> +		return -ENXIO;
>   
>   	if (ks8695wdt_miscdev.parent)
>   		return -EBUSY;
>
Greg Ungerer May 3, 2019, 7:02 a.m. UTC | #4
Hi Arnd,

On 16/4/19 6:24 am, Arnd Bergmann wrote:
> drivers should not rely on machine specific headers but
> get their information from the platform device.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

I dug out some old ks8695 based hardware to try this out.
I had a lot of trouble getting anything modern working on it.
In the end I still don't have a reliable test bed to test this properly.

Your patch series works as well as the kernel before the changes,
so I am happy enough to ack them as they are.

Acked-by: Greg Ungerer <gerg@kernel.org>

Ultimately though I am left wondering if the ks8695 support in the
kernel is useful to anyone the way it is at the moment. With a minimal
kernel configuration I can boot up to a shell - but the system is
really unreliable if you try to interactively use it. I don't think
it is the hardware - it seems to run reliably with the old code
it has running from flash on it. I am only testing the new kernel,
running with the existing user space root filesystem on it (which
dates from 2004 :-)

Regards
Greg



>   arch/arm/mach-ks8695/devices.c | 13 ++++++++++++-
>   drivers/watchdog/Kconfig       |  2 +-
>   drivers/watchdog/ks8695_wdt.c  | 30 +++++++++++++++++-------------
>   3 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/mach-ks8695/devices.c b/arch/arm/mach-ks8695/devices.c
> index 61cf20beb45f..57766817d86f 100644
> --- a/arch/arm/mach-ks8695/devices.c
> +++ b/arch/arm/mach-ks8695/devices.c
> @@ -169,11 +169,22 @@ void __init ks8696_add_device_hpna(void)
>   /* --------------------------------------------------------------------
>    *  Watchdog
>    * -------------------------------------------------------------------- */
> +#define KS8695_TMR_OFFSET      (0xF0000 + 0xE400)
> +#define KS8695_TMR_PA          (KS8695_IO_PA + KS8695_TMR_OFFSET)
> +static struct resource ks8695_wdt_resources[] = {
> +	[0] = {
> +		.name	= "tmr",
> +		.start	= KS8695_TMR_PA,
> +		.end	= KS8695_TMR_PA + 0xf,
> +		.flags	= IORESOURCE_MEM,
> +	},
> +};
>   
>   static struct platform_device ks8695_wdt_device = {
>   	.name		= "ks8695_wdt",
>   	.id		= -1,
> -	.num_resources	= 0,
> +	.resource	= ks8695_wdt_resources,
> +	.num_resources	= ARRAY_SIZE(ks8695_wdt_resources),
>   };
>   
>   static void __init ks8695_add_device_watchdog(void)
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 242eea859637..046e01daef57 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -397,7 +397,7 @@ config IXP4XX_WATCHDOG
>   
>   config KS8695_WATCHDOG
>   	tristate "KS8695 watchdog"
> -	depends on ARCH_KS8695
> +	depends on ARCH_KS8695 || COMPILE_TEST
>   	help
>   	  Watchdog timer embedded into KS8695 processor. This will reboot your
>   	  system when the timeout is reached.
> diff --git a/drivers/watchdog/ks8695_wdt.c b/drivers/watchdog/ks8695_wdt.c
> index 1e41818a44bc..87c542c2f912 100644
> --- a/drivers/watchdog/ks8695_wdt.c
> +++ b/drivers/watchdog/ks8695_wdt.c
> @@ -23,10 +23,8 @@
>   #include <linux/watchdog.h>
>   #include <linux/io.h>
>   #include <linux/uaccess.h>
> -#include <mach/hardware.h>
>   
> -#define KS8695_TMR_OFFSET	(0xF0000 + 0xE400)
> -#define KS8695_TMR_VA		(KS8695_IO_VA + KS8695_TMR_OFFSET)
> +#define KS8695_CLOCK_RATE  25000000
>   
>   /*
>    * Timer registers
> @@ -57,6 +55,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>   
>   static unsigned long ks8695wdt_busy;
>   static DEFINE_SPINLOCK(ks8695_lock);
> +static void __iomem *tmr_reg;
>   
>   /* ......................................................................... */
>   
> @@ -69,8 +68,8 @@ static inline void ks8695_wdt_stop(void)
>   
>   	spin_lock(&ks8695_lock);
>   	/* disable timer0 */
> -	tmcon = __raw_readl(KS8695_TMR_VA + KS8695_TMCON);
> -	__raw_writel(tmcon & ~TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
> +	tmcon = __raw_readl(tmr_reg + KS8695_TMCON);
> +	__raw_writel(tmcon & ~TMCON_T0EN, tmr_reg + KS8695_TMCON);
>   	spin_unlock(&ks8695_lock);
>   }
>   
> @@ -84,15 +83,15 @@ static inline void ks8695_wdt_start(void)
>   
>   	spin_lock(&ks8695_lock);
>   	/* disable timer0 */
> -	tmcon = __raw_readl(KS8695_TMR_VA + KS8695_TMCON);
> -	__raw_writel(tmcon & ~TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
> +	tmcon = __raw_readl(tmr_reg + KS8695_TMCON);
> +	__raw_writel(tmcon & ~TMCON_T0EN, tmr_reg + KS8695_TMCON);
>   
>   	/* program timer0 */
> -	__raw_writel(tval | T0TC_WATCHDOG, KS8695_TMR_VA + KS8695_T0TC);
> +	__raw_writel(tval | T0TC_WATCHDOG, tmr_reg + KS8695_T0TC);
>   
>   	/* re-enable timer0 */
> -	tmcon = __raw_readl(KS8695_TMR_VA + KS8695_TMCON);
> -	__raw_writel(tmcon | TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
> +	tmcon = __raw_readl(tmr_reg + KS8695_TMCON);
> +	__raw_writel(tmcon | TMCON_T0EN, tmr_reg + KS8695_TMCON);
>   	spin_unlock(&ks8695_lock);
>   }
>   
> @@ -105,9 +104,9 @@ static inline void ks8695_wdt_reload(void)
>   
>   	spin_lock(&ks8695_lock);
>   	/* disable, then re-enable timer0 */
> -	tmcon = __raw_readl(KS8695_TMR_VA + KS8695_TMCON);
> -	__raw_writel(tmcon & ~TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
> -	__raw_writel(tmcon | TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
> +	tmcon = __raw_readl(tmr_reg + KS8695_TMCON);
> +	__raw_writel(tmcon & ~TMCON_T0EN, tmr_reg + KS8695_TMCON);
> +	__raw_writel(tmcon | TMCON_T0EN, tmr_reg + KS8695_TMCON);
>   	spin_unlock(&ks8695_lock);
>   }
>   
> @@ -238,6 +237,11 @@ static struct miscdevice ks8695wdt_miscdev = {
>   static int ks8695wdt_probe(struct platform_device *pdev)
>   {
>   	int res;
> +	struct resource *resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	tmr_reg = devm_ioremap_resource(&pdev->dev, resource);
> +	if (!tmr_reg)
> +		return -ENXIO;
>   
>   	if (ks8695wdt_miscdev.parent)
>   		return -EBUSY;
>
Linus Walleij May 3, 2019, 7:16 a.m. UTC | #5
On Fri, May 3, 2019 at 8:02 AM Greg Ungerer <gerg@uclinux.org> wrote:

> I dug out some old ks8695 based hardware to try this out.
> I had a lot of trouble getting anything modern working on it.
> In the end I still don't have a reliable test bed to test this properly.

What is usually used by old ARMv4 systems is OpenWrt or
OpenEmbedded. Those is the only build systems that reliably
produce a userspace for these things now, and it is also the
appropriate size for this kind of systems.

> Ultimately though I am left wondering if the ks8695 support in the
> kernel is useful to anyone the way it is at the moment. With a minimal
> kernel configuration I can boot up to a shell - but the system is
> really unreliable if you try to interactively use it. I don't think
> it is the hardware - it seems to run reliably with the old code
> it has running from flash on it. I am only testing the new kernel,
> running with the existing user space root filesystem on it (which
> dates from 2004 :-)

Personally I think it is a bad sign that this subarch and boards do
not have active OpenWrt support, they are routers after all (right?)
and any active use of networking equipment should use a recent
userspace as well, given all the security bugs that popped up over
the years.

With IXP4xx, Gemini and EP93xx we have found active users and
companies selling the chips and reference designs and even
recommending it for new products (!) at times.  If this is not the
case with KS8695 and no hobbyists are willing to submit it
to OpenWrt and modernize it to use device tree I think it should be
deleted from the kernel.

Yours,
Linus Walleij
Guenter Roeck May 3, 2019, 5:06 p.m. UTC | #6
On Fri, May 03, 2019 at 08:16:05AM +0100, Linus Walleij wrote:
> On Fri, May 3, 2019 at 8:02 AM Greg Ungerer <gerg@uclinux.org> wrote:
> 
> > I dug out some old ks8695 based hardware to try this out.
> > I had a lot of trouble getting anything modern working on it.
> > In the end I still don't have a reliable test bed to test this properly.
> 
> What is usually used by old ARMv4 systems is OpenWrt or
> OpenEmbedded. Those is the only build systems that reliably
> produce a userspace for these things now, and it is also the
> appropriate size for this kind of systems.
> 
> > Ultimately though I am left wondering if the ks8695 support in the
> > kernel is useful to anyone the way it is at the moment. With a minimal
> > kernel configuration I can boot up to a shell - but the system is
> > really unreliable if you try to interactively use it. I don't think
> > it is the hardware - it seems to run reliably with the old code
> > it has running from flash on it. I am only testing the new kernel,
> > running with the existing user space root filesystem on it (which
> > dates from 2004 :-)
> 
> Personally I think it is a bad sign that this subarch and boards do
> not have active OpenWrt support, they are routers after all (right?)
> and any active use of networking equipment should use a recent
> userspace as well, given all the security bugs that popped up over
> the years.
> 
> With IXP4xx, Gemini and EP93xx we have found active users and
> companies selling the chips and reference designs and even
> recommending it for new products (!) at times.  If this is not the
> case with KS8695 and no hobbyists are willing to submit it
> to OpenWrt and modernize it to use device tree I think it should be
> deleted from the kernel.
> 

That may be the best approach if indeed no one is using it,
much less maintaining it.

Guenter
Greg Ungerer May 4, 2019, 2:26 p.m. UTC | #7
On 4/5/19 3:06 am, Guenter Roeck wrote:
> On Fri, May 03, 2019 at 08:16:05AM +0100, Linus Walleij wrote:
>> On Fri, May 3, 2019 at 8:02 AM Greg Ungerer <gerg@uclinux.org> wrote:
>>
>>> I dug out some old ks8695 based hardware to try this out.
>>> I had a lot of trouble getting anything modern working on it.
>>> In the end I still don't have a reliable test bed to test this properly.
>>
>> What is usually used by old ARMv4 systems is OpenWrt or
>> OpenEmbedded. Those is the only build systems that reliably
>> produce a userspace for these things now, and it is also the
>> appropriate size for this kind of systems.

No, I can produce a user space environment for the KS8695 as well
using the uClinux-dist build system. But that worked even less well
than the old root filesystem that I had (which was also built with
an older version of that build system).

But there is no reason that old root filesystem should not work.
And that is the thing that concerns me a bit here. I could mount
it ok (it was a CRAMFS), it would run up the shell to a shell prompt,
but when I try to run any commands from there they would oops.
I didn't debug any further than that.


>>> Ultimately though I am left wondering if the ks8695 support in the
>>> kernel is useful to anyone the way it is at the moment. With a minimal
>>> kernel configuration I can boot up to a shell - but the system is
>>> really unreliable if you try to interactively use it. I don't think
>>> it is the hardware - it seems to run reliably with the old code
>>> it has running from flash on it. I am only testing the new kernel,
>>> running with the existing user space root filesystem on it (which
>>> dates from 2004 :-)
>>
>> Personally I think it is a bad sign that this subarch and boards do
>> not have active OpenWrt support, they are routers after all (right?)
>> and any active use of networking equipment should use a recent
>> userspace as well, given all the security bugs that popped up over
>> the years.
>>
>> With IXP4xx, Gemini and EP93xx we have found active users and
>> companies selling the chips and reference designs and even
>> recommending it for new products (!) at times.  If this is not the
>> case with KS8695 and no hobbyists are willing to submit it
>> to OpenWrt and modernize it to use device tree I think it should be
>> deleted from the kernel.
>>
> 
> That may be the best approach if indeed no one is using it,
> much less maintaining it.

Well, I for one don't really use it any more. So I don't have a lot
of motivation to maintain it any longer.

Regards
Greg
Arnd Bergmann July 22, 2019, 2:44 p.m. UTC | #8
On Sat, May 4, 2019 at 4:27 PM Greg Ungerer <gerg@kernel.org> wrote:
> On 4/5/19 3:06 am, Guenter Roeck wrote:
> > On Fri, May 03, 2019 at 08:16:05AM +0100, Linus Walleij wrote:
> >> On Fri, May 3, 2019 at 8:02 AM Greg Ungerer <gerg@uclinux.org> wrote:
> >>> Ultimately though I am left wondering if the ks8695 support in the
> >>> kernel is useful to anyone the way it is at the moment. With a minimal
> >>> kernel configuration I can boot up to a shell - but the system is
> >>> really unreliable if you try to interactively use it. I don't think
> >>> it is the hardware - it seems to run reliably with the old code
> >>> it has running from flash on it. I am only testing the new kernel,
> >>> running with the existing user space root filesystem on it (which
> >>> dates from 2004 :-)
> >>
> >> Personally I think it is a bad sign that this subarch and boards do
> >> not have active OpenWrt support, they are routers after all (right?)
> >> and any active use of networking equipment should use a recent
> >> userspace as well, given all the security bugs that popped up over
> >> the years.

Looking around on the internet, I found that Micrel at some point
had their own openwrt fork for ks8695, but I can't find a copy
any more, as the micrel.com domain is no longer used after the
acquisition by Microchip.

https://wikidevi.com/wiki/Micrel has a list of devices based on
ks8695, and it seems that most of these are rather memory
limited, which is a problem for recent openwrt builds.

Only two of the 17 listed devices have the absolute minimum of 4MB
flash and 32MB RAM for openwrt, two more have 8/32 and one
or two have 4/64, but all these configurations are too limited for the
web U/I now.

> >> With IXP4xx, Gemini and EP93xx we have found active users and
> >> companies selling the chips and reference designs and even
> >> recommending it for new products (!) at times.  If this is not the
> >> case with KS8695 and no hobbyists are willing to submit it
> >> to OpenWrt and modernize it to use device tree I think it should be
> >> deleted from the kernel.
> >>
> >
> > That may be the best approach if indeed no one is using it,
> > much less maintaining it.
>
> Well, I for one don't really use it any more. So I don't have a lot
> of motivation to maintain it any longer.

I came across my patches while rebasing my backlog to 5.3-rc1.

Should I save the (very small) trouble of sending them out again
and just remove the platform then?

      Arnd
Olof Johansson July 22, 2019, 8:13 p.m. UTC | #9
On Mon, Jul 22, 2019 at 7:44 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Sat, May 4, 2019 at 4:27 PM Greg Ungerer <gerg@kernel.org> wrote:
> > On 4/5/19 3:06 am, Guenter Roeck wrote:
> > > On Fri, May 03, 2019 at 08:16:05AM +0100, Linus Walleij wrote:
> > >> On Fri, May 3, 2019 at 8:02 AM Greg Ungerer <gerg@uclinux.org> wrote:
> > >>> Ultimately though I am left wondering if the ks8695 support in the
> > >>> kernel is useful to anyone the way it is at the moment. With a minimal
> > >>> kernel configuration I can boot up to a shell - but the system is
> > >>> really unreliable if you try to interactively use it. I don't think
> > >>> it is the hardware - it seems to run reliably with the old code
> > >>> it has running from flash on it. I am only testing the new kernel,
> > >>> running with the existing user space root filesystem on it (which
> > >>> dates from 2004 :-)
> > >>
> > >> Personally I think it is a bad sign that this subarch and boards do
> > >> not have active OpenWrt support, they are routers after all (right?)
> > >> and any active use of networking equipment should use a recent
> > >> userspace as well, given all the security bugs that popped up over
> > >> the years.
>
> Looking around on the internet, I found that Micrel at some point
> had their own openwrt fork for ks8695, but I can't find a copy
> any more, as the micrel.com domain is no longer used after the
> acquisition by Microchip.
>
> https://wikidevi.com/wiki/Micrel has a list of devices based on
> ks8695, and it seems that most of these are rather memory
> limited, which is a problem for recent openwrt builds.
>
> Only two of the 17 listed devices have the absolute minimum of 4MB
> flash and 32MB RAM for openwrt, two more have 8/32 and one
> or two have 4/64, but all these configurations are too limited for the
> web U/I now.
>
> > >> With IXP4xx, Gemini and EP93xx we have found active users and
> > >> companies selling the chips and reference designs and even
> > >> recommending it for new products (!) at times.  If this is not the
> > >> case with KS8695 and no hobbyists are willing to submit it
> > >> to OpenWrt and modernize it to use device tree I think it should be
> > >> deleted from the kernel.
> > >>
> > >
> > > That may be the best approach if indeed no one is using it,
> > > much less maintaining it.
> >
> > Well, I for one don't really use it any more. So I don't have a lot
> > of motivation to maintain it any longer.
>
> I came across my patches while rebasing my backlog to 5.3-rc1.
>
> Should I save the (very small) trouble of sending them out again
> and just remove the platform then?

Given the lack of response/interest from users, I'm OK with removing it.

If someone shows up wanting support, we'll have a good opportunity to
discuss testing/modernization involving someone actively using the
hardware.


-Olof
Greg Ungerer July 29, 2019, 12:53 p.m. UTC | #10
Hi Arnd,

On 23/7/19 12:44 am, Arnd Bergmann wrote:
> On Sat, May 4, 2019 at 4:27 PM Greg Ungerer <gerg@kernel.org> wrote:
>> On 4/5/19 3:06 am, Guenter Roeck wrote:
>>> On Fri, May 03, 2019 at 08:16:05AM +0100, Linus Walleij wrote:
>>>> On Fri, May 3, 2019 at 8:02 AM Greg Ungerer <gerg@uclinux.org> wrote:
>>>>> Ultimately though I am left wondering if the ks8695 support in the
>>>>> kernel is useful to anyone the way it is at the moment. With a minimal
>>>>> kernel configuration I can boot up to a shell - but the system is
>>>>> really unreliable if you try to interactively use it. I don't think
>>>>> it is the hardware - it seems to run reliably with the old code
>>>>> it has running from flash on it. I am only testing the new kernel,
>>>>> running with the existing user space root filesystem on it (which
>>>>> dates from 2004 :-)
>>>>
>>>> Personally I think it is a bad sign that this subarch and boards do
>>>> not have active OpenWrt support, they are routers after all (right?)
>>>> and any active use of networking equipment should use a recent
>>>> userspace as well, given all the security bugs that popped up over
>>>> the years.
> 
> Looking around on the internet, I found that Micrel at some point
> had their own openwrt fork for ks8695, but I can't find a copy
> any more, as the micrel.com domain is no longer used after the
> acquisition by Microchip.

I build it with uClinux-dist, https://sourceforge.net/projects/uclinux/files/uClinux%20Stable/.
And again I can build for it, it just doesn't currently work
in any sort of reasonable way. So I get the impression it
hasn't worked for a while and nobody has noticed.


> https://wikidevi.com/wiki/Micrel has a list of devices based on
> ks8695, and it seems that most of these are rather memory
> limited, which is a problem for recent openwrt builds.
> 
> Only two of the 17 listed devices have the absolute minimum of 4MB
> flash and 32MB RAM for openwrt, two more have 8/32 and one
> or two have 4/64, but all these configurations are too limited for the
> web U/I now.


>>>> With IXP4xx, Gemini and EP93xx we have found active users and
>>>> companies selling the chips and reference designs and even
>>>> recommending it for new products (!) at times.  If this is not the
>>>> case with KS8695 and no hobbyists are willing to submit it
>>>> to OpenWrt and modernize it to use device tree I think it should be
>>>> deleted from the kernel.
>>>>
>>>
>>> That may be the best approach if indeed no one is using it,
>>> much less maintaining it.
>>
>> Well, I for one don't really use it any more. So I don't have a lot
>> of motivation to maintain it any longer.
> 
> I came across my patches while rebasing my backlog to 5.3-rc1.
> 
> Should I save the (very small) trouble of sending them out again
> and just remove the platform then?

At this time I have no issue with removing it.

Regards
Greg
Arnd Bergmann July 29, 2019, 3:45 p.m. UTC | #11
On Mon, Jul 29, 2019 at 2:53 PM Greg Ungerer <gerg@kernel.org> wrote:
> On 23/7/19 12:44 am, Arnd Bergmann wrote:
> > On Sat, May 4, 2019 at 4:27 PM Greg Ungerer <gerg@kernel.org> wrote:
> >> On 4/5/19 3:06 am, Guenter Roeck wrote:
> >>> On Fri, May 03, 2019 at 08:16:05AM +0100, Linus Walleij wrote:
> >>>> With IXP4xx, Gemini and EP93xx we have found active users and
> >>>> companies selling the chips and reference designs and even
> >>>> recommending it for new products (!) at times.  If this is not the
> >>>> case with KS8695 and no hobbyists are willing to submit it
> >>>> to OpenWrt and modernize it to use device tree I think it should be
> >>>> deleted from the kernel.
> >>>>
> >>>
> >>> That may be the best approach if indeed no one is using it,
> >>> much less maintaining it.
> >>
> >> Well, I for one don't really use it any more. So I don't have a lot
> >> of motivation to maintain it any longer.
> >
> > I came across my patches while rebasing my backlog to 5.3-rc1.
> >
> > Should I save the (very small) trouble of sending them out again
> > and just remove the platform then?
>
> At this time I have no issue with removing it.

Ok, let's do that then.

For reference, this is what I think we should do for the
remaining ARM9 based platforms that never gained multiplatform
support, as time permits:

netx: is now removed
ks8695: remove next
w90x900: probably remove, need to confirm with last known users
davinci: almost multiplatform now, should be done in 5.4
ep93xx: convert to common-clk, generic-irq, then enable multiplatform
 (linusw is on it)
omap1: convert to common-clk, change pcmcia driver to common
  I/O space method, use dma_pfn_offset for virt_to_bus, add ugly
  hacks for cpu_is_omap*() and omap_readl(), then enable multiplatform
  (arnd has started this)
lpc32xx: clean up header files so we can build last 6 drivers
  independently, then move to multiplatform, probably after 5.4
  I have patches for this somewhere.
s3c24xx: change 18 drivers that still use mach/* headers, get
  creative about mach-bast ISA I/O space

        Arnd
diff mbox series

Patch

diff --git a/arch/arm/mach-ks8695/devices.c b/arch/arm/mach-ks8695/devices.c
index 61cf20beb45f..57766817d86f 100644
--- a/arch/arm/mach-ks8695/devices.c
+++ b/arch/arm/mach-ks8695/devices.c
@@ -169,11 +169,22 @@  void __init ks8696_add_device_hpna(void)
 /* --------------------------------------------------------------------
  *  Watchdog
  * -------------------------------------------------------------------- */
+#define KS8695_TMR_OFFSET      (0xF0000 + 0xE400)
+#define KS8695_TMR_PA          (KS8695_IO_PA + KS8695_TMR_OFFSET)
+static struct resource ks8695_wdt_resources[] = {
+	[0] = {
+		.name	= "tmr",
+		.start	= KS8695_TMR_PA,
+		.end	= KS8695_TMR_PA + 0xf,
+		.flags	= IORESOURCE_MEM,
+	},
+};
 
 static struct platform_device ks8695_wdt_device = {
 	.name		= "ks8695_wdt",
 	.id		= -1,
-	.num_resources	= 0,
+	.resource	= ks8695_wdt_resources,
+	.num_resources	= ARRAY_SIZE(ks8695_wdt_resources),
 };
 
 static void __init ks8695_add_device_watchdog(void)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 242eea859637..046e01daef57 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -397,7 +397,7 @@  config IXP4XX_WATCHDOG
 
 config KS8695_WATCHDOG
 	tristate "KS8695 watchdog"
-	depends on ARCH_KS8695
+	depends on ARCH_KS8695 || COMPILE_TEST
 	help
 	  Watchdog timer embedded into KS8695 processor. This will reboot your
 	  system when the timeout is reached.
diff --git a/drivers/watchdog/ks8695_wdt.c b/drivers/watchdog/ks8695_wdt.c
index 1e41818a44bc..87c542c2f912 100644
--- a/drivers/watchdog/ks8695_wdt.c
+++ b/drivers/watchdog/ks8695_wdt.c
@@ -23,10 +23,8 @@ 
 #include <linux/watchdog.h>
 #include <linux/io.h>
 #include <linux/uaccess.h>
-#include <mach/hardware.h>
 
-#define KS8695_TMR_OFFSET	(0xF0000 + 0xE400)
-#define KS8695_TMR_VA		(KS8695_IO_VA + KS8695_TMR_OFFSET)
+#define KS8695_CLOCK_RATE  25000000
 
 /*
  * Timer registers
@@ -57,6 +55,7 @@  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 
 static unsigned long ks8695wdt_busy;
 static DEFINE_SPINLOCK(ks8695_lock);
+static void __iomem *tmr_reg;
 
 /* ......................................................................... */
 
@@ -69,8 +68,8 @@  static inline void ks8695_wdt_stop(void)
 
 	spin_lock(&ks8695_lock);
 	/* disable timer0 */
-	tmcon = __raw_readl(KS8695_TMR_VA + KS8695_TMCON);
-	__raw_writel(tmcon & ~TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
+	tmcon = __raw_readl(tmr_reg + KS8695_TMCON);
+	__raw_writel(tmcon & ~TMCON_T0EN, tmr_reg + KS8695_TMCON);
 	spin_unlock(&ks8695_lock);
 }
 
@@ -84,15 +83,15 @@  static inline void ks8695_wdt_start(void)
 
 	spin_lock(&ks8695_lock);
 	/* disable timer0 */
-	tmcon = __raw_readl(KS8695_TMR_VA + KS8695_TMCON);
-	__raw_writel(tmcon & ~TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
+	tmcon = __raw_readl(tmr_reg + KS8695_TMCON);
+	__raw_writel(tmcon & ~TMCON_T0EN, tmr_reg + KS8695_TMCON);
 
 	/* program timer0 */
-	__raw_writel(tval | T0TC_WATCHDOG, KS8695_TMR_VA + KS8695_T0TC);
+	__raw_writel(tval | T0TC_WATCHDOG, tmr_reg + KS8695_T0TC);
 
 	/* re-enable timer0 */
-	tmcon = __raw_readl(KS8695_TMR_VA + KS8695_TMCON);
-	__raw_writel(tmcon | TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
+	tmcon = __raw_readl(tmr_reg + KS8695_TMCON);
+	__raw_writel(tmcon | TMCON_T0EN, tmr_reg + KS8695_TMCON);
 	spin_unlock(&ks8695_lock);
 }
 
@@ -105,9 +104,9 @@  static inline void ks8695_wdt_reload(void)
 
 	spin_lock(&ks8695_lock);
 	/* disable, then re-enable timer0 */
-	tmcon = __raw_readl(KS8695_TMR_VA + KS8695_TMCON);
-	__raw_writel(tmcon & ~TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
-	__raw_writel(tmcon | TMCON_T0EN, KS8695_TMR_VA + KS8695_TMCON);
+	tmcon = __raw_readl(tmr_reg + KS8695_TMCON);
+	__raw_writel(tmcon & ~TMCON_T0EN, tmr_reg + KS8695_TMCON);
+	__raw_writel(tmcon | TMCON_T0EN, tmr_reg + KS8695_TMCON);
 	spin_unlock(&ks8695_lock);
 }
 
@@ -238,6 +237,11 @@  static struct miscdevice ks8695wdt_miscdev = {
 static int ks8695wdt_probe(struct platform_device *pdev)
 {
 	int res;
+	struct resource *resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	tmr_reg = devm_ioremap_resource(&pdev->dev, resource);
+	if (!tmr_reg)
+		return -ENXIO;
 
 	if (ks8695wdt_miscdev.parent)
 		return -EBUSY;