Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] serial: imx: Fix DCD reading
From: Fabio Estevam @ 2016-09-26 15:08 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAOMZO5B0Dkx8NTJPrNwR4WE1JJQEY1vczV=d1OtROzzDOGfQAQ@mail.gmail.com>

On Mon, Sep 26, 2016 at 11:28 AM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Uwe,
>
> On Mon, Sep 26, 2016 at 11:25 AM, Uwe Kleine-K?nig
> <u.kleine-koenig@pengutronix.de> wrote:
>
>>> Great, so it affects 4.5+ kernels and should probably have:
>>>
>>> Cc: <stable@vger.kernel.org> # 4.5+
>>
>> This is usually not necessary if there is a Fixes: line.
>
> I thought Greg prefers the explicit Cc: stable line as documented in
> Documentation/stable_kernel_rules.txt.
>
> Greg?

Ok, in this previous discussion Greg stated that he prefers Cc: stable
to be present:
http://www.spinics.net/lists/linux-gpio/msg10659.html

^ permalink raw reply

* [PATCH] ARM: dts: gose: use generic pinctrl properties in SDHI nodes
From: Simon Horman @ 2016-09-26 14:55 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1474020294-1544-1-git-send-email-horms+renesas@verge.net.au>

On Fri, Sep 16, 2016 at 12:04:54PM +0200, Simon Horman wrote:
> Since 16ccaf5bb5a5 ("pinctrl: sh-pfc: Accept standard function, pins and
> groups properties") renesas pfc drivers accept generic "function", "pins"
> and "groups" properties.
> 
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

I have queued this up.

^ permalink raw reply

* [PATCH] ARM: shmobile: Sort Kconfig selections
From: Simon Horman @ 2016-09-26 14:48 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1474619320-28888-1-git-send-email-geert+renesas@glider.be>

On Fri, Sep 23, 2016 at 10:28:40AM +0200, Geert Uytterhoeven wrote:
> Sort alphabetically all symbols selected by ARCH_RENESAS
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks, I have queued this up.

^ permalink raw reply

* [PATCH v4 2/5] ARM: dts: imx6q: Add Engicam i.CoreM6 Quad/Dual initial support
From: Jagan Teki @ 2016-09-26 14:28 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAOf5uwk3FJ4K4gL8xNNAp-z_AesjwTwSz986gfveVJsOU+N8Ow@mail.gmail.com>

Hi Fabio,

On Mon, Sep 26, 2016 at 7:26 PM, Michael Trimarchi
<michael@amarulasolutions.com> wrote:
> Hi
>
> On Mon, Sep 26, 2016 at 3:55 PM, Fabio Estevam <festevam@gmail.com> wrote:
>> On Mon, Sep 26, 2016 at 10:54 AM, Michael Trimarchi
>> <michael@amarulasolutions.com> wrote:
>>
>>>> Why don't you simply do like this?
>>>>
>>>>        reg_3p3v: regulator-3p3v {
>>>>                compatible = "regulator-fixed";
>>>>                regulator-name = "3P3V";
>>>>                regulator-min-microvolt = <3300000>;
>>>>                regulator-max-microvolt = <3300000>;
>>>>        };
>>>
>>> Should dts rispect the schematic connection?
>>
>> Yes
>
> So boot on and always on are used to declare their status and their
> behavionr. I don't
> see the point to not specify them in this way

Please try to understand that due to the case where we used fixed
voltage regulators on SOM as mentioned By Metteo and Michael dt nodes
should have regulator-always-on and regulator-boot-on properties. And
even we added one of our module which is already in ML[1]

Hope this is helpful conclude this thread.

[1] http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=a58e4e608bc88735581fdd9861676e522fa3c20d

thanks!
-- 
Jagan Teki
Free Software Engineer | Amarula Solutions
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

^ permalink raw reply

* [PATCH] serial: imx: Fix DCD reading
From: Fabio Estevam @ 2016-09-26 14:28 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20160926142506.ix53ekhykp2svodu@pengutronix.de>

Hi Uwe,

On Mon, Sep 26, 2016 at 11:25 AM, Uwe Kleine-K?nig
<u.kleine-koenig@pengutronix.de> wrote:

>> Great, so it affects 4.5+ kernels and should probably have:
>>
>> Cc: <stable@vger.kernel.org> # 4.5+
>
> This is usually not necessary if there is a Fixes: line.

I thought Greg prefers the explicit Cc: stable line as documented in
Documentation/stable_kernel_rules.txt.

Greg?

^ permalink raw reply

* [PATCH v2] ARM/dt: Respect property size when parsing CPUs
From: Robin Murphy @ 2016-09-26 14:25 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <31c4684aa8ef7ef210cc74da5f1b02ff2882a099.1474645829.git.robin.murphy@arm.com>

Whilst MPIDR values themselves are less than 32 bits, it is still
perfectly valid for a DT to have #address-cells > 1 in the CPUs node,
resulting in the "reg" property having leading zero cell(s). In that
situation, the big-endian nature of the data conspires with the current
behaviour of only reading the first cell to cause the kernel to think
all CPUs have ID 0, and become resoundingly unhappy as a consequence.

Take the full property length into account when parsing CPUs so as to
be correct under any circumstances.

CC: Russell King <linux@armlinux.org.uk>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v2: Check that any and all upper cells are zero rather than just
    ignoring them.

I think if we want to properly validate the DT as per Lorenzo's old
patch, that can follow on afterwards; here I'm really just concerned
with making sure the existing behaviour is actually robust and correct.

 arch/arm/kernel/devtree.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
index 40ecd5f514a2..f676febbb270 100644
--- a/arch/arm/kernel/devtree.c
+++ b/arch/arm/kernel/devtree.c
@@ -88,6 +88,8 @@ void __init arm_dt_init_cpu_maps(void)
 		return;
 
 	for_each_child_of_node(cpus, cpu) {
+		const __be32 *cell;
+		int prop_bytes;
 		u32 hwid;
 
 		if (of_node_cmp(cpu->type, "cpu"))
@@ -99,7 +101,8 @@ void __init arm_dt_init_cpu_maps(void)
 		 * properties is considered invalid to build the
 		 * cpu_logical_map.
 		 */
-		if (of_property_read_u32(cpu, "reg", &hwid)) {
+		cell = of_get_property(cpu, "reg", &prop_bytes);
+		if (!cell || prop_bytes < sizeof(*cell)) {
 			pr_debug(" * %s missing reg property\n",
 				     cpu->full_name);
 			of_node_put(cpu);
@@ -107,10 +110,15 @@ void __init arm_dt_init_cpu_maps(void)
 		}
 
 		/*
-		 * 8 MSBs must be set to 0 in the DT since the reg property
+		 * Bits n:24 must be set to 0 in the DT since the reg property
 		 * defines the MPIDR[23:0].
 		 */
-		if (hwid & ~MPIDR_HWID_BITMASK) {
+		do {
+			hwid = be32_to_cpu(*cell++);
+			prop_bytes -= sizeof(*cell);
+		} while (!hwid && prop_bytes > 0);
+
+		if (prop_bytes || (hwid & ~MPIDR_HWID_BITMASK)) {
 			of_node_put(cpu);
 			return;
 		}
-- 
2.8.1.dirty

^ permalink raw reply related

* [PATCH] serial: imx: Fix DCD reading
From: Uwe Kleine-König @ 2016-09-26 14:25 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAOMZO5Aw8pkt_tnM=M0SewN-Xjqq84E-mef-ewGs8aTv0xcHjQ@mail.gmail.com>

Hello Fabio,

On Mon, Sep 26, 2016 at 11:13:37AM -0300, Fabio Estevam wrote:
> On Mon, Sep 26, 2016 at 11:09 AM, Uwe Kleine-K?nig
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Mon, Sep 26, 2016 at 03:55:31PM +0200, Sascha Hauer wrote:
> >> The USR2_DCDIN bit is tested for in register usr1. As the name
> >> suggests the usr2 register should be used instead. This fixes
> >> reading the Carrier detect status.
> >>
> >> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > Fixes: 90ebc4838666 ("serial: imx: repair and complete handshaking")
> > Acked-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> 
> Great, so it affects 4.5+ kernels and should probably have:
> 
> Cc: <stable@vger.kernel.org> # 4.5+

This is usually not necessary if there is a Fixes: line.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply

* [PATCH] serial: imx: Fix DCD reading
From: Fabio Estevam @ 2016-09-26 14:13 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20160926140938.ikjatvprk3t4lsed@pengutronix.de>

On Mon, Sep 26, 2016 at 11:09 AM, Uwe Kleine-K?nig
<u.kleine-koenig@pengutronix.de> wrote:
> Hallo,
>
> On Mon, Sep 26, 2016 at 03:55:31PM +0200, Sascha Hauer wrote:
>> The USR2_DCDIN bit is tested for in register usr1. As the name
>> suggests the usr2 register should be used instead. This fixes
>> reading the Carrier detect status.
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Fixes: 90ebc4838666 ("serial: imx: repair and complete handshaking")
> Acked-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>

Great, so it affects 4.5+ kernels and should probably have:

Cc: <stable@vger.kernel.org> # 4.5+

^ permalink raw reply

* BUG: serial: imx: imprecise data abort
From: Holger Schurig @ 2016-09-26 14:12 UTC (permalink / raw)
  To: linux-arm-kernel

Hi all,

on an i.MX6Q we had a situation where we got "Imprecise Data Aborts".
The backtraces of those aborts were useless, all over the place.

But we found out that we can trigger this bug with this procedure:

- make some external PC send constantly through the serial port
  to the i.MX6Q.
- run a serial program on the i.MX6Q that receives the data and
  echos it back
- let this program terminate and restart every over second (we
  used a 4 second interval)

Chances were good that we reproduced the issue with various kernels
(up to 4.7.2).


In the drivers/tty/serial/tty/imx.c I disabled all DMA, because this was
my first suspicion. But to no avail. Eventually we asked some company
to help us. They produced the following patch. With this patch, we can
now run for a long time without any imprecise data abort (actually we
run into another issue, but according to
https://lkml.org/lkml/2016/5/16/452 "tty crash in Linux 4.6" this is
already in the working).

It's entirely clear to me that below WIP-patch has ZERO chance of being
added. It's not just checkpatch that will barf over it. :-)

My goal is to make the more knowledgeable people aware of the issue and
to give them a pointer, so that they can tell me how to fix the issue
in a correct way.


Holger




--- linux-4.6.orig/drivers/tty/serial/imx.c
+++ linux-4.6/drivers/tty/serial/imx.c
@@ -234,6 +234,9 @@
 	unsigned int	ucr3;
 };
 
+static unsigned int DBG_Starttx = 0;
+atomic_t imx_uart_is_in_irq = ATOMIC_INIT(0);
+
 static struct imx_uart_data imx_uart_devdata[] = {
 	[IMX1_UART] = {
 		.uts_reg = IMX1_UTS,
@@ -386,8 +389,8 @@
 		}
 	}
 
-	temp = readl(sport->port.membase + UCR2);
-	writel(temp & ~UCR2_RXEN, sport->port.membase + UCR2);
+	//temp = readl(sport->port.membase + UCR2);
+	//writel(temp & ~UCR2_RXEN, sport->port.membase + UCR2);
 
 	/* disable the `Receiver Ready Interrrupt` */
 	temp = readl(sport->port.membase + UCR1);
@@ -577,6 +580,7 @@
 	}
 
 	if (!sport->dma_is_enabled) {
+		//DBG_Starttx++;
 		temp = readl(sport->port.membase + UCR1);
 		writel(temp | UCR1_TXMPTYEN, sport->port.membase + UCR1);
 	}
 	spin_lock_irqsave(&sport->port.lock, flags);
 
 	while (readl(sport->port.membase + USR2) & USR2_RDR) {
+		//skip if not enabled 
+		if(((readl(sport->port.membase + UCR2) & UCR2_RXEN) ==0 )
+		   || ((readl(sport->port.membase + UCR1) & UCR1_UARTEN) ==0 ))
+			goto out;
+		
 		flg = TTY_NORMAL;
 		sport->port.icount.rx++;
+
+		
 
 		rx = readl(sport->port.membase + URXD0);
 
@@ -735,6 +746,7 @@
 	unsigned int sts;
 	unsigned int sts2;
 
+	atomic_add(1,&imx_uart_is_in_irq);
 	sts = readl(sport->port.membase + USR1);
 	sts2 = readl(sport->port.membase + USR2);
 
@@ -761,7 +773,7 @@
 		sport->port.icount.overrun++;
 		writel(USR2_ORE, sport->port.membase + USR2);
 	}
-
+	atomic_sub(1,&imx_uart_is_in_irq);
 	return IRQ_HANDLED;
 }
 
@@ -896,6 +908,7 @@
 	struct imx_port *sport = (struct imx_port *)data;
 	unsigned long flags;
 
+	atomic_add(1,&imx_uart_is_in_irq);
 	if (sport->port.state) {
 		spin_lock_irqsave(&sport->port.lock, flags);
 		imx_mctrl_check(sport);
@@ -903,6 +916,7 @@
 
 		mod_timer(&sport->timer, jiffies + MCTRL_TIMEOUT);
 	}
+	atomic_sub(1,&imx_uart_is_in_irq);
 }
 
 #define RX_BUF_SIZE	(PAGE_SIZE)
