All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] xen/arm64/ns16550: Support for Allwinner H5 SoC
@ 2017-09-19 13:38 Awais Masood
  2017-09-19 13:38 ` [PATCH 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i) Awais Masood
                   ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Awais Masood @ 2017-09-19 13:38 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
	Awais Masood, ian.jackson, tim, julien.grall, jbeulich

This patch series adds support for Allwinner H5 (ARM64 Cortex-A53)

Builds upon existing support for A20/A31(sun7i) with updates to support
different watchdog timer base addresses and usual dt compatibility.

The fix within ns16550 is essential to get dom0 booting on H5.

Tested On:

Hardware Platform: Orange Pi PC2

Dom0 Linux: https://github.com/Icenowy/linux/tree/sunxi64-4.13-rc6

u-boot: https://github.com/armbian/u-boot-sun50i

xen: master
    Built as:
        make dist-xen XEN_TARGET_ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu-

    For early printk existing switch for sun7i can be used
        CONFIG_EARLY_PRINTK=sun7i

Dom0 boot log: https://pastebin.com/CgUuqpi0

Awais Masood (2):
  xen/arm64: Add Support for Allwinner H5 (sun50i)
  xen/ns16550: Fix ISR lockup on Designware 8250 (H5)

 xen/arch/arm/platforms/Makefile |  1 +
 xen/arch/arm/platforms/sunxi.c  | 40 +++++++++++++++++++++++++++++++---------
 xen/drivers/char/ns16550.c      | 12 ++++++++++++
 3 files changed, 44 insertions(+), 9 deletions(-)

-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* [PATCH 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i)
  2017-09-19 13:38 [PATCH 0/2] xen/arm64/ns16550: Support for Allwinner H5 SoC Awais Masood
@ 2017-09-19 13:38 ` Awais Masood
  2017-09-19 13:50   ` Jan Beulich
  2017-09-19 13:38 ` [PATCH 2/2] xen/ns16550: Fix ISR lockup on Designware 8250 (H5) Awais Masood
  2017-09-26  9:37 ` [PATCH v2 0/2] xen/arm64/ns16550: Support for Allwinner H5 SoC Awais Masood
  2 siblings, 1 reply; 20+ messages in thread
From: Awais Masood @ 2017-09-19 13:38 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
	Awais Masood, ian.jackson, tim, julien.grall, jbeulich

Tested on Orange Pi PC2.

Makefile updated to enable ARM64 compilation of sunxi.c.

sunxi.c updates include:

Added H5 dt compatibility string.

Watchdog timer base address is different on sun5oi as compared to sun7i.
Reset function updated to handle different base addresses.

Signed-off-by: Awais Masood <awais.masood@vadion.com>
---
 xen/arch/arm/platforms/Makefile |  1 +
 xen/arch/arm/platforms/sunxi.c  | 40 +++++++++++++++++++++++++++++++---------
 2 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
index 49fa683..722897a 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -6,5 +6,6 @@ obj-$(CONFIG_ARM_32) += omap5.o
 obj-$(CONFIG_ARM_32) += rcar2.o
 obj-$(CONFIG_ARM_64) += seattle.o
 obj-$(CONFIG_ARM_32) += sunxi.o
+obj-$(CONFIG_ARM_64) += sunxi.o
 obj-$(CONFIG_ARM_64) += xgene-storm.o
 obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
diff --git a/xen/arch/arm/platforms/sunxi.c b/xen/arch/arm/platforms/sunxi.c
index 0ba7b3d..06d62e7 100644
--- a/xen/arch/arm/platforms/sunxi.c
+++ b/xen/arch/arm/platforms/sunxi.c
@@ -22,18 +22,18 @@
 #include <asm/io.h>
 
 /* Watchdog constants: */
-#define SUNXI_WDT_BASE            0x01c20c90
+#define SUNXI_WDT_A20_BASE        0x01c20c90
+#define SUNXI_WDT_H5_BASE         0x01c20cA0
 #define SUNXI_WDT_MODE            0x04
-#define SUNXI_WDT_MODEADDR        (SUNXI_WDT_BASE + SUNXI_WDT_MODE)
 #define SUNXI_WDT_MODE_EN         (1 << 0)
 #define SUNXI_WDT_MODE_RST_EN     (1 << 1)
 
 
-static void sunxi_reset(void)
+static void sunxi_reset(u32 base)
 {
     void __iomem *wdt;
 
-    wdt = ioremap_nocache(SUNXI_WDT_MODEADDR & PAGE_MASK, PAGE_SIZE);
+    wdt = ioremap_nocache((base + SUNXI_WDT_MODE) & PAGE_MASK, PAGE_SIZE);
     if ( !wdt )
     {
         dprintk(XENLOG_ERR, "Unable to map watchdog register!\n");
@@ -42,19 +42,35 @@ static void sunxi_reset(void)
 
     /* Enable watchdog to trigger a reset after 500 ms: */
     writel(SUNXI_WDT_MODE_EN | SUNXI_WDT_MODE_RST_EN,
-      wdt + (SUNXI_WDT_MODEADDR & ~PAGE_MASK));
+      wdt + ((base + SUNXI_WDT_MODE) & ~PAGE_MASK));
     iounmap(wdt);
 
     for (;;)
         wfi();
 }
 
-static const char * const sunxi_dt_compat[] __initconst =
+static void sunxi_a20_reset(void)
+{
+    sunxi_reset(SUNXI_WDT_A20_BASE);
+}
+
+static void sunxi_h5_reset(void)
+{
+    sunxi_reset(SUNXI_WDT_H5_BASE);
+}
+
+static const char * const sunxi_dt_allwinner_a20_compat[] __initconst =
 {
     "allwinner,sun7i-a20",
     NULL
 };
 
+static const char * const sunxi_dt_allwinner_h5_compat[] __initconst =
+{
+    "allwinner,sun50i-h5",
+    NULL
+};
+
 static const struct dt_device_match sunxi_blacklist_dev[] __initconst =
 {
     /*
@@ -65,10 +81,16 @@ static const struct dt_device_match sunxi_blacklist_dev[] __initconst =
     { /* sentinel */ },
 };
 
-PLATFORM_START(sunxi, "Allwinner A20")
-    .compatible = sunxi_dt_compat,
+PLATFORM_START(sunxia20, "Allwinner A20")
+    .compatible = sunxi_dt_allwinner_a20_compat,
+    .blacklist_dev = sunxi_blacklist_dev,
+    .reset = sunxi_a20_reset,
+PLATFORM_END
+
+PLATFORM_START(sunxih5, "Allwinner H5")
+    .compatible = sunxi_dt_allwinner_h5_compat,
     .blacklist_dev = sunxi_blacklist_dev,
-    .reset = sunxi_reset,
+    .reset = sunxi_h5_reset,
 PLATFORM_END
 
 /*
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* [PATCH 2/2] xen/ns16550: Fix ISR lockup on Designware 8250 (H5)
  2017-09-19 13:38 [PATCH 0/2] xen/arm64/ns16550: Support for Allwinner H5 SoC Awais Masood
  2017-09-19 13:38 ` [PATCH 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i) Awais Masood
@ 2017-09-19 13:38 ` Awais Masood
  2017-09-19 13:49   ` Jan Beulich
  2017-09-26  9:37 ` [PATCH v2 0/2] xen/arm64/ns16550: Support for Allwinner H5 SoC Awais Masood
  2 siblings, 1 reply; 20+ messages in thread
From: Awais Masood @ 2017-09-19 13:38 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
	Awais Masood, ian.jackson, tim, julien.grall, jbeulich

On H5 (Orange Pi PC2) serial driver goes into an infinite loop as soon
as interrupts are enabled. The reason being a residual "busy detect"
interrupt. Since the condition UART_IIR_NOINT is not true, we'll remain
in this while loop forever.

A check has been added to detect "busy detect" interrupt condition and
UART_USR register is read to clear the condition. Conditonal for
dw_usr_busy ensures this hw quirk fix affects only designware uarts.

The check during ns16550_setup_preirq call does not help on H5 because
its called before LCR is set and if the busy condition appears again
during subsequent LCR writes, we end up in this situation.

Tested on Orange Pi PC2

Signed-off-by: Awais Masood <awais.masood@vadion.com>
---
 xen/drivers/char/ns16550.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 6ab5ec3..6630720 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -521,6 +521,18 @@ static void ns16550_interrupt(
             serial_tx_interrupt(port, regs);
         if ( lsr & UART_LSR_DR )
             serial_rx_interrupt(port, regs);
+        if ( uart->dw_usr_bsy &&
+             (ns_read_reg(uart, UART_IIR) & UART_IIR_BSY) == UART_IIR_BSY )
+        {
+            /* If DesignWare 8250 UART became busy again when LCR was written
+             * earlier, it can raise a "busy detect" again.
+             * Read the UART Status Register to clear this state or we'll end up
+             * in an infinte loop because UART_IIR_NOINT is not true.
+             * Placing this check in setup_preirq after LCR write does not work
+             * probably due to a delayed interrupt.
+             */
+            ns_read_reg(uart, UART_USR);
+        }
     }
 }
 
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* Re: [PATCH 2/2] xen/ns16550: Fix ISR lockup on Designware 8250 (H5)
  2017-09-19 13:38 ` [PATCH 2/2] xen/ns16550: Fix ISR lockup on Designware 8250 (H5) Awais Masood
