[PATCH 0/7] primecell: make correct clock parsing possible

[PATCH 0/7] primecell: make correct clock parsing possible

[Mark Rutland]

Currently either the drivers for primecell peripherals and their bindings
disagree, or those bindings disagree with the primecell binding which they
derive from. It is impossible in some cases to meet the requirements of both
bindings and drivers.

These patches attempt to harmonize the bindings and the drivers with what's in
use today, in a backwards compatible fashion, relieving us of our present
Kafkaesque nightmare. Each peripheral's clock(s) are given explicit names which
can be used, though code will fall back to the existing behaviour if said names
are not provided. Additionally the currently unmet ordering requirement of
apb_pclk is dropped, given all existing that code requires this to be named
anyway.

I've used IS_ERR to test is a clock wasn't provided by name, but this isn't
always right. In the case of a dodgy clock specifier we might get an error,
even if the expected name was provided explicitly in clock-names. For that case
it would be nice to fail rather than grabbing an almost certainly incorrect
clock. I'm not entirely sure how to check for that with the current
infrastructure though, and while it's possible to use of_property_match_string
to achieve the desired effect, it feels like working around the abstraction we
have in place today.

There are some other issues in the area which remain:

* The pl041 exists in DTs, but has no binding.

* Both pl110 and pl111 have no binding, but appear to be in use on OF
  platforms, with the nspire code proving some sideband data via
  OF_DEV_AUXDATA. The driver grabs a clock (CLCDCLK) without using a name.

* I'm not sure what to do with sp804. The bindings imply a given set of names
  with a specific ordering, but all the dts do something different and the
  driver doesn't bother with names. The given binding is incompatible with the
  primecell binding's ordering requirement for apb_plck.

* There's no binding for the sp805, which grabs a clock with no name.

* There's no binding for the pl341 or pl354. Both seem to be unused yet exist
  in DTs.

* The PL330 docs don't mention clocks at all, though the apb_pclk is required.
  Use of PCLKEN isn't supported, but this doesn't seem to be a problem so far.

Thanks,
Mark.

Mark Rutland (7):
  Documentation: devicetree: fix up pl011 clocks
  serial: amba-pl011: attempt to get uartclk by name
  Documentation: devicetree: fix up pl022 clocks
  spi: pl022: attempt to get sspclk by name
  Documentation: devicetree: fix up pl18x clocks
  mmc: arm-mmci: attempt to get mclk by name
  Documentation: devicetree: loosen primecell clock requirements

 Documentation/devicetree/bindings/arm/primecell.txt | 11 ++++++-----
 Documentation/devicetree/bindings/mmc/mmci.txt      |  4 ++++
 Documentation/devicetree/bindings/serial/pl011.txt  |  6 ++++--
 Documentation/devicetree/bindings/spi/spi_pl022.txt |  2 ++
 drivers/mmc/host/mmci.c                             |  9 ++++++++-
 drivers/spi/spi-pl022.c                             |  9 ++++++++-
 drivers/tty/serial/amba-pl011.c                     |  9 ++++++++-
 7 files changed, 40 insertions(+), 10 deletions(-)

-- 
1.8.1.1

[PATCH 1/7] Documentation: devicetree: fix up pl011 clocks

[Mark Rutland]

The "arm,pl011" device tree binding only describes the apb_pclk clock
input, which is not sufficient to use the device. Knowledge of the
uartclk clock input is required to be able to change the baud rate, as
the baud rate is derived from the reference uartclk input. On systems
where the uartclk input is not initially enabled, it is also required to
use the device in any fashion.

This patch adds the uartclk input to the pl011 device tree binding. The
clock-names property is also described, as it is an implied requirement
of the primecell binding the pl011 binding is derived from.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
---
 Documentation/devicetree/bindings/serial/pl011.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/pl011.txt b/Documentation/devicetree/bindings/serial/pl011.txt
index 5d2e840..f0a9e77 100644
--- a/Documentation/devicetree/bindings/serial/pl011.txt
+++ b/Documentation/devicetree/bindings/serial/pl011.txt
@@ -8,8 +8,10 @@ Required properties:
 Optional properties:
 - pinctrl: When present, must have one state named "sleep"
 	   and one state named "default"
-- clocks:  When present, must refer to exactly one clock named
-	   "apb_pclk"
+- clocks:  When present, must refer to a clock named
+           "apb_pclk", and optionally "uartclk".
+- clock-names: When present, should include "apb_pclk" and
+	   "uartclk", matching the clocks property.
 - dmas:	   When present, may have one or two dma channels.
 	   The first one must be named "rx", the second one
 	   must be named "tx".
-- 
1.8.1.1

[PATCH 2/7] serial: amba-pl011: attempt to get uartclk by name

[Mark Rutland]