@@ -1251,7 +1267,7 @@
 		}
 		spin_lock_irqsave(&sport->port.lock, flags);
 		imx_stop_tx(port);
-		imx_stop_rx(port);
+//		imx_stop_rx(port);
 		imx_disable_dma(sport);
 		spin_unlock_irqrestore(&sport->port.lock, flags);
 		imx_uart_dma_exit(sport);
@@ -1261,7 +1277,7 @@
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 	temp = readl(sport->port.membase + UCR2);
-	temp &= ~(UCR2_TXEN);
+	//temp &= ~(UCR2_TXEN);
 	writel(temp, sport->port.membase + UCR2);
 	spin_unlock_irqrestore(&sport->port.lock, flags);
 
@@ -1276,13 +1292,16 @@
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 	temp = readl(sport->port.membase + UCR1);
-	temp &= ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_UARTEN);
+	//temp &= ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_UARTEN);
+	temp &= ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN);
 
 	writel(temp, sport->port.membase + UCR1);
 	spin_unlock_irqrestore(&sport->port.lock, flags);
 
-	clk_disable_unprepare(sport->clk_per);
-	clk_disable_unprepare(sport->clk_ipg);
+	while(atomic_read(&imx_uart_is_in_irq) == 1);
+
+	//clk_disable_unprepare(sport->clk_per);
+	//clk_disable_unprepare(sport->clk_ipg);
 }
 
 static void imx_flush_buffer(struct uart_port *port)
@@ -1732,8 +1750,8 @@
 	if (locked)
 		spin_unlock_irqrestore(&sport->port.lock, flags);
 
-	clk_disable(sport->clk_ipg);
-	clk_disable(sport->clk_per);
+	//clk_disable(sport->clk_ipg);
+	//clk_disable(sport->clk_per);
 }
 
 /*
@@ -1834,7 +1852,7 @@
 
 	retval = uart_set_options(&sport->port, co, baud, parity, bits, flow);
 
-	clk_disable(sport->clk_ipg);
+	/*clk_disable(sport->clk_ipg);
 	if (retval) {
 		clk_unprepare(sport->clk_ipg);
 		goto error_console;
@@ -1843,7 +1861,7 @@
 	retval = clk_prepare(sport->clk_per);
 	if (retval)
 		clk_disable_unprepare(sport->clk_ipg);
-
+	*/
 error_console:
 	return retval;
 }
@@ -2034,7 +2052,7 @@
 		 UCR1_TXMPTYEN | UCR1_RTSDEN);
 	writel_relaxed(reg, sport->port.membase + UCR1);
 
