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

[Alexandru Ardelean]

On Wed, Sep 30, 2020 at 8:16 PM Moritz Fischer <mdf@xxxxxxxxxx> 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@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,
>         },
>
>         { },
> };
>

oh, apologies for not thinking about this;
i'll spin this up

thanks
Alex

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