The primecell device tree binding (from which the pl011 binding is
derived from) states that the apb_pclk clock input should be listed
first for all primecell devices. The amba-pl011 driver requires the
uartclk clock input to enable the uart and calculate the correct uart
baud rate, but the way it currently grabs the clock means that it always
gets the first clock (which should be apb_pclk).

As the AMBA bus code grabs apb_pclk by name, some existing dts provide
uartclk then apb_pclk by name to work around this, in violation of both
the primecell binding and the pl011 binding. Some dtbs only provide
apb_pclk, which is evidently at a similar enough frequency to uartclk on
those platforms to allow the driver to function.

This patch attempts to fix the mess my having the amba-pl011 driver
first attempt to get uartclk by name. If this fails, it falls back to
the old behaviour of simply acquiring the first clock. This is
compatible with any old dtb, whether it lists uartclk by name or not,
and allows the driver to support dtbs which do not violate either
binding.

Signed-off-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
---
 drivers/tty/serial/amba-pl011.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index d58783d..067952a 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2123,7 +2123,14 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 		goto out;
 	}
 
-	uap->clk = devm_clk_get(&dev->dev, NULL);
+	/*
+	 * For compatibility with old DTBs and platform data, fall back to the
+	 * first clock if there's not an explicitly named "uartclk" entry.
+	 */
+	uap->clk = devm_clk_get(&dev->dev, "uartclk");
+	if (IS_ERR(uap->clk))
+		uap->clk = devm_clk_get(&dev->dev, NULL);
+
 	if (IS_ERR(uap->clk)) {
 		ret = PTR_ERR(uap->clk);
 		goto out;
-- 
1.8.1.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/7] Documentation: devicetree: fix up pl022 clocks

[Mark Rutland]

Currently the pl022 driver expects clocks, and dts provide them, yet the
binding does not mention clocks at all.

This patch adds a description of the clocks, "apb_pclk" (as required by
the primecell binding) and "sspclk" for the pl022 itself. The "sspclk"
name was chosen to match the official documentation, as currently a
variety of names are used in its place; it is expected that any
operating system supporting these can continue to do so in the absence
of an "sspclk" entry.

Signed-off-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
---
 Documentation/devicetree/bindings/spi/spi_pl022.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi_pl022.txt b/Documentation/devicetree/bindings/spi/spi_pl022.txt
index 22ed679..326e8e2 100644
--- a/Documentation/devicetree/bindings/spi/spi_pl022.txt
+++ b/Documentation/devicetree/bindings/spi/spi_pl022.txt
@@ -21,6 +21,8 @@ Optional properties:
 - dma-names: Names for the dma channels, if present. There must be at
 	     least one channel named "tx" for transmit and named "rx" for
              receive.
+- clocks: phandle + clock-specifiers, one for each entry in clock-names.
+- clock-names: should contain "apb_pclk" and "sspclk".
 
 
 SPI slave nodes must be children of the SPI master node and can
-- 
1.8.1.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/7] Documentation: devicetree: fix up pl022 clocks

[Linus Walleij]

On Tue, Feb 11, 2014 at 12:37 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:

> Currently the pl022 driver expects clocks, and dts provide them, yet the
> binding does not mention clocks at all.
>
> This patch adds a description of the clocks, "apb_pclk" (as required by
> the primecell binding) and "sspclk" for the pl022 itself. The "sspclk"
> name was chosen to match the official documentation, as currently a
> variety of names are used in its place; it is expected that any
> operating system supporting these can continue to do so in the absence
> of an "sspclk" entry.
>
> Signed-off-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>

Reviewed-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/7] spi: pl022: attempt to get sspclk by name

[Mark Rutland]

The primecell device tree binding (from which the pl022 binding is
derived from) states that the apb_pclk clock input should be listed
first for all primecell devices. The spi-pl022 driver requires the
sspclk clock input to enable the SPI bus, but the way it currently grabs
the clock means that it always gets the first clock (which should be
apb_pclk).

As the AMBA bus code grabs apb_pclk by name, some existing dts provide
apb_pclk as the second clock in the clocks list to work around this, in
violation of both the primecell binding. The pl022 binding does not
mention clocks at all, so the first clock (SSPCLK) is given an arbitrary
name.

This patch attempts to fix the mess my having the spi-pl022 driver first
attempt to get sspclk by name. If this fails, it falls back to the old
behaviour of simply acquiring the first clock. This is compatible with
any old dtb, whether it lists sspclk by name or not, and allows the
driver to support dtbs which do not violate the bindings. Hopefully this
will lead to future uniformity across dtbs.

Signed-off-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
---
 drivers/spi/spi-pl022.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index 2789b45..4b3941a 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -2176,7 +2176,14 @@ static int pl022_probe(struct amba_device *adev, const struct amba_id *id)
 	dev_info(&adev->dev, "mapped registers from %pa to %p\n",
 		&adev->res.start, pl022->virtbase);
 
