From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Horman Date: Fri, 14 Mar 2014 00:38:09 +0000 Subject: Re: [PATCH v3 01/20] clk: shmobile: r8a7779: Add clocks support Message-Id: <20140314003808.GD28137@verge.net.au> List-Id: References: <1393400016-23433-1-git-send-email-horms+renesas@verge.net.au> <3299454.uLzSWehPDg@avalon> <20140313022246.GA8802@verge.net.au> <28465199.RhyHDRhTSc@avalon> In-Reply-To: <28465199.RhyHDRhTSc@avalon> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-arm-kernel@lists.infradead.org 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 > > > > > > > Signed-off-by: Simon Horman > > > > > > > > > > > > > > --- > > > > > > > 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.