Re: [PATCH v2] power: reset: add linkstation-reset driver

Re: [PATCH v2] power: reset: add linkstation-reset driver

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 15717 bytes --]

Hi Roger,

On Fri, Dec 16, 2016 at 07:05:01PM +0900, Roger Shimizu wrote:
> Buffalo Linkstation / KuroBox and their variants need magic command
> sending to UART1 to power-off.
> 
> Power driver linkstation-reset implements the magic command and I/O
> routine, which come from files listed below:
>   - arch/arm/mach-orion5x/kurobox_pro-setup.c
>   - arch/arm/mach-orion5x/terastation_pro2-setup.c

Ok.

> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Martin Michlmayr <tbm@cyrius.com>
> Cc: Sylver Bruneau <sylver.bruneau@googlemail.com>
> Cc: Herbert Valerio Riedel <hvr@gnu.org>
> Reported-by: Ryan Tandy <ryan@nardis.ca>
> Signed-off-by: Roger Shimizu <rogershimizu@gmail.com>
> ---
> Dear Sebastian,
> 
> Kurobox-Pro (and variants) need more commands sending to UART1 to shutdown.
> So here I make this patch series to let current qnap-poweroff implementation
> be able to handle such case.
> 
> I already tested this change on Kurobox-Pro and Linkstation LS-GL devices,
> with a modified device-tree file. (Previous device-tree of kurobox-pro invokes
> restart-poweroff, so it simply restarts.)
> 
> Thank you and look forward to your feedback!
>
> Dear Andrew,
> 
> Thanks for your 2nd review!
> 
> So I accept your suggestion and make the new driver for linkstation series.
>
> Changes:
>   v0 => v1:
>   - Update 0003 to split kuroboxpro related code into kuroboxpro-common.c
>   v1 => v2:
>   - Slipt off linkstation/kuroboxpro related code to linkstation-reset.c
>     Because linkstation before kuroboxpro also need this driver to power
>     off properly. It's more proper to call it linkstation driver.
> 
> Cheers,
> --
> Roger Shimizu, GMT +9 Tokyo
> PGP/GPG: 4096R/6C6ACD6417B3ACB1
> 
>  .../bindings/power/reset/linkstation-reset.txt     |  26 ++++
>  drivers/power/reset/Kconfig                        |  10 ++
>  drivers/power/reset/Makefile                       |   1 +
>  drivers/power/reset/linkstation-common.c           | 124 +++++++++++++++
>  drivers/power/reset/linkstation-common.h           |   8 +
>  drivers/power/reset/linkstation-reset.c            | 172 +++++++++++++++++++++
>  6 files changed, 341 insertions(+)

With this being its own driver please merge linkstation-common and
linkstation-reset. The common part is only used by linkstation-reset
anyways.

>  create mode 100644 Documentation/devicetree/bindings/power/reset/linkstation-reset.txt

This patch is missing Cc for DT binding people (check "OPEN FIRMWARE
AND FLATTENED DEVICE TREE BINDINGS" in MAINTAINERS file).

>  create mode 100644 drivers/power/reset/linkstation-common.c
>  create mode 100644 drivers/power/reset/linkstation-common.h
>  create mode 100644 drivers/power/reset/linkstation-reset.c
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/linkstation-reset.txt b/Documentation/devicetree/bindings/power/reset/linkstation-reset.txt
> new file mode 100644
> index 0000000..815e340
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/linkstation-reset.txt
> @@ -0,0 +1,26 @@
> +* Buffalo Linkstation Reset Driver
> +
> +Power of some Buffalo Linkstation or KuroBox Pro is managed by
> +micro-controller, which connects to UART1. After being fed from UART1
> +by a few magic numbers, the so-called power-off command,
> +the micro-controller will turn power off the device.
> +
> +This is very similar to QNAP or Synology NAS devices, which is
> +described in qnap-poweroff.txt, however the command is much simpler,
> +only 1-byte long and without checksums.
> +
> +This driver adds a handler to pm_power_off which is called to turn the
> +power off.
> +
> +Required Properties:
> +- compatible: Should be "linkstation,power-off"
> +- reg: Address and length of the register set for UART1
> +- clocks: tclk clock
> +
> +Example:
> +
> +	reset {
> +		compatible = "linkstation,power-off";
> +		reg = <0x12100 0x100>;
> +		clocks = <&core_clk 0>;
> +	};

This might be another user for UART slave device [0].
[0] https://lkml.org/lkml/2016/8/24/769

Is the UART port used for anything else besides the reset
controller?

> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> index c74c3f6..77c44ca 100644
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> @@ -98,6 +98,16 @@ config POWER_RESET_IMX
>  	  say N here or disable in dts to make sure pm_power_off never be
>  	  overwrote wrongly by this driver.
>  
> +config POWER_RESET_LINKSTATION
> +	bool "Buffalo Linkstation and its variants reset driver"
> +	depends on OF_GPIO && PLAT_ORION
> +	help
> +	  This driver supports power off Buffalo Linkstation / KuroBox Pro
> +	  NAS and their variants by sending commands to the micro-controller
> +	  which controls the main power.
> +
> +	  Say Y if you have a Buffalo Linkstation / KuroBox Pro NAS.
> +
>  config POWER_RESET_MSM
>  	bool "Qualcomm MSM power-off driver"
>  	depends on ARCH_QCOM
> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
> index 1be307c..520afbe 100644
> --- a/drivers/power/reset/Makefile
> +++ b/drivers/power/reset/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
>  obj-$(CONFIG_POWER_RESET_GPIO_RESTART) += gpio-restart.o
>  obj-$(CONFIG_POWER_RESET_HISI) += hisi-reboot.o
>  obj-$(CONFIG_POWER_RESET_IMX) += imx-snvs-poweroff.o
> +obj-$(CONFIG_POWER_RESET_LINKSTATION) += linkstation-reset.o linkstation-common.o
>  obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
>  obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o
>  obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
> diff --git a/drivers/power/reset/linkstation-common.c b/drivers/power/reset/linkstation-common.c
> new file mode 100644
> index 0000000..a6d0930
> --- /dev/null
> +++ b/drivers/power/reset/linkstation-common.c
> @@ -0,0 +1,124 @@
> +/*
> + * Common I/O routine for micro-controller of Buffalo Linkstation
> + * and its variants.
> + *
> + * Copyright (C) 2016 Roger Shimizu <rogershimizu@gmail.com>
> + *
> + * Based on the code from:
> + *
> + * Copyright (C) 2008  Sylver Bruneau <sylver.bruneau@googlemail.com>
> + * Copyright (C) 2007  Herbert Valerio Riedel <hvr@gnu.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/serial_reg.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include "linkstation-common.h"
> +
> +static int uart1_micon_read(void *base, unsigned char *buf, int count)
> +{
> +	int i;
> +	int timeout;
> +
> +	for (i = 0; i < count; i++) {
> +		timeout = 10;
> +
> +		while (!(readl(UART1_REG(LSR)) & UART_LSR_DR)) {
> +			if (--timeout == 0)
> +				break;
> +			udelay(1000);
> +		}
> +
> +		if (timeout == 0)
> +			break;
> +		buf[i] = readl(UART1_REG(RX));
> +	}
> +
> +	/* return read bytes */
> +	return i;
> +}
> +
> +static int uart1_micon_write(void *base, const unsigned char *buf, int count)
> +{
> +	int i = 0;
> +
> +	while (count--) {
> +		while (!(readl(UART1_REG(LSR)) & UART_LSR_THRE))
> +			barrier();
> +		writel(buf[i++], UART1_REG(TX));
> +	}
> +
> +	return 0;
> +}
> +
> +int uart1_micon_send(void *base, const unsigned char *data, int count)
> +{
> +	int i;
> +	unsigned char checksum = 0;
> +	unsigned char recv_buf[40];
> +	unsigned char send_buf[40];
> +	unsigned char correct_ack[3];
> +	int retry = 2;
> +
> +	/* Generate checksum */
> +	for (i = 0; i < count; i++)
> +		checksum -=  data[i];
> +
> +	do {
> +		/* Send data */
> +		uart1_micon_write(base, data, count);
> +
> +		/* send checksum */
> +		uart1_micon_write(base, &checksum, 1);
> +
> +		if (uart1_micon_read(base, recv_buf, sizeof(recv_buf)) <= 3) {
> +			printk(KERN_ERR ">%s: receive failed.\n", __func__);
> +
> +			/* send preamble to clear the receive buffer */
> +			memset(&send_buf, 0xff, sizeof(send_buf));
> +			uart1_micon_write(base, send_buf, sizeof(send_buf));
> +
> +			/* make dummy reads */
> +			mdelay(100);
> +			uart1_micon_read(base, recv_buf, sizeof(recv_buf));
> +		} else {
> +			/* Generate expected ack */
> +			correct_ack[0] = 0x01;
> +			correct_ack[1] = data[1];
> +			correct_ack[2] = 0x00;
> +
> +			/* checksum Check */
> +			if ((recv_buf[0] + recv_buf[1] + recv_buf[2] +
> +			     recv_buf[3]) & 0xFF) {
> +				printk(KERN_ERR ">%s: Checksum Error : "
> +					"Received data[%02x, %02x, %02x, %02x]"
> +					"\n", __func__, recv_buf[0],
> +					recv_buf[1], recv_buf[2], recv_buf[3]);
> +			} else {
> +				/* Check Received Data */
> +				if (correct_ack[0] == recv_buf[0] &&
> +				    correct_ack[1] == recv_buf[1] &&
> +				    correct_ack[2] == recv_buf[2]) {
> +					/* Interval for next command */
> +					mdelay(10);
> +
> +					/* Receive ACK */
> +					return 0;
> +				}
> +			}
> +			/* Received NAK or illegal Data */
> +			printk(KERN_ERR ">%s: Error : NAK or Illegal Data "
> +					"Received\n", __func__);
> +		}
> +	} while (retry--);
> +
> +	/* Interval for next command */
> +	mdelay(10);
> +
> +	return -1;
> +}
> diff --git a/drivers/power/reset/linkstation-common.h b/drivers/power/reset/linkstation-common.h
> new file mode 100644
> index 0000000..89c64a9
> --- /dev/null
> +++ b/drivers/power/reset/linkstation-common.h
> @@ -0,0 +1,8 @@
> +#ifndef __LINKSTATION_COMMON_H__
> +#define __LINKSTATION_COMMON_H__
> +
> +#define UART1_REG(x)	(base + ((UART_##x) << 2))
> +
> +int uart1_micon_send(void *base, const unsigned char *data, int count);
> +
> +#endif
> diff --git a/drivers/power/reset/linkstation-reset.c b/drivers/power/reset/linkstation-reset.c
> new file mode 100644
> index 0000000..78a0137
> --- /dev/null
> +++ b/drivers/power/reset/linkstation-reset.c
> @@ -0,0 +1,172 @@
> +/*
> + * Buffalo Linkstation power reset driver.
> + * It may also be used on following devices:
> + *  - Buffalo Linkstation HG
> + *  - KuroBox HG
> + *  - Buffalo KURO-NAS/T4
> + *  - KuroBox Pro
> + *  - Buffalo Linkstation Pro (LS-GL)
> + *  - Buffalo Terastation Pro II/Live
> + *  - Buffalo Linkstation Duo (LS-WTGL)
> + *  - Buffalo Linkstation Mini (LS-WSGL)
> + *
> + * Copyright (C) 2016  Roger Shimizu <rogershimizu@gmail.com>
> + *
> + * Based on the code from:
> + *
> + * Copyright (C) 2012  Andrew Lunn <andrew@lunn.ch>
> + * Copyright (C) 2009  Martin Michlmayr <tbm@cyrius.com>
> + * Copyright (C) 2008  Byron Bradley <byron.bbradley@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/serial_reg.h>
> +#include <linux/kallsyms.h>
> +#include <linux/of.h>
> +#include <linux/io.h>
> +#include <linux/clk.h>
> +#include "linkstation-common.h"
> +
> +#define MICON_CMD_SIZE	4
> +
> +/* 4-byte magic hello command to UART1-attached microcontroller */
> +static const unsigned char linkstation_micon_magic[] = {
> +	0x1b,
> +	0x00,
> +	0x07,
> +	0x00
> +};