-	pl022->clk = devm_clk_get(&adev->dev, NULL);
+	/*
+	 * For compatibility with old DTBs and platform data, fall back to the
+	 * first clock if there's not an explicitly named "sspclk" entry.
+	 */
+	pl022->clk = devm_clk_get(&adev->dev, "sspclk");
+	if (IS_ERR(pl022->clk))
+		pl022->clk = devm_clk_get(&adev->dev, NULL);
+
 	if (IS_ERR(pl022->clk)) {
 		status = PTR_ERR(pl022->clk);
 		dev_err(&adev->dev, "could not retrieve SSP/SPI bus clock\n");
-- 
1.8.1.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] spi: pl022: attempt to get sspclk by name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 775 bytes --]

On Tue, Feb 11, 2014 at 11:37:09AM +0000, Mark Rutland wrote:

> -	pl022->clk = devm_clk_get(&adev->dev, NULL);
> +	/*
> +	 * For compatibility with old DTBs and platform data, fall back to the
> +	 * first clock if there's not an explicitly named "sspclk" entry.
> +	 */
> +	pl022->clk = devm_clk_get(&adev->dev, "sspclk");
> +	if (IS_ERR(pl022->clk))
> +		pl022->clk = devm_clk_get(&adev->dev, NULL);
> +

I'll just have a bit of a grumble here and point out that this sort of
stuff always worries me with the convention of using nameless clocks -
it causes hassle adding further clocks.

In any case you didn't CC me on the cover letter or any of the non-SPI
patches so I'm not sure what the dependencies are here (if there are
any), does the series need to go in as one?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/7] spi: pl022: attempt to get sspclk by name

[Mark Rutland]

On Tue, Feb 11, 2014 at 12:06:45PM +0000, Mark Brown wrote:
> On Tue, Feb 11, 2014 at 11:37:09AM +0000, Mark Rutland wrote:
> 
> > -	pl022->clk = devm_clk_get(&adev->dev, NULL);
> > +	/*
> > +	 * For compatibility with old DTBs and platform data, fall back to the
> > +	 * first clock if there's not an explicitly named "sspclk" entry.
> > +	 */
> > +	pl022->clk = devm_clk_get(&adev->dev, "sspclk");
> > +	if (IS_ERR(pl022->clk))
> > +		pl022->clk = devm_clk_get(&adev->dev, NULL);
> > +
> 
> I'll just have a bit of a grumble here and point out that this sort of
> stuff always worries me with the convention of using nameless clocks -
> it causes hassle adding further clocks.
>
> In any case you didn't CC me on the cover letter or any of the non-SPI
> patches so I'm not sure what the dependencies are here (if there are
> any), does the series need to go in as one?

Apologies for missing you for the cover letter (you can find a copy in
the lakml archive [1]).

The SPI patches don't depend on the rest of the series, but given
Russell's comments on the cover [2] this will probably need a v2 anyway.
I'll ensure you're Cc'd.

Cheers,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-February/231572.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-February/231594.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] spi: pl022: attempt to get sspclk by name

[Arnd Bergmann]

On Tuesday 11 February 2014, Mark Brown wrote:
> On Tue, Feb 11, 2014 at 11:37:09AM +0000, Mark Rutland wrote:
> 
> > -     pl022->clk = devm_clk_get(&adev->dev, NULL);
> > +     /*
> > +      * For compatibility with old DTBs and platform data, fall back to the
> > +      * first clock if there's not an explicitly named "sspclk" entry.
> > +      */
> > +     pl022->clk = devm_clk_get(&adev->dev, "sspclk");
> > +     if (IS_ERR(pl022->clk))
> > +             pl022->clk = devm_clk_get(&adev->dev, NULL);
> > +
> 
> I'll just have a bit of a grumble here and point out that this sort of
> stuff always worries me with the convention of using nameless clocks -
> it causes hassle adding further clocks.

I think the best solution for this is to continue with anonymous clocks
rather than adding names after the fact. This could be done (for DT-only
drivers) using the of_clk_get() interface that takes an index, or
we could add a generic dev_clk_get_index() or similar interface that
has the same behavior but also works for clkdev.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] spi: pl022: attempt to get sspclk by name

[Russell King - ARM Linux]

