drivers/tty/serial/8250: Make Aspeed VUART SIRQ polarity configurable
diff mbox series

Message ID 20190727134242.49847-1-osk@google.com
State New
Headers show
Series
  • drivers/tty/serial/8250: Make Aspeed VUART SIRQ polarity configurable
Related show

Commit Message

Oskar Senft July 27, 2019, 1:42 p.m. UTC
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(-)

Comments

Benjamin Herrenschmidt July 28, 2019, 1:30 a.m. UTC | #1
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@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..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,
>  };
Oskar Senft July 28, 2019, 4 a.m. UTC | #2
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@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..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,
> >  };
>
>
Oskar Senft July 28, 2019, 5:23 p.m. UTC | #3
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@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@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..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,
>> >  };
>>
>>
Jeremy Kerr July 29, 2019, 12:51 a.m. UTC | #4
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
Oskar Senft July 29, 2019, 1:14 a.m. UTC | #5
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
>
>
Jeremy Kerr July 29, 2019, 2:02 a.m. UTC | #6
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
Benjamin Herrenschmidt July 29, 2019, 2:19 a.m. UTC | #7
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@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@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..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,
> > >  };
> >
Benjamin Herrenschmidt July 29, 2019, 2:20 a.m. UTC | #8
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.
Oskar Senft July 29, 2019, 2:21 a.m. UTC | #9
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.
Oskar Senft July 29, 2019, 2:23 a.m. UTC | #10
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.
>
>
>
Andrew Jeffery July 29, 2019, 3:02 a.m. UTC | #11
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
Oskar Senft July 31, 2019, 1:36 a.m. UTC | #12
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
>

Patch
diff mbox series

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..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,
 };