2021-03-30 00:35:55

by Zev Weiss

[permalink] [raw]
Subject: [PATCH 2/3] dt-bindings: serial: 8250: update for aspeed,sirq-active-high

Update DT bindings documentation for the new incarnation of the
aspeed,sirq-polarity-sense property.

Signed-off-by: Zev Weiss <[email protected]>
---
Documentation/devicetree/bindings/serial/8250.yaml | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
index f54cae9ff7b2..0bbb7121f720 100644
--- a/Documentation/devicetree/bindings/serial/8250.yaml
+++ b/Documentation/devicetree/bindings/serial/8250.yaml
@@ -13,7 +13,7 @@ allOf:
- $ref: /schemas/serial.yaml#
- if:
required:
- - aspeed,sirq-polarity-sense
+ - aspeed,sirq-active-high
then:
properties:
compatible:
@@ -181,13 +181,11 @@ properties:
rng-gpios: true
dcd-gpios: true

- aspeed,sirq-polarity-sense:
- $ref: /schemas/types.yaml#/definitions/phandle-array
+ aspeed,sirq-active-high:
+ type: boolean
description: |
- Phandle to aspeed,ast2500-scu compatible syscon alongside register
- offset and bit number to identify how the SIRQ polarity should be
- configured. One possible data source is the LPC/eSPI mode bit. Only
- applicable to aspeed,ast2500-vuart.
+ Set to indicate that the SIRQ polarity is active-high (default
+ is active-low). Only applicable to aspeed,ast2500-vuart.

required:
- reg
@@ -227,7 +225,7 @@ examples:
interrupts = <8>;
clocks = <&syscon ASPEED_CLK_APB>;
no-loopback-test;
- aspeed,sirq-polarity-sense = <&syscon 0x70 25>;
+ aspeed,sirq-active-high;
};

...
--
2.31.1


2021-03-30 22:43:02

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 2/3] dt-bindings: serial: 8250: update for aspeed,sirq-active-high

On Mon, Mar 29, 2021 at 07:23:37PM -0500, Zev Weiss wrote:
> Update DT bindings documentation for the new incarnation of the
> aspeed,sirq-polarity-sense property.

Why?

This isn't a compatible change.

>
> Signed-off-by: Zev Weiss <[email protected]>
> ---
> Documentation/devicetree/bindings/serial/8250.yaml | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
> index f54cae9ff7b2..0bbb7121f720 100644
> --- a/Documentation/devicetree/bindings/serial/8250.yaml
> +++ b/Documentation/devicetree/bindings/serial/8250.yaml
> @@ -13,7 +13,7 @@ allOf:
> - $ref: /schemas/serial.yaml#
> - if:
> required:
> - - aspeed,sirq-polarity-sense
> + - aspeed,sirq-active-high
> then:
> properties:
> compatible:
> @@ -181,13 +181,11 @@ properties:
> rng-gpios: true
> dcd-gpios: true
>
> - aspeed,sirq-polarity-sense:
> - $ref: /schemas/types.yaml#/definitions/phandle-array
> + aspeed,sirq-active-high:
> + type: boolean
> description: |
> - Phandle to aspeed,ast2500-scu compatible syscon alongside register
> - offset and bit number to identify how the SIRQ polarity should be
> - configured. One possible data source is the LPC/eSPI mode bit. Only
> - applicable to aspeed,ast2500-vuart.
> + Set to indicate that the SIRQ polarity is active-high (default
> + is active-low). Only applicable to aspeed,ast2500-vuart.
>
> required:
> - reg
> @@ -227,7 +225,7 @@ examples:
> interrupts = <8>;
> clocks = <&syscon ASPEED_CLK_APB>;
> no-loopback-test;
> - aspeed,sirq-polarity-sense = <&syscon 0x70 25>;
> + aspeed,sirq-active-high;
> };
>
> ...
> --
> 2.31.1
>

2021-03-30 23:06:17

by Zev Weiss

[permalink] [raw]
Subject: Re: [PATCH 2/3] dt-bindings: serial: 8250: update for aspeed,sirq-active-high

On Tue, Mar 30, 2021 at 05:39:02PM CDT, Rob Herring wrote:
>On Mon, Mar 29, 2021 at 07:23:37PM -0500, Zev Weiss wrote:
>> Update DT bindings documentation for the new incarnation of the
>> aspeed,sirq-polarity-sense property.
>
>Why?
>
>This isn't a compatible change.
>