On Tue, Feb 11, 2014 at 03:08:06PM +0100, Arnd Bergmann wrote:
> On Tuesday 11 February 2014, Mark Brown wrote:
> > On Tue, Feb 11, 2014 at 11:37:09AM +0000, Mark Rutland wrote:
> > 
> > > -     pl022->clk = devm_clk_get(&adev->dev, NULL);
> > > +     /*
> > > +      * For compatibility with old DTBs and platform data, fall back to the
> > > +      * first clock if there's not an explicitly named "sspclk" entry.
> > > +      */
> > > +     pl022->clk = devm_clk_get(&adev->dev, "sspclk");
> > > +     if (IS_ERR(pl022->clk))
> > > +             pl022->clk = devm_clk_get(&adev->dev, NULL);
> > > +
> > 
> > I'll just have a bit of a grumble here and point out that this sort of
> > stuff always worries me with the convention of using nameless clocks -
> > it causes hassle adding further clocks.
> 
> I think the best solution for this is to continue with anonymous clocks
> rather than adding names after the fact. This could be done (for DT-only
> drivers) using the of_clk_get() interface that takes an index, or
> we could add a generic dev_clk_get_index() or similar interface that
> has the same behavior but also works for clkdev.

Mixing devm_* and non-devm_* interfaces doesn't work.  If you want to do
that, devm_of_clk_get() would be a prerequisit.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] spi: pl022: attempt to get sspclk by name

[Arnd Bergmann]

On Tuesday 11 February 2014 15:04:38 Russell King - ARM Linux wrote:
> On Tue, Feb 11, 2014 at 03:08:06PM +0100, Arnd Bergmann wrote:
> > On Tuesday 11 February 2014, Mark Brown wrote:
> > > On Tue, Feb 11, 2014 at 11:37:09AM +0000, Mark Rutland wrote:
> > > 
> > > > -     pl022->clk = devm_clk_get(&adev->dev, NULL);
> > > > +     /*
> > > > +      * For compatibility with old DTBs and platform data, fall back to the
> > > > +      * first clock if there's not an explicitly named "sspclk" entry.
> > > > +      */
> > > > +     pl022->clk = devm_clk_get(&adev->dev, "sspclk");
> > > > +     if (IS_ERR(pl022->clk))
> > > > +             pl022->clk = devm_clk_get(&adev->dev, NULL);
> > > > +
> > > 
> > > I'll just have a bit of a grumble here and point out that this sort of
> > > stuff always worries me with the convention of using nameless clocks -
> > > it causes hassle adding further clocks.
> > 
> > I think the best solution for this is to continue with anonymous clocks
> > rather than adding names after the fact. This could be done (for DT-only
> > drivers) using the of_clk_get() interface that takes an index, or
> > we could add a generic dev_clk_get_index() or similar interface that
> > has the same behavior but also works for clkdev.
> 
> Mixing devm_* and non-devm_* interfaces doesn't work.  If you want to do
> that, devm_of_clk_get() would be a prerequisit.

Yes, good point. So if we want to do it, we would have to add a new
function anyway, there is just the question whether it should be
devm_of_clk_get() or devm_clk_get_index() if that can also work for
non-DT devices. Do you think the latter actually makes sense in
the clkdev interfaces? I'm not familiar enough with the code to
tell how that would be implemented in a reasonable way.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] spi: pl022: attempt to get sspclk by name

[Mark Rutland]

On Tue, Feb 11, 2014 at 02:08:06PM +0000, Arnd Bergmann wrote:
> On Tuesday 11 February 2014, Mark Brown wrote:
> > On Tue, Feb 11, 2014 at 11:37:09AM +0000, Mark Rutland wrote:
> > 
> > > -     pl022->clk = devm_clk_get(&adev->dev, NULL);
> > > +     /*
> > > +      * For compatibility with old DTBs and platform data, fall back to the
> > > +      * first clock if there's not an explicitly named "sspclk" entry.
> > > +      */
> > > +     pl022->clk = devm_clk_get(&adev->dev, "sspclk");
> > > +     if (IS_ERR(pl022->clk))
> > > +             pl022->clk = devm_clk_get(&adev->dev, NULL);
> > > +
> > 
> > I'll just have a bit of a grumble here and point out that this sort of
> > stuff always worries me with the convention of using nameless clocks -
> > it causes hassle adding further clocks.
> 
> I think the best solution for this is to continue with anonymous clocks
> rather than adding names after the fact. This could be done (for DT-only
> drivers) using the of_clk_get() interface that takes an index, or
> we could add a generic dev_clk_get_index() or similar interface that
> has the same behavior but also works for clkdev.

That works, and if taken alone patch 7 would codify that existing
behaviour as the standard.

To me it feels odd to require the last clock in the list (apb_pclk) to
be named, and the rest to be in a particular order. For the dt case it
seems saner to add new clocks with names as it allows arbitrary subsets
of clocks to be wired up and described (though obviously in this case a
missing sspclk would be problematic).

For new bindings I'd really like to push people to always use named
clocks as it makes things far more flexible, but I appreciate that here
there are issues associated with modifying an existing binding.

Mark, do you have specific issues that named clocks cause that I could
look into?

Cheers,
Mark.

Re: [PATCH 4/7] spi: pl022: attempt to get sspclk by name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 622 bytes --]

