Re: [PATCH v3 01/20] clk: shmobile: r8a7779: Add clocks support

[Simon Horman <horms@verge.net.au>]

On Thu, Mar 13, 2014 at 10:07:45AM +0100, Laurent Pinchart wrote:
> Hi Simon,
> 
> On Thursday 13 March 2014 11:22:47 Simon Horman wrote:
> > On Wed, Mar 12, 2014 at 11:12:02AM +0100, Laurent Pinchart wrote:
> > > On Wednesday 12 March 2014 16:55:23 Simon Horman wrote:
> > > > On Wed, Feb 26, 2014 at 02:02:37PM +0100, Laurent Pinchart wrote:
> > >  > On Wednesday 26 February 2014 13:53:17 Laurent Pinchart wrote:
> > > > > > On Wednesday 26 February 2014 16:33:17 Simon Horman wrote:
> > > > > > > The R8A7779 SoC has several clocks that are too custom to be
> > > > > > > supported in a generic driver. Those clocks can be divided in two
> > > > > > > categories:
> > > > > > > 
> > > > > > > - Fixed rate clocks with multiplier and divisor set according to
> > > > > > >   boot mode configuration
> > > > > > > 
> > > > > > > - Custom divider clocks with SoC-specific divider values
> > > > > > > 
> > > > > > > This driver supports both.
> > > > > > 
> > > > > > Looking at the R8A7779 datasheet it looks like we only have fixed
> > > > > > rate clocks, without any configurable divider clock. Did I miss
> > > > > > something ?
> > > > 
> > > > Sorry, its a cut-and past error on my part.
> > > > I will revise the changelog.
> > > > 
> > > > > > > Based on work for R-Car Gen2 SoCs by Laurent Pinchart.
> > > > > > > 
> > > > > > > Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > > > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> > > > > > > 
> > > > > > > ---
> > > > > > > v3
> > > > > > > * As suggested by Laurent Pinchart
> > > > > > > 
> > > > > > >   - Added external clock input
> > > > > > >   - Use PLLA ratio set bu MD11 and MD12
> > > > > > >   - Add _div suffixes of fields of struct cpt_clk_config
> > > > > > >   - Register PLLA as a fixed factor clock
> > > > > > >   - Use sizeof() instead of sizeof
> > > > > > >   - Use num_clks instead of CPG_NUM_CLOCKS in
> > > > > > >   r8a7779_cpg_clocks_init()
> > > > > > >   
> > > > > > >   - I kept this as r8a7779 binding rather than moving to a R-Car
> > > > > > >   Gen1 binding which could be shared with other SoCs as I do not
> > > > > > >   believe that the SoCs is are sufficiently similar.
> > > > > > 
> > > > > > I had a look at the M1 datasheet and I still find its CPG very
> > > > > > similar with the H1 CPG. The PLLA multiplier and divider are
> > > > > > different, but if you look closely, they're both exactly twice the
> > > > > > value compared to H1, so there's no difference in practice.
> > > > > > 
> > > > > > What differences do you see that would make it impractical to share
> > > > > > a single driver for both ?
> > > > 
> > > > Thanks for pointing that out. Looking over the M1 datasheet again I
> > > > agree with you that there does seem to be rather a lot of similarities
> > > > with the H1. And I agree that it is likely that the driver could be
> > > > shared.
> > > > 
> > > > However, I'd like to leave that as future work at this time.
> > > > 
> > > > The reason for this is that on the one hand I believe such work sould be
> > > > done in conjunctin with integrating the driver on the bockw board.
> > > > And that seems to be non-trivial to me.
> > > 
> > > We can always rename the driver later, that's not a big issue, but we
> > > can't change compatibility strings. I would thus like to see
> > > "renesas,rcar-gen1-cpg-clocks" listed in the DT bindings.
> > 
> > I know that we have done such things in the past but these days I feel that
> > it is a bit contentious as it is a binding for a software abstraction
> > rather than the hardware. And as far as I know bindings are supposed to
> > describe the hardware.
> 
> Sure, and I consider rcar-gen1-cpf is a hardware description. We have two 
> generations of SoCs in the R-Car series so far, the first generation (H1 and 
> M1) and the second generation (H2 and M2). The second generation is even 
> explictly named as such in the M2 datasheet. SoCs within a generation share 
> hardware characteristics, and it makes sense to reflect that in compatible 
> strings.

I think that if it is documented, as it is for R-Car Gen 2, then
this is entirely reasonable and I agree that it describes the hardware.

However, if it is not documented I don't think that we can make assumptions
and thus I don't think it is appropriate for R-Car Gen 1.