-	clk_disable_unprepare(sport->clk_ipg);
+	//clk_disable_unprepare(sport->clk_ipg);
 
 	/*
 	 * Allocate the IRQ(s) i.MX1 has three interrupts whereas later
@@ -2136,7 +2154,7 @@
 
 	serial_imx_save_context(sport);
 
-	clk_disable(sport->clk_ipg);
+	//clk_disable(sport->clk_ipg);
 
 	return 0;
 }
@@ -2153,7 +2171,7 @@
 
 	serial_imx_restore_context(sport);
 
-	clk_disable(sport->clk_ipg);
+	//clk_disable(sport->clk_ipg);
 
 	return 0;
 }

^ permalink raw reply

* [PATCH] serial: imx: Fix DCD reading
From: Uwe Kleine-König @ 2016-09-26 14:09 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20160926135531.31888-1-s.hauer@pengutronix.de>

Hallo,

On Mon, Sep 26, 2016 at 03:55:31PM +0200, Sascha Hauer wrote:
> The USR2_DCDIN bit is tested for in register usr1. As the name
> suggests the usr2 register should be used instead. This fixes
> reading the Carrier detect status.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Fixes: 90ebc4838666 ("serial: imx: repair and complete handshaking")
Acked-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply

* [PATCH] serial: imx: Fix DCD reading
From: Fabio Estevam @ 2016-09-26 14:08 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20160926135531.31888-1-s.hauer@pengutronix.de>

Hi Sascha,

On Mon, Sep 26, 2016 at 10:55 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> The USR2_DCDIN bit is tested for in register usr1. As the name
> suggests the usr2 register should be used instead. This fixes
> reading the Carrier detect status.
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Good catch:

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>

Shouldn't this have stable on Cc?

^ permalink raw reply

* [PATCH 2/3] arm64: hw_breakpoint: Handle inexact watchpoint addresses
From: Will Deacon @ 2016-09-26 14:06 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1474643941-109020-2-git-send-email-labath@google.com>

Hi Pavel,

On Fri, Sep 23, 2016 at 04:19:00PM +0100, Pavel Labath wrote:
> Arm64 hardware does not always report a watchpoint hit address that
> matches one of the watchpoints set. It can also report an address
> "near" the watchpoint if a single instruction access both watched and
> unwatched addresses. There is no straight-forward way, short of
> disassembling the offending instruction, to map that address back to
> the watchpoint.
> 
> Previously, when the hardware reported a watchpoint hit on an address
> that did not match our watchpoint (this happens in case of instructions
> which access large chunks of memory such as "stp") the process would
> enter a loop where we would be continually resuming it (because we did
> not recognise that watchpoint hit) and it would keep hitting the
> watchpoint again and again. The tracing process would never get
> notified of the watchpoint hit.
> 
> This commit fixes the problem by looking at the watchpoints near the
> address reported by the hardware. If the address does not exactly match
> one of the watchpoints we have set, it attributes the hit to the
> nearest watchpoint we have.  This heuristic is a bit dodgy, but I don't
> think we can do much more, given the hardware limitations.
> 
> Signed-off-by: Pavel Labath <labath@google.com>
> ---
>  arch/arm64/kernel/hw_breakpoint.c | 98 +++++++++++++++++++++++++--------------
>  1 file changed, 64 insertions(+), 34 deletions(-)

If the first patch in the series is no longer required (as you stated in
your follow-up reply), then you can just drop it.

> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> index 14562ae..3ce27ea 100644
> --- a/arch/arm64/kernel/hw_breakpoint.c
> +++ b/arch/arm64/kernel/hw_breakpoint.c
> @@ -664,49 +664,63 @@ unlock:
>  }
>  NOKPROBE_SYMBOL(breakpoint_handler);
>  
> +/*
> + * Arm64 hardware does not always report a watchpoint hit address that matches
> + * one of the watchpoints set. It can also report an address "near" the
> + * watchpoint if a single instruction access both watched and unwatched
> + * addresses. There is no straight-forward way, short of disassembling the
> + * offending instruction, to map that address back to the watchpoint. This
> + * function computes the distance of the memory access from the watchpoint as a
> + * heuristic for the likelyhood that a given access triggered the watchpoint.
> + *
> + * See Section D2.10.5 "Determining the memory location that caused a Watchpoint
> + * exception" of ARMv8 Architecture Reference Manual for details.
> + *
> + * The function returns the distance of the address from the bytes watched by
> + * the watchpoint. In case of an exact match, it returns 0.
> + */
> +static u64 get_distance_from_watchpoint(unsigned long addr, int i,
> +					struct arch_hw_breakpoint *info)
> +{
> +	u64 wp_low, wp_high;
> +	int first_bit;
> +
> +	first_bit = ffs(info->ctrl.len);
> +	if (first_bit == 0)
> +		return -1;
> +
> +	wp_low = info->address + first_bit - 1;
> +	wp_high = info->address + fls(info->ctrl.len) - 1;

This would all be cleaner if you just called get_hbp_len(info->ctrl.len)
to get the size of the watchpoint. We don't do anything sophisticated
with the BAS, so you can assume everything is base + len.

> @@ -723,10 +748,15 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
>  		/* Do we need to handle the stepping? */
>  		if (is_default_overflow_handler(wp))
>  			step = 1;
> -
> -unlock:
> -		rcu_read_unlock();
>  	}
> +	if (min_dist > 0 && min_dist != -1) {

min_dist is unsigned, so this could be:

	if (min_dist + 1 > 1)

Will

^ permalink raw reply

* [PATCH v5] devicetree: bindings: uart: Add new compatible string for ZynqMP
From: Nava kishore Manne @ 2016-09-26 14:06 UTC (permalink / raw)
  To: linux-arm-kernel

From: Nava kishore Manne <nava.manne@xilinx.com>

This patch Adds the new compatible string for ZynqMP SoC.

Signed-off-by: Nava kishore Manne <navam@xilinx.com>
---
Changes for v5:
		-Fixed some minor comments.
Changes for v4:
                -Modified the ChangeLog comment.
Changes for v3:
                -Added changeLog comment.
Changes for v2:
                -None

 Documentation/devicetree/bindings/serial/cdns,uart.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/serial/cdns,uart.txt b/Documentation/devicetree/bindings/serial/cdns,uart.txt
index a3eb154..30c885c 100644
--- a/Documentation/devicetree/bindings/serial/cdns,uart.txt
+++ b/Documentation/devicetree/bindings/serial/cdns,uart.txt
@@ -1,7 +1,9 @@
 Binding for Cadence UART Controller
 
 Required properties:
-- compatible : should be "cdns,uart-r1p8", or "xlnx,xuartps"
+- compatible :
+  Use "cdns,uart-r1p8", or "xlnx,xuartps" for Zynq-7xxx SoC.
+  Use "cdns,uart-r1p12" for Zynq Ultrascale+ MPSoC.
 - reg: Should contain UART controller registers location and length.
 - interrupts: Should contain UART controller interrupts.
 - clocks: Must contain phandles to the UART clocks
-- 
2.1.2

^ permalink raw reply related

* [PATCH v4 2/5] ARM: dts: imx6q: Add Engicam i.CoreM6 Quad/Dual initial support
From: Michael Trimarchi @ 2016-09-26 13:56 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAOMZO5BHhA8rd0URLNGGLZKN00zFz9d5vsq4CuRWsKyT6yq-oA@mail.gmail.com>

Hi

On Mon, Sep 26, 2016 at 3:55 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Mon, Sep 26, 2016 at 10:54 AM, Michael Trimarchi
> <michael@amarulasolutions.com> wrote:
>
>>> Why don't you simply do like this?
>>>
>>>        reg_3p3v: regulator-3p3v {
>>>                compatible = "regulator-fixed";
>>>                regulator-name = "3P3V";
>>>                regulator-min-microvolt = <3300000>;
>>>                regulator-max-microvolt = <3300000>;
>>>        };
>>
>> Should dts rispect the schematic connection?
>
> Yes

So boot on and always on are used to declare their status and their
behavionr. I don't
see the point to not specify them in this way

Michael


-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

^ permalink raw reply

* [PATCH] serial: imx: Fix DCD reading
From: Sascha Hauer @ 2016-09-26 13:55 UTC (permalink / raw)
  To: linux-arm-kernel

The USR2_DCDIN bit is tested for in register usr1. As the name
suggests the usr2 register should be used instead. This fixes
reading the Carrier detect status.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/tty/serial/imx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 0df2b1c..615c027 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -740,12 +740,13 @@ static unsigned int imx_get_hwmctrl(struct imx_port *sport)
 {
 	unsigned int tmp = TIOCM_DSR;
 	unsigned usr1 = readl(sport->port.membase + USR1);
+	unsigned usr2 = readl(sport->port.membase + USR2);
 
 	if (usr1 & USR1_RTSS)
 		tmp |= TIOCM_CTS;
 
 	/* in DCE mode DCDIN is always 0 */
-	if (!(usr1 & USR2_DCDIN))
+	if (!(usr2 & USR2_DCDIN))
 		tmp |= TIOCM_CAR;
 
 	if (sport->dte_mode)
-- 
2.9.3

^ permalink raw reply related

* [PATCH v4 2/5] ARM: dts: imx6q: Add Engicam i.CoreM6 Quad/Dual initial support
From: Fabio Estevam @ 2016-09-26 13:55 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAOf5uw=HwErWWqvQ30ZJpb3CupqdvGZ9FnGAxh4+JVTrT6wTjw@mail.gmail.com>

On Mon, Sep 26, 2016 at 10:54 AM, Michael Trimarchi
<michael@amarulasolutions.com> wrote:

>> Why don't you simply do like this?
>>
>>        reg_3p3v: regulator-3p3v {
>>                compatible = "regulator-fixed";
>>                regulator-name = "3P3V";
>>                regulator-min-microvolt = <3300000>;
>>                regulator-max-microvolt = <3300000>;
>>        };
>
> Should dts rispect the schematic connection?

Yes

^ permalink raw reply

* [PATCH v4 2/5] ARM: dts: imx6q: Add Engicam i.CoreM6 Quad/Dual initial support
From: Michael Trimarchi @ 2016-09-26 13:54 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAOMZO5BWgqxfbOepJZwqdZceSWKBnSfd38Ab2TV-LN0oDttc-Q@mail.gmail.com>

Hi

On Mon, Sep 26, 2016 at 3:47 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Matteo,
>
> On Mon, Sep 26, 2016 at 10:44 AM, Matteo Lisi <matteo.lisi@engicam.com> wrote:
>> Hi Fabio,
>>
>> Our SOM doesn't use external PMIC.
>>
>> We powered all devices with a fix voltage regulators that cannot be drive
>> directly from SoC so
>> any external power rail can be driven by drivers.
>
> Sure, this is fine.
>
> Why don't you simply do like this?
>
>        reg_3p3v: regulator-3p3v {
>                compatible = "regulator-fixed";
>                regulator-name = "3P3V";
>                regulator-min-microvolt = <3300000>;
>                regulator-max-microvolt = <3300000>;
>        };

Should dts rispect the schematic connection?


Michael


-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

^ permalink raw reply

* [PATCH v4 2/5] ARM: dts: imx6q: Add Engicam i.CoreM6 Quad/Dual initial support
From: Fabio Estevam @ 2016-09-26 13:47 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <119ad2fc-8e74-1193-8ee2-417cf1e6fd49@engicam.com>

Hi Matteo,

On Mon, Sep 26, 2016 at 10:44 AM, Matteo Lisi <matteo.lisi@engicam.com> wrote:
> Hi Fabio,
>
> Our SOM doesn't use external PMIC.
>
> We powered all devices with a fix voltage regulators that cannot be drive
> directly from SoC so
> any external power rail can be driven by drivers.

Sure, this is fine.

Why don't you simply do like this?

       reg_3p3v: regulator-3p3v {
               compatible = "regulator-fixed";
               regulator-name = "3P3V";
               regulator-min-microvolt = <3300000>;
               regulator-max-microvolt = <3300000>;
       };

^ permalink raw reply

* [PATCH v4 2/5] ARM: dts: imx6q: Add Engicam i.CoreM6 Quad/Dual initial support
From: Matteo Lisi @ 2016-09-26 13:44 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAOMZO5CnP3=CNnQcOzra7eQ-cAwProgH+Kws0FHVOw7Q6nCDow@mail.gmail.com>

Hi Fabio,

Our SOM doesn't use external PMIC.

We powered all devices with a fix voltage regulators that cannot be 
drive directly from SoC so
any external power rail can be driven by drivers.

Regards

Matteo



Il 21/09/2016 18:46, Fabio Estevam ha scritto:
> Hi Michael,
>
> On Tue, Sep 20, 2016 at 10:23 AM, Michael Trimarchi
> <michael@amarulasolutions.com> wrote:
>
>> Engicam use fixed regulator always on and on on boot. Their board does
>> not have any
>> external pmu. Is this answer to your comment?
> All I am saying is that  "regulator-boot-on" and "regulator-always-on"
> properties should be removed.
>
> The "reg_3p3v" regulator will be turned on and off by the flexcan driver.
>


-- 

------------------------------------------------------------------------
http://www.engicam.com <http://www.engicam.com>

*/ENGICAM /*s.r.l.
50018 Scandicci - FIRENZE
Via dei Pratoni, 16
Tel. +39 055 7311387
Fax. +39 055 720608
Web www.engicam.com <http://www.engicam.com>

C.F./P.I. 05389070482
Registro Imprese di FIRENZE 542918
Capitale sociale i.v. 50.000,00?
------------------------------------------------------------------------
NOTICE: This message and attachments are intended only for the use of 
their addresses and may contain confidential information belonging to 
Engicam. If you are not the intended recipient, you are hereby notified 
that any reading, dissemination, distribution, or copying of this 
message, or any attachment, is strictly prohibited. If you have received 
this message in error, please notify the original sender immediately and 
delete this message, along with any attachments.

^ permalink raw reply

* [PATCH] serial: stm32: use mapbase instead of membase for DMA
From: Gerald Baeza @ 2016-09-26 13:21 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20160923193935.3726427-1-arnd@arndb.de>

Hi Arnd

On 09/23/2016 09:38 PM, Arnd Bergmann wrote:
> Building this driver with a 64-bit dma_addr_t type results in
> a compiler warning:
>
> drivers/tty/serial/stm32-usart.c: In function 'stm32_of_dma_rx_probe':
> drivers/tty/serial/stm32-usart.c:746:20: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
> drivers/tty/serial/stm32-usart.c: In function 'stm32_of_dma_tx_probe':
> drivers/tty/serial/stm32-usart.c:818:20: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>
> While the type conversion here is harmless, this hints at a different
> problem: we pass an __iomem pointer into a DMA engine, which expects
> a phys_addr_t. This happens to work because stm32 has no MMU and
> ioremap() is an identity mapping here, but it's still an incorrect
> API use. Using dma_addr_t is doubly wrong here, because that would
> be the result of dma_map_single() rather than the physical address.
>
> Using the mapbase instead fixes multiple issues:
>
> - the warning is gone
> - we don't go through ioremap in error
> - the cast is gone, making it use the correct resource_size_t/phys_addr_t
>   type in the process.
>
> Fixes: 3489187204eb ("serial: stm32: adding dma support")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/tty/serial/stm32-usart.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> index 4d3001b77e7e..2adb678a863b 100644
> --- a/drivers/tty/serial/stm32-usart.c
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -743,7 +743,7 @@ static int stm32_of_dma_rx_probe(struct stm32_port *stm32port,
>
>  	/* Configure DMA channel */
>  	memset(&config, 0, sizeof(config));
> -	config.src_addr = (dma_addr_t)port->membase + ofs->rdr;
> +	config.src_addr = port->mapbase + ofs->rdr;
>  	config.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>
>  	ret = dmaengine_slave_config(stm32port->rx_ch, &config);
> @@ -815,7 +815,7 @@ static int stm32_of_dma_tx_probe(struct stm32_port *stm32port,
>
>  	/* Configure DMA channel */
>  	memset(&config, 0, sizeof(config));
> -	config.dst_addr = (dma_addr_t)port->membase + ofs->tdr;
> +	config.dst_addr = port->mapbase + ofs->tdr;
>  	config.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>
>  	ret = dmaengine_slave_config(stm32port->tx_ch, &config);
>

Indeed, thanks for this finding.

Reviewed-by: Gerald Baeza <gerald.baeza@st.com>

^ permalink raw reply

* [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
From: Gabriele Paoloni @ 2016-09-26 13:21 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <10705430.q2aqv5uYO3@wuerfel>

Hi Arnd

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd at arndb.de]
> Sent: 23 September 2016 14:43
> To: Gabriele Paoloni
> Cc: zhichang.yuan; linux-arm-kernel at lists.infradead.org;
> devicetree at vger.kernel.org; lorenzo.pieralisi at arm.com; minyard at acm.org;
> linux-pci at vger.kernel.org; gregkh at linuxfoundation.org; John Garry;
> will.deacon at arm.com; linux-kernel at vger.kernel.org; Yuanzhichang;
> Linuxarm; xuwei (O); linux-serial at vger.kernel.org;
> benh at kernel.crashing.org; zourongrong at gmail.com; liviu.dudau at arm.com;
> kantyzc at 163.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Friday, September 23, 2016 10:23:30 AM CEST Gabriele Paoloni wrote:
> > Hi Arnd
> >
> > > -----Original Message-----
> > > From: Arnd Bergmann [mailto:arnd at arndb.de]
> > > Sent: 23 September 2016 10:52
> > > To: zhichang.yuan
> > > Cc: Gabriele Paoloni; linux-arm-kernel at lists.infradead.org;
> > > devicetree at vger.kernel.org; lorenzo.pieralisi at arm.com;
> minyard at acm.org;
> > > linux-pci at vger.kernel.org; gregkh at linuxfoundation.org; John Garry;
> > > will.deacon at arm.com; linux-kernel at vger.kernel.org; Yuanzhichang;
> > > Linuxarm; xuwei (O); linux-serial at vger.kernel.org;
> > > benh at kernel.crashing.org; zourongrong at gmail.com;
> liviu.dudau at arm.com;
> > > kantyzc at 163.com
> > > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> > > Hip06
> > >
> > > On Friday, September 23, 2016 12:27:17 AM CEST zhichang.yuan wrote:
> > > > For this patch sketch, I have a question.
> > > > Do we call pci_address_to_pio in arch_of_address_to_pio to get
> the
> > > > corresponding logical IO port
> > > > for LPC??
> > >
> > >
> > > No, of course not, that would be silly:
> > >
> > > The argument to pci_address_to_pio() is a phys_addr_t, and we we
> don't
> > > have one because there is no address associated with your PIO, that
> > > is the entire point of your driver!
> > >
> > > Also, we already know the mapping because this is what the inb/outb
> > > workaround is looking at, so there is absolutely no reason to call
> it
> > > either.
> >
> > Ok assume that we do not call pci_address_to_pio() for the ISA bus...
> > The LPC driver will register its phys address range in io_range_list,
> > then the IPMI driver probe will retrieve its physical address calling
> > of_address_to_resource and will use the indirect io to access this
> > address.
> >
> > From the perspective of the indirect IO function the input parameter
> > is an unsigned long addr that (now) can be either:
> > 1) an IO token coming from a legacy pci device
> > 2) a phys address that lives on the LPC bus
> >
> > These are conceptually two separate address spaces (and actually they
> > both start from 0).
> 
> Why? Any IORESOURCE_IO address always refers to the logical I/O port
> range in Linux, not the physical address that is used on a bus.
> 
> > If the input parameter can live on different address spaces that are
> > overlapped, even if I save the used LPC range in arm64_extio_ops-
> >start/end
> > there is no way for the indirect IO to tell if the input parameter is
> > an I/O token or a phys address that belongs to LPC...
>

Assume that in the probe function the LPC drivers calls pci_register_io_range
for the LPC cpu address range (0 to PCIBIOS_MIN_I0) and does not scan
the children DT nodes.

Consider for example the ipmi driver:
When the reg property is read to retrieve the ipmi <<i/o port>> in
http://lxr.free-electrons.com/source/drivers/char/ipmi/ipmi_si_intf.c#L2622
if we do not call pci_address_to_pio in __of_address_to_resource the input
parameter of inb/outb will be the cpu address of the ipmi (not translated
to a unique token id).

So inb/outb at this stage can be called passing either a cpu address or a
token io port.

If we set arm64_extio_ops->start/end to 0 and PCIBIOS_MIN_I0 respectively
we still cannot tell inside inb/outb if the passed address is a token or
an LPC cpu address as the ipmi cpu address can overlap with another device
I/O token...

My suggestion is to call pci_address_to_pio even for devices living on
the LPC bus; then in the LPC probe we set arm64_extio_ops->start/end to
the I/O tokens that correspond to the LPC cpu address range (in the LPC probe
function we call pci_address_to_pio after we have called pci_register_io_range);
finally in inb/outb we know that we can get only an I/O token as input
parameter and we check it against arm64_extio_ops->start/end to decide
whether to call the LPC accessors or readb/writeb...

> The start address is the offset: if you get an address between 'start'
> and 'end', you subtract the 'start' from it, and use that to call
> the registered driver function. That works because we can safely
> assume that the bus address range that the LPC driver registers starts
> zero.

Sorry I cannot follow what you said here above: <<if you get an address 
between 'start' and 'end'>>...in which function?

Thanks

Gab

> 
> 	Arnd

^ permalink raw reply

* [PATCH 5/5] arm64: Add uprobe support
From: Pratyush Anand @ 2016-09-26 13:03 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20160926110159.GB27498@e104818-lin.cambridge.arm.com>

On 26/09/2016:12:01:59 PM, Catalin Marinas wrote:
> On Sun, Sep 25, 2016 at 10:32:28PM +0530, Pratyush Anand wrote:
> > On Fri, Sep 23, 2016 at 6:35 PM, Catalin Marinas
> > <catalin.marinas@arm.com> wrote:
> > > On Fri, Sep 23, 2016 at 09:42:30AM +0530, Pratyush Anand wrote:
> > >> On 22/09/2016:05:50:30 PM, Catalin Marinas wrote:
> > >> > On Thu, Sep 22, 2016 at 08:53:28AM +0530, Pratyush Anand wrote:
> > >> > > On 21/09/2016:06:04:04 PM, Catalin Marinas wrote:
> > 
> > >> > As a quick workaround you could check mm->task_size > TASK_SIZE_32 in
> > >> > the arch_uprobe_analyze_insn() function.
> > >>
> > >> It would be doable. TASK_SIZE_32 is defined only for COMPAT. So, may be I can
> > >> return -EINVAL when mm->task_size < TASK_SIZE_64.
> > >
> > > That's just a temporary workaround. If we ever merge ILP32, this test
> > > would no longer be enough (as the ISA is AArch64 but with TASK_SIZE_32).
> > 
> > OK.. So what about doing something similar what x86 is doing.
> > We can have a flag for task Type in arch specific mm_context_t. We
> > also set this flag in COMPAT_SET_PERSONALITY() along with setting
> > thread_info flag, and we clear them in SET_PERSONALITY().
> 
> This looks like a better approach.
> 
> > > Looking at prepare_uprobe(), we have a weak is_trap_insn() function.
> > > This check is meaningless without knowing which instruction set we
> > > target. A false positive here, however, is not that bad as we wouldn't
> > > end up inserting the wrong breakpoint in the executable. But it looks to
> > > me like the core uprobe code needs to pass some additional information
> > > like the type of task or ELF format to the arch code to make a useful
> > > choice of breakpoint type.
> > 
> > It seems that 'strtle r0, [r0], #160' would have the closest matching
> > aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too
> > seems a bad instruction. So, may be we can use still weak
> > is_trap_insn().
> 
> Even if the is_trap_insn() check passes, we would reject the probe in
> arch_uprobe_analyze_insn() immediately after based on the mm type check,
> so not too bad.

OK..I will have an always returning false from arm64 is_trap_insn() in v2.

> 
> If we add support for probing 32-bit tasks, I would rather have
> is_trap_insn() take the mm_struct as argument so that a non-weak
> implementation can check for the correct encoding.

Yes, for 32 bit task we would need mm_struct as arg in is_trap_insn() as well as
in is_swbp_insn(). We would also need to have arm64 specific set_swbp().

Thanks for all your input. It was helpful. I will send V2 soon.

~Pratyush

^ permalink raw reply

* [RFC PATCH 1/2] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
From: Guenter Roeck @ 2016-09-26 13:02 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1474850361-20884-2-git-send-email-vz@mleia.com>

On 09/25/2016 05:39 PM, Vladimir Zapolskiy wrote:
> Power down counter enable/disable bit switch is located in WMCR
> register, but watchdog controllers found on legacy i.MX21, i.MX27 and
> i.MX31 SoCs don't have this register. As a result of writing data to
> the non-existing register on driver probe the SoC hangs up, to fix the
> problem add more OF compatible strings and on this basis get
> information about availability of the WMCR register.
>
> Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
>  drivers/watchdog/imx2_wdt.c | 47 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 45 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index 62f346b..b6763e0 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -29,6 +29,7 @@
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
>  #include <linux/of_address.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/watchdog.h>
> @@ -57,6 +58,10 @@
>
>  #define WDOG_SEC_TO_COUNT(s)	((s * 2 - 1) << 8)
>
> +struct imx2_wdt_data {
> +	bool has_pdc;
> +};
> +
>  struct imx2_wdt_device {
>  	struct clk *clk;
>  	struct regmap *regmap;
> @@ -64,6 +69,8 @@ struct imx2_wdt_device {
>  	bool ext_reset;
>  };
>
> +static const struct of_device_id imx2_wdt_dt_ids[];
> +
>  static bool nowayout = WATCHDOG_NOWAYOUT;
>  module_param(nowayout, bool, 0);
>  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> @@ -200,10 +207,13 @@ static const struct regmap_config imx2_wdt_regmap_config = {
>
>  static int __init imx2_wdt_probe(struct platform_device *pdev)
>  {
> +	const struct of_device_id *of_id;
> +	const struct imx2_wdt_data *data;
>  	struct imx2_wdt_device *wdev;
>  	struct watchdog_device *wdog;
>  	struct resource *res;
>  	void __iomem *base;
> +	bool has_pdc;
>  	int ret;
>  	u32 val;
>
> @@ -261,12 +271,24 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
>  		set_bit(WDOG_HW_RUNNING, &wdog->status);
>  	}
>
> +	if (pdev->dev.of_node) {
> +		of_id = of_match_device(imx2_wdt_dt_ids, &pdev->dev);
> +		if (!of_id)
> +			return -ENODEV;
> +
> +		data = of_id->data;
> +		has_pdc = data->has_pdc;
> +	} else {
> +		has_pdc = false;
> +	}
> +
>  	/*
>  	 * Disable the watchdog power down counter at boot. Otherwise the power
>  	 * down counter will pull down the #WDOG interrupt line for one clock
>  	 * cycle.
>  	 */
> -	regmap_write(wdev->regmap, IMX2_WDT_WMCR, 0);
> +	if (has_pdc)
> +		regmap_write(wdev->regmap, IMX2_WDT_WMCR, 0);
>
>  	ret = watchdog_register_device(wdog);
>  	if (ret) {
> @@ -363,8 +385,29 @@ static int imx2_wdt_resume(struct device *dev)
>  static SIMPLE_DEV_PM_OPS(imx2_wdt_pm_ops, imx2_wdt_suspend,
>  			 imx2_wdt_resume);
>
> +static const struct imx2_wdt_data imx21_wdt_data = {
> +	.has_pdc = false,
> +};
> +
> +static const struct imx2_wdt_data imx25_wdt_data = {
> +	.has_pdc = true,
> +};
> +
>  static const struct of_device_id imx2_wdt_dt_ids[] = {
> -	{ .compatible = "fsl,imx21-wdt", },
> +	{ .compatible = "fsl,imx21-wdt",  .data = &imx21_wdt_data },

Please just specify the flag directly, as in
					.data = (void *) true
or, if you prefer, use defines.
					.data = (void *) IMX_SUPPORTS_PDC

Then, in above code:
		has_pdc = (bool) of_id->data;

Guenter

> +	{ .compatible = "fsl,imx25-wdt",  .data = &imx25_wdt_data },
> +	{ .compatible = "fsl,imx27-wdt",  .data = &imx21_wdt_data },
> +	{ .compatible = "fsl,imx31-wdt",  .data = &imx21_wdt_data },
> +	{ .compatible = "fsl,imx35-wdt",  .data = &imx25_wdt_data },
> +	{ .compatible = "fsl,imx50-wdt",  .data = &imx25_wdt_data },
> +	{ .compatible = "fsl,imx51-wdt",  .data = &imx25_wdt_data },
> +	{ .compatible = "fsl,imx53-wdt",  .data = &imx25_wdt_data },
> +	{ .compatible = "fsl,imx6q-wdt",  .data = &imx25_wdt_data },
> +	{ .compatible = "fsl,imx6sl-wdt", .data = &imx25_wdt_data },
> +	{ .compatible = "fsl,imx6sx-wdt", .data = &imx25_wdt_data },
> +	{ .compatible = "fsl,imx6ul-wdt", .data = &imx25_wdt_data },
> +	{ .compatible = "fsl,imx7d-wdt",  .data = &imx25_wdt_data },
> +	{ .compatible = "fsl,vf610-wdt",  .data = &imx25_wdt_data },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, imx2_wdt_dt_ids);
>

^ permalink raw reply

* [PATCH V5 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge
From: Archit Taneja @ 2016-09-26 12:54 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <5734-57e90c80-11-398829c0@7164733>



On 09/26/2016 05:24 PM, Peter Senna Tschudin wrote:
>
> On Monday, September 26, 2016 12:29 CEST, Archit Taneja <architt@codeaurora.org> wrote:
>
>> Hi,
>>
>> Some comments.
>
> Thank you for the review!
>
>>
>> On 08/09/2016 10:11 PM, Peter Senna Tschudin wrote:
>>> Add a driver that create a drm_bridge and a drm_connector for the LVDS
>>> to DP++ display bridge of the GE B850v3.
>>>
>>> There are two physical bridges on the video signal pipeline: a
>>> STDP4028(LVDS to DP) and a STDP2690(DP to DP++).  The hardware and
>>> firmware made it complicated for this binding to comprise two device
>>> tree nodes, as the design goal is to configure both bridges based on
>>> the LVDS signal, which leave the driver powerless to control the video
>>> processing pipeline. The two bridges behaves as a single bridge, and
>>> the driver is only needed for telling the host about EDID / HPD, and
>>> for giving the host powers to ack interrupts. The video signal pipeline
>>> is as follows:
>>>
>>>   Host -> LVDS|--(STDP4028)--|DP -> DP|--(STDP2690)--|DP++ -> Video output
>>>
>>> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
>>> Cc: Martin Donnelly <martin.donnelly@ge.com>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
>>> Cc: Rob Herring <robh@kernel.org>
>>> Cc: Fabio Estevam <fabio.estevam@nxp.com>
>>> CC: David Airlie <airlied@linux.ie>
>>> CC: Thierry Reding <treding@nvidia.com>
>>> CC: Thierry Reding <thierry.reding@gmail.com>
>>> Reviewed-by: Enric Balletbo <enric.balletbo@collabora.com>
>>> Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
>>> ---
>>> Changes from V4:
>>>  - Check the output of the first call to i2c_smbus_write_word_data() and return
>>>    it's error code for failing gracefully on i2c issues
>>>  - Renamed the i2c_driver.name from "ge,b850v3-lvds-dp" to "b850v3-lvds-dp" to
>>>    remove the comma from the driver name
>>>
>>> Changes from V3:
>>>  - 3/4 instead of 4/5
>>>  - Tested on next-20160804
>>>
>>> Changes from V2:
>>>  - Made it atomic to be applied on next-20160729 on top of Liu Ying changes
>>>    that made imx-ldb atomic
>>>
>>> Changes from V1:
>>>  - New commit message
>>>  - Removed 3 empty entry points
>>>  - Removed memory leak from ge_b850v3_lvds_dp_get_modes()
>>>  - Added a lock for mode setting
>>>  - Removed a few blank lines
>>>  - Changed the order at Makefile and Kconfig
>>>
>>>  MAINTAINERS                                |   8 +
>>>  drivers/gpu/drm/bridge/Kconfig             |  11 +
>>>  drivers/gpu/drm/bridge/Makefile            |   1 +
>>>  drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c | 405 +++++++++++++++++++++++++++++
>>>  4 files changed, 425 insertions(+)
>>>  create mode 100644 drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index a306795..e8d106a 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -5142,6 +5142,14 @@ W:	https://linuxtv.org
>>>  S:	Maintained
>>>  F:	drivers/media/radio/radio-gemtek*
>>>
>>> +GENERAL ELECTRIC B850V3 LVDS/DP++ BRIDGE
>>> +M:	Peter Senna Tschudin <peter.senna@collabora.com>
>>> +M:	Martin Donnelly <martin.donnelly@ge.com>
>>> +M:	Martyn Welch <martyn.welch@collabora.co.uk>
>>> +S:	Maintained
>>> +F:	drivers/gpu/drm/bridge/ge_b850v3_dp2.c
>>> +F:	Documentation/devicetree/bindings/ge/b850v3_dp2_bridge.txt
>>> +
>>>  GENERIC GPIO I2C DRIVER
>>>  M:	Haavard Skinnemoen <hskinnemoen@gmail.com>
>>>  S:	Supported
>>
>> Could you move the MAINTAINERS change to a different patch? It would
>> make it easier to integrate separately.
>
> If needed, yes sure.
>
>>
>>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>>> index b590e67..b4b70fb 100644
>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>> @@ -32,6 +32,17 @@ config DRM_DW_HDMI_AHB_AUDIO
>>>  	  Designware HDMI block.  This is used in conjunction with
>>>  	  the i.MX6 HDMI driver.
>>>
>>> +config DRM_GE_B850V3_LVDS_DP
>>> +	tristate "GE B850v3 LVDS to DP++ display bridge"
>>> +	depends on OF
>>> +	select DRM_KMS_HELPER
>>> +	select DRM_PANEL
>>> +	---help---
>>> +          This is a driver for the display bridge of
>>> +          GE B850v3 that convert dual channel LVDS
>>> +          to DP++. This is used with the i.MX6 imx-ldb
>>> +          driver.
>>> +
>>>  config DRM_NXP_PTN3460
>>>  	tristate "NXP PTN3460 DP/LVDS bridge"
>>>  	depends on OF
>>> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
>>> index efdb07e..b9606f3 100644
>>> --- a/drivers/gpu/drm/bridge/Makefile
>>> +++ b/drivers/gpu/drm/bridge/Makefile
>>> @@ -3,6 +3,7 @@ ccflags-y := -Iinclude/drm
>>>  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
>>>  obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o
>>>  obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o
>>> +obj-$(CONFIG_DRM_GE_B850V3_LVDS_DP) += ge_b850v3_lvds_dp.o
>>>  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
>>>  obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>>>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>>> diff --git a/drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c b/drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c
>>> new file mode 100644
>>> index 0000000..81e9279
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c
>>> @@ -0,0 +1,405 @@
>>> +/*
>>> + * Driver for GE B850v3 DP display bridge
>>> +
>>> + * Copyright (c) 2016, Collabora Ltd.
>>> + * Copyright (c) 2016, General Electric Company
>>> +
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms and conditions of the GNU General Public License,
>>> + * version 2, as published by the Free Software Foundation.
>>> +
>>> + * This program is distributed in the hope it will be useful, but WITHOUT
>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>>> + * more details.
>>> +
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> +
>>> + * This driver creates a drm_bridge and a drm_connector for the LVDS to DP++
>>> + * display bridge of the GE B850v3. There are two physical bridges on the video
>>> + * signal pipeline: a STDP4028(LVDS to DP) and a STDP2690(DP to DP++). However
>>> + * the physical bridges are automatically configured by the input video signal,
>>> + * and the driver has no access to the video processing pipeline. The driver is
>>> + * only needed to read EDID from the STDP2690 and to handle HPD events from the
>>> + * STDP4028. The driver communicates with both bridges over i2c. The video
>>> + * signal pipeline is as follows:
>>> + *
>>> + *   Host -> LVDS|--(STDP4028)--|DP -> DP|--(STDP2690)--|DP++ -> Video output
>>> + *
>>> + */
>>> +
>>> +#include <linux/gpio.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <drm/drm_atomic.h>
>>> +#include <drm/drm_atomic_helper.h>
>>> +#include <drm/drm_crtc_helper.h>
>>> +#include <drm/drm_edid.h>
>>> +#include <drm/drmP.h>
>>> +
>>> +/*
>>> + * 220Mhz is a limitation of the host, as the bridge is capable of up to
>>> + * 330Mhz. See section 9.2.1.2.4 of the i.MX 6Dual/6Quad Applications
>>> + * Processor Reference Manual for more information about the 220Mhz limit.
>>> + * The imx-ldb driver will warn about clocks over 170Mhz, but it seem to work
>>> + * fine.
>>> + */
>>> +#define MAX_PIXEL_CLOCK 220000
>>> +
>>> +#define EDID_EXT_BLOCK_CNT 0x7E
>>> +
>>> +#define STDP4028_IRQ_OUT_CONF_REG 0x02
>>> +#define STDP4028_DPTX_IRQ_EN_REG 0x3C
>>> +#define STDP4028_DPTX_IRQ_STS_REG 0x3D
>>> +#define STDP4028_DPTX_STS_REG 0x3E
>>> +
>>> +#define STDP4028_DPTX_DP_IRQ_EN 0x1000
>>> +
>>> +#define STDP4028_DPTX_HOTPLUG_IRQ_EN 0x0400
>>> +#define STDP4028_DPTX_LINK_CH_IRQ_EN 0x2000
>>> +#define STDP4028_DPTX_IRQ_CONFIG \
>>> +		(STDP4028_DPTX_LINK_CH_IRQ_EN | STDP4028_DPTX_HOTPLUG_IRQ_EN)
>>> +
>>> +#define STDP4028_DPTX_HOTPLUG_STS 0x0200
>>> +#define STDP4028_DPTX_LINK_STS 0x1000
>>> +#define STDP4028_CON_STATE_CONNECTED \
>>> +		(STDP4028_DPTX_HOTPLUG_STS | STDP4028_DPTX_LINK_STS)
>>> +
>>> +#define STDP4028_DPTX_HOTPLUG_CH_STS 0x0400
>>> +#define STDP4028_DPTX_LINK_CH_STS 0x2000
>>> +#define STDP4028_DPTX_IRQ_CLEAR \
>>> +		(STDP4028_DPTX_LINK_CH_STS | STDP4028_DPTX_HOTPLUG_CH_STS)
>>> +
>>> +struct ge_b850v3_lvds_dp {
>>> +	struct drm_connector connector;
>>> +	struct drm_bridge bridge;
>>> +	struct i2c_client *ge_b850v3_lvds_dp_i2c;
>>> +	struct i2c_client *edid_i2c;
>>> +	struct edid *edid;
>>> +	struct mutex lock;
>>> +};
>>> +
>>> +static inline struct ge_b850v3_lvds_dp *
>>> +		bridge_to_ge_b850v3_lvds_dp(struct drm_bridge *bridge)
>>> +{
>>> +	return container_of(bridge, struct ge_b850v3_lvds_dp, bridge);
>>> +}
>>> +
>>> +static inline struct ge_b850v3_lvds_dp *
>>> +		connector_to_ge_b850v3_lvds_dp(struct drm_connector *connector)
>>> +{
>>> +	return container_of(connector, struct ge_b850v3_lvds_dp, connector);
>>> +}
>>> +
>>> +u8 *stdp2690_get_edid(struct i2c_client *client)
>>> +{
>>> +	struct i2c_adapter *adapter = client->adapter;
>>> +	unsigned char start = 0x00;
>>> +	unsigned int total_size;
>>> +	u8 *block = kmalloc(EDID_LENGTH, GFP_KERNEL);
>>> +
>>> +	struct i2c_msg msgs[] = {
>>> +		{
>>> +			.addr	= client->addr,
>>> +			.flags	= 0,
>>> +			.len	= 1,
>>> +			.buf	= &start,
>>> +		}, {
>>> +			.addr	= client->addr,
>>> +			.flags	= I2C_M_RD,
>>> +			.len	= EDID_LENGTH,
>>> +			.buf	= block,
>>> +		}
>>> +	};
>>> +
>>> +	if (!block)
>>> +		return NULL;
>>> +
>>> +	if (i2c_transfer(adapter, msgs, 2) != 2) {
>>> +		DRM_ERROR("Unable to read EDID.\n");
>>> +		goto err;
>>> +	}
>>> +
>>> +	if (!drm_edid_block_valid(block, 0, false, NULL)) {
>>> +		DRM_ERROR("Invalid EDID block\n");
>>> +		goto err;
>>> +	}
>>> +
>>> +	total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
>>> +	if (total_size > EDID_LENGTH) {
>>> +		kfree(block);
>>> +		block = kmalloc(total_size, GFP_KERNEL);
>>> +		if (!block)
>>> +			return NULL;
>>> +
>>> +		/* Yes, read the entire buffer, and do not skip the first
>>> +		 * EDID_LENGTH bytes.
>>> +		 */
>>> +		start = 0x00;
>>> +		msgs[1].len = total_size;
>>> +		msgs[1].buf = block;
>>> +
>>> +		if (i2c_transfer(adapter, msgs, 2) != 2) {
>>> +			DRM_ERROR("Unable to read EDID extension blocks.\n");
>>> +			goto err;
>>> +		}
>>> +	}
>>> +
>>> +	return block;
>>> +
>>> +err:
>>> +	kfree(block);
>>> +	return NULL;
>>> +}
>>> +
>>> +static int ge_b850v3_lvds_dp_get_modes(struct drm_connector *connector)
>>> +{
>>> +	struct ge_b850v3_lvds_dp *ptn_bridge;
>>> +	struct i2c_client *client;
>>> +	int num_modes = 0;
>>> +
>>> +	ptn_bridge = connector_to_ge_b850v3_lvds_dp(connector);
>>> +	client = ptn_bridge->edid_i2c;
>>> +
>>> +	mutex_lock(&ptn_bridge->lock);
>>> +
>>> +	kfree(ptn_bridge->edid);
>>> +	ptn_bridge->edid = (struct edid *) stdp2690_get_edid(client);
>>> +
>>> +	if (ptn_bridge->edid) {
>>> +		drm_mode_connector_update_edid_property(connector,
>>> +				ptn_bridge->edid);
>>> +		num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
>>> +	}
>>> +
>>> +	mutex_unlock(&ptn_bridge->lock);
>>> +
>>> +	return num_modes;
>>> +}
>>> +
>>> +
>>> +static enum drm_mode_status ge_b850v3_lvds_dp_mode_valid(
>>> +		struct drm_connector *connector, struct drm_display_mode *mode)
>>> +{
>>> +	if (mode->clock > MAX_PIXEL_CLOCK) {
>>> +		DRM_INFO("The pixel clock for the mode %s is too high, and not supported.",
>>> +				mode->name);
>>> +		return MODE_CLOCK_HIGH;
>>> +	}
>>> +
>>> +	return MODE_OK;
>>> +}
>>> +
>>> +static const struct
>>> +drm_connector_helper_funcs ge_b850v3_lvds_dp_connector_helper_funcs = {
>>> +	.get_modes = ge_b850v3_lvds_dp_get_modes,
>>> +	.mode_valid = ge_b850v3_lvds_dp_mode_valid,
>>> +};
>>> +
>>> +static enum drm_connector_status ge_b850v3_lvds_dp_detect(
>>> +		struct drm_connector *connector, bool force)
>>> +{
>>> +	struct ge_b850v3_lvds_dp *ptn_bridge =
>>> +			connector_to_ge_b850v3_lvds_dp(connector);
>>> +	struct i2c_client *ge_b850v3_lvds_dp_i2c =
>>> +			ptn_bridge->ge_b850v3_lvds_dp_i2c;
>>> +	s32 link_state;
>>> +
>>> +	link_state = i2c_smbus_read_word_data(ge_b850v3_lvds_dp_i2c,
>>> +			STDP4028_DPTX_STS_REG);
>>> +
>>> +	if (link_state == STDP4028_CON_STATE_CONNECTED)
>>> +		return connector_status_connected;
>>> +
>>> +	if (link_state == 0)
>>> +		return connector_status_disconnected;
>>> +
>>> +	return connector_status_unknown;
>>> +}
>>> +
>>> +static const struct drm_connector_funcs ge_b850v3_lvds_dp_connector_funcs = {
>>> +	.dpms = drm_atomic_helper_connector_dpms,
>>> +	.fill_modes = drm_helper_probe_single_connector_modes,
>>> +	.detect = ge_b850v3_lvds_dp_detect,
>>> +	.destroy = drm_connector_cleanup,
>>> +	.reset = drm_atomic_helper_connector_reset,
>>> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>>> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>> +};
>>> +
>>> +static irqreturn_t ge_b850v3_lvds_dp_irq_handler(int irq, void *dev_id)
>>> +{
>>> +	struct ge_b850v3_lvds_dp *ptn_bridge = dev_id;
>>> +	struct i2c_client *ge_b850v3_lvds_dp_i2c
>>> +			= ptn_bridge->ge_b850v3_lvds_dp_i2c;
>>> +
>>> +	mutex_lock(&ptn_bridge->lock);
>>> +
>>> +	i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
>>> +			STDP4028_DPTX_IRQ_STS_REG, STDP4028_DPTX_IRQ_CLEAR);
>>> +
>>> +	mutex_unlock(&ptn_bridge->lock);
>>> +
>>> +	if (ptn_bridge->connector.dev)
>>> +		drm_kms_helper_hotplug_event(ptn_bridge->connector.dev);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int ge_b850v3_lvds_dp_attach(struct drm_bridge *bridge)
>>> +{
>>> +	struct ge_b850v3_lvds_dp *ptn_bridge
>>> +			= bridge_to_ge_b850v3_lvds_dp(bridge);
>>> +	struct drm_connector *connector = &ptn_bridge->connector;
>>> +	struct i2c_client *ge_b850v3_lvds_dp_i2c
>>> +			= ptn_bridge->ge_b850v3_lvds_dp_i2c;
>>> +	int ret;
>>> +
>>> +	if (!bridge->encoder) {
>>> +		DRM_ERROR("Parent encoder object not found");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	connector->polled = DRM_CONNECTOR_POLL_HPD;
>>> +
>>> +	drm_connector_helper_add(connector,
>>> +			&ge_b850v3_lvds_dp_connector_helper_funcs);
>>> +
>>> +	ret = drm_connector_init(bridge->dev, connector,
>>> +			&ge_b850v3_lvds_dp_connector_funcs,
>>> +			DRM_MODE_CONNECTOR_DisplayPort);
>>> +	if (ret) {
>>> +		DRM_ERROR("Failed to initialize connector with drm\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	drm_connector_register(connector);
>>
>> Connectors shouldn't be registered in the bridge driver, they should
>> be registered by the kms driver after drm_dev_register is called.
>> The drm_connector_register_all() API is used for that.
>
> Hmm, I got this from:
>
> drivers/gpu/drm/bridge/nxp-ptn3460.c:	drm_connector_register(&ptn_bridge->connector);
> drivers/gpu/drm/bridge/parade-ps8622.c:	drm_connector_register(&ps8622->connector);
> drivers/gpu/drm/bridge/analogix-anx78xx.c:	err = drm_connector_register(&anx78xx->connector);

Yeah, we need to get rid of these too, eventually.

The connectors should be registered only after the
drm device has been registered. Relying on the drm
core to do that ensures that. See drm_modeset_register_all()
for more info.

Also, btw, the imx driver shouldn't use the load() drm_driver ops
anymore. You would want to change that too. See the comments in
drm_dev_register() in drm_drv.c for more details.

>
>
>>
>>> +	ret = drm_mode_connector_attach_encoder(connector, bridge->encoder);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	drm_bridge_enable(bridge);
>>
>> This drm_bridge_enable() doesn't seem to serve any purpose here. It also
>> doesn't seem to make much sense to call drm_bridge_() funcs within a
>> bridge op itself.
>
> Yes, removing this line has no effect. I'll send V6. Thank you!
>
>>
>>> +	if (ge_b850v3_lvds_dp_i2c->irq) {
>>> +		drm_helper_hpd_irq_event(connector->dev);
>>> +
>>> +		ret = devm_request_threaded_irq(&ge_b850v3_lvds_dp_i2c->dev,
>>> +				ge_b850v3_lvds_dp_i2c->irq, NULL,
>>> +				ge_b850v3_lvds_dp_irq_handler,
>>> +				IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
>>> +				"ge-b850v3-lvds-dp", ptn_bridge);
>>
>> Is there a reason why we register the interrupt handler here and not in
>> probe?
>
> Yes, drm_bridge_funcs.attach() is called when drm decides it is time to initialize the bridge, which means the drm is already running. i2c_driver.probe() is likely to be called way before drm initializes, and if for some reason the bridge is not attached, there is no need for the interrupt handler. In this case the interrupt handler is mostly used for events related to display plugging/unplugging.

The i2c device can probe before drm, this should be fixed by making sure
the device's interrupts are masked before we request the handler, and
unmasking it when we know it is ready for use.

If a bridge device exists and isn't attached, then we should question
why it probed in the first place if it wasn't to be used.

Besides, in the current scenario, consider the case where the imx
driver's call to drm_bridge_attach() succeeds, but then later fails at
some point. There would be no one to remove this interrupt handler. The
devm_ api attaches the lifetime of the handler with the i2c device, not
the drm device.

Also, the call to drm_helper_hpd_irq_event(drm_dev) should go too. These
should be called only when the drm device is registered.

Thanks,
Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply

* [PATCH v4] devicetree: bindings: uart: Add new compatible string for ZynqMP
From: Nava kishore Manne @ 2016-09-26 12:49 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20160923223747.GA1163@rob-hp-laptop>



> -----Original Message-----
> From: Rob Herring [mailto:robh at kernel.org]
> Sent: Saturday, September 24, 2016 4:08 AM
> To: Nava kishore Manne <navam@xilinx.com>
> Cc: gregkh at linuxfoundation.org; mark.rutland at arm.com; jslaby at suse.com;
> michal.simek at xilinx.com; Soren Brinkmann <sorenb@xilinx.com>; linux-
> serial at vger.kernel.org; devicetree at vger.kernel.org; linux-
> kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Punnaiah
> Choudary Kalluri <punnaia@xilinx.com>; Nava kishore Manne
> <navam@xilinx.com>
> Subject: Re: [PATCH v4] devicetree: bindings: uart: Add new compatible
> string for ZynqMP
>
> On Fri, Sep 23, 2016 at 04:55:48PM +0530, Nava kishore Manne wrote:
> > From: Nava kishore Manne <nava.manne@xilinx.com>
> >
> > This patch Adds the new compatible string for ZynqMP.
> >
> > Signed-off-by: Nava kishore Manne <navam@xilinx.com>
> > ---
> > Changes for v4:
> >             -Modified the ChangeLog comment.
> > Changes for v3:
> >             -Added changeLog comment.
> > Changes for v2:
> >                 -None
> >
> >  Documentation/devicetree/bindings/serial/cdns,uart.txt | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/serial/cdns,uart.txt
> b/Documentation/devicetree/bindings/serial/cdns,uart.txt
> > index a3eb154..4fe7aae 100644
> > --- a/Documentation/devicetree/bindings/serial/cdns,uart.txt
> > +++ b/Documentation/devicetree/bindings/serial/cdns,uart.txt
> > @@ -1,7 +1,8 @@
> >  Binding for Cadence UART Controller
> >
> >  Required properties:
> > -- compatible : should be "cdns,uart-r1p8", or "xlnx,xuartps"
> > +- compatible : should be "cdns,uart-r1p8", or "xlnx,xuartps" for Zynq and
> > +               "cdns,uart-r1p12" for Zynq Ultrascale+ MPSoC uart controller.
>
> Please format this as one valid combination per line.
>
> xuartps is not very specific. It needs some abbreviation of "Zynq
> Ultrascale+ MPSoC" in it.
>

Will fix the above comments in the next version.

Regards,
Navakishore.


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

^ 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