Ah, sorry -- that was a misunderstanding on my end. I'll resend a
compatible v2 shortly.


Zev

2021-03-30 23:29:58

by Joel Stanley

[permalink] [raw]
Subject: Re: [PATCH 2/3] dt-bindings: serial: 8250: update for aspeed,sirq-active-high

On Tue, 30 Mar 2021 at 22:39, Rob Herring <[email protected]> wrote:
>
> On Mon, Mar 29, 2021 at 07:23:37PM -0500, Zev Weiss wrote:
> > Update DT bindings documentation for the new incarnation of the
> > aspeed,sirq-polarity-sense property.
>
> Why?
>
> This isn't a compatible change.

We want to depreciate support for this property. It should have never
been added to the bindings; in it's current form it describes a
relationship that afaict doesn't exist ("This unrelated register over
here dictates the polarity of your virtual serial port IRQ"). See
https://lore.kernel.org/lkml/[email protected]/

The intent is to remove it from both the bindings and the code.
There's already no users of it in any device tree.

How would you like Zev to go about doing this?

Cheers,

Joel

>
> >
> > Signed-off-by: Zev Weiss <[email protected]>
> > ---
> > Documentation/devicetree/bindings/serial/8250.yaml | 14 ++++++--------
> > 1 file changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
> > index f54cae9ff7b2..0bbb7121f720 100644
> > --- a/Documentation/devicetree/bindings/serial/8250.yaml
> > +++ b/Documentation/devicetree/bindings/serial/8250.yaml
> > @@ -13,7 +13,7 @@ allOf:
> > - $ref: /schemas/serial.yaml#
> > - if:
> > required:
> > - - aspeed,sirq-polarity-sense
> > + - aspeed,sirq-active-high
> > then:
> > properties:
> > compatible:
> > @@ -181,13 +181,11 @@ properties:
> > rng-gpios: true
> > dcd-gpios: true
> >
> > - aspeed,sirq-polarity-sense:
> > - $ref: /schemas/types.yaml#/definitions/phandle-array
> > + aspeed,sirq-active-high:
> > + type: boolean
> > description: |
> > - Phandle to aspeed,ast2500-scu compatible syscon alongside register
> > - offset and bit number to identify how the SIRQ polarity should be
> > - configured. One possible data source is the LPC/eSPI mode bit. Only
> > - applicable to aspeed,ast2500-vuart.
> > + Set to indicate that the SIRQ polarity is active-high (default
> > + is active-low). Only applicable to aspeed,ast2500-vuart.
> >
> > required:
> > - reg
> > @@ -227,7 +225,7 @@ examples:
> > interrupts = <8>;
> > clocks = <&syscon ASPEED_CLK_APB>;
> > no-loopback-test;
> > - aspeed,sirq-polarity-sense = <&syscon 0x70 25>;
> > + aspeed,sirq-active-high;
> > };
> >
> > ...
> > --
> > 2.31.1
> >

2021-04-01 00:59:07

by Zev Weiss

[permalink] [raw]
Subject: [PATCH v2 0/3] simplify Aspeed VUART SIRQ polarity DT config

The aspeed,sirq-polarity-sense property was a bit of a design mistake
in that it ties Aspeed VUART SIRQ polarity to SCU register bits that
aren't really inherently related to it.

This patch series deprecates that property (though we hope to
eventually remove it) and adds a simple boolean property
(aspeed,sirq-active-high) to use instead.


Changes since v1:
- deprecate and retain aspeed,sirq-polarity-sense instead of removing it
- drop e3c246d4i dts addition from this series


Zev Weiss (3):
dt-bindings: serial: 8250: deprecate aspeed,sirq-polarity-sense
drivers/tty/serial/8250: add DT property for aspeed vuart sirq
polarity
dt-bindings: serial: 8250: add aspeed,sirq-active-high

Documentation/devicetree/bindings/serial/8250.yaml | 14 +++++++++++---
drivers/tty/serial/8250/8250_aspeed_vuart.c | 3 +++
2 files changed, 14 insertions(+), 3 deletions(-)

--
2.31.1

