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 <m...@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 <m...@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
