Re: [PATCH v4 5/7] clk: axi-clkgen: Respect ZYNQMP PFD/VCO frequency limits

[Moritz Fischer]

On Wed, Sep 30, 2020 at 08:22:23AM +0300, Alexandru Ardelean wrote:
> On Tue, Sep 29, 2020 at 6:30 PM Moritz Fischer <mdf@xxxxxxxxxx> wrote:
> >
> > Hi Alexandru,
> >
> > On Tue, Sep 29, 2020 at 05:44:15PM +0300, Alexandru Ardelean wrote:
> > > From: Mathias Tausen <mta@xxxxxxxxxxxx>
> > >
> > > Since axi-clkgen is now supported on ZYNQMP, make sure the max/min
> > > frequencies of the PFD and VCO are respected.
> > >
> > > Signed-off-by: Mathias Tausen <mta@xxxxxxxxxxxx>
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> >
> > This patch still does not cover the PCIe Zynq plugged into ZynqMP linux
> > machine case.
> >
> > > ---
> > >  drivers/clk/clk-axi-clkgen.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/clk/clk-axi-clkgen.c b/drivers/clk/clk-axi-clkgen.c
> > > index 4342b7735590..2319bb1c5c08 100644
> > > --- a/drivers/clk/clk-axi-clkgen.c
> > > +++ b/drivers/clk/clk-axi-clkgen.c
> > > @@ -108,12 +108,21 @@ static uint32_t axi_clkgen_lookup_lock(unsigned int m)
> > >       return 0x1f1f00fa;
> > >  }
> > >
> > > +#ifdef ARCH_ZYNQMP
> > > +static const struct axi_clkgen_limits axi_clkgen_default_limits = {
> > > +     .fpfd_min = 10000,
> > > +     .fpfd_max = 450000,
> > > +     .fvco_min = 800000,
> > > +     .fvco_max = 1600000,
> > > +};
> > > +#else
> > >  static const struct axi_clkgen_limits axi_clkgen_default_limits = {
> > >       .fpfd_min = 10000,
> > >       .fpfd_max = 300000,
> > >       .fvco_min = 600000,
> > >       .fvco_max = 1200000,
> > >  };
> > > +#endif
> >
> > I still don't understand this. You have a way to determine which fabric
> > you are looking at with the FPGA info. Why not:
> >
> > [..] axi_clkgen_zynqmp_default_limits = {
> > };
> >
> > [..] axi_clkgen_default_limits = {
> > };
> >
> > Set them based on what you read back, i.e. determine which fabric you
> > are looking at *per clock gen* and use that info, rather than making a
> > compile time decision to support only one of them.
> >
> > Generally speaking #ifdef $ARCH should be a last resort solution.
> 
> The support for reading back the fabric parameters is implemented in
> the AXI CLKGEN PCORE version starting with 5.0.a
> Links:
> https://github.com/analogdevicesinc/hdl/commits/master/library/common/up_clkgen.v
> https://github.com/analogdevicesinc/hdl/commit/66823682b63c1037abdc3fc1dd4d4e63d3cfbc1a
> https://github.com/analogdevicesinc/hdl/commit/7dcb2050c7946fab5ea5a273eda7c53ea7b969a6
> 
> Before that version, these details aren't there, so the best you can
> do is assume compile-time ARCH defaults.

This is a property of the instance and not of the driver. If you can't
query the hardware to figure out what you're looking at, but have
different behaviours, please use different compatible strings and make
the default limits platform data.

Something like this:

static const struct of_device_id axi_clkgen_ids[] = {
	{
		.compatible = "foo-zynqmp",
		.data = &zynqmp_default_limits,
	},
	{
		.compatible = "bar-zynq",
		.data = &zynq_default_limits,
	},

	{ },
};

And pull the info out in your probe function, then you can have both
configurations and select via device-tree.

Thanks,
Moritz