From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Subject: Re: [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals Date: Fri, 7 Nov 2014 12:02:16 +0100 Message-ID: <201411071202.16344.marex@denx.de> References: <1412960007-28159-1-git-send-email-j.uzycki@elproma.com.pl> <201411070903.11645.marex@denx.de> <545C992B.1070004@elproma.com.pl> Mime-Version: 1.0 Content-Type: Text/Plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mail-out.m-online.net ([212.18.0.9]:44036 "EHLO mail-out.m-online.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751281AbaKGLDP convert rfc822-to-8bit (ORCPT ); Fri, 7 Nov 2014 06:03:15 -0500 In-Reply-To: <545C992B.1070004@elproma.com.pl> Sender: linux-serial-owner@vger.kernel.org List-Id: linux-serial@vger.kernel.org To: Janusz =?utf-8?q?U=C5=BCycki?= Cc: Huang Shijie , Richard Genoud , Greg Kroah-Hartman , Russell King - ARM Linux , Jiri Slaby , Fabio Estevam , "linux-serial@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Alexandre Belloni On Friday, November 07, 2014 at 11:04:27 AM, Janusz U=C5=BCycki wrote: > W dniu 2014-11-07 o 09:03, Marek Vasut pisze: > > On Friday, November 07, 2014 at 02:34:31 AM, Huang Shijie wrote: > >> why change them to gpio? +CC Alexandre, since he might be interested :-) > Hardware RTS/CTS lines can be occupied by RX/TX of other AUART port > in order to obtain as much uarts as possible using i.mx283. > Therefore gpios can be used for "hardware" flow control. Your logic is outright flawed here, the first sentence doesn't implicat= e the=20 second sentence. In fact, those two are completely unrelated. > >> If we change them to gpio. Could the DMA still > >>=20 > >> works fine? > >> did you test the DMA with this patch? > >>=20 > >> Add Marek for this patch too. > >=20 > > I didn't look too deep into the patch, so here's just my experience= : > >=20 > > 1) The AUART block signals and GPIO block signals are not sychronis= ed > > using the > >=20 > > same clock. Therefore, the latency between toggling of the AUAR= T > > lines and the GPIO-driven pins will not be deterministic and wi= ll > > vary. There might be a way to approximate that, but that's > > definitelly not a reliable solution. > > =20 > > This is very bad for example if you drive RS485 DIR line with t= he RTS > > pin as a GPIO ; the RTS pin will toggle at non-deterministic ti= me > > compared to the end of UART transmission. This will trigger bit= -loss > > on the RS485 line and you just don't want that. > >=20 > > 2) Speaking of RS485, there's [1] and [2]. which I believe apply to= any > > combo > >=20 > > of UART+GPIO toggling. > >=20 > > So I hate to bring the bad news , but UART+GPIO combo toggling is r= eally > > a bad bad idea. >=20 > Unfortunately if hardware is limited there is no choice and UART+GPIO= is > necessary. You will run into timing problems (see above). What you're proposing here is a workaround for broken hardware, which w= as proven to be a bad idea and NAK'd already multiple times in the past (please s= ee the=20 links I posted in my last email). > Your experience confirms the discussion [3] with Russell King. DMA > should be disabled and > the patch disables DMA support in mxs_auart_init_gpios() if RTS or CT= S > line is set as gpio. DMA has nothing to do with those problems here. DMA can be safely ignor= ed for the purpose of the discussion altogether. Like I explained already -- the problem is with the GPIO and AUART bloc= k not being synchronised by the same clock. It is therefore impossible to pre= dict and control when the GPIO signals and the AUART signals will toggle in = relation to one another. > So I didn't test the patch with the DMA - I've just disabled it. This would break the driver for pretty much everyone using higher baud = rates. > The question is different. The driver supports the cases for RTS/CTS: > 1) both RTS/CTS assigned to hardware AUART (pinmux configured by DT, > DT sets fsl,uart-has-rtscts) > a) settermios() sets CRTSCTS: DMA enabled, auto RTS/CTS is enabl= ed > b) settermios() doesn't set CRTSCTS: DMA disabled, RTS/CTS > controlled by get/set_mctrl() > 2) both RTS/CTS assigned to hardware AUART (pinmux configured by DT, > DT doesn't set fsl,uart-has-rtscts) > a) settermios() sets CRTSCTS: DMA disabled, auto RTS/CTS is enab= led > b) settermios() doesn't set CRTSCTS: DMA disabled, RTS/CTS > controlled by get/set_mctrl() >=20 > and after the patch it is more complex (because of mixed cases): > 3) both RTS/CTS assigned to gpios (DT sets or not fsl,uart-has-rtscts= ): > the patch cancels fsl,uart-has-rtscts flag to disable DMA suppor= t, > settermios() sets or not CRTSCTS: DMA disabled, RTS/CTS controll= ed > by get/set_mctrl(), > both lines by gpios. In addition: > a) settermios() doesn't set CRTSCTS: RTS of hardware AUART is al= so > controlled but > the case assumes it is not selected by pinmux in DT > b) settermios() sets CRTSCTS: auto RTS of hardware AUART is also > enabled but > the case assumes it is not selected by pinmux in DT > 4) RTS assigned to hardware AUART (pinmux configured by DT, > fsl,uart-has-rtscts set or not), > CTS assigned to gpio: the patch cancels fsl,uart-has-rtscts flag= to > disable DMA support, > a) settermios() doesn't set CRTSCTS: DMA disabled, CTS (as gpio) > read by get_mctrl(), > RTS of hardware AUART controlled by set_mctrl() > b) settermios() sets CRTSCTS: DMA disabled, CTS (as gpio) read b= y > get_mctrl(), > auto RTS is enabled > So case 4) is exactly 3) case with just different pinmux > configuration and the patch > threats 3) and 4) as the same case. > 5) CTS assigned to hardware AUART (pinmux configured by DT, > fsl,uart-has-rtscts set or not), > RTS assigned to gpio: the patch cancels fsl,uart-has-rtscts flag= to > disable DMA support, > a) settermios() doesn't set CRTSCTS: DMA disabled, CTS of hardwa= re > AUART read by > get_mctrl(), RTS (as gpio) controlled by set_mctrl(), in > addition RTS of hardware AUART > is also controlled but the case assumes it is not selected b= y > pinmux in DT > b) settermios() sets CRTSCTS: DMA disabled, auto RTS/CTS is enab= led > but RTS is controlled by set_mctrl() as gpio because the case assumes= it > is not selected > by pinmux in DT >=20 > It is not nice description but I hadn't idea how to write it more cle= ar. > The mixed cases 4) and 5) are likely not so useful but possible and n= ot > so expensive > to support. > Now the question: "fsl,uart-has-rtscts" name seems to be misleading n= ow, > do you agree? It rather should include "dma" word in the name. Any > suggestion? >=20 > [3] http://thread.gmane.org/gmane.linux.serial/16069/focus=3D16077 The best suggestion I can give you is to fix your hardware early, befor= e you run into nasty deep s.....tuff. These workarounds do not work and they = will bit you later on, when it's hard to fix the hardware anymore. -- To unsubscribe from this list: send the line "unsubscribe linux-serial"= in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: marex@denx.de (Marek Vasut) Date: Fri, 7 Nov 2014 12:02:16 +0100 Subject: [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals In-Reply-To: <545C992B.1070004@elproma.com.pl> References: <1412960007-28159-1-git-send-email-j.uzycki@elproma.com.pl> <201411070903.11645.marex@denx.de> <545C992B.1070004@elproma.com.pl> Message-ID: <201411071202.16344.marex@denx.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Friday, November 07, 2014 at 11:04:27 AM, Janusz U?ycki wrote: > W dniu 2014-11-07 o 09:03, Marek Vasut pisze: > > On Friday, November 07, 2014 at 02:34:31 AM, Huang Shijie wrote: > >> why change them to gpio? +CC Alexandre, since he might be interested :-) > Hardware RTS/CTS lines can be occupied by RX/TX of other AUART port > in order to obtain as much uarts as possible using i.mx283. > Therefore gpios can be used for "hardware" flow control. Your logic is outright flawed here, the first sentence doesn't implicate the second sentence. In fact, those two are completely unrelated. > >> If we change them to gpio. Could the DMA still > >> > >> works fine? > >> did you test the DMA with this patch? > >> > >> Add Marek for this patch too. > > > > I didn't look too deep into the patch, so here's just my experience: > > > > 1) The AUART block signals and GPIO block signals are not sychronised > > using the > > > > same clock. Therefore, the latency between toggling of the AUART > > lines and the GPIO-driven pins will not be deterministic and will > > vary. There might be a way to approximate that, but that's > > definitelly not a reliable solution. > > > > This is very bad for example if you drive RS485 DIR line with the RTS > > pin as a GPIO ; the RTS pin will toggle at non-deterministic time > > compared to the end of UART transmission. This will trigger bit-loss > > on the RS485 line and you just don't want that. > > > > 2) Speaking of RS485, there's [1] and [2]. which I believe apply to any > > combo > > > > of UART+GPIO toggling. > > > > So I hate to bring the bad news , but UART+GPIO combo toggling is really > > a bad bad idea. > > Unfortunately if hardware is limited there is no choice and UART+GPIO is > necessary. You will run into timing problems (see above). What you're proposing here is a workaround for broken hardware, which was proven to be a bad idea and NAK'd already multiple times in the past (please see the links I posted in my last email). > Your experience confirms the discussion [3] with Russell King. DMA > should be disabled and > the patch disables DMA support in mxs_auart_init_gpios() if RTS or CTS > line is set as gpio. DMA has nothing to do with those problems here. DMA can be safely ignored for the purpose of the discussion altogether. Like I explained already -- the problem is with the GPIO and AUART block not being synchronised by the same clock. It is therefore impossible to predict and control when the GPIO signals and the AUART signals will toggle in relation to one another. > So I didn't test the patch with the DMA - I've just disabled it. This would break the driver for pretty much everyone using higher baud rates. > The question is different. The driver supports the cases for RTS/CTS: > 1) both RTS/CTS assigned to hardware AUART (pinmux configured by DT, > DT sets fsl,uart-has-rtscts) > a) settermios() sets CRTSCTS: DMA enabled, auto RTS/CTS is enabled > b) settermios() doesn't set CRTSCTS: DMA disabled, RTS/CTS > controlled by get/set_mctrl() > 2) both RTS/CTS assigned to hardware AUART (pinmux configured by DT, > DT doesn't set fsl,uart-has-rtscts) > a) settermios() sets CRTSCTS: DMA disabled, auto RTS/CTS is enabled > b) settermios() doesn't set CRTSCTS: DMA disabled, RTS/CTS > controlled by get/set_mctrl() > > and after the patch it is more complex (because of mixed cases): > 3) both RTS/CTS assigned to gpios (DT sets or not fsl,uart-has-rtscts): > the patch cancels fsl,uart-has-rtscts flag to disable DMA support, > settermios() sets or not CRTSCTS: DMA disabled, RTS/CTS controlled > by get/set_mctrl(), > both lines by gpios. In addition: > a) settermios() doesn't set CRTSCTS: RTS of hardware AUART is also > controlled but > the case assumes it is not selected by pinmux in DT > b) settermios() sets CRTSCTS: auto RTS of hardware AUART is also > enabled but > the case assumes it is not selected by pinmux in DT > 4) RTS assigned to hardware AUART (pinmux configured by DT, > fsl,uart-has-rtscts set or not), > CTS assigned to gpio: the patch cancels fsl,uart-has-rtscts flag to > disable DMA support, > a) settermios() doesn't set CRTSCTS: DMA disabled, CTS (as gpio) > read by get_mctrl(), > RTS of hardware AUART controlled by set_mctrl() > b) settermios() sets CRTSCTS: DMA disabled, CTS (as gpio) read by > get_mctrl(), > auto RTS is enabled > So case 4) is exactly 3) case with just different pinmux > configuration and the patch > threats 3) and 4) as the same case. > 5) CTS assigned to hardware AUART (pinmux configured by DT, > fsl,uart-has-rtscts set or not), > RTS assigned to gpio: the patch cancels fsl,uart-has-rtscts flag to > disable DMA support, > a) settermios() doesn't set CRTSCTS: DMA disabled, CTS of hardware > AUART read by > get_mctrl(), RTS (as gpio) controlled by set_mctrl(), in > addition RTS of hardware AUART > is also controlled but the case assumes it is not selected by > pinmux in DT > b) settermios() sets CRTSCTS: DMA disabled, auto RTS/CTS is enabled > but RTS is controlled by set_mctrl() as gpio because the case assumes it > is not selected > by pinmux in DT > > It is not nice description but I hadn't idea how to write it more clear. > The mixed cases 4) and 5) are likely not so useful but possible and not > so expensive > to support. > Now the question: "fsl,uart-has-rtscts" name seems to be misleading now, > do you agree? It rather should include "dma" word in the name. Any > suggestion? > > [3] http://thread.gmane.org/gmane.linux.serial/16069/focus=16077 The best suggestion I can give you is to fix your hardware early, before you run into nasty deep s.....tuff. These workarounds do not work and they will bit you later on, when it's hard to fix the hardware anymore.