| Message ID | 20190731013404.243755-1-osk@google.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2,1/3] drivers/tty/serial/8250: Make Aspeed VUART SIRQ polarity configurable | expand |
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
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 > >
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-
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(-)