4-byte magic hello command? Those are used as uart configuration as
far as I can see. Just move this directly into reset_cfg:

struct reset_cfg {
    u32 baud;
    u8 lcr;
    u8 ier;
    u8 fcr;
    u8 mcr;
    const unsigned char (*cmd)[MICON_CMD_SIZE];
};

> +// for each row, first byte is the size of command
> +static const unsigned char linkstation_power_off_cmd[][MICON_CMD_SIZE] = {
> +	{ 3,	0x01, 0x35, 0x00},
> +	{ 2,	0x00, 0x0c},
> +	{ 2,	0x00, 0x06},
> +	{}
> +};
> +
> +struct reset_cfg {
> +	u32 baud;
> +	const unsigned char *magic;
> +	const unsigned char (*cmd)[MICON_CMD_SIZE];
> +};
> +
> +static const struct reset_cfg linkstation_power_off_cfg = {
> +	.baud = 38400,
> +	.magic = linkstation_micon_magic,
> +	.cmd = linkstation_power_off_cmd,
> +};
> +
> +static const struct of_device_id linkstation_reset_of_match_table[] = {
> +	{ .compatible = "linkstation,power-off",
> +	  .data = &linkstation_power_off_cfg,
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, linkstation_reset_of_match_table);
> +
> +static void __iomem *base;
> +static unsigned long tclk;
> +static const struct reset_cfg *cfg;
> +
> +static void linkstation_reset(void)
> +{
> +	const unsigned divisor = ((tclk + (8 * cfg->baud)) / (16 * cfg->baud));
> +
> +	pr_err("%s: triggering power-off...\n", __func__);
> +
> +	/* hijack UART1 and reset into sane state */
> +	writel(0x83, UART1_REG(LCR));
> +	writel(divisor & 0xff, UART1_REG(DLL));
> +	writel((divisor >> 8) & 0xff, UART1_REG(DLM));
> +	writel(cfg->magic[0], UART1_REG(LCR));
> +	writel(cfg->magic[1], UART1_REG(IER));
> +	writel(cfg->magic[2], UART1_REG(FCR));
> +	writel(cfg->magic[3], UART1_REG(MCR));
> +
> +	/* send the power-off command to PIC */
> +	if(cfg->cmd[0][0] == 1 && cfg->cmd[1][0] == 0) {
> +		/* if it's simply one-byte command, send it directly */
> +		writel(cfg->cmd[0][1], UART1_REG(TX));
> +	}

I guess this optimization can be dropped and you can directly
call the for loop with uart1_micon_send().

> +	else {
> +		int i;
> +		for(i = 0; cfg->cmd[i][0] > 0; i ++) {
> +			/* [0] is size of the command; command starts from [1] */
> +			uart1_micon_send(base, &(cfg->cmd[i][1]), cfg->cmd[i][0]);
> +		}
> +	}
> +}
> +
> +static int linkstation_reset_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct resource *res;
> +	struct clk *clk;
> +	char symname[KSYM_NAME_LEN];
> +
> +	const struct of_device_id *match =
> +		of_match_node(linkstation_reset_of_match_table, np);
> +	cfg = match->data;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Missing resource");
> +		return -EINVAL;
> +	}
> +
> +	base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> +	if (!base) {
> +		dev_err(&pdev->dev, "Unable to map resource");
> +		return -EINVAL;
> +	}
> +
> +	/* We need to know tclk in order to calculate the UART divisor */
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk)) {
> +		dev_err(&pdev->dev, "Clk missing");
> +		return PTR_ERR(clk);
> +	}
> +
> +	tclk = clk_get_rate(clk);
> +
> +	/* Check that nothing else has already setup a handler */
> +	if (pm_power_off) {
> +		lookup_symbol_name((ulong)pm_power_off, symname);
> +		dev_err(&pdev->dev,
> +			"pm_power_off already claimed %p %s",
> +			pm_power_off, symname);
> +		return -EBUSY;
> +	}
> +	pm_power_off = linkstation_reset;
> +
> +	return 0;
> +}
> +
> +static int linkstation_reset_remove(struct platform_device *pdev)
> +{
> +	pm_power_off = NULL;
> +	return 0;
> +}
> +
> +static struct platform_driver linkstation_reset_driver = {
> +	.probe	= linkstation_reset_probe,
> +	.remove	= linkstation_reset_remove,
> +	.driver	= {
> +		.name	= "linkstation_reset",
> +		.of_match_table = of_match_ptr(linkstation_reset_of_match_table),
> +	},
> +};
> +
> +module_platform_driver(linkstation_reset_driver);
> +
> +MODULE_AUTHOR("Roger Shimizu <rogershimizu@gmail.com>");
> +MODULE_DESCRIPTION("KuroBox Pro Reset driver");
> +MODULE_LICENSE("GPL v2");

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] power: reset: add linkstation-reset driver

[Andrew Lunn]

> > +	reset {
> > +		compatible = "linkstation,power-off";
> > +		reg = <0x12100 0x100>;
> > +		clocks = <&core_clk 0>;
> > +	};
> 
> This might be another user for UART slave device [0].
> [0] https://lkml.org/lkml/2016/8/24/769
> 
> Is the UART port used for anything else besides the reset
> controller?

I don't know much about these specific devices, but the qnap
equivalent, there is a user space daemon which also talks to the
microcontroller, for things like a temperature sensor, buzzer, etc.

https://www.hellion.org.uk/qcontrol/

So the UART can be in a messed up state, which is why the QNAP driver,
which this code is modelled on, reset it back to a good state.

      Andrew

Re: [PATCH v2] power: reset: add linkstation-reset driver

[Roger Shimizu]

Thanks for your review!

On Tue, Dec 20, 2016 at 1:03 AM, Andrew Lunn <andrew@lunn.ch> wrote:
>> > +   reset {
>> > +           compatible = "linkstation,power-off";
>> > +           reg = <0x12100 0x100>;
>> > +           clocks = <&core_clk 0>;
>> > +   };
>>
>> This might be another user for UART slave device [0].
>> [0] https://lkml.org/lkml/2016/8/24/769
>>
>> Is the UART port used for anything else besides the reset
>> controller?
>
> I don't know much about these specific devices, but the qnap
> equivalent, there is a user space daemon which also talks to the
> microcontroller, for things like a temperature sensor, buzzer, etc.
>
> https://www.hellion.org.uk/qcontrol/
>
> So the UART can be in a messed up state, which is why the QNAP driver,
> which this code is modelled on, reset it back to a good state.

For Linkstation/KuroBox-Pro, it's quite similar, and the user-land program is
called micro-evtd [0], which is co-maintained by Ryan Tandy and me in Debian.

[0] https://tracker.debian.org/pkg/micro-evtd

>> [0] https://lkml.org/lkml/2016/8/24/769

Do I need to modify anything related to the above UART slave device?

Cheers,
-- 
Roger Shimizu, GMT +9 Tokyo
PGP/GPG: 4096R/6C6ACD6417B3ACB1

Re: [PATCH v2] power: reset: add linkstation-reset driver

[Roger Shimizu]

Dear Sebastian,

Thanks for your review!

On Tue, Dec 20, 2016 at 12:38 AM, Sebastian Reichel <sre@kernel.org> wrote:
>>
>>  .../bindings/power/reset/linkstation-reset.txt     |  26 ++++
>>  drivers/power/reset/Kconfig                        |  10 ++
>>  drivers/power/reset/Makefile                       |   1 +
>>  drivers/power/reset/linkstation-common.c           | 124 +++++++++++++++
>>  drivers/power/reset/linkstation-common.h           |   8 +
>>  drivers/power/reset/linkstation-reset.c            | 172 +++++++++++++++++++++
>>  6 files changed, 341 insertions(+)
>
> With this being its own driver please merge linkstation-common and
> linkstation-reset. The common part is only used by linkstation-reset
> anyways.

I'll add them into To/Cc list.

>> +/* 4-byte magic hello command to UART1-attached microcontroller */
>> +static const unsigned char linkstation_micon_magic[] = {
>> +     0x1b,
>> +     0x00,
>> +     0x07,
>> +     0x00
>> +};
>
> 4-byte magic hello command? Those are used as uart configuration as
> far as I can see. Just move this directly into reset_cfg:
>
> struct reset_cfg {
>     u32 baud;
>     u8 lcr;
>     u8 ier;
>     u8 fcr;
>     u8 mcr;
>     const unsigned char (*cmd)[MICON_CMD_SIZE];
> };
>

>> +static void linkstation_reset(void)
>> +{
>> +     const unsigned divisor = ((tclk + (8 * cfg->baud)) / (16 * cfg->baud));
>> +
>> +     pr_err("%s: triggering power-off...\n", __func__);
>> +
>> +     /* hijack UART1 and reset into sane state */
>> +     writel(0x83, UART1_REG(LCR));
>> +     writel(divisor & 0xff, UART1_REG(DLL));
>> +     writel((divisor >> 8) & 0xff, UART1_REG(DLM));
>> +     writel(cfg->magic[0], UART1_REG(LCR));
>> +     writel(cfg->magic[1], UART1_REG(IER));
>> +     writel(cfg->magic[2], UART1_REG(FCR));
>> +     writel(cfg->magic[3], UART1_REG(MCR));
>> +
>> +     /* send the power-off command to PIC */
>> +     if(cfg->cmd[0][0] == 1 && cfg->cmd[1][0] == 0) {
>> +             /* if it's simply one-byte command, send it directly */
>> +             writel(cfg->cmd[0][1], UART1_REG(TX));
>> +     }
>
> I guess this optimization can be dropped and you can directly
> call the for loop with uart1_micon_send().

Same response regarding above two comments.
The code is extensible because I want to extend in the future.

Current implementation is just for Linkstation Pro / KuroBox Pro to be
able to power-off.
But for some other model of Linkstation, restart also need similar
command via UART1.

Just one example, Linkstation Pro is ARM based, but it was PowerPC based before.
And the device support still exists in kernel tree:
  arch/powerpc/platforms/embedded6xx/linkstation.c
  arch/powerpc/platforms/embedded6xx/ls_uart.c
It shows sending "C" to restart and sending "E" to power-off for
PowerPC based Linkstation.

I'm not actually interested in PowerPC based Linkstation, it's just an
example to show the reason to be flexible.

If other part is fine, may I send the v3 patch after merging
linkstation-common.c into linkstation-reset.c?
Thank you!

Cheers,
-- 
Roger Shimizu, GMT +9 Tokyo
PGP/GPG: 4096R/6C6ACD6417B3ACB1

Re: [PATCH v2] power: reset: add linkstation-reset driver

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2141 bytes --]

Hi,