2021-04-01 00:59:53

by Zev Weiss

[permalink] [raw]
Subject: [PATCH v2 1/3] dt-bindings: serial: 8250: deprecate aspeed,sirq-polarity-sense

This property ties SIRQ polarity to SCU register bits that don't
necessarily have any direct relationship to it; the only use of it
was removed in commit c82bf6e133d30e0f9172a20807814fa28aef0f67.

Signed-off-by: Zev Weiss <[email protected]>
---
Documentation/devicetree/bindings/serial/8250.yaml | 1 +
1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
index f54cae9ff7b2..491b9297432d 100644
--- a/Documentation/devicetree/bindings/serial/8250.yaml
+++ b/Documentation/devicetree/bindings/serial/8250.yaml
@@ -188,6 +188,7 @@ properties:
offset and bit number to identify how the SIRQ polarity should be
configured. One possible data source is the LPC/eSPI mode bit. Only
applicable to aspeed,ast2500-vuart.
+ deprecated: true

required:
- reg
--
2.31.1

2021-04-01 01:02:08

by Zev Weiss

[permalink] [raw]
Subject: [PATCH v2 2/3] drivers/tty/serial/8250: add DT property for aspeed vuart sirq polarity

This provides a simple boolean to use instead of the deprecated
aspeed,sirq-polarity-sense property.

Signed-off-by: Zev Weiss <[email protected]>
---
drivers/tty/serial/8250/8250_aspeed_vuart.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index c33e02cbde93..e5ef9f957f9a 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -482,6 +482,9 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
of_node_put(sirq_polarity_sense_args.np);
}

+ if (of_property_read_bool(np, "aspeed,sirq-active-high"))
+ aspeed_vuart_set_sirq_polarity(vuart, 1);
+
aspeed_vuart_set_enabled(vuart, true);
aspeed_vuart_set_host_tx_discard(vuart, true);
platform_set_drvdata(pdev, vuart);
--
2.31.1

2021-04-01 01:03:38

by Zev Weiss

[permalink] [raw]
Subject: [PATCH v2 3/3] dt-bindings: serial: 8250: add aspeed,sirq-active-high

This provides a simpler, more direct alternative to the deprecated
aspeed,sirq-polarity-sense property for indicating the polarity of
the Aspeed VUART's SIRQ line.

Signed-off-by: Zev Weiss <[email protected]>
---
Documentation/devicetree/bindings/serial/8250.yaml | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
index 491b9297432d..e79bb6ab9d2c 100644
--- a/Documentation/devicetree/bindings/serial/8250.yaml
+++ b/Documentation/devicetree/bindings/serial/8250.yaml
@@ -12,8 +12,9 @@ maintainers:
allOf:
- $ref: /schemas/serial.yaml#
- if:
- required:
- - aspeed,sirq-polarity-sense
+ anyOf:
+ - required: [ aspeed,sirq-active-high ]
+ - required: [ aspeed,sirq-polarity-sense ]
then:
properties:
compatible:
@@ -190,6 +191,12 @@ properties:
applicable to aspeed,ast2500-vuart.
deprecated: true

+ aspeed,sirq-active-high:
+ type: boolean
+ description: |
+ Set to indicate that the SIRQ polarity is active-high (default
+ is active-low). Only applicable to aspeed,ast2500-vuart.
+
required:
- reg
- interrupts
@@ -228,7 +235,7 @@ examples:
interrupts = <8>;
clocks = <&syscon ASPEED_CLK_APB>;
no-loopback-test;
- aspeed,sirq-polarity-sense = <&syscon 0x70 25>;
+ aspeed,sirq-active-high;
};

...
--
2.31.1

2021-04-01 03:57:59

by Joel Stanley

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] dt-bindings: serial: 8250: deprecate aspeed,sirq-polarity-sense

On Thu, 1 Apr 2021 at 00:57, Zev Weiss <[email protected]> wrote:
>
> This property ties SIRQ polarity to SCU register bits that don't
> necessarily have any direct relationship to it; the only use of it
> was removed in commit c82bf6e133d30e0f9172a20807814fa28aef0f67.
>
> Signed-off-by: Zev Weiss <[email protected]>

Reviewed-by: Joel Stanley <[email protected]>

