[PATCH] clk: zynq: Fix EMIO clock use detection for gem0

[Ondřej Jirman]

On Thu, Apr 25, 2024 at 10:23:42AM GMT, megi xff wrote:
> On Wed, Apr 24, 2024 at 04:34:05PM GMT, Michal Simek wrote:
> > 
> > 
> > On 4/16/24 10:44, Ondřej Jirman wrote:
> > > From: Ondrej Jirman <megi at xff.cz>
> > > 
> > > According to TRM, the bit that differentiates between MIO and EMIO
> > > clocks is bit 6. This resolves failure to set clock when using EMIO
> > > clock for ethernet.
> > 
> > Not sure which TRM you are using but here
> > 
> > https://docs.amd.com/r/en-US/ug585-zynq-7000-SoC-TRM/Register-slcr-GEM0_RCLK_CTRL
> > 
> > SRCSEL it is bit 4 not bit 6.
> 
> Maybe TRM is wrong? :)
> 
> I have XSA produced by Vivado that has:
> 
> GEM0_RCLK_CTRL:
> 
>     EMIT_MASKWRITE(0XF8000138, 0x00000011U ,0x00000011U),
>     // .. CLKACT = 0x1
>     // .. ==> 0XF8000140[0:0] = 0x00000001U
>     // ..     ==> MASK : 0x00000001U    VAL : 0x00000001U
>     // .. SRCSEL = 0x4
>     // .. ==> 0XF8000140[6:4] = 0x00000004U
>     // ..     ==> MASK : 0x00000070U    VAL : 0x00000040U
>     // .. DIVISOR = 0x1
>     // .. ==> 0XF8000140[13:8] = 0x00000001U
>     // ..     ==> MASK : 0x00003F00U    VAL : 0x00000100U
>     // .. DIVISOR1 = 0x5
>     // .. ==> 0XF8000140[25:20] = 0x00000005U
>     // ..     ==> MASK : 0x03F00000U    VAL : 0x00500000U
>     // .. 

Eh, I take this back. :) Copied the wrong comment.

regards,
	o.

> https://megous.com/dl/tmp/20e2593dbe9e0b3b.png
> 
> GEM0_CLK_CTRL:
> 
>     EMIT_MASKWRITE(0XF8000140, 0x03F03F71U ,0x00500141U),
>     // .. CLKACT = 0x1
>     // .. ==> 0XF8000148[0:0] = 0x00000001U
>     // ..     ==> MASK : 0x00000001U    VAL : 0x00000001U
>     // .. SRCSEL = 0x0
>     // .. ==> 0XF8000148[5:4] = 0x00000000U
>     // ..     ==> MASK : 0x00000030U    VAL : 0x00000000U
>     // .. DIVISOR = 0x10
>     // .. ==> 0XF8000148[13:8] = 0x00000010U
>     // ..     ==> MASK : 0x00003F00U    VAL : 0x00001000U
>     // ..
> 
> https://megous.com/dl/tmp/65c6ee01818a06ea.png
> 
> So the PS init sequence in XSA suggests that GEM0_RCLK_CTRL layout
> actually matches the GEM0_CLK_CTRL documentation.
> 
> I guess Vivado is right and TRM is wrong, since HW works as expected
> with the PS configuration that doesn't match the TRM.
> 
> > > 
> > > Signed-off-by: Ondrej Jirman <megi at xff.cz>
> > > ---
> > >   drivers/clk/clk_zynq.c | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/clk_zynq.c b/drivers/clk/clk_zynq.c
> > > index e3cefe2e0c72..78e6886a000c 100644
> > > --- a/drivers/clk/clk_zynq.c
> > > +++ b/drivers/clk/clk_zynq.c
> > > @@ -42,6 +42,8 @@
> > >   #define CLK_CTRL_DIV3X_SHIFT	20
> > >   #define CLK_CTRL_DIV3X_MASK	(ZYNQ_CLK_MAXDIV << CLK_CTRL_DIV3X_SHIFT)
> > > +#define CLK_CTRL_GEM_EMIO	(1u << 6)
> > > +
> > >   DECLARE_GLOBAL_DATA_PTR;
> > >   #ifndef CONFIG_SPL_BUILD
> > > @@ -161,7 +163,7 @@ static enum zynq_clk_rclk zynq_clk_get_gem_rclk(enum zynq_clk id)
> > >   	else
> > >   		clk_ctrl = readl(&slcr_base->gem1_rclk_ctrl);
> > > -	srcsel = (clk_ctrl & CLK_CTRL_SRCSEL_MASK) >> CLK_CTRL_SRCSEL_SHIFT;
> > > +	srcsel = (clk_ctrl & CLK_CTRL_GEM_EMIO);
> >
> > Definitely using SRCSEL_MASK is not ideal solution because mask is 0x3 and
> > in gem case it is single bit. But based on description you should be getting
> > correct values even with 0x3 because SHIFT is correct.
> 
> Well, it doesn't help that the code is almost all refering to CLK_CTRL while
> actually accessing gem1_rclk_ctrl in the struct.
> 
> In any case it can't detect the case when sourcing the clock from EMIO and
> not one of the PLLs, apparently.
> 
> The failure I'm talking about is here in zynq_gem.c:
> 
>         ret = clk_get_rate(&priv->tx_clk);
>         if (ret != clk_rate) {
>                 ret = clk_set_rate(&priv->tx_clk, clk_rate);
>                 if (IS_ERR_VALUE(ret)) {
>                         dev_err(dev, "failed to set tx clock rate %ld\n", clk_rate);
>                         return ret;
>                 }
>         }
> 
> And all I get is "failed to set tx clock rate" from U-Boot and no ethernet.
> 
> regards,
> 	o.
> 
> > Thanks,
> > Michal