Re: [RFC/PATCH 2/6] serial: sh-sci: Drop the interface clock

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Simon,

On Tuesday 13 May 2014 13:06:12 Simon Horman wrote:
> On Tue, May 13, 2014 at 10:06:57AM +0900, Simon Horman wrote:
> > On Mon, May 12, 2014 at 10:23:31PM +0200, Laurent Pinchart wrote:
> > > On Wednesday 30 April 2014 15:43:04 Simon Horman wrote:
> > > > On Wed, Apr 30, 2014 at 02:31:42AM +0200, Laurent Pinchart wrote:
> > > > > As no platform defines an interface clock the SCI driver always fall
> > > > > back to a clock named "peripheral_clk". On SH platform that clock is
> > > > > the base clock for the SCI functional clock and has the same
> > > > > frequency. On ARM platforms that clock doesn't exist, and clk_get()
> > > > > will return the default clock for the device. We can thus make the
> > > > > functional clock mandatory and drop the interface clock.
> > > > > 
> > > > > Signed-off-by: Laurent Pinchart
> > > > > <laurent.pinchart+renesas@ideasonboard.com>
> > > > > ---
> > > > > 
> > > > >  .../bindings/serial/renesas,sci-serial.txt         |  4 +-
> > > > >  drivers/tty/serial/sh-sci.c                        | 52 ++++++-----
> > > > >  2 files changed, 31 insertions(+), 25 deletions(-)
> > > > > 
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
> > > > > b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
> > > > > index
> > > > > 53e6c17..efc9eda 100644
> > > > > ---
> > > > > a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
> > > > > +++
> > > > > b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
> > > > > 
> > > > > @@ -26,7 +26,7 @@ Required properties:
> > > > >    - clocks: Must contain a phandle and clock-specifier pair for
> > > > >      each entry in clock-names.
> > > > > -  - clock-names: Must contain "sci_ick" for the SCIx UART interface
> > > > > clock.
> > > > > +  - clock-names: Must contain "fck" for the SCIx UART functional
> > > > > clock.
> > > > 
> > > > I have tested that this patch works on the Koelsch and Lager boards
> > > > with the remainder of the series applied and a fixup for the Lager
> > > > patch, as noted in a response to that patch.
> > > > 
> > > > I have also tested that this patch works on the Marzen board with a
> > > > modified version of my series to enable SCI via DT on that board.
> > > > 
> > > > It was my understanding that this patch would not break existing
> > > > bindings. However, if my reading and testing of this patch correct
> > > > this patch modifies an existing binding, included in v3.14, in an
> > > > incompatible way. Requiring all users of "sci_ick" to be updated to
> > > > "fck".
> > > 
> > > You're right. Lack of testing mixed with coding during a 12h flight
> > > makes it easy to overlook basic issues :-)
> > > 
> > > > This includes the DT nodes provided for the r8a7790 and r8a7791 which
> > > > are present in v3.15-rc1 and will also be included in v3.15 unless
> > > > this patch, its dependencies and the patches in this series that
> > > > update the dtsi for those SoCs are submitted as fixes for v3.15. (I am
> > > > aware the nodes in question are "disabled" :)
> > > > 
> > > > I would prefer if this change it was done in a way that maintains
> > > > compatibility with the published binding.
> > > 
> > > I have mixed feelings here. On one hand you're right from a theoretical
> > > point of view. On the other hand, the bindings are so recent, and the
> > > DT nodes are marked as "disabled", so I'm pretty sure nobody uses them.
> > > It would be a shame to have to keep compatibility code around forever
> > > in the sh-sci driver, even if the code itself wouldn't be too complex.
> > > How much do you insist on this ?> > 
> > > :-)
> > 
> > I am prepared to negotiate :)
> > 
> > As you point out the only users in mainline are marked as disabled.
> > And I am not aware of anyone using them in the wild (which is not to
> > say that they aren't).
> > 
> > So I am inclined, as you seem to feel strongly about this, to make an
> > exception and allow a non-compatible change.
> 
> I have subsequently noticed that DT files for the r8a7791/henninger,
> which are already queued up for v3.16, use the existing binding.
> So, contrary to what I said immediately above, I really feel that
> compatibility has to be maintained.

Come on, that's easy to fix, isn't it ? :-)

I can resubmit this series with support for "sci_ick" for v3.16, with a patch 
that moves r8a7791/henninger to the "fck" clock also for v3.16, and then drop 
"sci_ick" support in v3.17. Would that be fine with you ?

> > > > Apart from addressing my concern above such an approach would also
> > > > allow patches that enable SCI via DT (using "sci_ick") to be merged
> > > > independently of this driver change. Although I am happy to wait on
> > > > those if you have a strong preference for them to use the updated
> > > > binding.
> > > 
> > > I don't have a strong preference here, except that I'd like to drop
> > > support for the sci_ick clock name completely, which wouldn't play very
> > > well with adding new users right now :-)
> > 
> > I am quite happy to delay accepting (new) users of the bindings until this
> > is resolved. But I would like to point out that I think that will result
> > in one of the two:
> > 
> > 1. A negotiation with Greg such that I can take the driver change
> >    through the shmobile tree, or;
> > 
> > 2. Some delay in accepting (new) users of the binding as I would need
> >    to wait for the binding change to appear in a branch I can use as a
> >    base before taking the patches to add new users.
> >    
> > > > >  Note: Each enabled SCIx UART should have an alias correctly
> > > > >  numbered in the "aliases" node.
> > > > > 
> > > > > @@ -42,5 +42,5 @@ Example:
> > > > >  		interrupt-parent = <&gic>;
> > > > >  		interrupts = <0 144 IRQ_TYPE_LEVEL_HIGH>;
> > > > >  		clocks = <&mstp2_clks R8A7790_CLK_SCIFA0>;
> > > > > 
> > > > > -		clock-names = "sci_ick";
> > > > > +		clock-names = "fck";
> > > > > 
> > > > >  	};

-- 
Regards,

Laurent Pinchart