| Message ID | 1458825865-7434-7-git-send-email-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Christoph, Cc += Shawn, Fabio On Fri, Aug 05, 2016 at 12:44:23AM +0200, Christoph Fritz wrote: > here on a imx6sx board newly introduced DSR-irq-handling breaks serial > console and network support. > > Reverting commit 27e16501052e5341934d3 "serial: imx: implement DSR irq > handling for DTE mode" fixes the issue. > > The underlying cause is the RMII network configuration where register > bit SION in IOMUXC_SW_MUX_CTL_PAD_ENET1_TX_CLK is necessary. But this > also falsely feeds signal UART1_DTR_B and triggers DSR irq... > > Can we revert this commit or add a quirk for RMII-SION configs or make > this optional by a device tree option? Hmm, MX6SX_PAD_ENET1_TX_CLK__ENET1_TX_CLK in arch/arm/boot/dts/imx6sx-pinfunc.h doesn't have SION set. I remember there were issues around eth and SION on i.MX6DL, but don't know the details. A quick net research found https://community.nxp.com/thread/359531 which has: "Set the SION bit. Note that this is not required because the funtion setting controls the signal path, but it is good practice as it reminds the user that the clock needs to fed back into the Ethernet MAC.". Sounds like dangerous smattering and a wrong expectation about "users". https://community.nxp.com/thread/376821 is from someone who has the same problem(?) on i.MX6SX, but no solution yet. Does that mean MX6SX_PAD_ENET1_TX_CLK__ENET1_TX_CLK always needs the SION bit set? Even if not, I'd like to have that documented there, similar to da4fa6fa8016 ("ARM: imx25-pinfunc: document SION being important for MX25_PAD_SD1_CMD__SD1_CMD"). Once this is done (and still an issue) I'd suggest to standardize a dt property disable-dsr; (and the same for the other handshaking lines) and support th{is,ese} in the imx uart driver. Reverting 27e16501052e IMO isn't a sensible option. Best regards Uwe
On Fri, 2016-08-05 at 08:58 +0200, Uwe Kleine-König wrote: > On Fri, Aug 05, 2016 at 12:44:23AM +0200, Christoph Fritz wrote: > > here on a imx6sx board newly introduced DSR-irq-handling breaks serial > > console and network support. > > > > Reverting commit 27e16501052e5341934d3 "serial: imx: implement DSR irq > > handling for DTE mode" fixes the issue. > > > > The underlying cause is the RMII network configuration where register > > bit SION in IOMUXC_SW_MUX_CTL_PAD_ENET1_TX_CLK is necessary. But this > > also falsely feeds signal UART1_DTR_B and triggers DSR irq... > > > > Can we revert this commit or add a quirk for RMII-SION configs or make > > this optional by a device tree option? > > Hmm, MX6SX_PAD_ENET1_TX_CLK__ENET1_TX_CLK in > arch/arm/boot/dts/imx6sx-pinfunc.h doesn't have SION set. > > I remember there were issues around eth and SION on i.MX6DL, but don't > know the details. A quick net research found > https://community.nxp.com/thread/359531 which has: "Set the SION bit. > Note that this is not required because the funtion setting controls the > signal path, but it is good practice as it reminds the user that the > clock needs to fed back into the Ethernet MAC.". Sounds like dangerous > smattering and a wrong expectation about "users". > > https://community.nxp.com/thread/376821 is from someone who has the same > problem(?) on i.MX6SX, but no solution yet. > > Does that mean MX6SX_PAD_ENET1_TX_CLK__ENET1_TX_CLK always needs the > SION bit set? SION is necessary for pinconfig MX6SX_PAD_ENET1_TX_CLK__ENET1_REF_CLK1 if bit 17 in IOMUXC_GPR_GPR1 is set and bit 13 not. So that ref_enetpll1 provides a clock not only for the external PHY but also for the internal controller. Using SION is a quirk here, because the silicon doesn't feed the clock back to the internal controller automatically. On the other hand, NXP could argue: You need to add a wire on your PCB between ENET1_REF_CLK1 and ENET1_TX_CLK -- but referring to the datasheet ENET1_TX_CLK isn't used in RMII config... So if Fabio or Shawn agrees with my assumption above, I'll add something like 27e16501052e5341934d3 "serial: imx: implement DSR irq handling for DTE mode". Okay?
Hi Christoph, On Fri, Aug 5, 2016 at 9:03 AM, Christoph Fritz <chf.fritz@googlemail.com> wrote: > SION is necessary for pinconfig MX6SX_PAD_ENET1_TX_CLK__ENET1_REF_CLK1 > if bit 17 in IOMUXC_GPR_GPR1 is set and bit 13 not. > > So that ref_enetpll1 provides a clock not only for the external PHY but > also for the internal controller. > > Using SION is a quirk here, because the silicon doesn't feed the clock > back to the internal controller automatically. > > On the other hand, NXP could argue: You need to add a wire on your PCB > between ENET1_REF_CLK1 and ENET1_TX_CLK -- but referring to the > datasheet ENET1_TX_CLK isn't used in RMII config... > > So if Fabio or Shawn agrees with my assumption above, I'll add something > like 27e16501052e5341934d3 "serial: imx: implement DSR irq > handling for DTE mode". > > Okay? Could you please post your suggestion as a patch or RFC so that we can understand what your proposal is? Thanks
Hey Uwe On Fri, 2016-08-05 at 08:58 +0200, Uwe Kleine-König wrote: > Once this is done (and still an issue) Yes, patch b5ca028fe9ba2090a (ARM: dts: imx6sx: document SION necessity of ENET1_REF_CLK1) is done and mainline and DSR irq handling is still an issue. > I'd suggest to standardize a dt > property > > disable-dsr; Would you go forward? > (and the same for the other handshaking lines) and support th{is,ese} in > the imx uart driver. Reverting 27e16501052e IMO isn't a sensible option. Okay, I'm fine with a 'disable-dsr' solution. Thanks -- Christoph
Hello Christoph, On Mon, Nov 21, 2016 at 12:00:11PM +0100, Christoph Fritz wrote: > On Fri, 2016-08-05 at 08:58 +0200, Uwe Kleine-König wrote: > > Once this is done (and still an issue) > > Yes, patch b5ca028fe9ba2090a (ARM: dts: imx6sx: document SION necessity > of ENET1_REF_CLK1) is done and mainline and DSR irq handling is still an > issue. > > > I'd suggest to standardize a dt > > property > > > > disable-dsr; > > Would you go forward? Currently I don't have the need and time to do this myself. So if you want to propose a patch and send if for review to the dt and serial MLs that would be a good next step. If you add me to Cc (or To) I can take a look. Best regards Uwe
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index c07e652f8f58..621e488cbb12 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -114,6 +114,7 @@ #define UCR3_RXDSEN (1<<6) /* Receive status interrupt enable */ #define UCR3_AIRINTEN (1<<5) /* Async IR wake interrupt enable */ #define UCR3_AWAKEN (1<<4) /* Async wake interrupt enable */ +#define UCR3_DTRDEN (1<<3) /* Data Terminal Ready Delta Enable. */ #define IMX21_UCR3_RXDMUXSEL (1<<2) /* RXD Muxed Input Select */ #define UCR3_INVT (1<<1) /* Inverted Infrared transmission */ #define UCR3_BPEN (1<<0) /* Preset registers enable */ @@ -142,7 +143,7 @@ #define USR1_FRAMERR (1<<10) /* Frame error interrupt flag */ #define USR1_RRDY (1<<9) /* Receiver ready interrupt/dma flag */ #define USR1_AGTIM (1<<8) /* Ageing timer interrupt flag */ -#define USR1_TIMEOUT (1<<7) /* Receive timeout interrupt status */ +#define USR1_DTRD (1<<7) /* DTR Delta */ #define USR1_RXDS (1<<6) /* Receiver idle interrupt flag */ #define USR1_AIRINT (1<<5) /* Async IR wake interrupt flag */ #define USR1_AWAKE (1<<4) /* Aysnc wake interrupt flag */ @@ -804,6 +805,19 @@ static irqreturn_t imx_int(int irq, void *dev_id) ret = IRQ_HANDLED; } + if (sts & USR1_DTRD) { + unsigned long flags; + + if (sts & USR1_DTRD) + writel(USR1_DTRD, sport->port.membase + USR1); + + spin_lock_irqsave(&sport->port.lock, flags); + imx_mctrl_check(sport); + spin_unlock_irqrestore(&sport->port.lock, flags); + + ret = IRQ_HANDLED; + } + if (sts & USR1_RTSD) { imx_rtsint(irq, dev_id); ret = IRQ_HANDLED; @@ -1202,7 +1216,7 @@ static int imx_startup(struct uart_port *port) /* * Finally, clear and enable interrupts */ - writel(USR1_RTSD, sport->port.membase + USR1); + writel(USR1_RTSD | USR1_DTRD, sport->port.membase + USR1); writel(USR2_ORE, sport->port.membase + USR2); if (sport->dma_is_inited && !sport->dma_is_enabled) @@ -1242,7 +1256,7 @@ static int imx_startup(struct uart_port *port) * now, too. */ temp |= IMX21_UCR3_RXDMUXSEL | UCR3_ADNIMP | - UCR3_RI | UCR3_DCD; + UCR3_DTRDEN | UCR3_RI | UCR3_DCD; if (sport->dte_mode) temp &= ~(UCR3_RI | UCR3_DCD);
Enable reporting of DSR events (which is named DTR in the registers because Freescale uses the names as seem from a DCE). Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/tty/serial/imx.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)