[PATCH V2 0/6] Enable X-Gene platform reboot driver.

[PATCH V2 0/6] Enable X-Gene platform reboot driver.

[Feng Kan]

Enable X-Gene platform driver for the X-Gene platform. Remove the
use of the mask attribute from the reboot dts node. Add support
for using the ACPI and DTS resource for the reboot driver.

V2 Change:
	- Add support for using ACPI resource.

Feng Kan (6):
  Documentation: add dts binding for X-Gene reboot dts node.
  arm64: add missing dts entry for X-Gene platform.
  arm64: Enable system reset via arch Kconfig for X-Gene system.
  power: reset: removed use of the dts mask and use fixed value for
    reset.
  power: reset: change xgene reboot driver to use both acpi and dts    
    resource for reboot.
  acpi: platform: add ACPI code for X-Gene reboot platform driver.

 .../devicetree/bindings/arm64/xgene/reboot.txt     |   10 +++++
 arch/arm64/Kconfig                                 |    1 +
 arch/arm64/boot/dts/apm-storm.dtsi                 |    5 +++
 drivers/acpi/acpi_platform.c                       |    3 ++
 drivers/power/reset/xgene-reboot.c                 |   38 +++++++++++++++----
 5 files changed, 49 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm64/xgene/reboot.txt

-- 
1.7.6.1

--
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 V2 1/6] Documentation: add dts binding for X-Gene reboot dts node.

[Feng Kan]

Add X-Gene reboot device tree node documentation.

Signed-off-by: Feng Kan <fkan@apm.com>
---
 .../devicetree/bindings/arm64/xgene/reboot.txt     |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm64/xgene/reboot.txt

diff --git a/Documentation/devicetree/bindings/arm64/xgene/reboot.txt b/Documentation/devicetree/bindings/arm64/xgene/reboot.txt
new file mode 100644
index 0000000..51d23c1
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm64/xgene/reboot.txt
@@ -0,0 +1,10 @@
+Reboot property to control system reboot on storm platforms:
+
+The register attribute contain the address of the reset register.
+
+Example:
+
+	reboot@0 {
+		compatible = "apm,xgene-reboot";
+		reg = <0x0 0x17000014 0x0 0x4>;
+	};
-- 
1.7.6.1

Re: [PATCH V2 1/6] Documentation: add dts binding for X-Gene reboot dts node.

[Mark Rutland]

On Tue, Jan 07, 2014 at 10:50:36PM +0000, Feng Kan wrote:
> Add X-Gene reboot device tree node documentation.
> 
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
>  .../devicetree/bindings/arm64/xgene/reboot.txt     |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm64/xgene/reboot.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm64/xgene/reboot.txt b/Documentation/devicetree/bindings/arm64/xgene/reboot.txt
> new file mode 100644
> index 0000000..51d23c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm64/xgene/reboot.txt
> @@ -0,0 +1,10 @@
> +Reboot property to control system reboot on storm platforms:
> +
> +The register attribute contain the address of the reset register.

Required properties:

- compatible: should contain "apm,xgene-reboot"

- reg: the base address and size of the reset register.

> +
> +Example:
> +
> +	reboot@0 {
> +		compatible = "apm,xgene-reboot";
> +		reg = <0x0 0x17000014 0x0 0x4>;
> +	};

Given this seems to be a 64-bit address, the unit address should
preferably be 0,17000014 rather than just 0.

Thanks,
Mark.

Re: [PATCH V2 1/6] Documentation: add dts binding for X-Gene reboot dts node.

[Arnd Bergmann]

On Wednesday 08 January 2014 10:05:50 Mark Rutland wrote:
> > +
> > +Example:
> > +
> > +     reboot@0 {
> > +             compatible = "apm,xgene-reboot";
> > +             reg = <0x0 0x17000014 0x0 0x4>;
> > +     };
> 
> Given this seems to be a 64-bit address, the unit address should
> preferably be 0,17000014 rather than just 0.