On Wed, Feb 12, 2014 at 10:33:29AM +0000, Mark Rutland wrote:

> For new bindings I'd really like to push people to always use named
> clocks as it makes things far more flexible, but I appreciate that here
> there are issues associated with modifying an existing binding.

> Mark, do you have specific issues that named clocks cause that I could
> look into?

None, I actively prefer naming them (though Russell's point about people
picking names poorly pre-DT does make it clear why it makes sense that
we weren't doing that).  I was just grumbling about the fact that
transitioning from unnamed to named is a bit ugly.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/7] spi: pl022: attempt to get sspclk by name

[Arnd Bergmann]

On Wednesday 12 February 2014, Mark Rutland wrote:
> To me it feels odd to require the last clock in the list (apb_pclk) to
> be named, and the rest to be in a particular order. For the dt case it
> seems saner to add new clocks with names as it allows arbitrary subsets
> of clocks to be wired up and described (though obviously in this case a
> missing sspclk would be problematic).

Yes, good point about the missing clocks, and I also agree mixing ordered
and named clocks in one device is rather odd and can lead to trouble.

I guess there isn't really a good way out here, and I certainly won't
ask for it to be done one way or the other if someone else has a 
good argument which way it should be implemented.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] spi: pl022: attempt to get sspclk by name

[Mark Rutland]

On Wed, Feb 12, 2014 at 11:21:50AM +0000, Arnd Bergmann wrote:
> On Wednesday 12 February 2014, Mark Rutland wrote:
> > To me it feels odd to require the last clock in the list (apb_pclk) to
> > be named, and the rest to be in a particular order. For the dt case it
> > seems saner to add new clocks with names as it allows arbitrary subsets
> > of clocks to be wired up and described (though obviously in this case a
> > missing sspclk would be problematic).
> 
> Yes, good point about the missing clocks, and I also agree mixing ordered
> and named clocks in one device is rather odd and can lead to trouble.
> 
> I guess there isn't really a good way out here, and I certainly won't
> ask for it to be done one way or the other if someone else has a 
> good argument which way it should be implemented.

Having thought about it, all dts that for the ssp_pclk must have some
name for the sspclk (though the specific name is arbitrary). I could get
the driver to try each of those before falling back to the index
(perhaps with a helper that takes a list of known aliases), so all
existing dts (that we are aware of) would work with the clock requested
by name.

I assume that for the non-dt case it's possible to name clock inputs to
a device without the clock being associated with the name globally? If
so we could get rid of the index usage entirely in this case.

Does that sound workable?

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] spi: pl022: attempt to get sspclk by name

[Arnd Bergmann]

On Wednesday 12 February 2014 11:47:40 Mark Rutland wrote:
> On Wed, Feb 12, 2014 at 11:21:50AM +0000, Arnd Bergmann wrote:
> > On Wednesday 12 February 2014, Mark Rutland wrote:
> > > To me it feels odd to require the last clock in the list (apb_pclk) to
> > > be named, and the rest to be in a particular order. For the dt case it
> > > seems saner to add new clocks with names as it allows arbitrary subsets
> > > of clocks to be wired up and described (though obviously in this case a
> > > missing sspclk would be problematic).
> > 
> > Yes, good point about the missing clocks, and I also agree mixing ordered
> > and named clocks in one device is rather odd and can lead to trouble.
> > 
> > I guess there isn't really a good way out here, and I certainly won't
> > ask for it to be done one way or the other if someone else has a 
> > good argument which way it should be implemented.
> 
> Having thought about it, all dts that for the ssp_pclk must have some
> name for the sspclk (though the specific name is arbitrary). I could get
> the driver to try each of those before falling back to the index
> (perhaps with a helper that takes a list of known aliases), so all
> existing dts (that we are aware of) would work with the clock requested
> by name.

Strange. Why do the even have names in there? What are those strings?

I noticed that ux500 has uses four different strings, one for each
instance, which is obviously a bug and should just be fixed. There is
no way this was intentional, and we can just rely on teh fallback
if you want to have that anyway.

> I assume that for the non-dt case it's possible to name clock inputs to
> a device without the clock being associated with the name globally? If
> so we could get rid of the index usage entirely in this case.

Sorry, I don't understand the question.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] spi: pl022: attempt to get sspclk by name

[Mark Rutland]

