| Message ID | 20190727134242.49847-1-osk@google.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | drivers/tty/serial/8250: Make Aspeed VUART SIRQ polarity configurable | expand |
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, > };
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, > > }; > >
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, >> > }; >> >>
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
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 > >
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
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, > > > }; > >
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.
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.
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. > > >
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
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 >
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, };
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(-)