I also wonder about the size and location of the register.
Are you sure the device is just a single 4-byte register at offset
0x14, rather than a complex device that happens to contain the
reset register?

	Arnd

Re: [PATCH V2 1/6] Documentation: add dts binding for X-Gene reboot dts node.

[Feng Kan]

On Wed, Jan 8, 2014 at 2:08 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 08 January 2014 10:05:50 Mark Rutland wrote:
>> > +
>> > +Example:
>> > +
>> > +     reboot@0 {
>> > +             compatible = "apm,xgene-reboot";
>> > +             reg = <0x0 0x17000014 0x0 0x4>;
>> > +     };
>>
>> Given this seems to be a 64-bit address, the unit address should
>> preferably be 0,17000014 rather than just 0.
>
> I also wonder about the size and location of the register.
> Are you sure the device is just a single 4-byte register at offset
> 0x14, rather than a complex device that happens to contain the
> reset register?

FKAN: this is a single 4 byte register when clicked perform the system
reset of the platform.

>
>         Arnd

Re: [PATCH V2 1/6] Documentation: add dts binding for X-Gene reboot dts node.

[Arnd Bergmann]

On Thursday 09 January 2014, Feng Kan wrote:
> On Wed, Jan 8, 2014 at 2:08 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday 08 January 2014 10:05:50 Mark Rutland wrote:
> >> > +
> >> > +Example:
> >> > +
> >> > +     reboot@0 {
> >> > +             compatible = "apm,xgene-reboot";
> >> > +             reg = <0x0 0x17000014 0x0 0x4>;
> >> > +     };
> >>
> >> Given this seems to be a 64-bit address, the unit address should
> >> preferably be 0,17000014 rather than just 0.
> >
> > I also wonder about the size and location of the register.
> > Are you sure the device is just a single 4-byte register at offset
> > 0x14, rather than a complex device that happens to contain the
> > reset register?
> 
> FKAN: this is a single 4 byte register when clicked perform the system
> reset of the platform.

That is not what I was asking about.

The problem with your binding is that it doesn't seem to describe
the hardware structure at all, but rather try to invent devices
because of how it's convenient for how you write the Linux drivers.

This is never a good idea and it will become a problem once you
try to port a different operating system, or when the structure
of the Linux drivers changes. Remember that in Linux we can freely
reorganize our code as requirements change, but the bindings have
to remain stable to allow running new kernels with an older device
tree that may be shipped with the firmware.

(Side note: if you end up doing an ACPI driver for this after all,
the problem is even worse because it's basically impossible to update
the ACPI tables in field, while with DT you can at least override
them in the firmware if something goes really wrong.)

On most SoCs I've seen, the system reset registers are part of
some on-chip device, most likely one of
- A SoC-specific system control register set containing various
  global registers that may be of interest to other parts of the
  kernel.
- A watchdog timer device that will reset the system after a crash,
  or when explicit requested by the OS
- A dedicated reset controller that has registers for individually
  resetting a number of subsystems of the SoC, or all at once in
  case of system reboot.
- an interface to an external or integrated power management IC
  that controls voltages and/or clock frequencies provided to
  the SoC and other parts of the system.

In each of those cases, we already have a subsystem that is supposed
to handle the entire register set and provide a system reset method
as one of the things it does. Given that your register is at offset
0x14 from the start of a larger MMIO range, I strongly suspect
that the same is true here.

	Arnd

Re: [PATCH V2 1/6] Documentation: add dts binding for X-Gene reboot dts node.

[Feng Kan]