On Tue, Dec 20, 2016 at 02:37:39AM +0900, Roger Shimizu wrote:
> [...]
>
> >> +static void linkstation_reset(void)
> >> +{
> >> +     const unsigned divisor = ((tclk + (8 * cfg->baud)) / (16 * cfg->baud));
> >> +
> >> +     pr_err("%s: triggering power-off...\n", __func__);
> >> +
> >> +     /* hijack UART1 and reset into sane state */
> >> +     writel(0x83, UART1_REG(LCR));
> >> +     writel(divisor & 0xff, UART1_REG(DLL));
> >> +     writel((divisor >> 8) & 0xff, UART1_REG(DLM));
> >> +     writel(cfg->magic[0], UART1_REG(LCR));
> >> +     writel(cfg->magic[1], UART1_REG(IER));
> >> +     writel(cfg->magic[2], UART1_REG(FCR));
> >> +     writel(cfg->magic[3], UART1_REG(MCR));
> >> +
> >> +     /* send the power-off command to PIC */
> >> +     if(cfg->cmd[0][0] == 1 && cfg->cmd[1][0] == 0) {
> >> +             /* if it's simply one-byte command, send it directly */
> >> +             writel(cfg->cmd[0][1], UART1_REG(TX));
> >> +     }
> >
> > I guess this optimization can be dropped and you can directly
> > call the for loop with uart1_micon_send().
> 
> Same response regarding above two comments.
> The code is extensible because I want to extend in the future.
> 
> Current implementation is just for Linkstation Pro / KuroBox Pro to be
> able to power-off.
> But for some other model of Linkstation, restart also need similar
> command via UART1.
>
> Just one example, Linkstation Pro is ARM based, but it was PowerPC based before.
> And the device support still exists in kernel tree:
>   arch/powerpc/platforms/embedded6xx/linkstation.c
>   arch/powerpc/platforms/embedded6xx/ls_uart.c
> It shows sending "C" to restart and sending "E" to power-off for
> PowerPC based Linkstation.
> 
> I'm not actually interested in PowerPC based Linkstation, it's just an
> example to show the reason to be flexible.
> 
> If other part is fine, may I send the v3 patch after merging
> linkstation-common.c into linkstation-reset.c?
> Thank you!

These models can just be added to qnap-poweroff, which handles
exactly this special case as far as I can see.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] power: reset: add linkstation-reset driver

[Andrew Lunn]

> These models can just be added to qnap-poweroff, which handles
> exactly this special case as far as I can see.

Nope, qnap is much, much simpler. The configuration of the serial port
is simpler, and it only needs to send a single byte. Here we have all
sorts of checksums to calculate, stuff coming back from the
microcontroller, etc. The complexity is much higher.

V1 of this patchset did extend the qnap driver. But in fact, very
little of the original code was left afterwards, and lots of new code
was added. So i requested a new driver be written, rather than extend
my qnap driver.

I would not like to see the nice and simple qnap driver get all this
code added to it, making it much harder to maintain, for very little
gain.

     Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] power: reset: add linkstation-reset driver

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1589 bytes --]

Hi Andrew,

On Wed, Dec 21, 2016 at 05:41:36PM +0100, Andrew Lunn wrote:
> > These models can just be added to qnap-poweroff, which handles
> > exactly this special case as far as I can see.
> 
> Nope, qnap is much, much simpler. The configuration of the serial port
> is simpler, and it only needs to send a single byte. Here we have all
> sorts of checksums to calculate, stuff coming back from the
> microcontroller, etc. The complexity is much higher.
> 
> V1 of this patchset did extend the qnap driver. But in fact, very
> little of the original code was left afterwards, and lots of new code
> was added. So i requested a new driver be written, rather than extend
> my qnap driver.
> 
> I would not like to see the nice and simple qnap driver get all this
> code added to it, making it much harder to maintain, for very little
> gain.

I'm talking about the special case, where it also sends only a
single byte:

> +     /* send the power-off command to PIC */
> +     if(cfg->cmd[0][0] == 1 && cfg->cmd[1][0] == 0) {
> +             /* if it's simply one-byte command, send it directly */
> +             writel(cfg->cmd[0][1], UART1_REG(TX));
> +     }

The configuration is different, but

a) probably it can just use the config from the qnap driver, since
   it just sends a single byte.
b) making the 4 config registers configurable in the qnap driver does
   not add much complexity.

So this case should be removed from the linkstation-reset driver. If
any board needs it, it should use the qnap-poweroff driver instead.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] power: reset: add linkstation-reset driver

[Roger Shimizu]

Dear Sebastian,

Thanks for your feedback!

On Thu, Dec 22, 2016 at 11:49 PM, Sebastian Reichel <sre@kernel.org> wrote:
>
> I'm talking about the special case, where it also sends only a
> single byte:
>
>> +     /* send the power-off command to PIC */
>> +     if(cfg->cmd[0][0] == 1 && cfg->cmd[1][0] == 0) {
>> +             /* if it's simply one-byte command, send it directly */
>> +             writel(cfg->cmd[0][1], UART1_REG(TX));
>> +     }
>
> The configuration is different, but
>
> a) probably it can just use the config from the qnap driver, since
>    it just sends a single byte.
> b) making the 4 config registers configurable in the qnap driver does
>    not add much complexity.
>
> So this case should be removed from the linkstation-reset driver. If
> any board needs it, it should use the qnap-poweroff driver instead.

Yes, if other board need sending one-byte command to UART1, it should
make use of qnap-poweroff driver.
However, one model of Linkstation need such kind of command, I would
like to add it in linkstation-reset driver.

Currently, there's no such usage, so if you don't like the redundant
code which sending one-byte command, I can remove in my next patch
series.

Merry Xmas and happy new year!

Cheers,
-- 
Roger Shimizu, GMT +9 Tokyo
PGP/GPG: 4096R/6C6ACD6417B3ACB1

[PATCH v3 0/3] make kurobox-pro be able to shutdown after device-tree migration

[Roger Shimizu]

Dear Kernel Maintainers,

Kurobox-Pro (and variants) need more commands sending to UART1 to shutdown.
So here I make this patch series to let current qnap-poweroff implementation
be able to handle such case.

Here I give 3 patches, for 3 different kernel subsystems respectivelay:
  - Patch 1/3: System reset/shutdown driver
  - Patch 2/3: Open firmware and flattened device tree bindings
  - Patch 3/3: ARM/Marvell Dove/MV78xx0/Orion SOC support

Ryan Tandy, the issue reporter, and I have already tested those changes on
Linkstation/KuroBoxPro devices, and confirmed it's working well.

Merry Xmax and look forward to your feedback!

Changes:
  v0 => v1:
  - Update 0003 to split kuroboxpro related code into kuroboxpro-common.c
  v1 => v2:
  - Split off linkstation/kuroboxpro related code to linkstation-reset.c
    Because linkstation before kuroboxpro also need this driver to power
    off properly. It's more proper to call it linkstation driver.
  v2 => v3:
  - Split off devicetree bindings doc into a separate patch.
  - Add device-tree patch to make Linkstation LS-GL and KuroBox Pro to
    use the newly created linkstation-reset driver.
  - Remove the unused one-byte command sending case in linkstation-reset
    driver.

Cheers,
Roger Shimizu, GMT +9 Tokyo
PGP/GPG: 4096R/6C6ACD6417B3ACB1

Roger Shimizu (3):
  power: reset: add linkstation-reset driver
  DT: bingdings: power: reset: add linkstation-reset doc
  ARM: DT: add power-off support to linkstation lsgl and kuroboxpro

 .../bindings/power/reset/linkstation-reset.txt     |  26 ++
 arch/arm/boot/dts/orion5x-kuroboxpro.dts           |   8 +
 arch/arm/boot/dts/orion5x-linkstation-lsgl.dts     |  10 +
 arch/arm/boot/dts/orion5x-linkstation-lswtgl.dts   |   4 +
 arch/arm/boot/dts/orion5x-linkstation.dtsi         |   4 -
 drivers/power/reset/Kconfig                        |  10 +
 drivers/power/reset/Makefile                       |   1 +
 drivers/power/reset/linkstation-reset.c            | 269 +++++++++++++++++++++
 8 files changed, 328 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/reset/linkstation-reset.txt
 create mode 100644 drivers/power/reset/linkstation-reset.c

-- 
2.11.0

[PATCH v3 1/3] power: reset: add linkstation-reset driver

[Roger Shimizu]

Buffalo Linkstation / KuroBox and their variants need magic command
sending to UART1 to power-off.

Power driver linkstation-reset implements the magic command and I/O
routine, which come from files listed below:
  - arch/arm/mach-orion5x/kurobox_pro-setup.c
  - arch/arm/mach-orion5x/terastation_pro2-setup.c

To: Sebastian Reichel <sre@kernel.org>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Martin Michlmayr <tbm@cyrius.com>
Cc: Sylver Bruneau <sylver.bruneau@googlemail.com>
Cc: Herbert Valerio Riedel <hvr@gnu.org>
Cc: Ryan Tandy <ryan@nardis.ca>
Cc: linux-pm@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org

Reported-by: Ryan Tandy <ryan@nardis.ca>
Tested-by: Ryan Tandy <ryan@nardis.ca>
Signed-off-by: Roger Shimizu <rogershimizu@gmail.com>
---
 drivers/power/reset/Kconfig             |  10 ++
 drivers/power/reset/Makefile            |   1 +
 drivers/power/reset/linkstation-reset.c | 269 ++++++++++++++++++++++++++++++++
 3 files changed, 280 insertions(+)
 create mode 100644 drivers/power/reset/linkstation-reset.c

diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index c74c3f67b8da..77c44cad7ece 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -98,6 +98,16 @@ config POWER_RESET_IMX
 	  say N here or disable in dts to make sure pm_power_off never be
 	  overwrote wrongly by this driver.
 
+config POWER_RESET_LINKSTATION
+	bool "Buffalo Linkstation and its variants reset driver"
+	depends on OF_GPIO && PLAT_ORION
+	help
+	  This driver supports power off Buffalo Linkstation / KuroBox Pro
+	  NAS and their variants by sending commands to the micro-controller
+	  which controls the main power.
+
+	  Say Y if you have a Buffalo Linkstation / KuroBox Pro NAS.
+
 config POWER_RESET_MSM
 	bool "Qualcomm MSM power-off driver"
 	depends on ARCH_QCOM
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index 1be307c7fc25..692ba6417cfb 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
 obj-$(CONFIG_POWER_RESET_GPIO_RESTART) += gpio-restart.o
 obj-$(CONFIG_POWER_RESET_HISI) += hisi-reboot.o
 obj-$(CONFIG_POWER_RESET_IMX) += imx-snvs-poweroff.o
+obj-$(CONFIG_POWER_RESET_LINKSTATION) += linkstation-reset.o
 obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
 obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o
 obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
