<div dir="auto">Update: I just learned from Aspeed that the SIRQ polarity is actually dependent on the interface mode, LPC vs. eSPI.<div dir="auto"><br></div><div dir="auto">For LPC, the polarity should be set to 1, for eSPI the default of 0 is correct.</div><div dir="auto"><br></div><div dir="auto">I'll amend the patch to read the interface mode from the HW pin strap register and day the default accordingly at driver load time.</div><div dir="auto"><br></div><div dir="auto">If there are no objections, I'll leave the sysfs part in there in case it does need to be changed.</div><div dir="auto"><br></div><div dir="auto">Oskar.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Jul 28, 2019, 12:00 AM Oskar Senft <<a href="mailto:osk@google.com">osk@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I was thinking exactly the same thing (which is why I pointed it out in the change description). Thanks for picking up on that :-D<div><br></div><div>I considered both options but decided to follow what's been done for the sirq and lpc_address settings, as sirq_polarity should really go together with the other two. I guess we could argue that the sirq_polarity likely always has to have a specific setting for the specific platform, whereas the sirq and the lpc_address might be configurable in some way from the host at runtime.<div><br></div><div>Another reason I decided against DTS is that the properties currently read from DTS are "standard properties" from the 8250 driver. So I wasn't sure if it's ok to add a property that clearly specific to the 8250_aspeed_vuart driver.</div><div><br></div><div>If anyone has strong feelings I can either change it from sysfs to DTS or add DTS on top - it's quite easy to do. Thoughts?</div><div><div><br></div><div>Thanks</div><div>Oskar.</div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Jul 27, 2019 at 9:30 PM Benjamin Herrenschmidt <<a href="mailto:benh@kernel.crashing.org" target="_blank" rel="noreferrer">benh@kernel.crashing.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Sat, 2019-07-27 at 09:42 -0400, Oskar Senft wrote:<br>
> Make the SIRQ polarity for Aspeed 24xx/25xx VUART configurable via<br>
> sysfs. It is required on some host platforms (e.g. TYAN S7106) to<br>
> reconfigure this setting from the default to enable the host to<br>
> receive<br>
> interrupts from the VUART.<br>
> <br>
> The setting is configurable via sysfs rather than device-tree to stay<br>
> in<br>
> line with other related configurable settings.<br>
<br>
If it's a fixed platform configuration that never changes at runtime,<br>
shouldn't it be in the device-tree instead ?<br>
<br>
Cheers,<br>
Ben<br>
<br>
> Tested: Verified on TYAN S7106 mainboard.<br>
> Signed-off-by: Oskar Senft <<a href="mailto:osk@google.com" target="_blank" rel="noreferrer">osk@google.com</a>><br>
> ---<br>
> .../ABI/stable/sysfs-driver-aspeed-vuart | 10 ++++-<br>
> drivers/tty/serial/8250/8250_aspeed_vuart.c | 39<br>
> +++++++++++++++++++<br>
> 2 files changed, 48 insertions(+), 1 deletion(-)<br>
> <br>
> diff --git a/Documentation/ABI/stable/sysfs-driver-aspeed-vuart<br>
> b/Documentation/ABI/stable/sysfs-driver-aspeed-vuart<br>
> index 8062953ce77b..64fad87ad964 100644<br>
> --- a/Documentation/ABI/stable/sysfs-driver-aspeed-vuart<br>
> +++ b/Documentation/ABI/stable/sysfs-driver-aspeed-vuart<br>
> @@ -6,10 +6,18 @@ Description: Configures which IO port the<br>
> host side of the UART<br>
> Users: OpenBMC. Proposed changes should be mailed to<br>
> <a href="mailto:openbmc@lists.ozlabs.org" target="_blank" rel="noreferrer">openbmc@lists.ozlabs.org</a><br>
> <br>
> -What: /sys/bus/platform/drivers/aspeed-vuart*/sirq<br>
> +What: /sys/bus/platform/drivers/aspeed-vuart/*/sirq<br>
> Date: April 2017<br>
> Contact: Jeremy Kerr <<a href="mailto:jk@ozlabs.org" target="_blank" rel="noreferrer">jk@ozlabs.org</a>><br>
> Description: Configures which interrupt number the host side of<br>
> the UART will appear on the host <-> BMC LPC bus.<br>
> Users: OpenBMC. Proposed changes should be mailed to<br>
> <a href="mailto:openbmc@lists.ozlabs.org" target="_blank" rel="noreferrer">openbmc@lists.ozlabs.org</a><br>
> +<br>
> +What: /sys/bus/platform/drivers/aspeed-<br>
> vuart/*/sirq_polarity<br>
> +Date: July 2019<br>
> +Contact: Oskar Senft <<a href="mailto:osk@google.com" target="_blank" rel="noreferrer">osk@google.com</a>><br>
> +Description: Configures the polarity of the serial interrupt to the<br>
> + host via the BMC LPC bus.<br>
> +Users: OpenBMC. Proposed changes should be mailed to<br>
> + <a href="mailto:openbmc@lists.ozlabs.org" target="_blank" rel="noreferrer">openbmc@lists.ozlabs.org</a><br>
> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c<br>
> b/drivers/tty/serial/8250/8250_aspeed_vuart.c<br>
> index 0438d9a905ce..ef0a6ff69841 100644<br>
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c<br>
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c<br>
> @@ -22,6 +22,7 @@<br>
> <br>
> #define ASPEED_VUART_GCRA 0x20<br>
> #define ASPEED_VUART_GCRA_VUART_EN BIT(0)<br>
> +#define ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY BIT(1)<br>
> #define ASPEED_VUART_GCRA_DISABLE_HOST_TX_DISCARD BIT(5)<br>
> #define ASPEED_VUART_GCRB 0x24<br>
> #define ASPEED_VUART_GCRB_HOST_SIRQ_MASK GENMASK(7, 4)<br>
> @@ -131,8 +132,46 @@ static ssize_t sirq_store(struct device *dev,<br>
> struct device_attribute *attr,<br>
> <br>
> static DEVICE_ATTR_RW(sirq);<br>
> <br>
> +static ssize_t sirq_polarity_show(struct device *dev,<br>
> + struct device_attribute *attr, char<br>
> *buf)<br>
> +{<br>
> + struct aspeed_vuart *vuart = dev_get_drvdata(dev);<br>
> + u8 reg;<br>
> +<br>
> + reg = readb(vuart->regs + ASPEED_VUART_GCRA);<br>
> + reg &= ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;<br>
> +<br>
> + return snprintf(buf, PAGE_SIZE - 1, "%u\n", reg ? 1 : 0);<br>
> +}<br>
> +<br>
> +static ssize_t sirq_polarity_store(struct device *dev,<br>
> + struct device_attribute *attr,<br>
> + const char *buf, size_t count)<br>
> +{<br>
> + struct aspeed_vuart *vuart = dev_get_drvdata(dev);<br>
> + unsigned long val;<br>
> + int err;<br>
> + u8 reg;<br>
> +<br>
> + err = kstrtoul(buf, 0, &val);<br>
> + if (err)<br>
> + return err;<br>
> +<br>
> + reg = readb(vuart->regs + ASPEED_VUART_GCRA);<br>
> + if (val != 0)<br>
> + reg |= ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;<br>
> + else<br>
> + reg &= ~ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;<br>
> + writeb(reg, vuart->regs + ASPEED_VUART_GCRA);<br>
> +<br>
> + return count;<br>
> +}<br>
> +<br>
> +static DEVICE_ATTR_RW(sirq_polarity);<br>
> +<br>
> static struct attribute *aspeed_vuart_attrs[] = {<br>
> &dev_attr_sirq.attr,<br>
> + &dev_attr_sirq_polarity.attr,<br>
> &dev_attr_lpc_address.attr,<br>
> NULL,<br>
> };<br>
<br>
</blockquote></div>
</blockquote></div>