[PATCH v2 1/3] drivers/tty/serial/8250: Make Aspeed VUART SIRQ polarity configurable

[PATCH v2 1/3] drivers/tty/serial/8250: Make Aspeed VUART SIRQ polarity configurable

[Oskar Senft]

Make the SIRQ polarity for Aspeed AST24xx/25xx VUART configurable via
sysfs. This setting need to be changed on specific host platforms
depending on the selected host interface (LPC / eSPI).

The setting is configurable via sysfs rather than device-tree to stay in
line with other related configurable settings.

On AST2500 the VUART SIRQ polarity can be auto-configured by reading the
LPC/eSPI interface configuration from the HW strap table.

Tested: Verified on TYAN S7106 mainboard.
Signed-off-by: Oskar Senft <osk@google.com>
---
 .../ABI/stable/sysfs-driver-aspeed-vuart      | 10 ++-
 drivers/tty/serial/8250/8250_aspeed_vuart.c   | 82 +++++++++++++++++++
 drivers/tty/serial/8250/Kconfig               |  1 +
 3 files changed, 92 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/stable/sysfs-driver-aspeed-vuart b/Documentation/ABI/stable/sysfs-driver-aspeed-vuart
index 8062953ce77b..64fad87ad964 100644
--- a/Documentation/ABI/stable/sysfs-driver-aspeed-vuart
+++ b/Documentation/ABI/stable/sysfs-driver-aspeed-vuart
@@ -6,10 +6,18 @@ Description:	Configures which IO port the host side of the UART
 Users:		OpenBMC.  Proposed changes should be mailed to
 		openbmc@lists.ozlabs.org
 
-What:		/sys/bus/platform/drivers/aspeed-vuart*/sirq
+What:		/sys/bus/platform/drivers/aspeed-vuart/*/sirq
 Date:		April 2017
 Contact:	Jeremy Kerr <jk@ozlabs.org>
 Description:	Configures which interrupt number the host side of
 		the UART will appear on the host <-> BMC LPC bus.
 Users:		OpenBMC.  Proposed changes should be mailed to
 		openbmc@lists.ozlabs.org
+
+What:		/sys/bus/platform/drivers/aspeed-vuart/*/sirq_polarity
+Date:		July 2019
+Contact:	Oskar Senft <osk@google.com>
+Description:	Configures the polarity of the serial interrupt to the
+		host via the BMC LPC bus.
+Users:		OpenBMC.  Proposed changes should be mailed to
+		openbmc@lists.ozlabs.org
diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index 0438d9a905ce..e0e441c38541 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -14,6 +14,8 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
 #include <linux/clk.h>
@@ -22,6 +24,7 @@
 
 #define ASPEED_VUART_GCRA		0x20
 #define ASPEED_VUART_GCRA_VUART_EN		BIT(0)
+#define ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY	BIT(1)
 #define ASPEED_VUART_GCRA_DISABLE_HOST_TX_DISCARD BIT(5)
 #define ASPEED_VUART_GCRB		0x24
 #define ASPEED_VUART_GCRB_HOST_SIRQ_MASK	GENMASK(7, 4)
@@ -131,8 +134,53 @@ static ssize_t sirq_store(struct device *dev, struct device_attribute *attr,
 
 static DEVICE_ATTR_RW(sirq);
 
+static ssize_t sirq_polarity_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct aspeed_vuart *vuart = dev_get_drvdata(dev);
+	u8 reg;
+
+	reg = readb(vuart->regs + ASPEED_VUART_GCRA);
+	reg &= ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;
+
+	return snprintf(buf, PAGE_SIZE - 1, "%u\n", reg ? 1 : 0);
+}
+
+static void aspeed_vuart_set_sirq_polarity(struct aspeed_vuart *vuart,
+					   bool polarity)
+{
+	u8 reg = readb(vuart->regs + ASPEED_VUART_GCRA);
+
+	if (polarity)
+		reg |= ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;
+	else
+		reg &= ~ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;
+
+	writeb(reg, vuart->regs + ASPEED_VUART_GCRA);
+}
+
+static ssize_t sirq_polarity_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct aspeed_vuart *vuart = dev_get_drvdata(dev);
+	unsigned long val;
+	int err;
+
+	err = kstrtoul(buf, 0, &val);
+	if (err)
+		return err;
+
+	aspeed_vuart_set_sirq_polarity(vuart, val != 0);
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(sirq_polarity);
+
 static struct attribute *aspeed_vuart_attrs[] = {
 	&dev_attr_sirq.attr,
+	&dev_attr_sirq_polarity.attr,
 	&dev_attr_lpc_address.attr,
 	NULL,
 };
