RE: [PATCH 1/2] clk: axi-clkgen: add support for ZynqMP (UltraScale)

["Ardelean, Alexandru" <alexandru.Ardelean@analog.com>]

> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Thursday, December 24, 2020 4:03 PM
> To: Ardelean, Alexandru <alexandru.Ardelean@analog.com>; linux-
> clk@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: mturquette@baylibre.com; sboyd@kernel.org; robh+dt@kernel.org;
> lars@metafoo.de; linux-fpga@vger.kernel.org; mdf@kernel.org; Bogdan,
> Dragos <Dragos.Bogdan@analog.com>; Mathias Tausen
> <mta@gomspace.com>
> Subject: Re: [PATCH 1/2] clk: axi-clkgen: add support for ZynqMP (UltraScale)
> 
> [External]
> 
> 
> On 12/21/20 6:42 AM, Alexandru Ardelean wrote:
> > From: Dragos Bogdan <dragos.bogdan@analog.com>
> >
> > This IP core also works and is supported on the Xilinx ZynqMP
> > (UltraScale) FPGA boards.
> > This patch enables the driver to be available on these platforms as well.
> >
> > Since axi-clkgen is now supported on ZYNQMP, we need to make sure the
> > max/min frequencies of the PFD and VCO are respected.
> >
> > This change adds two new compatible strings to select limits for Zynq
> > or ZynqMP from the device data (in the OF table). The old compatible
> > string (i.e. adi,axi-clkgen-2.00.a) is the same as
> > adi,zynq-axi-clkgen-2.00.a, since the original version of this driver
> > was designed on top of that platform.
> >

Apologies for the late reply on this, this looks like it went to some folder in my inbox and I forgot about it.
And Happy New Year :)

> > Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>
> > Signed-off-by: Mathias Tausen <mta@gomspace.com>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >
> > This is a re-spin of an older series.
> > It needed to wait a txt -> yaml dt conversion:
> > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux
> > -clk/patch/20201013143421.84188-1-alexandru.ardelean@analog.com/__;!!A
> > 3Ni8CS0y2Y!sPnN7f9iWjhGkjI-Gu8RTo73gVDV6x0-5m42Pu2kZwvVbHN-
> pZzEaFqTrDY
> > iPPDLdVc-wg$
> >
> > It's 2 patches squashed into one:
> > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux
> > -clk/patch/20200929144417.89816-12-alexandru.ardelean@analog.com/__;!!
> > A3Ni8CS0y2Y!sPnN7f9iWjhGkjI-Gu8RTo73gVDV6x0-5m42Pu2kZwvVbHN-
> pZzEaFqTrD
> > YiPPAkwzbuMA$
> > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux
> > -clk/patch/20200929144417.89816-14-alexandru.ardelean@analog.com/__;!!
> > A3Ni8CS0y2Y!sPnN7f9iWjhGkjI-Gu8RTo73gVDV6x0-5m42Pu2kZwvVbHN-
> pZzEaFqTrD
> > YiPPCaD6QXfQ$
> >
> > The series from where all this started is:
> > https://urldefense.com/v3/__https://lore.kernel.org/linux-clk/20200929
> > 144417.89816-1-
> alexandru.ardelean@analog.com/__;!!A3Ni8CS0y2Y!sPnN7f9i
> > WjhGkjI-Gu8RTo73gVDV6x0-5m42Pu2kZwvVbHN-pZzEaFqTrDYiPPAjuz4oHg$
> >
> > Well, v4 was the point where I decided to split this into smaller
> > series, and also do the conversion of the binding to yaml.
> >
> >  drivers/clk/Kconfig          |  2 +-
> >  drivers/clk/clk-axi-clkgen.c | 15 +++++++++++++++
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index
> > 85856cff506c..252333e585e7 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -247,7 +247,7 @@ config CLK_TWL6040
> >
> >  config COMMON_CLK_AXI_CLKGEN
> >  	tristate "AXI clkgen driver"
> > -	depends on ARCH_ZYNQ || MICROBLAZE || COMPILE_TEST
> > +	depends on ARCH_ZYNQ || ARCH_ZYNQMP || MICROBLAZE ||
> COMPILE_TEST
> >  	help
> >  	  Support for the Analog Devices axi-clkgen pcore clock generator for
> Xilinx
> >  	  FPGAs. It is commonly used in Analog Devices' reference designs.
> > diff --git a/drivers/clk/clk-axi-clkgen.c
> > b/drivers/clk/clk-axi-clkgen.c index ad86e031ba3e..a413c13334ff 100644
> > --- a/drivers/clk/clk-axi-clkgen.c
> > +++ b/drivers/clk/clk-axi-clkgen.c
> > @@ -108,6 +108,13 @@ static uint32_t axi_clkgen_lookup_lock(unsigned int
> m)
> >  	return 0x1f1f00fa;
> >  }
> >
> 
> Could something like
> 
> #ifdef ARCH_ZYNQMP

So, we decided not to do this in an older discussion:
https://lore.kernel.org/linux-clk/20200929153040.GA114067@archbook/
https://lore.kernel.org/linux-clk/20200930171607.GA121420@archbook/

Because, these drivers should also be usable in cases where a ZynqMP or Zynq device can be plugged in via PCIe on a host device.
Thinking about it now, I think I should remove the "depends on ARCH_ZYNQ || ARCH_ZYNQMP" limitation.

> 
> > +static const struct axi_clkgen_limits axi_clkgen_zynqmp_default_limits = {
> > +	.fpfd_min = 10000,
> > +	.fpfd_max = 450000,
> > +	.fvco_min = 800000,
> > +	.fvco_max = 1600000,
> > +};
> 
> #endif
> 
> be added here and similar places to limit unused code ?
> 
> > +
> >  static const struct axi_clkgen_limits axi_clkgen_zynq_default_limits = {
> >  	.fpfd_min = 10000,
> >  	.fpfd_max = 300000,
> > @@ -560,6 +567,14 @@ static int axi_clkgen_remove(struct
> > platform_device *pdev)  }
> >
> >  static const struct of_device_id axi_clkgen_ids[] = {
> > +	{
> > +		.compatible = "adi,zynqmp-axi-clkgen-2.00.a",
> > +		.data = &axi_clkgen_zynqmp_default_limits,
> > +	},
> > +	{
> > +		.compatible = "adi,zynq-axi-clkgen-2.00.a",
> > +		.data = &axi_clkgen_zynq_default_limits,
> > +	},
> 
> This looks like zynqmp AND zynq are being added.
> 
> Is this a mistake ?

The original driver supported only Zynq & Microblaze.
I keep forgetting about Microblaze, because it's one of those target-devices which aren't that popular compared to Zynq/ZynqMP [because of their lower speeds]; though they do pop-up.
I thought I'd add an explicit extra compatible string for Zynq to be able to differentiate it [explicitly], in the device tree.
But I think, it could make sense to just add the zynqmp compat string for now. 

Will fix my inbox filters first and re-spin.

Thanks
Alex

> 
> Tom
> 
> >  	{
> >  		.compatible = "adi,axi-clkgen-2.00.a",
> >  		.data = &axi_clkgen_zynq_default_limits,