diff --git a/drivers/power/reset/linkstation-reset.c b/drivers/power/reset/linkstation-reset.c
new file mode 100644
index 000000000000..4390495dfe0e
--- /dev/null
+++ b/drivers/power/reset/linkstation-reset.c
@@ -0,0 +1,269 @@
+/*
+ * Buffalo Linkstation power reset driver.
+ * It may also be used on following devices:
+ *  - KuroBox Pro
+ *  - Buffalo Linkstation Pro (LS-GL)
+ *  - Buffalo Terastation Pro II/Live
+ *  - Buffalo Linkstation Duo (LS-WTGL)
+ *  - Buffalo Linkstation Mini (LS-WSGL)
+ *
+ * Copyright (C) 2016  Roger Shimizu <rogershimizu@gmail.com>
+ *
+ * Based on the code from:
+ *
+ * Copyright (C) 2012  Andrew Lunn <andrew@lunn.ch>
+ * Copyright (C) 2009  Martin Michlmayr <tbm@cyrius.com>
+ * Copyright (C) 2008  Byron Bradley <byron.bbradley@gmail.com>
+ * Copyright (C) 2008  Sylver Bruneau <sylver.bruneau@googlemail.com>
+ * Copyright (C) 2007  Herbert Valerio Riedel <hvr@gnu.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/serial_reg.h>
+#include <linux/kallsyms.h>
+#include <linux/of.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+
+#define UART1_REG(x)	(base + ((UART_##x) << 2))
+#define MICON_CMD_SIZE	4
+
+/* 4-byte magic hello command to UART1-attached microcontroller */
+static const unsigned char linkstation_micon_magic[] = {
+	0x1b,
+	0x00,
+	0x07,
+	0x00
+};
+
+/* for each row, first byte is the size of command */
+static const unsigned char linkstation_power_off_cmd[][MICON_CMD_SIZE] = {
+	{ 3,	0x01, 0x35, 0x00},
+	{ 2,	0x00, 0x0c},
+	{ 2,	0x00, 0x06},
+	{}
+};
+
+struct reset_cfg {
+	u32 baud;
+	const unsigned char *magic;
+	const unsigned char (*cmd)[MICON_CMD_SIZE];
+};
+
+static const struct reset_cfg linkstation_power_off_cfg = {
+	.baud = 38400,
+	.magic = linkstation_micon_magic,
+	.cmd = linkstation_power_off_cmd,
+};
+
+static const struct of_device_id linkstation_reset_of_match_table[] = {
+	{ .compatible = "linkstation,power-off",
+	  .data = &linkstation_power_off_cfg,
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, linkstation_reset_of_match_table);
+
+static int uart1_micon_read(void *base, unsigned char *buf, int count)
+{
+	int i;
+	int timeout;
+
+	for (i = 0; i < count; i++) {
+		timeout = 10;
+
+		while (!(readl(UART1_REG(LSR)) & UART_LSR_DR)) {
+			if (--timeout == 0)
+				break;
+			udelay(1000);
+		}
+
+		if (timeout == 0)
+			break;
+		buf[i] = readl(UART1_REG(RX));
+	}
+
+	/* return read bytes */
+	return i;
+}
+
+static int uart1_micon_write(void *base, const unsigned char *buf, int count)
+{
+	int i = 0;
+
+	while (count--) {
+		while (!(readl(UART1_REG(LSR)) & UART_LSR_THRE))
+			barrier();
+		writel(buf[i++], UART1_REG(TX));
+	}
+
+	return 0;
+}
+
+int uart1_micon_send(void *base, const unsigned char *data, int count)
+{
+	int i;
+	unsigned char checksum = 0;
+	unsigned char recv_buf[40];
+	unsigned char send_buf[40];
+	unsigned char correct_ack[3];
+	int retry = 2;
+
+	/* Generate checksum */
+	for (i = 0; i < count; i++)
+		checksum -=  data[i];
+
+	do {
+		/* Send data */
+		uart1_micon_write(base, data, count);
+
+		/* send checksum */
+		uart1_micon_write(base, &checksum, 1);
+
+		if (uart1_micon_read(base, recv_buf, sizeof(recv_buf)) <= 3) {
+			printk(KERN_ERR ">%s: receive failed.\n", __func__);
+
+			/* send preamble to clear the receive buffer */
+			memset(&send_buf, 0xff, sizeof(send_buf));
+			uart1_micon_write(base, send_buf, sizeof(send_buf));
+
+			/* make dummy reads */
+			mdelay(100);
+			uart1_micon_read(base, recv_buf, sizeof(recv_buf));
+		} else {
+			/* Generate expected ack */
+			correct_ack[0] = 0x01;
+			correct_ack[1] = data[1];
+			correct_ack[2] = 0x00;
+
+			/* checksum Check */
+			if ((recv_buf[0] + recv_buf[1] + recv_buf[2] +
+			     recv_buf[3]) & 0xFF) {
+				printk(KERN_ERR ">%s: Checksum Error : "
+					"Received data[%02x, %02x, %02x, %02x]"
+					"\n", __func__, recv_buf[0],
+					recv_buf[1], recv_buf[2], recv_buf[3]);
+			} else {
+				/* Check Received Data */
+				if (correct_ack[0] == recv_buf[0] &&
+				    correct_ack[1] == recv_buf[1] &&
+				    correct_ack[2] == recv_buf[2]) {
+					/* Interval for next command */
+					mdelay(10);
+
+					/* Receive ACK */
+					return 0;
+				}
+			}
+			/* Received NAK or illegal Data */
+			printk(KERN_ERR ">%s: Error : NAK or Illegal Data "
+					"Received\n", __func__);
+		}
+	} while (retry--);
+
+	/* Interval for next command */
+	mdelay(10);
+
+	return -1;
+}
+
+static void __iomem *base;
+static unsigned long tclk;
+static const struct reset_cfg *cfg;
+
+static void linkstation_reset(void)
+{
+	const unsigned divisor = ((tclk + (8 * cfg->baud)) / (16 * cfg->baud));
+	int i;
+
+	pr_err("%s: triggering power-off...\n", __func__);
+
+	/* hijack UART1 and reset into sane state */
+	writel(0x83, UART1_REG(LCR));
+	writel(divisor & 0xff, UART1_REG(DLL));
+	writel((divisor >> 8) & 0xff, UART1_REG(DLM));
+	writel(cfg->magic[0], UART1_REG(LCR));
+	writel(cfg->magic[1], UART1_REG(IER));
+	writel(cfg->magic[2], UART1_REG(FCR));
+	writel(cfg->magic[3], UART1_REG(MCR));
+
+	/* send the power-off command to PIC */
+	for(i = 0; cfg->cmd[i][0] > 0; i ++) {
+		/* [0] is size of the command; command starts from [1] */
+		uart1_micon_send(base, &(cfg->cmd[i][1]), cfg->cmd[i][0]);
+	}
+}
+
+static int linkstation_reset_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct resource *res;
+	struct clk *clk;
+	char symname[KSYM_NAME_LEN];
+
+	const struct of_device_id *match =
+		of_match_node(linkstation_reset_of_match_table, np);
+	cfg = match->data;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "Missing resource");
+		return -EINVAL;
+	}
+
+	base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!base) {
+		dev_err(&pdev->dev, "Unable to map resource");
+		return -EINVAL;
+	}
+
+	/* We need to know tclk in order to calculate the UART divisor */
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk)) {
+		dev_err(&pdev->dev, "Clk missing");
+		return PTR_ERR(clk);
+	}
+
+	tclk = clk_get_rate(clk);
+
+	/* Check that nothing else has already setup a handler */
+	if (pm_power_off) {
+		lookup_symbol_name((ulong)pm_power_off, symname);
+		dev_err(&pdev->dev,
+			"pm_power_off already claimed %p %s",
+			pm_power_off, symname);
+		return -EBUSY;
+	}
+	pm_power_off = linkstation_reset;
+
+	return 0;
+}
+
+static int linkstation_reset_remove(struct platform_device *pdev)
+{
+	pm_power_off = NULL;
+	return 0;
+}
+
+static struct platform_driver linkstation_reset_driver = {
+	.probe	= linkstation_reset_probe,
+	.remove	= linkstation_reset_remove,
+	.driver	= {
+		.name	= "linkstation_reset",
+		.of_match_table = of_match_ptr(linkstation_reset_of_match_table),
+	},
+};
+
+module_platform_driver(linkstation_reset_driver);
+
+MODULE_AUTHOR("Roger Shimizu <rogershimizu@gmail.com>");
+MODULE_DESCRIPTION("Linkstation Reset driver");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

Re: [PATCH v3 1/3] power: reset: add linkstation-reset driver

[Florian Fainelli]

Hi Roger,

On 12/26/2016 11:06 PM, Roger Shimizu wrote:
> Buffalo Linkstation / KuroBox and their variants need magic command
> sending to UART1 to power-off.
> 
> Power driver linkstation-reset implements the magic command and I/O
> routine, which come from files listed below:
>   - arch/arm/mach-orion5x/kurobox_pro-setup.c
>   - arch/arm/mach-orion5x/terastation_pro2-setup.c

Interestingly, I submitted a patch doing nearly the same thing recently
after hacking on a Buffalo Terastation Pro II two days after yours
without seeing yours:

https://lkml.org/lkml/2016/12/28/273

Some comments below.

> +
> +static void __iomem *base;
> +static unsigned long tclk;
> +static const struct reset_cfg *cfg;

How about avoiding the singletons here and pass this down from the
platform driver's private data after we (see below) also make use of a
reboot notifier?

> +
> +static void linkstation_reset(void)
> +{
> +	const unsigned divisor = ((tclk + (8 * cfg->baud)) / (16 * cfg->baud));
> +	int i;
> +
> +	pr_err("%s: triggering power-off...\n", __func__);
> +
> +	/* hijack UART1 and reset into sane state */
> +	writel(0x83, UART1_REG(LCR));
> +	writel(divisor & 0xff, UART1_REG(DLL));
> +	writel((divisor >> 8) & 0xff, UART1_REG(DLM));
> +	writel(cfg->magic[0], UART1_REG(LCR));
> +	writel(cfg->magic[1], UART1_REG(IER));
> +	writel(cfg->magic[2], UART1_REG(FCR));
> +	writel(cfg->magic[3], UART1_REG(MCR));
> +
> +	/* send the power-off command to PIC */
> +	for(i = 0; cfg->cmd[i][0] > 0; i ++) {
> +		/* [0] is size of the command; command starts from [1] */
> +		uart1_micon_send(base, &(cfg->cmd[i][1]), cfg->cmd[i][0]);
> +	}
> +}
> +
> +static int linkstation_reset_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct resource *res;
> +	struct clk *clk;
> +	char symname[KSYM_NAME_LEN];
> +
> +	const struct of_device_id *match =
> +		of_match_node(linkstation_reset_of_match_table, np);
> +	cfg = match->data;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Missing resource");
> +		return -EINVAL;
> +	}
> +
> +	base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> +	if (!base) {
> +		dev_err(&pdev->dev, "Unable to map resource");
> +		return -EINVAL;
> +	}
> +
> +	/* We need to know tclk in order to calculate the UART divisor */
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk)) {
> +		dev_err(&pdev->dev, "Clk missing");
> +		return PTR_ERR(clk);
> +	}
> +
> +	tclk = clk_get_rate(clk);

Does this work with the Terastation II which has not been converted to
DT yet? Is tclk available through the CLK API there?

> +
> +	/* Check that nothing else has already setup a handler */
> +	if (pm_power_off) {
> +		lookup_symbol_name((ulong)pm_power_off, symname);
> +		dev_err(&pdev->dev,
> +			"pm_power_off already claimed %p %s",
> +			pm_power_off, symname);
> +		return -EBUSY;
> +	}
> +	pm_power_off = linkstation_reset;

That seems a bit complicated, why not just assume that there will be
either this driver used, or not at all?

Also, you are supposed to register a reboot notifier to which you can
pass private context:

https://lkml.org/lkml/2016/12/28/275

As indicated in my patch series, the UART1-attached micro controller
does a lot more things that just providing reboot, which is why I chose
to move this code to a MFD driver, as the starting point before adding
support for LEDs, FAN, PWM, beeper which would be other types of devices.

Is adding support for other peripherals on your TODO as well?

Thanks!
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] power: reset: add linkstation-reset driver

[Andrew Lunn]

> > +
> > +	/* Check that nothing else has already setup a handler */
> > +	if (pm_power_off) {
> > +		lookup_symbol_name((ulong)pm_power_off, symname);
> > +		dev_err(&pdev->dev,
> > +			"pm_power_off already claimed %p %s",
> > +			pm_power_off, symname);
> > +		return -EBUSY;
> > +	}
> > +	pm_power_off = linkstation_reset;
> 
> That seems a bit complicated, why not just assume that there will be
> either this driver used, or not at all?

That is probably my fault. This is a copy from code i wrote many years
ago for the QNAP. I guess at the time i was battling with two
different pm_power_off handlers, so put in this code.

> Also, you are supposed to register a reboot notifier to which you can
> pass private context:

At the time i wrote the QNAP code, this did not exist. So maybe my
code is no longer a good example to copy.

	 Andrew

Re: [PATCH v3 1/3] power: reset: add linkstation-reset driver

[Roger Shimizu]

Dear Florian, Andrew,

Thanks for your email!

On Tue, Jan 3, 2017 at 2:19 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> Interestingly, I submitted a patch doing nearly the same thing recently
> after hacking on a Buffalo Terastation Pro II two days after yours
> without seeing yours:
>
> https://lkml.org/lkml/2016/12/28/273

Glad to know there's other developer working on linkstation/kurobox platform!

> Some comments below.
>
>> +
>> +static void __iomem *base;
>> +static unsigned long tclk;
>> +static const struct reset_cfg *cfg;
>
> How about avoiding the singletons here and pass this down from the
> platform driver's private data after we (see below) also make use of a
> reboot notifier?

I see your patches. Indeed, it's a good idea to avoid the singletons
and use private data instead.

