| Message ID | 1458825865-7434-7-git-send-email-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| 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
Hello Christoph, On Sat, Aug 13, 2016 at 10:35:32PM +0200, Christoph Fritz wrote: > On Wed, 2016-08-10 at 17:54 -0300, Fabio Estevam wrote: > > Could you please post your suggestion as a patch or RFC so that we can > > understand what your proposal is? > > Sure, here is my proposal: I like it, just a few minor nits below. > +/* > + * SION bit is necessary for ENET1_REF_CLK1 (ENET2_REF_CLK2 untested) if it is > + * used as clock output of IMX6SX_CLK_ENET_REF (ENET1_TX_CLK) to e.g. supply a > + * PHY in RMII mode. This configuration is true if: Sounds a bit strage to me. Maybe s/true/necessary/? Or "valid"? > + * - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_PAD_DIR_MASK is set > + * - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_MUX_SEL_MASK unset > + * It seems to be a silicon bug that in this configuration ENET1_TX reference > + * clock isn't provided automatically. According to i.mx6sx reference manual s/i.mx6sx/i.MX6SX/ > + * (IOMUXC_GPR_GPR1 field descriptions: ENET1_CLK_SEL, Rev. 0 from 2/2015) it > + * should be the case. > + * This might have side effects for other hardware units that are connected to > + * that pin and use the respective function as input (e.g. DSR irq handling). s/DSR irq/UART1's DTR/. Then it's more obvious to relate to MX6SX_PAD_ENET1_TX_CLK__UART1_DTR_B. > + */ > #define MX6SX_PAD_ENET1_TX_CLK__ENET1_REF_CLK1 0x0090 0x03D8 0x0760 0x1 0x1 > #define MX6SX_PAD_ENET1_TX_CLK__AUDMUX_AUD4_RXD 0x0090 0x03D8 0x0644 0x2 0x1 > #define MX6SX_PAD_ENET1_TX_CLK__UART1_DTR_B 0x0090 0x03D8 0x0000 0x3 0x0 Thanks Uwe
Hi Christoph, On Wed, Aug 17, 2016 at 6:25 AM, Christoph Fritz <chf.fritz@googlemail.com> wrote: > +/* > + * SION bit is necessary for ENET1_REF_CLK1 (ENET2_REF_CLK2 untested) if it is > + * used as clock output of IMX6SX_CLK_ENET_REF (ENET1_TX_CLK) to e.g. supply a > + * PHY in RMII mode. This configuration is valid if: > + * - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_PAD_DIR_MASK is set > + * - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_MUX_SEL_MASK unset So in your case the imx6sx_enet_clk_sel() does not do what you need: static void __init imx6sx_enet_clk_sel(void) { struct regmap *gpr; gpr = syscon_regmap_lookup_by_compatible("fsl,imx6sx-iomuxc-gpr"); if (!IS_ERR(gpr)) { regmap_update_bits(gpr, IOMUXC_GPR1, IMX6SX_GPR1_FEC_CLOCK_MUX_SEL_MASK, 0); regmap_update_bits(gpr, IOMUXC_GPR1, IMX6SX_GPR1_FEC_CLOCK_PAD_DIR_MASK, 0); } else { pr_err("failed to find fsl,imx6sx-iomux-gpr regmap\n"); } } It seems that it is not a good idea to have imx6sx_enet_clk_sel() in common code as the GPR1 setting can change from board to board. I think we need the fec driver to be in charge of configuring the GPR1 register. This is unrelated to your patch though :-)
On Wed, 2016-08-17 at 11:26 -0300, Fabio Estevam wrote: > Hi Christoph, > > On Wed, Aug 17, 2016 at 6:25 AM, Christoph Fritz > <chf.fritz@googlemail.com> wrote: > > > +/* > > + * SION bit is necessary for ENET1_REF_CLK1 (ENET2_REF_CLK2 untested) if it is > > + * used as clock output of IMX6SX_CLK_ENET_REF (ENET1_TX_CLK) to e.g. supply a > > + * PHY in RMII mode. This configuration is valid if: > > + * - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_PAD_DIR_MASK is set > > + * - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_MUX_SEL_MASK unset > > So in your case the imx6sx_enet_clk_sel() does not do what you need: > > static void __init imx6sx_enet_clk_sel(void) > { > struct regmap *gpr; > > gpr = syscon_regmap_lookup_by_compatible("fsl,imx6sx-iomuxc-gpr"); > if (!IS_ERR(gpr)) { > regmap_update_bits(gpr, IOMUXC_GPR1, > IMX6SX_GPR1_FEC_CLOCK_MUX_SEL_MASK, 0); > regmap_update_bits(gpr, IOMUXC_GPR1, > IMX6SX_GPR1_FEC_CLOCK_PAD_DIR_MASK, 0); > } else { > pr_err("failed to find fsl,imx6sx-iomux-gpr regmap\n"); > } > } > > It seems that it is not a good idea to have imx6sx_enet_clk_sel() in > common code as the GPR1 setting can change from board to board. Yes, currently I'm adapting/quirking the fec config there and in imx6sx_clocks_init() to set the Phy-Clock-Frequency to 50Mhz. > I think we need the fec driver to be in charge of configuring the GPR1 register. What about making the Phy-Clock-Frequency IMX6SX_CLK_ENET_REF adjustable? > This is unrelated to your patch though :-) Thanks -- Christoph
On Wed, 2016-08-17 at 11:25 +0200, Christoph Fritz wrote: > Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com> > --- > arch/arm/boot/dts/imx6sx-pinfunc.h | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/arch/arm/boot/dts/imx6sx-pinfunc.h b/arch/arm/boot/dts/imx6sx-pinfunc.h > index bb9c6b7..42c4c80 100644 > --- a/arch/arm/boot/dts/imx6sx-pinfunc.h > +++ b/arch/arm/boot/dts/imx6sx-pinfunc.h > @@ -308,6 +308,20 @@ > #define MX6SX_PAD_ENET1_RX_CLK__VDEC_DEBUG_35 0x008C 0x03D4 0x0000 0x8 0x0 > #define MX6SX_PAD_ENET1_RX_CLK__PCIE_CTRL_DEBUG_29 0x008C 0x03D4 0x0000 0x9 0x0 > #define MX6SX_PAD_ENET1_TX_CLK__ENET1_TX_CLK 0x0090 0x03D8 0x0000 0x0 0x0 > +/* > + * SION bit is necessary for ENET1_REF_CLK1 (ENET2_REF_CLK2 untested) if it is > + * used as clock output of IMX6SX_CLK_ENET_REF (ENET1_TX_CLK) to e.g. supply a > + * PHY in RMII mode. This configuration is valid if: > + * - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_PAD_DIR_MASK is set > + * - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_MUX_SEL_MASK unset > + * It seems to be a silicon bug that in this configuration ENET1_TX reference > + * clock isn't provided automatically. According to i.MX6SX reference manual > + * (IOMUXC_GPR_GPR1 field descriptions: ENET1_CLK_SEL, Rev. 0 from 2/2015) it > + * should be the case. > + * So this might have unwanted side effects for other hardware units that are > + * also connected to that pin and using respective function as input (e.g. > + * UART1's DTR handling on MX6SX_PAD_ENET1_TX_CLK__UART1_DTR_B). > + */ > #define MX6SX_PAD_ENET1_TX_CLK__ENET1_REF_CLK1 0x0090 0x03D8 0x0760 0x1 0x1 > #define MX6SX_PAD_ENET1_TX_CLK__AUDMUX_AUD4_RXD 0x0090 0x03D8 0x0644 0x2 0x1 > #define MX6SX_PAD_ENET1_TX_CLK__UART1_DTR_B 0x0090 0x03D8 0x0000 0x3 0x0 @Uwe and Fabio: Can I get your Ack on this? @Shawn: Can you queue this patch for upstream?
On Wed, Aug 17, 2016 at 11:25:31AM +0200, Christoph Fritz wrote: > > Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com> Applied, thanks. > --- > arch/arm/boot/dts/imx6sx-pinfunc.h | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/arch/arm/boot/dts/imx6sx-pinfunc.h b/arch/arm/boot/dts/imx6sx-pinfunc.h > index bb9c6b7..42c4c80 100644 > --- a/arch/arm/boot/dts/imx6sx-pinfunc.h > +++ b/arch/arm/boot/dts/imx6sx-pinfunc.h > @@ -308,6 +308,20 @@ > #define MX6SX_PAD_ENET1_RX_CLK__VDEC_DEBUG_35 0x008C 0x03D4 0x0000 0x8 0x0 > #define MX6SX_PAD_ENET1_RX_CLK__PCIE_CTRL_DEBUG_29 0x008C 0x03D4 0x0000 0x9 0x0 > #define MX6SX_PAD_ENET1_TX_CLK__ENET1_TX_CLK 0x0090 0x03D8 0x0000 0x0 0x0 > +/* > + * SION bit is necessary for ENET1_REF_CLK1 (ENET2_REF_CLK2 untested) if it is > + * used as clock output of IMX6SX_CLK_ENET_REF (ENET1_TX_CLK) to e.g. supply a > + * PHY in RMII mode. This configuration is valid if: > + * - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_PAD_DIR_MASK is set > + * - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_MUX_SEL_MASK unset > + * It seems to be a silicon bug that in this configuration ENET1_TX reference > + * clock isn't provided automatically. According to i.MX6SX reference manual > + * (IOMUXC_GPR_GPR1 field descriptions: ENET1_CLK_SEL, Rev. 0 from 2/2015) it > + * should be the case. > + * So this might have unwanted side effects for other hardware units that are > + * also connected to that pin and using respective function as input (e.g. > + * UART1's DTR handling on MX6SX_PAD_ENET1_TX_CLK__UART1_DTR_B). > + */ > #define MX6SX_PAD_ENET1_TX_CLK__ENET1_REF_CLK1 0x0090 0x03D8 0x0760 0x1 0x1 > #define MX6SX_PAD_ENET1_TX_CLK__AUDMUX_AUD4_RXD 0x0090 0x03D8 0x0644 0x2 0x1 > #define MX6SX_PAD_ENET1_TX_CLK__UART1_DTR_B 0x0090 0x03D8 0x0000 0x3 0x0 > -- > 2.1.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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(-)