> ---
> Documentation/devicetree/bindings/serial/8250.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
> index f54cae9ff7b2..491b9297432d 100644
> --- a/Documentation/devicetree/bindings/serial/8250.yaml
> +++ b/Documentation/devicetree/bindings/serial/8250.yaml
> @@ -188,6 +188,7 @@ properties:
> offset and bit number to identify how the SIRQ polarity should be
> configured. One possible data source is the LPC/eSPI mode bit. Only
> applicable to aspeed,ast2500-vuart.
> + deprecated: true
>
> required:
> - reg
> --
> 2.31.1
>

2021-04-01 04:07:02

by Andrew Jeffery

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] dt-bindings: serial: 8250: add aspeed,sirq-active-high



On Thu, 1 Apr 2021, at 11:27, Zev Weiss wrote:
> This provides a simpler, more direct alternative to the deprecated
> aspeed,sirq-polarity-sense property for indicating the polarity of
> the Aspeed VUART's SIRQ line.
>
> Signed-off-by: Zev Weiss <[email protected]>
> ---
> Documentation/devicetree/bindings/serial/8250.yaml | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/serial/8250.yaml
> b/Documentation/devicetree/bindings/serial/8250.yaml
> index 491b9297432d..e79bb6ab9d2c 100644
> --- a/Documentation/devicetree/bindings/serial/8250.yaml
> +++ b/Documentation/devicetree/bindings/serial/8250.yaml
> @@ -12,8 +12,9 @@ maintainers:
> allOf:
> - $ref: /schemas/serial.yaml#
> - if:
> - required:
> - - aspeed,sirq-polarity-sense
> + anyOf:
> + - required: [ aspeed,sirq-active-high ]

Do you think we could make use of the approach I put forward here?

https://lore.kernel.org/openbmc/[email protected]/T/#u

Andrew

2021-04-01 04:17:59

by Joel Stanley

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] drivers/tty/serial/8250: add DT property for aspeed vuart sirq polarity

On Thu, 1 Apr 2021 at 00:57, Zev Weiss <[email protected]> wrote:
>
> This provides a simple boolean to use instead of the deprecated
> aspeed,sirq-polarity-sense property.
>
> Signed-off-by: Zev Weiss <[email protected]>
> ---
> drivers/tty/serial/8250/8250_aspeed_vuart.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> index c33e02cbde93..e5ef9f957f9a 100644
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> @@ -482,6 +482,9 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
> of_node_put(sirq_polarity_sense_args.np);
> }
>
> + if (of_property_read_bool(np, "aspeed,sirq-active-high"))
> + aspeed_vuart_set_sirq_polarity(vuart, 1);

This assumes the default is always low, so we don't need a property to
set it to that state?

Would it make more sense to have the property describe if it's high or
low? (I'm happy for the answer to be "no", as we've gotten by for the
past few years without it).

This brings up another point. We already have the sysfs file for
setting the lpc address, from userspace. In OpenBMC land this can be
set with obmc-console-client (/etc/obmc-console.conf). Should we add
support to that application for setting the irq polarity too, and do
away with device tree descriptions?

> +
> aspeed_vuart_set_enabled(vuart, true);
> aspeed_vuart_set_host_tx_discard(vuart, true);
> platform_set_drvdata(pdev, vuart);
> --
> 2.31.1
>

2021-04-01 05:01:11

by Zev Weiss

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] dt-bindings: serial: 8250: add aspeed,sirq-active-high

On Wed, Mar 31, 2021 at 11:04:44PM CDT, Andrew Jeffery wrote:
>
>
>On Thu, 1 Apr 2021, at 11:27, Zev Weiss wrote:
>> This provides a simpler, more direct alternative to the deprecated
>> aspeed,sirq-polarity-sense property for indicating the polarity of
>> the Aspeed VUART's SIRQ line.
>>
>> Signed-off-by: Zev Weiss <[email protected]>
>> ---
>> Documentation/devicetree/bindings/serial/8250.yaml | 13 ++++++++++---
>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/serial/8250.yaml
>> b/Documentation/devicetree/bindings/serial/8250.yaml
>> index 491b9297432d..e79bb6ab9d2c 100644
>> --- a/Documentation/devicetree/bindings/serial/8250.yaml
>> +++ b/Documentation/devicetree/bindings/serial/8250.yaml
>> @@ -12,8 +12,9 @@ maintainers:
>> allOf:
>> - $ref: /schemas/serial.yaml#
>> - if:
>> - required:
>> - - aspeed,sirq-polarity-sense
>> + anyOf:
>> + - required: [ aspeed,sirq-active-high ]
>
>Do you think we could make use of the approach I put forward here?
>
>https://lore.kernel.org/openbmc/[email protected]/T/#u
>
>Andrew

