Devicetree
 help / color / mirror / Atom feed
* Re: [PATCH 1/3] sound: soc: codecs: Add es8328 codec
From: Mark Brown @ 2014-02-10 14:23 UTC (permalink / raw)
  To: Sean Cross
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, Shawn Guo, Sascha Hauer,
	Liam Girdwood
In-Reply-To: <52F5E661.5010807-nXMMniAx+RbQT0dZR+AlfA@public.gmane.org>

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

On Sat, Feb 08, 2014 at 04:10:09PM +0800, Sean Cross wrote:
> On 8/2/14 2:12 AM, Mark Brown wrote:

> > In general this looks like it should be making much more use of the
> > framework rather than open coding, it looks like it's very much hard
> > coded for one use cae.

> Alright, I'll try to use more of the framework provided.  I tried to use
> one of the Wolfson codecs as an example.  Is there a "canonical" driver
> that's up-to-date and uses an appreciable amount of framework?

Try wm8731 I guess, something similar to the device, there's a lot of
variation in device complexity and several generations of hardware?  I'm
not sure what driver you were looking at here.

> >> +static const struct snd_soc_dapm_widget es8328_dapm_widgets[] = {
> >> +    SND_SOC_DAPM_DAC("Speaker Volume", "HiFi Playback",
> SND_SOC_NOPM, 0, 0),

> > Don't declare a stream by name, use DAPM routes to connect the stream to
> > the widget.

> Where can I read up on DAPM routes?  I don't see the word "route"
> mentioned in the dapm.txt SoC documentation.

Paths.

> >> +static int es8328_adc_enable(struct snd_soc_codec *codec)
> >> +{
> >> +    u16 reg = snd_soc_read(codec, ES8328_CHIPPOWER);
> >> +    reg &= ~(ES8328_CHIPPOWER_ADCVREF_OFF |
> >> +         ES8328_CHIPPOWER_ADCPLL_OFF |
> >> +         ES8328_CHIPPOWER_ADCSTM_RESET |
> >> +         ES8328_CHIPPOWER_ADCDIG_OFF);
> >> +    snd_soc_write(codec, ES8328_CHIPPOWER, reg);

> > This looks like it should be done in DAPM.

> The codec has very strict power sequencing.  If I'm understanding the
> document correctly, I'd add a DAPM widget for the DAC, and then register
> a widget event for when the DAC gets opened, and do power sequencing
> there?  Or is there a way to have multiple register writes defined as a
> single DAPM widget?

An event callback is the way to go.  However please check what the
datasheet is actually trying to say - I suspect you will find it's
giving you a procedure that's got common elements for core power up and
then is saying something like "keep the outputs muted while you power
them up" but is doing it with lots of examples.

> >> +    /* Frequency clock of 272 */
> >> +    snd_soc_write(codec, ES8328_ADCCONTROL5, 0x02);

> > What is the frequency clock in this context?

> The frequency is a specific divider defined in the datasheet.  There is
> a table of frequencies and dividers for various frequencies.  One option
> is to code this table into the driver and pick the frequency clocks out
> of the table.

That seems much better.

> I decided to assume the codec has a fixed input frequency of 22.5792
> MHz.  This means the codec can support 11.025, 22.05, and 44.1 kHz, but
> cannot support 48 kHz.  It should be possible to instead feed the codec
> a 24 MHz clock and run it at 48 kHz.  Pulseaudio runs at 44.1 by
> default, so I went with that.

This is what set_sysclk() is for, letting the driver know what the clock
rate is on the current board (if you are willing to go DT only you can
just use the clock API).

> My fault for omitting one.
> Much of the code in adc_enable() and dac_enable() come from the
> reference manual.  They indicate that the codec has very strict ordering
> requirements on registers that must be set during playback and
> recording.  Again, is there any way to ensure that hw_params(),
> set_dai_fmt(), and the DAPM widgets are called in a specific order?

No, there's no way to do that.  Like I say I am somewhat mistrustful of
the clarity/accuracy of the hardware limitations being described here.

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

^ permalink raw reply

* Re: [PATCH 1/3] sound: soc: codecs: Add es8328 codec
From: Mark Brown @ 2014-02-10 14:27 UTC (permalink / raw)
  To: Sean Cross
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, Shawn Guo, Sascha Hauer,
	Liam Girdwood
In-Reply-To: <52F89ADF.8020305-nXMMniAx+RbQT0dZR+AlfA@public.gmane.org>

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

On Mon, Feb 10, 2014 at 05:24:47PM +0800, Sean Cross wrote:
> On 8/2/14 2:12 AM, Mark Brown wrote:

> > In general this looks like it should be making much more use of the
> > framework rather than open coding, it looks like it's very much hard
> > coded for one use cae.

> I've tried to simplify the code somewhat, but I'm still unclear about
> how much of the DAPM code functions.  I'm running into two problems:
> Mapping the codec names "OUT1" and "OUT2" onto "Speaker" and "Headphone"
> respectively, and accommodating this codec's one DAC / two outputs
> architecture.

Describe the hardware accurately.  If the hardware has outputs called
OUT1 and OUT2 that aren't fixed function in some way then describe them
like that, if the CODEC has routing control just describe the routing
control.

> First, this particular codec doesn't specify what the outputs are used
> for, it only specifies the names OUT1 and OUT2.  It is permitted to
> connect either a speaker or headphones to either output.  In the initial
> patch, I hardcoded OUT1 to be named "Speaker", and OUT2 to "Headphone".
>  However, I've genericised the codec and changed them back to "OUT1" and
> "OUT2".  Inthe board definition I've added DAPM routes that map
> "Speaker" and "Headphone" back onto these names.  amixer still shows
> OUT1 and OUT2.

Good.

> There seems to be some overlap between DAPM and kcontrols.  How can I
> specify in the board definition file that OUT1 should have a kcontrol
> named "Speaker" and OUT2 should be named "Headphone"?  Tracing shows
> that DAPM at least recognizes the naming: e.g. "snd_soc_dapm_input_path:
> *Speaker <- (direct) <- ROUT1"

You can't, do this in your userspace configuration - UCM contains tools
to help do remapping if you are using raw ALSA.  Or extend the in kernel
frameworks but that's probably more trouble than it's worth and it's not
clear it's even a good idea to have the complexity in kernel.

> Second, I'm having trouble getting the DAPM routing to function
> properly.  There is one DAC that routes to two outputs, OUT1 and OUT2.
> Both OUT1 and OUT2 have their own volume controls as well, but the main
> state machine and amplification comes from the DAC.  This doesn't seem
> to be very common.  Are there any codecs I can use as an example that
> have one DAC routing to two outputs?

That's really common - the overwhelming majority of CODECs have such a
design if they have multiple outputs.  Could you articulate what's
unclear here?

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

^ permalink raw reply

* Re: [PATCH v2 1/8] ASoC: da9055: Fix device registration of PMIC and CODEC devices
From: Lee Jones @ 2014-02-10 14:42 UTC (permalink / raw)
  To: Mark Brown
  Cc: Adam Thomson, alsa-devel, linux-kernel, devicetree, Rob Herring,
	Linus Walleij, Dmitry Torokhov, Alessandro Zummo, Guenter Roeck
In-Reply-To: <20140210141400.GJ1757@sirena.org.uk>

> > > Applied, thanks.
> 
> > Actually, do you mind if I take this and apply it to an immutable?
> 
> > I'd rather not be on the wrong end of some wrath, due to not playing
> > the game properly.
> 
> OK, but it does need to go in as a fix targetted for stable.

Ah, in which case there's no need, as I don't have anything in my
-fixes branch with touches this file. Simply take it in, no problem.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply

* Re: [PATCH 2/3] sound: soc: fsl: Add support for Novena onboard audio
From: Mark Brown @ 2014-02-10 14:45 UTC (permalink / raw)
  To: Sean Cross
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, Shawn Guo, Sascha Hauer,
	Liam Girdwood
In-Reply-To: <52F5EA67.1080008-nXMMniAx+RbQT0dZR+AlfA@public.gmane.org>

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

On Sat, Feb 08, 2014 at 04:27:19PM +0800, Sean Cross wrote:
> On 8/2/14 2:14 AM, Mark Brown wrote:

> > The CODEC looked pretty simple, can this use simple-card?

> I had never heard of simple-card.  It doesn't look like it supports
> device tree.  Novena doesn't have a platform definition, and we run

Yes it does, please look at current code (you should be submitting
against -next but the DT code is in Linus' tree).

> Can you think of why this would oops on removal?  If I disable
> headphone jack detection (by simply making data->jack_gpio an invalid
> gpio), the module removes cleanly.

Nothing immediate springs to mind, sorry.  What's the oops.

> > There's supposed to be support for this sort of thing going into
> > the clock API with some sort of generic binding.

> Can you direct me to this patch?  Is it supposed to be defined in the
> device tree file?  As of right now, the clock is hardcoded to 24 MHz
> in clk-imx6q.c, which is why we need to change it here.

I'm not sure it's actually been written yet, sorry - I'd ask Mike
Turquette what the status is.

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

^ permalink raw reply

* Re: [PATCH v2 1/8] ASoC: da9055: Fix device registration of PMIC and CODEC devices
From: Mark Brown @ 2014-02-10 14:51 UTC (permalink / raw)
  To: Lee Jones
  Cc: Adam Thomson, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Linus Walleij,
	Dmitry Torokhov, Alessandro Zummo, Guenter Roeck
In-Reply-To: <20140210144234.GB26697@lee--X1>

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

On Mon, Feb 10, 2014 at 02:42:34PM +0000, Lee Jones wrote:

> > OK, but it does need to go in as a fix targetted for stable.

> Ah, in which case there's no need, as I don't have anything in my
> -fixes branch with touches this file. Simply take it in, no problem.

I'll do that - thanks for confirming.

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

^ permalink raw reply

* Re: Devicetree Maintenance in barebox
From: Jason Cooper @ 2014-02-10 15:06 UTC (permalink / raw)
  To: Jon Loeliger
  Cc: Sascha Hauer, Grant Likely, Ian Campbell,
	barebox-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <E1WCYdq-0004eu-Gr-CYoMK+44s/E@public.gmane.org>

On Sun, Feb 09, 2014 at 11:58:06AM -0600, Jon Loeliger wrote:
> > Hi Sascha,
> > 
> > + Grant Likely, Ian Campbell, devicetree ML
> > 
> > Also, In the DT meeting earlier this week, Grant Likely said he has the
> > request in to create a separate mailinglist for collaboration between
> > the different devicetree users (BSD, Linux, etc).
> 
> ...
> 
> > I think the proper solution will percolate out of the first
> > cross-project discussions on the new ML.
> 
> ...
> 
> > Definitely fodder for the new ML.
> > 
> > Grant, can you please add Sascha to the list of folks to notify when
> > the new ML is ready?
> 
> I don't think there needs to be a different mailing list
> in order to combine or discuss other OS's use of the device
> tree compiler.  The  DTC is OS and Use-agnostic.  Discussions
> of DTC needs for FreeBSD can happen right here as the orginal
> purpose of this list was DTC discussion.

Sorry, I wasn't clear.  I was referring to the devicetree bindings
currently being created in the linux tree, and the dts files for the
boards Linux supports.

And by 'here', I presume you me devicetree-u79uwXL29TaiAVqoAR/hOK1cXZ9k6wlg@public.gmane.org

> Are you, and Grant(?), suggesting that a separate list
> should be created for FreeBSD use of DTS-file contents?
> Or that DTS-file-content related discussions should be
> separated from DTC discussions?

As Ian mentioned, the separate list is to engage other consumers of
devicetree bindings/dts files/dtc use without the firehose of Linux
patches.

> > imho, the goal is to not have any project tied to a specific version
> > of the devicetree.
> >
> > iow, we don't break backwards compatibility in the
> > devicetrees, and projects should revert to default behavior if new dt
> > parameters are missing.  This means Linux and BSD shouldn't need to keep
> > a current copy of the devicetree in their trees.  However, building the
> > bootloader is a different animal.  It needs to provide the dt blob...
> 
> The devicetree source file format hasn't changed in years.
> Yes, it is enhanced, but compatibly.  Or do you mean the
> contents of the DTB for some specific platform?

I was referring specifically to dt bindings for new IP blocks, and new
versions of the same.

One problem we occasionally run into with kirkwood/dove/mvebu is that
it is very convenient having the dts tree in the linux tree.  It's
tempting to keep tying the dtb to the linux kernel version.  Which makes
it a lot harder to maintain backwards compatibility.

That gets a lot easier once the bindings and the dts files have their
own tree with their own release cycle.  Then, patch submitters would be
forced to consider how changing a binding affects the driver and vice
versa.  The concept of 'deployed dtbs' without the new whizbang bindings
would have to be considered and properly handled.

Don't get me wrong, all of us are trying very hard to do this now.  But
I think it's more like running it in qemu vice running on real hardware.
We're simulating in our heads what we think the problems will be, as
opposed to experiencing them and avoiding them.

imho, the sooner we have a separate tree for dts/bindings, the better.

thx,

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

^ permalink raw reply

* [RFC PATCH 0/3] Syscon early initialization
From: Michal Simek @ 2014-02-10 15:22 UTC (permalink / raw)
  To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Arnd Bergmann
  Cc: Mike Turquette, Kumar Gala, Peter Crosthwaite, Ian Campbell,
	Rob Herring, James Hogan, Michal Simek, Felipe Pena, Pawel Moll,
	Soren Brinkmann, Greg Kroah-Hartman, Stephen Warren,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Stephen Boyd, Russell King,
	Lee Jones, Rob Herring, Josh Cartwright, Steffen Trumtrar,
	Mark Brown, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Samuel Ortiz,
	Mark Rutland

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

Hi,

this series come from my discussion with Arnd at KS and then
on some other threads/IRCs(Arnd and Mark) that SoC vendors
are more and more try to add misc functionality to
one memory region. For this purpose syscon driver is in the kernel.
But regular syscon driver is initialized too late
and platforms are trying to create specific code to handle it.

For this purpose the series have been created to provide
early syscon initialization and regmap creation first
and then attaching device.

The last patch is zynq specific patch to clear slcr driver
and clock driver can profit from it too when clk regmap is ready.

Also moving syscon driver from mfs should be consider.

Thanks for your comments,
Michal


Michal Simek (3):
  regmap: Separate regmap dev initialization
  mfd: syscon: Support early initialization
  ARM: zynq: Use early syscon initialization

 arch/arm/boot/dts/zynq-7000.dtsi |   1 +
 arch/arm/mach-zynq/common.c      |   6 +-
 arch/arm/mach-zynq/slcr.c        |  42 +------------
 drivers/base/regmap/regmap.c     |  41 +++++++++----
 drivers/clk/zynq/clkc.c          |  57 ++++++------------
 drivers/mfd/syscon.c             | 126 +++++++++++++++++++++++++++++++++------
 include/linux/mfd/syscon.h       |  11 ++++
 include/linux/regmap.h           |   2 +
 8 files changed, 173 insertions(+), 113 deletions(-)

--
1.8.2.3


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

^ permalink raw reply

* [RFC PATCH 3/3] ARM: zynq: Use early syscon initialization
From: Michal Simek @ 2014-02-10 15:22 UTC (permalink / raw)
  To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Arnd Bergmann
  Cc: monstr-pSz03upnqPeHXe+LvDLADg, Josh Cartwright, Steffen Trumtrar,
	Rob Herring, Peter Crosthwaite, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Kumar Gala, Russell King,
	Mike Turquette, Soren Brinkmann, Stephen Boyd, Stephen Warren,
	James Hogan, Felipe Pena, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <cover.1392045742.git.michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>

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

Use early syscon initialization to simplify slcr code.
- Remove two slcr inits (zynq_slcr_init, zynq_early_slcr_init)
- Directly use regmap accesses in zynq_slcr_read/write
- Remove zynq_clock_init() and use addresses from syscon
  (This is the most problematic part now because clock
  doesn't support regmap accesses that's why reading
  slcr base is ugly. There are some attempts to get
  clk regmap to work - for example:
  https://lkml.org/lkml/2013/10/16/112)

Signed-off-by: Michal Simek <michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
---

Especially look at slcr.c which is much simpler than was before.
clkc.c will be simpler when regmap support is added because then
syscon_early_regmap_lookup_by_phandle() will be called
without zynq_slcr_base search.

---
 arch/arm/boot/dts/zynq-7000.dtsi |  1 +
 arch/arm/mach-zynq/common.c      |  6 ++---
 arch/arm/mach-zynq/slcr.c        | 42 ++---------------------------
 drivers/clk/zynq/clkc.c          | 57 ++++++++++++----------------------------
 4 files changed, 23 insertions(+), 83 deletions(-)

diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
index 7284499..e414489 100644
--- a/arch/arm/boot/dts/zynq-7000.dtsi
+++ b/arch/arm/boot/dts/zynq-7000.dtsi
@@ -135,6 +135,7 @@
 			compatible = "xlnx,zynq-slcr", "syscon";
 			reg = <0xF8000000 0x1000>;
 			ranges;
+			syscon = <&slcr>;
 			clkc: clkc@100 {
 				#clock-cells = <1>;
 				compatible = "xlnx,ps7-clkc";
diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c
index 9d3c88e..78589e3 100644
--- a/arch/arm/mach-zynq/common.c
+++ b/arch/arm/mach-zynq/common.c
@@ -28,6 +28,7 @@
 #include <linux/of.h>
 #include <linux/irqchip.h>
 #include <linux/irqchip/arm-gic.h>
+#include <linux/mfd/syscon.h>
 #include <linux/slab.h>
 #include <linux/sys_soc.h>

@@ -130,15 +131,14 @@ out:
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, parent);

 	platform_device_register(&zynq_cpuidle_device);
-
-	zynq_slcr_init();
 }

 static void __init zynq_timer_init(void)
 {
+	early_syscon_init();
+
 	zynq_early_slcr_init();

-	zynq_clock_init();
 	of_clk_init(NULL);
 	clocksource_of_init();
 }
diff --git a/arch/arm/mach-zynq/slcr.c b/arch/arm/mach-zynq/slcr.c
index 594b280..a89b082 100644
--- a/arch/arm/mach-zynq/slcr.c
+++ b/arch/arm/mach-zynq/slcr.c
@@ -35,7 +35,6 @@
 #define SLCR_PSS_IDCODE_DEVICE_SHIFT	12
 #define SLCR_PSS_IDCODE_DEVICE_MASK	0x1F

-static void __iomem *zynq_slcr_base;
 static struct regmap *zynq_slcr_regmap;

 /**
@@ -48,11 +47,6 @@ static struct regmap *zynq_slcr_regmap;
  */
 static int zynq_slcr_write(u32 val, u32 offset)
 {
-	if (!zynq_slcr_regmap) {
-		writel(val, zynq_slcr_base + offset);
-		return 0;
-	}
-
 	return regmap_write(zynq_slcr_regmap, offset, val);
 }

@@ -66,12 +60,7 @@ static int zynq_slcr_write(u32 val, u32 offset)
  */
 static int zynq_slcr_read(u32 *val, u32 offset)
 {
-	if (zynq_slcr_regmap)
-		return regmap_read(zynq_slcr_regmap, offset, val);
-
-	*val = readl(zynq_slcr_base + offset);
-
-	return 0;
+	return regmap_read(zynq_slcr_regmap, offset, val);
 }

 /**
@@ -169,24 +158,6 @@ void zynq_slcr_cpu_stop(int cpu)
 }

 /**
- * zynq_slcr_init - Regular slcr driver init
- *
- * Return:	0 on success, negative errno otherwise.
- *
- * Called early during boot from platform code to remap SLCR area.
- */
-int __init zynq_slcr_init(void)
-{
-	zynq_slcr_regmap = syscon_regmap_lookup_by_compatible("xlnx,zynq-slcr");
-	if (IS_ERR(zynq_slcr_regmap)) {
-		pr_err("%s: failed to find zynq-slcr\n", __func__);
-		return -ENODEV;
-	}
-
-	return 0;
-}
-
-/**
  * zynq_early_slcr_init - Early slcr init function
  *
  * Return:	0 on success, negative errno otherwise.
@@ -202,20 +173,11 @@ int __init zynq_early_slcr_init(void)
 		pr_err("%s: no slcr node found\n", __func__);
 		BUG();
 	}
-
-	zynq_slcr_base = of_iomap(np, 0);
-	if (!zynq_slcr_base) {
-		pr_err("%s: Unable to map I/O memory\n", __func__);
-		BUG();
-	}
-
-	np->data = (__force void *)zynq_slcr_base;
+	zynq_slcr_regmap = syscon_early_regmap_lookup_by_phandle(np, "syscon");

 	/* unlock the SLCR so that registers can be changed */
 	zynq_slcr_unlock();

-	pr_info("%s mapped to %p\n", np->name, zynq_slcr_base);
-
 	of_node_put(np);

 	return 0;
diff --git a/drivers/clk/zynq/clkc.c b/drivers/clk/zynq/clkc.c
index c812b93..b2fd160 100644
--- a/drivers/clk/zynq/clkc.c
+++ b/drivers/clk/zynq/clkc.c
@@ -214,6 +214,10 @@ err:
 		clks[clk1] = ERR_PTR(-ENOMEM);
 }

+struct syscon {
+	void __iomem *base;
+};
+
 static void __init zynq_clk_setup(struct device_node *np)
 {
 	int i;
@@ -227,6 +231,19 @@ static void __init zynq_clk_setup(struct device_node *np)
 	const char *periph_parents[4];
 	const char *swdt_ext_clk_mux_parents[2];
 	const char *can_mio_mux_parents[NUM_MIO_PINS];
+	struct resource res;
+	void __iomem *zynq_slcr_base;
+
+	struct device_node *slcr = of_get_parent(np);
+	struct syscon *syscon = slcr->data;
+	zynq_slcr_base = syscon->base;
+
+	if (of_address_to_resource(np, 0, &res)) {
+		pr_err("%s: failed to get resource\n", np->name);
+		return;
+	}
+
+	zynq_clkc_base = zynq_slcr_base + res.start;

 	pr_info("Zynq clock init\n");

@@ -569,43 +586,3 @@ static void __init zynq_clk_setup(struct device_node *np)
 }

 CLK_OF_DECLARE(zynq_clkc, "xlnx,ps7-clkc", zynq_clk_setup);
-
-void __init zynq_clock_init(void)
-{
-	struct device_node *np;
-	struct device_node *slcr;
-	struct resource res;
-
-	np = of_find_compatible_node(NULL, NULL, "xlnx,ps7-clkc");
-	if (!np) {
-		pr_err("%s: clkc node not found\n", __func__);
-		goto np_err;
-	}
-
-	if (of_address_to_resource(np, 0, &res)) {
-		pr_err("%s: failed to get resource\n", np->name);
-		goto np_err;
-	}
-
-	slcr = of_get_parent(np);
-
-	if (slcr->data) {
-		zynq_clkc_base = (__force void __iomem *)slcr->data + res.start;
-	} else {
-		pr_err("%s: Unable to get I/O memory\n", np->name);
-		of_node_put(slcr);
-		goto np_err;
-	}
-
-	pr_info("%s: clkc starts at %p\n", __func__, zynq_clkc_base);
-
-	of_node_put(slcr);
-	of_node_put(np);
-
-	return;
-
-np_err:
-	of_node_put(np);
-	BUG();
-	return;
-}
--
1.8.2.3


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

^ permalink raw reply related

* Re: [PATCH] dp83640: Get gpio and master/slave configuration from DT
From: Stefan Sørensen @ 2014-02-10 15:25 UTC (permalink / raw)
  To: Mark Rutland
  Cc: richardcochran@gmail.com, grant.likely@linaro.org,
	robh+dt@kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
In-Reply-To: <20140210134237.GF29080@e106331-lin.cambridge.arm.com>

On Mon, 2014-02-10 at 13:42 +0000, Mark Rutland wrote:

> Binding document please.

Dang, I somehow managed to drop that from the patch. I will fix for the
next version.

> > +	if (of_find_property(node, "dp83640,slave", NULL))
> > +		dp83640->slave = true;
> 
> Use of_property_read_bool.

Fixed.

> > +	prop = of_find_property(node, "dp83640,perout-gpios", &proplen);
> > +	if (prop) {
> > +		if (dp83640->slave) {
> > +			pr_err("dp83640,perout-gpios property can not be set together with dp83640,slave");
> > +			return -EINVAL;
> > +		}
> > +
> > +		clock->caps.n_per_out = proplen / sizeof(u32);
> > +		if (clock->caps.n_per_out > N_EXT) {
> > +			pr_err("dp83640,perout-gpios may not have more than %d entries",
> > +			       N_EXT);
> > +			return -EINVAL;
> > +		}
> > +		err = of_property_read_u32_array(node, "dp83640,perout-gpios",
> > +						 clock->perout_gpios,
> > +						 clock->caps.n_per_out);
> > +		if (err < 0)
> > +			return err;
> > +	}
> 
> This looks nothing like the standard gpio bindings. What _exactly_ is
> this property describing?

The dp83640 has a number of pins of which 8 of them can be used for
either timestamping events or triggering changes at programmed
intervals. These properties are used to configure which pins get
assigned to what function.

> If this is not using the standard gpio bindings then this should be
> renamed.

Maybe a gpio->pin renaming?

Stefan

^ permalink raw reply

* Re: [PATCH] dp83640: Get gpio and master/slave configuration from DT
From: Mark Rutland @ 2014-02-10 15:39 UTC (permalink / raw)
  To: Stefan Sørensen
  Cc: richardcochran@gmail.com, grant.likely@linaro.org,
	robh+dt@kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
In-Reply-To: <1392045915.30419.6.camel@e37108.spectralink.com>

On Mon, Feb 10, 2014 at 03:25:15PM +0000, Stefan Sørensen wrote:
> On Mon, 2014-02-10 at 13:42 +0000, Mark Rutland wrote:
> 
> > Binding document please.
> 
> Dang, I somehow managed to drop that from the patch. I will fix for the
> next version.

Cheers.

> 
> > > +	if (of_find_property(node, "dp83640,slave", NULL))
> > > +		dp83640->slave = true;
> > 
> > Use of_property_read_bool.
> 
> Fixed.
> 
> > > +	prop = of_find_property(node, "dp83640,perout-gpios", &proplen);
> > > +	if (prop) {
> > > +		if (dp83640->slave) {
> > > +			pr_err("dp83640,perout-gpios property can not be set together with dp83640,slave");
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		clock->caps.n_per_out = proplen / sizeof(u32);
> > > +		if (clock->caps.n_per_out > N_EXT) {
> > > +			pr_err("dp83640,perout-gpios may not have more than %d entries",
> > > +			       N_EXT);
> > > +			return -EINVAL;
> > > +		}
> > > +		err = of_property_read_u32_array(node, "dp83640,perout-gpios",
> > > +						 clock->perout_gpios,
> > > +						 clock->caps.n_per_out);
> > > +		if (err < 0)
> > > +			return err;
> > > +	}
> > 
> > This looks nothing like the standard gpio bindings. What _exactly_ is
> > this property describing?
> 
> The dp83640 has a number of pins of which 8 of them can be used for
> either timestamping events or triggering changes at programmed
> intervals. These properties are used to configure which pins get
> assigned to what function.

Ok.

> 
> > If this is not using the standard gpio bindings then this should be
> > renamed.
> 
> Maybe a gpio->pin renaming?

That sounds good to me.

Thanks,
Mark.

^ permalink raw reply

* Re: [PATCH v6 0/3] Introduce clocksource driver for Keystone platform
From: Santosh Shilimkar @ 2014-02-10 15:50 UTC (permalink / raw)
  To: daniel.lezcano
  Cc: Ivan Khoronzhuk, rob, linux, galak, robh+dt, pawel.moll,
	mark.rutland, ijc+devicetree, tglx, devicetree, linux-doc,
	linux-arm-kernel, linux-kernel, grygorii.strashko, joshc
In-Reply-To: <1392027058-11680-1-git-send-email-ivan.khoronzhuk@ti.com>

Daniel,

On Monday 10 February 2014 05:10 AM, Ivan Khoronzhuk wrote:
> Add a broadcast timer64 based clockevent driver for keystone arch.
> This driver uses timer in 64-bit general purpose mode as clock event
> device.
> 
> Documentation:
>     http://www.ti.com/lit/ug/sprugv5a/sprugv5a.pdf
> 
> Based on
> git://git.kernel.org/pub/scm/linux/kernel/git/ssantosh/linux-keystone.git
> keystone/master
> 
> v5..v6:
> 	added function to encapsulate __iowmb().
> 
> v4..v5:
> 	used __iowmb() insted of wmb()
> 
> v3..v4:
> 	rebased on latest of linux-keystone.git keystone/master
> 
> v2..v3:
> - clocksource: timer-keystone: introduce clocksource driver for
> 	changed "u64" type to "unsigned long" for hz_period as more appropriate
> 	hz_period rounded up by DIV_ROUND_UP(rate, HZ)
> 	corrected comments
> 
> v1..v2:
> - clocksource: timer-keystone: introduce clocksource driver for
> 	renamed timer on "timer-keystone"
> 	in keystone_timer_interrupt() evet pointer is passed via "dev_id"
> 	used __relaxed variants of writel/readl and added explicit barriers
> 	added "keystone_timer_disable()" for using in keystone_set_mode()
> 	keystone_timer_config() is not used for disabling the timer any more
> 	in case of an unsupported mode the keystone_timer_config() returns -1.
> 	used request_irq() instead of setup_irq()
> 	assigned irq for event_device in event_dev->irq
> 	calculated timer.hz_period for CLOCK_EVT_MODE_PERIODIC at init
> 	deleted spare call of keystone_timer_config() in keystone_timer_init()
> 
> Ivan Khoronzhuk (3):
>   clocksource: timer-keystone: introduce clocksource driver for Keystone
>   clocksource: keystone: add bindings for keystone timer
>   arm: dts: keystone: add keystone timer entry
> 
Can you queue the first two patches from the series ?
I will queue the dts patch via my tree.

Regards,
Santosh



^ permalink raw reply

* Re: [PATCHv2 1/2] net: stmmac: Add SOCFPGA glue driver
From: Dinh Nguyen @ 2014-02-10 15:52 UTC (permalink / raw)
  To: David Miller
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA, dinh.linux-Re5JQEeQqe8AvxtiuMwx3w,
	devicetree-u79uwXL29TY76Z2rM5mHXA, peppe.cavallaro-qxv4g6HH51o,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, pawel.moll-5wv7dgnIgG8,
	mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
	galak-sgV2jX0FEOL9JmXXK+q4OQ,
	vbridgers2013-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <20140209.180642.1194468196963903290.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>

On Sun, 2014-02-09 at 18:06 -0800, David Miller wrote:
> From: <dinguyen-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>
> Date: Thu, 6 Feb 2014 17:19:54 -0600
> 
> > +MODULE_ALIAS("platform:socfpga-dwmac");
> > +MODULE_AUTHOR("Dinh Nguyen <dinguyen-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("Altera SOCFPGA DWMAC Glue Layer");
> > +
> 
> Do not add stray empty lines at the end of files.  Your second patch
> has this problem is a few places too.
> 

Will fix in v3.

Thanks,
Dinh


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

^ permalink raw reply

* Re: [PATCH v3 0/4] ARM: keystone: enable gpio support
From: Santosh Shilimkar @ 2014-02-10 15:53 UTC (permalink / raw)
  To: Grygorii Strashko, Rob Herring
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	ivan.khoronzhuk-l0cyMroinI0, Grant Likely, Mark Rutland
In-Reply-To: <1392050481-19086-1-git-send-email-grygorii.strashko-l0cyMroinI0@public.gmane.org>

On Monday 10 February 2014 11:41 AM, Grygorii Strashko wrote:
> This series adds Keystone GPIO IP definition in DT and enable GPIO support in
> Keystone configuration file.
> 
> Also, it enables LEDs support for Keystone 2 K2H EVM as specified in board 
> schematic. For more information see:
> - http://www.advantech.com/Support/TI-EVM/EVMK2HX.aspx 
> 
> Changes in v3:
>  - rebased on top 3.14 + "[PATCH v2] arm: dts: keystone: add watchdog entry"
>    http://www.spinics.net/lists/devicetree/msg20211.html
> 
> [v1-v2] http://www.spinics.net/lists/arm-kernel/msg294655.html
> 
> Grygorii Strashko (4):
>   ARM: dts: keystone: add gpio device entry
>   ARM: dts: keystone-evm: add leds supports
>   ARM: keystone_defconfig: enable gpio support
>   ARM: keystone_defconfig: enable led support
> 
Thanks for reposting the patchset Grygorii. I will
take them for the 3.15.

Would be good to get the blessing from DT folks
on first two patches.

Regards,
Santosh

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

^ permalink raw reply

* Re: Fwd: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support
From: Ivan T. Ivanov @ 2014-02-10 15:54 UTC (permalink / raw)
  To: dsneddon-sgV2jX0FEOL9JmXXK+q4OQ
  Cc: broonie-DgEjT+Ai2ygdnm+yROfE0A,
	grant.likely-QSEj5FYQhm4dnm+yROfE0A,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, linux-spi-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	inux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, alokc-sgV2jX0FEOL9JmXXK+q4OQ,
	gavidov-sgV2jX0FEOL9JmXXK+q4OQ, kgunda-sgV2jX0FEOL9JmXXK+q4OQ,
	sdharia-sgV2jX0FEOL9JmXXK+q4OQ
In-Reply-To: <214fe9fc7e62ab30bdfbb4ac5d1ee250.squirrel-mMfbam+mt9083fI46fginR2eb7JE58TQ@public.gmane.org>


Hi Daniel,

On Fri, 2014-02-07 at 16:34 +0000, dsneddon-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org wrote: 
> > From: "Ivan T. Ivanov" <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>


<snip>

> > +
> > +static int spi_qup_set_state(struct spi_qup *controller, u32 state)
> > +{
> > +       unsigned long loop = 0;
> > +       u32 cur_state;
> > +
> > +       cur_state = readl_relaxed(controller->base + QUP_STATE);
> Make sure the state is valid before you read the current state.

Why? Controller is always left in valid state (after probe and every
transaction)? I know that CAF code contain this check, but now driver
is little bit different. I have made some tests and controller is
always in valid state in this point.

> > +       /*
> > +        * Per spec: for PAUSE_STATE to RESET_STATE, two writes
> > +        * of (b10) are required
> > +        */
> > +       if (((cur_state & QUP_STATE_MASK) == QUP_STATE_PAUSE) &&
> > +           (state == QUP_STATE_RESET)) {
> > +               writel_relaxed(QUP_STATE_CLEAR, controller->base +
> > QUP_STATE);
> > +               writel_relaxed(QUP_STATE_CLEAR, controller->base +
> > QUP_STATE);
> > +       } else {
> Make sure you don't transition from RESET to PAUSE.

I don't see this to be handled in CAF code. Could you give me
more details, please?

Right now possible state transactions are:

* if everything is fine:
  RESET -> RUN -> PAUSE -> RUN -> RESET 
* in case of error: 
  RESET -> RUN -> RESET 
  RESET -> RUN -> PAUSE -> RESET

Please correct me if I am wrong.

> > +
> > +static void spi_qup_fifo_read(struct spi_qup *controller,
> > +                             struct spi_transfer *xfer)
> > +{
> > +       u8 *rx_buf = xfer->rx_buf;
> > +       u32 word, state;
> > +       int idx, shift;
> > +
> > +       while (controller->rx_bytes < xfer->len) {
> > +
> > +               state = readl_relaxed(controller->base + QUP_OPERATIONAL);
> > +               if (0 == (state & QUP_OP_IN_FIFO_NOT_EMPTY))
> > +                       break;
> > +
> > +               word = readl_relaxed(controller->base + QUP_INPUT_FIFO);
> > +
> > +               for (idx = 0; idx < controller->bytes_per_word &&
> > +                    controller->rx_bytes < xfer->len; idx++,
> > +                    controller->rx_bytes++) {
> > +
> > +                       if (!rx_buf)
> > +                               continue;
> If there is no rx_buf just set rx_bytes to xfer->len and skip the loop
> entirely.

Well, FIFO buffer still should be drained, right?
Anyway. I am looking for better way to handle this.
Same applies for filling FIFO buffer.

> > +                       /*
> > +                        * The data format depends on bytes_per_word:
> > +                        *  4 bytes: 0x12345678
> > +                        *  2 bytes: 0x00001234
> > +                        *  1 byte : 0x00000012
> > +                        */
> > +                       shift = BITS_PER_BYTE;
> > +                       shift *= (controller->bytes_per_word - idx - 1);
> > +                       rx_buf[controller->rx_bytes] = word >> shift;
> > +               }
> > +       }
> > +}

<snip>

> > +static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
> > +{
> > +       struct spi_qup *controller = dev_id;
> > +       struct spi_transfer *xfer;
> > +       u32 opflags, qup_err, spi_err;
> > +
> > +       xfer = controller->xfer;
> > +
> > +       qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS);
> > +       spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS);
> > +       opflags = readl_relaxed(controller->base + QUP_OPERATIONAL);
> > +
> > +       writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
> > +       writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
> > +       writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> > +
> > +       if (!xfer)
> > +               return IRQ_HANDLED;
> > +
> > +       if (qup_err) {
> > +               if (qup_err & QUP_ERROR_OUTPUT_OVER_RUN)
> > +                       dev_warn(controller->dev, "OUTPUT_OVER_RUN\n");
> > +               if (qup_err & QUP_ERROR_INPUT_UNDER_RUN)
> > +                       dev_warn(controller->dev, "INPUT_UNDER_RUN\n");
> > +               if (qup_err & QUP_ERROR_OUTPUT_UNDER_RUN)
> > +                       dev_warn(controller->dev, "OUTPUT_UNDER_RUN\n");
> > +               if (qup_err & QUP_ERROR_INPUT_OVER_RUN)
> > +                       dev_warn(controller->dev, "INPUT_OVER_RUN\n");
> > +
> > +               controller->error = -EIO;
> > +       }
> > +
> > +       if (spi_err) {
> > +               if (spi_err & SPI_ERROR_CLK_OVER_RUN)
> > +                       dev_warn(controller->dev, "CLK_OVER_RUN\n");
> > +               if (spi_err & SPI_ERROR_CLK_UNDER_RUN)
> > +                       dev_warn(controller->dev, "CLK_UNDER_RUN\n");
> > +
> > +               controller->error = -EIO;
> > +       }
> > +
> > +       if (opflags & QUP_OP_IN_SERVICE_FLAG) {
> > +               writel_relaxed(QUP_OP_IN_SERVICE_FLAG,
> > +                      controller->base + QUP_OPERATIONAL);
> Write is not necessary since already cleared above.
> > +               spi_qup_fifo_read(controller, xfer);
> > +       }
> > +
> > +       if (opflags & QUP_OP_OUT_SERVICE_FLAG) {
> > +               writel_relaxed(QUP_OP_OUT_SERVICE_FLAG,
> > +                      controller->base + QUP_OPERATIONAL);
> Write is not necessary since already cleared above.

True. I have forgot to remove these writes. Will fix.

Regards,
Ivan

> > +               spi_qup_fifo_write(controller, xfer);
> > +       }
> > +
> > +       if (controller->rx_bytes == xfer->len ||
> > +           controller->error)
> > +               complete(&controller->done);
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +


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

^ permalink raw reply

* Re: [PATCH V5 4/8] phy: st-miphy-40lp: Add skeleton driver
From: Arnd Bergmann @ 2014-02-10 15:54 UTC (permalink / raw)
  To: Mohit Kumar
  Cc: Pratyush Anand, Kishon Vijay Abraham I,
	spear-devel-nkJGhpqTU55BDgjK7y7TUQ,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <fb505abd332645671d9ad125003d8253b6346715.1391871170.git.pratyush.anand-qxv4g6HH51o@public.gmane.org>

On Monday 10 February 2014, Mohit Kumar wrote:
> diff --git a/Documentation/devicetree/bindings/phy/st-miphy40lp.txt b/Documentation/devicetree/bindings/phy/st-miphy40lp.txt
> new file mode 100644
> index 0000000..d0c7096
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/st-miphy40lp.txt
> @@ -0,0 +1,12 @@
> +Required properties:
> +- compatible : should be "st,miphy40lp-phy"
> +	Other supported soc specific compatible:
> +		"st,spear1310-miphy"
> +		"st,spear1340-miphy"
> +- reg : offset and length of the PHY register set.
> +- misc: phandle for the syscon node to access misc registers
> +- phy-id: Instance id of the phy.
> +- #phy-cells : from the generic PHY bindings, must be 1.
> +	- 1st cell: phandle to the phy node.
> +	- 2nd cell: 0 if phy (in 1st cell) is to be used for SATA, 1 for PCIe
> +	  and 2 for Super Speed USB.

It's common to start this file with a small header explaining what this
hardware is.

> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index afa2354..2f58993 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -64,4 +64,10 @@ config BCM_KONA_USB2_PHY
>  	help
>  	  Enable this to support the Broadcom Kona USB 2.0 PHY.
>  
> +config PHY_ST_MIPHY40LP
> +	tristate "ST MIPHY 40LP driver"
> +	help
> +	  Support for ST MIPHY 40LP which can be used for PCIe, SATA and Super Speed USB.
> +	select GENERIC_PHY
> +
>  endmenu

The 'select' statement should come before 'help', for consistency with the
rest of the kernel. Maybe mention that this phy is used inside the
spear13xx SoC here rather than a standalone phy.

> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv) {
> +		dev_err(dev, "can't alloc miphy40lp private date memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	priv->plat_ops = (struct miphy40lp_plat_ops *)of_id->data;

The cast would incorrectly remove the 'const' attribute of the pointer.
Better remove the cast and make priv->plat_ops const.

> +static int __init miphy40lp_phy_init(void)
> +{
> +
> +	return platform_driver_probe(&miphy40lp_driver,
> +				miphy40lp_probe);
> +}
> +module_init(miphy40lp_phy_init);

There should certainly be a module_exit() function here so you can unload the
driver.

	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

^ permalink raw reply

* Re: [PATCH v4 2/2] memory: ti-aemif: add bindings for AEMIF driver
From: Santosh Shilimkar @ 2014-02-10 16:14 UTC (permalink / raw)
  To: Ivan Khoronzhuk, galak
  Cc: rob, linux, devicetree, pawel.moll, mark.rutland, rob.herring,
	swarren, ijc+devicetree, linux-kernel, linux-arm-kernel,
	linux-mtd, grygorii.strashko, gregkh, dwmw2, nsekhar
In-Reply-To: <1391629574-18955-3-git-send-email-ivan.khoronzhuk@ti.com>

Kumar,

On Wednesday 05 February 2014 02:46 PM, Ivan Khoronzhuk wrote:
> Add bindings for TI Async External Memory Interface (AEMIF) controller.
> 
> The Async External Memory Interface (EMIF16/AEMIF) controller is intended to
> provide a glue-less interface to a variety of asynchronous memory devices like
> ASRA M, NOR and NAND memory. A total of 256M bytes of any of these memories
> can be accessed via 4 chip selects with 64M byte access per chip select.
> 
> We are not encoding CS number in reg property, it's memory partition number.
> The CS number is encoded for Davinci NAND node using standalone property
> "ti,davinci-chipselect" and we need to provide two memory ranges to it,
> as result we can't encode CS number in "reg" for AEMIF child devices
> (NAND/NOR/etc), as it will break bindings compatibility.
> 
> In this patch, NAND node is used just as an example of child node.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
Can you please have a look at the binding which we discussed in previous
versions ? I want to queue these for 3.15 via Greg's tree but need
your ack on DT part at least.

>  .../bindings/memory-controllers/ti-aemif.txt       | 210 +++++++++++++++++++++
>  1 file changed, 210 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt b/Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt
> new file mode 100644
> index 0000000..d1466dc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt
> @@ -0,0 +1,210 @@
> +* Device tree bindings for Texas instruments AEMIF controller
> +
> +The Async External Memory Interface (EMIF16/AEMIF) controller is intended to
> +provide a glue-less interface to a variety of asynchronous memory devices like
> +ASRA M, NOR and NAND memory. A total of 256M bytes of any of these memories
> +can be accessed at any given time via four chip selects with 64M byte access
> +per chip select. Synchronous memories such as DDR1 SD RAM, SDR SDRAM
> +and Mobile SDR are not supported.
> +
> +Documentation:
> +Davinci DM646x - http://www.ti.com/lit/ug/sprueq7c/sprueq7c.pdf
> +OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh77a/spruh77a.pdf
> +Kestone - http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf
> +
> +Required properties:
> +
> +- compatible:		"ti,davinci-aemif"
> +			"ti,keystone-aemif"
> +			"ti,da850-aemif"
> +
> +- reg:			contains offset/length value for AEMIF control registers
> +			space.
> +
> +- #address-cells:	Must be 2. The partition number has to be encoded in the
> +			first address cell and it may accept values 0..N-1
> +			(N - total number of partitions). It's recommended to
> +			assign N-1 number for the control partition. The second
> +			cell is the offset into the partition.
> +
> +- #size-cells:		Must be set to 1.
> +
> +- ranges:		Contains memory regions. There are two types of
> +			ranges/partitions:
> +			- CS-specific partition/range. If continuous, must be
> +			set up to reflect the memory layout for 4 chipselects,
> +			if not then additional range/partition can be added and
> +			child device can select the proper one.
> +			- control partition which is common for all CS
> +			interfaces.
> +
> +- clocks:		the clock feeding the controller clock. Required only
> +			if clock tree data present in device tree.
> +			See clock-bindings.txt
> +
> +- clock-names:		clock name. It has to be "aemif". Required only if clock
> +			tree data present in device tree, in another case don't
> +			use it.
> +			See clock-bindings.txt
> +
> +- clock-ranges:		Empty property indicating that child nodes can inherit
> +			named clocks. Required only if clock tree data present
> +			in device tree.
> +			See clock-bindings.txt
> +
> +
> +Child chip-select (cs) nodes contain the memory devices nodes connected to
> +such as NOR (e.g. cfi-flash) and NAND (ti,davinci-nand, see davinci-nand.txt).
> +There might be board specific devices like FPGAs.
> +
> +Required child cs node properties:
> +
> +- #address-cells:	Must be 2.
> +
> +- #size-cells:		Must be 1.
> +
> +- ranges:		Empty property indicating that child nodes can inherit
> +			memory layout.
> +
> +- clock-ranges:		Empty property indicating that child nodes can inherit
> +			named clocks. Required only if clock tree data present
> +			in device tree.
> +
> +- ti,cs-chipselect:	number of chipselect. Indicates on the aemif driver
> +			which chipselect is used for accessing the memory. For
> +			compatibles "ti,davinci-aemif" and "ti,keystone-aemif"
> +			it can be in range [0-3]. For compatible
> +			"ti,da850-aemif" range is [2-5].
> +
> +Optional child cs node properties:
> +
> +- ti,bus-width:			width of the asynchronous device's data bus
> +				8 or 16 if not preset 8
> +
> +- ti,cs-ss:		enable/disable select strobe mode
> +				In select strobe mode chip select behaves as
> +				the strobe and is active only during the strobe
> +				period. If present then enable.
> +
> +- ti,cs-ew:		enable/disable extended wait mode
> +				if set, the controller monitors the EMIFWAIT pin
> +				mapped to that chip select to determine if the
> +				device wants to extend the strobe period. If
> +				present then enable.
> +
> +- ti,cs-ta:		minimum turn around time, ns
> +				Time between the end of one asynchronous memory
> +				access and the start of another asynchronous
> +				memory access. This delay is not incurred
> +				between a read followed by read or a write
> +				followed by a write to same chip select.
> +
> +- ti,cs-rsetup:		read setup width, ns
> +				Time between the beginning of a memory cycle
> +				and the activation of read strobe.
> +				Minimum value is 1 (0 treated as 1).
> +
> +- ti,cs-rstobe:		read strobe width, ns
> +				Time between the activation and deactivation of
> +				the read strobe.
> +				Minimum value is 1 (0 treated as 1).
> +
> +- ti,cs-rhold:		read hold width, ns
> +				Time between the deactivation of the read
> +				strobe and the end of the cycle (which may be
> +				either an address change or the deactivation of
> +				the chip select signal.
> +				Minimum value is 1 (0 treated as 1).
> +
> +- ti,cs-wsetup:		write setup width, ns
> +				Time between the beginning of a memory cycle
> +				and the activation of write strobe.
> +				Minimum value is 1 (0 treated as 1).
> +
> +- ti,cs-wstrobe:	write strobe width, ns
> +				Time between the activation and deactivation of
> +				the write strobe.
> +				Minimum value is 1 (0 treated as 1).
> +
> +- ti,cs-whold:		write hold width, ns
> +				Time between the deactivation of the write
> +				strobe and the end of the cycle (which may be
> +				either an address change or the deactivation of
> +				the chip select signal.
> +				Minimum value is 1 (0 treated as 1).
> +
> +If any of the above parameters are absent, current parameter value will be taken
> +from the corresponding HW reg.
> +
> +Example for aemif, davinci nand and nor flash chip select shown below.
> +
> +memory-controller@21000A00 {
> +	compatible = "ti,davinci-aemif";
> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +	clocks = <&clkaemif 0>;
> +	clock-names = "aemif";
> +	clock-ranges;
> +	reg = <0x2100A00 0x00000100>;
> +	ranges = <0 0 0x70000000 0x10000000
> +		  1 0 0x21000A00 0x0000100>;
> +		  /*
> +		   * Partition0: CS-specific memory range which is
> +		   * implemented as continuous physical memory region
> +		   * Partition1: control memory range
> +		   */
> +
> +	nand:cs2 {
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		clock-ranges;
> +		ranges;
> +
> +		ti,cs-chipselect = <2>;
> +		/* all timings in nanoseconds */
> +		ti,cs-ta = <0>;
> +		ti,cs-rhold = <7>;
> +		ti,cs-rstrobe = <42>;
> +		ti,cs-rsetup = <14>;
> +		ti,cs-whold = <7>;
> +		ti,cs-wstrobe = <42>;
> +		ti,cs-wsetup = <14>;
> +
> +		nand@0,0x8000000 {
> +			compatible = "ti,davinci-nand";
> +			reg = <0 0x8000000 0x4000000
> +			       1 0x0000000 0x0000100>;
> +			/*
> +			 * Partition0, offset 0x8000000, size 0x4000000
> +			 * Partition1, offset 0x0000000, size 0x0000100
> +			 */
> +
> +			.. see davinci-nand.txt
> +		};
> +	};
> +
> +	nor:cs0 {
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		clock-ranges;
> +		ranges;
> +
> +		ti,cs-chipselect = <0>;
> +		/* all timings in nanoseconds */
> +		ti,cs-ta = <0>;
> +		ti,cs-rhold = <8>;
> +		ti,cs-rstrobe = <40>;
> +		ti,cs-rsetup = <14>;
> +		ti,cs-whold = <7>;
> +		ti,cs-wstrobe = <40>;
> +		ti,cs-wsetup = <14>;
> +		ti,cs-asize = <1>;
> +
> +		flash@0,0x0000000 {
> +			compatible = "cfi-flash";
> +			reg = <0 0x0000000 0x4000000>;
> +
> +			...
> +		};
> +	};
> +};
> 

^ permalink raw reply

* Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree
From: Gerlando Falauto @ 2014-02-10 16:14 UTC (permalink / raw)
  To: Florian Fainelli, Matthew Garrett
  Cc: netdev, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	Kishon Vijay Abraham I
In-Reply-To: <CAGVrzcZ4TFd=9KP+aoG47QbmqDJ1i23WBcEWDbzNRUfGmPvZHQ@mail.gmail.com>

Hi,

I'm currently trying to fix an issue for which this patch provides a 
partial solution, so apologies in advance for jumping into the 
discussion for my own purposes...

On 02/04/2014 09:39 PM, Florian Fainelli wrote:> 2014-01-17 Matthew 
Garrett <matthew.garrett@nebula.com>:
 >> Some hardware may be broken in interesting and board-specific ways, such
 >> that various bits of functionality don't work. This patch provides a
 >> mechanism for overriding mii registers during init based on the 
contents of
 >> the device tree data, allowing board-specific fixups without having to
 >> pollute generic code.
 >
 > It would be good to explain exactly how your hardware is broken
 > exactly. I really do not think that such a fine-grained setting where
 > you could disable, e.g: 100BaseT_Full, but allow 100BaseT_Half to
 > remain usable makes that much sense. In general, Gigabit might be
 > badly broken, but 100 and 10Mbits/sec should work fine. How about the
 > MASTER-SLAVE bit, is overriding it really required?
 >
 > Is not a PHY fixup registered for a specific OUI the solution you are
 > looking for? I am also concerned that this creates PHY troubleshooting
 > issues much harder to debug than before as we may have no idea about
 > how much information has been put in Device Tree to override that.
 >
 > Finally, how about making this more general just like the BCM87xx PHY
 > driver, which is supplied value/reg pairs directly? There are 16
 > common MII registers, and 16 others for vendor specific registers,
 > this is just covering for about 2% of the possible changes.

Good point. That would easily help me with my current issue, which 
requires autoneg to be disabled to begin with (by clearing BMCR_ANENABLE 
from register 0).
This would not however fix it entirely (I tried a quick hardwired 
implementation), as the whole PHY machinery would not take that into 
account and would re-enable autoneg anyway.
I also tried changing the patch so that phydev->support gets updated 
(instead of phydev->advertising):

 >> +               if (!of_property_read_u32(np, override->prop, &tmp)) {
 >> +                       if (tmp) {
 >> +                               *val |= override->value;
 >> +                               phydev->advertising |= 
override->supported;
 >> +                       } else {
 >> +                               phydev->advertising &= 
~(override->supported);
 >> +                       }
 >> +
 >> +                       *mask |= override->value;

What I find weird is that the only way phydev->autoneg could ever be set 
to disabled is from here (phy.c):

static void phy_sanitize_settings(struct phy_device *phydev)
{
	u32 features = phydev->supported;
	int idx;

	/* Sanitize settings based on PHY capabilities */
	if ((features & SUPPORTED_Autoneg) == 0)
		phydev->autoneg = AUTONEG_DISABLE;

which is in turn only called when phydev->autoneg is set to 
AUTONEG_DISABLE to begin with:

int phy_start_aneg(struct phy_device *phydev)
{
	int err;

	mutex_lock(&phydev->lock);

	if (AUTONEG_DISABLE == phydev->autoneg)
		phy_sanitize_settings(phydev);

So could someone please help me figure out what I'm missing here?

Thanks!
Gerlando

^ permalink raw reply

* Re: Fwd: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support
From: dsneddon @ 2014-02-10 16:21 UTC (permalink / raw)
  To: Ivan T. Ivanov
  Cc: dsneddon, broonie, grant.likely, robh+dt, linux-spi,
	linux-arm-msm, inux-kernel, devicetree, alokc, gavidov, kgunda,
	sdharia
In-Reply-To: <1392047651.2853.19.camel@iivanov-dev>

Hi Ivan,
>
>> > +
>> > +static int spi_qup_set_state(struct spi_qup *controller, u32 state)
>> > +{
>> > +       unsigned long loop = 0;
>> > +       u32 cur_state;
>> > +
>> > +       cur_state = readl_relaxed(controller->base + QUP_STATE);
>> Make sure the state is valid before you read the current state.
>
> Why? Controller is always left in valid state (after probe and every
> transaction)? I know that CAF code contain this check, but now driver
> is little bit different. I have made some tests and controller is
> always in valid state in this point.

The hardware programming guide we recommends doing this.  I'd have to talk
to the hardware designers to know exactly the reason why.  I can follow up
on this if you'd like.

>
>> > +       /*
>> > +        * Per spec: for PAUSE_STATE to RESET_STATE, two writes
>> > +        * of (b10) are required
>> > +        */
>> > +       if (((cur_state & QUP_STATE_MASK) == QUP_STATE_PAUSE) &&
>> > +           (state == QUP_STATE_RESET)) {
>> > +               writel_relaxed(QUP_STATE_CLEAR, controller->base +
>> > QUP_STATE);
>> > +               writel_relaxed(QUP_STATE_CLEAR, controller->base +
>> > QUP_STATE);
>> > +       } else {
>> Make sure you don't transition from RESET to PAUSE.
>
> I don't see this to be handled in CAF code. Could you give me
> more details, please?
>
> Right now possible state transactions are:
>
> * if everything is fine:
>   RESET -> RUN -> PAUSE -> RUN -> RESET
> * in case of error:
>   RESET -> RUN -> RESET
>   RESET -> RUN -> PAUSE -> RESET
>
> Please correct me if I am wrong.

According to the hardware documentation if the hardware is in the RESET
state and we try to transition to the PAUSE state the hardware behavior is
"undefined", which usually means bad things will happen.  Admittedly, if
the driver always follows the "valid" rules (the ones you've listed above)
it _should_ never happen.  However, it is _possible_ the hardware is in
the RESET state while the driver thinks it's in the RUN state and the
driver tries to put is in the PAUSE state.  In other words, I'd like to
err on the side of caution since the check doesn't really cost us
anything.

>
>> > +
>> > +static void spi_qup_fifo_read(struct spi_qup *controller,
>> > +                             struct spi_transfer *xfer)
>> > +{
>> > +       u8 *rx_buf = xfer->rx_buf;
>> > +       u32 word, state;
>> > +       int idx, shift;
>> > +
>> > +       while (controller->rx_bytes < xfer->len) {
>> > +
>> > +               state = readl_relaxed(controller->base +
>> QUP_OPERATIONAL);
>> > +               if (0 == (state & QUP_OP_IN_FIFO_NOT_EMPTY))
>> > +                       break;
>> > +
>> > +               word = readl_relaxed(controller->base +
>> QUP_INPUT_FIFO);
>> > +
>> > +               for (idx = 0; idx < controller->bytes_per_word &&
>> > +                    controller->rx_bytes < xfer->len; idx++,
>> > +                    controller->rx_bytes++) {
>> > +
>> > +                       if (!rx_buf)
>> > +                               continue;
>> If there is no rx_buf just set rx_bytes to xfer->len and skip the loop
>> entirely.
>
> Well, FIFO buffer still should be drained, right?
> Anyway. I am looking for better way to handle this.
> Same applies for filling FIFO buffer.
>

Yes.  Still read from the FIFO but skip the for loop since we're just
adding bytes_per_word to the total rx_byte count one iteration at a time
and not doing anything with the data.

Thanks,
Dan

^ permalink raw reply

* Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support
From: Ivan T. Ivanov @ 2014-02-10 16:29 UTC (permalink / raw)
  To: Mark Brown
  Cc: Grant Likely, Rob Herring, linux-spi-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Alok Chauhan, Gilad Avidov,
	Kiran Gunda, Sagar Dharia
In-Reply-To: <20140207171202.GD1757-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>


Hi,

On Fri, 2014-02-07 at 17:12 +0000, Mark Brown wrote: 
> On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:
> 
> This looks mostly good, there's a few odd things and missing use of
> framework features.
> 
> > Qualcomm Universal Peripheral (QUP) core is an AHB slave that
> > provides a common data path (an output FIFO and an input FIFO)
> > for serial peripheral interface (SPI) mini-core. SPI in master mode
> > support up to 50MHz, up to four chip selects, and a programmable
> > data path from 4 bits to 32 bits; MODE0..3 protocols
> 
> The grammar in this and the Kconfig text is a bit garbled, might want to
> give it a once over (support -> supports for example).

Ok

> 
> > +static void spi_qup_deassert_cs(struct spi_qup *controller,
> > +				struct spi_qup_device *chip)
> > +{
> 
> 
> > +	if (chip->mode & SPI_CS_HIGH)
> > +		iocontol &= ~mask;
> > +	else
> > +		iocontol |= mask;
> 
> Implement a set_cs() operation and let the core worry about all this
> for you as well as saving two implementations.

Nice. Will us it.

> 
> > +		word = 0;
> > +		for (idx = 0; idx < controller->bytes_per_word &&
> > +		     controller->tx_bytes < xfer->len; idx++,
> > +		     controller->tx_bytes++) {
> > +
> > +			if (!tx_buf)
> > +				continue;
> 
> Do you need to set the _MUST_TX flag?
> 
> > +	qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS);
> > +	spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS);
> > +	opflags = readl_relaxed(controller->base + QUP_OPERATIONAL);
> > +
> > +	writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
> > +	writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
> > +	writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> > +
> > +	if (!xfer)
> > +		return IRQ_HANDLED;
> 
> Are you sure?  It seems wrong to just ignore interrupts, some comments
> would help explain why.

Of course this should never happen. This was left from initial stage
of the implementation.

> 
> > +static int spi_qup_transfer_do(struct spi_qup *controller,
> > +			       struct spi_qup_device *chip,
> > +			       struct spi_transfer *xfer)
> 
> This looks like a transfer_one() function, please use the framework
> features where you can.

Sure, will do. Somehow I have missed this.

> 
> > +	if (controller->speed_hz != chip->speed_hz) {
> > +		ret = clk_set_rate(controller->cclk, chip->speed_hz);
> > +		if (ret) {
> > +			dev_err(controller->dev, "fail to set frequency %d",
> > +				chip->speed_hz);
> > +			return -EIO;
> > +		}
> > +	}
> 
> Is calling into the clock framework really so expensive that we need to
> avoid doing it?  

Probably not. But why to call it if the frequency is the same.


> You also shouldn't be interacting with the hardware in
> setup().

This is internal hw-setup, not master::setup() or I am
missing something?

> 
> > +	if (chip->bits_per_word <= 8)
> > +		controller->bytes_per_word = 1;
> > +	else if (chip->bits_per_word <= 16)
> > +		controller->bytes_per_word = 2;
> > +	else
> > +		controller->bytes_per_word = 4;
> 
> This looks like a switch statement, and looking at the above it's not
> clear that the device actually supports anything other than whole bytes.
> I'm not sure what that would mean from an API point of view.

SPI API didn't validate struct spi_transfer::len field.

The whole sniped looks like this:

	chip->bits_per_word = spi->bits_per_word;
	if (xfer->bits_per_word)
		chip->bits_per_word = xfer->bits_per_word;

	if (chip->bits_per_word <= 8)
		controller->bytes_per_word = 1;
	else if (chip->bits_per_word <= 16)
		controller->bytes_per_word = 2;
	else
		controller->bytes_per_word = 4;

	if (controller->bytes_per_word > xfer->len ||
	    xfer->len % controller->bytes_per_word != 0){
		/* No partial transfers */
		dev_err(controller->dev, "invalid len %d for %d bits\n",
			xfer->len, chip->bits_per_word);
		return -EIO;
	}

	n_words = xfer->len / controller->bytes_per_word;

'bytes_per_word' have to be power of 2. This is my understanding of
struct spi_transfer description. So I am discarding all transfers with
'len' non multiple of word size.


> 
> > +static int spi_qup_transfer_one(struct spi_master *master,
> > +				struct spi_message *msg)
> > +{
> 
> This entire function can be removed, the core can do it for you.

Sure, will use it.

> 
> > +	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
> > +		max_freq = 19200000;
> > +
> > +	if (!max_freq) {
> > +		dev_err(dev, "invalid clock frequency %d\n", max_freq);
> > +		return -ENXIO;
> > +	}
> > +
> > +	ret = clk_set_rate(cclk, max_freq);
> > +	if (ret)
> > +		dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);
> 
> You set the clock rate per transfer so why bother setting it here,

Only if differs from the current one.

> perhaps we support the rate the devices request but not this maximum
> rate?

Thats why it is just a warning. I will see how to handle this better.

> 
> > +	master->num_chipselect = SPI_NUM_CHIPSELECTS;
> > +	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
> 
> Are you *sure* the device supports anything other than whole bytes?

Yep. I see them on the scope.

> 
> > +	ret = devm_spi_register_master(dev, master);
> > +	if (!ret) {
> > +		pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
> > +		pm_runtime_use_autosuspend(dev);
> > +		pm_runtime_set_active(dev);
> > +		pm_runtime_enable(dev);
> > +		return ret;
> > +	}
> 
> This is really unclearly written, the success case looks like error
> handling.

I suppose that if use a goto, I will have to do it consistently. 
Will rework it.

> 
> > +#ifdef CONFIG_PM_RUNTIME
> > +static int spi_qup_pm_suspend_runtime(struct device *device)
> > +{
> > +	struct spi_master *master = dev_get_drvdata(device);
> > +	struct spi_qup *controller = spi_master_get_devdata(master);
> > +
> > +	disable_irq(controller->irq);
> 
> Why do you need to disable the interrupt?  Will the hardware generate
> spurious interrupts, if so some documentation is in order.

I don't know. Just extra protection? I will have t o find how to 
test this.

> 
> > +static int spi_qup_pm_resume_runtime(struct device *device)
> > +{
> > +	struct spi_master *master = dev_get_drvdata(device);
> > +	struct spi_qup *controller = spi_master_get_devdata(master);
> > +
> > +	clk_prepare_enable(controller->cclk);
> > +	clk_prepare_enable(controller->iclk);
> > +	enable_irq(controller->irq);
> 
> No error checking here...

Will fix.

Thanks,
Ivan


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

^ permalink raw reply

* [PATCH v3 0/4] ARM: keystone: enable gpio support
From: Grygorii Strashko @ 2014-02-10 16:41 UTC (permalink / raw)
  To: santosh.shilimkar-l0cyMroinI0
  Cc: Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	ivan.khoronzhuk-l0cyMroinI0, Grygorii Strashko

This series adds Keystone GPIO IP definition in DT and enable GPIO support in
Keystone configuration file.

Also, it enables LEDs support for Keystone 2 K2H EVM as specified in board 
schematic. For more information see:
- http://www.advantech.com/Support/TI-EVM/EVMK2HX.aspx 

Changes in v3:
 - rebased on top 3.14 + "[PATCH v2] arm: dts: keystone: add watchdog entry"
   http://www.spinics.net/lists/devicetree/msg20211.html

[v1-v2] http://www.spinics.net/lists/arm-kernel/msg294655.html

Grygorii Strashko (4):
  ARM: dts: keystone: add gpio device entry
  ARM: dts: keystone-evm: add leds supports
  ARM: keystone_defconfig: enable gpio support
  ARM: keystone_defconfig: enable led support

 arch/arm/boot/dts/k2hk-evm.dts      |   23 ++++++++++++++++++
 arch/arm/boot/dts/keystone.dtsi     |   45 +++++++++++++++++++++++++++++++++++
 arch/arm/configs/keystone_defconfig |   11 +++++++++
 3 files changed, 79 insertions(+)

-- 
1.7.9.5

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

^ permalink raw reply

* [PATCH v3 1/4] ARM: dts: keystone: add gpio device entry
From: Grygorii Strashko @ 2014-02-10 16:41 UTC (permalink / raw)
  To: santosh.shilimkar-l0cyMroinI0
  Cc: Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	ivan.khoronzhuk-l0cyMroinI0, Grygorii Strashko
In-Reply-To: <1392050481-19086-1-git-send-email-grygorii.strashko-l0cyMroinI0@public.gmane.org>

This patch adds Keystone GPIO IP device definitions in DT which supports
up to 32 GPIO lines and each GPIO line can be configured as separate
interrupt source (so called "unbanked" IRQ).

For more information see:
 http://www.ti.com/lit/ug/sprugv1/sprugv1.pdf

Signed-off-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
---
 arch/arm/boot/dts/keystone.dtsi |   45 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/arch/arm/boot/dts/keystone.dtsi b/arch/arm/boot/dts/keystone.dtsi
index 3a83ffe..0f82bb9 100644
--- a/arch/arm/boot/dts/keystone.dtsi
+++ b/arch/arm/boot/dts/keystone.dtsi
@@ -7,6 +7,7 @@
  */
 
 #include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/gpio/gpio.h>
 
 #include "skeleton.dtsi"
 
@@ -214,5 +215,49 @@
 			reg = <0x022f0080 0x80>;
 			clocks = <&clkwdtimer0>;
 		};
+
+		gpio0: gpio@260bf00 {
+			compatible = "ti,keystone-gpio";
+			reg = <0x0260bf00 0x100>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			/* HW Interrupts mapped to GPIO pins */
+			interrupts = <GIC_SPI 120 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 121 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 122 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 123 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 124 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 125 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 126 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 127 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 128 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 129 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 130 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 131 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 132 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 133 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 134 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 135 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 136 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 137 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 138 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 139 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 140 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 141 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 142 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 143 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 144 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 145 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 146 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 147 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 148 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 149 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 150 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 151 IRQ_TYPE_EDGE_RISING>;
+			clocks = <&clkgpio>;
+			clock-names = "gpio";
+			ti,ngpio = <32>;
+			ti,davinci-gpio-unbanked = <32>;
+		};
 	};
 };
-- 
1.7.9.5

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

^ permalink raw reply related

* [PATCH v3 2/4] ARM: dts: keystone-evm: add leds supports
From: Grygorii Strashko @ 2014-02-10 16:41 UTC (permalink / raw)
  To: santosh.shilimkar-l0cyMroinI0
  Cc: Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	ivan.khoronzhuk-l0cyMroinI0, Grygorii Strashko
In-Reply-To: <1392050481-19086-1-git-send-email-grygorii.strashko-l0cyMroinI0@public.gmane.org>

Keystone EVMK2HX supports 4 debug LEDs controlled by GPIO lines as
following (active level is high);

DBG_D1 green gpio12
DBG_D1 red   gpio13
DBG_D1 blue  gpio14
DBG_D1 blue  gpio15

For more information see schematics:
 http://wfcache.advantech.com/www/support/TI-EVM/download/Schematics/PDF/K2H_K2EVM-HK_SCH_A102_Rev1_0.pdf

Signed-off-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
---
 arch/arm/boot/dts/k2hk-evm.dts |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm/boot/dts/k2hk-evm.dts b/arch/arm/boot/dts/k2hk-evm.dts
index eaefdfe..3513f8b 100644
--- a/arch/arm/boot/dts/k2hk-evm.dts
+++ b/arch/arm/boot/dts/k2hk-evm.dts
@@ -52,6 +52,29 @@
 			};
 		};
 	};
+
+	leds {
+		compatible = "gpio-leds";
+		debug1_1 {
+			label = "keystone:green:debug1";
+			gpios = <&gpio0 12 GPIO_ACTIVE_HIGH>; /* 12 */
+		};
+
+		debug1_2 {
+			label = "keystone:red:debug1";
+			gpios = <&gpio0 13 GPIO_ACTIVE_HIGH>; /* 13 */
+		};
+
+		debug2 {
+			label = "keystone:blue:debug2";
+			gpios = <&gpio0 14 GPIO_ACTIVE_HIGH>; /* 14 */
+		};
+
+		debug3 {
+			label = "keystone:blue:debug3";
+			gpios = <&gpio0 15 GPIO_ACTIVE_HIGH>; /* 15 */
+		};
+	};
 };
 
 &usb_phy {
-- 
1.7.9.5

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

^ permalink raw reply related

* [PATCH v3 3/4] ARM: keystone_defconfig: enable gpio support
From: Grygorii Strashko @ 2014-02-10 16:41 UTC (permalink / raw)
  To: santosh.shilimkar-l0cyMroinI0
  Cc: Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	ivan.khoronzhuk-l0cyMroinI0, Grygorii Strashko
In-Reply-To: <1392050481-19086-1-git-send-email-grygorii.strashko-l0cyMroinI0@public.gmane.org>

Enable enable GPIO support for Keystone by setting CONFIG_GPIOLIB and
CONFIG_GPIO_DAVINCI options in keystone_defconfig.

Signed-off-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
---
 arch/arm/configs/keystone_defconfig |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/configs/keystone_defconfig b/arch/arm/configs/keystone_defconfig
index 332ca72..f9467ad 100644
--- a/arch/arm/configs/keystone_defconfig
+++ b/arch/arm/configs/keystone_defconfig
@@ -179,3 +179,6 @@ CONFIG_CRYPTO_DES=y
 CONFIG_CRYPTO_ANSI_CPRNG=y
 CONFIG_CRYPTO_USER_API_HASH=y
 CONFIG_CRYPTO_USER_API_SKCIPHER=y
+CONFIG_GPIOLIB=y
+CONFIG_GPIO_SYSFS=y
+CONFIG_GPIO_DAVINCI=y
-- 
1.7.9.5

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

^ permalink raw reply related

* [PATCH v3 4/4] ARM: keystone_defconfig: enable led support
From: Grygorii Strashko @ 2014-02-10 16:41 UTC (permalink / raw)
  To: santosh.shilimkar-l0cyMroinI0
  Cc: Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	ivan.khoronzhuk-l0cyMroinI0, Grygorii Strashko
In-Reply-To: <1392050481-19086-1-git-send-email-grygorii.strashko-l0cyMroinI0@public.gmane.org>

The Keystone GPIO functionality is ready for use, so LED support can be
enabled in config.

Signed-off-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
---
 arch/arm/configs/keystone_defconfig |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/configs/keystone_defconfig b/arch/arm/configs/keystone_defconfig
index f9467ad..7056ef4 100644
--- a/arch/arm/configs/keystone_defconfig
+++ b/arch/arm/configs/keystone_defconfig
@@ -182,3 +182,11 @@ CONFIG_CRYPTO_USER_API_SKCIPHER=y
 CONFIG_GPIOLIB=y
 CONFIG_GPIO_SYSFS=y
 CONFIG_GPIO_DAVINCI=y
+CONFIG_LEDS_CLASS=y
+CONFIG_NEW_LEDS=y
+CONFIG_LEDS_GPIO=y
+CONFIG_LEDS_TRIGGERS=y
+CONFIG_LEDS_TRIGGER_ONESHOT=y
+CONFIG_LEDS_TRIGGER_HEARTBEAT=y
+CONFIG_LEDS_TRIGGER_BACKLIGHT=y
+CONFIG_LEDS_TRIGGER_GPIO=y
-- 
1.7.9.5

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

^ permalink raw reply related

* Re: [PATCH v6 05/19] watchdog: orion: Make sure the watchdog is initially stopped
From: linux-0h96xk9xTtrk1uMJSBkQmQ @ 2014-02-10 16:48 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: Jason Gunthorpe, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-watchdog-u79uwXL29TY76Z2rM5mHXA, Wim Van Sebroeck,
	Jason Cooper, Thomas Petazzoni, Gregory Clement, Lior Amsalem,
	Sebastian Hesselbarth, Andrew Lunn
In-Reply-To: <20140210122223.GF10872@localhost>

Quoting Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:

> On Fri, Feb 07, 2014 at 10:43:14AM -0700, Jason Gunthorpe wrote:
>> On Fri, Feb 07, 2014 at 07:40:45AM -0300, Ezequiel Garcia wrote:
>>
>> > Well, this is related to the discussion about the bootloader not
>> > reseting the watchdog properly, provoking spurious watchdog triggering.
>> >
>> > Jason Gunthorpe explained [1] that we needed a particular sequence:
>> >
>> >  1. Disable WDT
>> >  2. Clear bridge
>> >  3. Enable WDT
>> >
>> > We added the irq handling to satisfy (2), and the watchdog stop for (1).
>>
>> The issue here is the driver configures two 'machine kill' elements:
>> the PANIC IRQ and the RstOut setup.
>>
>> Before configuring either of those the driver needs to ensure that any
>> old watchdog events are cleared out of the HW. We must not get a
>> spurious event.
>>
>> I agree not disabling an already functional and properly configured
>> counter from the bootloader is desirable.
>>
>> So lets break it down a bit..
>>
>> 1) The IRQ:
>>   It looks like the cause bit latches high on watchdog timer
>>   expiration but has no side effect unless it is unmasked.
>>
>>   The new IRQ flow code ensures the bit is cleared during request_irq
>>   so no old events can trigger the IRQ. Thus it is solved now.
>>
>
> Agreed.
>
>> 3) The timer itself:
>>   The WDT is just a general timer with an optional hookup to the
>>   rst control. If it is harmlessly counting but not resetting we need
>>   to stop that before enabling rst out.
>>
>
> Actually, the current flow is to:
>
> 1. Disable rst out and then disable the counter, in probe().
>
> 2. Enable the counter, and then enable rst out, in start().
>
>> So, how about this for psuedo-code in probe:
>>
>> if (readl(RSTOUTn) & WDRstOutEn)
>> {
>>     /* Watchdog is configured and may be down counting,
>>        don't touch it */
>>     request_irq(..);
>> }
>> else
>> {
>>     /* Watchdog is not configured, fully disable the timer
>>        and configure for watchdog operation. */
>>     disable_watchdog();
>>     request_irq();
>>     writel(RSTOUTn), .. WDRstOutEn);
>> }
>>
>
> Sounds good, although it seems to me it's actually simpler:
>
>   /* Let's make sure the watchdog is fully stopped, unless
>    * it's explicitly enabled and running
>    */
>   if ( !(wdt_rst_out_en && wdt_timer_enabled) ) {
>     watchdog_stop();
>   }
>

     if (!wdt_rst_out_en || !wdt_timer_enabled)
         watchdog_stop();

seems to be a bit easier to understand.

Also watch out for coding style issues. Above code
snippet has multiple violations ;-).

Guenter

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

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox