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

From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Lukas Wunner <lukas@wunner.de>
Cc: linux-serial <linux-serial@vger.kernel.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	LKML <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>, Rob Herring <robh@kernel.org>
Subject: Re: [PATCH 1/7] serial: 8250_dwlib: RS485 HW half duplex support
Date: Mon, 7 Mar 2022 12:54:19 +0200 (EET)	[thread overview]
Message-ID: <c2607267-798b-d7a0-86f6-4a729c22911f@linux.intel.com> (raw)
In-Reply-To: <20220306184857.GA19394@wunner.de>

[-- Attachment #1: Type: text/plain, Size: 3920 bytes --]

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/20200517215610.2131618-4-heiko@sntech.de/
> 
> 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.