If you mean using a u32 property (say aspeed,sirq-polarity) with an
explicit IRQ_TYPE_LEVEL_{LOW,HIGH} instead of a present/absent bool,
sure, I guess that seems like a somewhat clearer, more orthogonal
arrangement.


Zev

2021-04-01 05:22:57

by Zev Weiss

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] drivers/tty/serial/8250: add DT property for aspeed vuart sirq polarity

On Wed, Mar 31, 2021 at 11:15:44PM CDT, Joel Stanley wrote:
>On Thu, 1 Apr 2021 at 00:57, Zev Weiss <[email protected]> wrote:
>>
>> This provides a simple boolean to use instead of the deprecated
>> aspeed,sirq-polarity-sense property.
>>
>> Signed-off-by: Zev Weiss <[email protected]>
>> ---
>> drivers/tty/serial/8250/8250_aspeed_vuart.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
>> index c33e02cbde93..e5ef9f957f9a 100644
>> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
>> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
>> @@ -482,6 +482,9 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
>> of_node_put(sirq_polarity_sense_args.np);
>> }
>>
>> + if (of_property_read_bool(np, "aspeed,sirq-active-high"))
>> + aspeed_vuart_set_sirq_polarity(vuart, 1);
>
>This assumes the default is always low, so we don't need a property to
>set it to that state?
>
>Would it make more sense to have the property describe if it's high or
>low? (I'm happy for the answer to be "no", as we've gotten by for the
>past few years without it).
>

Yeah, that sounds like better way to approach it -- I think I'll
rearrange as Andrew suggested in
https://lore.kernel.org/openbmc/[email protected]/

>This brings up another point. We already have the sysfs file for
>setting the lpc address, from userspace. In OpenBMC land this can be
>set with obmc-console-client (/etc/obmc-console.conf). Should we add
>support to that application for setting the irq polarity too, and do
>away with device tree descriptions?
>

I guess I might lean slightly toward keeping the DT description so that
if for whatever reason obmc-console-server flakes out and doesn't start
you're better positioned to try banging on /dev/ttyS* manually if you're
desperate. Though I suppose that in turn might imply that I'm arguing
for adding DT properties for lpc_address and sirq too, and if you're
really that desperate you can just fiddle with sysfs anyway, so...shrug?
I could be convinced either way fairly easily.


Zev

2021-04-01 05:38:46

by Andrew Jeffery

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] drivers/tty/serial/8250: add DT property for aspeed vuart sirq polarity



On Thu, 1 Apr 2021, at 15:48, Zev Weiss wrote:
> On Wed, Mar 31, 2021 at 11:15:44PM CDT, Joel Stanley wrote:
> >On Thu, 1 Apr 2021 at 00:57, Zev Weiss <[email protected]> wrote:
> >>
> >> This provides a simple boolean to use instead of the deprecated
> >> aspeed,sirq-polarity-sense property.
> >>
> >> Signed-off-by: Zev Weiss <[email protected]>
> >> ---
> >> drivers/tty/serial/8250/8250_aspeed_vuart.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> >> index c33e02cbde93..e5ef9f957f9a 100644
> >> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> >> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> >> @@ -482,6 +482,9 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
> >> of_node_put(sirq_polarity_sense_args.np);
> >> }
> >>
> >> + if (of_property_read_bool(np, "aspeed,sirq-active-high"))
> >> + aspeed_vuart_set_sirq_polarity(vuart, 1);
> >
> >This assumes the default is always low, so we don't need a property to
> >set it to that state?
> >
> >Would it make more sense to have the property describe if it's high or
> >low? (I'm happy for the answer to be "no", as we've gotten by for the
> >past few years without it).
> >
>
> Yeah, that sounds like better way to approach it -- I think I'll
> rearrange as Andrew suggested in
> https://lore.kernel.org/openbmc/[email protected]/
>
> >This brings up another point. We already have the sysfs file for
> >setting the lpc address, from userspace. In OpenBMC land this can be
> >set with obmc-console-client (/etc/obmc-console.conf). Should we add
> >support to that application for setting the irq polarity too, and do
> >away with device tree descriptions?
> >
>
> I guess I might lean slightly toward keeping the DT description so that
> if for whatever reason obmc-console-server flakes out and doesn't start
> you're better positioned to try banging on /dev/ttyS* manually if you're
> desperate. Though I suppose that in turn might imply that I'm arguing
> for adding DT properties for lpc_address and sirq too,

