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

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

[Oskar Senft]

Make the SIRQ polarity for Aspeed 24xx/25xx VUART configurable via
sysfs. It is required on some host platforms (e.g. TYAN S7106) to
reconfigure this setting from the default to enable the host to receive
interrupts from the VUART.

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

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   | 39 +++++++++++++++++++
 2 files changed, 48 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 at 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 at 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 at 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..ef0a6ff69841 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -22,6 +22,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 +132,46 @@ 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 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;
+	u8 reg;
+
+	err = kstrtoul(buf, 0, &val);
+	if (err)
+		return err;
+
+	reg = readb(vuart->regs + ASPEED_VUART_GCRA);
+	if (val != 0)
+		reg |= ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;
+	else
+		reg &= ~ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;
+	writeb(reg, vuart->regs + ASPEED_VUART_GCRA);
+
+	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,
 };
-- 
2.22.0.709.g102302147b-goog

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

[Benjamin Herrenschmidt]

On Sat, 2019-07-27 at 09:42 -0400, Oskar Senft wrote:
> Make the SIRQ polarity for Aspeed 24xx/25xx VUART configurable via
> sysfs. It is required on some host platforms (e.g. TYAN S7106) to
> reconfigure this setting from the default to enable the host to
> receive
> interrupts from the VUART.
> 
> The setting is configurable via sysfs rather than device-tree to stay
> in
> line with other related configurable settings.

If it's a fixed platform configuration that never changes at runtime,
shouldn't it be in the device-tree instead ?

Cheers,
Ben

> 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   | 39
> +++++++++++++++++++
>  2 files changed, 48 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 at 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 at 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 at 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..ef0a6ff69841 100644
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> @@ -22,6 +22,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 +132,46 @@ 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 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;
> +	u8 reg;
> +
> +	err = kstrtoul(buf, 0, &val);
> +	if (err)
> +		return err;
> +
> +	reg = readb(vuart->regs + ASPEED_VUART_GCRA);
> +	if (val != 0)
> +		reg |= ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;
> +	else
> +		reg &= ~ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;
> +	writeb(reg, vuart->regs + ASPEED_VUART_GCRA);
> +
> +	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,
>  };

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

[Oskar Senft]

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

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.

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.

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?

Thanks
Oskar.

On Sat, Jul 27, 2019 at 9:30 PM Benjamin Herrenschmidt <
benh@kernel.crashing.org> wrote:

> On Sat, 2019-07-27 at 09:42 -0400, Oskar Senft wrote:
> > Make the SIRQ polarity for Aspeed 24xx/25xx VUART configurable via
> > sysfs. It is required on some host platforms (e.g. TYAN S7106) to
> > reconfigure this setting from the default to enable the host to
> > receive
> > interrupts from the VUART.
> >
> > The setting is configurable via sysfs rather than device-tree to stay
> > in
> > line with other related configurable settings.
>
> If it's a fixed platform configuration that never changes at runtime,
> shouldn't it be in the device-tree instead ?
>
> Cheers,
> Ben
>
> > 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   | 39
> > +++++++++++++++++++
> >  2 files changed, 48 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 at 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 at 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 at 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..ef0a6ff69841 100644
> > --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> > +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> > @@ -22,6 +22,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 +132,46 @@ 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 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;
> > +     u8 reg;
> > +
> > +     err = kstrtoul(buf, 0, &val);
> > +     if (err)
> > +             return err;
> > +
> > +     reg = readb(vuart->regs + ASPEED_VUART_GCRA);
> > +     if (val != 0)
> > +             reg |= ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;
> > +     else
> > +             reg &= ~ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;
> > +     writeb(reg, vuart->regs + ASPEED_VUART_GCRA);
> > +
> > +     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,
> >  };
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20190728/31e179ee/attachment.htm>

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

[Oskar Senft]

Update: I just learned from Aspeed that the SIRQ polarity is actually
dependent on the interface mode, LPC vs. eSPI.