>> +static int linkstation_reset_probe(struct platform_device *pdev)
>> +{
>> +     struct device_node *np = pdev->dev.of_node;
>> +     struct resource *res;
>> +     struct clk *clk;
>> +     char symname[KSYM_NAME_LEN];
>> +
>> +     const struct of_device_id *match =
>> +             of_match_node(linkstation_reset_of_match_table, np);
>> +     cfg = match->data;
>> +
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     if (!res) {
>> +             dev_err(&pdev->dev, "Missing resource");
>> +             return -EINVAL;
>> +     }
>> +
>> +     base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>> +     if (!base) {
>> +             dev_err(&pdev->dev, "Unable to map resource");
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* We need to know tclk in order to calculate the UART divisor */
>> +     clk = devm_clk_get(&pdev->dev, NULL);
>> +     if (IS_ERR(clk)) {
>> +             dev_err(&pdev->dev, "Clk missing");
>> +             return PTR_ERR(clk);
>> +     }
>> +
>> +     tclk = clk_get_rate(clk);
>
> Does this work with the Terastation II which has not been converted to
> DT yet? Is tclk available through the CLK API there?

I have no idea whether device-based legacy code and make use of power
reset driver.
Maybe Sebastian Reichel can offer a comment?

However, I think Terastation II should convert to DT first.
If you're willing to test, I can help to provide a dts/dtb.
(If you use Debian, I can even provide DEB of kernel image and
flash-kernel patch, which is easy for you to test)

On Tue, Jan 3, 2017 at 10:09 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> > +
>> > +   /* Check that nothing else has already setup a handler */
>> > +   if (pm_power_off) {
>> > +           lookup_symbol_name((ulong)pm_power_off, symname);
>> > +           dev_err(&pdev->dev,
>> > +                   "pm_power_off already claimed %p %s",
>> > +                   pm_power_off, symname);
>> > +           return -EBUSY;
>> > +   }
>> > +   pm_power_off = linkstation_reset;
>>
>> That seems a bit complicated, why not just assume that there will be
>> either this driver used, or not at all?
>
> That is probably my fault. This is a copy from code i wrote many years
> ago for the QNAP. I guess at the time i was battling with two
> different pm_power_off handlers, so put in this code.
>
>> Also, you are supposed to register a reboot notifier to which you can
>> pass private context:
>
> At the time i wrote the QNAP code, this did not exist. So maybe my
> code is no longer a good example to copy.

Really appreciated, Andrew!
I'll modify this part in next series.

BTW. the private data passing to reboot notifier can be shared to
power-off function as well?
Do you have example?

> https://lkml.org/lkml/2016/12/28/275
>
> As indicated in my patch series, the UART1-attached micro controller
> does a lot more things that just providing reboot, which is why I chose
> to move this code to a MFD driver, as the starting point before adding
> support for LEDs, FAN, PWM, beeper which would be other types of devices.
>
> Is adding support for other peripherals on your TODO as well?

Except reset feature (power-off and reboot), other feature, such as
LEDs / FAN speed / buttons, is managed by user-land program micro-evtd
[0][1].
Since the upstream [1] is not active anymore, Ryan Tandy (in CC) and I
are maintaining it in Debian [0].

[0] https://tracker.debian.org/pkg/micro-evtd
[1] https://sourceforge.net/projects/ppc-evtd

micro-evtd worked well for device-based legacy code, but after DT
conversion, Ryan found the device cannot shutdown properly (reboot is
OK).
That why I created this patch series.
I think for such old hardware and mature user-land program, it doesn't
deserve the effort to implement those again in kernel side.
What do you think?

Cheers,
-- 
Roger Shimizu, GMT +9 Tokyo
PGP/GPG: 4096R/6C6ACD6417B3ACB1

Re: [PATCH v3 1/3] power: reset: add linkstation-reset driver

[Florian Fainelli]

On 01/03/2017 06:08 AM, Roger Shimizu wrote:
> Dear Florian, Andrew,
> 
> Thanks for your email!
> 
> On Tue, Jan 3, 2017 at 2:19 PM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>> Interestingly, I submitted a patch doing nearly the same thing recently
>> after hacking on a Buffalo Terastation Pro II two days after yours
>> without seeing yours:
>>
>> https://lkml.org/lkml/2016/12/28/273
> 
> Glad to know there's other developer working on linkstation/kurobox platform!
> 
>> Some comments below.
>>
>>> +
>>> +static void __iomem *base;
>>> +static unsigned long tclk;
>>> +static const struct reset_cfg *cfg;
>>
>> How about avoiding the singletons here and pass this down from the
>> platform driver's private data after we (see below) also make use of a
>> reboot notifier?
> 
> I see your patches. Indeed, it's a good idea to avoid the singletons
> and use private data instead.
> 
>>> +static int linkstation_reset_probe(struct platform_device *pdev)
>>> +{
>>> +     struct device_node *np = pdev->dev.of_node;
>>> +     struct resource *res;
>>> +     struct clk *clk;
>>> +     char symname[KSYM_NAME_LEN];
>>> +
>>> +     const struct of_device_id *match =
>>> +             of_match_node(linkstation_reset_of_match_table, np);
>>> +     cfg = match->data;
>>> +
>>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +     if (!res) {
>>> +             dev_err(&pdev->dev, "Missing resource");
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>>> +     if (!base) {
>>> +             dev_err(&pdev->dev, "Unable to map resource");
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     /* We need to know tclk in order to calculate the UART divisor */
>>> +     clk = devm_clk_get(&pdev->dev, NULL);
>>> +     if (IS_ERR(clk)) {
>>> +             dev_err(&pdev->dev, "Clk missing");
>>> +             return PTR_ERR(clk);
>>> +     }
>>> +
>>> +     tclk = clk_get_rate(clk);
>>
>> Does this work with the Terastation II which has not been converted to
>> DT yet? Is tclk available through the CLK API there?
> 
> I have no idea whether device-based legacy code and make use of power
> reset driver.
> Maybe Sebastian Reichel can offer a comment?
> 
> However, I think Terastation II should convert to DT first.
> If you're willing to test, I can help to provide a dts/dtb.
> (If you use Debian, I can even provide DEB of kernel image and
> flash-kernel patch, which is easy for you to test)
> 
> On Tue, Jan 3, 2017 at 10:09 PM, Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org> wrote:
>>>> +
>>>> +   /* Check that nothing else has already setup a handler */
>>>> +   if (pm_power_off) {
>>>> +           lookup_symbol_name((ulong)pm_power_off, symname);
>>>> +           dev_err(&pdev->dev,
>>>> +                   "pm_power_off already claimed %p %s",
>>>> +                   pm_power_off, symname);
>>>> +           return -EBUSY;
>>>> +   }
>>>> +   pm_power_off = linkstation_reset;
>>>
>>> That seems a bit complicated, why not just assume that there will be
>>> either this driver used, or not at all?
>>
>> That is probably my fault. This is a copy from code i wrote many years
>> ago for the QNAP. I guess at the time i was battling with two
>> different pm_power_off handlers, so put in this code.
>>
>>> Also, you are supposed to register a reboot notifier to which you can
>>> pass private context:
>>
>> At the time i wrote the QNAP code, this did not exist. So maybe my
>> code is no longer a good example to copy.
> 
> Really appreciated, Andrew!
> I'll modify this part in next series.
> 
> BTW. the private data passing to reboot notifier can be shared to
> power-off function as well?
> Do you have example?
> 
>> https://lkml.org/lkml/2016/12/28/275
>>
>> As indicated in my patch series, the UART1-attached micro controller
>> does a lot more things that just providing reboot, which is why I chose
>> to move this code to a MFD driver, as the starting point before adding
>> support for LEDs, FAN, PWM, beeper which would be other types of devices.
>>
>> Is adding support for other peripherals on your TODO as well?
> 
> Except reset feature (power-off and reboot), other feature, such as
> LEDs / FAN speed / buttons, is managed by user-land program micro-evtd
> [0][1].
> Since the upstream [1] is not active anymore, Ryan Tandy (in CC) and I
> are maintaining it in Debian [0].
> 
> [0] https://tracker.debian.org/pkg/micro-evtd
> [1] https://sourceforge.net/projects/ppc-evtd
> 
> micro-evtd worked well for device-based legacy code, but after DT
> conversion, Ryan found the device cannot shutdown properly (reboot is
> OK).
> That why I created this patch series.
> I think for such old hardware and mature user-land program, it doesn't
> deserve the effort to implement those again in kernel side.
> What do you think?

An argument could be made that these are hardware peripherals, and so
the kernel is the best place to abstract support for that, and present
you with the standard device drive model for such kind of peripherals.

We don't have to do everything at the same time, so first things first,
get the reboot working, have me convert the Terastation over to Device
Tree and then we can decide what to do with these additional peripherals.

Thanks!
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/3] DT: bingdings: power: reset: add linkstation-reset doc

[Roger Shimizu]

Add linkstation-reset doc to describe the newly added
POWER_RESET_LINKSTATION driver, which controls magic command
sending to UART1 to power-off Buffalo Linkstation / KuroBox
and their variants.

To: Sebastian Reichel <sre@kernel.org>
To: Rob Herring <robh+dt@kernel.org>
To: Mark Rutland <mark.rutland@arm.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Ryan Tandy <ryan@nardis.ca>
Cc: linux-pm@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org

Signed-off-by: Roger Shimizu <rogershimizu@gmail.com>
---
 .../bindings/power/reset/linkstation-reset.txt     | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/reset/linkstation-reset.txt

diff --git a/Documentation/devicetree/bindings/power/reset/linkstation-reset.txt b/Documentation/devicetree/bindings/power/reset/linkstation-reset.txt
new file mode 100644
index 000000000000..815e340318f3
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/linkstation-reset.txt
@@ -0,0 +1,26 @@
+* Buffalo Linkstation Reset Driver
+
+Power of some Buffalo Linkstation or KuroBox Pro is managed by
+micro-controller, which connects to UART1. After being fed from UART1
+by a few magic numbers, the so-called power-off command,
+the micro-controller will turn power off the device.
+
+This is very similar to QNAP or Synology NAS devices, which is
+described in qnap-poweroff.txt, however the command is much simpler,
+only 1-byte long and without checksums.
+
+This driver adds a handler to pm_power_off which is called to turn the
+power off.
+
+Required Properties:
+- compatible: Should be "linkstation,power-off"
+- reg: Address and length of the register set for UART1
+- clocks: tclk clock
+
+Example:
+
+	reset {
+		compatible = "linkstation,power-off";
+		reg = <0x12100 0x100>;
+		clocks = <&core_clk 0>;
+	};
-- 
2.11.0

Re: [PATCH v3 2/3] DT: bingdings: power: reset: add linkstation-reset doc

[Florian Fainelli]

On 12/26/2016 11:06 PM, Roger Shimizu wrote:
> Add linkstation-reset doc to describe the newly added
> POWER_RESET_LINKSTATION driver, which controls magic command
> sending to UART1 to power-off Buffalo Linkstation / KuroBox
> and their variants.
> 
> To: Sebastian Reichel <sre@kernel.org>
> To: Rob Herring <robh+dt@kernel.org>
> To: Mark Rutland <mark.rutland@arm.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Ryan Tandy <ryan@nardis.ca>
> Cc: linux-pm@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> 
> Signed-off-by: Roger Shimizu <rogershimizu@gmail.com>
> ---
>  .../bindings/power/reset/linkstation-reset.txt     | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/reset/linkstation-reset.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/linkstation-reset.txt b/Documentation/devicetree/bindings/power/reset/linkstation-reset.txt
> new file mode 100644
> index 000000000000..815e340318f3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/linkstation-reset.txt
> @@ -0,0 +1,26 @@
> +* Buffalo Linkstation Reset Driver
> +
> +Power of some Buffalo Linkstation or KuroBox Pro is managed by
> +micro-controller, which connects to UART1. After being fed from UART1
> +by a few magic numbers, the so-called power-off command,
> +the micro-controller will turn power off the device.
> +
> +This is very similar to QNAP or Synology NAS devices, which is
> +described in qnap-poweroff.txt, however the command is much simpler,
> +only 1-byte long and without checksums.
> +
> +This driver adds a handler to pm_power_off which is called to turn the
> +power off.

This is a driver implementation detail, so does not really belong in the
DT here.

> +
> +Required Properties:
> +- compatible: Should be "linkstation,power-off"
> +- reg: Address and length of the register set for UART1

Humm, should we instead have a phandle to the uart1 node?

> +- clocks: tclk clock
> +
> +Example:
> +
> +	reset {
> +		compatible = "linkstation,power-off";
> +		reg = <0x12100 0x100>;
> +		clocks = <&core_clk 0>;
> +	};
> 

-- 
Florian

Re: [PATCH v3 2/3] DT: bingdings: power: reset: add linkstation-reset doc

[Andrew Lunn]

> > +
> > +Required Properties:
> > +- compatible: Should be "linkstation,power-off"
> > +- reg: Address and length of the register set for UART1
> 
> Humm, should we instead have a phandle to the uart1 node?

Probably. Again, this is to do with copying the QNAP driver. I was
young, new to device tree, and i just did a logical conversion of the
existing code, and did not at the time understand phandles, etc.

	 Andrew

Re: [PATCH v3 2/3] DT: bingdings: power: reset: add linkstation-reset doc

[Roger Shimizu]

Dear Florian, Andrew,

Thanks for your comments!

On Tue, Jan 3, 2017 at 10:12 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> > +
>> > +Required Properties:
>> > +- compatible: Should be "linkstation,power-off"
>> > +- reg: Address and length of the register set for UART1
>>
>> Humm, should we instead have a phandle to the uart1 node?
>
> Probably. Again, this is to do with copying the QNAP driver. I was
> young, new to device tree, and i just did a logical conversion of the
> existing code, and did not at the time understand phandles, etc.

Can you tell me where should I place this document?
or simply remove it?

Cheers,
-- 
Roger Shimizu, GMT +9 Tokyo
PGP/GPG: 4096R/6C6ACD6417B3ACB1

Re: [PATCH v3 2/3] DT: bingdings: power: reset: add linkstation-reset doc

[Rob Herring]

On Tue, Dec 27, 2016 at 04:06:10PM +0900, Roger Shimizu wrote:
> Add linkstation-reset doc to describe the newly added
> POWER_RESET_LINKSTATION driver, which controls magic command
> sending to UART1 to power-off Buffalo Linkstation / KuroBox
> and their variants.
> 
> To: Sebastian Reichel <sre@kernel.org>
> To: Rob Herring <robh+dt@kernel.org>
> To: Mark Rutland <mark.rutland@arm.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Ryan Tandy <ryan@nardis.ca>
> Cc: linux-pm@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> 
> Signed-off-by: Roger Shimizu <rogershimizu@gmail.com>
> ---
>  .../bindings/power/reset/linkstation-reset.txt     | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/reset/linkstation-reset.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/linkstation-reset.txt b/Documentation/devicetree/bindings/power/reset/linkstation-reset.txt
> new file mode 100644
> index 000000000000..815e340318f3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/linkstation-reset.txt
> @@ -0,0 +1,26 @@
> +* Buffalo Linkstation Reset Driver
> +
> +Power of some Buffalo Linkstation or KuroBox Pro is managed by
> +micro-controller, which connects to UART1. After being fed from UART1
> +by a few magic numbers, the so-called power-off command,
> +the micro-controller will turn power off the device.

This needs to model the uC connected to the UART rather than some node 
that defines only some portion of the functionality. I'm working on 
bindings and proper bus support for this[1], but it's not done yet. 
Though, the binding side is pretty simple.

Rob

[1] https://git.kernel.org/cgit/linux/kernel/git/robh/linux.git/log/?h=serial-bus-v2

Re: [PATCH v3 2/3] DT: bingdings: power: reset: add linkstation-reset doc

[Roger Shimizu]

Dear Rob,

Thanks for your comments!

On Wed, Jan 4, 2017 at 2:09 AM, Rob Herring <robh@kernel.org> wrote:
>
> This needs to model the uC connected to the UART rather than some node
> that defines only some portion of the functionality. I'm working on
> bindings and proper bus support for this[1], but it's not done yet.
> Though, the binding side is pretty simple.
>
> Rob
>
> [1] https://git.kernel.org/cgit/linux/kernel/git/robh/linux.git/log/?h=serial-bus-v2

Actually I have limited knowledge on DT binding.
I think it should be OK to remove the 2/3 patch (this thread) in my
next series, and I'll do my homework when you finish it.
Thank you!

Cheers,
-- 
Roger Shimizu, GMT +9 Tokyo
PGP/GPG: 4096R/6C6ACD6417B3ACB1

[PATCH v3 3/3] ARM: DT: add power-off support to linkstation lsgl and kuroboxpro

[Roger Shimizu]

A few models of Linkstation / KuroBox has micro-controller which
controls the power (as well as FAN, but not related here), while
others not. So remove the restart-poweroff driver in linkstation
common dtsi file, and specify the proper power driver in dts
of each device.

Devices need micro-controler to power-off:
  - Linkstation LS-GL
  - KuroBox Pro
Device continues using original restart-poweroff driver:
  - Linkstation LS-WTGL

To: Jason Cooper <jason@lakedaemon.net>
To: Andrew Lunn <andrew@lunn.ch>
To: Gregory Clement <gregory.clement@free-electrons.com>
To: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Ryan Tandy <ryan@nardis.ca>
Cc: linux-pm@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org

Reported-by: Ryan Tandy <ryan@nardis.ca>
Tested-by: Ryan Tandy <ryan@nardis.ca>
Signed-off-by: Roger Shimizu <rogershimizu@gmail.com>
---
 arch/arm/boot/dts/orion5x-kuroboxpro.dts         |  8 ++++++++
 arch/arm/boot/dts/orion5x-linkstation-lsgl.dts   | 10 ++++++++++
 arch/arm/boot/dts/orion5x-linkstation-lswtgl.dts |  4 ++++
 arch/arm/boot/dts/orion5x-linkstation.dtsi       |  4 ----
 4 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/orion5x-kuroboxpro.dts b/arch/arm/boot/dts/orion5x-kuroboxpro.dts
index 1a672b098d0b..aba38d802bda 100644
--- a/arch/arm/boot/dts/orion5x-kuroboxpro.dts
+++ b/arch/arm/boot/dts/orion5x-kuroboxpro.dts
@@ -60,6 +60,14 @@
 				 <MBUS_ID(0x09, 0x00) 0 0xf2200000 0x800>,
 				 <MBUS_ID(0x01, 0x0f) 0 0xf4000000 0x40000>,
 				 <MBUS_ID(0x01, 0x1e) 0 0xfc000000 0x1000000>;
+
+		internal-regs {
+			power_off {
+				compatible = "linkstation,power-off";
+				reg = <0x12100 0x100>;
+				clocks = <&core_clk 0>;
+			};
+		};
 	};
 
 	memory { /* 128 MB */
diff --git a/arch/arm/boot/dts/orion5x-linkstation-lsgl.dts b/arch/arm/boot/dts/orion5x-linkstation-lsgl.dts
index 51dc734cd5b9..370fc17a6dd9 100644
--- a/arch/arm/boot/dts/orion5x-linkstation-lsgl.dts
+++ b/arch/arm/boot/dts/orion5x-linkstation-lsgl.dts
@@ -56,6 +56,16 @@
 	model = "Buffalo Linkstation Pro/Live";
 	compatible = "buffalo,lsgl", "marvell,orion5x-88f5182", "marvell,orion5x";
 
+	soc {
+		internal-regs {
+			power_off {
+				compatible = "linkstation,power-off";
+				reg = <0x12100 0x100>;
+				clocks = <&core_clk 0>;
+			};
+		};
+	};
+
 	memory { /* 128 MB */
 		device_type = "memory";
 		reg = <0x00000000 0x8000000>;
diff --git a/arch/arm/boot/dts/orion5x-linkstation-lswtgl.dts b/arch/arm/boot/dts/orion5x-linkstation-lswtgl.dts
index 0eead400f427..571a71f5b7ad 100644
--- a/arch/arm/boot/dts/orion5x-linkstation-lswtgl.dts
+++ b/arch/arm/boot/dts/orion5x-linkstation-lswtgl.dts
@@ -59,6 +59,10 @@
 		reg = <0x00000000 0x4000000>;
 	};
 
+	restart_poweroff {
+		compatible = "restart-poweroff";
+	};
+
 	gpio_keys {
 		power-on-switch {
 			gpios = <&gpio0 8 GPIO_ACTIVE_LOW>;
diff --git a/arch/arm/boot/dts/orion5x-linkstation.dtsi b/arch/arm/boot/dts/orion5x-linkstation.dtsi
index ed456ab35fd8..40770a8d4b36 100644
--- a/arch/arm/boot/dts/orion5x-linkstation.dtsi
+++ b/arch/arm/boot/dts/orion5x-linkstation.dtsi
@@ -57,10 +57,6 @@
 				 <MBUS_ID(0x01, 0x0f) 0 0xf4000000 0x40000>;
 	};
 
-	restart_poweroff {
-		compatible = "restart-poweroff";
-	};
-
 	regulators {
 		compatible = "simple-bus";
 		#address-cells = <1>;
-- 
2.11.0

[PATCH v4 0/2] make kurobox-pro be able to shutdown after device-tree migration

[Roger Shimizu]

Dear Kernel Maintainers,

Kurobox-Pro (and variants) need more commands sending to UART1 to shutdown.
So here I make this patch series to let current qnap-poweroff implementation
be able to handle such case.

Here I give 3 patches, for 3 different kernel subsystems respectivelay:
  - Patch 1/3: System reset/shutdown driver
  - Patch 2/3: Open firmware and flattened device tree bindings
  - Patch 3/3: ARM/Marvell Dove/MV78xx0/Orion SOC support

Ryan Tandy, the issue reporter, and I have already tested those changes on
Linkstation/KuroBoxPro devices, and confirmed it's working well.

Merry Xmax and look forward to your feedback!

Changes:
  v0 => v1:
  - Update 0003 to split kuroboxpro related code into kuroboxpro-common.c
  v1 => v2:
  - Split off linkstation/kuroboxpro related code to linkstation-reset.c
    Because linkstation before kuroboxpro also need this driver to power
    off properly. It's more proper to call it linkstation driver.
  v2 => v3:
  - Split off devicetree bindings doc into a separate patch.
  - Add device-tree patch to make Linkstation LS-GL and KuroBox Pro to
    use the newly created linkstation-reset driver.
  - Remove the unused one-byte command sending case in linkstation-reset
    driver.
  v3 => v4:
  - Reduce the global singleton variables by moving to structure, which makes
    the driver easy to support reboot function in the future.  Thanks to
    Florian Fainelli.
  - Simplify the power_off function registration. Thanks to Florian Fainelli.
  - Drop DT binding doc.

Cheers,
Roger Shimizu, GMT +9 Tokyo
PGP/GPG: 4096R/6C6ACD6417B3ACB1

Roger Shimizu (2):
  power: reset: add linkstation-reset driver
  ARM: DT: add power-off support to linkstation lsgl and kuroboxpro

 arch/arm/boot/dts/orion5x-kuroboxpro.dts         |   8 +
 arch/arm/boot/dts/orion5x-linkstation-lsgl.dts   |  10 +
 arch/arm/boot/dts/orion5x-linkstation-lswtgl.dts |   4 +
 arch/arm/boot/dts/orion5x-linkstation.dtsi       |   4 -
 drivers/power/reset/Kconfig                      |  10 +
 drivers/power/reset/Makefile                     |   1 +
 drivers/power/reset/linkstation-reset.c          | 270 +++++++++++++++++++++++
 7 files changed, 303 insertions(+), 4 deletions(-)
 create mode 100644 drivers/power/reset/linkstation-reset.c

-- 
2.11.0

[PATCH v4 1/2] power: reset: add linkstation-reset driver

[Roger Shimizu]

Buffalo Linkstation / KuroBox and their variants need magic command
sending to UART1 to power-off.

Power driver linkstation-reset implements the magic command and I/O
routine, which come from files listed below:
  - arch/arm/mach-orion5x/kurobox_pro-setup.c
  - arch/arm/mach-orion5x/terastation_pro2-setup.c

To: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
Cc: Martin Michlmayr <tbm-R+vWnYXSFMfQT0dZR+AlfA@public.gmane.org>
Cc: Sylver Bruneau <sylver.bruneau-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
Cc: Herbert Valerio Riedel <hvr-mXXj517/zsQ@public.gmane.org>
Cc: Ryan Tandy <ryan-pRYBVO4bdZ33fQ9qLvQP4Q@public.gmane.org>
Cc: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org

Reported-by: Ryan Tandy <ryan-pRYBVO4bdZ33fQ9qLvQP4Q@public.gmane.org>
Tested-by: Ryan Tandy <ryan-pRYBVO4bdZ33fQ9qLvQP4Q@public.gmane.org>
Signed-off-by: Roger Shimizu <rogershimizu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/power/reset/Kconfig             |  10 ++
 drivers/power/reset/Makefile            |   1 +
 drivers/power/reset/linkstation-reset.c | 270 ++++++++++++++++++++++++++++++++
 3 files changed, 281 insertions(+)
 create mode 100644 drivers/power/reset/linkstation-reset.c

diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index c74c3f67b8da..77c44cad7ece 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -98,6 +98,16 @@ config POWER_RESET_IMX
 	  say N here or disable in dts to make sure pm_power_off never be
 	  overwrote wrongly by this driver.
 
+config POWER_RESET_LINKSTATION
+	bool "Buffalo Linkstation and its variants reset driver"
+	depends on OF_GPIO && PLAT_ORION
+	help
+	  This driver supports power off Buffalo Linkstation / KuroBox Pro
+	  NAS and their variants by sending commands to the micro-controller
+	  which controls the main power.
+
+	  Say Y if you have a Buffalo Linkstation / KuroBox Pro NAS.
+
 config POWER_RESET_MSM
 	bool "Qualcomm MSM power-off driver"
 	depends on ARCH_QCOM
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index 1be307c7fc25..692ba6417cfb 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
 obj-$(CONFIG_POWER_RESET_GPIO_RESTART) += gpio-restart.o
 obj-$(CONFIG_POWER_RESET_HISI) += hisi-reboot.o
 obj-$(CONFIG_POWER_RESET_IMX) += imx-snvs-poweroff.o
+obj-$(CONFIG_POWER_RESET_LINKSTATION) += linkstation-reset.o
 obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
 obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o
 obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
diff --git a/drivers/power/reset/linkstation-reset.c b/drivers/power/reset/linkstation-reset.c
new file mode 100644
index 000000000000..c191b7671076
--- /dev/null
+++ b/drivers/power/reset/linkstation-reset.c
@@ -0,0 +1,270 @@
+/*
+ * Buffalo Linkstation power reset driver.
+ * It may also be used on following devices:
+ *  - KuroBox Pro
+ *  - Buffalo Linkstation Pro (LS-GL)
+ *  - Buffalo Terastation Pro II/Live
+ *  - Buffalo Linkstation Duo (LS-WTGL)
+ *  - Buffalo Linkstation Mini (LS-WSGL)
+ *
+ * Copyright (C) 2016  Roger Shimizu <rogershimizu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *
+ * Based on the code from:
+ *
+ * Copyright (C) 2012  Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
+ * Copyright (C) 2009  Martin Michlmayr <tbm-R+vWnYXSFMfQT0dZR+AlfA@public.gmane.org>
+ * Copyright (C) 2008  Byron Bradley <byron.bbradley-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ * Copyright (C) 2008  Sylver Bruneau <sylver.bruneau-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
+ * Copyright (C) 2007  Herbert Valerio Riedel <hvr-mXXj517/zsQ@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/serial_reg.h>
+#include <linux/kallsyms.h>
+#include <linux/of.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+
+#define UART1_REG(x)	((UART_##x) << 2)
+#define MICON_CMD_SIZE	4
+
+/* 4-byte magic hello command to UART1-attached microcontroller */
+static const unsigned char linkstation_micon_magic[] = {
+	0x1b,
+	0x00,
+	0x07,
+	0x00
+};
+
+/* for each row, first byte is the size of command */
+static const unsigned char linkstation_power_off_cmd[][MICON_CMD_SIZE] = {
+	{ 3,	0x01, 0x35, 0x00},
+	{ 2,	0x00, 0x0c},
+	{ 2,	0x00, 0x06},
+	{}
+};
+
+struct reset_cfg {
+	u32 baud;
+	const unsigned char *magic;
+	const unsigned char (*cmd)[MICON_CMD_SIZE];
+};
+
+struct device_cfg {
+	const struct device *dev;
+	void __iomem *base;
+	unsigned long tclk;
+	const struct reset_cfg *cfg;
+};
+
+static const struct reset_cfg linkstation_power_off_cfg = {
+	.baud = 38400,
+	.magic = linkstation_micon_magic,
+	.cmd = linkstation_power_off_cmd,
+};
+
+static const struct of_device_id linkstation_reset_of_match_table[] = {
+	{ .compatible = "linkstation,power-off",
+	  .data = &linkstation_power_off_cfg,
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, linkstation_reset_of_match_table);
+
+static int uart1_micon_read(const struct device_cfg *dev, unsigned char *buf, int count)
+{
+	int i;
+	int timeout;
+
+	for (i = 0; i < count; i++) {
+		timeout = 10;
+
+		while (!(readl(dev->base + UART1_REG(LSR)) & UART_LSR_DR)) {
+			if (--timeout == 0)
+				break;
+			udelay(1000);
+		}
+
+		if (timeout == 0)
+			break;
+		buf[i] = readl(dev->base + UART1_REG(RX));
+	}
+
+	/* return read bytes */
+	return i;
+}
+
+static int uart1_micon_write(const struct device_cfg *dev, const unsigned char *buf, int count)
+{
+	int i = 0;
+
+	while (count--) {
+		while (!(readl(dev->base + UART1_REG(LSR)) & UART_LSR_THRE))
+			barrier();
+		writel(buf[i++], dev->base + UART1_REG(TX));
+	}
+
+	return 0;
+}
+
+int uart1_micon_send(const struct device_cfg *dev, const unsigned char *data, int count)
+{
+	int i;
+	unsigned char checksum = 0;
+	unsigned char recv_buf[40];
+	unsigned char send_buf[40];
+	unsigned char correct_ack[3];
+	int retry = 2;
+
+	/* Generate checksum */
+	for (i = 0; i < count; i++)
+		checksum -=  data[i];
+
+	do {
+		/* Send data */
+		uart1_micon_write(dev, data, count);
+
+		/* send checksum */
+		uart1_micon_write(dev, &checksum, 1);
+
+		if (uart1_micon_read(dev, recv_buf, sizeof(recv_buf)) <= 3) {
+			dev_err(dev->dev, ">%s: receive failed.\n", __func__);
+
+			/* send preamble to clear the receive buffer */
+			memset(&send_buf, 0xff, sizeof(send_buf));
+			uart1_micon_write(dev, send_buf, sizeof(send_buf));
+
+			/* make dummy reads */
+			mdelay(100);
+			uart1_micon_read(dev, recv_buf, sizeof(recv_buf));
+		} else {
+			/* Generate expected ack */
+			correct_ack[0] = 0x01;
+			correct_ack[1] = data[1];
+			correct_ack[2] = 0x00;
+
+			/* checksum Check */
+			if ((recv_buf[0] + recv_buf[1] + recv_buf[2] +
+			     recv_buf[3]) & 0xFF) {
+				dev_err(dev->dev, ">%s: Checksum Error : "
+					"Received data[%02x, %02x, %02x, %02x]"
+					"\n", __func__, recv_buf[0],
+					recv_buf[1], recv_buf[2], recv_buf[3]);
+			} else {
+				/* Check Received Data */
+				if (correct_ack[0] == recv_buf[0] &&
+				    correct_ack[1] == recv_buf[1] &&
+				    correct_ack[2] == recv_buf[2]) {
+					/* Interval for next command */
+					mdelay(10);
+
+					/* Receive ACK */
+					return 0;
+				}
+			}
+			/* Received NAK or illegal Data */
+			dev_err(dev->dev, ">%s: Error : NAK or Illegal Data "
+					"Received\n", __func__);
+		}
+	} while (retry--);
+
+	/* Interval for next command */
+	mdelay(10);
+
+	return -1;
+}
+
+static struct device_cfg reset;
+
+static void linkstation_reset(void)
+{
+	const unsigned divisor = ((reset.tclk + (8 * reset.cfg->baud)) / (16 * reset.cfg->baud));
+	int i;
+
+	pr_err("%s: triggering power-off...\n", __func__);
+
+	/* hijack UART1 and reset into sane state */
+	writel(0x83, reset.base + UART1_REG(LCR));
+	writel(divisor & 0xff, reset.base + UART1_REG(DLL));
+	writel((divisor >> 8) & 0xff, reset.base + UART1_REG(DLM));
+	writel(reset.cfg->magic[0], reset.base + UART1_REG(LCR));
+	writel(reset.cfg->magic[1], reset.base + UART1_REG(IER));
+	writel(reset.cfg->magic[2], reset.base + UART1_REG(FCR));
+	writel(reset.cfg->magic[3], reset.base + UART1_REG(MCR));
+
+	/* send the power-off command to PIC */
+	for(i = 0; reset.cfg->cmd[i][0] > 0; i ++) {
+		/* [0] is size of the command; command starts from [1] */
+		uart1_micon_send(&reset, &(reset.cfg->cmd[i][1]), reset.cfg->cmd[i][0]);
+	}
+}
+
+static int linkstation_reset_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct resource *res;
+	struct clk *clk;
+
+	const struct of_device_id *match =
+		of_match_node(linkstation_reset_of_match_table, np);
+	reset.cfg = match->data;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "Missing resource");
+		return -EINVAL;
+	}
+
+	reset.dev = &pdev->dev;
+	reset.base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!reset.base) {
+		dev_err(reset.dev, "Unable to map resource");
+		return -EINVAL;
+	}
+
+	/* We need to know tclk in order to calculate the UART divisor */
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk)) {
+		dev_err(reset.dev, "Clk missing");
+		return PTR_ERR(clk);
+	}
+
+	reset.tclk = clk_get_rate(clk);
+
+	/* Check that nothing else has already setup a handler */
+	if (!pm_power_off) {
+		pm_power_off = linkstation_reset;
+	}
+
+	return 0;
+}
+
+static int linkstation_reset_remove(struct platform_device *pdev)
+{
+	if (pm_power_off == linkstation_reset)
+		pm_power_off = NULL;
+	return 0;
+}
+
+static struct platform_driver linkstation_reset_driver = {
+	.probe	= linkstation_reset_probe,
+	.remove	= linkstation_reset_remove,
+	.driver	= {
+		.name	= "linkstation_reset",
+		.of_match_table = of_match_ptr(linkstation_reset_of_match_table),
+	},
+};
+
+module_platform_driver(linkstation_reset_driver);
+
+MODULE_AUTHOR("Roger Shimizu <rogershimizu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("Linkstation Reset driver");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] power: reset: add linkstation-reset driver

[Ryan Tandy]

Hi Roger,

On Sun, Jan 08, 2017 at 12:04:50AM +0900, Roger Shimizu wrote:
>+config POWER_RESET_LINKSTATION
>+	bool "Buffalo Linkstation and its variants reset driver"
>+	depends on OF_GPIO && PLAT_ORION
>+	help
>+	  This driver supports power off Buffalo Linkstation / KuroBox Pro
>+	  NAS and their variants by sending commands to the micro-controller
>+	  which controls the main power.
>+
>+	  Say Y if you have a Buffalo Linkstation / KuroBox Pro NAS.
>+

Would it make sense to mention something about these being the 
ARM9/orion5x Linkstations? If I understand correctly, the older PPC 
Linkstations have a single-byte poweroff command. (Maybe they could even 
be supported by qnap-poweroff.)

See arch/powerpc/platforms/embedded6xx/linkstation.c around line 123, 
and arch/powerpc/platforms/embedded6xx/ls_uart.c.

Re: [PATCH v4 1/2] power: reset: add linkstation-reset driver

[Roger Shimizu]

Dear Ryan,

Thanks for your comments!

On Mon, Jan 9, 2017 at 2:02 AM, Ryan Tandy <ryan@nardis.ca> wrote:
> On Sun, Jan 08, 2017 at 12:04:50AM +0900, Roger Shimizu wrote:
>>
>> +config POWER_RESET_LINKSTATION
>> +       bool "Buffalo Linkstation and its variants reset driver"
>> +       depends on OF_GPIO && PLAT_ORION
>> +       help
>> +         This driver supports power off Buffalo Linkstation / KuroBox Pro
>> +         NAS and their variants by sending commands to the
>> micro-controller
>> +         which controls the main power.
>
> Would it make sense to mention something about these being the ARM9/orion5x
> Linkstations? If I understand correctly, the older PPC Linkstations have a
> single-byte poweroff command. (Maybe they could even be supported by
> qnap-poweroff.)
>
> See arch/powerpc/platforms/embedded6xx/linkstation.c around line 123, and
> arch/powerpc/platforms/embedded6xx/ls_uart.c.

This driver, linkstation-reset, can also handle PPC Linkstation after
it's converted to DT.
I already considered this and mentioned in previous reply [0].

[0] http://marc.info/?l=linux-pm&m=148216908031283

Cheers,
-- 
Roger Shimizu, GMT +9 Tokyo
PGP/GPG: 4096R/6C6ACD6417B3ACB1

Re: [PATCH v4 1/2] power: reset: add linkstation-reset driver

[Ryan Tandy]

On Mon, Jan 09, 2017 at 12:31:44PM +0900, Roger Shimizu wrote:
>This driver, linkstation-reset, can also handle PPC Linkstation after
>it's converted to DT.
>I already considered this and mentioned in previous reply [0].

OK. I was thinking of Sebastian's earlier comment:

On Wed, Dec 21, 2016 at 04:59:29PM +0100, Sebastian Reichel wrote:
>These models can just be added to qnap-poweroff, which handles
>exactly this special case as far as I can see.

and forgot that you planned to handle it in this driver. Sorry for the 
noise.

Re: [PATCH v4 1/2] power: reset: add linkstation-reset driver

[Roger Shimizu]

Dear Sebastian,

On Sun, Jan 8, 2017 at 12:04 AM, Roger Shimizu <rogershimizu@gmail.com> wrote:
> Buffalo Linkstation / KuroBox and their variants need magic command
> sending to UART1 to power-off.
>
> Power driver linkstation-reset implements the magic command and I/O
> routine, which come from files listed below:
>   - arch/arm/mach-orion5x/kurobox_pro-setup.c
>   - arch/arm/mach-orion5x/terastation_pro2-setup.c

I think there's not much concern regarding to this series.
Could you kindly help to apply this patch?
Thank you!

Cheers,
-- 
Roger Shimizu, GMT +9 Tokyo
PGP/GPG: 4096R/6C6ACD6417B3ACB1

Re: [PATCH v4 1/2] power: reset: add linkstation-reset driver

[Sebastian Reichel]

[-- Attachment #1.1: Type: text/plain, Size: 1225 bytes --]

Hi Roger,

On Wed, Jan 18, 2017 at 09:08:13PM +0900, Roger Shimizu wrote:
> On Sun, Jan 8, 2017 at 12:04 AM, Roger Shimizu <rogershimizu@gmail.com> wrote:
> > Buffalo Linkstation / KuroBox and their variants need magic command
> > sending to UART1 to power-off.
> >
> > Power driver linkstation-reset implements the magic command and I/O
> > routine, which come from files listed below:
> >   - arch/arm/mach-orion5x/kurobox_pro-setup.c
> >   - arch/arm/mach-orion5x/terastation_pro2-setup.c
> 
> I think there's not much concern regarding to this series.
> Could you kindly help to apply this patch?

Well you dropped the DT binding, but still introduce new DT
properties. Since they are not documented without the binding
I won't merge this. DT binding documentation is not optional.
In other words: NAK on the binding effectively means NAK on
the driver in its current state.

If you want to see this merged rebase it to Rob's generic
serial bindings [0] and help to support him getting everything
into mainline ASAP. Currently the feedback seems to be quite
positive, so I hope to see it merged for 4.11.

[0] https://lwn.net/Articles/711794/ (current proposal from 2 days ago)

-- Sebastian

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/2] power: reset: add linkstation-reset driver

[Gregory CLEMENT]

Hi Roger,
 
 On jeu., janv. 19 2017, Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> Hi Roger,
>
> On Wed, Jan 18, 2017 at 09:08:13PM +0900, Roger Shimizu wrote:
>> On Sun, Jan 8, 2017 at 12:04 AM, Roger Shimizu <rogershimizu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > Buffalo Linkstation / KuroBox and their variants need magic command
>> > sending to UART1 to power-off.
>> >
>> > Power driver linkstation-reset implements the magic command and I/O
>> > routine, which come from files listed below:
>> >   - arch/arm/mach-orion5x/kurobox_pro-setup.c
>> >   - arch/arm/mach-orion5x/terastation_pro2-setup.c
>> 
>> I think there's not much concern regarding to this series.
>> Could you kindly help to apply this patch?
>
> Well you dropped the DT binding, but still introduce new DT
> properties. Since they are not documented without the binding
> I won't merge this. DT binding documentation is not optional.
> In other words: NAK on the binding effectively means NAK on
> the driver in its current state.
>
> If you want to see this merged rebase it to Rob's generic
> serial bindings [0] and help to support him getting everything
> into mainline ASAP. Currently the feedback seems to be quite
> positive, so I hope to see it merged for 4.11.
>
> [0] https://lwn.net/Articles/711794/ (current proposal from 2 days
> ago)

What is te status for your series?

Do you plan sending a new version soon?

Gregory


>
> -- Sebastian
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] power: reset: add linkstation-reset driver

