Hi Simon,
On Friday 25 April 2014 09:26:42 Simon Horman wrote:
> On Fri, Apr 25, 2014 at 01:33:29AM +0200, Laurent Pinchart wrote:
> > Hi Simon,
> >
> > Thank you for the patch.
> >
> > On Thursday 24 April 2014 15:54:44 Simon Horman wrote:
> > > According to the platform data for the legacy-C initialisation of sh-sci
> > > for the r8a7779 SoC and my own testing the SCIx_SH4_SCIF_REGTYPE bit of
> > > scscr needs to be set.
> > >
> > > Signed-off-by: Simon Horman <horms+renesas@xxxxxxxxxxxx>
> > >
> > > ---
> > >
> > > With the approach taken by this patch sh-sci may be initialised using
> > > the "renesas,scif-r8a7779" compat string but not the generic
> > > "renesas,scif" compat string. An alternate approach would be to add a
> > > binding to allow setting of this bit to be controlled directly from DT.
> >
> > Handling the SCSCR bits properly with DT has been on my to-do list for
> > some time. Thank you for volunteering to implement that :-D
> >
> > The CKE bits control both the behaviour of the SCK pin and the SCIF input
> > clock selection. The SCIF can use various internal and external clocks,
> > with up to two baud rate generators (programmable dividers) chained.
> > Whether the SoC is provided with an external clock for its serial ports
> > is a board property, not an SoC property. I would thus like to implement
> > support for this feature properly and avoid hardcoding the CKE bits like
> > done below.
> >
> > The Marzen board has an external clock generator connected to the SCIF_SCK
> > pin that can be used as a clock source, but I don't really see a reason
> > why we couldn't use the internal P clock instead. I don't want to delay
> > integration of SCIF DT support until we have a proper solution to handle
> > clock configuration, but couldn't we use the internal P clock on the
> > Marzen board in the meantime ?
>
> I think that approach sounds reasonable and I seem to have it working
> without any driver changes. Could you confirm that the following
> DT snippet is what you have in mind? (I intend to update it as per any
> relevant changes you make when reposting your DT sci patch for the
> r8a7791.)
That's what I have in mind, but please see below for a small comment.
> diff --git a/arch/arm/boot/dts/r8a7779.dtsi b/arch/arm/boot/dts/r8a7779.dtsi
> index e924f96..3e9cca4 100644
> --- a/arch/arm/boot/dts/r8a7779.dtsi
> +++ b/arch/arm/boot/dts/r8a7779.dtsi
> @@ -203,6 +203,66 @@
> status = "disabled";
> };
>
> + scif0: serial@ffe40000 {
> + compatible = "renesas,scif", "renesas,scif-r8a7779";
> + reg = <0xffe40000 265>;
> + interrupt-parent = <&gic>;
> + interrupts = <0 88 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cpg_clocks R8A7779_CLK_P>;
> + clock-names = "sci_ick";
Clock handling in the sh-sci driver should probably be improved. The driver
currently requires an "sci_ick" interface clock and supports an optional
"sci_fck" functional clock. In practice, as far as I can see, platforms that
provide both sci_ick and sci_fck set the two clocks to the same source.
Furthermore, some SCI* instances support a second internal clock source for
the baud rate generator. We don't support that at the moment.
This leads me to believe that we should merge the "sci_ick" and "sci_fck"
clocks for the DT case into a single clock that I would call "fck" (no need
for a "sci_" prefix), and later add support for the second baud rate clock.
This would require a small driver change. I can try to submit patches in the
next couple of days if you agree with the approach.
> + status = "disabled";
> + };
> +
> + scif1: serial@ffe41000 {
> + compatible = "renesas,scif", "renesas,scif-r8a7779";
> + reg = <0xffe41000 265>;
> + interrupt-parent = <&gic>;
> + interrupts = <0 89 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cpg_clocks R8A7779_CLK_P>;
> + clock-names = "sci_ick";
> + status = "disabled";
> + };
> +
> + scif2: serial@ffe42000 {
> + compatible = "renesas,scif", "renesas,scif-r8a7779";
> + reg = <0xffe42000 265>;
> + interrupt-parent = <&gic>;
> + interrupts = <0 90 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cpg_clocks R8A7779_CLK_P>;
> + clock-names = "sci_ick";
> + status = "disabled";
> + };
> +
> + scif3: serial@ffe43000 {
> + compatible = "renesas,scif", "renesas,scif-r8a7779";
> + reg = <0xffe43000 265>;
> + interrupt-parent = <&gic>;
> + interrupts = <0 91 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cpg_clocks R8A7779_CLK_P>;
> + clock-names = "sci_ick";
> + status = "disabled";
> + };
> +
> + scif4: serial@ffe44000 {
> + compatible = "renesas,scif", "renesas,scif-r8a7779";
> + reg = <0xffe44000 265>;
> + interrupt-parent = <&gic>;
> + interrupts = <0 92 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cpg_clocks R8A7779_CLK_P>;
> + clock-names = "sci_ick";
> + status = "disabled";
> + };
> +
> + scif5: serial@ffe45000 {
> + compatible = "renesas,scif", "renesas,scif-r8a7779";
> + reg = <0xffe45000 265>;
> + interrupt-parent = <&gic>;
> + interrupts = <0 93 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cpg_clocks R8A7779_CLK_P>;
> + clock-names = "sci_ick";
> + status = "disabled";
> + };
> +
> pfc: pfc@fffc0000 {
> compatible = "renesas,pfc-r8a7779";
> reg = <0xfffc0000 0x23c>;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