@ 2017-09-19 13:49   ` Jan Beulich
  0 siblings, 0 replies; 20+ messages in thread
From: Jan Beulich @ 2017-09-19 13:49 UTC (permalink / raw)
  To: Awais Masood
  Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
	ian.jackson, tim, julien.grall, xen-devel

>>> On 19.09.17 at 15:38, <awais.masood@vadion.com> wrote:
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -521,6 +521,18 @@ static void ns16550_interrupt(
>              serial_tx_interrupt(port, regs);
>          if ( lsr & UART_LSR_DR )
>              serial_rx_interrupt(port, regs);
> +        if ( uart->dw_usr_bsy &&
> +             (ns_read_reg(uart, UART_IIR) & UART_IIR_BSY) == UART_IIR_BSY )
> +        {
> +            /* If DesignWare 8250 UART became busy again when LCR was written
> +             * earlier, it can raise a "busy detect" again.
> +             * Read the UART Status Register to clear this state or we'll end up
> +             * in an infinte loop because UART_IIR_NOINT is not true.
> +             * Placing this check in setup_preirq after LCR write does not work
> +             * probably due to a delayed interrupt.
> +             */
> +            ns_read_reg(uart, UART_USR);
> +        }

This same code already exists in ns16550_setup_preirq() - please
introduce a helper function. It would also help if you referred to
the commit introducing that other instance of the code, explaining
why what was done there was not enough (because pretty clearly
the author must have assumed that change to be sufficient).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* Re: [PATCH 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i)
  2017-09-19 13:38 ` [PATCH 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i) Awais Masood
@ 2017-09-19 13:50   ` Jan Beulich
  0 siblings, 0 replies; 20+ messages in thread
From: Jan Beulich @ 2017-09-19 13:50 UTC (permalink / raw)
  To: Awais Masood
  Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
	ian.jackson, tim, julien.grall, xen-devel

>>> On 19.09.17 at 15:38, <awais.masood@vadion.com> wrote:
> Tested on Orange Pi PC2.
> 
> Makefile updated to enable ARM64 compilation of sunxi.c.
> 
> sunxi.c updates include:
> 
> Added H5 dt compatibility string.
> 
> Watchdog timer base address is different on sun5oi as compared to sun7i.
> Reset function updated to handle different base addresses.
> 
> Signed-off-by: Awais Masood <awais.masood@vadion.com>
> ---
>  xen/arch/arm/platforms/Makefile |  1 +
>  xen/arch/arm/platforms/sunxi.c  | 40 +++++++++++++++++++++++++++++++---------
>  2 files changed, 32 insertions(+), 9 deletions(-)

I can't correlate the Cc list with the files being changed here.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* [PATCH v2 0/2] xen/arm64/ns16550: Support for Allwinner H5 SoC
  2017-09-19 13:38 [PATCH 0/2] xen/arm64/ns16550: Support for Allwinner H5 SoC Awais Masood
  2017-09-19 13:38 ` [PATCH 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i) Awais Masood
  2017-09-19 13:38 ` [PATCH 2/2] xen/ns16550: Fix ISR lockup on Designware 8250 (H5) Awais Masood
@ 2017-09-26  9:37 ` Awais Masood
  2017-09-26  9:37   ` [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i) Awais Masood
                     ` (2 more replies)
  2 siblings, 3 replies; 20+ messages in thread
From: Awais Masood @ 2017-09-26  9:37 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Awais Masood, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich

This patch series adds support for Allwinner H5 (ARM64/Cortex-A53)

Builds upon existing support for A20/A31(sun7i) with updates to
support different watchdog timer base addresses and device tree
compatibility strings for sun50i.

The ns16550 update is a fix for a hardware quirk that causes a
lockup within interrupt handler.

Tested On:

 -- Hardware Platform: Orange Pi PC2
 -- Dom0 Linux: https://github.com/Icenowy/linux/tree/sunxi64-4.13-rc6
                Mainline Linus kernel tested as well
 -- u-boot: https://github.com/armbian/u-boot-sun50i
 -- xen: master
      Built as:
        make dist-xen XEN_TARGET_ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu-

      For early printk existing switch for sun7i can be used
        CONFIG_EARLY_PRINTK=sun7i

Dom0 boot log: https://pastebin.com/CgUuqpi0

CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>

Awais Masood (2):
  xen/arm64: Add Support for Allwinner H5 (sun50i)
  xen/ns16550: Fix ISR lockup on Designware 8250 (H5)

 xen/arch/arm/platforms/Makefile |  1 +
 xen/arch/arm/platforms/sunxi.c  | 40 +++++++++++++++++++++++++++++++---------
 xen/drivers/char/ns16550.c      | 27 ++++++++++++++++++---------
 3 files changed, 50 insertions(+), 18 deletions(-)

-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i)
  2017-09-26  9:37 ` [PATCH v2 0/2] xen/arm64/ns16550: Support for Allwinner H5 SoC Awais Masood
@ 2017-09-26  9:37   ` Awais Masood
  2017-09-28 20:03     ` Julien Grall
  2017-09-26  9:37   ` [PATCH v2 2/2] xen/ns16550: Fix ISR lockup on Designware 8250 (H5) Awais Masood
  2017-10-04 11:44   ` [PATCH v3] xen/ns16550: Fix ISR lockup on Allwinner uart Awais Masood
  2 siblings, 1 reply; 20+ messages in thread
From: Awais Masood @ 2017-09-26  9:37 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, xen-devel, Awais Masood

This patch adds support for Allwinner H5/sun50i SoC.

Makefile updated to enable ARM64 compilation for sunxi.c.

sunxi.c updates include:
  - Addition of H5/sun50i dt compatibility string.
  - Handling of different Watchdog timer base addresses on sun7i
    and sun50i.

Tested on Orange Pi PC2

Signed-off-by: Awais Masood <awais.masood@vadion.com>

---
Changes since v1:
  - Improved patch description
---
 xen/arch/arm/platforms/Makefile |  1 +
 xen/arch/arm/platforms/sunxi.c  | 40 +++++++++++++++++++++++++++++++---------
 2 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
index 49fa683..722897a 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -6,5 +6,6 @@ obj-$(CONFIG_ARM_32) += omap5.o
 obj-$(CONFIG_ARM_32) += rcar2.o
 obj-$(CONFIG_ARM_64) += seattle.o
 obj-$(CONFIG_ARM_32) += sunxi.o
+obj-$(CONFIG_ARM_64) += sunxi.o
 obj-$(CONFIG_ARM_64) += xgene-storm.o
 obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
diff --git a/xen/arch/arm/platforms/sunxi.c b/xen/arch/arm/platforms/sunxi.c
index 0ba7b3d..06d62e7 100644
--- a/xen/arch/arm/platforms/sunxi.c
+++ b/xen/arch/arm/platforms/sunxi.c
@@ -22,18 +22,18 @@
 #include <asm/io.h>
 
 /* Watchdog constants: */
-#define SUNXI_WDT_BASE            0x01c20c90
+#define SUNXI_WDT_A20_BASE        0x01c20c90
+#define SUNXI_WDT_H5_BASE         0x01c20cA0
 #define SUNXI_WDT_MODE            0x04
-#define SUNXI_WDT_MODEADDR        (SUNXI_WDT_BASE + SUNXI_WDT_MODE)
 #define SUNXI_WDT_MODE_EN         (1 << 0)
 #define SUNXI_WDT_MODE_RST_EN     (1 << 1)
 
 
-static void sunxi_reset(void)
+static void sunxi_reset(u32 base)
 {
     void __iomem *wdt;
 
-    wdt = ioremap_nocache(SUNXI_WDT_MODEADDR & PAGE_MASK, PAGE_SIZE);
+    wdt = ioremap_nocache((base + SUNXI_WDT_MODE) & PAGE_MASK, PAGE_SIZE);
     if ( !wdt )
     {
         dprintk(XENLOG_ERR, "Unable to map watchdog register!\n");
@@ -42,19 +42,35 @@ static void sunxi_reset(void)
 
     /* Enable watchdog to trigger a reset after 500 ms: */
     writel(SUNXI_WDT_MODE_EN | SUNXI_WDT_MODE_RST_EN,
-      wdt + (SUNXI_WDT_MODEADDR & ~PAGE_MASK));
+      wdt + ((base + SUNXI_WDT_MODE) & ~PAGE_MASK));
     iounmap(wdt);
 
     for (;;)
         wfi();
 }
 
-static const char * const sunxi_dt_compat[] __initconst =
+static void sunxi_a20_reset(void)
+{
+    sunxi_reset(SUNXI_WDT_A20_BASE);
+}
+
+static void sunxi_h5_reset(void)
+{
+    sunxi_reset(SUNXI_WDT_H5_BASE);
+}
+
+static const char * const sunxi_dt_allwinner_a20_compat[] __initconst =
 {
     "allwinner,sun7i-a20",
     NULL
 };
 
+static const char * const sunxi_dt_allwinner_h5_compat[] __initconst =
+{
+    "allwinner,sun50i-h5",
+    NULL
+};
+
 static const struct dt_device_match sunxi_blacklist_dev[] __initconst =
 {
     /*
@@ -65,10 +81,16 @@ static const struct dt_device_match sunxi_blacklist_dev[] __initconst =
     { /* sentinel */ },
 };
 
-PLATFORM_START(sunxi, "Allwinner A20")
-    .compatible = sunxi_dt_compat,
+PLATFORM_START(sunxia20, "Allwinner A20")
+    .compatible = sunxi_dt_allwinner_a20_compat,
+    .blacklist_dev = sunxi_blacklist_dev,
+    .reset = sunxi_a20_reset,
+PLATFORM_END
+
+PLATFORM_START(sunxih5, "Allwinner H5")
+    .compatible = sunxi_dt_allwinner_h5_compat,
     .blacklist_dev = sunxi_blacklist_dev,
-    .reset = sunxi_reset,
+    .reset = sunxi_h5_reset,
 PLATFORM_END
 
 /*
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* [PATCH v2 2/2] xen/ns16550: Fix ISR lockup on Designware 8250 (H5)
  2017-09-26  9:37 ` [PATCH v2 0/2] xen/arm64/ns16550: Support for Allwinner H5 SoC Awais Masood
  2017-09-26  9:37   ` [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i) Awais Masood
@ 2017-09-26  9:37   ` Awais Masood
  2017-09-26 11:58     ` Jan Beulich
  2017-10-04 11:44   ` [PATCH v3] xen/ns16550: Fix ISR lockup on Allwinner uart Awais Masood
  2 siblings, 1 reply; 20+ messages in thread