[Roger Shimizu]

Dear Gregory,

Thanks for contacting!

On Fri, Jan 27, 2017 at 12:28 AM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
>
> What is te status for your series?
>
> Do you plan sending a new version soon?

I'm still trying/working on it, but seems won't finish it very soon.

Cheers,
-- 
Roger Shimizu, GMT +9 Tokyo
PGP/GPG: 4096R/6C6ACD6417B3ACB1

Re: [PATCH v4 1/2] power: reset: add linkstation-reset driver

[Gregory CLEMENT]

Hi Roger,
 
 On jeu., janv. 26 2017, Roger Shimizu <rogershimizu@gmail.com> wrote:

> Dear Gregory,
>
> Thanks for contacting!
>
> On Fri, Jan 27, 2017 at 12:28 AM, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>>
>> What is te status for your series?
>>
>> Do you plan sending a new version soon?
>
> I'm still trying/working on it, but seems won't finish it very soon.

OK it is not a problem for me, but just keep in mind that the merge
window for arm-soc will be close soon.  That means that the dt part
could be delayed to 4.12.

Gregory


>
> Cheers,
> -- 
> Roger Shimizu, GMT +9 Tokyo
> PGP/GPG: 4096R/6C6ACD6417B3ACB1

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v4 2/2] ARM: DT: add power-off support to linkstation lsgl and kuroboxpro