> That is not what I was asking about.
>
> The problem with your binding is that it doesn't seem to describe
> the hardware structure at all, but rather try to invent devices
> because of how it's convenient for how you write the Linux drivers.
>
> This is never a good idea and it will become a problem once you
> try to port a different operating system, or when the structure
> of the Linux drivers changes. Remember that in Linux we can freely
> reorganize our code as requirements change, but the bindings have
> to remain stable to allow running new kernels with an older device
> tree that may be shipped with the firmware.
>
> (Side note: if you end up doing an ACPI driver for this after all,
> the problem is even worse because it's basically impossible to update
> the ACPI tables in field, while with DT you can at least override
> them in the firmware if something goes really wrong.)
>
> On most SoCs I've seen, the system reset registers are part of
> some on-chip device, most likely one of
> - A SoC-specific system control register set containing various
>   global registers that may be of interest to other parts of the
>   kernel.
FKAN: our system block in your reference would be the system clock unit (SCU).
It handles number of things (JTAG, clock control, power control not
touched by OS).
The register that is relevant to the system reset is this lone
register. I reduce the
range of the dts aperture due to this fact.


> - A watchdog timer device that will reset the system after a crash,
>   or when explicit requested by the OS
> - A dedicated reset controller that has registers for individually
>   resetting a number of subsystems of the SoC, or all at once in
>   case of system reboot.
> - an interface to an external or integrated power management IC
>   that controls voltages and/or clock frequencies provided to
>   the SoC and other parts of the system.
>
> In each of those cases, we already have a subsystem that is supposed
> to handle the entire register set and provide a system reset method
> as one of the things it does. Given that your register is at offset
> 0x14 from the start of a larger MMIO range, I strongly suspect
> that the same is true here.
FKAN: I could remove this dts node and create another dts node that
describe the range of registers on the SCU and use that node in this driver.
I am not sure which subsystem I can use to handle this case, I do see a reset
subsystem in the kernel but more used for ip resets. Please kindly let
me know. Thanks for the great feedback.
>
>         Arnd

Re: [PATCH V2 1/6] Documentation: add dts binding for X-Gene reboot dts node.

[Arnd Bergmann]

On Monday 13 January 2014, Feng Kan wrote:
> FKAN: I could remove this dts node and create another dts node that
> describe the range of registers on the SCU and use that node in this driver.
> I am not sure which subsystem I can use to handle this case, I do see a reset
> subsystem in the kernel but more used for ip resets. Please kindly let
> me know. Thanks for the great feedback.

Is this related to the standard ARM SCU that manages multiprocessor
systems, or a different unit that uses the same name?

Since this is a global register range with a variety of things in it,
the best candidate IMHO would be to use the "syscon" driver. You can
mark the device node as 'compatible="apm,xgene-scu","syscon"' to let
it get picked up by the drivers/mfd/syscon driver, which creates
a "regmap" structure for it.

The reset driver then uses a DT reference with a phandle to the
SCU node and calls syscon_regmap_lookup_by_phandle() to get the
regmap. It can get the register number inside the regmap from DT
as well and use the regmap API to perform the reset.

	Arnd

Re: [PATCH V2 1/6] Documentation: add dts binding for X-Gene reboot dts node.

[Feng Kan]

>
> Is this related to the standard ARM SCU that manages multiprocessor
> systems, or a different unit that uses the same name?

FKAN: You mean the snoop control unit in ARM. This is different from
that, the main function of this unit is clk control.

>
> Since this is a global register range with a variety of things in it,
> the best candidate IMHO would be to use the "syscon" driver. You can
> mark the device node as 'compatible="apm,xgene-scu","syscon"' to let
> it get picked up by the drivers/mfd/syscon driver, which creates
> a "regmap" structure for it.
>
> The reset driver then uses a DT reference with a phandle to the
> SCU node and calls syscon_regmap_lookup_by_phandle() to get the
> regmap. It can get the register number inside the regmap from DT
> as well and use the regmap API to perform the reset.

Thanks, will do.
>
>         Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 1/6] Documentation: add dts binding for X-Gene reboot dts node.

[Arnd Bergmann]

On Tuesday 14 January 2014, Feng Kan wrote:
> 
> >
> > Is this related to the standard ARM SCU that manages multiprocessor
> > systems, or a different unit that uses the same name?
> 
> FKAN: You mean the snoop control unit in ARM. This is different from
> that, the main function of this unit is clk control.

