Hi Kay-Liu,
On Thu, Apr 25, 2019 at 8:09 AM <[email protected]> wrote:
>
> From: Kay-Liu <[email protected]>
>
> The imx6sx's dts file defines five clocks for fec, the
> 'ahb'clock's value is IMX6SX_CLK_ENET_AHB, but in the
> i.MX6SX Reference Manual there is no such enet ahb clock,
> there is only one "enet clock" in the CCM_CCGR3 register
> which is controlled by bits 5-4, the enet clock is defined
> for the 'ipg' clock, this can cause problem.
> The original phenomenon is using imx6-solox processor and
> Marvel 88E6390 switch with linux OS, the kernel will hang
> during the startup of the linux OS.
> After analyzing the phenomenon, the reason of CPU hang is
> read/write enet module's register when the enet clock
> is disabled. The kernel code try to avoids the problem
> by resume enet clock before read/write enet register.
> But the enet module's clock config will cause a special
> environment which can bypass the clock resume mechanism.
> The CPU has only one enet clock, after kernel parses
> the dts file, the two clock variables 'ipg' and 'ahb'
> finnaly point to the same enet clock register. This will
> cause enet clock be disabled after fec probe over.
> Because the power saving module will affect the BUG, so
> there are two situations for this problem:
> 1)Turn off power saving
> Turn off power saving means that the resume mechanism is
> disabled, so after fec probe over if any one read/write
> enet module's register, the CPU will hang because no one
> could resume the enet clock.
> 2)Turn on power saving
> Turn on power saving could resume enet clock before
> read/write enet register by enable 'ipg' clk, this will
> cause 'ahb' variable state and enet clock register value
> don't match.If any task read/write enet at a high
> frequently, the kernel will keep resume state and never
> enter suspend process, this means that the kernel will
> only modifies the register value during the first resume.
> But the kernel init will check unused clock variable in
> the late initcall, the 'ahb' clock will be treated as
> unused, at this time, the enet clock will be disabled
> bypass the resume mechanism, then the next read/write
> enet module's register will cause the CPU hang.
> Proposed solution is delete the 'ahb' clock's definition
> in the clk-imx6sx.c, and modify fec device’s clocks in
> the dts file, point ‘ahb’ from IMX6SX_CLK_ENET_AHB to
> IMX6SX_CLK_ENET
>
> Signed-off-by: Kay-Liu <[email protected]>
> ---
> Change since v1:
> -inproved commit log description
> -add platform related clock change instead of describe is in the external URL
>
> arch/arm/boot/dts/imx6sx.dtsi | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
> index 5b16e65..b8b23a6 100644
> --- a/arch/arm/boot/dts/imx6sx.dtsi
> +++ b/arch/arm/boot/dts/imx6sx.dtsi
> @@ -919,7 +919,7 @@
> interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&clks IMX6SX_CLK_ENET>,
> - <&clks IMX6SX_CLK_ENET_AHB>,
> + <&clks IMX6SX_CLK_ENET>,
Yes, there is really no IMX6SX_CLK_ENET_AHB as per the Refernce Manual
and it is the same we do on imx6qdl.dtsi:
Reviewed-by: Fabio Estevam <[email protected]>
> From: Fabio Estevam [mailto:[email protected]]
> Sent: Saturday, May 4, 2019 7:04 PM
>
> Hi Kay-Liu,
>
> On Thu, Apr 25, 2019 at 8:09 AM <[email protected]> wrote:
> >
> > From: Kay-Liu <[email protected]>
> >
> > The imx6sx's dts file defines five clocks for fec, the 'ahb'clock's
> > value is IMX6SX_CLK_ENET_AHB, but in the i.MX6SX Reference Manual
> > there is no such enet ahb clock, there is only one "enet clock" in the
> > CCM_CCGR3 register which is controlled by bits 5-4, the enet clock is
> > defined for the 'ipg' clock, this can cause problem.
> > The original phenomenon is using imx6-solox processor and Marvel
> > 88E6390 switch with linux OS, the kernel will hang during the startup
> > of the linux OS.
> > After analyzing the phenomenon, the reason of CPU hang is read/write
> > enet module's register when the enet clock is disabled. The kernel
> > code try to avoids the problem by resume enet clock before read/write
> > enet register.
> > But the enet module's clock config will cause a special environment
> > which can bypass the clock resume mechanism.
> > The CPU has only one enet clock, after kernel parses the dts file, the
> > two clock variables 'ipg' and 'ahb'
> > finnaly point to the same enet clock register. This will cause enet
> > clock be disabled after fec probe over.
> > Because the power saving module will affect the BUG, so there are two
> > situations for this problem:
> > 1)Turn off power saving
> > Turn off power saving means that the resume mechanism is disabled, so
> > after fec probe over if any one read/write enet module's register, the
> > CPU will hang because no one could resume the enet clock.
> > 2)Turn on power saving
> > Turn on power saving could resume enet clock before read/write enet
> > register by enable 'ipg' clk, this will cause 'ahb' variable state and
> > enet clock register value don't match.If any task read/write enet at a
> > high frequently, the kernel will keep resume state and never enter
> > suspend process, this means that the kernel will only modifies the
> > register value during the first resume.
> > But the kernel init will check unused clock variable in the late
> > initcall, the 'ahb' clock will be treated as unused, at this time, the
> > enet clock will be disabled bypass the resume mechanism, then the next
> > read/write enet module's register will cause the CPU hang.
> > Proposed solution is delete the 'ahb' clock's definition in the
> > clk-imx6sx.c, and modify fec device’s clocks in the dts file, point
> > ‘ahb’ from IMX6SX_CLK_ENET_AHB to IMX6SX_CLK_ENET
> >
> > Signed-off-by: Kay-Liu <[email protected]>
> > ---
> > Change since v1:
> > -inproved commit log description
> > -add platform related clock change instead of describe is in the
> > external URL
> >
> > arch/arm/boot/dts/imx6sx.dtsi | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/imx6sx.dtsi
> > b/arch/arm/boot/dts/imx6sx.dtsi index 5b16e65..b8b23a6 100644
> > --- a/arch/arm/boot/dts/imx6sx.dtsi
> > +++ b/arch/arm/boot/dts/imx6sx.dtsi
> > @@ -919,7 +919,7 @@
> > interrupts = <GIC_SPI 118
> IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 119
> IRQ_TYPE_LEVEL_HIGH>;
> > clocks = <&clks IMX6SX_CLK_ENET>,
> > - <&clks
> IMX6SX_CLK_ENET_AHB>,
> > + <&clks IMX6SX_CLK_ENET>,
>
> Yes, there is really no IMX6SX_CLK_ENET_AHB as per the Refernce Manual and
> it is the same we do on imx6qdl.dtsi:
>
> Reviewed-by: Fabio Estevam <[email protected]>
Copy Andy, the ENET owner, to comment.
BTW, it's strange that I did not receive the original patch email.
Also can't grep from the open list.
Regards
Dong Aisheng
From: Aisheng Dong Sent: Sunday, May 5, 2019 4:03 PM
> > From: Fabio Estevam [mailto:[email protected]]
> > Sent: Saturday, May 4, 2019 7:04 PM
> >
> > Hi Kay-Liu,
> >
> > On Thu, Apr 25, 2019 at 8:09 AM <[email protected]> wrote:
> > >
> > > From: Kay-Liu <[email protected]>
> > >
> > > The imx6sx's dts file defines five clocks for fec, the 'ahb'clock's
> > > value is IMX6SX_CLK_ENET_AHB, but in the i.MX6SX Reference Manual
> > > there is no such enet ahb clock, there is only one "enet clock" in
> > > the
> > > CCM_CCGR3 register which is controlled by bits 5-4, the enet clock
> > > is defined for the 'ipg' clock, this can cause problem.
> > > The original phenomenon is using imx6-solox processor and Marvel
> > > 88E6390 switch with linux OS, the kernel will hang during the
> > > startup of the linux OS.
> > > After analyzing the phenomenon, the reason of CPU hang is read/write
> > > enet module's register when the enet clock is disabled. The kernel
> > > code try to avoids the problem by resume enet clock before
> > > read/write enet register.
> > > But the enet module's clock config will cause a special environment
> > > which can bypass the clock resume mechanism.
> > > The CPU has only one enet clock, after kernel parses the dts file,
> > > the two clock variables 'ipg' and 'ahb'
> > > finnaly point to the same enet clock register. This will cause enet
> > > clock be disabled after fec probe over.
> > > Because the power saving module will affect the BUG, so there are
> > > two situations for this problem:
> > > 1)Turn off power saving
> > > Turn off power saving means that the resume mechanism is disabled,
> > > so after fec probe over if any one read/write enet module's
> > > register, the CPU will hang because no one could resume the enet clock.
> > > 2)Turn on power saving
> > > Turn on power saving could resume enet clock before read/write enet
> > > register by enable 'ipg' clk, this will cause 'ahb' variable state
> > > and enet clock register value don't match.If any task read/write
> > > enet at a high frequently, the kernel will keep resume state and
> > > never enter suspend process, this means that the kernel will only
> > > modifies the register value during the first resume.
> > > But the kernel init will check unused clock variable in the late
> > > initcall, the 'ahb' clock will be treated as unused, at this time,
> > > the enet clock will be disabled bypass the resume mechanism, then
> > > the next read/write enet module's register will cause the CPU hang.
> > > Proposed solution is delete the 'ahb' clock's definition in the
> > > clk-imx6sx.c, and modify fec device’s clocks in the dts file, point
> > > ‘ahb’ from IMX6SX_CLK_ENET_AHB to IMX6SX_CLK_ENET
> > >
> > > Signed-off-by: Kay-Liu <[email protected]>
> > > ---
> > > Change since v1:
> > > -inproved commit log description
> > > -add platform related clock change instead of describe is in the
> > > external URL
> > >
> > > arch/arm/boot/dts/imx6sx.dtsi | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/arm/boot/dts/imx6sx.dtsi
> > > b/arch/arm/boot/dts/imx6sx.dtsi index 5b16e65..b8b23a6 100644
> > > --- a/arch/arm/boot/dts/imx6sx.dtsi
> > > +++ b/arch/arm/boot/dts/imx6sx.dtsi
> > > @@ -919,7 +919,7 @@
> > > interrupts = <GIC_SPI 118
> > IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 119
> > IRQ_TYPE_LEVEL_HIGH>;
> > > clocks = <&clks
> IMX6SX_CLK_ENET>,
> > > - <&clks
> > IMX6SX_CLK_ENET_AHB>,
> > > + <&clks
> IMX6SX_CLK_ENET>,
> >
> > Yes, there is really no IMX6SX_CLK_ENET_AHB as per the Refernce Manual
> > and it is the same we do on imx6qdl.dtsi:
> >
> > Reviewed-by: Fabio Estevam <[email protected]>
Nack the patch !
Firstly, i.MX6SX has ENET AHB bus clock for MAC, and currently it is set 200Mhz like clock tree:
IMX6SX_CLK_ENET_PODF 200Mhz -> IMX6SX_CLK_ENET_SEL -> IMX6SX_CLK_ENET_AHB
IMX6SX_CLK_ENET the clock is IPG clock for ENET IP ipg_clk_mac0_s/ipg_clk_s.
(Please check RM Table 18-3. System Clocks, Gating, and Override)
Secondly, for your issue you caught, which was fixed by patch:
commit d7c3a206e6338e4ccdf030719dec028e26a521d5
Author: Andy Duan <[email protected]>
Date: Tue Apr 9 03:40:56 2019 +0000
net: fec: manage ahb clock in runtime pm
Some SOC like i.MX6SX clock have some limits:
- ahb clock should be disabled before ipg.
- ahb and ipg clocks are required for MAC MII bus.
So, move the ahb clock to runtime management together with
ipg clock.
Signed-off-by: Fugang Duan <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
So, please don't remove ahb clock.
Andy
>
> Copy Andy, the ENET owner, to comment.
>
> BTW, it's strange that I did not receive the original patch email.
> Also can't grep from the open list.
>
> Regards
> Dong Aisheng
Hi Andy,
On Sun, May 5, 2019 at 5:15 AM Andy Duan <[email protected]> wrote:
> Nack the patch !
>
> Firstly, i.MX6SX has ENET AHB bus clock for MAC, and currently it is set 200Mhz like clock tree:
> IMX6SX_CLK_ENET_PODF 200Mhz -> IMX6SX_CLK_ENET_SEL -> IMX6SX_CLK_ENET_AHB
>
> IMX6SX_CLK_ENET the clock is IPG clock for ENET IP ipg_clk_mac0_s/ipg_clk_s.
> (Please check RM Table 18-3. System Clocks, Gating, and Override)
Ok, but could you please show us where in the Reference Manual the
IMX6SX_CLK_ENET_AHB is mentioned?
I don't see ENET_AHB in imx6qdl Reference Manual either and we don't
have a ENET_AHB the clk-imx6q driver and nor in the devicetree,
> Secondly, for your issue you caught, which was fixed by patch:
> commit d7c3a206e6338e4ccdf030719dec028e26a521d5
> Author: Andy Duan <[email protected]>
> Date: Tue Apr 9 03:40:56 2019 +0000
>
> net: fec: manage ahb clock in runtime pm
Would this also fix the case where power management support is disabled?
If I understand correctly the explanation from Kay-Liu he would still
see a hang in the case when PM is disabled.
Thanks
From: Fabio Estevam <[email protected]>
> Hi Andy,
>
> On Sun, May 5, 2019 at 5:15 AM Andy Duan <[email protected]> wrote:
>
> > Nack the patch !
> >
...
> > Secondly, for your issue you caught, which was fixed by patch:
> > commit d7c3a206e6338e4ccdf030719dec028e26a521d5
> > Author: Andy Duan <[email protected]>
> > Date: Tue Apr 9 03:40:56 2019 +0000
> >
> > net: fec: manage ahb clock in runtime pm
>
> Would this also fix the case where power management support is disabled?
>
> If I understand correctly the explanation from Kay-Liu he would still see a
> hang in the case when PM is disabled.
>
> Thanks
From current design, it still work even if disable PM.
Please double check it.
Andy