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

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

On Mon, May 12, 2014 at 10:23:31PM +0200, Laurent Pinchart wrote:
> Hi Simon,
> 
> 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.

> > 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
>