For LPC, the polarity should be set to 1, for eSPI the default of 0 is
correct.

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.

If there are no objections, I'll leave the sysfs part in there in case it
does need to be changed.

Oskar.

On Sun, Jul 28, 2019, 12:00 AM Oskar Senft <osk@google.com> wrote:

> 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
>
> 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.
>
> 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.
>
> 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?
>
> Thanks
> Oskar.
>
> On Sat, Jul 27, 2019 at 9:30 PM Benjamin Herrenschmidt <
> benh at kernel.crashing.org> wrote:
>
>> On Sat, 2019-07-27 at 09:42 -0400, Oskar Senft wrote:
>> > Make the SIRQ polarity for Aspeed 24xx/25xx VUART configurable via
>> > sysfs. It is required on some host platforms (e.g. TYAN S7106) to
>> > reconfigure this setting from the default to enable the host to
>> > receive
>> > interrupts from the VUART.
>> >
>> > The setting is configurable via sysfs rather than device-tree to stay
>> > in
>> > line with other related configurable settings.
>>
>> If it's a fixed platform configuration that never changes at runtime,
>> shouldn't it be in the device-tree instead ?
>>
>> Cheers,
>> Ben
>>
>> > 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   | 39
>> > +++++++++++++++++++
>> >  2 files changed, 48 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 at 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 at 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 at 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..ef0a6ff69841 100644
>> > --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
>> > +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
>> > @@ -22,6 +22,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 +132,46 @@ 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 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;
>> > +     u8 reg;
>> > +
>> > +     err = kstrtoul(buf, 0, &val);
>> > +     if (err)
>> > +             return err;
>> > +
>> > +     reg = readb(vuart->regs + ASPEED_VUART_GCRA);
>> > +     if (val != 0)
>> > +             reg |= ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;
>> > +     else
>> > +             reg &= ~ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;
>> > +     writeb(reg, vuart->regs + ASPEED_VUART_GCRA);
>> > +
>> > +     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,
>> >  };
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20190728/cd289cb0/attachment-0001.htm>

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

[Jeremy Kerr]

Hi Oskar,

> For LPC, the polarity should be set to 1, for eSPI the default of 0 is
> correct.

Would there ever be a case where different SIRQs need different
polarities? If not, we may want this to be global, rather than for each
device (the VUART being one...)

Cheers,


Jeremy

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

[Oskar Senft]

Hi Jeremy

In my understanding the SIRQ polarity should be the same for all UARTs on
the particular bus, i.e. both UARTs controlled by the SuperI/O and the
VUART or PUART (pass-through). However, the host controls the UARTs on the
SuperI/O itself. The VUART is covered by this code and we don't have a
PUART driver yet.

