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

[Michal Simek]

On 4/25/24 10:23, Ondřej Jirman 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
>      // ..
> 
> https://megous.com/dl/tmp/20e2593dbe9e0b3b.png
> 
> GEM0_CLK_CTRL:
> 
>      EMIT_MASKWRITE(0XF8000140, 0x03F03F71U ,0x00500141U),

Here you have 0x140

>      // .. CLKACT = 0x1
>      // .. ==> 0XF8000148[0:0] = 0x00000001U

And here 0x148


This is what I see for external clock

   265     // .. CLKACT = 0x1
   266     // .. ==> 0XF8000138[0:0] = 0x00000001U
   267     // ..     ==> MASK : 0x00000001U    VAL : 0x00000001U
   268     // .. SRCSEL = 0x1
   269     // .. ==> 0XF8000138[4:4] = 0x00000001U
   270     // ..     ==> MASK : 0x00000010U    VAL : 0x00000010U
   271     // ..
   272     EMIT_MASKWRITE(0XF8000138, 0x00000011U ,0x00000011U),

and this one for io pll

   265     // .. CLKACT = 0x1
   266     // .. ==> 0XF8000138[0:0] = 0x00000001U
   267     // ..     ==> MASK : 0x00000001U    VAL : 0x00000001U
   268     // .. SRCSEL = 0x0
   269     // .. ==> 0XF8000138[4:4] = 0x00000000U
   270     // ..     ==> MASK : 0x00000010U    VAL : 0x00000000U
   271     // ..
   272     EMIT_MASKWRITE(0XF8000138, 0x00000011U ,0x00000001U),

It means even Vivado is generating it as BIT 4.


>      // ..     ==> 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.

Zynq is quite a old device but I expect you have external clock out of chip or 
you generate it in PL. For that you need to describe it.

How does your DT looks like for external clocks?

Thanks,
Michal