[Roger Shimizu]

A few models of Linkstation / KuroBox has micro-controller which
controls the power (as well as FAN, but not related here), while
others not. So remove the restart-poweroff driver in linkstation
common dtsi file, and specify the proper power driver in dts
of each device.

Devices need micro-controler to power-off:
  - Linkstation LS-GL
  - KuroBox Pro
Device continues using original restart-poweroff driver:
  - Linkstation LS-WTGL

To: Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org>
To: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
To: Gregory Clement <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
To: Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Ryan Tandy <ryan-pRYBVO4bdZ33fQ9qLvQP4Q@public.gmane.org>
Cc: linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org

Reported-by: Ryan Tandy <ryan-pRYBVO4bdZ33fQ9qLvQP4Q@public.gmane.org>
Tested-by: Ryan Tandy <ryan-pRYBVO4bdZ33fQ9qLvQP4Q@public.gmane.org>
Signed-off-by: Roger Shimizu <rogershimizu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/orion5x-kuroboxpro.dts         |  8 ++++++++
 arch/arm/boot/dts/orion5x-linkstation-lsgl.dts   | 10 ++++++++++
 arch/arm/boot/dts/orion5x-linkstation-lswtgl.dts |  4 ++++
 arch/arm/boot/dts/orion5x-linkstation.dtsi       |  4 ----
 4 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/orion5x-kuroboxpro.dts b/arch/arm/boot/dts/orion5x-kuroboxpro.dts