From: Awais Masood @ 2017-09-26  9:37 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Awais Masood, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel

On Allwinner H5 (Orange Pi PC2) serial driver goes into an
infinite loop when interrupts are enabled. The reason is a
residual "busy detect" interrupt. Since the condition
UART_IIR_NOINT will not be true unless this interrupt is
cleared, the interrupt handler will remain locked up in this
while loop.

A hw quirk fix was previously added for designware uart under
commit:
50417cd978aa54930d065ac1f139f935d14af76d

It checks for a busy condition during setup and clears the
condition by reading UART_USR register.

On Allwinner H5 (and H3), the "busy detect" condition occurs
later because an LCR write is performed during setup 'after'
this clear and if uart is busy, the "busy detect" condition
will trigger again and cause the ISR lockup.

To solve this problem, the same UART_USR read operation is
added within the interrupt handler to clear the condition.

Linux dw 8250 driver also handles this condition within
interrupt handler
http://elixir.free-electrons.com/linux/latest/source/drivers/tty/serial/8250/8250_dw.c#L233

Tested on Orange Pi PC2 (H5). Earlier this issue was seen on H3
as well and the same fix should help.

Signed-off-by: Awais Masood <awais.masood@vadion.com>

---
Changes since v1:
 * Common quirk fix code moved to a helper function
 * Patch description improved with earlier commit link
---
 xen/drivers/char/ns16550.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 6ab5ec3..44ed4ec 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -505,6 +505,19 @@ static int ns16550_ioport_invalid(struct ns16550 *uart)
     return ns_read_reg(uart, UART_IER) == 0xff;
 }
 
+static void ns16550_handle_dw_usr_busy_quirk(struct ns16550 *uart)
+{
+    if ( uart->dw_usr_bsy &&
+         (ns_read_reg(uart, UART_IIR) & UART_IIR_BSY) == UART_IIR_BSY )
+    {
+        /* DesignWare 8250 detects if LCR is written while the UART is
+         * busy and raises a "busy detect" interrupt. Read the UART
+         * Status Register to clear this state.
+         */
+        ns_read_reg(uart, UART_USR);
+    }
+}
+
 static void ns16550_interrupt(
     int irq, void *dev_id, struct cpu_user_regs *regs)
 {
@@ -521,6 +534,9 @@ static void ns16550_interrupt(
             serial_tx_interrupt(port, regs);
         if ( lsr & UART_LSR_DR )
             serial_rx_interrupt(port, regs);
+
+        /* Handle the DesignWare 8250 'busy-detect' quirk. */
+        ns16550_handle_dw_usr_busy_quirk(uart);
     }
 }
 
@@ -623,15 +639,8 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
     /* No interrupts. */
     ns_write_reg(uart, UART_IER, 0);
 
-    if ( uart->dw_usr_bsy &&
-         (ns_read_reg(uart, UART_IIR) & UART_IIR_BSY) == UART_IIR_BSY )
-    {
-        /* DesignWare 8250 detects if LCR is written while the UART is
-         * busy and raises a "busy detect" interrupt. Read the UART
-         * Status Register to clear this state.
-         */
-        ns_read_reg(uart, UART_USR);
-    }
+    /* Handle the DesignWare 8250 'busy-detect' quirk. */
+    ns16550_handle_dw_usr_busy_quirk(uart);
 
     /* Line control and baud-rate generator. */
     ns_write_reg(uart, UART_LCR, lcr | UART_LCR_DLAB);
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* Re: [PATCH v2 2/2] xen/ns16550: Fix ISR lockup on Designware 8250 (H5)
  2017-09-26  9:37   ` [PATCH v2 2/2] xen/ns16550: Fix ISR lockup on Designware 8250 (H5) Awais Masood
@ 2017-09-26 11:58     ` Jan Beulich
  0 siblings, 0 replies; 20+ messages in thread
From: Jan Beulich @ 2017-09-26 11:58 UTC (permalink / raw)
  To: Awais Masood
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Ian Jackson, Tim Deegan, xen-devel

>>> On 26.09.17 at 11:37, <awais.masood@vadion.com> wrote:

First of all please only send to one of xen-devel@lists.xen.org and
xen-devel@lists.xenproject.org.

> On Allwinner H5 (Orange Pi PC2) serial driver goes into an
> infinite loop when interrupts are enabled. The reason is a
> residual "busy detect" interrupt. Since the condition
> UART_IIR_NOINT will not be true unless this interrupt is
> cleared, the interrupt handler will remain locked up in this
> while loop.
> 
> A hw quirk fix was previously added for designware uart under
> commit:
> 50417cd978aa54930d065ac1f139f935d14af76d
> 
> It checks for a busy condition during setup and clears the
> condition by reading UART_USR register.
> 
> On Allwinner H5 (and H3), the "busy detect" condition occurs
> later because an LCR write is performed during setup 'after'
> this clear and if uart is busy, the "busy detect" condition
> will trigger again and cause the ISR lockup.
> 
> To solve this problem, the same UART_USR read operation is
> added within the interrupt handler to clear the condition.
> 
> Linux dw 8250 driver also handles this condition within
> interrupt handler
> http://elixir.free-electrons.com/linux/latest/source/drivers/tty/serial/8250/ 
> 8250_dw.c#L233
> 
> Tested on Orange Pi PC2 (H5). Earlier this issue was seen on H3
> as well and the same fix should help.

So the description almost exclusively talks about Allwinner hardware,
but ...

> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -505,6 +505,19 @@ static int ns16550_ioport_invalid(struct ns16550 *uart)
>      return ns_read_reg(uart, UART_IER) == 0xff;
>  }
>  
> +static void ns16550_handle_dw_usr_busy_quirk(struct ns16550 *uart)
> +{
> +    if ( uart->dw_usr_bsy &&
> +         (ns_read_reg(uart, UART_IIR) & UART_IIR_BSY) == UART_IIR_BSY )
> +    {
> +        /* DesignWare 8250 detects if LCR is written while the UART is
> +         * busy and raises a "busy detect" interrupt. Read the UART
> +         * Status Register to clear this state.
> +         */
> +        ns_read_reg(uart, UART_USR);
> +    }
> +}

... here you don't mention it at all, and ...

> @@ -521,6 +534,9 @@ static void ns16550_interrupt(
>              serial_tx_interrupt(port, regs);
>          if ( lsr & UART_LSR_DR )
>              serial_rx_interrupt(port, regs);
> +
> +        /* Handle the DesignWare 8250 'busy-detect' quirk. */
> +        ns16550_handle_dw_usr_busy_quirk(uart);
>      }
>  }

... here, according to the description, the issue doesn't even apply
to DesignWare hardware.

Also while indeed there are many example to the contrary in this
file, please don't follow the bad habit of prefixing local functions
with something derived from the file name.
handle_dw_usr_busy_quirk() is perfectly fine a name for the new
function.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* Re: [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i)
  2017-09-26  9:37   ` [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i) Awais Masood
@ 2017-09-28 20:03     ` Julien Grall
  2017-09-28 22:49       ` Andre Przywara
  0 siblings, 1 reply; 20+ messages in thread
From: Julien Grall @ 2017-09-28 20:03 UTC (permalink / raw)
  To: Awais Masood, xen-devel; +Cc: Andre Przywara, Stefano Stabellini, Xen-devel

Hi,

On 09/26/2017 10:37 AM, Awais Masood wrote:
> This patch adds support for Allwinner H5/sun50i SoC.
> 
> Makefile updated to enable ARM64 compilation for sunxi.c.
> 
> sunxi.c updates include:
>    - Addition of H5/sun50i dt compatibility string.
>    - Handling of different Watchdog timer base addresses on sun7i
>      and sun50i.
> 
> Tested on Orange Pi PC2
> 
> Signed-off-by: Awais Masood <awais.masood@vadion.com>
> 
> ---
> Changes since v1:
>    - Improved patch description
> ---
>   xen/arch/arm/platforms/Makefile |  1 +
>   xen/arch/arm/platforms/sunxi.c  | 40 +++++++++++++++++++++++++++++++---------
>   2 files changed, 32 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> index 49fa683..722897a 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -6,5 +6,6 @@ obj-$(CONFIG_ARM_32) += omap5.o
>   obj-$(CONFIG_ARM_32) += rcar2.o
>   obj-$(CONFIG_ARM_64) += seattle.o
>   obj-$(CONFIG_ARM_32) += sunxi.o
> +obj-$(CONFIG_ARM_64) += sunxi.o

Please use obj-y += sunxi.o as the platform is now supported by both 
Arm32 and Arm64.

>   obj-$(CONFIG_ARM_64) += xgene-storm.o
>   obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
> diff --git a/xen/arch/arm/platforms/sunxi.c b/xen/arch/arm/platforms/sunxi.c
> index 0ba7b3d..06d62e7 100644
> --- a/xen/arch/arm/platforms/sunxi.c
> +++ b/xen/arch/arm/platforms/sunxi.c
> @@ -22,18 +22,18 @@
>   #include <asm/io.h>
>   
>   /* Watchdog constants: */
> -#define SUNXI_WDT_BASE            0x01c20c90
> +#define SUNXI_WDT_A20_BASE        0x01c20c90
> +#define SUNXI_WDT_H5_BASE         0x01c20cA0

I know that we hardcoded this value for the A20. However, I am wondering 
if we could find this address from the Device-Tree?

>   #define SUNXI_WDT_MODE            0x04
> -#define SUNXI_WDT_MODEADDR        (SUNXI_WDT_BASE + SUNXI_WDT_MODE)
>   #define SUNXI_WDT_MODE_EN         (1 << 0)
>   #define SUNXI_WDT_MODE_RST_EN     (1 << 1)
>   
>   
> -static void sunxi_reset(void)
> +static void sunxi_reset(u32 base)
>   {
>       void __iomem *wdt;
>   
> -    wdt = ioremap_nocache(SUNXI_WDT_MODEADDR & PAGE_MASK, PAGE_SIZE);
> +    wdt = ioremap_nocache((base + SUNXI_WDT_MODE) & PAGE_MASK, PAGE_SIZE);
>       if ( !wdt )
>       {
>           dprintk(XENLOG_ERR, "Unable to map watchdog register!\n");
> @@ -42,19 +42,35 @@ static void sunxi_reset(void)
>   
>       /* Enable watchdog to trigger a reset after 500 ms: */
>       writel(SUNXI_WDT_MODE_EN | SUNXI_WDT_MODE_RST_EN,
> -      wdt + (SUNXI_WDT_MODEADDR & ~PAGE_MASK));
> +      wdt + ((base + SUNXI_WDT_MODE) & ~PAGE_MASK));
>       iounmap(wdt); >
>       for (;;)
>           wfi();
>   }
> 
> -static const char * const sunxi_dt_compat[] __initconst =
> +static void sunxi_a20_reset(void)
> +{
> +    sunxi_reset(SUNXI_WDT_A20_BASE);
> +}
> +
> +static void sunxi_h5_reset(void)
> +{
> +    sunxi_reset(SUNXI_WDT_H5_BASE);

If I read correctly the Device-Tree for 
(linux/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi), the firmware is 
supporting PSCI 0.2.

PSCI 0.2 provides call for power-off/reset, so implementation the reset 
callback should not be necessary.

Similarly the cubietrucks we have in osstest are using PSCI 0.2 and 
should not need the reset. Andre do you know if it is the case for all 
the A20?

For H5, I would impose PSCI 0.2 as the way to reset the platform. I am 
leaning towards the same for A20 given that it would just be a matter of 
upgrading the bootloader. Most likely you would have already done that 
to get fixes.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* Re: [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i)
  2017-09-28 20:03     ` Julien Grall