It might make sense to have this as a global setting which each driver
could read. But wouldn't this be an exercise for the future where we
actually have a second device? I don't think the Aspeed currently has any
other devices that could generate a SIRQ (except for the PUART for which
there's no driver).

Having said that, ideally I'd like the SIRQ polarity to be auto-configured
from the LPC/eSPI HW pin strap anyway. I have the code for that almost
done. Maybe we shouldn't even have the sysfs interface for it and I should
strip that out?

Thanks
Oskar.

On Sun, Jul 28, 2019 at 8:51 PM Jeremy Kerr <jk@ozlabs.org> wrote:

> Hi Oskar,
>
> > For LPC, the polarity should be set to 1, for eSPI the default of 0 is
> > correct.
>
> Would there ever be a case where different SIRQs need different
> polarities? If not, we may want this to be global, rather than for each
> device (the VUART being one...)
>
> Cheers,
>
>
> Jeremy
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20190728/b73f494d/attachment.htm>

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

[Jeremy Kerr]

Hi Oskar,

> In my understanding the SIRQ polarity should be the same for all UARTs
> on the particular bus, i.e. both UARTs controlled by the SuperI/O and
> the VUART or PUART (pass-through). However, the host controls the
> UARTs on the SuperI/O itself.

Somewhat offtopic, but are you sure you want to enable the SuperIO
device?

>  The VUART is covered by this code and we don't have a PUART driver
> yet.
> 
> It might make sense to have this as a global setting which each driver
> could read. But wouldn't this be an exercise for the future where we
> actually have a second device? I don't think the Aspeed currently has
> any other devices that could generate a SIRQ (except for the PUART for
> which there's no driver).

We have a bunch of SIRQ sources that we use (on OpenPOWER platforms) -
the BT interface, the KCS interface, the UARTs, and the mbox hardware.
It's not just the VUART/PUART :)

> Having said that, ideally I'd like the SIRQ polarity to be auto-
> configured from the LPC/eSPI HW pin strap anyway. I have the code for
> that almost done. Maybe we shouldn't even have the sysfs interface for
> it and I should strip that out?

Yeah, I think I agree with that. The only downside is that the 
individual drivers will now need to poke at the SCU to query the
LPC/eSPI configuration. If you can find a nice way to do that, then that
sounds like the best approach. Can you query it through the parent bus
perhaps?

Cheers,


Jeremy

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

[Oskar Senft]

Hi Jeremy

Somewhat offtopic, but are you sure you want to enable the SuperIO
> device?
>
No :-D I'm aware of CVE-2019-6260. I just listed it as a potential source
of SIRQs.


> >  The VUART is covered by this code and we don't have a PUART driver
> > yet.
> >
> > It might make sense to have this as a global setting which each driver
> > could read. But wouldn't this be an exercise for the future where we
> > actually have a second device? I don't think the Aspeed currently has
> > any other devices that could generate a SIRQ (except for the PUART for
> > which there's no driver).
>
> We have a bunch of SIRQ sources that we use (on OpenPOWER platforms) -
> the BT interface, the KCS interface, the UARTs, and the mbox hardware.
> It's not just the VUART/PUART :)
>
Interesting. From what Aspeed explained, the SIRQ polarity for UARTs is
inverted to that for other devices. I haven't looked into how other devices
work, to be honest. Do we set the polarity there explicitly?


> > Having said that, ideally I'd like the SIRQ polarity to be auto-
> > configured from the LPC/eSPI HW pin strap anyway. I have the code for
> > that almost done. Maybe we shouldn't even have the sysfs interface for
> > it and I should strip that out?
>
> Yeah, I think I agree with that. The only downside is that the
> individual drivers will now need to poke at the SCU to query the
> LPC/eSPI configuration. If you can find a nice way to do that, then that
> sounds like the best approach. Can you query it through the parent bus
> perhaps?
>
I'm experimenting with this:
syscon_regmap_lookup_by_compatible("aspeed,ast2500-scu");

I'll need to think on how to make this work on both ast2400 and ast2500,
though. I actually need to have a look at the ast2400 data sheet wrt. VUART
registers, too!

The structure is this:
apb {
  syscon {
    ...
  }
  vuart {
    ...
  }
}

So the vuart is a sibling of syscon, which holds the registers. I guess
I'll have to go with "by_compatible"?

Oskar.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20190728/9c5140c3/attachment.htm>

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

[Andrew Jeffery]

On Mon, 29 Jul 2019, at 11:51, Oskar Senft wrote:
> 
> Hi Jeremy
> 
> > Somewhat offtopic, but are you sure you want to enable the SuperIO
> >  device?
> No :-D I'm aware of CVE-2019-6260. I just listed it as a potential 
> source of SIRQs.
> > > The VUART is covered by this code and we don't have a PUART driver
> >  > yet.
> >  > 
> >  > It might make sense to have this as a global setting which each driver
> >  > could read. But wouldn't this be an exercise for the future where we
> >  > actually have a second device? I don't think the Aspeed currently has
> >  > any other devices that could generate a SIRQ (except for the PUART for
> >  > which there's no driver).
> > 
> >  We have a bunch of SIRQ sources that we use (on OpenPOWER platforms) -
> >  the BT interface, the KCS interface, the UARTs, and the mbox hardware.
> >  It's not just the VUART/PUART :)
> Interesting. From what Aspeed explained, the SIRQ polarity for UARTs is 
> inverted to that for other devices. I haven't looked into how other 
> devices work, to be honest. Do we set the polarity there explicitly?
> > > Having said that, ideally I'd like the SIRQ polarity to be auto-
> >  > configured from the LPC/eSPI HW pin strap anyway. I have the code for
> >  > that almost done. Maybe we shouldn't even have the sysfs interface for
> >  > it and I should strip that out?
> > 
> >  Yeah, I think I agree with that. The only downside is that the 
> >  individual drivers will now need to poke at the SCU to query the
> >  LPC/eSPI configuration. If you can find a nice way to do that, then that
> >  sounds like the best approach. Can you query it through the parent bus
> >  perhaps?
> I'm experimenting with this:
> syscon_regmap_lookup_by_compatible("aspeed,ast2500-scu");
> 
> I'll need to think on how to make this work on both ast2400 and 
> ast2500, though. I actually need to have a look at the ast2400 data 
> sheet wrt. VUART registers, too!
> 
> The structure is this:
> apb {
>  syscon {
>  ...
>  }
>  vuart {
>  ...
>  }
> }
> 
> So the vuart is a sibling of syscon, which holds the registers. I guess 
> I'll have to go with "by_compatible"?

It might be better to add a property to the UART DT node that references
the SCU syscon by phandle, that way you don't need to muck around
with compatible names for the SCU syscon.

Andrew

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

[Oskar Senft]

Done. I submitted just a v2 of the patch alongside the other bits and
pieces that are required to make this work.

Let me know if that works for you or if there's anything that should be
changed.

Oskar.

On Sun, Jul 28, 2019 at 11:02 PM Andrew Jeffery <andrew@aj.id.au> wrote:

>
>
> On Mon, 29 Jul 2019, at 11:51, Oskar Senft wrote:
> >
> > Hi Jeremy
> >
> > > Somewhat offtopic, but are you sure you want to enable the SuperIO
> > >  device?
> > No :-D I'm aware of CVE-2019-6260. I just listed it as a potential
> > source of SIRQs.
> > > > The VUART is covered by this code and we don't have a PUART driver
> > >  > yet.
> > >  >
> > >  > It might make sense to have this as a global setting which each
> driver
> > >  > could read. But wouldn't this be an exercise for the future where we
> > >  > actually have a second device? I don't think the Aspeed currently
> has
> > >  > any other devices that could generate a SIRQ (except for the PUART
> for
> > >  > which there's no driver).
> > >
> > >  We have a bunch of SIRQ sources that we use (on OpenPOWER platforms) -
> > >  the BT interface, the KCS interface, the UARTs, and the mbox hardware.
> > >  It's not just the VUART/PUART :)
> > Interesting. From what Aspeed explained, the SIRQ polarity for UARTs is
> > inverted to that for other devices. I haven't looked into how other
> > devices work, to be honest. Do we set the polarity there explicitly?
> > > > Having said that, ideally I'd like the SIRQ polarity to be auto-
> > >  > configured from the LPC/eSPI HW pin strap anyway. I have the code
> for
> > >  > that almost done. Maybe we shouldn't even have the sysfs interface
> for
> > >  > it and I should strip that out?
> > >
> > >  Yeah, I think I agree with that. The only downside is that the
> > >  individual drivers will now need to poke at the SCU to query the
> > >  LPC/eSPI configuration. If you can find a nice way to do that, then
> that
> > >  sounds like the best approach. Can you query it through the parent bus
> > >  perhaps?
> > I'm experimenting with this:
> > syscon_regmap_lookup_by_compatible("aspeed,ast2500-scu");
> >
> > I'll need to think on how to make this work on both ast2400 and
> > ast2500, though. I actually need to have a look at the ast2400 data
> > sheet wrt. VUART registers, too!
> >
> > The structure is this:
> > apb {
> >  syscon {
> >  ...
> >  }
> >  vuart {
> >  ...
> >  }
> > }
> >
> > So the vuart is a sibling of syscon, which holds the registers. I guess
> > I'll have to go with "by_compatible"?
>
> It might be better to add a property to the UART DT node that references
> the SCU syscon by phandle, that way you don't need to muck around
> with compatible names for the SCU syscon.
>
> Andrew
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20190730/a1c5c05f/attachment.htm>

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

[Benjamin Herrenschmidt]

On Mon, 2019-07-29 at 08:51 +0800, Jeremy Kerr wrote:
> Hi Oskar,
> 
> > For LPC, the polarity should be set to 1, for eSPI the default of 0 is
> > correct.
> 
> Would there ever be a case where different SIRQs need different
> polarities? If not, we may want this to be global, rather than for each
> device (the VUART being one...)

Leave it per device. The way to configure this is completely device
dependent. In fact for most devices it can only be done by the host via
SIO.

Cheers,
Ben.

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

[Oskar Senft]

Ok, so it sounds that a default value derived from HW strap for LPC/eSPI
(see discussion on how to access the registers correctly) + sysfs seems to
be ok?

Oskar.

On Sun, Jul 28, 2019 at 10:20 PM Benjamin Herrenschmidt <
benh@kernel.crashing.org> wrote:

> On Mon, 2019-07-29 at 08:51 +0800, Jeremy Kerr wrote:
> > Hi Oskar,
> >
> > > For LPC, the polarity should be set to 1, for eSPI the default of 0 is
> > > correct.
> >
> > Would there ever be a case where different SIRQs need different
> > polarities? If not, we may want this to be global, rather than for each
> > device (the VUART being one...)
>
> Leave it per device. The way to configure this is completely device
> dependent. In fact for most devices it can only be done by the host via
> SIO.
>
> Cheers,
> Ben.
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20190728/f7c08438/attachment-0001.htm>

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

[Benjamin Herrenschmidt]

On Sun, 2019-07-28 at 00:00 -0400, Oskar Senft wrote:
> 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
> 
> 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.
> 
> 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.
> 
> 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?

No strong feelings. Adding properties should be ok but on the other
hand, I agree with keeping things consistent with the other LPC
settings (address & irq#).

Cheers,
Ben.

> 
> Thanks
> Oskar.
> 
> On Sat, Jul 27, 2019 at 9:30 PM Benjamin Herrenschmidt <
> benh at kernel.crashing.org> wrote:
> > On Sat, 2019-07-27 at 09:42 -0400, Oskar Senft wrote:
> > > Make the SIRQ polarity for Aspeed 24xx/25xx VUART configurable
> > via
> > > sysfs. It is required on some host platforms (e.g. TYAN S7106) to
> > > reconfigure this setting from the default to enable the host to
> > > receive
> > > interrupts from the VUART.
> > > 
> > > The setting is configurable via sysfs rather than device-tree to
> > stay
> > > in
> > > line with other related configurable settings.
> > 
> > If it's a fixed platform configuration that never changes at
> > runtime,
> > shouldn't it be in the device-tree instead ?
> > 
> > Cheers,
> > Ben
> > 
> > > 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   | 39
> > > +++++++++++++++++++
> > >  2 files changed, 48 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 at 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 at 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 at 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..ef0a6ff69841 100644
> > > --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> > > +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> > > @@ -22,6 +22,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 +132,46 @@ 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 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;
> > > +     u8 reg;
> > > +
> > > +     err = kstrtoul(buf, 0, &val);
> > > +     if (err)
> > > +             return err;
> > > +
> > > +     reg = readb(vuart->regs + ASPEED_VUART_GCRA);
> > > +     if (val != 0)
> > > +             reg |= ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;
> > > +     else
> > > +             reg &= ~ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;
> > > +     writeb(reg, vuart->regs + ASPEED_VUART_GCRA);
> > > +
> > > +     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,
> > >  };
> >