Hmm, if it's *mainly* used for clocks, you may actually just want to
have a driver in drivers/clk for it that also performs a few of the
other things on the side.

	Arnd

Re: [PATCH V2 1/6] Documentation: add dts binding for X-Gene reboot dts node.

[Feng Kan]

On Mon, Jan 13, 2014 at 10:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 14 January 2014, Feng Kan wrote:
>>
>> >
>> > Is this related to the standard ARM SCU that manages multiprocessor
>> > systems, or a different unit that uses the same name?
>>
>> FKAN: You mean the snoop control unit in ARM. This is different from
>> that, the main function of this unit is clk control.
>
> Hmm, if it's *mainly* used for clocks, you may actually just want to
> have a driver in drivers/clk for it that also performs a few of the
> other things on the side.
>

FKAN: there among other thing, reset controls, fuse id's and deep sleep
resume registers... I like your idea of using syscon, we can have a node
for SCU and then the specific driver to parse out the needed chunk to use.

>         Arnd

Re: [PATCH V2 1/6] Documentation: add dts binding for X-Gene reboot dts node.

[Arnd Bergmann]

On Tuesday 14 January 2014 10:35:33 Feng Kan wrote:
> On Mon, Jan 13, 2014 at 10:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 14 January 2014, Feng Kan wrote:
> >>
> >> >
> >> > Is this related to the standard ARM SCU that manages multiprocessor
> >> > systems, or a different unit that uses the same name?
> >>
> >> FKAN: You mean the snoop control unit in ARM. This is different from
> >> that, the main function of this unit is clk control.
> >
> > Hmm, if it's *mainly* used for clocks, you may actually just want to
> > have a driver in drivers/clk for it that also performs a few of the
> > other things on the side.
> >
> 
> FKAN: there among other thing, reset controls, fuse id's and deep sleep
> resume registers... I like your idea of using syscon, we can have a node
> for SCU and then the specific driver to parse out the needed chunk to use.

Yes, sounds good.

	Arnd

Re: [PATCH V2 1/6] Documentation: add dts binding for X-Gene reboot dts node.

[Feng Kan]

On Wed, Jan 8, 2014 at 2:05 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Jan 07, 2014 at 10:50:36PM +0000, Feng Kan wrote:
>> Add X-Gene reboot device tree node documentation.
>>
>> Signed-off-by: Feng Kan <fkan@apm.com>
>> ---
>>  .../devicetree/bindings/arm64/xgene/reboot.txt     |   10 ++++++++++
>>  1 files changed, 10 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/arm64/xgene/reboot.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm64/xgene/reboot.txt b/Documentation/devicetree/bindings/arm64/xgene/reboot.txt
>> new file mode 100644
>> index 0000000..51d23c1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm64/xgene/reboot.txt
>> @@ -0,0 +1,10 @@
>> +Reboot property to control system reboot on storm platforms:
>> +
>> +The register attribute contain the address of the reset register.
>
> Required properties:
>
> - compatible: should contain "apm,xgene-reboot"
>
> - reg: the base address and size of the reset register.
>
>> +
>> +Example:
>> +
>> +     reboot@0 {
>> +             compatible = "apm,xgene-reboot";
>> +             reg = <0x0 0x17000014 0x0 0x4>;
>> +     };
>
> Given this seems to be a 64-bit address, the unit address should
> preferably be 0,17000014 rather than just 0.

FKAN: Mark, can I keep this as it is to be consistent with the reset
of the file?
>
> Thanks,
> Mark.

[PATCH V2 2/6] arm64: add missing dts entry for X-Gene platform.

[Feng Kan]

Add missing dts entry for the X-Gene reboot driver.

Signed-off-by: Feng Kan <fkan@apm.com>
---
 arch/arm64/boot/dts/apm-storm.dtsi |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