On Wed, Feb 12, 2014 at 01:03:26PM +0000, Arnd Bergmann wrote:
> On Wednesday 12 February 2014 11:47:40 Mark Rutland wrote:
> > On Wed, Feb 12, 2014 at 11:21:50AM +0000, Arnd Bergmann wrote:
> > > On Wednesday 12 February 2014, Mark Rutland wrote:
> > > > To me it feels odd to require the last clock in the list (apb_pclk) to
> > > > be named, and the rest to be in a particular order. For the dt case it
> > > > seems saner to add new clocks with names as it allows arbitrary subsets
> > > > of clocks to be wired up and described (though obviously in this case a
> > > > missing sspclk would be problematic).
> > > 
> > > Yes, good point about the missing clocks, and I also agree mixing ordered
> > > and named clocks in one device is rather odd and can lead to trouble.
> > > 
> > > I guess there isn't really a good way out here, and I certainly won't
> > > ask for it to be done one way or the other if someone else has a 
> > > good argument which way it should be implemented.
> > 
> > Having thought about it, all dts that for the ssp_pclk must have some
> > name for the sspclk (though the specific name is arbitrary). I could get
> > the driver to try each of those before falling back to the index
> > (perhaps with a helper that takes a list of known aliases), so all
> > existing dts (that we are aware of) would work with the clock requested
> > by name.
> 
> Strange. Why do the even have names in there? What are those strings?

I guess they're there as spacers to allow the AMBA bus code to get the
right clock when it calls clk_get(&pcdev->dev, "apb_pclk").  Everyone
seems to have worked around the binding rather than reporting the issue
or fixing it.

>From a quick grep, for pl022's SSPCLK we currently have the strings:

* ssp{0,1}clk
* spi_clk
* spi{0,1,2,3}clk

Though I may have missed a string or two where nodes get amended in more
specific files. A grep for apb_clk to find neighbours didn't highlight
any obvious ones.

> 
> I noticed that ux500 has uses four different strings, one for each
> instance, which is obviously a bug and should just be fixed. There is
> no way this was intentional, and we can just rely on teh fallback
> if you want to have that anyway.

Sure, I'll fix those up once we have a preferred name. I guess this
would be SSPCLK by Russell's comments, I wasn't able to find a prior use
in the git history, but it would be in keeping with KMIREFCLK as used by
the pl050 driver.

Given the general policy of trying to not break support for existing
DTBs we'll need some fallback, either using the first clock or getting
the driver to try the names above.

> 
> > I assume that for the non-dt case it's possible to name clock inputs to
> > a device without the clock being associated with the name globally? If
> > so we could get rid of the index usage entirely in this case.
> 
> Sorry, I don't understand the question.

I thought one of the issues before dt was that clocks were in a global
namespace. Mark's reply implies that's not necessarily the case, so I'll
take a tour through clkdev to educate myself.

Cheers,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] spi: pl022: attempt to get sspclk by name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 639 bytes --]

On Wed, Feb 12, 2014 at 04:12:06PM +0000, Mark Rutland wrote:

> I thought one of the issues before dt was that clocks were in a global
> namespace. Mark's reply implies that's not necessarily the case, so I'll
> take a tour through clkdev to educate myself.

There's both a global namespace and a device local namespace, the most
exact match is used - see the comment on clk_find().  Remember that in
the past clock implementations didn't have to use clkdev at all so the
implementations varied a lot (which is half the problem with drivers
now).  Lots of implementations just used global clock names and didn't
pay any attention to dev.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/7] spi: pl022: attempt to get sspclk by name

[Arnd Bergmann]

On Wednesday 12 February 2014 16:12:06 Mark Rutland wrote:
> On Wed, Feb 12, 2014 at 01:03:26PM +0000, Arnd Bergmann wrote:
> > On Wednesday 12 February 2014 11:47:40 Mark Rutland wrote:

> From a quick grep, for pl022's SSPCLK we currently have the strings:
> 
> * ssp{0,1}clk
> * spi_clk
> * spi{0,1,2,3}clk
> 
> Though I may have missed a string or two where nodes get amended in more
> specific files. A grep for apb_clk to find neighbours didn't highlight
> any obvious ones.

Ok. Both ssp{0,1}clk and spi{0,1,2,3}clk /only/ appear in
arch/arm/boot/dts/ste-dbx5x0.dtsi and are clearly a bug, so unless
Linus Walleij has objections, I'd declare those to be bugs that
should be fixed by changing the DT file to spi_clk.

> > I noticed that ux500 has uses four different strings, one for each
> > instance, which is obviously a bug and should just be fixed. There is
> > no way this was intentional, and we can just rely on teh fallback
> > if you want to have that anyway.
> 
> Sure, I'll fix those up once we have a preferred name. I guess this
> would be SSPCLK by Russell's comments, I wasn't able to find a prior use
> in the git history, but it would be in keeping with KMIREFCLK as used by
> the pl050 driver.

We do have a few cases of spi_clk, so I'd use that one. Ideally it
should be the string given in the data sheet for the IP block of course,
possibly with capital letters and underscores turned converted to
more regular strings.

