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

[Moritz Fischer <mdf@kernel.org>]

On Thu, Oct 01, 2020 at 08:18:59AM +0300, Alexandru Ardelean wrote:
> On Wed, Sep 30, 2020 at 8:16 PM Moritz Fischer <mdf@kernel.org> wrote:
> >
> > 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@kernel.org> wrote:
> > > >
> > > > Hi Alexandru,
> > > >
> > > > On Tue, Sep 29, 2020 at 05:44:15PM +0300, Alexandru Ardelean wrote:
> > > > > From: Mathias Tausen <mta@gomspace.com>
> > > > >
> > > > > 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@gomspace.com>
> > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > >
> > > > 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,
> >         },
> >
> >         { },
> > };
> >
> 
> oh, apologies for not thinking about this;
> i'll spin this up

No worries :) 

Moritz