index d37d736..78d29781 100644
--- a/arch/arm64/boot/dts/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm-storm.dtsi
@@ -187,5 +187,10 @@
 			interrupt-parent = <&gic>;
 			interrupts = <0x0 0x4c 0x4>;
 		};
+
+		reboot@17000014 {
+			compatible = "apm,xgene-reboot";
+			reg = <0x0 0x17000014 0x0 0x4>;
+		};
 	};
 };
-- 
1.7.6.1

[PATCH V2 3/6] arm64: Enable system reset via arch Kconfig for X-Gene system.

[Feng Kan]

Select X-Gene reboot driver for X-Gene platform.

Signed-off-by: Feng Kan <fkan@apm.com>
---
 arch/arm64/Kconfig |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 249acb9..3d0c81e 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -122,6 +122,7 @@ config ARCH_VEXPRESS
 
 config ARCH_XGENE
 	bool "AppliedMicro X-Gene SOC Family"
+	select POWER_RESET_XGENE
 	help
 	  This enables support for AppliedMicro X-Gene SOC Family
 
-- 
1.7.6.1

[PATCH V2 4/6] power: reset: removed use of the dts mask and use fixed value for reset.

[Feng Kan]

Remove the use of the mask attribute in the X-Gene reboot driver and
use fixed value.

Signed-off-by: Feng Kan <fkan-qTEPVZfXA3Y@public.gmane.org>
---
 drivers/power/reset/xgene-reboot.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/power/reset/xgene-reboot.c b/drivers/power/reset/xgene-reboot.c
index ecd55f8..683238c 100644
--- a/drivers/power/reset/xgene-reboot.c
+++ b/drivers/power/reset/xgene-reboot.c
@@ -40,7 +40,7 @@ struct xgene_reboot_context {
 
 static struct xgene_reboot_context *xgene_restart_ctx;
 
-static void xgene_restart(char str, const char *cmd)
+static void xgene_restart(enum reboot_mode reboot_mode, const char *cmd)
 {
 	struct xgene_reboot_context *ctx = xgene_restart_ctx;
 	unsigned long timeout;
@@ -73,8 +73,7 @@ static int xgene_reboot_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	if (of_property_read_u32(pdev->dev.of_node, "mask", &ctx->mask))
-		ctx->mask = 0xFFFFFFFF;
+	ctx->mask = 0x1;
 
 	ctx->pdev = pdev;
 	arm_pm_restart = xgene_restart;
-- 
1.7.6.1

--
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 4/6] power: reset: removed use of the dts mask and use fixed value for reset.

[Arnd Bergmann]

On Tuesday 07 January 2014, Feng Kan wrote:
> @@ -73,8 +73,7 @@ static int xgene_reboot_probe(struct platform_device *pdev)
>                 return -ENODEV;
>         }
>  
> -       if (of_property_read_u32(pdev->dev.of_node, "mask", &ctx->mask))
> -               ctx->mask = 0xFFFFFFFF;
> +       ctx->mask = 0x1;

Is the "mask" property now deprecated? Please update the binding document
accordingly if it's no longer needed.

	Arnd

[PATCH V2 5/6] power: reset: change xgene reboot driver to use both acpi and dts resource for reboot.

[Feng Kan]

Enable the X-Gene reboot driver to use either the ACPI or the DTS
resource using the platform driver method.

Signed-off-by: Feng Kan <fkan@apm.com>
---
 drivers/power/reset/xgene-reboot.c |   33 ++++++++++++++++++++++++++++-----
 1 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/power/reset/xgene-reboot.c b/drivers/power/reset/xgene-reboot.c
index 683238c..e3223c2 100644
--- a/drivers/power/reset/xgene-reboot.c
+++ b/drivers/power/reset/xgene-reboot.c
@@ -30,6 +30,8 @@
 #include <linux/platform_device.h>
 #include <linux/stat.h>
 #include <linux/slab.h>