> > > I assume that for the non-dt case it's possible to name clock inputs to
> > > a device without the clock being associated with the name globally? If
> > > so we could get rid of the index usage entirely in this case.
> > 
> > Sorry, I don't understand the question.
> 
> I thought one of the issues before dt was that clocks were in a global
> namespace. Mark's reply implies that's not necessarily the case, so I'll
> take a tour through clkdev to educate myself.

The whole point of clkdev is to create a local per-device namespace
so drivers don't need to care about the global names, as far as I understand
it.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] spi: pl022: attempt to get sspclk by name

[Linus Walleij]

On Wed, Feb 12, 2014 at 5:31 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:

> Ok. Both ssp{0,1}clk and spi{0,1,2,3}clk /only/ appear in
> arch/arm/boot/dts/ste-dbx5x0.dtsi and are clearly a bug, so unless
> Linus Walleij has objections, I'd declare those to be bugs that
> should be fixed by changing the DT file to spi_clk.

OK I'll fix. But didn't Mark just say that the preferred name should be
SSPCLK?

Here is the PL022 TRM:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0194g/index.html

The clock name on the silicon side is clearly "SSPCLK" so let's
stick with this.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] spi: pl022: attempt to get sspclk by name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 303 bytes --]

On Wed, Feb 12, 2014 at 11:47:40AM +0000, Mark Rutland wrote:

> I assume that for the non-dt case it's possible to name clock inputs to
> a device without the clock being associated with the name globally? If
> so we could get rid of the index usage entirely in this case.

Yes, using clk_add_alias().

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 5/7] Documentation: devicetree: fix up pl18x clocks

[Mark Rutland]

Currently the pl18x driver expects clocks, and dts provide them, yet the
binding does not mention clocks at all.

This patch adds a description of the clocks, "apb_pclk" (as required by
the primecell binding) and "mclk" for the pl18x itself. The "mclk"
name was chosen to match the official documentation, as currently a
variety of names are used in its place; it is expected that any
operating system supporting these can continue to do so in the absence
of an "mclk" entry.

Signed-off-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Chris Ball <cjb-2X9k7bc8m7Mdnm+yROfE0A@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
---
 Documentation/devicetree/bindings/mmc/mmci.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmci.txt b/Documentation/devicetree/bindings/mmc/mmci.txt
index 2b584ca..1b72d4d 100644
--- a/Documentation/devicetree/bindings/mmc/mmci.txt
+++ b/Documentation/devicetree/bindings/mmc/mmci.txt
@@ -9,6 +9,10 @@ by mmc.txt and the properties used by the mmci driver.
 Required properties:
 - compatible             : contains "arm,pl18x", "arm,primecell".
 - arm,primecell-periphid : contains the PrimeCell Peripheral ID.
+- clocks : a list of phandles + clock-specifiers, one for each entry in
+           clock-names.
+- clock-names : should contain "apb_pclk" and "mclk".
+
 
 Optional properties:
 - mmc-cap-mmc-highspeed  : indicates whether MMC is high speed capable
-- 
1.8.1.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/7] mmc: arm-mmci: attempt to get mclk by name

[Mark Rutland]

The primecell device tree binding (from which the pl18x binding is
derived from) states that the apb_pclk clock input should be listed
first for all primecell devices. The arm-mmci driver requires the mclk
clock input, but the way it currently grabs the clock means that it
always gets the first clock (which should be apb_pclk).

As the AMBA bus code grabs apb_pclk by name, some existing dts provide
apb_pclk as the second clock in the clocks list to work around this, in
violation of both the primecell binding. The pl18x binding does not
mention clocks at all, so the first clock (MCLK) is given an arbitrary
name.

This patch attempts to fix the mess my having the arm-mmci driver first
attempt to get mclk by name. If this fails, it falls back to the old
behaviour of simply acquiring the first clock. This is compatible with
any old dtb, whether it lists mclk by name or not, and allows the driver
to support dtbs which do not violate the bindings. Hopefully this will
lead to future uniformity across dtbs.

