diff mbox

ARM: dts: imx6sx: document SION necessity of ENET1_REF_CLK1

Message ID 1471425931.1934.18.camel@googlemail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Christoph Fritz Aug. 17, 2016, 9:25 a.m. UTC 
Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com>
---
 arch/arm/boot/dts/imx6sx-pinfunc.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Fabio Estevam Aug. 17, 2016, 2:26 p.m. UTC | #1 
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 :-)
Christoph Fritz Aug. 17, 2016, 6:03 p.m. UTC | #2 
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
Christoph Fritz Aug. 22, 2016, 3:08 p.m. UTC | #3 
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?
Shawn Guo Aug. 29, 2016, 1:18 a.m. UTC | #4 
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
diff mbox

Patch

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