@ 2017-09-28 22:49       ` Andre Przywara
  2017-09-29 16:35         ` Andre Przywara
  2017-10-03 11:32         ` Julien Grall
  0 siblings, 2 replies; 20+ messages in thread
From: Andre Przywara @ 2017-09-28 22:49 UTC (permalink / raw)
  To: Julien Grall, Awais Masood, xen-devel; +Cc: Stefano Stabellini, Xen-devel

Hi,

On 09/28/2017 01:03 PM, Julien Grall wrote:
> Hi,
> 
> On 09/26/2017 10:37 AM, Awais Masood wrote:
>> This patch adds support for Allwinner H5/sun50i SoC.
>>
>> Makefile updated to enable ARM64 compilation for sunxi.c.
>>
>> sunxi.c updates include:
>>    - Addition of H5/sun50i dt compatibility string.
>>    - Handling of different Watchdog timer base addresses on sun7i
>>      and sun50i.
>>
>> Tested on Orange Pi PC2
>>
>> Signed-off-by: Awais Masood <awais.masood@vadion.com>
>>
>> ---
>> Changes since v1:
>>    - Improved patch description
>> ---
>>   xen/arch/arm/platforms/Makefile |  1 +
>>   xen/arch/arm/platforms/sunxi.c  | 40 
>> +++++++++++++++++++++++++++++++---------
>>   2 files changed, 32 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/arm/platforms/Makefile 
>> b/xen/arch/arm/platforms/Makefile
>> index 49fa683..722897a 100644
>> --- a/xen/arch/arm/platforms/Makefile
>> +++ b/xen/arch/arm/platforms/Makefile
>> @@ -6,5 +6,6 @@ obj-$(CONFIG_ARM_32) += omap5.o
>>   obj-$(CONFIG_ARM_32) += rcar2.o
>>   obj-$(CONFIG_ARM_64) += seattle.o
>>   obj-$(CONFIG_ARM_32) += sunxi.o
>> +obj-$(CONFIG_ARM_64) += sunxi.o
> 
> Please use obj-y += sunxi.o as the platform is now supported by both 
> Arm32 and Arm64.
> 
>>   obj-$(CONFIG_ARM_64) += xgene-storm.o
>>   obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
>> diff --git a/xen/arch/arm/platforms/sunxi.c 
>> b/xen/arch/arm/platforms/sunxi.c
>> index 0ba7b3d..06d62e7 100644
>> --- a/xen/arch/arm/platforms/sunxi.c
>> +++ b/xen/arch/arm/platforms/sunxi.c
>> @@ -22,18 +22,18 @@
>>   #include <asm/io.h>
>>   /* Watchdog constants: */
>> -#define SUNXI_WDT_BASE            0x01c20c90
>> +#define SUNXI_WDT_A20_BASE        0x01c20c90
>> +#define SUNXI_WDT_H5_BASE         0x01c20cA0
> 
> I know that we hardcoded this value for the A20. However, I am wondering 
> if we could find this address from the Device-Tree?

Yes, both sun7i-a20.dtsi and the H5 .dts have the WDT.
Its compatible strings are sun4i-a10-wdt and sun6i-a31-wdt, 
respectively. I have to check what the differences are, but I guess for 
our purposes these should be small.
That seems like a call to some proper DT driven timer/WDT driver?

> 
>>   #define SUNXI_WDT_MODE            0x04
>> -#define SUNXI_WDT_MODEADDR        (SUNXI_WDT_BASE + SUNXI_WDT_MODE)
>>   #define SUNXI_WDT_MODE_EN         (1 << 0)
>>   #define SUNXI_WDT_MODE_RST_EN     (1 << 1)
>> -static void sunxi_reset(void)
>> +static void sunxi_reset(u32 base)
>>   {
>>       void __iomem *wdt;
>> -    wdt = ioremap_nocache(SUNXI_WDT_MODEADDR & PAGE_MASK, PAGE_SIZE);
>> +    wdt = ioremap_nocache((base + SUNXI_WDT_MODE) & PAGE_MASK, 
>> PAGE_SIZE);
>>       if ( !wdt )
>>       {
>>           dprintk(XENLOG_ERR, "Unable to map watchdog register!\n");
>> @@ -42,19 +42,35 @@ static void sunxi_reset(void)
>>       /* Enable watchdog to trigger a reset after 500 ms: */
>>       writel(SUNXI_WDT_MODE_EN | SUNXI_WDT_MODE_RST_EN,
>> -      wdt + (SUNXI_WDT_MODEADDR & ~PAGE_MASK));
>> +      wdt + ((base + SUNXI_WDT_MODE) & ~PAGE_MASK));
>>       iounmap(wdt); >
>>       for (;;)
>>           wfi();
>>   }
>>
>> -static const char * const sunxi_dt_compat[] __initconst =
>> +static void sunxi_a20_reset(void)
>> +{
>> +    sunxi_reset(SUNXI_WDT_A20_BASE);
>> +}
>> +
>> +static void sunxi_h5_reset(void)
>> +{
>> +    sunxi_reset(SUNXI_WDT_H5_BASE);
> 
> If I read correctly the Device-Tree for 
> (linux/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi), the firmware is 
> supporting PSCI 0.2.
> 
> PSCI 0.2 provides call for power-off/reset, so implementation the reset 
> callback should not be necessary.

Yes, indeed, on the H5 PSCI 0.2 reset works via ATF.

> Similarly the cubietrucks we have in osstest are using PSCI 0.2 and 
> should not need the reset. Andre do you know if it is the case for all 
> the A20?

It claims 0.2, but in fact it seems not to be fully compliant, as (from 
looking at the code) U-Boot lacks the reset and poweroff calls. But it 
looks rather straight-forward to add them, as U-Boot knows how to reset 
and one would just need to wire up psci_system_reset to this.

> For H5, I would impose PSCI 0.2 as the way to reset the platform.

Yes.

> I am 
> leaning towards the same for A20 given that it would just be a matter of 
> upgrading the bootloader. Most likely you would have already done that 
> to get fixes.

Not sure we should push people to upgrade U-Boot in general to be able 
to use Xen, but as even current mainline U-Boot doesn't seem to support 
it, I would rather leave the current reset support code in. Last time I 
checked Linux does the same.

Cheers,
Andre.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* Re: [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i)
  2017-09-28 22:49       ` Andre Przywara
