* Re: [RFC/PATCH 2/6] serial: sh-sci: Drop the interface clock
2014-04-30 0:31 [RFC/PATCH 2/6] serial: sh-sci: Drop the interface clock Laurent Pinchart
@ 2014-04-30 6:43 ` Simon Horman
2014-05-12 20:23 ` Laurent Pinchart
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2014-04-30 6:43 UTC (permalink / raw)
To: linux-sh
Hi Laurent,
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".
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.
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.
> 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";
> };
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 88236da..ea4255c 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -90,8 +90,6 @@ struct sci_port {
> struct timer_list break_timer;
> int break_flag;
>
> - /* Interface clock */
> - struct clk *iclk;
> /* Function clock */
> struct clk *fclk;
>
> @@ -442,9 +440,8 @@ static void sci_port_enable(struct sci_port *sci_port)
>
> pm_runtime_get_sync(sci_port->port.dev);
>
> - clk_prepare_enable(sci_port->iclk);
> - sci_port->port.uartclk = clk_get_rate(sci_port->iclk);
> clk_prepare_enable(sci_port->fclk);
> + sci_port->port.uartclk = clk_get_rate(sci_port->fclk);
> }
>
> static void sci_port_disable(struct sci_port *sci_port)
> @@ -461,7 +458,6 @@ static void sci_port_disable(struct sci_port *sci_port)
> sci_port->break_flag = 0;
>
> clk_disable_unprepare(sci_port->fclk);
> - clk_disable_unprepare(sci_port->iclk);
>
> pm_runtime_put_sync(sci_port->port.dev);
> }
> @@ -1045,7 +1041,7 @@ static int sci_notifier(struct notifier_block *self,
> struct uart_port *port = &sci_port->port;
>
> spin_lock_irqsave(&port->lock, flags);
> - port->uartclk = clk_get_rate(sci_port->iclk);
> + port->uartclk = clk_get_rate(sci_port->fclk);
> spin_unlock_irqrestore(&port->lock, flags);
> }
>
> @@ -2097,6 +2093,30 @@ static struct uart_ops sci_uart_ops = {
> #endif
> };
>
> +static int sci_init_clocks(struct sci_port *sci_port, struct device *dev)
> +{
> + /* Get the SCI functional clock. It's called "fck" on ARM. */
> + sci_port->fclk = clk_get(dev, "fck");
> + if (!IS_ERR(sci_port->fclk))
> + return 0;
> +
> + /* SH has historically named the clock "sci_fck". */
> + sci_port->fclk = clk_get(dev, "sci_fck");
> + if (!IS_ERR(sci_port->fclk))
> + return 0;
> +
> + /*
> + * Not all SH platforms declare a clock lookup entry for SCI devices, in
> + * which case we need to get the global "peripheral_clk" clock.
> + */
> + sci_port->fclk = clk_get(dev, "peripheral_clk");
> + if (!IS_ERR(sci_port->fclk))
> + return 0;
> +
> + dev_err(dev, "failed to can't get fclk\n");
> + return PTR_ERR(sci_port->fclk);
> +}
> +
> static int sci_init_single(struct platform_device *dev,
> struct sci_port *sci_port, unsigned int index,
> struct plat_sci_port *p, bool early)
> @@ -2183,22 +2203,9 @@ static int sci_init_single(struct platform_device *dev,
> : sampling_rate;
>
> if (!early) {
> - sci_port->iclk = clk_get(&dev->dev, "sci_ick");
> - if (IS_ERR(sci_port->iclk)) {
> - sci_port->iclk = clk_get(&dev->dev, "peripheral_clk");
> - if (IS_ERR(sci_port->iclk)) {
> - dev_err(&dev->dev, "can't get iclk\n");
> - return PTR_ERR(sci_port->iclk);
> - }
> - }
> -
> - /*
> - * The function clock is optional, ignore it if we can't
> - * find it.
> - */
> - sci_port->fclk = clk_get(&dev->dev, "sci_fck");
> - if (IS_ERR(sci_port->fclk))
> - sci_port->fclk = NULL;
> + ret = sci_init_clocks(sci_port, &dev->dev);
> + if (ret < 0)
> + return ret;
>
> port->dev = &dev->dev;
>
> @@ -2252,7 +2259,6 @@ static int sci_init_single(struct platform_device *dev,
>
> static void sci_cleanup_single(struct sci_port *port)
> {
> - clk_put(port->iclk);
> clk_put(port->fclk);
>
> pm_runtime_disable(port->port.dev);
> --
> 1.8.3.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC/PATCH 2/6] serial: sh-sci: Drop the interface clock
2014-04-30 0:31 [RFC/PATCH 2/6] serial: sh-sci: Drop the interface clock Laurent Pinchart
2014-04-30 6:43 ` Simon Horman
@ 2014-05-12 20:23 ` Laurent Pinchart
2014-05-13 1:06 ` Simon Horman
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Laurent Pinchart @ 2014-05-12 20:23 UTC (permalink / raw)
To: linux-sh
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 ?
:-)
> 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 :-)
> > 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC/PATCH 2/6] serial: sh-sci: Drop the interface clock
2014-04-30 0:31 [RFC/PATCH 2/6] serial: sh-sci: Drop the interface clock Laurent Pinchart
2014-04-30 6:43 ` Simon Horman
2014-05-12 20:23 ` Laurent Pinchart
@ 2014-05-13 1:06 ` Simon Horman
2014-05-13 4:06 ` Simon Horman
2014-05-13 11:44 ` Laurent Pinchart
4 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2014-05-13 1:06 UTC (permalink / raw)
To: linux-sh
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC/PATCH 2/6] serial: sh-sci: Drop the interface clock
2014-04-30 0:31 [RFC/PATCH 2/6] serial: sh-sci: Drop the interface clock Laurent Pinchart
` (2 preceding siblings ...)
2014-05-13 1:06 ` Simon Horman
@ 2014-05-13 4:06 ` Simon Horman
2014-05-13 11:44 ` Laurent Pinchart
4 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2014-05-13 4:06 UTC (permalink / raw)
To: linux-sh
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:
> > 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.
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.
> > > 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
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC/PATCH 2/6] serial: sh-sci: Drop the interface clock
2014-04-30 0:31 [RFC/PATCH 2/6] serial: sh-sci: Drop the interface clock Laurent Pinchart
` (3 preceding siblings ...)
2014-05-13 4:06 ` Simon Horman
@ 2014-05-13 11:44 ` Laurent Pinchart
4 siblings, 0 replies; 6+ messages in thread
From: Laurent Pinchart @ 2014-05-13 11:44 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 6+ messages in thread