RE: [PATCHv2 1/2] ARM: dts: imx6sx: Use MX6SX_CLK_ENET for fec 'ahb' clock

[Andy Duan]

From: Aisheng Dong Sent: Sunday, May 5, 2019 4:03 PM
> > From: Fabio Estevam [mailto:festevam@xxxxxxxxx]
> > Sent: Saturday, May 4, 2019 7:04 PM
> >
> > Hi Kay-Liu,
> >
> > On Thu, Apr 25, 2019 at 8:09 AM <liuk@xxxxxxxxxxxx> wrote:
> > >
> > > From: Kay-Liu <liuk@xxxxxxxxxxxx>
> > >
> > > 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 <liuk@xxxxxxxxxxxx>
> > > ---
> > > 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 <festevam@xxxxxxxxx>

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 <fugang.duan@xxxxxxx>
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 <fugang.duan@xxxxxxx>
    Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx> 


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