Why not just adopt exactly what I've done with KCS, where we have aspeed,lpc-io-reg and aspeed,lpc-interrupts?

Andrew

2021-04-01 07:38:36

by Zev Weiss

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] drivers/tty/serial/8250: add DT property for aspeed vuart sirq polarity

On Thu, Apr 01, 2021 at 12:34:04AM CDT, Andrew Jeffery wrote:
>
>
>On Thu, 1 Apr 2021, at 15:48, Zev Weiss wrote:
>> On Wed, Mar 31, 2021 at 11:15:44PM CDT, Joel Stanley wrote:
>> >On Thu, 1 Apr 2021 at 00:57, Zev Weiss <[email protected]> wrote:
>> >>
>> >> This provides a simple boolean to use instead of the deprecated
>> >> aspeed,sirq-polarity-sense property.
>> >>
>> >> Signed-off-by: Zev Weiss <[email protected]>
>> >> ---
>> >> drivers/tty/serial/8250/8250_aspeed_vuart.c | 3 +++
>> >> 1 file changed, 3 insertions(+)
>> >>
>> >> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
>> >> index c33e02cbde93..e5ef9f957f9a 100644
>> >> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
>> >> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
>> >> @@ -482,6 +482,9 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
>> >> of_node_put(sirq_polarity_sense_args.np);
>> >> }
>> >>
>> >> + if (of_property_read_bool(np, "aspeed,sirq-active-high"))
>> >> + aspeed_vuart_set_sirq_polarity(vuart, 1);
>> >
>> >This assumes the default is always low, so we don't need a property to
>> >set it to that state?
>> >
>> >Would it make more sense to have the property describe if it's high or
>> >low? (I'm happy for the answer to be "no", as we've gotten by for the
>> >past few years without it).
>> >
>>
>> Yeah, that sounds like better way to approach it -- I think I'll
>> rearrange as Andrew suggested in
>> https://lore.kernel.org/openbmc/[email protected]/
>>
>> >This brings up another point. We already have the sysfs file for
>> >setting the lpc address, from userspace. In OpenBMC land this can be
>> >set with obmc-console-client (/etc/obmc-console.conf). Should we add
>> >support to that application for setting the irq polarity too, and do
>> >away with device tree descriptions?
>> >
>>
>> I guess I might lean slightly toward keeping the DT description so that
>> if for whatever reason obmc-console-server flakes out and doesn't start
>> you're better positioned to try banging on /dev/ttyS* manually if you're
>> desperate. Though I suppose that in turn might imply that I'm arguing
>> for adding DT properties for lpc_address and sirq too,
>
>Why not just adopt exactly what I've done with KCS, where we have aspeed,lpc-io-reg and aspeed,lpc-interrupts?
>
>Andrew

Ah -- yes, that does sound like a sensible approach. I'll send a v3
with that worked in.


Zev

2021-04-01 17:47:57

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] dt-bindings: serial: 8250: add aspeed, sirq-active-high

On Wed, 31 Mar 2021 19:57:02 -0500, Zev Weiss wrote:
> This provides a simpler, more direct alternative to the deprecated
> aspeed,sirq-polarity-sense property for indicating the polarity of
> the Aspeed VUART's SIRQ line.
>
> Signed-off-by: Zev Weiss <[email protected]>
> ---
> Documentation/devicetree/bindings/serial/8250.yaml | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/serial/8250.yaml:16:30: [warning] too few spaces after comma (commas)
./Documentation/devicetree/bindings/serial/8250.yaml:17:30: [warning] too few spaces after comma (commas)

dtschema/dtc warnings/errors:

See https://patchwork.ozlabs.org/patch/1460791

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.