The Synopsys DesignWare UART has a build-in support for the
RS485 protocol from IP version 4.0 onward. This commit
enables basic hardware-controlled half duplex mode support
for it.
HW will take care of managing DE and RE, the driver just gives
it permission to use either by setting both to 1.
Co-developed-by: Heikki Krogerus <[email protected]>
Signed-off-by: Heikki Krogerus <[email protected]>
Co-developed-by: Raymond Tan <[email protected]>
Signed-off-by: Raymond Tan <[email protected]>
Co-developed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
Signed-off-by: Ilpo Järvinen <[email protected]>
---
drivers/tty/serial/8250/8250_dwlib.c | 67 +++++++++++++++++++++++++++-
drivers/tty/serial/8250/8250_dwlib.h | 3 ++
2 files changed, 68 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
index 622d3b0d89e7..a4f09a95049b 100644
--- a/drivers/tty/serial/8250/8250_dwlib.c
+++ b/drivers/tty/serial/8250/8250_dwlib.c
@@ -2,19 +2,33 @@
/* Synopsys DesignWare 8250 library. */
#include <linux/bitops.h>
+#include <linux/bitfield.h>
#include <linux/device.h>
#include <linux/io.h>
#include <linux/kernel.h>
+#include <linux/property.h>
#include <linux/serial_8250.h>
#include <linux/serial_core.h>
#include "8250_dwlib.h"
/* Offsets for the DesignWare specific registers */
+#define DW_UART_TCR 0xac /* Transceiver Control Register (RS485) */
+#define DW_UART_DE_EN 0xb0 /* Driver Output Enable Register */
+#define DW_UART_RE_EN 0xb4 /* Receiver Output Enable Register */
#define DW_UART_DLF 0xc0 /* Divisor Latch Fraction Register */
#define DW_UART_CPR 0xf4 /* Component Parameter Register */
#define DW_UART_UCV 0xf8 /* UART Component Version */
+/* Transceiver Control Register bits */
+#define DW_UART_TCR_RS485_EN BIT(0)
+#define DW_UART_TCR_RE_POL BIT(1)
+#define DW_UART_TCR_DE_POL BIT(2)
+#define DW_UART_TCR_XFER_MODE GENMASK(4, 3)
+#define DW_UART_TCR_XFER_MODE_DE_DURING_RE FIELD_PREP(DW_UART_TCR_XFER_MODE, 0)
+#define DW_UART_TCR_XFER_MODE_SW_DE_OR_RE FIELD_PREP(DW_UART_TCR_XFER_MODE, 1)
+#define DW_UART_TCR_XFER_MODE_DE_OR_RE FIELD_PREP(DW_UART_TCR_XFER_MODE, 2)
+
/* Component Parameter Register bits */
#define DW_UART_CPR_ABP_DATA_WIDTH (3 << 0)
#define DW_UART_CPR_AFCE_MODE (1 << 4)
@@ -87,11 +101,62 @@ void dw8250_do_set_termios(struct uart_port *p, struct ktermios *termios, struct
}
EXPORT_SYMBOL_GPL(dw8250_do_set_termios);
+static int dw8250_rs485_config(struct uart_port *p, struct serial_rs485 *rs485)
+{
+ u32 tcr;
+
+ tcr = dw8250_readl_ext(p, DW_UART_TCR);
+ tcr &= ~DW_UART_TCR_XFER_MODE;
+
+ if (rs485->flags & SER_RS485_ENABLED) {
+ /* Clearing unsupported flags. */
+ rs485->flags &= SER_RS485_ENABLED;
+
+ tcr |= DW_UART_TCR_RS485_EN | DW_UART_TCR_XFER_MODE_DE_OR_RE;
+ dw8250_writel_ext(p, DW_UART_DE_EN, 1);
+ dw8250_writel_ext(p, DW_UART_RE_EN, 1);
+ } else {
+ rs485->flags = 0;
+
+ tcr &= ~DW_UART_TCR_RS485_EN;
+ dw8250_writel_ext(p, DW_UART_DE_EN, 0);
+ dw8250_writel_ext(p, DW_UART_RE_EN, 0);
+ }
+
+ /* Resetting the default DE_POL & RE_POL */
+ tcr &= ~(DW_UART_TCR_DE_POL | DW_UART_TCR_RE_POL);
+
+ if (device_property_read_bool(p->dev, "snps,de-active-high"))
+ tcr |= DW_UART_TCR_DE_POL;
+ if (device_property_read_bool(p->dev, "snps,re-active-high"))
+ tcr |= DW_UART_TCR_RE_POL;
+
+ dw8250_writel_ext(p, DW_UART_TCR, tcr);
+
+ /*
+ * XXX: Though we could interpret the "RTS" timings as Driver Enable
+ * (DE) assertion/de-assertion timings, initially not supporting that.
+ * Ideally we should have timing values for the Driver instead of the
+ * RTS signal.
+ */
+ rs485->delay_rts_before_send = 0;
+ rs485->delay_rts_after_send = 0;
+
+ p->rs485 = *rs485;
+
+ return 0;
+}
+
void dw8250_setup_port(struct uart_port *p)
{
+ struct dw8250_port_data *d = p->private_data;
struct uart_8250_port *up = up_to_u8250p(p);
u32 reg;
+ d->hw_rs485_support = device_property_read_bool(p->dev, "snps,rs485-interface-en");
+ if (d->hw_rs485_support)
+ p->rs485_config = dw8250_rs485_config;
+
/*
* If the Component Version Register returns zero, we know that
* ADDITIONAL_FEATURES are not enabled. No need to go any further.
@@ -108,8 +173,6 @@ void dw8250_setup_port(struct uart_port *p)
dw8250_writel_ext(p, DW_UART_DLF, 0);
if (reg) {
- struct dw8250_port_data *d = p->private_data;
-
d->dlf_size = fls(reg);
p->get_divisor = dw8250_get_divisor;
p->set_divisor = dw8250_set_divisor;
diff --git a/drivers/tty/serial/8250/8250_dwlib.h b/drivers/tty/serial/8250/8250_dwlib.h
index 83d528e5cc21..a8fa020ca544 100644
--- a/drivers/tty/serial/8250/8250_dwlib.h
+++ b/drivers/tty/serial/8250/8250_dwlib.h
@@ -14,6 +14,9 @@ struct dw8250_port_data {
/* Hardware configuration */
u8 dlf_size;
+
+ /* RS485 variables */
+ bool hw_rs485_support;
};
void dw8250_do_set_termios(struct uart_port *p, struct ktermios *termios, struct ktermios *old);
--
2.30.2
On Wed, Mar 02, 2022 at 11:56:00AM +0200, Ilpo J?rvinen wrote:
> +static int dw8250_rs485_config(struct uart_port *p, struct serial_rs485 *rs485)
> +{
> + u32 tcr;
> +
> + tcr = dw8250_readl_ext(p, DW_UART_TCR);
> + tcr &= ~DW_UART_TCR_XFER_MODE;
> +
> + if (rs485->flags & SER_RS485_ENABLED) {
> + /* Clearing unsupported flags. */
Nit: Usually we use imperative mood, i.e. "/* clear unsupported flags */".
> + rs485->flags &= SER_RS485_ENABLED;
> +
> + tcr |= DW_UART_TCR_RS485_EN | DW_UART_TCR_XFER_MODE_DE_OR_RE;
> + dw8250_writel_ext(p, DW_UART_DE_EN, 1);
> + dw8250_writel_ext(p, DW_UART_RE_EN, 1);
> + } else {
> + rs485->flags = 0;
> +
> + tcr &= ~DW_UART_TCR_RS485_EN;
> + dw8250_writel_ext(p, DW_UART_DE_EN, 0);
> + dw8250_writel_ext(p, DW_UART_RE_EN, 0);
Do the DW_UART_DE_EN and DW_UART_RE_EN registers have any effect at all
if DW_UART_TCR_RS485_EN is disabled in the TCR register?
If they don't, there's no need to clear them here. It would be sufficient
to set them once (e.g. on probe).
> + /* Resetting the default DE_POL & RE_POL */
> + tcr &= ~(DW_UART_TCR_DE_POL | DW_UART_TCR_RE_POL);
Nit: Imperative mood, i.e. "/* reset to default polarity */"
> + if (device_property_read_bool(p->dev, "snps,de-active-high"))
> + tcr |= DW_UART_TCR_DE_POL;
That device property is a duplication of the existing "rs485-rts-active-low"
property. Please use the existing one unless there are devices already
in the field which use the new property (in which case that should be
provided as justification in the commit message).
Does the DesignWare UART use dedicated DE and RE pins instead of
the RTS pin? That would be quite unusual.
> + if (device_property_read_bool(p->dev, "snps,re-active-high"))
> + tcr |= DW_UART_TCR_RE_POL;
Heiko St?bner (+cc) posted patches in 2020 to support a separate RE pin
in addition to a DE pin (which is usually simply the RTS pin):
https://lore.kernel.org/linux-serial/[email protected]/
He called the devicetree property for the pin "rs485-rx-enable",
so perhaps "rs485-rx-active-low" would be a better name here.
> + /*
> + * XXX: Though we could interpret the "RTS" timings as Driver Enable
> + * (DE) assertion/de-assertion timings, initially not supporting that.
> + * Ideally we should have timing values for the Driver instead of the
> + * RTS signal.
> + */
> + rs485->delay_rts_before_send = 0;
> + rs485->delay_rts_after_send = 0;
I don't quite understand this code comment.
> void dw8250_setup_port(struct uart_port *p)
> {
> + struct dw8250_port_data *d = p->private_data;
> struct uart_8250_port *up = up_to_u8250p(p);
> u32 reg;
>
> + d->hw_rs485_support = device_property_read_bool(p->dev, "snps,rs485-interface-en");
> + if (d->hw_rs485_support)
> + p->rs485_config = dw8250_rs485_config;
> +
You wrote in the commit message that rs485 support is present from
version 4.0 onward. Can't we just check the IP version and enable
rs485 support for >= 4.0? That would seem more appropriate instead
of introducing yet another new property.
Note that dw8250_setup_port() already reads the version from the
DW_UART_UCV register.
Thanks,
Lukas
On Mon, Mar 7, 2022 at 12:00 AM Lukas Wunner <[email protected]> wrote:
> On Wed, Mar 02, 2022 at 11:56:00AM +0200, Ilpo Järvinen wrote:
...
> Does the DesignWare UART use dedicated DE and RE pins instead of
> the RTS pin? That would be quite unusual.
They are muxed with other UART pins on SoC level, but I don't remember
by heart which ones. According to the Synopsys datasheet they are
separate signals. It might be that I'm missing something, since the
last time I looked was last year.
...
> > + d->hw_rs485_support = device_property_read_bool(p->dev, "snps,rs485-interface-en");
> > + if (d->hw_rs485_support)
> > + p->rs485_config = dw8250_rs485_config;
> > +
>
> You wrote in the commit message that rs485 support is present from
> version 4.0 onward. Can't we just check the IP version and enable
> rs485 support for >= 4.0? That would seem more appropriate instead
> of introducing yet another new property.
AFAIU this is dependent on the IP syntheses. I.o.w. version 4.0+ is a
prerequisite, but doesn't automatically mean that there is a support.
Unfortunately there is no way to tell this clearly in the IP
configuration register.
--
With Best Regards,
Andy Shevchenko
On Mon, 7 Mar 2022, Andy Shevchenko wrote:
> On Mon, Mar 7, 2022 at 12:00 AM Lukas Wunner <[email protected]> wrote:
> > On Wed, Mar 02, 2022 at 11:56:00AM +0200, Ilpo Järvinen wrote:
>
> ...
>
> > Does the DesignWare UART use dedicated DE and RE pins instead of
> > the RTS pin? That would be quite unusual.
>
> They are muxed with other UART pins on SoC level, but I don't remember
> by heart which ones. According to the Synopsys datasheet they are
> separate signals. It might be that I'm missing something, since the
> last time I looked was last year.
Unusual or not, there is a pin for both DE and RE. DE is muxed with RTS.
> > > + d->hw_rs485_support = device_property_read_bool(p->dev, "snps,rs485-interface-en");
> > > + if (d->hw_rs485_support)
> > > + p->rs485_config = dw8250_rs485_config;
> > > +
> >
> > You wrote in the commit message that rs485 support is present from
> > version 4.0 onward. Can't we just check the IP version and enable
> > rs485 support for >= 4.0? That would seem more appropriate instead
> > of introducing yet another new property.
>
> AFAIU this is dependent on the IP syntheses. I.o.w. version 4.0+ is a
> prerequisite, but doesn't automatically mean that there is a support.
> Unfortunately there is no way to tell this clearly in the IP
> configuration register.
And the IP synthesis only part of the picture, in general case, it'd
also matter that there's something connected to that RE (i.e.,
an RS485 transceiver).
On the board I'm testing with, I can also turn RS485 on/off from BIOS
which makes the pins (mainly RE) behave differently.
I initially had additional version check here while developing this
patch series but it seemed to not provide any added value due those
other factors that need to be considered.
--
i.
On Sun, 6 Mar 2022, Lukas Wunner wrote:
> On Wed, Mar 02, 2022 at 11:56:00AM +0200, Ilpo J?rvinen wrote:
>
> > + rs485->flags &= SER_RS485_ENABLED;
> > +
> > + tcr |= DW_UART_TCR_RS485_EN | DW_UART_TCR_XFER_MODE_DE_OR_RE;
> > + dw8250_writel_ext(p, DW_UART_DE_EN, 1);
> > + dw8250_writel_ext(p, DW_UART_RE_EN, 1);
> > + } else {
> > + rs485->flags = 0;
> > +
> > + tcr &= ~DW_UART_TCR_RS485_EN;
> > + dw8250_writel_ext(p, DW_UART_DE_EN, 0);
> > + dw8250_writel_ext(p, DW_UART_RE_EN, 0);
>
> Do the DW_UART_DE_EN and DW_UART_RE_EN registers have any effect at all
> if DW_UART_TCR_RS485_EN is disabled in the TCR register?
>
> If they don't, there's no need to clear them here. It would be sufficient
> to set them once (e.g. on probe).
They have no impact when in non-RS485 mode. I just removed them.
> > + if (device_property_read_bool(p->dev, "snps,de-active-high"))
> > + tcr |= DW_UART_TCR_DE_POL;
>
> That device property is a duplication of the existing "rs485-rts-active-low"
> property. Please use the existing one unless there are devices already
> in the field which use the new property (in which case that should be
> provided as justification in the commit message).
>
> Does the DesignWare UART use dedicated DE and RE pins instead of
> the RTS pin? That would be quite unusual.
>
> > + if (device_property_read_bool(p->dev, "snps,re-active-high"))
> > + tcr |= DW_UART_TCR_RE_POL;
>
> Heiko St?bner (+cc) posted patches in 2020 to support a separate RE pin
> in addition to a DE pin (which is usually simply the RTS pin):
>
> https://lore.kernel.org/linux-serial/[email protected]/
>
> He called the devicetree property for the pin "rs485-rx-enable",
> so perhaps "rs485-rx-active-low" would be a better name here.
While I believe there exist devices on the field with
snps,re-active-high set to true, if the default matches to that, the
impact of the naming mismatch will be near zero (likely zero).
Based on the Rob's earlier comment on the dt patch itself. I had already
plans on changing these. My thought was to make it like this:
- rs485-de-active-low
- rs485-re-active-high
I don't have strong opinion on the actual names myself (every RS-485
transceivers I've come across name their pins to DE and RE).
Given that you seemed to consider DE "unusual" despite being reality
with this hw, I don't know whether you still think the meaning of
rs485-rts-active-low should be overloaded to also mean rs485-de-active-low?
(I think such overloading would be harmless so I'm not exactly opposing
other than noting FW/HW folks might find it odd to misname it to rts.)
What I think is more important though, is that RE would be by default
active low which matches to about every RS485 transceiver expectations.
Given what device_property_read_bool does when the property is missing,
it would make sense to have the property named as -active-high but I
don't know if that breaks some dt naming rule to have them opposites
of each other like that?
> > + /*
> > + * XXX: Though we could interpret the "RTS" timings as Driver Enable
> > + * (DE) assertion/de-assertion timings, initially not supporting that.
> > + * Ideally we should have timing values for the Driver instead of the
> > + * RTS signal.
> > + */
> > + rs485->delay_rts_before_send = 0;
> > + rs485->delay_rts_after_send = 0;
>
> I don't quite understand this code comment.
It seemed to be missing one "Enable" word.
Here's my interpretation of it (this comment was written by somebody
else, perhaps either Heikki or Andy):
Since this HW has dedicated DE/RE in contrast to RTS, it is not specified
anywhere that delay_rts_* should apply to them as well and that the
writer of that comment was hoping to have something dedicated to them
rather than repurposing RTS-related fields.
I could of course change this if everything called RTS should be applied
to DE as well?
--
i.
On Mon, 7 Mar 2022, Andy Shevchenko wrote:
> On Mon, Mar 07, 2022 at 08:18:54PM +0100, Lukas Wunner wrote:
> > On Mon, Mar 07, 2022 at 11:19:59AM +0200, Ilpo J?rvinen wrote:
> > > On Mon, 7 Mar 2022, Andy Shevchenko wrote:
>
> ...
>
> > That's for DT platforms, but I suppose you've got ACPI. Not sure
> > how it's handled there, the ACPI 6.4 spec contains a "UART Serial Bus
> > Connection Resource Descriptor" but nothing on RS-485, so I guess
> > the only option is to use regular DT properties in a _DSD object?
>
> Which make me think that this series needs an additional patch to
> describe RS485 enumeration for ACPI case (somewhere in
> Documentation/firmware-guide/acpi/enumeration.rst IIRC the filename).
>
> ...
>
> > > I initially had additional version check here while developing this
> > > patch series but it seemed to not provide any added value due those
> > > other factors that need to be considered.
> >
> > Here's another idea:
> >
> > Read TCR register on ->probe. It's POR value is 0x6 if RS-485 is
> > supported by the chip, else 0x0. (Page 220 of the 4.01a spec says
> > UCV register does not exist if additional features are not implemented
> > and reading from this register address returns 0, I suppose the same
> > applies to TCR if RS-485 is not implemented.)
> >
> > Since the driver may change the polarity in the TCR register, be sure
> > to write 0x6 to it on ->remove so that you can still correctly detect
> > presence of the RS-485 feature after unbind/rebind of the driver.
>
> What to do in the case when DE pin is muxed to RTS and locked in pin control
> IP by firmware (no possibility to change the muxing in the OS)?
The SoC also has a pin to select between RS485 and RS232. With a combo
transceiver, TCR-based heuristic just runs into the same problems as the
version-based one did.
--
i.
On Mon, Mar 07, 2022 at 11:19:59AM +0200, Ilpo J?rvinen wrote:
> On Mon, 7 Mar 2022, Andy Shevchenko wrote:
> > On Mon, Mar 7, 2022 at 12:00 AM Lukas Wunner <[email protected]> wrote:
> > > On Wed, Mar 02, 2022 at 11:56:00AM +0200, Ilpo J?rvinen wrote:
> > > > + d->hw_rs485_support = device_property_read_bool(p->dev, "snps,rs485-interface-en");
> > > > + if (d->hw_rs485_support)
> > > > + p->rs485_config = dw8250_rs485_config;
> > > > +
> > >
> > > You wrote in the commit message that rs485 support is present from
> > > version 4.0 onward. Can't we just check the IP version and enable
> > > rs485 support for >= 4.0? That would seem more appropriate instead
> > > of introducing yet another new property.
> >
> > AFAIU this is dependent on the IP syntheses. I.o.w. version 4.0+ is a
> > prerequisite, but doesn't automatically mean that there is a support.
> > Unfortunately there is no way to tell this clearly in the IP
> > configuration register.
>
> And the IP synthesis only part of the picture, in general case, it'd
> also matter that there's something connected to that RE (i.e.,
> an RS485 transceiver).
If an RS-485 transceiver is *soldered* to the UART, the devicetree
is supposed to contain the property "linux,rs485-enabled-at-boot-time"
under the UART's of_node. In that case the UART driver can (and should)
enable rs485 mode already on ->probe.
Of course there's also the possibility to enable RS-485 after ->open
with the TIOCSRS485 ioctl. That can be used if the transceiver is
attached at runtime (which is likely a rare use case) or as a legacy
enablement method if the driver lacks linux,rs485-enabled-at-boot-time
support.
That's for DT platforms, but I suppose you've got ACPI. Not sure
how it's handled there, the ACPI 6.4 spec contains a "UART Serial Bus
Connection Resource Descriptor" but nothing on RS-485, so I guess
the only option is to use regular DT properties in a _DSD object?
> I initially had additional version check here while developing this
> patch series but it seemed to not provide any added value due those
> other factors that need to be considered.
Here's another idea:
Read TCR register on ->probe. It's POR value is 0x6 if RS-485 is
supported by the chip, else 0x0. (Page 220 of the 4.01a spec says
UCV register does not exist if additional features are not implemented
and reading from this register address returns 0, I suppose the same
applies to TCR if RS-485 is not implemented.)
Since the driver may change the polarity in the TCR register, be sure
to write 0x6 to it on ->remove so that you can still correctly detect
presence of the RS-485 feature after unbind/rebind of the driver.
Thanks,
Lukas
On Tue, 8 Mar 2022, Lukas Wunner wrote:
> On Tue, Mar 08, 2022 at 02:16:56PM +0200, Ilpo J?rvinen wrote:
> > The SoC also has a pin to select between RS485 and RS232. With a combo
> > transceiver, TCR-based heuristic just runs into the same problems as the
> > version-based one did.
>
> I thought this was about detecting whether hardware-assisted DE assertion
> may be used (versus software-controlled), not about whether to enable
> RS-485 mode. Right?
HW DE assertion only works when RS485 mode is enabled so I don't see how
these questions could be easily decoupled like that. That's assuming with
"software-controlled" you mean RTS(RS232)+em485?
--
i.
On Tue, Mar 08, 2022 at 04:53:56PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 8, 2022 at 4:50 PM Lukas Wunner <[email protected]> wrote:
> > Of course, if hardware-assisted DE assertion requires a particular pinmux
> > state, we could double-check whether that pinmux state is set.
>
> I'm wondering how to achieve this.
On DT platforms, the devicetree specifies the pin controller settings
which need to be configured for a device to be usable, e.g.:
pinctrl-names = "default";
pinctrl-0 = <...>;
Before a driver is bound to the device, really_probe() in drivers/base/dd.c
calls pinctrl_bind_pins() which configures the pin controller accordingly.
In other words, the OS is fully in charge of configuring the pinmux.
I'm not sure how this is done on ACPI platforms. If the pinmux is
exclusively under the control of the platform firmware and the OS has
no way of getting or setting the pinmux configuration, then that would
be a competitive disadvantage vis-?-vis DT platforms which should really
be addressed. However I notice there are various drivers for Intel
chipsets in drivers/pinctrl/intel/, so surely there's a way to let the
OS handle pinmux settings?
Thanks,
Lukas
On Tue, Mar 8, 2022 at 4:50 PM Lukas Wunner <[email protected]> wrote:
> On Tue, Mar 08, 2022 at 02:59:59PM +0200, Ilpo Järvinen wrote:
...
> Of course, if hardware-assisted DE assertion requires a particular pinmux
> state, we could double-check whether that pinmux state is set.
I'm wondering how to achieve this.
--
With Best Regards,
Andy Shevchenko
On Tue, Mar 08, 2022 at 02:59:59PM +0200, Ilpo J?rvinen wrote:
> On Tue, 8 Mar 2022, Lukas Wunner wrote:
> > On Tue, Mar 08, 2022 at 02:16:56PM +0200, Ilpo J?rvinen wrote:
> > > The SoC also has a pin to select between RS485 and RS232. With a combo
> > > transceiver, TCR-based heuristic just runs into the same problems as the
> > > version-based one did.
> >
> > I thought this was about detecting whether hardware-assisted DE assertion
> > may be used (versus software-controlled), not about whether to enable
> > RS-485 mode. Right?
>
> HW DE assertion only works when RS485 mode is enabled so I don't see how
> these questions could be easily decoupled like that. That's assuming with
> "software-controlled" you mean RTS(RS232)+em485?
Right, that's what I meant.
Enabling RS-485 mode is only supposed to happen upon a TIOCSRS485 ioctl
or if the "linux,rs485-enabled-at-boot-time" property is present.
We don't need to second-guess the user's decision to enable RS-485 mode.
If that's what they've asked for, then we can and should assume that an
RS-485 transceiver is attached.
Of course, if hardware-assisted DE assertion requires a particular pinmux
state, we could double-check whether that pinmux state is set. If the
RTS/DE pin is not muxed as a DE pin but rather as an RTS pin, one option
would be to fall back to software-controlled RTS assertion. A warning
message may be warranted in that case.
Whether hardware-assisted DE assertion is supported by the chip can not
only be detected by checking for the POR 0x6 value in the TCR register:
You can alternatively write a non-zero value to any of the RS-485 registers,
then check if reading the register back returns a non-zero value
(RE_EN is probably a good candidate). That approach is more robust
than relying on the POR value 0x6 in TCR because you never know if
boot firmware fiddled with the registers before passing control to the
kernel.
Thanks,
Lukas
On Mon, Mar 07, 2022 at 08:18:54PM +0100, Lukas Wunner wrote:
> On Mon, Mar 07, 2022 at 11:19:59AM +0200, Ilpo J?rvinen wrote:
> > On Mon, 7 Mar 2022, Andy Shevchenko wrote:
...
> That's for DT platforms, but I suppose you've got ACPI. Not sure
> how it's handled there, the ACPI 6.4 spec contains a "UART Serial Bus
> Connection Resource Descriptor" but nothing on RS-485, so I guess
> the only option is to use regular DT properties in a _DSD object?
Which make me think that this series needs an additional patch to
describe RS485 enumeration for ACPI case (somewhere in
Documentation/firmware-guide/acpi/enumeration.rst IIRC the filename).
...
> > I initially had additional version check here while developing this
> > patch series but it seemed to not provide any added value due those
> > other factors that need to be considered.
>
> Here's another idea:
>
> Read TCR register on ->probe. It's POR value is 0x6 if RS-485 is
> supported by the chip, else 0x0. (Page 220 of the 4.01a spec says
> UCV register does not exist if additional features are not implemented
> and reading from this register address returns 0, I suppose the same
> applies to TCR if RS-485 is not implemented.)
>
> Since the driver may change the polarity in the TCR register, be sure
> to write 0x6 to it on ->remove so that you can still correctly detect
> presence of the RS-485 feature after unbind/rebind of the driver.
What to do in the case when DE pin is muxed to RTS and locked in pin control
IP by firmware (no possibility to change the muxing in the OS)?
--
With Best Regards,
Andy Shevchenko
On Tue, Mar 08, 2022 at 02:16:56PM +0200, Ilpo J?rvinen wrote:
> The SoC also has a pin to select between RS485 and RS232. With a combo
> transceiver, TCR-based heuristic just runs into the same problems as the
> version-based one did.
I thought this was about detecting whether hardware-assisted DE assertion
may be used (versus software-controlled), not about whether to enable
RS-485 mode. Right?
Thanks,
Lukas
On Mon, Mar 07, 2022 at 12:54:19PM +0200, Ilpo J?rvinen wrote:
> I don't have strong opinion on the actual names myself (every RS-485
> transceivers I've come across name their pins to DE and RE).
It's true that transceiver datasheets usually call these pins DE and RE,
but on the UART side, by convention the RTS pin is used to drive DE.
And RTS is then either asserted/deasserted in software (by the kernel
driver), or by the UART hardware if it's capable of it.
Hence the properties in Documentation/devicetree/bindings/serial/rs485.yaml
and the members in struct serial_rs485 refer to "rts". It's synonymous
to DE. I suppose Synopsys wanted to afford the integrator of the IP core
as much freedom as possible and therefore offers separate RTS+DE pins
as well as an RE pin. But that degree of freedom also leads to confusion,
particularly if firmware might mux the pins in an unexpected way
behind the OS's back.
The RE pin of transceivers is usually either pulled-up (i.e. always
asserted, full-duplex) or connected to negated RTS (half-duplex).
A lot of transceivers have a !RE pin so RTS can be wired directly
to DE and !RE.
Full-duplex is primarily for RS-422. If full-duplex is enabled with
RS-485, you'll receive your own echo, which is often (but not always)
undesirable (depends on the application).
For simplicity and consistency, it is best if the existing properties
defined in rs485.yaml are used, instead of defining new ones which
have the same meaning. For this reason I'd recommend using
rs485-rts-active-low for the polarity of the DE pin.
> What I think is more important though, is that RE would be by default
> active low which matches to about every RS485 transceiver expectations.
> Given what device_property_read_bool does when the property is missing,
> it would make sense to have the property named as -active-high but I
> don't know if that breaks some dt naming rule to have them opposites
> of each other like that?
That's a good point, I agree. I don't think that would violate any
DT naming rule.
Thanks,
Lukas
On Tue, 8 Mar 2022, Lukas Wunner wrote:
> On Tue, Mar 08, 2022 at 04:53:56PM +0200, Andy Shevchenko wrote:
> > On Tue, Mar 8, 2022 at 4:50 PM Lukas Wunner <[email protected]> wrote:
> > > Of course, if hardware-assisted DE assertion requires a particular pinmux
> > > state, we could double-check whether that pinmux state is set.
> >
> > I'm wondering how to achieve this.
>
> On DT platforms, the devicetree specifies the pin controller settings
> which need to be configured for a device to be usable, e.g.:
>
> pinctrl-names = "default";
> pinctrl-0 = <...>;
>
> Before a driver is bound to the device, really_probe() in drivers/base/dd.c
> calls pinctrl_bind_pins() which configures the pin controller accordingly.
> In other words, the OS is fully in charge of configuring the pinmux.
>
> I'm not sure how this is done on ACPI platforms. If the pinmux is
> exclusively under the control of the platform firmware and the OS has
> no way of getting or setting the pinmux configuration, then that would
> be a competitive disadvantage vis-?-vis DT platforms which should really
> be addressed. However I notice there are various drivers for Intel
> chipsets in drivers/pinctrl/intel/, so surely there's a way to let the
> OS handle pinmux settings?
The problem here is that the driver ("we could double-check" in your
initial suggestion above) doesn't know which pins it should check the
states for. I don't think any general mapping exists between drivers and
pins.
Based on what I read, the mapping is a feature not wanted into pinmuxing.
Assuming I understood them correctly, they don't want to do such thing
on kernel side (based on experience with earlier approaches with
mapping. It probably got too messy/unmaintainable in the end :-)).
So at best, one can only read and control pin mux states but that's about
as far as pinmuxing in kernel goes (and control could be locked by FW).
Anyway, I've implemented the detection now based on RE_EN non-zero write +
read + check based on your suggestion (despite still thinking myself it
has these obvious problems with pinmux & other hw config unrelated dw uart
itself).
--
i.
On Mon, 7 Mar 2022, Ilpo J?rvinen wrote:
> On Sun, 6 Mar 2022, Lukas Wunner wrote:
>
> > On Wed, Mar 02, 2022 at 11:56:00AM +0200, Ilpo J?rvinen wrote:
>
> > > + /*
> > > + * XXX: Though we could interpret the "RTS" timings as Driver Enable
> > > + * (DE) assertion/de-assertion timings, initially not supporting that.
> > > + * Ideally we should have timing values for the Driver instead of the
> > > + * RTS signal.
> > > + */
> > > + rs485->delay_rts_before_send = 0;
> > > + rs485->delay_rts_after_send = 0;
> >
> > I don't quite understand this code comment.
>
> It seemed to be missing one "Enable" word.
>
> Here's my interpretation of it (this comment was written by somebody
> else, perhaps either Heikki or Andy):
>
> Since this HW has dedicated DE/RE in contrast to RTS, it is not specified
> anywhere that delay_rts_* should apply to them as well and that the
> writer of that comment was hoping to have something dedicated to them
> rather than repurposing RTS-related fields.
>
> I could of course change this if everything called RTS should be applied
> to DE as well?
Now that it has been pretty much established that anything called "rts"
should be applied to DE as well, I took another look on implementing these
delays.
It turns out to be impractical to do/ineffective because "serial clock
periods" are used as the unit by the HW ("serial clock periods" is not as
clearly defined by the datasheet as I'd like but it is most likely based
on the high-rated uartclk cycles). With the uartclk I've on test HW, the
combined delay with max turnaround time and DE assert/de-assert timings
cannot do even the smallest possible non-zero value (1 msec). That's
because the TAT and DET registers allow only 16-bit and 8-bit values for
delays.
I also attempted to test it by writing the maximum values into them and
got no visible difference. There a note about +1 for delay in TAT so to
play safe I put 0xfff0 instead 0xffff (if the HW couldn't handle that
16-bit overflow properly).
Perhaps the SW half-duplex with DE/RE will have to be used to cover this
case?
--
i.
On Wed, Mar 09, 2022 at 02:19:39PM +0200, Ilpo J?rvinen wrote:
> Now that it has been pretty much established that anything called "rts"
> should be applied to DE as well, I took another look on implementing these
> delays.
>
> It turns out to be impractical to do/ineffective because "serial clock
> periods" are used as the unit by the HW ("serial clock periods" is not as
> clearly defined by the datasheet as I'd like but it is most likely based
> on the high-rated uartclk cycles). With the uartclk I've on test HW, the
> combined delay with max turnaround time and DE assert/de-assert timings
> cannot do even the smallest possible non-zero value (1 msec). That's
> because the TAT and DET registers allow only 16-bit and 8-bit values for
> delays.
A mistake was made when RS-485 support was introduced in the kernel
more than 10 years ago with commits c26c56c0 and 93f3350c:
The delays were defined in msec, but if you look at datasheets of
RS-485 transceivers, they only need a delay in the nanosecond or
single-digit usec range.
Here's a collection of delays I compiled two years ago:
DrvEnable-to-Output DriverPropagation
MAX13450E/MAX13451E 5200 ns 800 ns
MAXM22510 2540 ns 1040 ns
MAXM22511 80 ns 65 ns
SN65HVD72 9000 ns 1000 ns
SN65HVD75 7000 ns 17 ns
SN65HVD78 8000 ns 15 ns
SN65HVD485E 2600 ns 30 ns
ADM1486 15 ns 17 ns
ADM3485/ADM3490/ADM3491 900 ns 35 ns
ADM3483/ADM3488 1300 ns 1500 ns
XR33193 2000 ns 1300 ns
Because these delays are so short, it is usually sufficient to set
them to zero in struct serial_rs485.
I've begun a commit to change the delays to nsec, it's still a WIP:
https://github.com/l1k/linux/commit/2c08878b63d6
This is a little tricky because the delays are user-space ABI,
so great care is needed to avoid breakage. Also, every rs485
driver with support for delays needs to be touched. Some
UARTs have fixed delays which depend on different clocks,
other UARTs support configurable delays. Another complication
is that delay calculations easily overflow with nsec because
the numbers become quite large.
A positive side effect of changing the delays to nsec is that
the horrible hrtimer kludge for rs485 software emulation in the
8250_port.c can be eliminated. Also, all the illegal mdelay()s
in spinlocked context (e.g. serial console output) are replaced
by much more reasonable ndelay()s.
Eliminating the hrtimer kludge in 8250_port.c might also make these
runtime PM patches by Andy simpler:
https://lore.kernel.org/linux-serial/[email protected]/
My suggestion would be to set the delays to zero for now in 8250_dw.c
and implement proper delay handling after I've finished the conversion
to nsec.
Thanks,
Lukas