+#include <linux/acpi.h>
+#include <linux/efi.h>
 #include <asm/system_misc.h>
 
 struct xgene_reboot_context {
@@ -59,6 +61,14 @@ static void xgene_restart(enum reboot_mode reboot_mode, const char *cmd)
 static int xgene_reboot_probe(struct platform_device *pdev)
 {
 	struct xgene_reboot_context *ctx;
+	struct resource *res;
+	int rc;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "no resource address\n");
+		return -ENODEV;
+	}
 
 	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx) {
@@ -66,20 +76,27 @@ static int xgene_reboot_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	ctx->csr = of_iomap(pdev->dev.of_node, 0);
+	ctx->csr = devm_ioremap_resource(&pdev->dev, res);
 	if (!ctx->csr) {
-		devm_kfree(&pdev->dev, ctx);
-		dev_err(&pdev->dev, "can not map resource\n");
-		return -ENODEV;
+		dev_err(&pdev->dev, "can't map CSR resource\n");
+		rc  = -ENOMEM;
+		goto error;
 	}
 
 	ctx->mask = 0x1;
 
+	dev_info(&pdev->dev, "X-Gene register reboot driver\n");
+
 	ctx->pdev = pdev;
 	arm_pm_restart = xgene_restart;
 	xgene_restart_ctx = ctx;
-
 	return 0;
+
+error:
+	if (ctx->csr)
+		devm_iounmap(&pdev->dev, ctx->csr);
+	devm_kfree(&pdev->dev, ctx);
+	return rc;
 }
 
 static struct of_device_id xgene_reboot_of_match[] = {
@@ -87,11 +104,17 @@ static struct of_device_id xgene_reboot_of_match[] = {
 	{}
 };
 
+static const struct acpi_device_id xgene_reset_acpi_ids[] = {
+	{ "APMC0D08", 0 },
+	{}
+};
+
 static struct platform_driver xgene_reboot_driver = {
 	.probe = xgene_reboot_probe,
 	.driver = {
 		.name = "xgene-reboot",
 		.of_match_table = xgene_reboot_of_match,
+		.acpi_match_table = ACPI_PTR(xgene_reset_acpi_ids),
 	},
 };
 
-- 
1.7.6.1

Re: [PATCH V2 5/6] power: reset: change xgene reboot driver to use both acpi and dts resource for reboot.

[Arnd Bergmann]

On Tuesday 07 January 2014, Feng Kan wrote:
> Enable the X-Gene reboot driver to use either the ACPI or the DTS
> resource using the platform driver method.
> 
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
>  drivers/power/reset/xgene-reboot.c |   33 ++++++++++++++++++++++++++++-----
>  1 files changed, 28 insertions(+), 5 deletions(-)
> 

Shouldn't an ACPI based system always be able to reboot through the
UEFI reset_system call? I really don't think we want to support
ACPI on non-UEFI systems.

	Arnd

Re: [PATCH V2 5/6] power: reset: change xgene reboot driver to use both acpi and dts resource for reboot.

[Feng Kan]

On Wed, Jan 8, 2014 at 1:43 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 07 January 2014, Feng Kan wrote:
>> Enable the X-Gene reboot driver to use either the ACPI or the DTS
>> resource using the platform driver method.
>>
>> Signed-off-by: Feng Kan <fkan@apm.com>
>> ---
>>  drivers/power/reset/xgene-reboot.c |   33 ++++++++++++++++++++++++++++-----
>>  1 files changed, 28 insertions(+), 5 deletions(-)
>>
>
> Shouldn't an ACPI based system always be able to reboot through the
> UEFI reset_system call? I really don't think we want to support
> ACPI on non-UEFI systems.
>
>         Arnd
Thanks Arnd, I will retract the ACPI support for now. We are still
trying to figure
out how best to support ACPI. The larger problem seems to be the fact that
UEFI and ACPI were originally made for x86 and is ill suited for the
arm64. I will
see if I can find a better path for this patch.

Re: [PATCH V2 5/6] power: reset: change xgene reboot driver to use both acpi and dts resource for reboot.

[Arnd Bergmann]

On Thursday 09 January 2014, Feng Kan wrote:
> On Wed, Jan 8, 2014 at 1:43 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 07 January 2014, Feng Kan wrote:
> >> Enable the X-Gene reboot driver to use either the ACPI or the DTS
> >> resource using the platform driver method.
> >>
> >> Signed-off-by: Feng Kan <fkan@apm.com>
> >> ---
> >>  drivers/power/reset/xgene-reboot.c |   33 ++++++++++++++++++++++++++++-----
> >>  1 files changed, 28 insertions(+), 5 deletions(-)
> >>
> >
> > Shouldn't an ACPI based system always be able to reboot through the
> > UEFI reset_system call? I really don't think we want to support
> > ACPI on non-UEFI systems.
> >
> >         Arnd
> Thanks Arnd, I will retract the ACPI support for now. We are still
> trying to figure out how best to support ACPI.

Ok, that makes sense.

> The larger problem seems to be the fact that
> UEFI and ACPI were originally made for x86 and is ill suited for the
> arm64. I will see if I can find a better path for this patch.

I would strongly recommend to implement the standard UEFI reset
mechanism on arm64-linux and then ensure your firmware provides
the appropriate methods.

Note that any proper arm64 system that you want a standard Linux
distro to run on will likely need to implement UEFI anyway.

Regarding ACPI, I'm well aware of the problems you are facing, but
they have less to do with the intruction set (x86 vs arm64) and much
more with the lack of a standard hardware platform. There is work
underway to support a standardized subset of arm64 servers with ACPI,
but X-Gene is rather unlikely to fit into that subset because of how
it's just a collection of homegrown hardware blocks that are attached
in seemingly random ways without a clear structure and certainly
not following any standard.

This is not unusual for ARM SoCs, and we can have learned to
deal with it, but as we have no intention of supporting multiple
incompatible ACPI implementations in Linux, you should not spend
too much time thinking about ACPI on your platform.

	Arnd

[PATCH V2 6/6] acpi: platform: add ACPI code for X-Gene reboot platform driver.

[Feng Kan]

Add ACPI code for X-Gene reboot platform driver.

Signed-off-by: Feng Kan <fkan@apm.com>
---
 drivers/acpi/acpi_platform.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
index dbfe49e..9fe1d4f 100644
--- a/drivers/acpi/acpi_platform.c
+++ b/drivers/acpi/acpi_platform.c
@@ -37,6 +37,9 @@ static const struct acpi_device_id acpi_platform_device_ids[] = {
 	{ "INT33C8" },
 	{ "80860F28" },
 
+	/* APM X-Gene */
+	{ "APMC0D08" },
+
 	{ }
 };
 
-- 
1.7.6.1

Re: [PATCH V2 0/6] Enable X-Gene platform reboot driver.

[Matthew Garrett]

On Tue, Jan 07, 2014 at 02:50:35PM -0800, Feng Kan wrote:
> Enable X-Gene platform driver for the X-Gene platform. Remove the
> use of the mask attribute from the reboot dts node. Add support
> for using the ACPI and DTS resource for the reboot driver.

Why can't you just use the standard ACPI reboot register values in the 
FADT?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH V2 0/6] Enable X-Gene platform reboot driver.

[Feng Kan]

On Tue, Jan 14, 2014 at 8:07 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Tue, Jan 07, 2014 at 02:50:35PM -0800, Feng Kan wrote:
>> Enable X-Gene platform driver for the X-Gene platform. Remove the
>> use of the mask attribute from the reboot dts node. Add support
>> for using the ACPI and DTS resource for the reboot driver.
>
> Why can't you just use the standard ACPI reboot register values in the
> FADT?
>
FKAN: I am removing the acpi patch in the next round. Mainly, I don't see a
way for the arm64 kernel to use those values. EFI in x86 have some runtime
services that can be triggered to call reset_system, but that code is not
available yet in arm64.

> --
> Matthew Garrett | mjg59@srcf.ucam.org