Re: [PATCH 1/7] serial: 8250_dwlib: RS485 HW half duplex support

From: Lukas Wunner <lukas@wunner.de>
To: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>,
	"open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Johan Hovold <johan@kernel.org>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>,
	Raymond Tan <raymond.tan@intel.com>,
	Heiko Stuebner <heiko@sntech.de>
Subject: Re: [PATCH 1/7] serial: 8250_dwlib: RS485 HW half duplex support
Date: Mon, 7 Mar 2022 20:18:54 +0100	[thread overview]
Message-ID: <20220307191854.GA27748@wunner.de> (raw)
In-Reply-To: <ab82f6a-8d1b-8e89-4ea-77d1a55667d2@linux.intel.com>

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 <lukas@wunner.de> 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