@ 2017-09-29 16:35         ` Andre Przywara
  2017-10-04  9:16           ` Awais Masood
  2017-10-03 11:32         ` Julien Grall
  1 sibling, 1 reply; 20+ messages in thread
From: Andre Przywara @ 2017-09-29 16:35 UTC (permalink / raw)
  To: Julien Grall, Awais Masood, xen-devel; +Cc: Stefano Stabellini, Xen-devel

Hi,

On 09/28/2017 03:49 PM, Andre Przywara wrote:
> Hi,
> 
> On 09/28/2017 01:03 PM, Julien Grall wrote:
>> Hi,
>>
>> On 09/26/2017 10:37 AM, Awais Masood wrote:
>>> This patch adds support for Allwinner H5/sun50i SoC.
>>>
>>> Makefile updated to enable ARM64 compilation for sunxi.c.

...

>>> --- a/xen/arch/arm/platforms/sunxi.c
>>> +++ b/xen/arch/arm/platforms/sunxi.c
>>> @@ -22,18 +22,18 @@
>>>   #include <asm/io.h>
>>>   /* Watchdog constants: */
>>> -#define SUNXI_WDT_BASE            0x01c20c90
>>> +#define SUNXI_WDT_A20_BASE        0x01c20c90
>>> +#define SUNXI_WDT_H5_BASE         0x01c20cA0
>>
>> I know that we hardcoded this value for the A20. However, I am 
>> wondering if we could find this address from the Device-Tree?
> 
> Yes, both sun7i-a20.dtsi and the H5 .dts have the WDT.
> Its compatible strings are sun4i-a10-wdt and sun6i-a31-wdt, 
> respectively. I have to check what the differences are, but I guess for 
> our purposes these should be small.
> That seems like a call to some proper DT driven timer/WDT driver?

Scratch that. I just see that this is solely used for the reset 
function. So we should not need this for the H5 (and the A64 for that 
matter). We may need this for the H3 (Cortex-A7) support, however, which 
seems quite popular on cheap boards.

Cheers,
Andre

>>>   #define SUNXI_WDT_MODE            0x04
>>> -#define SUNXI_WDT_MODEADDR        (SUNXI_WDT_BASE + SUNXI_WDT_MODE)
>>>   #define SUNXI_WDT_MODE_EN         (1 << 0)
>>>   #define SUNXI_WDT_MODE_RST_EN     (1 << 1)
>>> -static void sunxi_reset(void)
>>> +static void sunxi_reset(u32 base)
>>>   {
>>>       void __iomem *wdt;
>>> -    wdt = ioremap_nocache(SUNXI_WDT_MODEADDR & PAGE_MASK, PAGE_SIZE);
>>> +    wdt = ioremap_nocache((base + SUNXI_WDT_MODE) & PAGE_MASK, 
>>> PAGE_SIZE);
>>>       if ( !wdt )
>>>       {
>>>           dprintk(XENLOG_ERR, "Unable to map watchdog register!\n");
>>> @@ -42,19 +42,35 @@ static void sunxi_reset(void)
>>>       /* Enable watchdog to trigger a reset after 500 ms: */
>>>       writel(SUNXI_WDT_MODE_EN | SUNXI_WDT_MODE_RST_EN,
>>> -      wdt + (SUNXI_WDT_MODEADDR & ~PAGE_MASK));
>>> +      wdt + ((base + SUNXI_WDT_MODE) & ~PAGE_MASK));
>>>       iounmap(wdt); >
>>>       for (;;)
>>>           wfi();
>>>   }
>>>
>>> -static const char * const sunxi_dt_compat[] __initconst =
>>> +static void sunxi_a20_reset(void)
>>> +{
>>> +    sunxi_reset(SUNXI_WDT_A20_BASE);
>>> +}
>>> +
>>> +static void sunxi_h5_reset(void)
>>> +{
>>> +    sunxi_reset(SUNXI_WDT_H5_BASE);
>>
>> If I read correctly the Device-Tree for 
>> (linux/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi), the firmware is 
>> supporting PSCI 0.2.
>>
>> PSCI 0.2 provides call for power-off/reset, so implementation the 
>> reset callback should not be necessary.
> 
> Yes, indeed, on the H5 PSCI 0.2 reset works via ATF.
> 
>> Similarly the cubietrucks we have in osstest are using PSCI 0.2 and 
>> should not need the reset. Andre do you know if it is the case for all 
>> the A20?
> 
> It claims 0.2, but in fact it seems not to be fully compliant, as (from 
> looking at the code) U-Boot lacks the reset and poweroff calls. But it 
> looks rather straight-forward to add them, as U-Boot knows how to reset 
> and one would just need to wire up psci_system_reset to this.
> 
>> For H5, I would impose PSCI 0.2 as the way to reset the platform.
> 
> Yes.
> 
>> I am leaning towards the same for A20 given that it would just be a 
>> matter of upgrading the bootloader. Most likely you would have already 
>> done that to get fixes.
> 
> Not sure we should push people to upgrade U-Boot in general to be able 
> to use Xen, but as even current mainline U-Boot doesn't seem to support 
> it, I would rather leave the current reset support code in. Last time I 
> checked Linux does the same.
> 
> Cheers,
> Andre.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* Re: [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i)
  2017-09-28 22:49       ` Andre Przywara
  2017-09-29 16:35         ` Andre Przywara
@ 2017-10-03 11:32         ` Julien Grall
  1 sibling, 0 replies; 20+ messages in thread
From: Julien Grall @ 2017-10-03 11:32 UTC (permalink / raw)
  To: Andre Przywara, Awais Masood, xen-devel; +Cc: Stefano Stabellini, Xen-devel

On 28/09/17 23:49, Andre Przywara wrote:
> Hi,

Hi,