index 1a672b098d0b..aba38d802bda 100644
--- a/arch/arm/boot/dts/orion5x-kuroboxpro.dts
+++ b/arch/arm/boot/dts/orion5x-kuroboxpro.dts
@@ -60,6 +60,14 @@
 				 <MBUS_ID(0x09, 0x00) 0 0xf2200000 0x800>,
 				 <MBUS_ID(0x01, 0x0f) 0 0xf4000000 0x40000>,
 				 <MBUS_ID(0x01, 0x1e) 0 0xfc000000 0x1000000>;
+
+		internal-regs {
+			power_off {
+				compatible = "linkstation,power-off";
+				reg = <0x12100 0x100>;
+				clocks = <&core_clk 0>;
+			};
+		};
 	};
 
 	memory { /* 128 MB */
diff --git a/arch/arm/boot/dts/orion5x-linkstation-lsgl.dts b/arch/arm/boot/dts/orion5x-linkstation-lsgl.dts
index 51dc734cd5b9..370fc17a6dd9 100644
--- a/arch/arm/boot/dts/orion5x-linkstation-lsgl.dts
+++ b/arch/arm/boot/dts/orion5x-linkstation-lsgl.dts
@@ -56,6 +56,16 @@
 	model = "Buffalo Linkstation Pro/Live";
 	compatible = "buffalo,lsgl", "marvell,orion5x-88f5182", "marvell,orion5x";
 
+	soc {
+		internal-regs {
+			power_off {
+				compatible = "linkstation,power-off";
+				reg = <0x12100 0x100>;
+				clocks = <&core_clk 0>;
+			};
+		};
+	};
+
 	memory { /* 128 MB */
 		device_type = "memory";
 		reg = <0x00000000 0x8000000>;
diff --git a/arch/arm/boot/dts/orion5x-linkstation-lswtgl.dts b/arch/arm/boot/dts/orion5x-linkstation-lswtgl.dts
index 0eead400f427..571a71f5b7ad 100644
--- a/arch/arm/boot/dts/orion5x-linkstation-lswtgl.dts
+++ b/arch/arm/boot/dts/orion5x-linkstation-lswtgl.dts
@@ -59,6 +59,10 @@
 		reg = <0x00000000 0x4000000>;
 	};
 
+	restart_poweroff {
+		compatible = "restart-poweroff";
+	};
+
 	gpio_keys {
 		power-on-switch {
 			gpios = <&gpio0 8 GPIO_ACTIVE_LOW>;
diff --git a/arch/arm/boot/dts/orion5x-linkstation.dtsi b/arch/arm/boot/dts/orion5x-linkstation.dtsi
index ed456ab35fd8..40770a8d4b36 100644
--- a/arch/arm/boot/dts/orion5x-linkstation.dtsi
+++ b/arch/arm/boot/dts/orion5x-linkstation.dtsi
@@ -57,10 +57,6 @@
 				 <MBUS_ID(0x01, 0x0f) 0 0xf4000000 0x40000>;
 	};
 
-	restart_poweroff {
-		compatible = "restart-poweroff";
-	};
-
 	regulators {
 		compatible = "simple-bus";
 		#address-cells = <1>;
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html