| Message ID | 20240416084456.2821567-1-megi@xff.cz |
|---|---|
| State | New |
| Delegated to: | Michal Simek |
| Headers | show |
| Series | clk: zynq: Fix EMIO clock use detection for gem0 | expand |
On 4/16/24 10:44, Ondřej Jirman wrote: > From: Ondrej Jirman <megi@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. > > Signed-off-by: Ondrej Jirman <megi@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. Thanks, Michal
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@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), // .. 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@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
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@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@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
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@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@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
On Thu, Apr 25, 2024 at 12:59:29PM GMT, Michal Simek wrote: > > 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? The situation is that RX/TX clocks just come from outside https://megous.com/dl/tmp/af9e7c9e8d51781b.png via PL. 25MHz clock is generated by the PHY. I solved it eventually. I described the PHY generated clock as a fixed-clock node, and added a reference to it under &clkc as a source for "gem0_emio_clk". https://megous.com/git/u-boot/tree/arch/arm/dts/zynq-ebaz-megi.dts?h=v2024.07#n26 Zynq clk code seems to fetch gem0_emio_clk from DT for the purpose of determining the clk rate for gem0 in emio mode: https://elixir.bootlin.com/u-boot/latest/source/drivers/clk/clk_zynq.c#L524 https://elixir.bootlin.com/u-boot/latest/source/drivers/clk/clk_zynq.c#L280 This got rid of the "failed to set tx clock rate" error. Thank you for the hint. BTW, I managed to get the .bit file loaded via SPL only with a SPL patch: https://megous.com/git/u-boot/commit/?h=v2024.07&id=c1403bd080bc517c9dc6f507ee12b62fa85153bc It's similar to how it's done in: https://elixir.bootlin.com/u-boot/latest/source/boot/image-board.c#L677 fpga_load() function doesn't work with .bit file I get from Vivado but fpga_loadbitstream() one does. SPL doesn't call it, though. Is that by design? Should I be converting .bit file to something else usable by fpga_load() somehow? I'd like to load bitstream in SPL from FIT because U-Boot proper may already need some PL logic activated for ethernet to work there. Also it's one less file on the /boot filesystem to worry about. :) Thank you and kind regards, o. > Thanks, > Michal
On 4/25/24 17:18, Ondřej Jirman wrote: > On Thu, Apr 25, 2024 at 12:59:29PM GMT, Michal Simek wrote: >>> 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? > > The situation is that RX/TX clocks just come from outside > https://megous.com/dl/tmp/af9e7c9e8d51781b.png via PL. 25MHz clock is > generated by the PHY. > > I solved it eventually. > > I described the PHY generated clock as a fixed-clock node, and added a > reference to it under &clkc as a source for "gem0_emio_clk". > > https://megous.com/git/u-boot/tree/arch/arm/dts/zynq-ebaz-megi.dts?h=v2024.07#n26 > > Zynq clk code seems to fetch gem0_emio_clk from DT for the purpose of > determining the clk rate for gem0 in emio mode: > > https://elixir.bootlin.com/u-boot/latest/source/drivers/clk/clk_zynq.c#L524 > https://elixir.bootlin.com/u-boot/latest/source/drivers/clk/clk_zynq.c#L280 > > This got rid of the "failed to set tx clock rate" error. Thank you for the hint. > > > BTW, I managed to get the .bit file loaded via SPL only with a SPL patch: > > https://megous.com/git/u-boot/commit/?h=v2024.07&id=c1403bd080bc517c9dc6f507ee12b62fa85153bc > > It's similar to how it's done in: > > https://elixir.bootlin.com/u-boot/latest/source/boot/image-board.c#L677 > > fpga_load() function doesn't work with .bit file I get from Vivado but > fpga_loadbitstream() one does. SPL doesn't call it, though. Is that by design? > Should I be converting .bit file to something else usable by fpga_load() > somehow? Yes there was limitation or missing detection which fpga format is supported. > > I'd like to load bitstream in SPL from FIT because U-Boot proper may already > need some PL logic activated for ethernet to work there. Also it's one less > file on the /boot filesystem to worry about. :) I think you have two options. 1. convert your bitstream in bit format to bin format 2. Extend SPL code to support both formats. I would go with option 1 but patches for option 2 are definitely welcome. Thanks, Michal
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); if (srcsel) return emio_clk; else