> On 09/28/2017 01:03 PM, Julien Grall wrote:
>>
>>>   #define SUNXI_WDT_MODE            0x04
>>> -#define SUNXI_WDT_MODEADDR        (SUNXI_WDT_BASE + SUNXI_WDT_MODE)
>>>   #define SUNXI_WDT_MODE_EN         (1 << 0)
>>>   #define SUNXI_WDT_MODE_RST_EN     (1 << 1)
>>> -static void sunxi_reset(void)
>>> +static void sunxi_reset(u32 base)
>>>   {
>>>       void __iomem *wdt;
>>> -    wdt = ioremap_nocache(SUNXI_WDT_MODEADDR & PAGE_MASK, PAGE_SIZE);
>>> +    wdt = ioremap_nocache((base + SUNXI_WDT_MODE) & PAGE_MASK, 
>>> PAGE_SIZE);
>>>       if ( !wdt )
>>>       {
>>>           dprintk(XENLOG_ERR, "Unable to map watchdog register!\n");
>>> @@ -42,19 +42,35 @@ static void sunxi_reset(void)
>>>       /* Enable watchdog to trigger a reset after 500 ms: */
>>>       writel(SUNXI_WDT_MODE_EN | SUNXI_WDT_MODE_RST_EN,
>>> -      wdt + (SUNXI_WDT_MODEADDR & ~PAGE_MASK));
>>> +      wdt + ((base + SUNXI_WDT_MODE) & ~PAGE_MASK));
>>>       iounmap(wdt); >
>>>       for (;;)
>>>           wfi();
>>>   }
>>>
>>> -static const char * const sunxi_dt_compat[] __initconst =
>>> +static void sunxi_a20_reset(void)
>>> +{
>>> +    sunxi_reset(SUNXI_WDT_A20_BASE);
>>> +}
>>> +
>>> +static void sunxi_h5_reset(void)
>>> +{
>>> +    sunxi_reset(SUNXI_WDT_H5_BASE);
>>
>> If I read correctly the Device-Tree for 
>> (linux/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi), the firmware is 
>> supporting PSCI 0.2.
>>
>> PSCI 0.2 provides call for power-off/reset, so implementation the 
>> reset callback should not be necessary.
> 
> Yes, indeed, on the H5 PSCI 0.2 reset works via ATF.
> 
>> Similarly the cubietrucks we have in osstest are using PSCI 0.2 and 
>> should not need the reset. Andre do you know if it is the case for all 
>> the A20?
> 
> It claims 0.2, but in fact it seems not to be fully compliant, as (from 
> looking at the code) U-Boot lacks the reset and poweroff calls. But it 
> looks rather straight-forward to add them, as U-Boot knows how to reset 
> and one would just need to wire up psci_system_reset to this.

Hmmm ok :/. Please ignore my suggestion below to drop reset callback for 
all the allwinner platforms.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* Re: [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i)
  2017-09-29 16:35         ` Andre Przywara
@ 2017-10-04  9:16           ` Awais Masood
  2017-10-04  9:26             ` Andre Przywara
  0 siblings, 1 reply; 20+ messages in thread
From: Awais Masood @ 2017-10-04  9:16 UTC (permalink / raw)
  To: Andre Przywara, Julien Grall, xen-devel; +Cc: Stefano Stabellini

Hi,

On 09/29/2017 09:35 PM, Andre Przywara wrote:
> Hi,
> 
> On 09/28/2017 03:49 PM, Andre Przywara wrote:
>> Hi,
>>
>> On 09/28/2017 01:03 PM, Julien Grall wrote:
>>> Hi,
>>>
>>> On 09/26/2017 10:37 AM, Awais Masood wrote:
>>>> This patch adds support for Allwinner H5/sun50i SoC.
>>>>
>>>> Makefile updated to enable ARM64 compilation for sunxi.c.
> 
> ...
> 
>>>> --- a/xen/arch/arm/platforms/sunxi.c
>>>> +++ b/xen/arch/arm/platforms/sunxi.c
>>>> @@ -22,18 +22,18 @@
>>>>   #include <asm/io.h>
>>>>   /* Watchdog constants: */
>>>> -#define SUNXI_WDT_BASE            0x01c20c90
>>>> +#define SUNXI_WDT_A20_BASE        0x01c20c90
>>>> +#define SUNXI_WDT_H5_BASE         0x01c20cA0
>>>
>>> I know that we hardcoded this value for the A20. However, I am wondering if we could find this address from the Device-Tree?
>>
>> Yes, both sun7i-a20.dtsi and the H5 .dts have the WDT.
>> Its compatible strings are sun4i-a10-wdt and sun6i-a31-wdt, respectively. I have to check what the differences are, but I guess for our purposes these should be small.
>> That seems like a call to some proper DT driven timer/WDT driver?
> 
> Scratch that. I just see that this is solely used for the reset function. So we should not need this for the H5 (and the A64 for that matter). We may need this for the H3 (Cortex-A7) support, however, which seems quite popular on cheap boards.
> 

Since reset routine will not be required with PSCI, I assume should revert the reset code changes for this H5 patch and leave the DT retrieval for another patch that adds H3 support. Or should I try that stuff for next version of this patch? 

Awais

> Cheers,
> Andre
> 
>>>>   #define SUNXI_WDT_MODE            0x04
>>>> -#define SUNXI_WDT_MODEADDR        (SUNXI_WDT_BASE + SUNXI_WDT_MODE)
>>>>   #define SUNXI_WDT_MODE_EN         (1 << 0)
>>>>   #define SUNXI_WDT_MODE_RST_EN     (1 << 1)
>>>> -static void sunxi_reset(void)
>>>> +static void sunxi_reset(u32 base)
>>>>   {
>>>>       void __iomem *wdt;
>>>> -    wdt = ioremap_nocache(SUNXI_WDT_MODEADDR & PAGE_MASK, PAGE_SIZE);
>>>> +    wdt = ioremap_nocache((base + SUNXI_WDT_MODE) & PAGE_MASK, PAGE_SIZE);
>>>>       if ( !wdt )
>>>>       {
>>>>           dprintk(XENLOG_ERR, "Unable to map watchdog register!\n");
>>>> @@ -42,19 +42,35 @@ static void sunxi_reset(void)
>>>>       /* Enable watchdog to trigger a reset after 500 ms: */
>>>>       writel(SUNXI_WDT_MODE_EN | SUNXI_WDT_MODE_RST_EN,
>>>> -      wdt + (SUNXI_WDT_MODEADDR & ~PAGE_MASK));
>>>> +      wdt + ((base + SUNXI_WDT_MODE) & ~PAGE_MASK));
>>>>       iounmap(wdt); >
>>>>       for (;;)
>>>>           wfi();
>>>>   }
>>>>
>>>> -static const char * const sunxi_dt_compat[] __initconst =
>>>> +static void sunxi_a20_reset(void)
>>>> +{
>>>> +    sunxi_reset(SUNXI_WDT_A20_BASE);
>>>> +}
>>>> +
>>>> +static void sunxi_h5_reset(void)
>>>> +{
>>>> +    sunxi_reset(SUNXI_WDT_H5_BASE);
>>>
>>> If I read correctly the Device-Tree for (linux/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi), the firmware is supporting PSCI 0.2.
>>>
>>> PSCI 0.2 provides call for power-off/reset, so implementation the reset callback should not be necessary.
>>
>> Yes, indeed, on the H5 PSCI 0.2 reset works via ATF.
>>
>>> Similarly the cubietrucks we have in osstest are using PSCI 0.2 and should not need the reset. Andre do you know if it is the case for all the A20?
>>
>> It claims 0.2, but in fact it seems not to be fully compliant, as (from looking at the code) U-Boot lacks the reset and poweroff calls. But it looks rather straight-forward to add them, as U-Boot knows how to reset and one would just need to wire up psci_system_reset to this.
>>
>>> For H5, I would impose PSCI 0.2 as the way to reset the platform.
>>
>> Yes.
>>
>>> I am leaning towards the same for A20 given that it would just be a matter of upgrading the bootloader. Most likely you would have already done that to get fixes.
>>
>> Not sure we should push people to upgrade U-Boot in general to be able to use Xen, but as even current mainline U-Boot doesn't seem to support it, I would rather leave the current reset support code in. Last time I checked Linux does the same.
>>
>> Cheers,
>> Andre.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* Re: [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i)
  2017-10-04  9:16           ` Awais Masood
@ 2017-10-04  9:26             ` Andre Przywara
  2017-10-04  9:39               ` Awais Masood
  0 siblings, 1 reply; 20+ messages in thread
From: Andre Przywara @ 2017-10-04  9:26 UTC (permalink / raw)
  To: Awais Masood, Julien Grall, xen-devel; +Cc: Stefano Stabellini

Hi Awais,

On 04/10/17 10:16, Awais Masood wrote:
> Hi,
> 
> On 09/29/2017 09:35 PM, Andre Przywara wrote:
>> Hi,
>>
>> On 09/28/2017 03:49 PM, Andre Przywara wrote:
>>> Hi,
>>>
>>> On 09/28/2017 01:03 PM, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 09/26/2017 10:37 AM, Awais Masood wrote:
>>>>> This patch adds support for Allwinner H5/sun50i SoC.
>>>>>
>>>>> Makefile updated to enable ARM64 compilation for sunxi.c.
>>
>> ...
>>
>>>>> --- a/xen/arch/arm/platforms/sunxi.c
>>>>> +++ b/xen/arch/arm/platforms/sunxi.c
>>>>> @@ -22,18 +22,18 @@
>>>>>   #include <asm/io.h>
>>>>>   /* Watchdog constants: */
>>>>> -#define SUNXI_WDT_BASE            0x01c20c90
>>>>> +#define SUNXI_WDT_A20_BASE        0x01c20c90
>>>>> +#define SUNXI_WDT_H5_BASE         0x01c20cA0
>>>>
>>>> I know that we hardcoded this value for the A20. However, I am wondering if we could find this address from the Device-Tree?
>>>
>>> Yes, both sun7i-a20.dtsi and the H5 .dts have the WDT.
>>> Its compatible strings are sun4i-a10-wdt and sun6i-a31-wdt, respectively. I have to check what the differences are, but I guess for our purposes these should be small.
>>> That seems like a call to some proper DT driven timer/WDT driver?
>>
>> Scratch that. I just see that this is solely used for the reset function. So we should not need this for the H5 (and the A64 for that matter). We may need this for the H3 (Cortex-A7) support, however, which seems quite popular on cheap boards.
>>
> 
> Since reset routine will not be required with PSCI, I assume should revert the reset code changes for this H5 patch and leave the DT retrieval for another patch that adds H3 support. Or should I try that stuff for next version of this patch? 

Thanks for the offer, but I already made a patch that adds support for
basically all virtualization capable Allwinner SoCs (both v7 and v8
ones). This looks into the DT for ARMv7 SoCs, but relies entirely on
PSCI for ARMv8 SoCs. I just need to test it, then will send it out.

So actually we won't need anything from that patch here at all, since my
patch supersedes it in a more generic way.
Do you plan on reworking/resending the UART fix (which should come
first, btw, as it is a prerequisite for H5 enablement)?

You could either send the UART fix on its own if there are changes or I
include it as patch 1/2 of my Allwinner "series".

Thanks!
Andre.

>> Cheers,
>> Andre
>>
>>>>>   #define SUNXI_WDT_MODE            0x04
>>>>> -#define SUNXI_WDT_MODEADDR        (SUNXI_WDT_BASE + SUNXI_WDT_MODE)
>>>>>   #define SUNXI_WDT_MODE_EN         (1 << 0)
>>>>>   #define SUNXI_WDT_MODE_RST_EN     (1 << 1)
>>>>> -static void sunxi_reset(void)
>>>>> +static void sunxi_reset(u32 base)
>>>>>   {
>>>>>       void __iomem *wdt;
>>>>> -    wdt = ioremap_nocache(SUNXI_WDT_MODEADDR & PAGE_MASK, PAGE_SIZE);
>>>>> +    wdt = ioremap_nocache((base + SUNXI_WDT_MODE) & PAGE_MASK, PAGE_SIZE);
>>>>>       if ( !wdt )
>>>>>       {
>>>>>           dprintk(XENLOG_ERR, "Unable to map watchdog register!\n");
>>>>> @@ -42,19 +42,35 @@ static void sunxi_reset(void)
>>>>>       /* Enable watchdog to trigger a reset after 500 ms: */
>>>>>       writel(SUNXI_WDT_MODE_EN | SUNXI_WDT_MODE_RST_EN,
>>>>> -      wdt + (SUNXI_WDT_MODEADDR & ~PAGE_MASK));
>>>>> +      wdt + ((base + SUNXI_WDT_MODE) & ~PAGE_MASK));
>>>>>       iounmap(wdt); >
>>>>>       for (;;)
>>>>>           wfi();
>>>>>   }
>>>>>
>>>>> -static const char * const sunxi_dt_compat[] __initconst =
>>>>> +static void sunxi_a20_reset(void)
>>>>> +{
>>>>> +    sunxi_reset(SUNXI_WDT_A20_BASE);
>>>>> +}
>>>>> +
>>>>> +static void sunxi_h5_reset(void)
>>>>> +{
>>>>> +    sunxi_reset(SUNXI_WDT_H5_BASE);
>>>>
>>>> If I read correctly the Device-Tree for (linux/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi), the firmware is supporting PSCI 0.2.
>>>>
>>>> PSCI 0.2 provides call for power-off/reset, so implementation the reset callback should not be necessary.
>>>
>>> Yes, indeed, on the H5 PSCI 0.2 reset works via ATF.
>>>
>>>> Similarly the cubietrucks we have in osstest are using PSCI 0.2 and should not need the reset. Andre do you know if it is the case for all the A20?
>>>
>>> It claims 0.2, but in fact it seems not to be fully compliant, as (from looking at the code) U-Boot lacks the reset and poweroff calls. But it looks rather straight-forward to add them, as U-Boot knows how to reset and one would just need to wire up psci_system_reset to this.
>>>
>>>> For H5, I would impose PSCI 0.2 as the way to reset the platform.
>>>
>>> Yes.
>>>
>>>> I am leaning towards the same for A20 given that it would just be a matter of upgrading the bootloader. Most likely you would have already done that to get fixes.
>>>
>>> Not sure we should push people to upgrade U-Boot in general to be able to use Xen, but as even current mainline U-Boot doesn't seem to support it, I would rather leave the current reset support code in. Last time I checked Linux does the same.
>>>
>>> Cheers,
>>> Andre.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* Re: [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i)
  2017-10-04  9:26             ` Andre Przywara
@ 2017-10-04  9:39               ` Awais Masood
  2017-10-04 10:03                 ` Andre Przywara
  0 siblings, 1 reply; 20+ messages in thread
From: Awais Masood @ 2017-10-04  9:39 UTC (permalink / raw)
  To: Andre Przywara, Julien Grall, xen-devel; +Cc: Stefano Stabellini

Hi,

On 10/04/2017 02:26 PM, Andre Przywara wrote:
> Hi Awais,
> 
> On 04/10/17 10:16, Awais Masood wrote:
>> Hi,
>>
>> On 09/29/2017 09:35 PM, Andre Przywara wrote:
>>> Hi,
>>>
>>> On 09/28/2017 03:49 PM, Andre Przywara wrote:
>>>> Hi,
>>>>
>>>> On 09/28/2017 01:03 PM, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 09/26/2017 10:37 AM, Awais Masood wrote:
>>>>>> This patch adds support for Allwinner H5/sun50i SoC.
>>>>>>
>>>>>> Makefile updated to enable ARM64 compilation for sunxi.c.
>>>
>>> ...
>>>
>>>>>> --- a/xen/arch/arm/platforms/sunxi.c
>>>>>> +++ b/xen/arch/arm/platforms/sunxi.c
>>>>>> @@ -22,18 +22,18 @@
>>>>>>   #include <asm/io.h>
>>>>>>   /* Watchdog constants: */
>>>>>> -#define SUNXI_WDT_BASE            0x01c20c90
>>>>>> +#define SUNXI_WDT_A20_BASE        0x01c20c90
>>>>>> +#define SUNXI_WDT_H5_BASE         0x01c20cA0
>>>>>
>>>>> I know that we hardcoded this value for the A20. However, I am wondering if we could find this address from the Device-Tree?
>>>>
>>>> Yes, both sun7i-a20.dtsi and the H5 .dts have the WDT.
>>>> Its compatible strings are sun4i-a10-wdt and sun6i-a31-wdt, respectively. I have to check what the differences are, but I guess for our purposes these should be small.
>>>> That seems like a call to some proper DT driven timer/WDT driver?
>>>
>>> Scratch that. I just see that this is solely used for the reset function. So we should not need this for the H5 (and the A64 for that matter). We may need this for the H3 (Cortex-A7) support, however, which seems quite popular on cheap boards.
>>>
>>
>> Since reset routine will not be required with PSCI, I assume should revert the reset code changes for this H5 patch and leave the DT retrieval for another patch that adds H3 support. Or should I try that stuff for next version of this patch? 
> 
> Thanks for the offer, but I already made a patch that adds support for
> basically all virtualization capable Allwinner SoCs (both v7 and v8
> ones). This looks into the DT for ARMv7 SoCs, but relies entirely on
> PSCI for ARMv8 SoCs. I just need to test it, then will send it out.
> 
> So actually we won't need anything from that patch here at all, since my
> patch supersedes it in a more generic way.
> Do you plan on reworking/resending the UART fix (which should come
> first, btw, as it is a prerequisite for H5 enablement)?
> 
> You could either send the UART fix on its own if there are changes or I
> include it as patch 1/2 of my Allwinner "series".

I'll send the latest version of UART fix as a standalone patch.

Awais

> 
> Thanks!
> Andre.
> 
>>> Cheers,
>>> Andre
>>>
>>>>>>   #define SUNXI_WDT_MODE            0x04
>>>>>> -#define SUNXI_WDT_MODEADDR        (SUNXI_WDT_BASE + SUNXI_WDT_MODE)
>>>>>>   #define SUNXI_WDT_MODE_EN         (1 << 0)
>>>>>>   #define SUNXI_WDT_MODE_RST_EN     (1 << 1)
>>>>>> -static void sunxi_reset(void)
>>>>>> +static void sunxi_reset(u32 base)
>>>>>>   {
>>>>>>       void __iomem *wdt;
>>>>>> -    wdt = ioremap_nocache(SUNXI_WDT_MODEADDR & PAGE_MASK, PAGE_SIZE);
>>>>>> +    wdt = ioremap_nocache((base + SUNXI_WDT_MODE) & PAGE_MASK, PAGE_SIZE);
>>>>>>       if ( !wdt )
>>>>>>       {
>>>>>>           dprintk(XENLOG_ERR, "Unable to map watchdog register!\n");
>>>>>> @@ -42,19 +42,35 @@ static void sunxi_reset(void)
>>>>>>       /* Enable watchdog to trigger a reset after 500 ms: */
>>>>>>       writel(SUNXI_WDT_MODE_EN | SUNXI_WDT_MODE_RST_EN,
>>>>>> -      wdt + (SUNXI_WDT_MODEADDR & ~PAGE_MASK));
>>>>>> +      wdt + ((base + SUNXI_WDT_MODE) & ~PAGE_MASK));
>>>>>>       iounmap(wdt); >
>>>>>>       for (;;)
>>>>>>           wfi();
>>>>>>   }
>>>>>>
>>>>>> -static const char * const sunxi_dt_compat[] __initconst =
>>>>>> +static void sunxi_a20_reset(void)
>>>>>> +{
>>>>>> +    sunxi_reset(SUNXI_WDT_A20_BASE);
>>>>>> +}
>>>>>> +
>>>>>> +static void sunxi_h5_reset(void)
>>>>>> +{
>>>>>> +    sunxi_reset(SUNXI_WDT_H5_BASE);
>>>>>
>>>>> If I read correctly the Device-Tree for (linux/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi), the firmware is supporting PSCI 0.2.
>>>>>
>>>>> PSCI 0.2 provides call for power-off/reset, so implementation the reset callback should not be necessary.
>>>>
>>>> Yes, indeed, on the H5 PSCI 0.2 reset works via ATF.
>>>>
>>>>> Similarly the cubietrucks we have in osstest are using PSCI 0.2 and should not need the reset. Andre do you know if it is the case for all the A20?
>>>>
>>>> It claims 0.2, but in fact it seems not to be fully compliant, as (from looking at the code) U-Boot lacks the reset and poweroff calls. But it looks rather straight-forward to add them, as U-Boot knows how to reset and one would just need to wire up psci_system_reset to this.
>>>>
>>>>> For H5, I would impose PSCI 0.2 as the way to reset the platform.
>>>>
>>>> Yes.
>>>>
>>>>> I am leaning towards the same for A20 given that it would just be a matter of upgrading the bootloader. Most likely you would have already done that to get fixes.
>>>>
>>>> Not sure we should push people to upgrade U-Boot in general to be able to use Xen, but as even current mainline U-Boot doesn't seem to support it, I would rather leave the current reset support code in. Last time I checked Linux does the same.
>>>>
>>>> Cheers,
>>>> Andre.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* Re: [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i)
  2017-10-04  9:39               ` Awais Masood
@ 2017-10-04 10:03                 ` Andre Przywara
  0 siblings, 0 replies; 20+ messages in thread
From: Andre Przywara @ 2017-10-04 10:03 UTC (permalink / raw)
  To: Awais Masood, Julien Grall, xen-devel; +Cc: Stefano Stabellini

Hi,

....

>>> Since reset routine will not be required with PSCI, I assume should revert the reset code changes for this H5 patch and leave the DT retrieval for another patch that adds H3 support. Or should I try that stuff for next version of this patch? 
>>
>> Thanks for the offer, but I already made a patch that adds support for
>> basically all virtualization capable Allwinner SoCs (both v7 and v8
>> ones). This looks into the DT for ARMv7 SoCs, but relies entirely on
>> PSCI for ARMv8 SoCs. I just need to test it, then will send it out.
>>
>> So actually we won't need anything from that patch here at all, since my
>> patch supersedes it in a more generic way.
>> Do you plan on reworking/resending the UART fix (which should come
>> first, btw, as it is a prerequisite for H5 enablement)?
>>
>> You could either send the UART fix on its own if there are changes or I
>> include it as patch 1/2 of my Allwinner "series".
> 
> I'll send the latest version of UART fix as a standalone patch.

Thanks Awais, much appreciated. I don't have that other patch here at
the moment, but will send it to you for testing ASAP.

Cheers,
Andre.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* [PATCH v3] xen/ns16550: Fix ISR lockup on Allwinner uart
  2017-09-26  9:37 ` [PATCH v2 0/2] xen/arm64/ns16550: Support for Allwinner H5 SoC Awais Masood
  2017-09-26  9:37   ` [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i) Awais Masood
  2017-09-26  9:37   ` [PATCH v2 2/2] xen/ns16550: Fix ISR lockup on Designware 8250 (H5) Awais Masood
@ 2017-10-04 11:44   ` Awais Masood
  2017-10-06 14:50     ` Jan Beulich
  2017-10-10 23:59     ` Stefano Stabellini
  2 siblings, 2 replies; 20+ messages in thread
From: Awais Masood @ 2017-10-04 11:44 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
	George Dunlap, Andrew Cooper, Awais Masood, Ian Jackson,
	Tim Deegan, Jan Beulich

This patch fixes an ISR lockup seen on Allwinner uart

On Allwinner H5, serial driver goes into an infinite loop
when interrupts are enabled. The reason is a residual
"busy detect" interrupt. Since the condition UART_IIR_NOINT
will not be true unless this interrupt is cleared, the
interrupt handler will remain locked up in this while loop.

A HW quirk fix was previously added for designware uart under
commit:
50417cd978aa54930d065ac1f139f935d14af76d

It checks for a busy condition during setup and clears the
condition by reading UART_USR register.

On Allwinner hardware, the "busy detect" condition occurs
later because an LCR write is performed during setup 'after'
this clear and if uart is busy, the "busy detect" condition
will trigger again and cause the ISR lockup.

To solve this problem, the same UART_USR read operation needs
to be performed within the interrupt handler to clear this
condition.

Linux dw 8250 driver also handles this condition within
interrupt handler
http://elixir.free-electrons.com/linux/latest/source/drivers/tty/serial/8250/8250_dw.c#L233

Tested on Orange Pi PC2 (H5). This issue is seen on H3
as well and the same fix works.

Signed-off-by: Awais Masood <awais.masood@vadion.com>

---

CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>

Changes since v2
 - Updated comments to clarify that fix is for Allwinner hardware
 - Removed ns16550 prefix from local function

Changes since v1
 - Common quirk fix code moved to a helper function
 - Patch description improved with earlier commit link
---
 xen/drivers/char/ns16550.c | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 6ab5ec3..e0f8199 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -505,6 +505,23 @@ static int ns16550_ioport_invalid(struct ns16550 *uart)
     return ns_read_reg(uart, UART_IER) == 0xff;
 }
 
+static void handle_dw_usr_busy_quirk(struct ns16550 *uart)
+{
+    if ( uart->dw_usr_bsy &&
+         (ns_read_reg(uart, UART_IIR) & UART_IIR_BSY) == UART_IIR_BSY )
+    {
+        /* DesignWare 8250 detects if LCR is written while the UART is
+         * busy and raises a "busy detect" interrupt. Read the UART
+         * Status Register to clear this state.
+         *
+         * Allwinner/sunxi UART hardware is similar to DesignWare 8250
+         * and also contains a "busy detect" interrupt. So this quirk
+         * fix will also be used for Allwinner UART.
+         */
+        ns_read_reg(uart, UART_USR);
+    }
+}
+
 static void ns16550_interrupt(
     int irq, void *dev_id, struct cpu_user_regs *regs)
 {
@@ -521,6 +538,16 @@ static void ns16550_interrupt(
             serial_tx_interrupt(port, regs);
         if ( lsr & UART_LSR_DR )
             serial_rx_interrupt(port, regs);
+
+        /* A "busy-detect" condition is observed on Allwinner/sunxi UART
+         * after LCR is written during setup. It needs to be cleared at
+         * this point or UART_IIR_NOINT will never be set and this loop
+         * will continue forever.
+         *
+         * This state can be cleared by calling the dw_usr_busy quirk
+         * handler that resolves "busy-detect" for  DesignWare uart.
+         */
+        handle_dw_usr_busy_quirk(uart);
     }
 }
 
@@ -623,15 +650,8 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
     /* No interrupts. */
     ns_write_reg(uart, UART_IER, 0);
 
-    if ( uart->dw_usr_bsy &&
-         (ns_read_reg(uart, UART_IIR) & UART_IIR_BSY) == UART_IIR_BSY )
-    {
-        /* DesignWare 8250 detects if LCR is written while the UART is
-         * busy and raises a "busy detect" interrupt. Read the UART
-         * Status Register to clear this state.
-         */
-        ns_read_reg(uart, UART_USR);
-    }
+    /* Handle the DesignWare 8250 'busy-detect' quirk. */
+    handle_dw_usr_busy_quirk(uart);
 
     /* Line control and baud-rate generator. */
     ns_write_reg(uart, UART_LCR, lcr | UART_LCR_DLAB);
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* Re: [PATCH v3] xen/ns16550: Fix ISR lockup on Allwinner uart
  2017-10-04 11:44   ` [PATCH v3] xen/ns16550: Fix ISR lockup on Allwinner uart Awais Masood
@ 2017-10-06 14:50     ` Jan Beulich
  2017-10-10 23:59     ` Stefano Stabellini
  1 sibling, 0 replies; 20+ messages in thread
From: Jan Beulich @ 2017-10-06 14:50 UTC (permalink / raw)
  To: Awais Masood
  Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
	George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel

>>> On 04.10.17 at 13:44, <awais.masood@vadion.com> wrote:
> This patch fixes an ISR lockup seen on Allwinner uart
> 
> On Allwinner H5, serial driver goes into an infinite loop
> when interrupts are enabled. The reason is a residual
> "busy detect" interrupt. Since the condition UART_IIR_NOINT
> will not be true unless this interrupt is cleared, the
> interrupt handler will remain locked up in this while loop.
> 
> A HW quirk fix was previously added for designware uart under
> commit:
> 50417cd978aa54930d065ac1f139f935d14af76d
> 
> It checks for a busy condition during setup and clears the
> condition by reading UART_USR register.
> 
> On Allwinner hardware, the "busy detect" condition occurs
> later because an LCR write is performed during setup 'after'
> this clear and if uart is busy, the "busy detect" condition
> will trigger again and cause the ISR lockup.
> 
> To solve this problem, the same UART_USR read operation needs
> to be performed within the interrupt handler to clear this
> condition.
> 
> Linux dw 8250 driver also handles this condition within
> interrupt handler
> http://elixir.free-electrons.com/linux/latest/source/drivers/tty/serial/8250/ 
> 8250_dw.c#L233
> 
> Tested on Orange Pi PC2 (H5). This issue is seen on H3
> as well and the same fix works.
> 
> Signed-off-by: Awais Masood <awais.masood@vadion.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

* Re: [PATCH v3] xen/ns16550: Fix ISR lockup on Allwinner uart
  2017-10-04 11:44   ` [PATCH v3] xen/ns16550: Fix ISR lockup on Allwinner uart Awais Masood
  2017-10-06 14:50     ` Jan Beulich
@ 2017-10-10 23:59     ` Stefano Stabellini
  1 sibling, 0 replies; 20+ messages in thread
From: Stefano Stabellini @ 2017-10-10 23:59 UTC (permalink / raw)
  To: Awais Masood
  Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
	George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
	Jan Beulich, xen-devel

On Wed, 4 Oct 2017, Awais Masood wrote:
> This patch fixes an ISR lockup seen on Allwinner uart
> 
> On Allwinner H5, serial driver goes into an infinite loop
> when interrupts are enabled. The reason is a residual
> "busy detect" interrupt. Since the condition UART_IIR_NOINT
> will not be true unless this interrupt is cleared, the
> interrupt handler will remain locked up in this while loop.
> 
> A HW quirk fix was previously added for designware uart under
> commit:
> 50417cd978aa54930d065ac1f139f935d14af76d
> 
> It checks for a busy condition during setup and clears the
> condition by reading UART_USR register.
> 
> On Allwinner hardware, the "busy detect" condition occurs
> later because an LCR write is performed during setup 'after'
> this clear and if uart is busy, the "busy detect" condition
> will trigger again and cause the ISR lockup.
> 
> To solve this problem, the same UART_USR read operation needs
> to be performed within the interrupt handler to clear this
> condition.
> 
> Linux dw 8250 driver also handles this condition within
> interrupt handler
> http://elixir.free-electrons.com/linux/latest/source/drivers/tty/serial/8250/8250_dw.c#L233
> 
> Tested on Orange Pi PC2 (H5). This issue is seen on H3
> as well and the same fix works.
> 
> Signed-off-by: Awais Masood <awais.masood@vadion.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Tim Deegan <tim@xen.org>
> CC: Wei Liu <wei.liu2@citrix.com>
> 
> Changes since v2
>  - Updated comments to clarify that fix is for Allwinner hardware
>  - Removed ns16550 prefix from local function
> 
> Changes since v1
>  - Common quirk fix code moved to a helper function
>  - Patch description improved with earlier commit link
> ---
>  xen/drivers/char/ns16550.c | 38 +++++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 6ab5ec3..e0f8199 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -505,6 +505,23 @@ static int ns16550_ioport_invalid(struct ns16550 *uart)
>      return ns_read_reg(uart, UART_IER) == 0xff;
>  }
>  
> +static void handle_dw_usr_busy_quirk(struct ns16550 *uart)
> +{
> +    if ( uart->dw_usr_bsy &&
> +         (ns_read_reg(uart, UART_IIR) & UART_IIR_BSY) == UART_IIR_BSY )
> +    {
> +        /* DesignWare 8250 detects if LCR is written while the UART is
> +         * busy and raises a "busy detect" interrupt. Read the UART
> +         * Status Register to clear this state.
> +         *
> +         * Allwinner/sunxi UART hardware is similar to DesignWare 8250
> +         * and also contains a "busy detect" interrupt. So this quirk
> +         * fix will also be used for Allwinner UART.
> +         */
> +        ns_read_reg(uart, UART_USR);
> +    }
> +}
> +
>  static void ns16550_interrupt(
>      int irq, void *dev_id, struct cpu_user_regs *regs)
>  {
> @@ -521,6 +538,16 @@ static void ns16550_interrupt(
>              serial_tx_interrupt(port, regs);
>          if ( lsr & UART_LSR_DR )
>              serial_rx_interrupt(port, regs);
> +
> +        /* A "busy-detect" condition is observed on Allwinner/sunxi UART
> +         * after LCR is written during setup. It needs to be cleared at
> +         * this point or UART_IIR_NOINT will never be set and this loop
> +         * will continue forever.
> +         *
> +         * This state can be cleared by calling the dw_usr_busy quirk
> +         * handler that resolves "busy-detect" for  DesignWare uart.
> +         */
> +        handle_dw_usr_busy_quirk(uart);
>      }
>  }
>  
> @@ -623,15 +650,8 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
>      /* No interrupts. */
>      ns_write_reg(uart, UART_IER, 0);
>  
> -    if ( uart->dw_usr_bsy &&
> -         (ns_read_reg(uart, UART_IIR) & UART_IIR_BSY) == UART_IIR_BSY )
> -    {
> -        /* DesignWare 8250 detects if LCR is written while the UART is
> -         * busy and raises a "busy detect" interrupt. Read the UART
> -         * Status Register to clear this state.
> -         */
> -        ns_read_reg(uart, UART_USR);
> -    }
> +    /* Handle the DesignWare 8250 'busy-detect' quirk. */
> +    handle_dw_usr_busy_quirk(uart);
>  
>      /* Line control and baud-rate generator. */
>      ns_write_reg(uart, UART_LCR, lcr | UART_LCR_DLAB);
> -- 
> 2.7.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

end of thread, other threads:[~2017-10-10 23:59 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-19 13:38 [PATCH 0/2] xen/arm64/ns16550: Support for Allwinner H5 SoC Awais Masood
2017-09-19 13:38 ` [PATCH 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i) Awais Masood
2017-09-19 13:50   ` Jan Beulich
2017-09-19 13:38 ` [PATCH 2/2] xen/ns16550: Fix ISR lockup on Designware 8250 (H5) Awais Masood
2017-09-19 13:49   ` Jan Beulich
2017-09-26  9:37 ` [PATCH v2 0/2] xen/arm64/ns16550: Support for Allwinner H5 SoC Awais Masood
2017-09-26  9:37   ` [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i) Awais Masood
2017-09-28 20:03     ` Julien Grall
2017-09-28 22:49       ` Andre Przywara
2017-09-29 16:35         ` Andre Przywara
2017-10-04  9:16           ` Awais Masood
2017-10-04  9:26             ` Andre Przywara
2017-10-04  9:39               ` Awais Masood
2017-10-04 10:03                 ` Andre Przywara
2017-10-03 11:32         ` Julien Grall
2017-09-26  9:37   ` [PATCH v2 2/2] xen/ns16550: Fix ISR lockup on Designware 8250 (H5) Awais Masood
2017-09-26 11:58     ` Jan Beulich
2017-10-04 11:44   ` [PATCH v3] xen/ns16550: Fix ISR lockup on Allwinner uart Awais Masood
2017-10-06 14:50     ` Jan Beulich
2017-10-10 23:59     ` Stefano Stabellini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.