@@ -302,6 +350,27 @@ static int aspeed_vuart_handle_irq(struct uart_port *port)
 	return 1;
 }
 
+static void aspeed_vuart_auto_configure_sirq_polarity(
+	struct aspeed_vuart *vuart, struct device_node *syscon_np,
+	u32 reg_offset, u32 reg_mask)
+{
+	struct regmap *regmap;
+	u32 value;
+
+	regmap = syscon_node_to_regmap(syscon_np);
+	if (IS_ERR(regmap)) {
+		dev_warn(vuart->dev,
+			 "could not get regmap for espi-enabled\n");
+		return;
+	}
+	if (regmap_read(regmap, reg_offset, &value)) {
+		dev_warn(vuart->dev, "could not read hw strap table\n");
+		return;
+	}
+
+	aspeed_vuart_set_sirq_polarity(vuart, (value & reg_mask) == 0);
+}
+
 static int aspeed_vuart_probe(struct platform_device *pdev)
 {
 	struct uart_8250_port port;
@@ -310,6 +379,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 	struct resource *res;
 	u32 clk, prop;
 	int rc;
+	struct of_phandle_args espi_enabled_args;
 
 	np = pdev->dev.of_node;
 
@@ -402,6 +472,18 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 
 	vuart->line = rc;
 
+	rc = of_parse_phandle_with_fixed_args(
+		np, "espi-enabled", 2, 0, &espi_enabled_args);
+	if (rc < 0) {
+		dev_warn(&pdev->dev, "espi-enabled property not found\n");
+	} else {
+		aspeed_vuart_auto_configure_sirq_polarity(
+			vuart, espi_enabled_args.np,
+			espi_enabled_args.args[0],
+			BIT(espi_enabled_args.args[1]));
+		of_node_put(espi_enabled_args.np);
+	}
+
 	aspeed_vuart_set_enabled(vuart, true);
 	aspeed_vuart_set_host_tx_discard(vuart, true);
 	platform_set_drvdata(pdev, vuart);
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 509f6a3bb9ff..98e25781a293 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -243,6 +243,7 @@ config SERIAL_8250_ASPEED_VUART
 	tristate "Aspeed Virtual UART"
 	depends on SERIAL_8250
 	depends on OF
+	depends on REGMAP && MFD_SYSCON
 	help
 	  If you want to use the virtual UART (VUART) device on Aspeed
 	  BMC platforms, enable this option. This enables the 16550A-
-- 
2.22.0.709.g102302147b-goog

[PATCH v2 2/3] dt-bindings: serial: 8250: Add documentation for espi-enabled.

[Oskar Senft]

Add documentation for 8250_aspeed_vuart's espi-enabled property that
enables to auto-configure the VUART's SIRQ polarity.

Signed-off-by: Oskar Senft <osk@google.com>
---
 Documentation/devicetree/bindings/serial/8250.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt
index 20d351f268ef..4b8b9e502179 100644
--- a/Documentation/devicetree/bindings/serial/8250.txt
+++ b/Documentation/devicetree/bindings/serial/8250.txt
@@ -56,6 +56,11 @@ Optional properties:
 - {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
   line respectively. It will use specified GPIO instead of the peripheral
   function pin for the UART feature. If unsure, don't specify this property.
+- espi-enabled: Only applicable to aspeed,ast2500-vuart. Value is a phandle to
+  aspeed,ast2500-scu syscon alongside register offset and bit number to
+  identify whether the system is in eSPI mode. This is used to auto-configure
+  SIRQ polarity on the vuart.
+  Example: espi-enabled = <&syscon 0x70 25>
 
 Note:
 * fsl,ns16550:
-- 
2.22.0.709.g102302147b-goog

[PATCH v2 3/3] arm: dts: aspeed: Add vuart espi-enabled to aspeed-g5.dtsi

[Oskar Senft]

Enable auto-configuration of VUART SIRQ polarity on AST2500.

Signed-off-by: Oskar Senft <osk@google.com>
---
 arch/arm/boot/dts/aspeed-g5.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index 01ffe70f64d5..9e561504042a 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -351,6 +351,7 @@
 				interrupts = <8>;
 				clocks = <&syscon ASPEED_CLK_APB>;
 				no-loopback-test;
+				espi-enabled = <&syscon 0x70 25>;
 				status = "disabled";
 			};
 
-- 
2.22.0.709.g102302147b-goog

Re: [PATCH v2 2/3] dt-bindings: serial: 8250: Add documentation for espi-enabled.

[Andrew Jeffery]

Hi Oskar,

On Wed, 31 Jul 2019, at 11:04, Oskar Senft wrote:
> Add documentation for 8250_aspeed_vuart's espi-enabled property that
> enables to auto-configure the VUART's SIRQ polarity.
> 
> Signed-off-by: Oskar Senft <osk@google.com>
> ---
>  Documentation/devicetree/bindings/serial/8250.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/8250.txt 
> b/Documentation/devicetree/bindings/serial/8250.txt
> index 20d351f268ef..4b8b9e502179 100644
> --- a/Documentation/devicetree/bindings/serial/8250.txt
> +++ b/Documentation/devicetree/bindings/serial/8250.txt
> @@ -56,6 +56,11 @@ Optional properties:
>  - {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for 
> RTS/CTS/DTR/DSR/RI/DCD
>    line respectively. It will use specified GPIO instead of the 
> peripheral
>    function pin for the UART feature. If unsure, don't specify this 
> property.
> +- espi-enabled: Only applicable to aspeed,ast2500-vuart.

Bit of a bikeshed, but:

Given it's ASPEED-specific I expect you should use a vendor prefix for the
property, e.g. aspeed,espi-enabled.

However, as I understand it you want to determine what polarity the SIRQ
should be regardless of which of eSPI or LPC are enabled, so I don't think
the property name should be an explicit statement about eSPI. Maybe
"aspeed,sirq-polarity-sense"? Anyway, see the point below:

Please use ./scripts/get_maintainer.pl to determine where to send the
series - Copying just the linux-aspeed@ list for upstream patches is not
enough. For instance the series needs to at least go via the linux-serial@
list given that's the affected subsystem, and you're adding to the
devicetree binding so you need to send to the devicetree@ list as well
(you'll need an ack from Rob Herring). The get_maintainer.pl script will
give you all the information you need.

Cheers,

Andrew

Re: [PATCH v2 1/3] drivers/tty/serial/8250: Make Aspeed VUART SIRQ polarity configurable

[Jeremy Kerr]

Hi Oskar,

Looks good to me, some minor comments though:

> +
> +What:		/sys/bus/platform/drivers/aspeed-vuart/*/sirq_polarity
> +Date:		July 2019
> +Contact:	Oskar Senft <osk@google.com>
> +Description:	Configures the polarity of the serial interrupt to the
> +		host via the BMC LPC bus.

Can you mention what the value represents? 1/0 don't really indicate a
specific polarity.

Alternatively, we could use descriptive values (say, "active-low" /
"idle-low").

> @@ -310,6 +379,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	u32 clk, prop;
>  	int rc;
> +	struct of_phandle_args espi_enabled_args;

Minor: can you reverse-christmas-tree this?

> @@ -402,6 +472,18 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
>  
>  	vuart->line = rc;
>  
> +	rc = of_parse_phandle_with_fixed_args(
> +		np, "espi-enabled", 2, 0, &espi_enabled_args);
> +	if (rc < 0) {
> +		dev_warn(&pdev->dev, "espi-enabled property not found\n");

In the binding spec, you've listed this property at optional, but here
we dev_warn() if its not present. Can we default to existing behaviour
if it's not there?

That may just be a matter of changing this to dev_debug.

Cheers,


Jeremy

Re: [PATCH v2 2/3] dt-bindings: serial: 8250: Add documentation for espi-enabled.

[Jeremy Kerr]

Hi Oskar,

> Given it's ASPEED-specific I expect you should use a vendor prefix for the
> property, e.g. aspeed,espi-enabled.
> 
> However, as I understand it you want to determine what polarity the SIRQ
> should be regardless of which of eSPI or LPC are enabled, so I don't think
> the property name should be an explicit statement about eSPI. Maybe
> "aspeed,sirq-polarity-sense"?

Yep, +1 on Andrew's comments here. This property isn't an indication on
whether espi is enabled, but a method to detect it.

Cheers,


Jeremy

Re: [PATCH v2 1/3] drivers/tty/serial/8250: Make Aspeed VUART SIRQ polarity configurable

[Oskar Senft]

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

Hi Jeremy

Thanks for your comments, they were really helpful!


> > +What:
> /sys/bus/platform/drivers/aspeed-vuart/*/sirq_polarity
> > +Date:                July 2019
> > +Contact:     Oskar Senft <osk@google.com>
> > +Description: Configures the polarity of the serial interrupt to the
> > +             host via the BMC LPC bus.
>
> Can you mention what the value represents? 1/0 don't really indicate a
> specific polarity.
>
Good point. Not sure why I didn't do that initially.


> > @@ -310,6 +379,7 @@ static int aspeed_vuart_probe(struct platform_device
> *pdev)
> >       struct resource *res;
> >       u32 clk, prop;
> >       int rc;
> > +     struct of_phandle_args espi_enabled_args;
> Minor: can you reverse-christmas-tree this?
>
Oops, yeah. Sorry! Conflicting coding styles in my head got confused.


> > +     rc = of_parse_phandle_with_fixed_args(
> > +             np, "espi-enabled", 2, 0, &espi_enabled_args);
> > +     if (rc < 0) {
> > +             dev_warn(&pdev->dev, "espi-enabled property not found\n");
> That may just be a matter of changing this to dev_debug.
>
That was my intent, sorry for that.

I'll send v3 with those changes in a few minutes.

Thanks
Oskar.


On Wed, Sep 4, 2019 at 9:14 PM Jeremy Kerr <jk@ozlabs.org> wrote:

> Hi Oskar,
>
> Looks good to me, some minor comments though:
>
> > +
> > +What:
> /sys/bus/platform/drivers/aspeed-vuart/*/sirq_polarity
> > +Date:                July 2019
> > +Contact:     Oskar Senft <osk@google.com>
> > +Description: Configures the polarity of the serial interrupt to the
> > +             host via the BMC LPC bus.
>
> Can you mention what the value represents? 1/0 don't really indicate a
> specific polarity.
>
> Alternatively, we could use descriptive values (say, "active-low" /
> "idle-low").
>
> > @@ -310,6 +379,7 @@ static int aspeed_vuart_probe(struct platform_device
> *pdev)
> >       struct resource *res;
> >       u32 clk, prop;
> >       int rc;
> > +     struct of_phandle_args espi_enabled_args;
>
> Minor: can you reverse-christmas-tree this?
>
> > @@ -402,6 +472,18 @@ static int aspeed_vuart_probe(struct
> platform_device *pdev)
> >
> >       vuart->line = rc;
> >
> > +     rc = of_parse_phandle_with_fixed_args(
> > +             np, "espi-enabled", 2, 0, &espi_enabled_args);
> > +     if (rc < 0) {
> > +             dev_warn(&pdev->dev, "espi-enabled property not found\n");
>
> In the binding spec, you've listed this property at optional, but here
> we dev_warn() if its not present. Can we default to existing behaviour
> if it's not there?
>
> That may just be a matter of changing this to dev_debug.
>
> Cheers,
>
>
> Jeremy
>
>

[-- Attachment #2: Type: text/html, Size: 4218 bytes --]

Re: [PATCH v2 2/3] dt-bindings: serial: 8250: Add documentation for espi-enabled.

[Oskar Senft]

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

Hi Andrew and Jeremy

Thanks for both your input, that was helpful. I'm sorry it took me so long
to get back to this. I hope it's in better shape now.

> Given it's ASPEED-specific I expect you should use a vendor prefix for the
> > property, e.g. aspeed,espi-enabled.
>
That was a very good point.


> > However, as I understand it you want to determine what polarity the SIRQ
> > should be regardless of which of eSPI or LPC are enabled, so I don't
> think
> > the property name should be an explicit statement about eSPI. Maybe
> > "aspeed,sirq-polarity-sense"?
>
> Yep, +1 on Andrew's comments here. This property isn't an indication on
> whether espi is enabled, but a method to detect it.
>
I agree. I was so focused on functionality that I didn't look at this with
a wider view.

I'll send v3 of the patch that contains appropriate changes. I'll also
include further lists and individuals to get OKs as needed.

Thanks
Oskar.

[-- Attachment #2: Type: text/html, Size: 1486 bytes --]