drivers/tty/serial/8250: Make Aspeed VUART SIRQ polarity configurable
PATCH v2 1/3 drivers/tty/serial/8250: Make Aspeed VUART SIRQ polarity configurable
Make the SIRQ polarity for Aspeed AST24xx/25xx VUART configurable via sysfs. This setting need to be changed on specific host platforms depending on the selected host interface (LPC / eSPI).
The setting is configurable via sysfs rather than device-tree to stay in line with other related configurable settings.
On AST2500 the VUART SIRQ polarity can be auto-configured by reading the LPC/eSPI interface configuration from the HW strap table.
Tested: Verified on TYAN S7106 mainboard.
--- .../ABI/stable/sysfs-driver-aspeed-vuart | 10 ++- drivers/tty/serial/8250/8250_aspeed_vuart.c | 82 +++++++++++++++++++ drivers/tty/serial/8250/Kconfig | 1 + 3 files changed, 92 insertions(+), 1 deletion(-)
Documentation/ABI/stable/sysfs-driver-aspeed-vuart +9 -1
@@ -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 <[email protected]> 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 <[email protected]> +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
drivers/tty/serial/8250/8250_aspeed_vuart.c +82 -0
120 lines not on this page · raw message
drivers/tty/serial/8250/Kconfig +1 -0
@@ -243,6 +243,7 @@ config SERIAL_8250_ASPEED_VUART tristate "Aspeed Virtual UART" depends on SERIAL_8250 depends on OF + depends on REGMAP && MFD_SYSCON help If you want to use the virtual UART (VUART) device on Aspeed BMC platforms, enable this option. This enables the 16550A-
Signature · 1 line
2.22.0.709.g102302147b-goog
PATCH v2 2/3 dt-bindings: serial: 8250: Add documentation for espi-enabled.
Add documentation for 8250_aspeed_vuart's espi-enabled property that enables to auto-configure the VUART's SIRQ polarity.
--- Documentation/devicetree/bindings/serial/8250.txt | 5 +++++ 1 file changed, 5 insertions(+)
Documentation/devicetree/bindings/serial/8250.txt +5 -0
@@ -56,6 +56,11 @@ Optional properties: - {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD line respectively. It will use specified GPIO instead of the peripheral function pin for the UART feature. If unsure, don't specify this property. +- espi-enabled: Only applicable to aspeed,ast2500-vuart. Value is a phandle to + aspeed,ast2500-scu syscon alongside register offset and bit number to + identify whether the system is in eSPI mode. This is used to auto-configure + SIRQ polarity on the vuart. + Example: espi-enabled = <&syscon 0x70 25> Note: * fsl,ns16550:
Signature · 1 line
2.22.0.709.g102302147b-goog
Re: [PATCH v2 2/3] dt-bindings: serial: 8250: Add documentation for espi-enabled.
Hi Oskar,
Quoted 15 lines · Oskar Senft
Add documentation for 8250_aspeed_vuart's espi-enabled property that enables to auto-configure the VUART's SIRQ polarity.
Signed-off-by Oskar Senft <[email protected]>--- Documentation/devicetree/bindings/serial/8250.txt | 5 +++++ 1 file changed, 5 insertions(+)Documentation/devicetree/bindings/serial/8250.txt +0
-0@@ -56,6 +56,11 @@ Optional properties: - {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for
Quoted 6 lines
RTS/CTS/DTR/DSR/RI/DCD
line respectively. It will use specified GPIO instead of the
peripheral
function pin for the UART feature. If unsure, don't specify this
property.
+- espi-enabled: Only applicable to aspeed,ast2500-vuart.
Bit of a bikeshed, but:
Given it's ASPEED-specific I expect you should use a vendor prefix for the property, e.g. aspeed,espi-enabled.
However, as I understand it you want to determine what polarity the SIRQ should be regardless of which of eSPI or LPC are enabled, so I don't think the property name should be an explicit statement about eSPI. Maybe "aspeed,sirq-polarity-sense"? Anyway, see the point below:
Please use ./scripts/get_maintainer.pl to determine where to send the series - Copying just the linux-aspeed@ list for upstream patches is not enough. For instance the series needs to at least go via the linux-serial@ list given that's the affected subsystem, and you're adding to the devicetree binding so you need to send to the devicetree@ list as well (you'll need an ack from Rob Herring). The get_maintainer.pl script will give you all the information you need.
Cheers,
Andrew
PATCH v2 2/3 dt-bindings: serial: 8250: Add documentation for espi-enabled.
Hi Oskar,
Quoted 7 lines
Given it's ASPEED-specific I expect you should use a vendor prefix for the property, e.g. aspeed,espi-enabled.
However, as I understand it you want to determine what polarity the SIRQ should be regardless of which of eSPI or LPC are enabled, so I don't think the property name should be an explicit statement about eSPI. Maybe "aspeed,sirq-polarity-sense"?
Yep, +1 on Andrew's comments here. This property isn't an indication on whether espi is enabled, but a method to detect it.
Cheers,
Jeremy
PATCH v2 2/3 dt-bindings: serial: 8250: Add documentation for espi-enabled.
Hi Andrew and Jeremy
Thanks for both your input, that was helpful. I'm sorry it took me so long to get back to this. I hope it's in better shape now.
Quoted 3 lines
Given it's ASPEED-specific I expect you should use a vendor prefix for the
Quoted 1 line
property, e.g. aspeed,espi-enabled.
That was a very good point.
Quoted 9 lines
Quoted 2 lines
However, as I understand it you want to determine what polarity the SIRQ should be regardless of which of eSPI or LPC are enabled, so I don't
think
Quoted 2 lines
the property name should be an explicit statement about eSPI. Maybe "aspeed,sirq-polarity-sense"?
Yep, +1 on Andrew's comments here. This property isn't an indication on whether espi is enabled, but a method to detect it.
I agree. I was so focused on functionality that I didn't look at this with a wider view.
I'll send v3 of the patch that contains appropriate changes. I'll also include further lists and individuals to get OKs as needed.
Thanks
Oskar.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20190905/b5d5a676/attachment.htm>
PATCH v2 3/3 arm: dts: aspeed: Add vuart espi-enabled to aspeed-g5.dtsi
In reply to Oskar SenftEnable auto-configuration of VUART SIRQ polarity on AST2500.
--- arch/arm/boot/dts/aspeed-g5.dtsi | 1 + 1 file changed, 1 insertion(+)
arch/arm/boot/dts/aspeed-g5.dtsi +1 -0
@@ -351,6 +351,7 @@ interrupts = <8>; clocks = <&syscon ASPEED_CLK_APB>; no-loopback-test; + espi-enabled = <&syscon 0x70 25>; status = "disabled"; };
Signature · 1 line
2.22.0.709.g102302147b-goog
PATCH v2 1/3 drivers/tty/serial/8250: Make Aspeed VUART SIRQ polarity configurable
In reply to Oskar SenftHi Oskar,
Looks good to me, some minor comments though:
Documentation/ABI/stable/sysfs-driver-aspeed-vuart Description: Configures which IO port the host side of the UART · line 21
Quoted 6 lines
Documentation/ABI/stable/sysfs-driver-aspeed-vuart +6
-0+ +What: /sys/bus/platform/drivers/aspeed-vuart/*/sirq_polarity +Date: July 2019 +Contact: Oskar Senft <[email protected]> +Description: Configures the polarity of the serial interrupt to the + host via the BMC LPC bus.
Can you mention what the value represents? 1/0 don't really indicate a specific polarity.
Alternatively, we could use descriptive values (say, "active-low" / "idle-low").
drivers/tty/serial/8250/8250_aspeed_vuart.c static int aspeed_vuart_probe(struct platform_device *pdev) · line 382
Quoted 5 lines
drivers/tty/serial/8250/8250_aspeed_vuart.c +1
-0@@ -310,6 +379,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev)struct resource *res;u32 clk, prop;int rc; +struct of_phandle_args espi_enabled_args;
Minor: can you reverse-christmas-tree this?
drivers/tty/serial/8250/8250_aspeed_vuart.c static int aspeed_vuart_probe(struct platform_device *pdev) · line 478
Quoted 8 lines
drivers/tty/serial/8250/8250_aspeed_vuart.c +4
-0@@ -402,6 +472,18 @@ static int aspeed_vuart_probe(struct platform_device *pdev) vuart->line = rc; + rc =of_parse_phandle_with_fixed_args ( + np,"espi-enabled" ,2 ,0 , &espi_enabled_args); +if (rc <0 ) { +dev_warn (&pdev->dev,"espi-enabled property not found\n" );
In the binding spec, you've listed this property at optional, but here we dev_warn() if its not present. Can we default to existing behaviour if it's not there?
That may just be a matter of changing this to dev_debug.
Cheers,
Jeremy
PATCH v2 1/3 drivers/tty/serial/8250: Make Aspeed VUART SIRQ polarity configurable
Hi Jeremy
Thanks for your comments, they were really helpful!
Quoted 10 lines
Quoted 1 line
+What:
/sys/bus/platform/drivers/aspeed-vuart/*/sirq_polarityDocumentation/ABI/stable/sysfs-driver-aspeed-vuart Description: Configures which IO port the host side of the UART · line 21
Quoted 4 lines
Documentation/ABI/stable/sysfs-driver-aspeed-vuart +4
-0+Date: July 2019 +Contact: Oskar Senft <[email protected]> +Description: Configures the polarity of the serial interrupt to the + host via the BMC LPC bus.Can you mention what the value represents? 1/0 don't really indicate a specific polarity.
Good point. Not sure why I didn't do that initially.
Quoted 8 lines
Review static int aspeed_vuart_probe(struct platform_device
Quoted 1 line
(quoted diff) +0
-0@@ -310,6 +379,7 @@ static int aspeed_vuart_probe(struct platform_device*pdev)
drivers/tty/serial/8250/8250_aspeed_vuart.c static int aspeed_vuart_probe(struct platform_device *pdev) · line 382
Quoted 4 lines
drivers/tty/serial/8250/8250_aspeed_vuart.c +1
-0struct resource *res;u32 clk, prop;int rc; +struct of_phandle_args espi_enabled_args;Minor: can you reverse-christmas-tree this?
Oops, yeah. Sorry! Conflicting coding styles in my head got confused.
Quoted 6 lines
drivers/tty/serial/8250/8250_aspeed_vuart.c static int aspeed_vuart_probe(struct platform_device *pdev) · line 478
Quoted 4 lines
drivers/tty/serial/8250/8250_aspeed_vuart.c +4
-0+ rc =of_parse_phandle_with_fixed_args ( + np,"espi-enabled" ,2 ,0 , &espi_enabled_args); +if (rc <0 ) { +dev_warn (&pdev->dev,"espi-enabled property not found\n" );That may just be a matter of changing this to
dev_debug.
That was my intent, sorry for that.
I'll send v3 with those changes in a few minutes.
Thanks
Oskar.
Quoted 41 lines · Jeremy Kerr
Hi Oskar,
Looks good to me, some minor comments though:
Review
Quoted 2 lines
(quoted diff) +2
-0+ +What:
/sys/bus/platform/drivers/aspeed-vuart/*/sirq_polarityDocumentation/ABI/stable/sysfs-driver-aspeed-vuart Description: Configures which IO port the host side of the UART · line 21
Quoted 4 lines
Documentation/ABI/stable/sysfs-driver-aspeed-vuart +4
-0+Date: July 2019 +Contact: Oskar Senft <[email protected]> +Description: Configures the polarity of the serial interrupt to the + host via the BMC LPC bus.Can you mention what the value represents? 1/0 don't really indicate a specific polarity.
Alternatively, we could use descriptive values (say, "active-low" / "idle-low").
Review static int aspeed_vuart_probe(struct platform_device
Quoted 1 line
(quoted diff) +0
-0@@ -310,6 +379,7 @@ static int aspeed_vuart_probe(struct platform_device*pdev)
drivers/tty/serial/8250/8250_aspeed_vuart.c static int aspeed_vuart_probe(struct platform_device *pdev) · line 382
Quoted 4 lines
drivers/tty/serial/8250/8250_aspeed_vuart.c +1
-0struct resource *res;u32 clk, prop;int rc; +struct of_phandle_args espi_enabled_args;Minor: can you reverse-christmas-tree this?
Review static int aspeed_vuart_probe(struct
Quoted 1 line
(quoted diff) +0
-0@@ -402,6 +472,18 @@ static int aspeed_vuart_probe(struct
platform_device*pdev)drivers/tty/serial/8250/8250_aspeed_vuart.c static int aspeed_vuart_probe(struct platform_device *pdev) · line 478
Quoted 7 lines
drivers/tty/serial/8250/8250_aspeed_vuart.c +4
-0vuart->line = rc; + rc =of_parse_phandle_with_fixed_args ( + np,"espi-enabled" ,2 ,0 , &espi_enabled_args); +if (rc <0 ) { +dev_warn (&pdev->dev,"espi-enabled property not found\n" );In the binding spec, you've listed this property at optional, but here we
dev_warn()if its not present. Can we default to existing behaviour if it's not there?
Quoted 8 lines
That may just be a matter of changing this to
dev_debug.Cheers,
Jeremy
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20190905/a92748e8/attachment.htm>