[v2,15/19] clk: imx6ul: fix enet1 gate configuration
Commit Message
According to the "i.MX 6UltraLite Applications Processor Reference Manual,
Rev. 2, 03/2017", BIT(13) is ENET1_125M_EN which is not controlling root
of PLL6. It is controlling ENET1 separately.
So, instead of this picture (implementation before this patch):
fec1 <- enet_ref (divider) <---------------------------,
|- pll6_enet (gate)
fec2 <- enet2_ref_125m (gate) <- enet2_ref (divider) <-´
we should have this one (after this patch):
fec1 <- enet1_ref_125m (gate) <- enet1_ref (divider) <-,
|- pll6_enet
fec2 <- enet2_ref_125m (gate) <- enet2_ref (divider) <-´
With this fix, the RMII reference clock will be turned off, after
setting network interface down on each separate interface
(ip l s dev eth0 down). Which was not working before, on system with both
FECs enabled.
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/clk/imx/clk-imx6ul.c | 7 ++++---
include/dt-bindings/clock/imx6ul-clock.h | 3 ++-
2 files changed, 6 insertions(+), 4 deletions(-)
Comments
On 23-01-17 07:14:49, Oleksij Rempel wrote:
> According to the "i.MX 6UltraLite Applications Processor Reference Manual,
> Rev. 2, 03/2017", BIT(13) is ENET1_125M_EN which is not controlling root
> of PLL6. It is controlling ENET1 separately.
>
> So, instead of this picture (implementation before this patch):
> fec1 <- enet_ref (divider) <---------------------------,
> |- pll6_enet (gate)
> fec2 <- enet2_ref_125m (gate) <- enet2_ref (divider) <-´
>
> we should have this one (after this patch):
> fec1 <- enet1_ref_125m (gate) <- enet1_ref (divider) <-,
> |- pll6_enet
> fec2 <- enet2_ref_125m (gate) <- enet2_ref (divider) <-´
>
> With this fix, the RMII reference clock will be turned off, after
> setting network interface down on each separate interface
> (ip l s dev eth0 down). Which was not working before, on system with both
> FECs enabled.
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
I'm OK with this. Maybe a fixes tag ?
Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
> ---
> drivers/clk/imx/clk-imx6ul.c | 7 ++++---
> include/dt-bindings/clock/imx6ul-clock.h | 3 ++-
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-imx6ul.c b/drivers/clk/imx/clk-imx6ul.c
> index 67a7a77ca540..c3c465c1b0e7 100644
> --- a/drivers/clk/imx/clk-imx6ul.c
> +++ b/drivers/clk/imx/clk-imx6ul.c
> @@ -176,7 +176,7 @@ static void __init imx6ul_clocks_init(struct device_node *ccm_node)
> hws[IMX6UL_CLK_PLL3_USB_OTG] = imx_clk_hw_gate("pll3_usb_otg", "pll3_bypass", base + 0x10, 13);
> hws[IMX6UL_CLK_PLL4_AUDIO] = imx_clk_hw_gate("pll4_audio", "pll4_bypass", base + 0x70, 13);
> hws[IMX6UL_CLK_PLL5_VIDEO] = imx_clk_hw_gate("pll5_video", "pll5_bypass", base + 0xa0, 13);
> - hws[IMX6UL_CLK_PLL6_ENET] = imx_clk_hw_gate("pll6_enet", "pll6_bypass", base + 0xe0, 13);
> + hws[IMX6UL_CLK_PLL6_ENET] = imx_clk_hw_fixed_factor("pll6_enet", "pll6_bypass", 1, 1);
> hws[IMX6UL_CLK_PLL7_USB_HOST] = imx_clk_hw_gate("pll7_usb_host", "pll7_bypass", base + 0x20, 13);
>
> /*
> @@ -205,12 +205,13 @@ static void __init imx6ul_clocks_init(struct device_node *ccm_node)
> hws[IMX6UL_CLK_PLL3_PFD2] = imx_clk_hw_pfd("pll3_pfd2_508m", "pll3_usb_otg", base + 0xf0, 2);
> hws[IMX6UL_CLK_PLL3_PFD3] = imx_clk_hw_pfd("pll3_pfd3_454m", "pll3_usb_otg", base + 0xf0, 3);
>
> - hws[IMX6UL_CLK_ENET_REF] = clk_hw_register_divider_table(NULL, "enet_ref", "pll6_enet", 0,
> + hws[IMX6UL_CLK_ENET_REF] = clk_hw_register_divider_table(NULL, "enet1_ref", "pll6_enet", 0,
> base + 0xe0, 0, 2, 0, clk_enet_ref_table, &imx_ccm_lock);
> hws[IMX6UL_CLK_ENET2_REF] = clk_hw_register_divider_table(NULL, "enet2_ref", "pll6_enet", 0,
> base + 0xe0, 2, 2, 0, clk_enet_ref_table, &imx_ccm_lock);
>
> - hws[IMX6UL_CLK_ENET2_REF_125M] = imx_clk_hw_gate("enet_ref_125m", "enet2_ref", base + 0xe0, 20);
> + hws[IMX6UL_CLK_ENET1_REF_125M] = imx_clk_hw_gate("enet1_ref_125m", "enet1_ref", base + 0xe0, 13);
> + hws[IMX6UL_CLK_ENET2_REF_125M] = imx_clk_hw_gate("enet2_ref_125m", "enet2_ref", base + 0xe0, 20);
> hws[IMX6UL_CLK_ENET_PTP_REF] = imx_clk_hw_fixed_factor("enet_ptp_ref", "pll6_enet", 1, 20);
> hws[IMX6UL_CLK_ENET_PTP] = imx_clk_hw_gate("enet_ptp", "enet_ptp_ref", base + 0xe0, 21);
>
> diff --git a/include/dt-bindings/clock/imx6ul-clock.h b/include/dt-bindings/clock/imx6ul-clock.h
> index 79094338e6f1..b44920f1edb0 100644
> --- a/include/dt-bindings/clock/imx6ul-clock.h
> +++ b/include/dt-bindings/clock/imx6ul-clock.h
> @@ -256,7 +256,8 @@
> #define IMX6UL_CLK_GPIO4 247
> #define IMX6UL_CLK_GPIO5 248
> #define IMX6UL_CLK_MMDC_P1_IPG 249
> +#define IMX6UL_CLK_ENET1_REF_125M 250
>
> -#define IMX6UL_CLK_END 250
> +#define IMX6UL_CLK_END 251
>
> #endif /* __DT_BINDINGS_CLOCK_IMX6UL_H */
> --
> 2.30.2
>
On Sun, Jan 29, 2023 at 07:32:31PM +0200, Abel Vesa wrote:
> On 23-01-17 07:14:49, Oleksij Rempel wrote:
> > According to the "i.MX 6UltraLite Applications Processor Reference Manual,
> > Rev. 2, 03/2017", BIT(13) is ENET1_125M_EN which is not controlling root
> > of PLL6. It is controlling ENET1 separately.
> >
> > So, instead of this picture (implementation before this patch):
> > fec1 <- enet_ref (divider) <---------------------------,
> > |- pll6_enet (gate)
> > fec2 <- enet2_ref_125m (gate) <- enet2_ref (divider) <-´
> >
> > we should have this one (after this patch):
> > fec1 <- enet1_ref_125m (gate) <- enet1_ref (divider) <-,
> > |- pll6_enet
> > fec2 <- enet2_ref_125m (gate) <- enet2_ref (divider) <-´
> >
> > With this fix, the RMII reference clock will be turned off, after
> > setting network interface down on each separate interface
> > (ip l s dev eth0 down). Which was not working before, on system with both
> > FECs enabled.
> >
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>
> I'm OK with this. Maybe a fixes tag ?
Hm. Initial commit was:
Fixes: 787b4271a6a0 ("clk: imx: add imx6ul clk tree support")
but this patch will not apply on top of it.
Next possible commit would be:
Fixes: 1487b60dc2d2 ("clk: imx6ul: Switch to clk_hw based API")
But this patch didn't introduce this issue, it was just refactoring.
What do you prefer?
Regards,
Oleksij
On 23-01-30 13:15:30, Oleksij Rempel wrote:
> On Sun, Jan 29, 2023 at 07:32:31PM +0200, Abel Vesa wrote:
> > On 23-01-17 07:14:49, Oleksij Rempel wrote:
> > > According to the "i.MX 6UltraLite Applications Processor Reference Manual,
> > > Rev. 2, 03/2017", BIT(13) is ENET1_125M_EN which is not controlling root
> > > of PLL6. It is controlling ENET1 separately.
> > >
> > > So, instead of this picture (implementation before this patch):
> > > fec1 <- enet_ref (divider) <---------------------------,
> > > |- pll6_enet (gate)
> > > fec2 <- enet2_ref_125m (gate) <- enet2_ref (divider) <-´
> > >
> > > we should have this one (after this patch):
> > > fec1 <- enet1_ref_125m (gate) <- enet1_ref (divider) <-,
> > > |- pll6_enet
> > > fec2 <- enet2_ref_125m (gate) <- enet2_ref (divider) <-´
> > >
> > > With this fix, the RMII reference clock will be turned off, after
> > > setting network interface down on each separate interface
> > > (ip l s dev eth0 down). Which was not working before, on system with both
> > > FECs enabled.
> > >
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> >
> > I'm OK with this. Maybe a fixes tag ?
>
> Hm. Initial commit was:
> Fixes: 787b4271a6a0 ("clk: imx: add imx6ul clk tree support")
> but this patch will not apply on top of it.
> Next possible commit would be:
> Fixes: 1487b60dc2d2 ("clk: imx6ul: Switch to clk_hw based API")
> But this patch didn't introduce this issue, it was just refactoring.
Hm, in that case I don't think is qoing to be backported ever.
>
> What do you prefer?
I'll apply it as it is.
Thanks.
>
> Regards,
> Oleksij
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
@@ -176,7 +176,7 @@ static void __init imx6ul_clocks_init(struct device_node *ccm_node)
hws[IMX6UL_CLK_PLL3_USB_OTG] = imx_clk_hw_gate("pll3_usb_otg", "pll3_bypass", base + 0x10, 13);
hws[IMX6UL_CLK_PLL4_AUDIO] = imx_clk_hw_gate("pll4_audio", "pll4_bypass", base + 0x70, 13);
hws[IMX6UL_CLK_PLL5_VIDEO] = imx_clk_hw_gate("pll5_video", "pll5_bypass", base + 0xa0, 13);
- hws[IMX6UL_CLK_PLL6_ENET] = imx_clk_hw_gate("pll6_enet", "pll6_bypass", base + 0xe0, 13);
+ hws[IMX6UL_CLK_PLL6_ENET] = imx_clk_hw_fixed_factor("pll6_enet", "pll6_bypass", 1, 1);
hws[IMX6UL_CLK_PLL7_USB_HOST] = imx_clk_hw_gate("pll7_usb_host", "pll7_bypass", base + 0x20, 13);
/*
@@ -205,12 +205,13 @@ static void __init imx6ul_clocks_init(struct device_node *ccm_node)
hws[IMX6UL_CLK_PLL3_PFD2] = imx_clk_hw_pfd("pll3_pfd2_508m", "pll3_usb_otg", base + 0xf0, 2);
hws[IMX6UL_CLK_PLL3_PFD3] = imx_clk_hw_pfd("pll3_pfd3_454m", "pll3_usb_otg", base + 0xf0, 3);
- hws[IMX6UL_CLK_ENET_REF] = clk_hw_register_divider_table(NULL, "enet_ref", "pll6_enet", 0,
+ hws[IMX6UL_CLK_ENET_REF] = clk_hw_register_divider_table(NULL, "enet1_ref", "pll6_enet", 0,
base + 0xe0, 0, 2, 0, clk_enet_ref_table, &imx_ccm_lock);
hws[IMX6UL_CLK_ENET2_REF] = clk_hw_register_divider_table(NULL, "enet2_ref", "pll6_enet", 0,
base + 0xe0, 2, 2, 0, clk_enet_ref_table, &imx_ccm_lock);
- hws[IMX6UL_CLK_ENET2_REF_125M] = imx_clk_hw_gate("enet_ref_125m", "enet2_ref", base + 0xe0, 20);
+ hws[IMX6UL_CLK_ENET1_REF_125M] = imx_clk_hw_gate("enet1_ref_125m", "enet1_ref", base + 0xe0, 13);
+ hws[IMX6UL_CLK_ENET2_REF_125M] = imx_clk_hw_gate("enet2_ref_125m", "enet2_ref", base + 0xe0, 20);
hws[IMX6UL_CLK_ENET_PTP_REF] = imx_clk_hw_fixed_factor("enet_ptp_ref", "pll6_enet", 1, 20);
hws[IMX6UL_CLK_ENET_PTP] = imx_clk_hw_gate("enet_ptp", "enet_ptp_ref", base + 0xe0, 21);
@@ -256,7 +256,8 @@
#define IMX6UL_CLK_GPIO4 247
#define IMX6UL_CLK_GPIO5 248
#define IMX6UL_CLK_MMDC_P1_IPG 249
+#define IMX6UL_CLK_ENET1_REF_125M 250
-#define IMX6UL_CLK_END 250
+#define IMX6UL_CLK_END 251
#endif /* __DT_BINDINGS_CLOCK_IMX6UL_H */