Signed-off-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Chris Ball <cjb-2X9k7bc8m7Mdnm+yROfE0A@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
---
 drivers/mmc/host/mmci.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index b931226..4af962c 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1470,7 +1470,14 @@ static int mmci_probe(struct amba_device *dev,
 	dev_dbg(mmc_dev(mmc), "designer ID = 0x%02x\n", host->hw_designer);
 	dev_dbg(mmc_dev(mmc), "revision = 0x%01x\n", host->hw_revision);
 
-	host->clk = devm_clk_get(&dev->dev, NULL);
+	/*
+	 * For compatibility with old DTBs and platform data, fall back to the
+	 * first clock if there's not an explicitly named "mclk" entry.
+	 */
+	host->clk = devm_clk_get(&dev->dev, "mclk");
+	if (IS_ERR(host->clk))
+		host->clk = devm_clk_get(&dev->dev, NULL);
+
 	if (IS_ERR(host->clk)) {
 		ret = PTR_ERR(host->clk);
 		goto host_free;
-- 
1.8.1.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 7/7] Documentation: devicetree: loosen primecell clock requirements

[Mark Rutland]

The primecell binding requires the APB PCLK (named "apb_pclk") to be the
first entry in the clocks list, yet existing drivers and dts files
expect other clocks first, in clear violation of this requirement.
Additionally, the code handling the apb_pclk always acquires the clock
by name rather than index, making the requirement irrelevant.

As there are no other implementations handling the primecell bindings,
this patch loosens the requirements to an apb_pclk entry existing in the
clocks list. This is compatible with existing software, and any new
software handling the weaker requirements will be able to use existing
dts. Any software relying on the original stricter requirements will be
unable to use many existing dts, so the loosened requirement aids
compatibility rather than hindering it.

Signed-off-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
---
 Documentation/devicetree/bindings/arm/primecell.txt | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/primecell.txt b/Documentation/devicetree/bindings/arm/primecell.txt
index 0df6aca..0a66506 100644
--- a/Documentation/devicetree/bindings/arm/primecell.txt
+++ b/Documentation/devicetree/bindings/arm/primecell.txt
@@ -13,9 +13,10 @@ Required properties:
 Optional properties:
 
 - arm,primecell-periphid : Value to override the h/w value with
-- clocks : From common clock binding. First clock is phandle to clock for apb
-	pclk. Additional clocks are optional and specific to those peripherals.
-- clock-names : From common clock binding. Shall be "apb_pclk" for first clock.
+- clocks : From common clock binding. One clock must be the apb pclk.
+           Additional clocks are optional and specific to those peripherals.
+- clock-names : From common clock binding. Shall include "apb_pclk" for the apb
+                pclk.
 - dmas : From common DMA binding. If present, refers to one or more dma channels.
 - dma-names : From common DMA binding, needs to match the 'dmas' property.
               Devices with exactly one receive and transmit channel shall name
@@ -31,8 +32,8 @@ serial@fff36000 {
 	compatible = "arm,pl011", "arm,primecell";
 	arm,primecell-periphid = <0x00341011>;
 
-	clocks = <&pclk>;
-	clock-names = "apb_pclk";
+	clocks = <&refclk>, <&pclk>;
+	clock-names = "uartclk", "apb_pclk";
 
 	dmas = <&dma-controller 4>, <&dma-controller 5>;
 	dma-names = "rx", "tx";	
-- 
1.8.1.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/7] primecell: make correct clock parsing possible

[Russell King - ARM Linux]

On Tue, Feb 11, 2014 at 11:37:05AM +0000, Mark Rutland wrote:
> These patches attempt to harmonize the bindings and the drivers with what's in
> use today, in a backwards compatible fashion, relieving us of our present
> Kafkaesque nightmare. Each peripheral's clock(s) are given explicit names which
> can be used, though code will fall back to the existing behaviour if said names
> are not provided. Additionally the currently unmet ordering requirement of
> apb_pclk is dropped, given all existing that code requires this to be named
> anyway.

The reason why these clocks ended up with NULL names was to force the
issue that clocks shall not be named solely by their connection IDs,
which was a major problem before DT.  Now that we have DT, I'm happier
to reinstate the names, but I think we should just reinstate the names
we had previously.  That should be in the git history.

The down-side is those names are all in capitals - but they were named
after the signal name(s) given in the Primecell documentation.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/7] primecell: make correct clock parsing possible

[Mark Rutland]

On Tue, Feb 11, 2014 at 12:33:56PM +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 11, 2014 at 11:37:05AM +0000, Mark Rutland wrote:
> > These patches attempt to harmonize the bindings and the drivers with what's in
> > use today, in a backwards compatible fashion, relieving us of our present
> > Kafkaesque nightmare. Each peripheral's clock(s) are given explicit names which
> > can be used, though code will fall back to the existing behaviour if said names
> > are not provided. Additionally the currently unmet ordering requirement of
> > apb_pclk is dropped, given all existing that code requires this to be named
> > anyway.
> 
> The reason why these clocks ended up with NULL names was to force the
> issue that clocks shall not be named solely by their connection IDs,
> which was a major problem before DT.  Now that we have DT, I'm happier
> to reinstate the names, but I think we should just reinstate the names
> we had previously.  That should be in the git history.

Good to know, that sounds fine to me.

> 
> The down-side is those names are all in capitals - but they were named
> after the signal name(s) given in the Primecell documentation.

As long as we have standardised names I'm not too worried about which
case they happen to be in, and matching the documentation seems
sensible. The pl050 driver expects "KMIREFCLK" already, so it would be
in keeping with (as far as I can tell) the only Primecell driver
currently requesting a clock by name.

I'll prepare a v2 shortly.

Cheers,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html