Re: [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver

[Alexey Brodkin <Alexey.Brodkin@synopsys.com>]

Hi Jose,

On Thu, 2016-04-21 at 10:51 +0100, Jose Abreu wrote:
> Hi Alexey,


> > > > Otherwise, I still prefer two DTS files for the two different FPGA
> > > > versions. At the least, please use ioremap for any pointers that
> > > > you readl/writel here.
> > > > 
> > > > Beyond that, we should have a fixed rate source clk somewhere in
> > > > the software view of the clk tree, because that reflects reality.
> > > > Hardcoding the parent rate in the structure works, but doesn't
> > > > properly express the clk tree.
> > > > 
> > > Can I use a property in the DT to pass this reference clock? something like this:
> > >     snps,parent-freq = <0xFBED9 27000000>, <0x0 28224000>; /* Tuple
> > > <fpga-version reference-clock-freq>, fpga-version = 0 is default */
> > > 
> > > Or use a parent clock? like:
> > >     clk {
> > >         compatible = "fixed-clock";
> > >         clock-frequency = <27000000>;
> > >         #clock-cells = <0>;
> > >         snps,fpga-version = <0xFBED9>;
> > >     }
> > > 
> > > It is important to distinguish between the different versions automatically, is
> > > any of these solutions ok?
> > I do like that solution with a master clock but with some fine-tuning
> > for simplification.
> > 
> > We'll add master clock node for I2S as a fixed clock like that:
> > ------------------->8------------------
> > 	i2s_master_clock: clk {
> > 		#clock-cells = <0>;
> > 		compatible = "fixed-clock";
> > 		clock-frequency = <27000000>;
> > 	};
> > ------------------->8------------------
> > 
> > Note there's no mention of MB version, just a value of the frequency.
> > And in the driver itself value of that master clock will be used for
> > population of "pll_clk->ref_clk" directly.
> > 
> > These are benefits we'll get with that approach:
> >  [1] We escape any IOs not related to our clock device (I mean
> >      "snps,i2s-pll-clock") itself.
> >  [2] We'll use whatever reference clock value is given.
> >      I.e. we'll be able to do a fix-up of that reference clock
> >      value early in platform code depending on HW we're running on.
> >      That's what people do here and there.
> >  [3] Remember another clock driver for AXS10x board is right around
> >      the corner. I mean the one for ARC PGU which uses exactly the same
> >      master clock. So one fixup as mentioned above will work
> >      at once for 2 clock drivers.
> > 
> > Let me know if above makes sense.
> That approach can't be used because the reference clock value will change in the
> next firmware release.  The new release will have a reference clock of 28224000
> Hz instead of the usual 27000000 Hz, so we need to have a way to distinguish
> between them. Because of that we can't have only one master clock unless you
> state to users that they have to change the reference clock value when using the
> new firmware release. Stephen suggested to use two DT files (one for each
> firmware release), but as Vineet said this would be annoying to the user so I am
> trying to use another solution so that only one DT file is required.

Ok reference clock will change.
But I may guess we'll still be able to determine at least that new
firmware version in run-time, right? If so we'll update a fix-up in
early axs10x platform code so that reference clock will be set as 28224000 Hz.

And indeed 2 DT files is a no go - we want to run the same one binary
(with built-in .dtb) on all flavors of AXS boards. And fix-up I'm talking about
will actually do transformation of .dtb early on kernel boot process so that will
be a complete equivalent of different DT files.

-Alexey