from:"JeffyChen"


Hi Brian,

On 05/08/2018 10:31 AM, JeffyChen wrote:

Hi Brian,

On 05/08/2018 09:56 AM, Brian Norris wrote:

On Tue, May 08, 2018 at 09:36:24AM +0800, Jeffy Chen wrote:

On 05/08/2018 06:15 AM, Brian Norris wrote:

On the other hand...this also implies there may be a race condition
there, where we might lose an interrupt if there is an edge between the
re-configuration of the polarity in rockchip_irq_demux() and the
clearing/handling of the interrupt (handle_edge_irq() ->
chip->irq_ack()). If we have an edge between there, then we might ack
it, but leave the polarity such that we aren't ready for the next
(inverted) edge.


if let me guess, the unexpected irq we saw is the hardware trying to
avoid
losing irq? for example, we set a EDGE_RISING, and the hardware saw
the gpio
is already high, then though it might lost an irq, so fake one for safe?


I won't pretend to know what the IC designers were doing, but I don't
think that would resolve the problem I'm talking about. The sequence is
something like:
1. EDGE_BOTH IRQ occurs (e.g., low to high)
2. reconfigure polarity in rockchip_irq_demux() (polarity=low)
3. continue to handle_edge_irq()
4. another HW edge occurs (e.g., high to low)
5. handle_edge_irq() (from 3) acks (clears) IRQ (before a subsequent
rockchip_irq_demux() gets a chance to run and flip the polarity)
...

Now the polarity is still low, but the next trigger should be a
low-to-high edge.


oops, i see the problem.

so what if we do these:
1/ edge irq triggered
2/ read gpio level
3/ handle irq(ack irq)
4/ toggle edge mode(with a while gpio level check)

if the gpio changed in 2/ -> 3/, the 4/ will trigger an irq when writing
GPIO_INT_POLARITY(which is what we are trying to avoid in the set_type
case)

but this would not work if i'm wrong about how the HW fake an irq when
changing POLARITY...


or maybe we could just check the gpio status again after
handle_edge_irq, and correct the polarity in this case


i saw the pinctrl-msm.c do this in the ack(),

and also at the end of set_type(), which might avoid another race in the 
set_type()







i'll try to confirm it with IC guys.


Brian

Re: [RESEND PATCH] pinctrl: rockchip: Disable interrupt when changing it's capability


Hi Brian,

On 05/08/2018 10:31 AM, JeffyChen wrote:

Hi Brian,

On 05/08/2018 09:56 AM, Brian Norris wrote:

On Tue, May 08, 2018 at 09:36:24AM +0800, Jeffy Chen wrote:

On 05/08/2018 06:15 AM, Brian Norris wrote:

On the other hand...this also implies there may be a race condition
there, where we might lose an interrupt if there is an edge between the
re-configuration of the polarity in rockchip_irq_demux() and the
clearing/handling of the interrupt (handle_edge_irq() ->
chip->irq_ack()). If we have an edge between there, then we might ack
it, but leave the polarity such that we aren't ready for the next
(inverted) edge.


if let me guess, the unexpected irq we saw is the hardware trying to
avoid
losing irq? for example, we set a EDGE_RISING, and the hardware saw
the gpio
is already high, then though it might lost an irq, so fake one for safe?


I won't pretend to know what the IC designers were doing, but I don't
think that would resolve the problem I'm talking about. The sequence is
something like:
1. EDGE_BOTH IRQ occurs (e.g., low to high)
2. reconfigure polarity in rockchip_irq_demux() (polarity=low)
3. continue to handle_edge_irq()
4. another HW edge occurs (e.g., high to low)
5. handle_edge_irq() (from 3) acks (clears) IRQ (before a subsequent
rockchip_irq_demux() gets a chance to run and flip the polarity)
...

Now the polarity is still low, but the next trigger should be a
low-to-high edge.


oops, i see the problem.

so what if we do these:
1/ edge irq triggered
2/ read gpio level
3/ handle irq(ack irq)
4/ toggle edge mode(with a while gpio level check)

if the gpio changed in 2/ -> 3/, the 4/ will trigger an irq when writing
GPIO_INT_POLARITY(which is what we are trying to avoid in the set_type
case)

but this would not work if i'm wrong about how the HW fake an irq when
changing POLARITY...


or maybe we could just check the gpio status again after
handle_edge_irq, and correct the polarity in this case


i saw the pinctrl-msm.c do this in the ack(),

and also at the end of set_type(), which might avoid another race in the 
set_type()







i'll try to confirm it with IC guys.


Brian

Re: [RESEND PATCH] pinctrl: rockchip: Disable interrupt when changing it's capability


Hi Brian,

On 05/08/2018 09:56 AM, Brian Norris wrote:

On Tue, May 08, 2018 at 09:36:24AM +0800, Jeffy Chen wrote:

On 05/08/2018 06:15 AM, Brian Norris wrote:

On the other hand...this also implies there may be a race condition
there, where we might lose an interrupt if there is an edge between the
re-configuration of the polarity in rockchip_irq_demux() and the
clearing/handling of the interrupt (handle_edge_irq() ->
chip->irq_ack()). If we have an edge between there, then we might ack
it, but leave the polarity such that we aren't ready for the next
(inverted) edge.


if let me guess, the unexpected irq we saw is the hardware trying to avoid
losing irq? for example, we set a EDGE_RISING, and the hardware saw the gpio
is already high, then though it might lost an irq, so fake one for safe?


I won't pretend to know what the IC designers were doing, but I don't
think that would resolve the problem I'm talking about. The sequence is
something like:
1. EDGE_BOTH IRQ occurs (e.g., low to high)
2. reconfigure polarity in rockchip_irq_demux() (polarity=low)
3. continue to handle_edge_irq()
4. another HW edge occurs (e.g., high to low)
5. handle_edge_irq() (from 3) acks (clears) IRQ (before a subsequent
rockchip_irq_demux() gets a chance to run and flip the polarity)
...

Now the polarity is still low, but the next trigger should be a
low-to-high edge.


oops, i see the problem.

so what if we do these:
1/ edge irq triggered
2/ read gpio level
3/ handle irq(ack irq)
4/ toggle edge mode(with a while gpio level check)

if the gpio changed in 2/ -> 3/, the 4/ will trigger an irq when writing 
GPIO_INT_POLARITY(which is what we are trying to avoid in the set_type case)


but this would not work if i'm wrong about how the HW fake an irq when 
changing POLARITY...



or maybe we could just check the gpio status again after 
handle_edge_irq, and correct the polarity in this case





i'll try to confirm it with IC guys.


Brian

Re: [RESEND PATCH] pinctrl: rockchip: Disable interrupt when changing it's capability


Hi Brian,

On 05/08/2018 09:56 AM, Brian Norris wrote:

On Tue, May 08, 2018 at 09:36:24AM +0800, Jeffy Chen wrote:

On 05/08/2018 06:15 AM, Brian Norris wrote:

On the other hand...this also implies there may be a race condition
there, where we might lose an interrupt if there is an edge between the
re-configuration of the polarity in rockchip_irq_demux() and the
clearing/handling of the interrupt (handle_edge_irq() ->
chip->irq_ack()). If we have an edge between there, then we might ack
it, but leave the polarity such that we aren't ready for the next
(inverted) edge.


if let me guess, the unexpected irq we saw is the hardware trying to avoid
losing irq? for example, we set a EDGE_RISING, and the hardware saw the gpio
is already high, then though it might lost an irq, so fake one for safe?


I won't pretend to know what the IC designers were doing, but I don't
think that would resolve the problem I'm talking about. The sequence is
something like:
1. EDGE_BOTH IRQ occurs (e.g., low to high)
2. reconfigure polarity in rockchip_irq_demux() (polarity=low)
3. continue to handle_edge_irq()
4. another HW edge occurs (e.g., high to low)
5. handle_edge_irq() (from 3) acks (clears) IRQ (before a subsequent
rockchip_irq_demux() gets a chance to run and flip the polarity)
...

Now the polarity is still low, but the next trigger should be a
low-to-high edge.


oops, i see the problem.

so what if we do these:
1/ edge irq triggered
2/ read gpio level
3/ handle irq(ack irq)
4/ toggle edge mode(with a while gpio level check)

if the gpio changed in 2/ -> 3/, the 4/ will trigger an irq when writing 
GPIO_INT_POLARITY(which is what we are trying to avoid in the set_type case)


but this would not work if i'm wrong about how the HW fake an irq when 
changing POLARITY...



or maybe we could just check the gpio status again after 
handle_edge_irq, and correct the polarity in this case





i'll try to confirm it with IC guys.


Brian

Re: [RESEND PATCH] pinctrl: rockchip: Disable interrupt when changing it's capability


Hi Brian,

On 05/08/2018 06:15 AM, Brian Norris wrote:

+ Doug

Hi Jeffy,

On Thu, May 03, 2018 at 02:55:53PM +0800, Jeffy Chen wrote:

We saw spurious irq when changing irq's trigger type, for example
setting gpio-keys's wakeup irq trigger type.

And according to the TRM:
"Programming the GPIO registers for interrupt capability, edge-sensitive
or level-sensitive interrupts, and interrupt polarity should be
completed prior to enabling the interrupts on Port A in order to prevent
spurious glitches on the interrupt lines to the interrupt controller."

Reported-by: Brian Norris 
Signed-off-by: Jeffy Chen 
---

  drivers/pinctrl/pinctrl-rockchip.c | 10 ++
  1 file changed, 10 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c 
b/drivers/pinctrl/pinctrl-rockchip.c
index 3924779f55785..7ff45ec8330d1 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -2727,9 +2727,19 @@ static int rockchip_irq_set_type(struct irq_data *d, 
unsigned int type)
return -EINVAL;
}

+   /**


The typical multiline comment style is to use only 1 asterisk -- 2
asterisks usually denote something more formal, like kerneldoc.


ok, will fix it.




+* According to the TRM, we should keep irq disabled during programming
+* interrupt capability to prevent spurious glitches on the interrupt
+* lines to the interrupt controller.
+*/


It's also worth noting that this case may come up in
rockchip_irq_demux() for EDGE_BOTH triggers:

/*
 * Triggering IRQ on both rising and falling edge
 * needs manual intervention.
 */
if (bank->toggle_edge_mode & BIT(irq)) {
...
polarity = readl_relaxed(bank->reg_base +
 GPIO_INT_POLARITY);
if (data & BIT(irq))
polarity &= ~BIT(irq);
else
polarity |= BIT(irq);
writel(polarity,
   bank->reg_base + GPIO_INT_POLARITY);

AIUI, this case is not actually a problem, because this polarity
reconfiguration happens before we call generic_handle_irq(), so the
extra spurious interrupt is handled and cleared after this point. But it
seems like this should be noted somewhere in either the commit message,
the code comments, or both.


just from my testing, the spurious irq only happen when set EDGE_RISING 
to a gpio which is already high level, or set EDGE_FALLING to a gpio 
which is already low level.so the demux / EDGE_BOTH seem ok.


but adding comments as your suggested is a good idea, will do that.



On the other hand...this also implies there may be a race condition
there, where we might lose an interrupt if there is an edge between the
re-configuration of the polarity in rockchip_irq_demux() and the
clearing/handling of the interrupt (handle_edge_irq() ->
chip->irq_ack()). If we have an edge between there, then we might ack
it, but leave the polarity such that we aren't ready for the next
(inverted) edge.


if let me guess, the unexpected irq we saw is the hardware trying to 
avoid losing irq? for example, we set a EDGE_RISING, and the hardware 
saw the gpio is already high, then though it might lost an irq, so fake 
one for safe?


i'll try to confirm it with IC guys.



I'm not 100% sure about the above, so it might be good to verify it. But
I also expect this means there's really no way to 100% reliably
implement both-edge trigger types on hardware like this that doesn't
directly implement it. So I'm not sure what we'd do with that knowledge.


+   data = readl(bank->reg_base + GPIO_INTEN);
+   writel_relaxed(data & ~mask, gc->reg_base + GPIO_INTEN);


Not sure if this is a needless optimization: but couldn't you only
disable the interrupt (and make the level and polarity changes) only if
the level or polarity are actually changing? For example, it's possible
to end up here when changing from EDGE_BOTH to EDGE_RISING, but we might
not actually program a new value.


right, i noticed that too, will add a patch for that in v2.


Brian


+
writel_relaxed(level, gc->reg_base + GPIO_INTTYPE_LEVEL);
writel_relaxed(polarity, gc->reg_base + GPIO_INT_POLARITY);

+   writel_relaxed(data, gc->reg_base + GPIO_INTEN);
+
irq_gc_unlock(gc);
raw_spin_unlock_irqrestore(>slock, flags);
clk_disable(bank->clk);
--
2.11.0

Re: [RESEND PATCH] pinctrl: rockchip: Disable interrupt when changing it's capability


Hi Brian,

On 05/08/2018 06:15 AM, Brian Norris wrote:

+ Doug

Hi Jeffy,

On Thu, May 03, 2018 at 02:55:53PM +0800, Jeffy Chen wrote:

We saw spurious irq when changing irq's trigger type, for example
setting gpio-keys's wakeup irq trigger type.

And according to the TRM:
"Programming the GPIO registers for interrupt capability, edge-sensitive
or level-sensitive interrupts, and interrupt polarity should be
completed prior to enabling the interrupts on Port A in order to prevent
spurious glitches on the interrupt lines to the interrupt controller."

Reported-by: Brian Norris 
Signed-off-by: Jeffy Chen 
---

  drivers/pinctrl/pinctrl-rockchip.c | 10 ++
  1 file changed, 10 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c 
b/drivers/pinctrl/pinctrl-rockchip.c
index 3924779f55785..7ff45ec8330d1 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -2727,9 +2727,19 @@ static int rockchip_irq_set_type(struct irq_data *d, 
unsigned int type)
return -EINVAL;
}

+   /**


The typical multiline comment style is to use only 1 asterisk -- 2
asterisks usually denote something more formal, like kerneldoc.


ok, will fix it.




+* According to the TRM, we should keep irq disabled during programming
+* interrupt capability to prevent spurious glitches on the interrupt
+* lines to the interrupt controller.
+*/


It's also worth noting that this case may come up in
rockchip_irq_demux() for EDGE_BOTH triggers:

/*
 * Triggering IRQ on both rising and falling edge
 * needs manual intervention.
 */
if (bank->toggle_edge_mode & BIT(irq)) {
...
polarity = readl_relaxed(bank->reg_base +
 GPIO_INT_POLARITY);
if (data & BIT(irq))
polarity &= ~BIT(irq);
else
polarity |= BIT(irq);
writel(polarity,
   bank->reg_base + GPIO_INT_POLARITY);

AIUI, this case is not actually a problem, because this polarity
reconfiguration happens before we call generic_handle_irq(), so the
extra spurious interrupt is handled and cleared after this point. But it
seems like this should be noted somewhere in either the commit message,
the code comments, or both.


just from my testing, the spurious irq only happen when set EDGE_RISING 
to a gpio which is already high level, or set EDGE_FALLING to a gpio 
which is already low level.so the demux / EDGE_BOTH seem ok.


but adding comments as your suggested is a good idea, will do that.



On the other hand...this also implies there may be a race condition
there, where we might lose an interrupt if there is an edge between the
re-configuration of the polarity in rockchip_irq_demux() and the
clearing/handling of the interrupt (handle_edge_irq() ->
chip->irq_ack()). If we have an edge between there, then we might ack
it, but leave the polarity such that we aren't ready for the next
(inverted) edge.


if let me guess, the unexpected irq we saw is the hardware trying to 
avoid losing irq? for example, we set a EDGE_RISING, and the hardware 
saw the gpio is already high, then though it might lost an irq, so fake 
one for safe?


i'll try to confirm it with IC guys.



I'm not 100% sure about the above, so it might be good to verify it. But
I also expect this means there's really no way to 100% reliably
implement both-edge trigger types on hardware like this that doesn't
directly implement it. So I'm not sure what we'd do with that knowledge.


+   data = readl(bank->reg_base + GPIO_INTEN);
+   writel_relaxed(data & ~mask, gc->reg_base + GPIO_INTEN);


Not sure if this is a needless optimization: but couldn't you only
disable the interrupt (and make the level and polarity changes) only if
the level or polarity are actually changing? For example, it's possible
to end up here when changing from EDGE_BOTH to EDGE_RISING, but we might
not actually program a new value.


right, i noticed that too, will add a patch for that in v2.


Brian


+
writel_relaxed(level, gc->reg_base + GPIO_INTTYPE_LEVEL);
writel_relaxed(polarity, gc->reg_base + GPIO_INT_POLARITY);

+   writel_relaxed(data, gc->reg_base + GPIO_INTEN);
+
irq_gc_unlock(gc);
raw_spin_unlock_irqrestore(>slock, flags);
clk_disable(bank->clk);
--
2.11.0

Re: [regression, bisected] rockchip rk3399 video output breakage


Hi Jokab,

Thanks for your reply.

On 04/24/2018 09:11 PM, Jakob Unterwurzacher wrote:

On 24.04.18 14:37, JeffyChen wrote:


right, i think it's a known issue, as the iommu failed to get clks:
[1.525153] rk_iommu ff8f3f00.iommu: Failed to get clk 'iface': -2
[1.525316] rk_iommu: probe of ff8f3f00.iommu failed with error -2
[1.525484] rk_iommu ff903f00.iommu: Failed to get clk 'iface': -2
[1.525643] rk_iommu: probe of ff903f00.iommu failed with error -2


there's a followed patch to add those clks to the dtsi, and also a fix
patch to make those clks optional(by heiko):

https://www.spinics.net/lists/arm-kernel/msg645696.html

http://lists.infradead.org/pipermail/linux-rockchip/2018-April/020349.html



Thanks, I tested both, and both get the screen to display some kernel
output!

However, I am getting some nasty error messages and the screen seems to
refresh only once every 30 seconds:


[   15.586502] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[CRTC:30:crtc-0] flip_done timed out
[   25.826490] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[CONNECTOR:37:HDMI-A-1] flip_done timed out
[   36.066490] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[PLANE:28:plane-0] flip_done timed out
[   36.066504] [drm:drm_calc_timestamping_constants] crtc 30: hwmode:
htotal 2200, vtotal 1125, vdisplay 1080
[   36.066508] [drm:drm_calc_timestamping_constants] crtc 30: clock
148500 kHz framedur 1666 linedur 14814
[   36.076535] rockchip-vop ff8f.vop: [drm:vop_crtc_atomic_flush]
*ERROR* VOP vblank IRQ stuck for 10 ms
[   36.076577] WARNING: CPU: 1 PID: 83 at
drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1004
vop_crtc_atomic_flush+0x1c0/0x1c8


this looks like an issue recently reported by heiko, we found that might 
due to an unbalanced irq disable in vop driver.


my test patch is:

+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1601,6 +1601,8 @@ static void vop_unbind(struct device *dev, struct 
device *master, void *data)

 {
struct vop *vop = dev_get_drvdata(dev);

+   // Pair with the initial disable_irq()
+   enable_irq(vop->irq);



and i think sandy would send the real patch soon(maybe already sent?)


Full dmesg with patch "arm64: dts: rockchip: add clocks in iommu nodes":
https://gist.github.com/jakob-tsd/3fd49894d52dcd8a409eb9e6136b2d39

Full dmesg with patch "iommu/rockchip: make clock handling optional":
https://gist.github.com/jakob-tsd/da96572a40d11f0f6dff3ee481098138
(looks the same)

Thanks,
Jakob

Re: [regression, bisected] rockchip rk3399 video output breakage


Hi Jokab,

Thanks for your reply.

On 04/24/2018 09:11 PM, Jakob Unterwurzacher wrote:

On 24.04.18 14:37, JeffyChen wrote:


right, i think it's a known issue, as the iommu failed to get clks:
[1.525153] rk_iommu ff8f3f00.iommu: Failed to get clk 'iface': -2
[1.525316] rk_iommu: probe of ff8f3f00.iommu failed with error -2
[1.525484] rk_iommu ff903f00.iommu: Failed to get clk 'iface': -2
[1.525643] rk_iommu: probe of ff903f00.iommu failed with error -2


there's a followed patch to add those clks to the dtsi, and also a fix
patch to make those clks optional(by heiko):

https://www.spinics.net/lists/arm-kernel/msg645696.html

http://lists.infradead.org/pipermail/linux-rockchip/2018-April/020349.html



Thanks, I tested both, and both get the screen to display some kernel
output!

However, I am getting some nasty error messages and the screen seems to
refresh only once every 30 seconds:


[   15.586502] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[CRTC:30:crtc-0] flip_done timed out
[   25.826490] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[CONNECTOR:37:HDMI-A-1] flip_done timed out
[   36.066490] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[PLANE:28:plane-0] flip_done timed out
[   36.066504] [drm:drm_calc_timestamping_constants] crtc 30: hwmode:
htotal 2200, vtotal 1125, vdisplay 1080
[   36.066508] [drm:drm_calc_timestamping_constants] crtc 30: clock
148500 kHz framedur 1666 linedur 14814
[   36.076535] rockchip-vop ff8f.vop: [drm:vop_crtc_atomic_flush]
*ERROR* VOP vblank IRQ stuck for 10 ms
[   36.076577] WARNING: CPU: 1 PID: 83 at
drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1004
vop_crtc_atomic_flush+0x1c0/0x1c8


this looks like an issue recently reported by heiko, we found that might 
due to an unbalanced irq disable in vop driver.


my test patch is:

+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1601,6 +1601,8 @@ static void vop_unbind(struct device *dev, struct 
device *master, void *data)

 {
struct vop *vop = dev_get_drvdata(dev);

+   // Pair with the initial disable_irq()
+   enable_irq(vop->irq);



and i think sandy would send the real patch soon(maybe already sent?)


Full dmesg with patch "arm64: dts: rockchip: add clocks in iommu nodes":
https://gist.github.com/jakob-tsd/3fd49894d52dcd8a409eb9e6136b2d39

Full dmesg with patch "iommu/rockchip: make clock handling optional":
https://gist.github.com/jakob-tsd/da96572a40d11f0f6dff3ee481098138
(looks the same)

Thanks,
Jakob

Re: [regression, bisected] rockchip rk3399 video output breakage


Hi Jakob,

Thanks for your message.

On 04/24/2018 08:19 PM, Jakob Unterwurzacher wrote:


Full dmesg:
https://gist.github.com/jakob-tsd/33cf395e355bf9bb6956c36438d999e7

I have bisected the "master bind failed" down to:


commit 9176a303d971dc0fb35469c531c0d263667d2277
Author: Jeffy Chen 
Date:   Fri Mar 23 15:38:10 2018 +0800

iommu/rockchip: Use IOMMU device for dma mapping operations

Use the first registered IOMMU device for dma mapping operations, and
drop the domain platform device.

This is similar to exynos iommu driver.

Signed-off-by: Jeffy Chen 
Reviewed-by: Tomasz Figa 
Reviewed-by: Robin Murphy 
Signed-off-by: Joerg Roedel 


Moving to one commit earlier brings the screen to life. Just with
colorful garbage, but I guess that's a different problem.

Is this a known issue with the IOMMU change?


right, i think it's a known issue, as the iommu failed to get clks:
[1.525153] rk_iommu ff8f3f00.iommu: Failed to get clk 'iface': -2
[1.525316] rk_iommu: probe of ff8f3f00.iommu failed with error -2
[1.525484] rk_iommu ff903f00.iommu: Failed to get clk 'iface': -2
[1.525643] rk_iommu: probe of ff903f00.iommu failed with error -2


there's a followed patch to add those clks to the dtsi, and also a fix 
patch to make those clks optional(by heiko):


https://www.spinics.net/lists/arm-kernel/msg645696.html

http://lists.infradead.org/pipermail/linux-rockchip/2018-April/020349.html



Thanks,
Jakob

Re: [regression, bisected] rockchip rk3399 video output breakage


Hi Jakob,

Thanks for your message.

On 04/24/2018 08:19 PM, Jakob Unterwurzacher wrote:


Full dmesg:
https://gist.github.com/jakob-tsd/33cf395e355bf9bb6956c36438d999e7

I have bisected the "master bind failed" down to:


commit 9176a303d971dc0fb35469c531c0d263667d2277
Author: Jeffy Chen 
Date:   Fri Mar 23 15:38:10 2018 +0800

iommu/rockchip: Use IOMMU device for dma mapping operations

Use the first registered IOMMU device for dma mapping operations, and
drop the domain platform device.

This is similar to exynos iommu driver.

Signed-off-by: Jeffy Chen 
Reviewed-by: Tomasz Figa 
Reviewed-by: Robin Murphy 
Signed-off-by: Joerg Roedel 


Moving to one commit earlier brings the screen to life. Just with
colorful garbage, but I guess that's a different problem.

Is this a known issue with the IOMMU change?


right, i think it's a known issue, as the iommu failed to get clks:
[1.525153] rk_iommu ff8f3f00.iommu: Failed to get clk 'iface': -2
[1.525316] rk_iommu: probe of ff8f3f00.iommu failed with error -2
[1.525484] rk_iommu ff903f00.iommu: Failed to get clk 'iface': -2
[1.525643] rk_iommu: probe of ff903f00.iommu failed with error -2


there's a followed patch to add those clks to the dtsi, and also a fix 
patch to make those clks optional(by heiko):


https://www.spinics.net/lists/arm-kernel/msg645696.html

http://lists.infradead.org/pipermail/linux-rockchip/2018-April/020349.html



Thanks,
Jakob

Re: [PATCH v8 11/14] iommu/rockchip: Use OF_IOMMU to attach devices automatically


Hi Daniel,

Thanks for your reply.

On 04/04/2018 12:11 AM, Daniel Kurtz wrote:

Hi Jeffy,

Sorry for delayed response.

On Mon, Mar 26, 2018 at 1:58 AM JeffyChen <jeffy.c...@rock-chips.com> wrote:


Hi Daniel,



Thanks for your reply.



On 03/26/2018 02:31 PM, Daniel Kurtz wrote:

+struct rk_iommudata {
+   struct rk_iommu *iommu;
+};

Why do we need this struct?  Can't we just assign a pointer to struct
rk_iommu directly to dev->archdata.iommu?


hmmm, i was trying to add more device related data in patch[13]:



struct rk_iommudata {
+   struct device_link *link; /* runtime PM link from IOMMU to master

*/

  struct rk_iommu *iommu;
};




Can't you just add link to rk_iommu directly?


adding link to rk_iommu would be fine if we only have one master device 
per rk_iommu.


but now we are supporting multiple master devices sharing a iommu device :)

Re: [PATCH v8 11/14] iommu/rockchip: Use OF_IOMMU to attach devices automatically


Hi Daniel,

Thanks for your reply.

On 04/04/2018 12:11 AM, Daniel Kurtz wrote:

Hi Jeffy,

Sorry for delayed response.

On Mon, Mar 26, 2018 at 1:58 AM JeffyChen  wrote:


Hi Daniel,



Thanks for your reply.



On 03/26/2018 02:31 PM, Daniel Kurtz wrote:

+struct rk_iommudata {
+   struct rk_iommu *iommu;
+};

Why do we need this struct?  Can't we just assign a pointer to struct
rk_iommu directly to dev->archdata.iommu?


hmmm, i was trying to add more device related data in patch[13]:



struct rk_iommudata {
+   struct device_link *link; /* runtime PM link from IOMMU to master

*/

  struct rk_iommu *iommu;
};




Can't you just add link to rk_iommu directly?


adding link to rk_iommu would be fine if we only have one master device 
per rk_iommu.


but now we are supporting multiple master devices sharing a iommu device :)

Re: [PATCH] iommu: rockchip: fix building without CONFIG_OF


Hi Amd,

Thanks for your patch.

On 04/04/2018 06:23 PM, Arnd Bergmann wrote:

We get a build error when compiling the iommu driver without CONFIG_OF:

drivers/iommu/rockchip-iommu.c: In function 'rk_iommu_of_xlate':
drivers/iommu/rockchip-iommu.c:1101:2: error: implicit declaration of function 
'of_dev_put'; did you mean 'of_node_put'? 
[-Werror=implicit-function-declaration]


oops, didn't notice that.

and checking other iommu drivers which call of_find_device_by_node() 
too, seems they didn't put() it? maybe we should fix them too



This replaces the of_dev_put() with the equivalent platform_device_put(),
and adds an error check for of_find_device_by_node() returning NULL,
which seems to be appropriate here given that we pass the device into
platform_get_drvdata() next, and that of_find_device_by_node() might
theoretically return a NULL pointer.

hmm, i thought the of_iommu_xlate() checked that before calling us:


static int of_iommu_xlate(struct device *dev,
  struct of_phandle_args *iommu_spec)
{
...
if ((ops && !ops->of_xlate) ||
!of_device_is_available(iommu_spec->np) ||
(!ops && !of_iommu_driver_present(iommu_spec->np)))
return NO_IOMMU;
...
return ops->of_xlate(dev, iommu_spec);


but it's better to check it ourself :)




Fixes: 5fd577c3eac3 ("iommu/rockchip: Use OF_IOMMU to attach devices 
automatically")
Signed-off-by: Arnd Bergmann 
---
This warning appears to only have been introduced in linux-next after
the merge window opened.
---
  drivers/iommu/rockchip-iommu.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 5fc8656c60f9..fe9c9cc1a9d4 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1094,11 +1094,15 @@ static int rk_iommu_of_xlate(struct device *dev,
return -ENOMEM;

iommu_dev = of_find_device_by_node(args->np);
+   if (!iommu_dev) {
+   kfree(data);
+   return -ENODEV;
+   }

data->iommu = platform_get_drvdata(iommu_dev);
dev->archdata.iommu = data;

-   of_dev_put(iommu_dev);
+   platform_device_put(iommu_dev);

return 0;
  }

Re: [PATCH] iommu: rockchip: fix building without CONFIG_OF


Hi Amd,

Thanks for your patch.

On 04/04/2018 06:23 PM, Arnd Bergmann wrote:

We get a build error when compiling the iommu driver without CONFIG_OF:

drivers/iommu/rockchip-iommu.c: In function 'rk_iommu_of_xlate':
drivers/iommu/rockchip-iommu.c:1101:2: error: implicit declaration of function 
'of_dev_put'; did you mean 'of_node_put'? 
[-Werror=implicit-function-declaration]


oops, didn't notice that.

and checking other iommu drivers which call of_find_device_by_node() 
too, seems they didn't put() it? maybe we should fix them too



This replaces the of_dev_put() with the equivalent platform_device_put(),
and adds an error check for of_find_device_by_node() returning NULL,
which seems to be appropriate here given that we pass the device into
platform_get_drvdata() next, and that of_find_device_by_node() might
theoretically return a NULL pointer.

hmm, i thought the of_iommu_xlate() checked that before calling us:


static int of_iommu_xlate(struct device *dev,
  struct of_phandle_args *iommu_spec)
{
...
if ((ops && !ops->of_xlate) ||
!of_device_is_available(iommu_spec->np) ||
(!ops && !of_iommu_driver_present(iommu_spec->np)))
return NO_IOMMU;
...
return ops->of_xlate(dev, iommu_spec);


but it's better to check it ourself :)




Fixes: 5fd577c3eac3 ("iommu/rockchip: Use OF_IOMMU to attach devices 
automatically")
Signed-off-by: Arnd Bergmann 
---
This warning appears to only have been introduced in linux-next after
the merge window opened.
---
  drivers/iommu/rockchip-iommu.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 5fc8656c60f9..fe9c9cc1a9d4 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1094,11 +1094,15 @@ static int rk_iommu_of_xlate(struct device *dev,
return -ENOMEM;

iommu_dev = of_find_device_by_node(args->np);
+   if (!iommu_dev) {
+   kfree(data);
+   return -ENODEV;
+   }

data->iommu = platform_get_drvdata(iommu_dev);
dev->archdata.iommu = data;

-   of_dev_put(iommu_dev);
+   platform_device_put(iommu_dev);

return 0;
  }

Re: [PATCH v5 3/4] clk: lpc32xx: Set name of regmap_config

2018-03-18 Thread JeffyChen

Hi Vladimir,

On 03/19/2018 05:57 AM, Vladimir Zapolskiy wrote:

>  static struct regmap_config lpc32xx_scb_regmap_config = {
>+   .name = "lpc32xx-scb",

please rename it to "scb", LPC32xx SoC name is already known from the context.

When it's done, feel free to add to the next version my

Acked-by: Vladimir Zapolskiy

ok, will do, thanks:)

Re: [PATCH v5 3/4] clk: lpc32xx: Set name of regmap_config

2018-03-18 Thread JeffyChen

Hi Vladimir,

On 03/19/2018 05:57 AM, Vladimir Zapolskiy wrote:

>  static struct regmap_config lpc32xx_scb_regmap_config = {
>+   .name = "lpc32xx-scb",

please rename it to "scb", LPC32xx SoC name is already known from the context.

When it's done, feel free to add to the next version my

Acked-by: Vladimir Zapolskiy

ok, will do, thanks:)

Re: [PATCH v5 1/3] Input: gpio-keys - add support for wakeup event action


Hi Dmitry,

Thanks for your reply.

On 03/11/2018 02:15 AM, Dmitry Torokhov wrote:>> +static int 
gpio_keys_enable_wakeup(struct gpio_button_data *bdata)

> These new helpers need to be __maybe_unused in case we compile on system
> without suspend support.
>
> I also wanted a bit more of error handling, so I ended up with the
> version of the patch below. Can you please try it and let me know if I
> broke it.

sure, the patch you sent works :)

Tested-by: Jeffy Chen 

>
> Thanks.
>
> -- Dmitry
>
> Input: gpio-keys - add support for wakeup event action
>
> From: Jeffy Chen 
>
> Add support for specifying event actions to trigger wakeup when using
> the gpio-keys input device as a wakeup source.
>
> This would allow the device to configure when to wakeup the system. For
> example a gpio-keys input device for pen insert, may only want to wakeup
> the system when ejecting the pen.
>
> Suggested-by: Brian Norris 
> Signed-off-by: Jeffy Chen 
> Reviewed-by: Rob Herring 
> Signed-off-by: Dmitry Torokhov

Re: [PATCH v5 1/3] Input: gpio-keys - add support for wakeup event action


Hi Dmitry,

Thanks for your reply.

On 03/11/2018 02:15 AM, Dmitry Torokhov wrote:>> +static int 
gpio_keys_enable_wakeup(struct gpio_button_data *bdata)

> These new helpers need to be __maybe_unused in case we compile on system
> without suspend support.
>
> I also wanted a bit more of error handling, so I ended up with the
> version of the patch below. Can you please try it and let me know if I
> broke it.

sure, the patch you sent works :)

Tested-by: Jeffy Chen 

>
> Thanks.
>
> -- Dmitry
>
> Input: gpio-keys - add support for wakeup event action
>
> From: Jeffy Chen 
>
> Add support for specifying event actions to trigger wakeup when using
> the gpio-keys input device as a wakeup source.
>
> This would allow the device to configure when to wakeup the system. For
> example a gpio-keys input device for pen insert, may only want to wakeup
> the system when ejecting the pen.
>
> Suggested-by: Brian Norris 
> Signed-off-by: Jeffy Chen 
> Reviewed-by: Rob Herring 
> Signed-off-by: Dmitry Torokhov

Re: [RESEND PATCH v4 1/4] mfd: syscon: Set name of regmap_config


Hi Lee,

On 03/12/2018 05:12 PM, Lee Jones wrote:

On Fri, 09 Mar 2018, Jeffy Chen wrote:


>We are now allowing to register debugfs without a valid device, and not
>having a valid name will end up using "dummy*" to create debugfs dir.
>
>Signed-off-by: Jeffy Chen
>---
>
>Changes in v4: None
>Changes in v3:
>Modify commit message.
>
>  drivers/mfd/syscon.c | 1 +
>  1 file changed, 1 insertion(+)

Is this the RESEND I requested?  I can still only see this patch.

oops, sorry, i was using patman, and i force it to CC everyone in the 
cover letter's CC list, but that doesn't contain your mail address..i'll 
try another way :)

Re: [RESEND PATCH v4 1/4] mfd: syscon: Set name of regmap_config


Hi Lee,

On 03/12/2018 05:12 PM, Lee Jones wrote:

On Fri, 09 Mar 2018, Jeffy Chen wrote:


>We are now allowing to register debugfs without a valid device, and not
>having a valid name will end up using "dummy*" to create debugfs dir.
>
>Signed-off-by: Jeffy Chen
>---
>
>Changes in v4: None
>Changes in v3:
>Modify commit message.
>
>  drivers/mfd/syscon.c | 1 +
>  1 file changed, 1 insertion(+)

Is this the RESEND I requested?  I can still only see this patch.

oops, sorry, i was using patman, and i force it to CC everyone in the 
cover letter's CC list, but that doesn't contain your mail address..i'll 
try another way :)

Re: [PATCH v4 0/4] Set name of regmap_config

2018-03-09 Thread JeffyChen

Hi Lee,

Thanks for your reply.

On 03/09/2018 04:16 PM, Lee Jones wrote:

>
>Jeffy Chen (4):
>   mfd: syscon: Set name of regmap_config
>   rtc: at91sam9: Set name of regmap_config
>   clk: lpc32xx: Set name of regmap_config
>   ARM: rockchip: Set name of regmap_config
>
>  arch/arm/mach-rockchip/platsmp.c | 1 +
>  drivers/clk/nxp/clk-lpc32xx.c| 1 +
>  drivers/mfd/syscon.c | 1 +

It's difficult to review, since we cannot see what's happening with
the rest of the patch set.  Would you mind re-sending the whole set to
all parties please?

sure, i can do that :)

Re: [PATCH v4 0/4] Set name of regmap_config

2018-03-09 Thread JeffyChen

Hi Lee,

Thanks for your reply.

On 03/09/2018 04:16 PM, Lee Jones wrote:

>
>Jeffy Chen (4):
>   mfd: syscon: Set name of regmap_config
>   rtc: at91sam9: Set name of regmap_config
>   clk: lpc32xx: Set name of regmap_config
>   ARM: rockchip: Set name of regmap_config
>
>  arch/arm/mach-rockchip/platsmp.c | 1 +
>  drivers/clk/nxp/clk-lpc32xx.c| 1 +
>  drivers/mfd/syscon.c | 1 +

It's difficult to review, since we cannot see what's happening with
the rest of the patch set.  Would you mind re-sending the whole set to
all parties please?

sure, i can do that :)

Re: [PATCH v3 2/4] rtc: at91sam9: Pass pdev->dev to regmap_init_mmio()

2018-03-08 Thread JeffyChen


Hi Alexandre,

On 03/09/2018 04:49 AM, Alexandre Belloni wrote:

On 08/03/2018 at 18:21:52 +0800, Jeffy Chen wrote:

Otherwise it would use "dummy*" to create regmap debugfs dir.

Signed-off-by: Jeffy Chen 
---

Changes in v3: None

  drivers/rtc/rtc-at91sam9.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c
index 7418a763ce52..0d6a17a77841 100644
--- a/drivers/rtc/rtc-at91sam9.c
+++ b/drivers/rtc/rtc-at91sam9.c
@@ -402,7 +402,7 @@ static int at91_rtc_probe(struct platform_device *pdev)
if (IS_ERR(gpbr))
return PTR_ERR(gpbr);

-   rtc->gpbr = regmap_init_mmio(NULL, gpbr,
+   rtc->gpbr = regmap_init_mmio(>dev, gpbr,
 _regmap_config);


This would be misleading as the gpbr is not directly related to the rtc.

ok, will fix that, thanks.

Re: [PATCH v3 2/4] rtc: at91sam9: Pass pdev->dev to regmap_init_mmio()

2018-03-08 Thread JeffyChen


Hi Alexandre,

On 03/09/2018 04:49 AM, Alexandre Belloni wrote:

On 08/03/2018 at 18:21:52 +0800, Jeffy Chen wrote:

Otherwise it would use "dummy*" to create regmap debugfs dir.

Signed-off-by: Jeffy Chen 
---

Changes in v3: None

  drivers/rtc/rtc-at91sam9.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c
index 7418a763ce52..0d6a17a77841 100644
--- a/drivers/rtc/rtc-at91sam9.c
+++ b/drivers/rtc/rtc-at91sam9.c
@@ -402,7 +402,7 @@ static int at91_rtc_probe(struct platform_device *pdev)
if (IS_ERR(gpbr))
return PTR_ERR(gpbr);

-   rtc->gpbr = regmap_init_mmio(NULL, gpbr,
+   rtc->gpbr = regmap_init_mmio(>dev, gpbr,
 _regmap_config);


This would be misleading as the gpbr is not directly related to the rtc.

ok, will fix that, thanks.

Re: [PATCH v4 1/3] Input: gpio-keys - add support for wakeup event action


Hi Andy,

Thanks for your reply.

On 03/07/2018 07:57 PM, Andy Shevchenko wrote:

On Tue, Mar 6, 2018 at 9:44 AM, Jeffy Chen  wrote:

Add support for specifying event actions to trigger wakeup when using
the gpio-keys input device as a wakeup source.

This would allow the device to configure when to wakeup the system. For
example a gpio-keys input device for pen insert, may only want to wakeup
the system when ejecting the pen.



+   bool wakeup_enabled;



+   if (bdata->wakeup_enabled) {


This is redundant. IRQ core keeps track on this information already.

Check below as an example
aef3ad103a68 ("serial: core: remove unneeded irq_wake flag")


right, that make sense.

Re: [PATCH v4 1/3] Input: gpio-keys - add support for wakeup event action


Hi Andy,

Thanks for your reply.

On 03/07/2018 07:57 PM, Andy Shevchenko wrote:

On Tue, Mar 6, 2018 at 9:44 AM, Jeffy Chen  wrote:

Add support for specifying event actions to trigger wakeup when using
the gpio-keys input device as a wakeup source.

This would allow the device to configure when to wakeup the system. For
example a gpio-keys input device for pen insert, may only want to wakeup
the system when ejecting the pen.



+   bool wakeup_enabled;



+   if (bdata->wakeup_enabled) {


This is redundant. IRQ core keeps track on this information already.

Check below as an example
aef3ad103a68 ("serial: core: remove unneeded irq_wake flag")


right, that make sense.

Re: [PATCH v2 2/3] regmap: debugfs: Fix kmemleak in regmap_debugfs_init


Hi Mark,

Thanks for your reply.

On 03/06/2018 08:59 PM, Mark Brown wrote:

On Tue, Mar 06, 2018 at 07:04:02PM +0800, Jeffy Chen wrote:

Use map->debugfs_name to store allocated debugfs name, so it would be
freed in regmap_debugfs_exit().


I'm missing patch 1 in this series and I think this collides with a fix
I already have locally.


sorry for that, i should CC you the whole seires.

the patch 1 should be:
https://patchwork.kernel.org/patch/10261413

Re: [PATCH v2 2/3] regmap: debugfs: Fix kmemleak in regmap_debugfs_init


Hi Mark,

Thanks for your reply.

On 03/06/2018 08:59 PM, Mark Brown wrote:

On Tue, Mar 06, 2018 at 07:04:02PM +0800, Jeffy Chen wrote:

Use map->debugfs_name to store allocated debugfs name, so it would be
freed in regmap_debugfs_exit().


I'm missing patch 1 in this series and I think this collides with a fix
I already have locally.


sorry for that, i should CC you the whole seires.

the patch 1 should be:
https://patchwork.kernel.org/patch/10261413

Re: [PATCH v2 1/3] mfd: syscon: Set name of regmap_config


Hi Mark,

On 03/07/2018 06:20 PM, Mark Brown wrote:

On Tue, Mar 06, 2018 at 10:58:26PM +0800, JeffyChen wrote:


even this is already fixed by a430ab205d29 ("regmap: debugfs: Disambiguate
dummy debugfs file name")



but maybe we can still have this for a better debugfs name?


That's why the facility is there.

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.


ok, sorry for that :)

recently we are now allowing to register debugfs for syscon regmap, then 
i hit a lot of:

(NULL device *): Failed to create debugfs directory

so i sent this patch to fix that by using the of_node name to create 
debugfs.


but later i found we have:
commit a430ab205d29e7d1537b220fcf989b8080d8267f
Author: Fabio Estevam <fabio.este...@nxp.com>
Date:   Mon Mar 5 15:52:09 2018 -0300

regmap: debugfs: Disambiguate dummy debugfs file name

which would fix that in another way by naming dummy0, dummy1, dummy2.

even if the issue is fixed, i wonder could we still have this patch to 
make it more readable:


+++ b/drivers/mfd/syscon.c
@@ -109,6 +109,7 @@ static struct syscon *of_syscon_register(struct 
device_node *np)

syscon_config.reg_stride = reg_io_width;
syscon_config.val_bits = reg_io_width * 8;
syscon_config.max_register = resource_size() - reg_io_width;
+   syscon_config.name = of_node_full_name(np);




and i've sent 2 more patches to fix the memleak there:
https://patchwork.kernel.org/patch/10261409
https://patchwork.kernel.org/patch/10261411

and sorry again, i forgot to add a cover letter, and i should CC you the 
whole series.

Re: [PATCH v2 1/3] mfd: syscon: Set name of regmap_config


Hi Mark,

On 03/07/2018 06:20 PM, Mark Brown wrote:

On Tue, Mar 06, 2018 at 10:58:26PM +0800, JeffyChen wrote:


even this is already fixed by a430ab205d29 ("regmap: debugfs: Disambiguate
dummy debugfs file name")



but maybe we can still have this for a better debugfs name?


That's why the facility is there.

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.


ok, sorry for that :)

recently we are now allowing to register debugfs for syscon regmap, then 
i hit a lot of:

(NULL device *): Failed to create debugfs directory

so i sent this patch to fix that by using the of_node name to create 
debugfs.


but later i found we have:
commit a430ab205d29e7d1537b220fcf989b8080d8267f
Author: Fabio Estevam 
Date:   Mon Mar 5 15:52:09 2018 -0300

regmap: debugfs: Disambiguate dummy debugfs file name

which would fix that in another way by naming dummy0, dummy1, dummy2.

even if the issue is fixed, i wonder could we still have this patch to 
make it more readable:


+++ b/drivers/mfd/syscon.c
@@ -109,6 +109,7 @@ static struct syscon *of_syscon_register(struct 
device_node *np)

syscon_config.reg_stride = reg_io_width;
syscon_config.val_bits = reg_io_width * 8;
syscon_config.max_register = resource_size() - reg_io_width;
+   syscon_config.name = of_node_full_name(np);




and i've sent 2 more patches to fix the memleak there:
https://patchwork.kernel.org/patch/10261409
https://patchwork.kernel.org/patch/10261411

and sorry again, i forgot to add a cover letter, and i should CC you the 
whole series.

Re: [PATCH v2 1/3] mfd: syscon: Set name of regmap_config

2018-03-06 Thread JeffyChen


+CC Mark.

even this is already fixed by a430ab205d29 ("regmap: debugfs: 
Disambiguate dummy debugfs file name")


but maybe we can still have this for a better debugfs name?

On 03/06/2018 07:04 PM, Jeffy Chen wrote:

We are now allowing to register debugfs for syscon regmap, and not
having a valid name will end up using "dummy" to create debugfs dir.

Fixes: 9b947a13e7f6 ("regmap: use debugfs even when no device")
Signed-off-by: Jeffy Chen 
---

  drivers/mfd/syscon.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 7eaa40bc703f..250d22f40c84 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -109,6 +109,7 @@ static struct syscon *of_syscon_register(struct device_node 
*np)
syscon_config.reg_stride = reg_io_width;
syscon_config.val_bits = reg_io_width * 8;
syscon_config.max_register = resource_size() - reg_io_width;
+   syscon_config.name = of_node_full_name(np);

regmap = regmap_init_mmio(NULL, base, _config);
if (IS_ERR(regmap)) {

Re: [PATCH v2 1/3] mfd: syscon: Set name of regmap_config

2018-03-06 Thread JeffyChen


+CC Mark.

even this is already fixed by a430ab205d29 ("regmap: debugfs: 
Disambiguate dummy debugfs file name")


but maybe we can still have this for a better debugfs name?

On 03/06/2018 07:04 PM, Jeffy Chen wrote:

We are now allowing to register debugfs for syscon regmap, and not
having a valid name will end up using "dummy" to create debugfs dir.

Fixes: 9b947a13e7f6 ("regmap: use debugfs even when no device")
Signed-off-by: Jeffy Chen 
---

  drivers/mfd/syscon.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 7eaa40bc703f..250d22f40c84 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -109,6 +109,7 @@ static struct syscon *of_syscon_register(struct device_node 
*np)
syscon_config.reg_stride = reg_io_width;
syscon_config.val_bits = reg_io_width * 8;
syscon_config.max_register = resource_size() - reg_io_width;
+   syscon_config.name = of_node_full_name(np);

regmap = regmap_init_mmio(NULL, base, _config);
if (IS_ERR(regmap)) {

Re: [PATCH v3 1/3] Input: gpio-keys - add support for wakeup event action


Hi Dmitry,

Thanks for your review.

On 03/06/2018 08:30 AM, Dmitry Torokhov wrote:

>+   switch (button->wakeup_event_action) {
>+   case EV_ACT_ASSERTED:
>+   bdata->wakeup_trigger_type = active_low ?
>+   IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;

IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;


ok, will fix in next version

>+   break;
>+   case EV_ACT_DEASSERTED:
>+   bdata->wakeup_trigger_type = active_low ?
>+   IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
>+   break;

case EV_ACT_ANY:

ok, will fix in next version

>+   default:
>+   /*
>+* For other cases, we are OK letting suspend/resume
>+* not reconfigure the trigger type.
>+*/
>+   break;
>+   }
>} else {
>if (!button->irq) {
>dev_err(dev, "Found button without gpio or irq\n");
>@@ -586,6 +606,12 @@ static int gpio_keys_setup_key(struct platform_device 
*pdev,
>
>isr = gpio_keys_irq_isr;
>irqflags = 0;
>+
>+   /*
>+* For IRQ buttons, the irq trigger type for press and release
>+* are the same. So we don't need to reconfigure the trigger
>+* type for wakeup.

That is not entirely accurate. Interrupt triggers button press, which is
followed by either immediate or delayed release. There is no interrupt
for release.

ok, will fix the comment



>+*/
>}
>
>bdata->code = >keymap[idx];
>@@ -618,6 +644,8 @@ static int gpio_keys_setup_key(struct platform_device 
*pdev,
>return error;
>}
>
>+   bdata->irq_trigger_type = irq_get_trigger_type(bdata->irq);

Why do we need to store the trigger type? It is always both edges for
gpio-based keys and we do not support changing wakeup trigger for
interrupt-based keys.

right, this is not needed.



>+
>return 0;
>  }
>
>@@ -718,6 +746,9 @@ gpio_keys_get_devtree_pdata(struct device *dev)
>/* legacy name */
>fwnode_property_read_bool(child, "gpio-key,wakeup");
>
>+   fwnode_property_read_u32(child, "wakeup-event-action",
>+>wakeup_event_action);
>+
>button->can_disable =
>fwnode_property_read_bool(child, "linux,can-disable");
>
>@@ -854,6 +885,10 @@ static int __maybe_unused gpio_keys_suspend(struct device 
*dev)
>if (device_may_wakeup(dev)) {
>for (i = 0; i < ddata->pdata->nbuttons; i++) {
>struct gpio_button_data *bdata = >data[i];
>+
>+   if (bdata->button->wakeup && bdata->wakeup_trigger_type)
>+   irq_set_irq_type(bdata->irq,
>+bdata->wakeup_trigger_type);

if (bdata->button->wakeup) {
if (bdata->wakeup_trigger_type) {
error = ...;
}

enable_irq_wake(bdata->irq);
}

Might need to be split into a helper; if you add error handling to
enable_irq_wake() that woudl be great too.

ok, will do that.



>if (bdata->button->wakeup)
>enable_irq_wake(bdata->irq);
>bdata->suspended = true;
>@@ -878,6 +913,10 @@ static int __maybe_unused gpio_keys_resume(struct device 
*dev)
>if (device_may_wakeup(dev)) {
>for (i = 0; i < ddata->pdata->nbuttons; i++) {
>struct gpio_button_data *bdata = >data[i];
>+
>+   if (bdata->button->wakeup && bdata->wakeup_trigger_type)
>+   irq_set_irq_type(bdata->irq,
>+bdata->irq_trigger_type);

Just use IRQ_TYPE_EDGE_BOTH.


>if (bdata->button->wakeup)
>disable_irq_wake(bdata->irq);
>bdata->suspended = false;
>diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
>index d06bf77400f1..7160df54a6fe 100644
>--- a/include/linux/gpio_keys.h
>+++ b/include/linux/gpio_keys.h
>@@ -13,6 +13,7 @@ struct device;
>   * @desc: label that will be attached to button's gpio
>   * @type: input event type (%EV_KEY, %EV_SW, %EV_ABS)
>   * @wakeup:   configure the button as a wake-up source
>+ * @wakeup_event_action:   event action to trigger wakeup
>   * @debounce_interval:debounce ticks interval in msecs
>   * @can_disable:  %true indicates that userspace is allowed to
>   *disable button via sysfs
>@@ -26,6 +27,7 @@ struct gpio_keys_button {
>const char *desc;
>unsigned int type;
>int wakeup;
>+   int wakeup_event_action;
>int

Re: [PATCH v3 1/3] Input: gpio-keys - add support for wakeup event action


Hi Dmitry,

Thanks for your review.

On 03/06/2018 08:30 AM, Dmitry Torokhov wrote:

>+   switch (button->wakeup_event_action) {
>+   case EV_ACT_ASSERTED:
>+   bdata->wakeup_trigger_type = active_low ?
>+   IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;

IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;


ok, will fix in next version

>+   break;
>+   case EV_ACT_DEASSERTED:
>+   bdata->wakeup_trigger_type = active_low ?
>+   IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
>+   break;

case EV_ACT_ANY:

ok, will fix in next version

>+   default:
>+   /*
>+* For other cases, we are OK letting suspend/resume
>+* not reconfigure the trigger type.
>+*/
>+   break;
>+   }
>} else {
>if (!button->irq) {
>dev_err(dev, "Found button without gpio or irq\n");
>@@ -586,6 +606,12 @@ static int gpio_keys_setup_key(struct platform_device 
*pdev,
>
>isr = gpio_keys_irq_isr;
>irqflags = 0;
>+
>+   /*
>+* For IRQ buttons, the irq trigger type for press and release
>+* are the same. So we don't need to reconfigure the trigger
>+* type for wakeup.

That is not entirely accurate. Interrupt triggers button press, which is
followed by either immediate or delayed release. There is no interrupt
for release.

ok, will fix the comment



>+*/
>}
>
>bdata->code = >keymap[idx];
>@@ -618,6 +644,8 @@ static int gpio_keys_setup_key(struct platform_device 
*pdev,
>return error;
>}
>
>+   bdata->irq_trigger_type = irq_get_trigger_type(bdata->irq);

Why do we need to store the trigger type? It is always both edges for
gpio-based keys and we do not support changing wakeup trigger for
interrupt-based keys.

right, this is not needed.



>+
>return 0;
>  }
>
>@@ -718,6 +746,9 @@ gpio_keys_get_devtree_pdata(struct device *dev)
>/* legacy name */
>fwnode_property_read_bool(child, "gpio-key,wakeup");
>
>+   fwnode_property_read_u32(child, "wakeup-event-action",
>+>wakeup_event_action);
>+
>button->can_disable =
>fwnode_property_read_bool(child, "linux,can-disable");
>
>@@ -854,6 +885,10 @@ static int __maybe_unused gpio_keys_suspend(struct device 
*dev)
>if (device_may_wakeup(dev)) {
>for (i = 0; i < ddata->pdata->nbuttons; i++) {
>struct gpio_button_data *bdata = >data[i];
>+
>+   if (bdata->button->wakeup && bdata->wakeup_trigger_type)
>+   irq_set_irq_type(bdata->irq,
>+bdata->wakeup_trigger_type);

if (bdata->button->wakeup) {
if (bdata->wakeup_trigger_type) {
error = ...;
}

enable_irq_wake(bdata->irq);
}

Might need to be split into a helper; if you add error handling to
enable_irq_wake() that woudl be great too.

ok, will do that.



>if (bdata->button->wakeup)
>enable_irq_wake(bdata->irq);
>bdata->suspended = true;
>@@ -878,6 +913,10 @@ static int __maybe_unused gpio_keys_resume(struct device 
*dev)
>if (device_may_wakeup(dev)) {
>for (i = 0; i < ddata->pdata->nbuttons; i++) {
>struct gpio_button_data *bdata = >data[i];
>+
>+   if (bdata->button->wakeup && bdata->wakeup_trigger_type)
>+   irq_set_irq_type(bdata->irq,
>+bdata->irq_trigger_type);

Just use IRQ_TYPE_EDGE_BOTH.


>if (bdata->button->wakeup)
>disable_irq_wake(bdata->irq);
>bdata->suspended = false;
>diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
>index d06bf77400f1..7160df54a6fe 100644
>--- a/include/linux/gpio_keys.h
>+++ b/include/linux/gpio_keys.h
>@@ -13,6 +13,7 @@ struct device;
>   * @desc: label that will be attached to button's gpio
>   * @type: input event type (%EV_KEY, %EV_SW, %EV_ABS)
>   * @wakeup:   configure the button as a wake-up source
>+ * @wakeup_event_action:   event action to trigger wakeup
>   * @debounce_interval:debounce ticks interval in msecs
>   * @can_disable:  %true indicates that userspace is allowed to
>   *disable button via sysfs
>@@ -26,6 +27,7 @@ struct gpio_keys_button {
>const char *desc;
>unsigned int type;
>int wakeup;
>+   int wakeup_event_action;
>int

Re: [RESEND PATCH v6 09/14] dt-bindings: iommu/rockchip: Add clock property


Hi Rob,

Thanks for your reply.

On 03/06/2018 10:27 AM, Rob Herring wrote:

On Thu, Mar 01, 2018 at 06:18:32PM +0800, Jeffy Chen wrote:

Add clock property, since we are going to control clocks in rockchip
iommu driver.

Signed-off-by: Jeffy Chen 
---

Changes in v6: None


Really? There was a different number of clocks before.

hmm, right, forgot to modify the change log, sorry...



Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

  Documentation/devicetree/bindings/iommu/rockchip,iommu.txt | 7 +++
  1 file changed, 7 insertions(+)


Reviewed-by: Rob Herring

Re: [RESEND PATCH v6 09/14] dt-bindings: iommu/rockchip: Add clock property


Hi Rob,

Thanks for your reply.

On 03/06/2018 10:27 AM, Rob Herring wrote:

On Thu, Mar 01, 2018 at 06:18:32PM +0800, Jeffy Chen wrote:

Add clock property, since we are going to control clocks in rockchip
iommu driver.

Signed-off-by: Jeffy Chen 
---

Changes in v6: None


Really? There was a different number of clocks before.

hmm, right, forgot to modify the change log, sorry...



Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

  Documentation/devicetree/bindings/iommu/rockchip,iommu.txt | 7 +++
  1 file changed, 7 insertions(+)


Reviewed-by: Rob Herring

Re: [PATCH v9 5/5] drm/bridge/synopsys: dw-hdmi: Add missing bridge detach

2018-03-04 Thread JeffyChen


Hi Laurent,

sorry, you're right, this patch should not be needed.

the connector should be cleanup by 
drm_mode_config_cleanup->drm_connector_put.


i did that in analogix_dp is to avoid a use-after-free issue not 
kmemleak, because the connector was allocated/freed in analogix_dp's 
bind/unbind.


but i found a kmemleak issue(dma_mask not freed) in dw-hdmi when testing 
that, will send patch soon.


On 03/03/2018 08:20 AM, JeffyChen wrote:

Hi Laurent,

On 03/03/2018 05:49 AM, Laurent Pinchart wrote:

Hi Enric,

Thank you for the patch.

On Friday, 2 March 2018 19:57:57 EET Enric Balletbo i Serra wrote:

From: Jeffy Chen <jeffy.c...@rock-chips.com>

We inited connector in attach(), so need a detach() to cleanup.


Do we ? The dw-hdmi driver already sets drm_connector_cleanup() as the
connector .destroy() handler, and the .destroy() operation is called
by the
DRM core. None of the other bridge drivers call drm_connector_cleanup()
directly.


hmmm, checking the code, there are also lots of drivers do the
cleanup(drm_connector_cleanup or funcs->destroy):
drm# grep -r "connector.*funcs->destroy" .
./rockchip/inno_hdmi.c: hdmi->connector.funcs->destroy(>connector);
./rockchip/cdn-dp-core.c:   connector->funcs->destroy(connector);
./bridge/analogix/analogix_dp_core.c:
dp->connector.funcs->destroy(>connector);
./msm/hdmi/hdmi.c: hdmi->connector->funcs->destroy(hdmi->connector);
./msm/dsi/dsi.c: msm_dsi->connector->funcs->destroy(msm_dsi->connector);
./msm/edp/edp.c: edp->connector->funcs->destroy(edp->connector);
./zte/zx_hdmi.c:hdmi->connector.funcs->destroy(>connector);
./drm_connector.c:  connector->funcs->destroy(connector);
./drm_connector.c:  connector->funcs->destroy(connector);
./nouveau/dispnv04/disp.c: connector->funcs->destroy(connector);
./nouveau/nv50_display.c: mstc->connector.funcs->destroy(>connector);
./nouveau/nv50_display.c: connector->funcs->destroy(connector);



when i debug analogix_dp bind/unbind, i found that we need to cleanup
the connector(reported by kmemleak). so i added it to
./bridge/analogix/analogix_dp_core.c...after that i saw dw-hdmi missing
that too(by checking the code), so make this patch.

but i didn't really tested it on devices using dw-hdmi, so i'm not very
sure the dw-hdmi(maybe also other bridges) is the same with analogix_dp.

i can try to find a chromebook veyron to check it next week :)

but even there's a leak, i'm still not very sure about:
should the caller of drm_connector_init cleanup it
or the caller of drm_bridge_attach should do it(for example
analogix_dp_bind/analogix_dp_unbind)
or should the DRM core take care of that?




Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
Signed-off-by: Thierry Escande <thierry.esca...@collabora.com>
Signed-off-by: Enric Balletbo i Serra <enric.balle...@collabora.com>
---

Re: [PATCH v9 5/5] drm/bridge/synopsys: dw-hdmi: Add missing bridge detach

2018-03-04 Thread JeffyChen


Hi Laurent,

sorry, you're right, this patch should not be needed.

the connector should be cleanup by 
drm_mode_config_cleanup->drm_connector_put.


i did that in analogix_dp is to avoid a use-after-free issue not 
kmemleak, because the connector was allocated/freed in analogix_dp's 
bind/unbind.


but i found a kmemleak issue(dma_mask not freed) in dw-hdmi when testing 
that, will send patch soon.


On 03/03/2018 08:20 AM, JeffyChen wrote:

Hi Laurent,

On 03/03/2018 05:49 AM, Laurent Pinchart wrote:

Hi Enric,

Thank you for the patch.

On Friday, 2 March 2018 19:57:57 EET Enric Balletbo i Serra wrote:

From: Jeffy Chen 

We inited connector in attach(), so need a detach() to cleanup.


Do we ? The dw-hdmi driver already sets drm_connector_cleanup() as the
connector .destroy() handler, and the .destroy() operation is called
by the
DRM core. None of the other bridge drivers call drm_connector_cleanup()
directly.


hmmm, checking the code, there are also lots of drivers do the
cleanup(drm_connector_cleanup or funcs->destroy):
drm# grep -r "connector.*funcs->destroy" .
./rockchip/inno_hdmi.c: hdmi->connector.funcs->destroy(>connector);
./rockchip/cdn-dp-core.c:   connector->funcs->destroy(connector);
./bridge/analogix/analogix_dp_core.c:
dp->connector.funcs->destroy(>connector);
./msm/hdmi/hdmi.c: hdmi->connector->funcs->destroy(hdmi->connector);
./msm/dsi/dsi.c: msm_dsi->connector->funcs->destroy(msm_dsi->connector);
./msm/edp/edp.c: edp->connector->funcs->destroy(edp->connector);
./zte/zx_hdmi.c:hdmi->connector.funcs->destroy(>connector);
./drm_connector.c:  connector->funcs->destroy(connector);
./drm_connector.c:  connector->funcs->destroy(connector);
./nouveau/dispnv04/disp.c: connector->funcs->destroy(connector);
./nouveau/nv50_display.c: mstc->connector.funcs->destroy(>connector);
./nouveau/nv50_display.c: connector->funcs->destroy(connector);



when i debug analogix_dp bind/unbind, i found that we need to cleanup
the connector(reported by kmemleak). so i added it to
./bridge/analogix/analogix_dp_core.c...after that i saw dw-hdmi missing
that too(by checking the code), so make this patch.

but i didn't really tested it on devices using dw-hdmi, so i'm not very
sure the dw-hdmi(maybe also other bridges) is the same with analogix_dp.

i can try to find a chromebook veyron to check it next week :)

but even there's a leak, i'm still not very sure about:
should the caller of drm_connector_init cleanup it
or the caller of drm_bridge_attach should do it(for example
analogix_dp_bind/analogix_dp_unbind)
or should the DRM core take care of that?




Signed-off-by: Jeffy Chen 
Signed-off-by: Thierry Escande 
Signed-off-by: Enric Balletbo i Serra 
---

Re: [PATCH v9 5/5] drm/bridge/synopsys: dw-hdmi: Add missing bridge detach

2018-03-02 Thread JeffyChen


Hi Laurent,

On 03/03/2018 05:49 AM, Laurent Pinchart wrote:

Hi Enric,

Thank you for the patch.

On Friday, 2 March 2018 19:57:57 EET Enric Balletbo i Serra wrote:

From: Jeffy Chen 

We inited connector in attach(), so need a detach() to cleanup.


Do we ? The dw-hdmi driver already sets drm_connector_cleanup() as the
connector .destroy() handler, and the .destroy() operation is called by the
DRM core. None of the other bridge drivers call drm_connector_cleanup()
directly.


hmmm, checking the code, there are also lots of drivers do the 
cleanup(drm_connector_cleanup or funcs->destroy):

drm# grep -r "connector.*funcs->destroy" .
./rockchip/inno_hdmi.c: hdmi->connector.funcs->destroy(>connector);
./rockchip/cdn-dp-core.c:   connector->funcs->destroy(connector);
./bridge/analogix/analogix_dp_core.c: 
dp->connector.funcs->destroy(>connector);
./msm/hdmi/hdmi.c: 
hdmi->connector->funcs->destroy(hdmi->connector);
./msm/dsi/dsi.c: 
msm_dsi->connector->funcs->destroy(msm_dsi->connector);
./msm/edp/edp.c: 
edp->connector->funcs->destroy(edp->connector);

./zte/zx_hdmi.c:hdmi->connector.funcs->destroy(>connector);
./drm_connector.c:  connector->funcs->destroy(connector);
./drm_connector.c:  connector->funcs->destroy(connector);
./nouveau/dispnv04/disp.c: 
connector->funcs->destroy(connector);
./nouveau/nv50_display.c: 
mstc->connector.funcs->destroy(>connector);
./nouveau/nv50_display.c: 
connector->funcs->destroy(connector);




when i debug analogix_dp bind/unbind, i found that we need to cleanup 
the connector(reported by kmemleak). so i added it to 
./bridge/analogix/analogix_dp_core.c...after that i saw dw-hdmi missing 
that too(by checking the code), so make this patch.


but i didn't really tested it on devices using dw-hdmi, so i'm not very 
sure the dw-hdmi(maybe also other bridges) is the same with analogix_dp.


i can try to find a chromebook veyron to check it next week :)

but even there's a leak, i'm still not very sure about:
should the caller of drm_connector_init cleanup it
or the caller of drm_bridge_attach should do it(for example 
analogix_dp_bind/analogix_dp_unbind)

or should the DRM core take care of that?




Signed-off-by: Jeffy Chen 
Signed-off-by: Thierry Escande 
Signed-off-by: Enric Balletbo i Serra 
---

Changes in v9: None

  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 8 
  1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index
f9802399cc0d..5626922f95f9 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -1985,6 +1985,13 @@ static int dw_hdmi_bridge_attach(struct drm_bridge
*bridge) return 0;
  }

+static void dw_hdmi_bridge_detach(struct drm_bridge *bridge)
+{
+   struct dw_hdmi *hdmi = bridge->driver_private;
+
+   drm_connector_cleanup(>connector);
+}
+
  static enum drm_mode_status
  dw_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
  const struct drm_display_mode *mode)
@@ -2041,6 +2048,7 @@ static void dw_hdmi_bridge_enable(struct drm_bridge
*bridge)

  static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = {
.attach = dw_hdmi_bridge_attach,
+   .detach = dw_hdmi_bridge_detach,
.enable = dw_hdmi_bridge_enable,
.disable = dw_hdmi_bridge_disable,
.mode_set = dw_hdmi_bridge_mode_set,

Re: [PATCH v9 5/5] drm/bridge/synopsys: dw-hdmi: Add missing bridge detach

2018-03-02 Thread JeffyChen


Hi Laurent,

On 03/03/2018 05:49 AM, Laurent Pinchart wrote:

Hi Enric,

Thank you for the patch.

On Friday, 2 March 2018 19:57:57 EET Enric Balletbo i Serra wrote:

From: Jeffy Chen 

We inited connector in attach(), so need a detach() to cleanup.


Do we ? The dw-hdmi driver already sets drm_connector_cleanup() as the
connector .destroy() handler, and the .destroy() operation is called by the
DRM core. None of the other bridge drivers call drm_connector_cleanup()
directly.


hmmm, checking the code, there are also lots of drivers do the 
cleanup(drm_connector_cleanup or funcs->destroy):

drm# grep -r "connector.*funcs->destroy" .
./rockchip/inno_hdmi.c: hdmi->connector.funcs->destroy(>connector);
./rockchip/cdn-dp-core.c:   connector->funcs->destroy(connector);
./bridge/analogix/analogix_dp_core.c: 
dp->connector.funcs->destroy(>connector);
./msm/hdmi/hdmi.c: 
hdmi->connector->funcs->destroy(hdmi->connector);
./msm/dsi/dsi.c: 
msm_dsi->connector->funcs->destroy(msm_dsi->connector);
./msm/edp/edp.c: 
edp->connector->funcs->destroy(edp->connector);

./zte/zx_hdmi.c:hdmi->connector.funcs->destroy(>connector);
./drm_connector.c:  connector->funcs->destroy(connector);
./drm_connector.c:  connector->funcs->destroy(connector);
./nouveau/dispnv04/disp.c: 
connector->funcs->destroy(connector);
./nouveau/nv50_display.c: 
mstc->connector.funcs->destroy(>connector);
./nouveau/nv50_display.c: 
connector->funcs->destroy(connector);




when i debug analogix_dp bind/unbind, i found that we need to cleanup 
the connector(reported by kmemleak). so i added it to 
./bridge/analogix/analogix_dp_core.c...after that i saw dw-hdmi missing 
that too(by checking the code), so make this patch.


but i didn't really tested it on devices using dw-hdmi, so i'm not very 
sure the dw-hdmi(maybe also other bridges) is the same with analogix_dp.


i can try to find a chromebook veyron to check it next week :)

but even there's a leak, i'm still not very sure about:
should the caller of drm_connector_init cleanup it
or the caller of drm_bridge_attach should do it(for example 
analogix_dp_bind/analogix_dp_unbind)

or should the DRM core take care of that?




Signed-off-by: Jeffy Chen 
Signed-off-by: Thierry Escande 
Signed-off-by: Enric Balletbo i Serra 
---

Changes in v9: None

  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 8 
  1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index
f9802399cc0d..5626922f95f9 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -1985,6 +1985,13 @@ static int dw_hdmi_bridge_attach(struct drm_bridge
*bridge) return 0;
  }

+static void dw_hdmi_bridge_detach(struct drm_bridge *bridge)
+{
+   struct dw_hdmi *hdmi = bridge->driver_private;
+
+   drm_connector_cleanup(>connector);
+}
+
  static enum drm_mode_status
  dw_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
  const struct drm_display_mode *mode)
@@ -2041,6 +2048,7 @@ static void dw_hdmi_bridge_enable(struct drm_bridge
*bridge)

  static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = {
.attach = dw_hdmi_bridge_attach,
+   .detach = dw_hdmi_bridge_detach,
.enable = dw_hdmi_bridge_enable,
.disable = dw_hdmi_bridge_disable,
.mode_set = dw_hdmi_bridge_mode_set,

Re: [PATCH v2 1/3] Input: gpio-keys - add support for wakeup event action

Hi Brain,

Thanks for your reply.

On 03/02/2018 10:32 AM, Brian Norris wrote:

> >What about the 'else' case? Shouldn't we try to handle that?

>i think the else case is for irq key, which would generate down and up
>events in one irq, so it would use the same trigger type for all these 3
>cases.

Not necessarily. It uses whatever trigger was provided in
platform/DT/etc. data. You could retrieve that with
irq_get_trigger_type() and try to interpret that. Or you could just
outlaw using that combination (e.g., in the binding documentation).
i think for the IRQ button case the assert/deassert/any are using the 
same irq trigger type, so it should be ok to leave the wakeup trigger 
type to be 0(not reconfigure the trigger type)...

i've made a v3 to add a comment about that, but forgot to send it :(

Brian

Re: [PATCH v2 1/3] Input: gpio-keys - add support for wakeup event action

Hi Brain,

Thanks for your reply.

On 03/02/2018 10:32 AM, Brian Norris wrote:

> >What about the 'else' case? Shouldn't we try to handle that?

>i think the else case is for irq key, which would generate down and up
>events in one irq, so it would use the same trigger type for all these 3
>cases.

Not necessarily. It uses whatever trigger was provided in
platform/DT/etc. data. You could retrieve that with
irq_get_trigger_type() and try to interpret that. Or you could just
outlaw using that combination (e.g., in the binding documentation).
i think for the IRQ button case the assert/deassert/any are using the 
same irq trigger type, so it should be ok to leave the wakeup trigger 
type to be 0(not reconfigure the trigger type)...

i've made a v3 to add a comment about that, but forgot to send it :(

Brian

Re: [RESEND PATCH v6 14/14] iommu/rockchip: Support sharing IOMMU between masters


Hi Robin,

On 03/01/2018 07:03 PM, Robin Murphy wrote:


+static struct iommu_group *rk_iommu_device_group(struct device *dev)
+{
+struct rk_iommu *iommu;
+
+iommu = rk_iommu_from_dev(dev);
+
+return iommu->group;


Oops, seems I overlooked this in my previous review - it should really be:

 return iommu_group_get(iommu->group);

or the refcounting will be unbalanced on those future systems where it
really will be called more than once.


hmmm, right, it should be return iommu_group_ref_get(iommu->group);

i was following omap iommu:
static struct iommu_group *omap_iommu_device_group(struct device *dev)
{
struct omap_iommu_arch_data *arch_data = dev->archdata.iommu;
struct iommu_group *group = ERR_PTR(-EINVAL);

if (arch_data->iommu_dev)
group = arch_data->iommu_dev->group;

return group;
}

will fix it too in the version.


Robin.

Re: [RESEND PATCH v6 14/14] iommu/rockchip: Support sharing IOMMU between masters


Hi Robin,

On 03/01/2018 07:03 PM, Robin Murphy wrote:


+static struct iommu_group *rk_iommu_device_group(struct device *dev)
+{
+struct rk_iommu *iommu;
+
+iommu = rk_iommu_from_dev(dev);
+
+return iommu->group;


Oops, seems I overlooked this in my previous review - it should really be:

 return iommu_group_get(iommu->group);

or the refcounting will be unbalanced on those future systems where it
really will be called more than once.


hmmm, right, it should be return iommu_group_ref_get(iommu->group);

i was following omap iommu:
static struct iommu_group *omap_iommu_device_group(struct device *dev)
{
struct omap_iommu_arch_data *arch_data = dev->archdata.iommu;
struct iommu_group *group = ERR_PTR(-EINVAL);

if (arch_data->iommu_dev)
group = arch_data->iommu_dev->group;

return group;
}

will fix it too in the version.


Robin.

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

Hi Geert,

Thanks for your reply.

On 03/01/2018 04:33 PM, Geert Uytterhoeven wrote:

>so maybe we can:
>1/ let the device(dts) or driver decide which clock is PM clk, and add it
>using*pm_clk_add*  APIs (even of_pm_clk_add_clks() if all clocks are pm clk)
>
>2/ add support for critical PM clk, which would return error to the driver
>if anything wrong
>
>3/ make sure PM clk always be controlled(otherwise it might be unexpected
>disabled by other clocks under the same clk parent?):
>  a) make sure Runtime PM is always enabled. and as discussed, we can select
>PM in ARCH_ROCKCHIP

On Renesas SoCs, we only add the device's module clock with pm_clk_add().
Drivers that don't care about properties of the module clock just call
pm_runtime_*(). That way the same driver works on different SoCs using
the same device, with and without power and/or clock domains.

well, i think we may need to check:
1/ so the driver might not know is the clock really enabled(currently):

static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
{
if (!ce->clk)
ce->clk = clk_get(dev, ce->con_id);
if (IS_ERR(ce->clk)) {
ce->status = PCE_STATUS_ERROR;

static inline void __pm_clk_enable(struct device *dev, struct 
pm_clock_entry *ce)

{
int ret;

if (ce->status < PCE_STATUS_ERROR) {
ret = clk_enable(ce->clk);
if (!ret)
ce->status = PCE_STATUS_ENABLED;

it seems the status is private.

2/ the pm_clk_resume/suspend seems only be called in the domain's 
suspend/resume ops(or USE_PM_CLK_RUNTIME_OPS, or called directly in the 
drivers for example tegra-aconnect):

# cgrep pm_clk_resume -w
./include/linux/pm_clock.h:50:extern int pm_clk_resume(struct device *dev);
./include/linux/pm_clock.h:83:#define pm_clk_resume NULL
./drivers/bus/tegra-aconnect.c:62:  return pm_clk_resume(dev);
./drivers/dma/tegra210-adma.c:649:  ret = pm_clk_resume(dev);
./drivers/base/power/clock_ops.c:421: * pm_clk_resume - Enable clocks in 
a device's PM clock list.

./drivers/base/power/clock_ops.c:424:int pm_clk_resume(struct device *dev)
./drivers/base/power/clock_ops.c:444:EXPORT_SYMBOL_GPL(pm_clk_resume);
./drivers/base/power/clock_ops.c:533:   ret = pm_clk_resume(dev);
./drivers/base/power/domain.c:1685: genpd->dev_ops.start = 
pm_clk_resume;

./drivers/irqchip/irq-gic-pm.c:36:  ret = pm_clk_resume(dev);

so if the device doesn't have a power domain, the PM clks could be 
unmanaged right?

by the way, the tegra drivers(tegra-aconnect/tegra210-adma) seems like 
good examples of using current PM clks...

but anyway, force adding all clocks as PM clks in the rockchip power 
domain driver seems wrong...

Drivers that care about properties of the module clock (mainly frequency)
can still use the clk_*() API for that. Other (optional) clocks must be
handled by the device driver itself.

Gr{oetje,eeting}s,

 Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 --ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 -- Linus Torvalds

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

Hi Geert,

Thanks for your reply.

On 03/01/2018 04:33 PM, Geert Uytterhoeven wrote:

>so maybe we can:
>1/ let the device(dts) or driver decide which clock is PM clk, and add it
>using*pm_clk_add*  APIs (even of_pm_clk_add_clks() if all clocks are pm clk)
>
>2/ add support for critical PM clk, which would return error to the driver
>if anything wrong
>
>3/ make sure PM clk always be controlled(otherwise it might be unexpected
>disabled by other clocks under the same clk parent?):
>  a) make sure Runtime PM is always enabled. and as discussed, we can select
>PM in ARCH_ROCKCHIP

On Renesas SoCs, we only add the device's module clock with pm_clk_add().
Drivers that don't care about properties of the module clock just call
pm_runtime_*(). That way the same driver works on different SoCs using
the same device, with and without power and/or clock domains.

well, i think we may need to check:
1/ so the driver might not know is the clock really enabled(currently):

static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
{
if (!ce->clk)
ce->clk = clk_get(dev, ce->con_id);
if (IS_ERR(ce->clk)) {
ce->status = PCE_STATUS_ERROR;

static inline void __pm_clk_enable(struct device *dev, struct 
pm_clock_entry *ce)

{
int ret;

if (ce->status < PCE_STATUS_ERROR) {
ret = clk_enable(ce->clk);
if (!ret)
ce->status = PCE_STATUS_ENABLED;

it seems the status is private.

2/ the pm_clk_resume/suspend seems only be called in the domain's 
suspend/resume ops(or USE_PM_CLK_RUNTIME_OPS, or called directly in the 
drivers for example tegra-aconnect):

# cgrep pm_clk_resume -w
./include/linux/pm_clock.h:50:extern int pm_clk_resume(struct device *dev);
./include/linux/pm_clock.h:83:#define pm_clk_resume NULL
./drivers/bus/tegra-aconnect.c:62:  return pm_clk_resume(dev);
./drivers/dma/tegra210-adma.c:649:  ret = pm_clk_resume(dev);
./drivers/base/power/clock_ops.c:421: * pm_clk_resume - Enable clocks in 
a device's PM clock list.

./drivers/base/power/clock_ops.c:424:int pm_clk_resume(struct device *dev)
./drivers/base/power/clock_ops.c:444:EXPORT_SYMBOL_GPL(pm_clk_resume);
./drivers/base/power/clock_ops.c:533:   ret = pm_clk_resume(dev);
./drivers/base/power/domain.c:1685: genpd->dev_ops.start = 
pm_clk_resume;

./drivers/irqchip/irq-gic-pm.c:36:  ret = pm_clk_resume(dev);

so if the device doesn't have a power domain, the PM clks could be 
unmanaged right?

by the way, the tegra drivers(tegra-aconnect/tegra210-adma) seems like 
good examples of using current PM clks...

but anyway, force adding all clocks as PM clks in the rockchip power 
domain driver seems wrong...

Drivers that care about properties of the module clock (mainly frequency)
can still use the clk_*() API for that. Other (optional) clocks must be
handled by the device driver itself.

Gr{oetje,eeting}s,

 Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 --ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 -- Linus Torvalds

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks


Hi guys,

if i'm reading the code right, the PM clk means:
1/ the clocks which would be enabled while power on
2/ these clocks are optional, it's ok if anything wrong with them
3/ controlled by pm_domain(or USE_PM_CLK_RUNTIME_OPS & pm_clk_add_notifier)

and currently we're adding all clocks of the attached device as PM clk 
in rockchip PM domain driver, which seems wrong. because we might have 
these kinds of clocks:
1/ critical, should block power on if anything wrong with it(failed to 
get/ prepare/ enable)

2/ optional, could ignore it if anything wrong
3/ only required in some special cases, for example register r/w, and 
doesn't need to stay enabled while power on


so maybe we can:
1/ let the device(dts) or driver decide which clock is PM clk, and add 
it using *pm_clk_add* APIs (even of_pm_clk_add_clks() if all clocks are 
pm clk)


2/ add support for critical PM clk, which would return error to the 
driver if anything wrong


3/ make sure PM clk always be controlled(otherwise it might be 
unexpected disabled by other clocks under the same clk parent?):
 a) make sure Runtime PM is always enabled. and as discussed, we can 
select PM in ARCH_ROCKCHIP


 b) make sure the device has a PM domain to control PM clk:
select ROCKCHIP_PM_DOMAINS for ARCH_ROCKCHIP(that would also select 
PM_GENERIC_DOMAINS)


check dev->pm_domain before hand over PM clk, since we only care about 
EPROBE_DEFER when attach PM domain:

ret = dev_pm_domain_attach(_dev, true);
if (ret != -EPROBE_DEFER) {
if (drv->probe) {
ret = drv->probe(dev);

or

 c) make pm_clk_suspend/pm_clk_resume part of the generic PM Runtime 
flow(even without a PM domain)



On 02/28/2018 10:07 PM, Tomasz Figa wrote:

On Wed, Feb 28, 2018 at 10:11 PM, Geert Uytterhoeven
 wrote:

Hi Tomasz,

On Wed, Feb 28, 2018 at 1:49 PM, Tomasz Figa  wrote:

On Wed, Feb 28, 2018 at 9:32 PM, Geert Uytterhoeven
 wrote:

On Wed, Feb 28, 2018 at 1:29 PM, Tomasz Figa  wrote:

Also, how about systems where runtime PM is disabled? I think that's
one of the reasons we control the clocks explicitly in the drivers
anyway.


On many platforms, Runtime PM is always enabled.


Can we make such assumption? If so, could we just make an explicit
"select PM_RUNTIME" in Kconfig of the SoC?


Note that the PM_RUNTIME symbol was removed in commit 464ed18ebdb6236f
("PM: Eliminate CONFIG_PM_RUNTIME"), in favor of plain PM.

The following already select PM unconditionally:
   - ARCH_OMAP2PLUS_TYPICAL
   - ARCH_RENESAS (except EMEV2)
   - ARCH_TEGRA
   - ARCH_VEXPRESS


Thanks! Sounds like we might be able to simplify a lot of things with
doing the same for Rockchip.

Best regards,
Tomasz

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks


Hi guys,

if i'm reading the code right, the PM clk means:
1/ the clocks which would be enabled while power on
2/ these clocks are optional, it's ok if anything wrong with them
3/ controlled by pm_domain(or USE_PM_CLK_RUNTIME_OPS & pm_clk_add_notifier)

and currently we're adding all clocks of the attached device as PM clk 
in rockchip PM domain driver, which seems wrong. because we might have 
these kinds of clocks:
1/ critical, should block power on if anything wrong with it(failed to 
get/ prepare/ enable)

2/ optional, could ignore it if anything wrong
3/ only required in some special cases, for example register r/w, and 
doesn't need to stay enabled while power on


so maybe we can:
1/ let the device(dts) or driver decide which clock is PM clk, and add 
it using *pm_clk_add* APIs (even of_pm_clk_add_clks() if all clocks are 
pm clk)


2/ add support for critical PM clk, which would return error to the 
driver if anything wrong


3/ make sure PM clk always be controlled(otherwise it might be 
unexpected disabled by other clocks under the same clk parent?):
 a) make sure Runtime PM is always enabled. and as discussed, we can 
select PM in ARCH_ROCKCHIP


 b) make sure the device has a PM domain to control PM clk:
select ROCKCHIP_PM_DOMAINS for ARCH_ROCKCHIP(that would also select 
PM_GENERIC_DOMAINS)


check dev->pm_domain before hand over PM clk, since we only care about 
EPROBE_DEFER when attach PM domain:

ret = dev_pm_domain_attach(_dev, true);
if (ret != -EPROBE_DEFER) {
if (drv->probe) {
ret = drv->probe(dev);

or

 c) make pm_clk_suspend/pm_clk_resume part of the generic PM Runtime 
flow(even without a PM domain)



On 02/28/2018 10:07 PM, Tomasz Figa wrote:

On Wed, Feb 28, 2018 at 10:11 PM, Geert Uytterhoeven
 wrote:

Hi Tomasz,

On Wed, Feb 28, 2018 at 1:49 PM, Tomasz Figa  wrote:

On Wed, Feb 28, 2018 at 9:32 PM, Geert Uytterhoeven
 wrote:

On Wed, Feb 28, 2018 at 1:29 PM, Tomasz Figa  wrote:

Also, how about systems where runtime PM is disabled? I think that's
one of the reasons we control the clocks explicitly in the drivers
anyway.


On many platforms, Runtime PM is always enabled.


Can we make such assumption? If so, could we just make an explicit
"select PM_RUNTIME" in Kconfig of the SoC?


Note that the PM_RUNTIME symbol was removed in commit 464ed18ebdb6236f
("PM: Eliminate CONFIG_PM_RUNTIME"), in favor of plain PM.

The following already select PM unconditionally:
   - ARCH_OMAP2PLUS_TYPICAL
   - ARCH_RENESAS (except EMEV2)
   - ARCH_TEGRA
   - ARCH_VEXPRESS


Thanks! Sounds like we might be able to simplify a lot of things with
doing the same for Rockchip.

Best regards,
Tomasz

Re: [PATCH v5 08/13] iommu/rockchip: Control clocks needed to access the IOMMU

Hi Robin,

Thanks for your reply.

On 02/28/2018 11:06 PM, Robin Murphy wrote:

On 28/02/18 13:00, JeffyChen wrote:

Hi Robin,

Thanks for your reply.

On 02/28/2018 12:59 AM, Robin Murphy wrote:

the rockchip IOMMU is part of the master block in hardware, so it
needs
to control the master's power domain and some of the master's clocks
when access it's registers.

and the number of clocks needed here, might be different between each
IOMMUs(according to which master block it belongs), it's a little like
our power domain:
https://elixir.free-electrons.com/linux/latest/source/arch/arm64/boot/dts/rockchip/rk3399.dtsi#L935

i'm not sure how to describe this correctly, is it ok use something
like
"the same as it's master block"?

would it make sense to add a property to specify the master who owns
the iommu, and we can get all clocks(only some of those clocks are
actually needed) from it in the of_xlate()? and we can also reuse the
clock-names of that master to build clk_bulk_data and log errors in
clk_bulk_get.

I'm inclined to agree with Rob here - if we're to add anything to the
binding, it should only be whatever clock inputs are defined for the
IOMMU IP block itself. If Linux doesn't properly handle the interconnect
clock hierarchy external to a particular integration, that's a separate
issue and it's not the binding's problem.

I actually quite like the hack of "borrowing" the clocks from
dev->of_node in of_xlate() - you shouldn't need any DT changes for that,
because you already know that each IOMMU instance only has the one
master device anyway.

Thanks:) but actually we are going to support sharing IOMMU between
multiple masters(one of them is the main master i think) in the newer
chips(not yet supported on upstream kernel)...

Ha! OK, fair enough, back to the first point then...

So we might have to get all clocks from all masters, or find a way to
specify the main master...and for the multiple masters case, do it in
of_xlate() turns out to be a little racy...maybe we can add a property
to specify main master, and get it's clocks in probe()?

I notice that the 4.4 BSP kernel consistently specifies "aclk" and
"hclk" for the IOMMU instances - it feels unusual to say "why don't we
follow the downstream binding?", but it does look a lot like what I
would expect (I'd guess at one for the register slave interface and one
for the master interface/general operation?)

huh, right.
i did noticed that, but there's a hevc_mmu with ("aclk", "hclk",
"clk_core", "clk_cabac") confused me.

so confirmed with Simon, that hevc_mmu is wrong. currently all IOMMUs
should only have 2 clks, either aclk+hclk or aclk+pclk (depends on the
clk tree)

so it seems to be a good idea to do so, will send patches soon, thanks :)

If we can implement conceptually-correct clock handling based on an
accurate binding, which should cover most cases, and *then* look at
hacking around those where it doesn't quite work in practice due to
shortcomings elsewhere, that would be ideal, and of course a lot nicer
than just jumping straight into piles of hacks.

Robin.

Re: [PATCH v5 08/13] iommu/rockchip: Control clocks needed to access the IOMMU

Hi Robin,

Thanks for your reply.

On 02/28/2018 11:06 PM, Robin Murphy wrote:

On 28/02/18 13:00, JeffyChen wrote:

Hi Robin,

Thanks for your reply.

On 02/28/2018 12:59 AM, Robin Murphy wrote:

the rockchip IOMMU is part of the master block in hardware, so it
needs
to control the master's power domain and some of the master's clocks
when access it's registers.

i'm not sure how to describe this correctly, is it ok use something
like
"the same as it's master block"?

Thanks:) but actually we are going to support sharing IOMMU between
multiple masters(one of them is the main master i think) in the newer
chips(not yet supported on upstream kernel)...

Ha! OK, fair enough, back to the first point then...

huh, right.
i did noticed that, but there's a hevc_mmu with ("aclk", "hclk",
"clk_core", "clk_cabac") confused me.

so confirmed with Simon, that hevc_mmu is wrong. currently all IOMMUs
should only have 2 clks, either aclk+hclk or aclk+pclk (depends on the
clk tree)

so it seems to be a good idea to do so, will send patches soon, thanks :)

Robin.

Re: [PATCH v5 08/13] iommu/rockchip: Control clocks needed to access the IOMMU


Hi Robin,

Thanks for your reply.

On 02/28/2018 12:59 AM, Robin Murphy wrote:

the rockchip IOMMU is part of the master block in hardware, so it needs
to control the master's power domain and some of the master's clocks
when access it's registers.

and the number of clocks needed here, might be different between each
IOMMUs(according to which master block it belongs), it's a little like
our power domain:
https://elixir.free-electrons.com/linux/latest/source/arch/arm64/boot/dts/rockchip/rk3399.dtsi#L935



i'm not sure how to describe this correctly, is it ok use something like
"the same as it's master block"?


would it make sense to add a property to specify the master who owns
the iommu, and we can get all clocks(only some of those clocks are
actually needed) from it in the of_xlate()? and we can also reuse the
clock-names of that master to build clk_bulk_data and log errors in
clk_bulk_get.


I'm inclined to agree with Rob here - if we're to add anything to the
binding, it should only be whatever clock inputs are defined for the
IOMMU IP block itself. If Linux doesn't properly handle the interconnect
clock hierarchy external to a particular integration, that's a separate
issue and it's not the binding's problem.

I actually quite like the hack of "borrowing" the clocks from
dev->of_node in of_xlate() - you shouldn't need any DT changes for that,
because you already know that each IOMMU instance only has the one
master device anyway.


Thanks:) but actually we are going to support sharing IOMMU between 
multiple masters(one of them is the main master i think) in the newer 
chips(not yet supported on upstream kernel)...


So we might have to get all clocks from all masters, or find a way to 
specify the main master...and for the multiple masters case, do it in 
of_xlate() turns out to be a little racy...maybe we can add a property 
to specify main master, and get it's clocks in probe()?




Robin.

Re: [PATCH v5 08/13] iommu/rockchip: Control clocks needed to access the IOMMU


Hi Robin,

Thanks for your reply.

On 02/28/2018 12:59 AM, Robin Murphy wrote:

the rockchip IOMMU is part of the master block in hardware, so it needs
to control the master's power domain and some of the master's clocks
when access it's registers.

and the number of clocks needed here, might be different between each
IOMMUs(according to which master block it belongs), it's a little like
our power domain:
https://elixir.free-electrons.com/linux/latest/source/arch/arm64/boot/dts/rockchip/rk3399.dtsi#L935



i'm not sure how to describe this correctly, is it ok use something like
"the same as it's master block"?


would it make sense to add a property to specify the master who owns
the iommu, and we can get all clocks(only some of those clocks are
actually needed) from it in the of_xlate()? and we can also reuse the
clock-names of that master to build clk_bulk_data and log errors in
clk_bulk_get.


I'm inclined to agree with Rob here - if we're to add anything to the
binding, it should only be whatever clock inputs are defined for the
IOMMU IP block itself. If Linux doesn't properly handle the interconnect
clock hierarchy external to a particular integration, that's a separate
issue and it's not the binding's problem.

I actually quite like the hack of "borrowing" the clocks from
dev->of_node in of_xlate() - you shouldn't need any DT changes for that,
because you already know that each IOMMU instance only has the one
master device anyway.


Thanks:) but actually we are going to support sharing IOMMU between 
multiple masters(one of them is the main master i think) in the newer 
chips(not yet supported on upstream kernel)...


So we might have to get all clocks from all masters, or find a way to 
specify the main master...and for the multiple masters case, do it in 
of_xlate() turns out to be a little racy...maybe we can add a property 
to specify main master, and get it's clocks in probe()?




Robin.

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks


Hi Geert,

Thanks for you reply.

On 02/28/2018 08:17 PM, Geert Uytterhoeven wrote:

Hi Jeffy,

On Wed, Feb 28, 2018 at 12:11 PM, Jeffy Chen  wrote:

Currently we are adding all of the attached devices' clocks as pm clocks
and enable them when powering on the power domain.

This seems unnecessary, because those clocks are already controlled in
the devices' drivers with better error handling.

Tested on my chromebook minnie(rk3288) and chromebook kevin(rk3399).

Signed-off-by: Jeffy Chen 


Thanks for your patch!

Just wondering: so you prefer to handle the clocks explicitly in all drivers,
instead of delegating this task to Runtime PM?
hmmm, i think we should control PM clks here, but not all clocks...at 
least some of the clocks are not required to be enabled while pd power 
on(waste power?).


and seems the drivers might have better control for error 
handling(decide to ignore or fail to probe or else).


also Runtime PM seems optional(could be disabled in config), and some 
devices(or even chips) don't have PM.




Gr{oetje,eeting}s,

 Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 -- Linus Torvalds

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks


Hi Geert,

Thanks for you reply.

On 02/28/2018 08:17 PM, Geert Uytterhoeven wrote:

Hi Jeffy,

On Wed, Feb 28, 2018 at 12:11 PM, Jeffy Chen  wrote:

Currently we are adding all of the attached devices' clocks as pm clocks
and enable them when powering on the power domain.

This seems unnecessary, because those clocks are already controlled in
the devices' drivers with better error handling.

Tested on my chromebook minnie(rk3288) and chromebook kevin(rk3399).

Signed-off-by: Jeffy Chen 


Thanks for your patch!

Just wondering: so you prefer to handle the clocks explicitly in all drivers,
instead of delegating this task to Runtime PM?
hmmm, i think we should control PM clks here, but not all clocks...at 
least some of the clocks are not required to be enabled while pd power 
on(waste power?).


and seems the drivers might have better control for error 
handling(decide to ignore or fail to probe or else).


also Runtime PM seems optional(could be disabled in config), and some 
devices(or even chips) don't have PM.




Gr{oetje,eeting}s,

 Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 -- Linus Torvalds

Re: [PATCH] rtc: cros-ec: return -ETIME when refused to set alarms in the past

Hi Brian,

Thanks for your reply.

On 02/27/2018 02:37 AM, Brian Norris wrote:

>+   /* Don't set an alarm in the past. */
>+   if ((u32)alarm_time <= current_time)
>+   return -ETIME;
>+
>if (!alrm->enabled) {
>/*
> * If the alarm is being disabled, send an alarm
>@@ -196,11 +200,7 @@ static int cros_ec_rtc_set_alarm(struct device *dev, 
struct rtc_wkalrm *alrm)
>alarm_offset = EC_RTC_ALARM_CLEAR;
>cros_ec_rtc->saved_alarm = (u32)alarm_time;
>} else {
>-   /* Don't set an alarm in the past. */
>-   if ((u32)alarm_time < current_time)

It's probably worth noting in the commit message that you're also fixing
the case where 'alarm_time == current_time'; in the current driver
source, it*looks*  like you're setting a 0-second alarm. But in fact, 0
means EC_RTC_ALARM_CLEAR, which would disable the alarm. So you are
(correctly) returning -ETIME in that case.

Right, i'll rewrite the commit message, and move the check back here:)

Brian

Re: [PATCH] rtc: cros-ec: return -ETIME when refused to set alarms in the past

Hi Brian,

Thanks for your reply.

On 02/27/2018 02:37 AM, Brian Norris wrote:

>+   /* Don't set an alarm in the past. */
>+   if ((u32)alarm_time <= current_time)
>+   return -ETIME;
>+
>if (!alrm->enabled) {
>/*
> * If the alarm is being disabled, send an alarm
>@@ -196,11 +200,7 @@ static int cros_ec_rtc_set_alarm(struct device *dev, 
struct rtc_wkalrm *alrm)
>alarm_offset = EC_RTC_ALARM_CLEAR;
>cros_ec_rtc->saved_alarm = (u32)alarm_time;
>} else {
>-   /* Don't set an alarm in the past. */
>-   if ((u32)alarm_time < current_time)

It's probably worth noting in the commit message that you're also fixing
the case where 'alarm_time == current_time'; in the current driver
source, it*looks*  like you're setting a 0-second alarm. But in fact, 0
means EC_RTC_ALARM_CLEAR, which would disable the alarm. So you are
(correctly) returning -ETIME in that case.

Right, i'll rewrite the commit message, and move the check back here:)

Brian

Re: [PATCH v3] rtc: cros-ec: return -ETIME when refused to set alarms in the past


Hi guys,

On 02/27/2018 10:47 AM, Jeffy Chen wrote:

/* Don't set an alarm in the past. */
if ((u32)alarm_time < current_time)

Oops, i'm a idiot, forgot to use '<='... will resend it.

-   alarm_offset = EC_RTC_ALARM_CLEAR;
-   else
-   alarm_offset = (u32)alarm_time - current_time;
+   return -ETIME;
+
+   alarm_offset = (u32)alarm_time - current_time;
}

Re: [PATCH v3] rtc: cros-ec: return -ETIME when refused to set alarms in the past


Hi guys,

On 02/27/2018 10:47 AM, Jeffy Chen wrote:

/* Don't set an alarm in the past. */
if ((u32)alarm_time < current_time)

Oops, i'm a idiot, forgot to use '<='... will resend it.

-   alarm_offset = EC_RTC_ALARM_CLEAR;
-   else
-   alarm_offset = (u32)alarm_time - current_time;
+   return -ETIME;
+
+   alarm_offset = (u32)alarm_time - current_time;
}

Re: [PATCH v4 08/13] iommu/rockchip: Control clocks needed to access the IOMMU


Hi guys,

On 01/25/2018 06:24 PM, JeffyChen wrote:

On 01/25/2018 05:42 PM, Randy Li wrote:




confirmed with Simon, there might be some iommus don't have a pd, and

We use the pd to control the NIU node(not on upstream), without a pd or
fake pd, none of the platform would work.

after talked offline, it's possible to have iommu without pd in upstream
kernel(and chromeos kernel), but on our internal kernel, the drivers
would require pd(or fake pd) to reset modules when error happens.

anyway, i think that means we do need clock control here.


found another reason to not depend on pd to control clocks.

currently we are using pd's pm_clk to keep clocks enabled during power 
on. but in our pd binding doc, that is not needed:
- clocks (optional): phandles to clocks which need to be enabled while 
power domain

switches state.

confirmed with Caesar, the pm_clk only required for some old 
chips(rk3288 for example) due to hardware issue.


and i tested my chromebook kevin(rk3399), it works well after remove the 
pm_clk:


+++ b/drivers/soc/rockchip/pm_domains.c
@@ -478,7 +478,6 @@ static int rockchip_pm_add_one_domain(struct 
rockchip_pmu *pmu,

pd->genpd.power_on = rockchip_pd_power_on;
pd->genpd.attach_dev = rockchip_pd_attach_dev;
pd->genpd.detach_dev = rockchip_pd_detach_dev;
-   pd->genpd.flags = GENPD_FLAG_PM_CLK;

will do more tests and send patch tomorrow.




the CONFIG_PM could be disabled.I am hard to believe a modern platform
can work without that.

so it might be better to control clocks in iommu driver itself.

I won't
insist how the version of the iommu patch on the upstream, I
just post an idea here.
The version for kernel 4.4 is under internal review, the implementation
has been modified many times.

I would suggest the managing clocks in pd is a more easy way and don't
need to spare those thing in two places.

Re: [PATCH v4 08/13] iommu/rockchip: Control clocks needed to access the IOMMU


Hi guys,

On 01/25/2018 06:24 PM, JeffyChen wrote:

On 01/25/2018 05:42 PM, Randy Li wrote:




confirmed with Simon, there might be some iommus don't have a pd, and

We use the pd to control the NIU node(not on upstream), without a pd or
fake pd, none of the platform would work.

after talked offline, it's possible to have iommu without pd in upstream
kernel(and chromeos kernel), but on our internal kernel, the drivers
would require pd(or fake pd) to reset modules when error happens.

anyway, i think that means we do need clock control here.


found another reason to not depend on pd to control clocks.

currently we are using pd's pm_clk to keep clocks enabled during power 
on. but in our pd binding doc, that is not needed:
- clocks (optional): phandles to clocks which need to be enabled while 
power domain

switches state.

confirmed with Caesar, the pm_clk only required for some old 
chips(rk3288 for example) due to hardware issue.


and i tested my chromebook kevin(rk3399), it works well after remove the 
pm_clk:


+++ b/drivers/soc/rockchip/pm_domains.c
@@ -478,7 +478,6 @@ static int rockchip_pm_add_one_domain(struct 
rockchip_pmu *pmu,

pd->genpd.power_on = rockchip_pd_power_on;
pd->genpd.attach_dev = rockchip_pd_attach_dev;
pd->genpd.detach_dev = rockchip_pd_detach_dev;
-   pd->genpd.flags = GENPD_FLAG_PM_CLK;

will do more tests and send patch tomorrow.




the CONFIG_PM could be disabled.I am hard to believe a modern platform
can work without that.

so it might be better to control clocks in iommu driver itself.

I won't
insist how the version of the iommu patch on the upstream, I
just post an idea here.
The version for kernel 4.4 is under internal review, the implementation
has been modified many times.

I would suggest the managing clocks in pd is a more easy way and don't
need to spare those thing in two places.

Re: [PATCH v2 1/3] Input: gpio-keys - add support for wakeup event action


Hi Enric,

Thanks for your reply.

On 02/14/2018 06:25 AM, Enric Balletbo Serra wrote:

Hi,

2018-02-13 19:25 GMT+01:00 Brian Norris :

Hi Enric,

On Tue, Feb 13, 2018 at 11:40:44AM +0100, Enric Balletbo i Serra wrote:

On 12/02/18 23:13, Brian Norris wrote:

On Sat, Feb 10, 2018 at 07:09:05PM +0800, Jeffy Chen wrote:

Add support for specifying event actions to trigger wakeup when using
the gpio-keys input device as a wakeup source.

This would allow the device to configure when to wakeup the system. For
example a gpio-keys input device for pen insert, may only want to wakeup
the system when ejecting the pen.

Suggested-by: Brian Norris 
Signed-off-by: Jeffy Chen 
---

Changes in v2:
Specify wakeup event action instead of irq trigger type as Brian
suggested.

[...]

Not sure if you were aware but there is also some discussion related to this,
maybe we can join the efforts?

v1: https://patchwork.kernel.org/patch/10208261/
v2: https://patchwork.kernel.org/patch/10211147/


Thanks for the pointers. IIUC, that's talking about a different problem:
how to utilize a GPIO key in level-triggered mode. That touches similar
code, but it doesn't really have anything to do with configuring a
different wakeup trigger type.



Right, sorry. I see now what you are doing.


The two patches would need to be reconciled, if they both are going to
be merged. But otherwise, I think they're perfectly fine to be separate.



Yes, that's why I got confused, I had those patches applied on my tree
and when I tried to apply these failed and I wrongly assumed that were
doing the same. Waiting to test the third version ;)
right, they are not related, and i should add the level irq case after 
that series merged :)




Thanks,
  Enric


Brian

Re: [PATCH v2 1/3] Input: gpio-keys - add support for wakeup event action


Hi Enric,

Thanks for your reply.

On 02/14/2018 06:25 AM, Enric Balletbo Serra wrote:

Hi,

2018-02-13 19:25 GMT+01:00 Brian Norris :

Hi Enric,

On Tue, Feb 13, 2018 at 11:40:44AM +0100, Enric Balletbo i Serra wrote:

On 12/02/18 23:13, Brian Norris wrote:

On Sat, Feb 10, 2018 at 07:09:05PM +0800, Jeffy Chen wrote:

Add support for specifying event actions to trigger wakeup when using
the gpio-keys input device as a wakeup source.

This would allow the device to configure when to wakeup the system. For
example a gpio-keys input device for pen insert, may only want to wakeup
the system when ejecting the pen.

Suggested-by: Brian Norris 
Signed-off-by: Jeffy Chen 
---

Changes in v2:
Specify wakeup event action instead of irq trigger type as Brian
suggested.

[...]

Not sure if you were aware but there is also some discussion related to this,
maybe we can join the efforts?

v1: https://patchwork.kernel.org/patch/10208261/
v2: https://patchwork.kernel.org/patch/10211147/


Thanks for the pointers. IIUC, that's talking about a different problem:
how to utilize a GPIO key in level-triggered mode. That touches similar
code, but it doesn't really have anything to do with configuring a
different wakeup trigger type.



Right, sorry. I see now what you are doing.


The two patches would need to be reconciled, if they both are going to
be merged. But otherwise, I think they're perfectly fine to be separate.



Yes, that's why I got confused, I had those patches applied on my tree
and when I tried to apply these failed and I wrongly assumed that were
doing the same. Waiting to test the third version ;)
right, they are not related, and i should add the level irq case after 
that series merged :)




Thanks,
  Enric


Brian

Re: [PATCH v2 1/3] Input: gpio-keys - add support for wakeup event action

Hi Brian,

Thanks for your reply.

On 02/13/2018 06:13 AM, Brian Norris wrote:

>
>if (bdata->gpiod) {
>+   int active_low = gpiod_is_active_low(bdata->gpiod);
>+
>if (button->debounce_interval) {
>error = gpiod_set_debounce(bdata->gpiod,
>button->debounce_interval * 1000);
>@@ -568,6 +572,16 @@ static int gpio_keys_setup_key(struct platform_device 
*pdev,
>isr = gpio_keys_gpio_isr;
>irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
>
>+   switch (button->wakeup_event_action) {
>+   case EV_ACT_ASSERTED:
>+   bdata->wakeup_trigger_type = active_low ?
>+   IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
>+   break;
>+   case EV_ACT_DEASSERTED:
>+   bdata->wakeup_trigger_type = active_low ?
>+   IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
>+   break;

What about EV_ACT_ANY? And default case? I think for ANY, we're OK
letting suspend/resume not reconfigure the trigger type, but maybe a
comment here to note that?

right, will add comment in the next version.

>+   }
>} else {

What about the 'else' case? Shouldn't we try to handle that?
i think the else case is for irq key, which would generate down and up 
events in one irq, so it would use the same trigger type for all these 3 
cases.

i'll add comment in the next version too.

Brian

Re: [PATCH v2 1/3] Input: gpio-keys - add support for wakeup event action

Hi Brian,

Thanks for your reply.

On 02/13/2018 06:13 AM, Brian Norris wrote:

>
>if (bdata->gpiod) {
>+   int active_low = gpiod_is_active_low(bdata->gpiod);
>+
>if (button->debounce_interval) {
>error = gpiod_set_debounce(bdata->gpiod,
>button->debounce_interval * 1000);
>@@ -568,6 +572,16 @@ static int gpio_keys_setup_key(struct platform_device 
*pdev,
>isr = gpio_keys_gpio_isr;
>irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
>
>+   switch (button->wakeup_event_action) {
>+   case EV_ACT_ASSERTED:
>+   bdata->wakeup_trigger_type = active_low ?
>+   IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
>+   break;
>+   case EV_ACT_DEASSERTED:
>+   bdata->wakeup_trigger_type = active_low ?
>+   IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
>+   break;

What about EV_ACT_ANY? And default case? I think for ANY, we're OK
letting suspend/resume not reconfigure the trigger type, but maybe a
comment here to note that?

right, will add comment in the next version.

>+   }
>} else {

What about the 'else' case? Shouldn't we try to handle that?
i think the else case is for irq key, which would generate down and up 
events in one irq, so it would use the same trigger type for all these 3 
cases.

i'll add comment in the next version too.

Brian

Re: [PATCH v2 0/3] gpio-keys: Add support for specifying wakeup event action


Hi Heiko,

Thanks for your reply :)

On 02/14/2018 09:39 PM, Heiko Stübner wrote:

Hi Jeffy,

Am Samstag, 10. Februar 2018, 12:09:04 CET schrieb Jeffy Chen:

On chromebook kevin, we are using gpio-keys for pen insert event. But
we only want it to wakeup the system when ejecting the pen.

So we may need to change the interrupt trigger type during suspending.

Changes in v2:
Specify wakeup event action instead of irq trigger type as Brian
suggested.

Jeffy Chen (3):
   Input: gpio-keys - add support for wakeup event action
   Input: gpio-keys - allow setting wakeup event action in DT
   arm64: dts: rockchip: Avoid wakeup when inserting the pen


The recipient-selection for the mail thread seems to be a bit off and
I only got the cover-letter and patch 3/3.
Therefore I won't really see if and when the Input-changes get
accepted. So please ping me once that has happened so I can pick up
the rockchip dts change for it.

oops, sorry, i'll add you to the CC list in the next version :)




Thanks
Heiko

Re: [PATCH v2 0/3] gpio-keys: Add support for specifying wakeup event action


Hi Heiko,

Thanks for your reply :)

On 02/14/2018 09:39 PM, Heiko Stübner wrote:

Hi Jeffy,

Am Samstag, 10. Februar 2018, 12:09:04 CET schrieb Jeffy Chen:

On chromebook kevin, we are using gpio-keys for pen insert event. But
we only want it to wakeup the system when ejecting the pen.

So we may need to change the interrupt trigger type during suspending.

Changes in v2:
Specify wakeup event action instead of irq trigger type as Brian
suggested.

Jeffy Chen (3):
   Input: gpio-keys - add support for wakeup event action
   Input: gpio-keys - allow setting wakeup event action in DT
   arm64: dts: rockchip: Avoid wakeup when inserting the pen


The recipient-selection for the mail thread seems to be a bit off and
I only got the cover-letter and patch 3/3.
Therefore I won't really see if and when the Input-changes get
accepted. So please ping me once that has happened so I can pick up
the rockchip dts change for it.

oops, sorry, i'll add you to the CC list in the next version :)




Thanks
Heiko

Re: [PATCH 2/3] Input: gpio-keys - allow setting wakeup interrupt trigger type in DT

2018-02-09 Thread JeffyChen


Hi Brian,

Thanks for your reply.

On 02/10/2018 07:42 AM, Brian Norris wrote:

Hi Jeffy,

On Fri, Feb 09, 2018 at 07:55:09PM +0800, Jeffy Chen wrote:

Allow specifying a different interrupt trigger type for wakeup when
using the gpio-keys input device as a wakeup source.

Signed-off-by: Jeffy Chen 
---

  Documentation/devicetree/bindings/input/gpio-keys.txt | 9 +
  1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/gpio-keys.txt 
b/Documentation/devicetree/bindings/input/gpio-keys.txt
index a94940481e55..61926cef708f 100644
--- a/Documentation/devicetree/bindings/input/gpio-keys.txt
+++ b/Documentation/devicetree/bindings/input/gpio-keys.txt
@@ -26,6 +26,15 @@ Optional subnode-properties:
  If not specified defaults to 5.
- wakeup-source: Boolean, button can wake-up the system.
 (Legacy property supported: "gpio-key,wakeup")
+   - wakeup-trigger-type: Specifies the interrupt trigger type for wakeup.
+The value is defined in 



Do you really want to codify interrupt triggers here? It seems like most
of the information about edge vs. level is already codified elsewhere,
so this becomes a little redundant. And in fact, some bindings may be
specifying a "gpio", not technically an interrupt (at least not
directly), so it feels weird to apply IRQ_* flags to them right here.
Anyway, I think he only piece you really want to describe here is, do we
wake on "event asserted", "event deasserted", or both. (The "none" case
would just mean you shouldn't have the "wakeup-source" property.)

So maybe:

wakeup-trigger-type: Specifies whether the key should wake the
system when asserted, when deasserted, or both. This property is
only valid for keys that wake up the system (e.g., when the
"wakeup-source" property is also provided). Supported values
are:
  1: asserted
  2: deasserted
  3: both asserted and deasserted

? We could still make macros out of those, if we want
(input/linux-event-codes.h?). And then leave it up to the driver to
determine how to translate that into the appropriate edge or level
triggers.

make sense, will do it in the next version.


Brian


+Only the following flags are supported:
+   IRQ_TYPE_NONE
+   IRQ_TYPE_EDGE_RISING
+   IRQ_TYPE_EDGE_FALLING
+   IRQ_TYPE_EDGE_BOTH
+   IRQ_TYPE_LEVEL_HIGH
+   IRQ_TYPE_LEVEL_LOW
- linux,can-disable: Boolean, indicates that button is connected
  to dedicated (not shared) interrupt which can be disabled to
  suppress events from the button.
--
2.11.0

Re: [PATCH 2/3] Input: gpio-keys - allow setting wakeup interrupt trigger type in DT

2018-02-09 Thread JeffyChen


Hi Brian,

Thanks for your reply.

On 02/10/2018 07:42 AM, Brian Norris wrote:

Hi Jeffy,

On Fri, Feb 09, 2018 at 07:55:09PM +0800, Jeffy Chen wrote:

Allow specifying a different interrupt trigger type for wakeup when
using the gpio-keys input device as a wakeup source.

Signed-off-by: Jeffy Chen 
---

  Documentation/devicetree/bindings/input/gpio-keys.txt | 9 +
  1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/gpio-keys.txt 
b/Documentation/devicetree/bindings/input/gpio-keys.txt
index a94940481e55..61926cef708f 100644
--- a/Documentation/devicetree/bindings/input/gpio-keys.txt
+++ b/Documentation/devicetree/bindings/input/gpio-keys.txt
@@ -26,6 +26,15 @@ Optional subnode-properties:
  If not specified defaults to 5.
- wakeup-source: Boolean, button can wake-up the system.
 (Legacy property supported: "gpio-key,wakeup")
+   - wakeup-trigger-type: Specifies the interrupt trigger type for wakeup.
+The value is defined in 



Do you really want to codify interrupt triggers here? It seems like most
of the information about edge vs. level is already codified elsewhere,
so this becomes a little redundant. And in fact, some bindings may be
specifying a "gpio", not technically an interrupt (at least not
directly), so it feels weird to apply IRQ_* flags to them right here.
Anyway, I think he only piece you really want to describe here is, do we
wake on "event asserted", "event deasserted", or both. (The "none" case
would just mean you shouldn't have the "wakeup-source" property.)

So maybe:

wakeup-trigger-type: Specifies whether the key should wake the
system when asserted, when deasserted, or both. This property is
only valid for keys that wake up the system (e.g., when the
"wakeup-source" property is also provided). Supported values
are:
  1: asserted
  2: deasserted
  3: both asserted and deasserted

? We could still make macros out of those, if we want
(input/linux-event-codes.h?). And then leave it up to the driver to
determine how to translate that into the appropriate edge or level
triggers.

make sense, will do it in the next version.


Brian


+Only the following flags are supported:
+   IRQ_TYPE_NONE
+   IRQ_TYPE_EDGE_RISING
+   IRQ_TYPE_EDGE_FALLING
+   IRQ_TYPE_EDGE_BOTH
+   IRQ_TYPE_LEVEL_HIGH
+   IRQ_TYPE_LEVEL_LOW
- linux,can-disable: Boolean, indicates that button is connected
  to dedicated (not shared) interrupt which can be disabled to
  suppress events from the button.
--
2.11.0

Re: [PATCH v5 08/13] iommu/rockchip: Control clocks needed to access the IOMMU

2018-01-26 Thread JeffyChen


Hi Robin,

Thanks for your reply.

On 01/24/2018 09:49 PM, Robin Murphy wrote:


+Optional properties:
+- clocks : A list of master clocks requires for the IOMMU to be
accessible


s/requires/required/

ok



+   by the host CPU. The number of clocks depends on the master
+   block and might as well be zero. See [1] for generic clock


Oops, some subtleties of English here :)

To say "the number of clocks ... might as well be zero" effectively
implies "there's no point ever specifying any clocks". I guess what you
really mean here is "...might well be...", i.e. it is both valid and
reasonably likely to require zero clocks.

ok



+   bindings description.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
  Optional properties:
  - rockchip,disable-mmu-reset : Don't use the mmu reset operation.
@@ -27,5 +34,6 @@ Example:
  reg = <0xff940300 0x100>;
  interrupts = ;
  interrupt-names = "vopl_mmu";
+clocks = < ACLK_VOP1>, < DCLK_VOP1>, < HCLK_VOP1>;
  #iommu-cells = <0>;
  };
diff --git a/drivers/iommu/rockchip-iommu.c
b/drivers/iommu/rockchip-iommu.c
index c4131ca792e0..8a5e2a659b67 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -4,6 +4,7 @@
   * published by the Free Software Foundation.
   */
+#include 
  #include 
  #include 
  #include 
@@ -91,6 +92,8 @@ struct rk_iommu {
  struct device *dev;
  void __iomem **bases;
  int num_mmu;
+struct clk_bulk_data *clocks;
+int num_clocks;
  bool reset_disabled;
  struct iommu_device iommu;
  struct list_head node; /* entry in rk_iommu_domain.iommus */
@@ -450,6 +453,38 @@ static int rk_iommu_force_reset(struct rk_iommu
*iommu)
  return 0;
  }
+static int rk_iommu_of_get_clocks(struct rk_iommu *iommu)
+{
+struct device_node *np = iommu->dev->of_node;
+int ret;
+int i;
+
+ret = of_count_phandle_with_args(np, "clocks", "#clock-cells");
+if (ret == -ENOENT)
+return 0;
+else if (ret < 0)
+return ret;
+
+iommu->num_clocks = ret;
+iommu->clocks = devm_kcalloc(iommu->dev, iommu->num_clocks,
+ sizeof(*iommu->clocks), GFP_KERNEL);
+if (!iommu->clocks)
+return -ENOMEM;
+
+for (i = 0; i < iommu->num_clocks; ++i) {
+iommu->clocks[i].clk = of_clk_get(np, i);
+if (IS_ERR(iommu->clocks[i].clk)) {
+ret = PTR_ERR(iommu->clocks[i].clk);
+goto err_clk_put;
+}
+}


Just to confirm my understanding from a quick scan through the code, the
reason we can't use clk_bulk_get() here is that currently, clocks[i].id
being NULL means we'd end up just getting the first clock multiple
times, right?

right, without a valid name, it would return the first clock.

/* Walk up the tree of devices looking for a clock that matches */
while (np) {
int index = 0;

/*
 * For named clocks, first look up the name in the
 * "clock-names" property.  If it cannot be found, then
 * index will be an error code, and of_clk_get() will fail.
 */
if (name)
index = of_property_match_string(np, "clock-names", name);
clk = __of_clk_get(np, index, dev_id, name);




I guess there could be other users who also want "just get whatever
clocks I have" functionality, so it might be worth proposing that for
the core API as a separate/follow-up patch, but it definitely doesn't
need to be part of this series.

right, i can try to do it later :)


I really don't know enough about correct clk API usage, but modulo the
binding comments it certainly looks nice and tidy now;

Acked-by: Robin Murphy 

thanks.


Thanks,
Robin.

Re: [PATCH v5 08/13] iommu/rockchip: Control clocks needed to access the IOMMU

2018-01-26 Thread JeffyChen


Hi Robin,

Thanks for your reply.

On 01/24/2018 09:49 PM, Robin Murphy wrote:


+Optional properties:
+- clocks : A list of master clocks requires for the IOMMU to be
accessible


s/requires/required/

ok



+   by the host CPU. The number of clocks depends on the master
+   block and might as well be zero. See [1] for generic clock


Oops, some subtleties of English here :)

To say "the number of clocks ... might as well be zero" effectively
implies "there's no point ever specifying any clocks". I guess what you
really mean here is "...might well be...", i.e. it is both valid and
reasonably likely to require zero clocks.

ok



+   bindings description.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
  Optional properties:
  - rockchip,disable-mmu-reset : Don't use the mmu reset operation.
@@ -27,5 +34,6 @@ Example:
  reg = <0xff940300 0x100>;
  interrupts = ;
  interrupt-names = "vopl_mmu";
+clocks = < ACLK_VOP1>, < DCLK_VOP1>, < HCLK_VOP1>;
  #iommu-cells = <0>;
  };
diff --git a/drivers/iommu/rockchip-iommu.c
b/drivers/iommu/rockchip-iommu.c
index c4131ca792e0..8a5e2a659b67 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -4,6 +4,7 @@
   * published by the Free Software Foundation.
   */
+#include 
  #include 
  #include 
  #include 
@@ -91,6 +92,8 @@ struct rk_iommu {
  struct device *dev;
  void __iomem **bases;
  int num_mmu;
+struct clk_bulk_data *clocks;
+int num_clocks;
  bool reset_disabled;
  struct iommu_device iommu;
  struct list_head node; /* entry in rk_iommu_domain.iommus */
@@ -450,6 +453,38 @@ static int rk_iommu_force_reset(struct rk_iommu
*iommu)
  return 0;
  }
+static int rk_iommu_of_get_clocks(struct rk_iommu *iommu)
+{
+struct device_node *np = iommu->dev->of_node;
+int ret;
+int i;
+
+ret = of_count_phandle_with_args(np, "clocks", "#clock-cells");
+if (ret == -ENOENT)
+return 0;
+else if (ret < 0)
+return ret;
+
+iommu->num_clocks = ret;
+iommu->clocks = devm_kcalloc(iommu->dev, iommu->num_clocks,
+ sizeof(*iommu->clocks), GFP_KERNEL);
+if (!iommu->clocks)
+return -ENOMEM;
+
+for (i = 0; i < iommu->num_clocks; ++i) {
+iommu->clocks[i].clk = of_clk_get(np, i);
+if (IS_ERR(iommu->clocks[i].clk)) {
+ret = PTR_ERR(iommu->clocks[i].clk);
+goto err_clk_put;
+}
+}


Just to confirm my understanding from a quick scan through the code, the
reason we can't use clk_bulk_get() here is that currently, clocks[i].id
being NULL means we'd end up just getting the first clock multiple
times, right?

right, without a valid name, it would return the first clock.

/* Walk up the tree of devices looking for a clock that matches */
while (np) {
int index = 0;

/*
 * For named clocks, first look up the name in the
 * "clock-names" property.  If it cannot be found, then
 * index will be an error code, and of_clk_get() will fail.
 */
if (name)
index = of_property_match_string(np, "clock-names", name);
clk = __of_clk_get(np, index, dev_id, name);




I guess there could be other users who also want "just get whatever
clocks I have" functionality, so it might be worth proposing that for
the core API as a separate/follow-up patch, but it definitely doesn't
need to be part of this series.

right, i can try to do it later :)


I really don't know enough about correct clk API usage, but modulo the
binding comments it certainly looks nice and tidy now;

Acked-by: Robin Murphy 

thanks.


Thanks,
Robin.

Re: [PATCH v4 08/13] iommu/rockchip: Control clocks needed to access the IOMMU

2018-01-25 Thread JeffyChen


On 01/25/2018 05:42 PM, Randy Li wrote:




confirmed with Simon, there might be some iommus don't have a pd, and

We use the pd to control the NIU node(not on upstream), without a pd or
fake pd, none of the platform would work.
after talked offline, it's possible to have iommu without pd in upstream 
kernel(and chromeos kernel), but on our internal kernel, the drivers 
would require pd(or fake pd) to reset modules when error happens.


anyway, i think that means we do need clock control here.


the CONFIG_PM could be disabled.I am hard to believe a modern platform
can work without that.

so it might be better to control clocks in iommu driver itself.

I won't
insist how the version of the iommu patch on the upstream, I
just post an idea here.
The version for kernel 4.4 is under internal review, the implementation
has been modified many times.

I would suggest the managing clocks in pd is a more easy way and don't
need to spare those thing in two places.

Re: [PATCH v4 08/13] iommu/rockchip: Control clocks needed to access the IOMMU

2018-01-25 Thread JeffyChen


On 01/25/2018 05:42 PM, Randy Li wrote:




confirmed with Simon, there might be some iommus don't have a pd, and

We use the pd to control the NIU node(not on upstream), without a pd or
fake pd, none of the platform would work.
after talked offline, it's possible to have iommu without pd in upstream 
kernel(and chromeos kernel), but on our internal kernel, the drivers 
would require pd(or fake pd) to reset modules when error happens.


anyway, i think that means we do need clock control here.


the CONFIG_PM could be disabled.I am hard to believe a modern platform
can work without that.

so it might be better to control clocks in iommu driver itself.

I won't
insist how the version of the iommu patch on the upstream, I
just post an idea here.
The version for kernel 4.4 is under internal review, the implementation
has been modified many times.

I would suggest the managing clocks in pd is a more easy way and don't
need to spare those thing in two places.

Re: [PATCH v7 00/10] rockchip: kevin: Enable edp display

2018-01-23 Thread JeffyChen


Hi Thierry,

Thanks for posting these :)

Hi Archit,

Thanks for your reply.

On 01/10/2018 05:46 PM, Archit Taneja wrote:


I don't know if the rest of the rockchip patches in the series
depend on the 4 bridge patches. If they do, the rockchip maintainer
can queue both rockchip and bridge patches. If not, I can pick up
the bridge patches.


Mark is no longer maintain rockchip drm driver, i will ask Sandy Huang 
to check these, thanks.


Thanks,
Archit

Re: [PATCH v7 00/10] rockchip: kevin: Enable edp display

2018-01-23 Thread JeffyChen


Hi Thierry,

Thanks for posting these :)

Hi Archit,

Thanks for your reply.

On 01/10/2018 05:46 PM, Archit Taneja wrote:


I don't know if the rest of the rockchip patches in the series
depend on the 4 bridge patches. If they do, the rockchip maintainer
can queue both rockchip and bridge patches. If not, I can pick up
the bridge patches.


Mark is no longer maintain rockchip drm driver, i will ask Sandy Huang 
to check these, thanks.


Thanks,
Archit

Re: [PATCH v4 08/13] iommu/rockchip: Control clocks needed to access the IOMMU


Hi Randy,

On 01/22/2018 10:15 AM, JeffyChen wrote:

Hi Randy,

On 01/22/2018 09:18 AM, Randy Li wrote:



Also the power domain driver could manage the clocks as well, I would
suggest to use pm_runtime_*.


actually the clocks required by pm domain may not be the same as what we
want to control here, there might be some clocks only be needed when
accessing mmu registers.

but i'm not very sure about that, will confirm it with Simon Xue.


confirmed with Simon, there might be some iommus don't have a pd, and 
the CONFIG_PM could be disabled.


so it might be better to control clocks in iommu driver itself.

Re: [PATCH v4 08/13] iommu/rockchip: Control clocks needed to access the IOMMU


Hi Randy,

On 01/22/2018 10:15 AM, JeffyChen wrote:

Hi Randy,

On 01/22/2018 09:18 AM, Randy Li wrote:



Also the power domain driver could manage the clocks as well, I would
suggest to use pm_runtime_*.


actually the clocks required by pm domain may not be the same as what we
want to control here, there might be some clocks only be needed when
accessing mmu registers.

but i'm not very sure about that, will confirm it with Simon Xue.


confirmed with Simon, there might be some iommus don't have a pd, and 
the CONFIG_PM could be disabled.


so it might be better to control clocks in iommu driver itself.

Re: [PATCH v4 08/13] iommu/rockchip: Control clocks needed to access the IOMMU


Hi Randy,

On 01/22/2018 09:18 AM, Randy Li wrote:



Also the power domain driver could manage the clocks as well, I would
suggest to use pm_runtime_*.


actually the clocks required by pm domain may not be the same as what we 
want to control here, there might be some clocks only be needed when 
accessing mmu registers.


but i'm not very sure about that, will confirm it with Simon Xue.

Re: [PATCH v4 08/13] iommu/rockchip: Control clocks needed to access the IOMMU


Hi Randy,

On 01/22/2018 09:18 AM, Randy Li wrote:



Also the power domain driver could manage the clocks as well, I would
suggest to use pm_runtime_*.


actually the clocks required by pm domain may not be the same as what we 
want to control here, there might be some clocks only be needed when 
accessing mmu registers.


but i'm not very sure about that, will confirm it with Simon Xue.

Re: [PATCH v4 07/13] ARM: dts: rockchip: add clocks in vop iommu nodes


Hi Tomasz,

Thanks for your reply.

On 01/19/2018 11:23 AM, Tomasz Figa wrote:

On Thu, Jan 18, 2018 at 8:52 PM, Jeffy Chen  wrote:

Add clocks in vop iommu nodes, since we are going to control clocks in
rockchip iommu driver.

Signed-off-by: Jeffy Chen 
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

  arch/arm/boot/dts/rk3036.dtsi | 2 ++
  arch/arm/boot/dts/rk3288.dtsi | 4 
  2 files changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/rk3036.dtsi b/arch/arm/boot/dts/rk3036.dtsi
index 3b704cfed69a..95b0ebc7a40f 100644
--- a/arch/arm/boot/dts/rk3036.dtsi
+++ b/arch/arm/boot/dts/rk3036.dtsi
@@ -197,6 +197,8 @@
 reg = <0x10118300 0x100>;
 interrupts = ;
 interrupt-names = "vop_mmu";
+   clocks = < ACLK_LCDC>, < SCLK_LCDC>, < HCLK_LCDC>;
+   clock-names = "aclk_vop", "dclk_vop", "hclk_vop";


We should remove clock-names from IOMMU nodes. The Rockchip IOMMU
bindings don't define clock names and only the clocks property should
be given.

hmmm, i'm trying to switch to clk_bulk APIs, the get and put are name 
based. or maybe i can use clk_get/put along with other clk_bulk APIs

Not even saying that the names currently listed are not good examples,
they name SoC clock controller output, rather than device inputs.

Best regards,
Tomasz

Re: [PATCH v4 07/13] ARM: dts: rockchip: add clocks in vop iommu nodes


Hi Tomasz,

Thanks for your reply.

On 01/19/2018 11:23 AM, Tomasz Figa wrote:

On Thu, Jan 18, 2018 at 8:52 PM, Jeffy Chen  wrote:

Add clocks in vop iommu nodes, since we are going to control clocks in
rockchip iommu driver.

Signed-off-by: Jeffy Chen 
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

  arch/arm/boot/dts/rk3036.dtsi | 2 ++
  arch/arm/boot/dts/rk3288.dtsi | 4 
  2 files changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/rk3036.dtsi b/arch/arm/boot/dts/rk3036.dtsi
index 3b704cfed69a..95b0ebc7a40f 100644
--- a/arch/arm/boot/dts/rk3036.dtsi
+++ b/arch/arm/boot/dts/rk3036.dtsi
@@ -197,6 +197,8 @@
 reg = <0x10118300 0x100>;
 interrupts = ;
 interrupt-names = "vop_mmu";
+   clocks = < ACLK_LCDC>, < SCLK_LCDC>, < HCLK_LCDC>;
+   clock-names = "aclk_vop", "dclk_vop", "hclk_vop";


We should remove clock-names from IOMMU nodes. The Rockchip IOMMU
bindings don't define clock names and only the clocks property should
be given.

hmmm, i'm trying to switch to clk_bulk APIs, the get and put are name 
based. or maybe i can use clk_get/put along with other clk_bulk APIs

Not even saying that the names currently listed are not good examples,
they name SoC clock controller output, rather than device inputs.

Best regards,
Tomasz

Re: [PATCH] iommu/of: Only do IOMMU lookup for available ones

Hi Will,

Thanks for your reply.

On 01/18/2018 10:41 PM, Will Deacon wrote:

>
>Makes sense to me, but I'd like to have an OK from Robin or Will (added
>to Cc) before applying this.

I don't think this patch makes a lot of sense in isolation: the SMMU drivers
themselves will likely still probe, and it's unclear what we should about
DMA when an IOMMU is not deemed to be available. See:

https://patchwork.kernel.org/patch/9681211/

Jeffy -- are you solving a real issue here, or is this just an attempt at
some cleanup?

hmmm, it's just a cleanup, but it seems like we already removed this 
of_iommu_init_fn in:

b0c560f7d8a4 iommu: Clean up of_iommu_init_fn

so this patch can be disregarded too :)

Will

Re: [PATCH] iommu/of: Only do IOMMU lookup for available ones

Hi Will,

Thanks for your reply.

On 01/18/2018 10:41 PM, Will Deacon wrote:

>
>Makes sense to me, but I'd like to have an OK from Robin or Will (added
>to Cc) before applying this.

I don't think this patch makes a lot of sense in isolation: the SMMU drivers
themselves will likely still probe, and it's unclear what we should about
DMA when an IOMMU is not deemed to be available. See:

https://patchwork.kernel.org/patch/9681211/

Jeffy -- are you solving a real issue here, or is this just an attempt at
some cleanup?

hmmm, it's just a cleanup, but it seems like we already removed this 
of_iommu_init_fn in:

b0c560f7d8a4 iommu: Clean up of_iommu_init_fn

so this patch can be disregarded too :)

Will

Re: [PATCH v4 08/13] iommu/rockchip: Control clocks needed to access the IOMMU


Hi Robin,

On 01/18/2018 08:27 PM, Robin Murphy wrote:




Is it worth using the clk_bulk_*() APIs for this? At a glance, most of
the code being added here appears to duplicate what those functions
already do (but I'm no clk API expert, for sure).
right, i think it's doable, the clk_bulk APIs are very helpful. i think 
we didn't use that is because this patch were wrote for the chromeos 4.4 
kernel, which doesn't have clk_bulk yet:)


will do it in the next version, thanks.


Robin.

Re: [PATCH v4 08/13] iommu/rockchip: Control clocks needed to access the IOMMU


Hi Robin,

On 01/18/2018 08:27 PM, Robin Murphy wrote:




Is it worth using the clk_bulk_*() APIs for this? At a glance, most of
the code being added here appears to duplicate what those functions
already do (but I'm no clk API expert, for sure).
right, i think it's doable, the clk_bulk APIs are very helpful. i think 
we didn't use that is because this patch were wrote for the chromeos 4.4 
kernel, which doesn't have clk_bulk yet:)


will do it in the next version, thanks.


Robin.

Re: [PATCH v4 04/13] iommu/rockchip: Fix error handling in attach


Hi Robin,

On 01/18/2018 09:23 PM, Robin Murphy wrote:


@@ -837,7 +837,7 @@ static int rk_iommu_attach_device(struct
iommu_domain *domain,
  ret = rk_iommu_enable_paging(iommu);
  if (ret)
-return ret;
+goto err_disable_stall;
  spin_lock_irqsave(_domain->iommus_lock, flags);
  list_add_tail(>node, _domain->iommus);
@@ -848,6 +848,11 @@ static int rk_iommu_attach_device(struct
iommu_domain *domain,
  rk_iommu_disable_stall(iommu);
  return 0;


Nit: if you like, it looks reasonable to name the label
"out_disable_stall" and remove these lines above here, to save the
duplication between the error and success paths (since ret will already
be 0 on the latter).


right, i think so, will do it in the next version.

Either way,

Reviewed-by: Robin Murphy

Re: [PATCH v4 04/13] iommu/rockchip: Fix error handling in attach


Hi Robin,

On 01/18/2018 09:23 PM, Robin Murphy wrote:


@@ -837,7 +837,7 @@ static int rk_iommu_attach_device(struct
iommu_domain *domain,
  ret = rk_iommu_enable_paging(iommu);
  if (ret)
-return ret;
+goto err_disable_stall;
  spin_lock_irqsave(_domain->iommus_lock, flags);
  list_add_tail(>node, _domain->iommus);
@@ -848,6 +848,11 @@ static int rk_iommu_attach_device(struct
iommu_domain *domain,
  rk_iommu_disable_stall(iommu);
  return 0;


Nit: if you like, it looks reasonable to name the label
"out_disable_stall" and remove these lines above here, to save the
duplication between the error and success paths (since ret will already
be 0 on the latter).


right, i think so, will do it in the next version.

Either way,

Reviewed-by: Robin Murphy

Re: [PATCH v4 05/13] iommu/rockchip: Use iopoll helpers to wait for hardware


Hi Robin,

On 01/18/2018 09:09 PM, Robin Murphy wrote:

-#define FORCE_RESET_TIMEOUT100/* ms */
+#define FORCE_RESET_TIMEOUT10/* us */
+#define POLL_TIMEOUT1000/* us */


Nit: the callsites look a bit odd with the combination of POLL_TIMEOUT
and the magic number 100 - should we not also define something like
POLL_PERIOD for that? Also POLL_TIMEOUT is a rather generic name and
overlaps with several other drivers, so a namespace prefix would be
helpful (i.e. RK_IOMMU_POLL*, or at least RK_POLL*).

FWIW, my personal preference would be to also suffix these with _US for
absolute clarity, but it's not essential (especially if longer names
lead to more linebreaks at the callsites).

right, that make sense, will fix it in next version, thanks :)


With those undocumented "100"s fixed up,

Reviewed-by: Robin Murphy

Re: [PATCH v4 05/13] iommu/rockchip: Use iopoll helpers to wait for hardware


Hi Robin,

On 01/18/2018 09:09 PM, Robin Murphy wrote:

-#define FORCE_RESET_TIMEOUT100/* ms */
+#define FORCE_RESET_TIMEOUT10/* us */
+#define POLL_TIMEOUT1000/* us */


Nit: the callsites look a bit odd with the combination of POLL_TIMEOUT
and the magic number 100 - should we not also define something like
POLL_PERIOD for that? Also POLL_TIMEOUT is a rather generic name and
overlaps with several other drivers, so a namespace prefix would be
helpful (i.e. RK_IOMMU_POLL*, or at least RK_POLL*).

FWIW, my personal preference would be to also suffix these with _US for
absolute clarity, but it's not essential (especially if longer names
lead to more linebreaks at the callsites).

right, that make sense, will fix it in next version, thanks :)


With those undocumented "100"s fixed up,

Reviewed-by: Robin Murphy

Re: [PATCH v4 00/13] iommu/rockchip: Use OF_IOMMU


Hi Joerg,

Thanks for your reply.

On 01/18/2018 08:44 PM, Joerg Roedel wrote:

On Thu, Jan 18, 2018 at 07:52:38PM +0800, Jeffy Chen wrote:

Jeffy Chen (9):
   iommu/rockchip: Prohibit unbind and remove
   iommu/rockchip: Fix error handling in probe
   iommu/rockchip: Request irqs in rk_iommu_probe()
   ARM: dts: rockchip: add clocks in vop iommu nodes
   iommu/rockchip: Use IOMMU device for dma mapping operations
   iommu/rockchip: Use OF_IOMMU to attach devices automatically
   iommu/rockchip: Fix error handling in init
   iommu/rockchip: Add runtime PM support
   iommu/rockchip: Support sharing IOMMU between masters

Tomasz Figa (4):
   iommu/rockchip: Fix error handling in attach
   iommu/rockchip: Use iopoll helpers to wait for hardware
   iommu/rockchip: Fix TLB flush of secondary IOMMUs
   iommu/rockchip: Control clocks needed to access the IOMMU


Please stop resending this every day with minimal changes. I am not
going to take it for v4.16, as we are late in the cycle already and there
are still review comments.

Just collect all feedback, update the patches, rebase them to v4.16-rc1
when its out, and send a new version.


oops, sorry, i send v4 today is mainly because i found v3 would break 
some of the rk platforms, which needs an extra patch [7] (ARM: dts: 
rockchip: add clocks in vop iommu nodes),  but forgot to mention it in 
the change log...


will send new version after all the rest patches been reviewed :)



Joerg

Re: [PATCH v4 00/13] iommu/rockchip: Use OF_IOMMU


Hi Joerg,

Thanks for your reply.

On 01/18/2018 08:44 PM, Joerg Roedel wrote:

On Thu, Jan 18, 2018 at 07:52:38PM +0800, Jeffy Chen wrote:

Jeffy Chen (9):
   iommu/rockchip: Prohibit unbind and remove
   iommu/rockchip: Fix error handling in probe
   iommu/rockchip: Request irqs in rk_iommu_probe()
   ARM: dts: rockchip: add clocks in vop iommu nodes
   iommu/rockchip: Use IOMMU device for dma mapping operations
   iommu/rockchip: Use OF_IOMMU to attach devices automatically
   iommu/rockchip: Fix error handling in init
   iommu/rockchip: Add runtime PM support
   iommu/rockchip: Support sharing IOMMU between masters

Tomasz Figa (4):
   iommu/rockchip: Fix error handling in attach
   iommu/rockchip: Use iopoll helpers to wait for hardware
   iommu/rockchip: Fix TLB flush of secondary IOMMUs
   iommu/rockchip: Control clocks needed to access the IOMMU


Please stop resending this every day with minimal changes. I am not
going to take it for v4.16, as we are late in the cycle already and there
are still review comments.

Just collect all feedback, update the patches, rebase them to v4.16-rc1
when its out, and send a new version.


oops, sorry, i send v4 today is mainly because i found v3 would break 
some of the rk platforms, which needs an extra patch [7] (ARM: dts: 
rockchip: add clocks in vop iommu nodes),  but forgot to mention it in 
the change log...


will send new version after all the rest patches been reviewed :)



Joerg

Re: [PATCH v3 01/12] iommu/rockchip: Prohibiat unbind and remove


Hi Tomasz,

Thanks for your reply.

and just found i forgot to add iommu clocks for other rockchip 
platforms(rk3399 already has that)...will also do that in the next version.


On 01/18/2018 12:17 PM, Tomasz Figa wrote:

On Thu, Jan 18, 2018 at 12:25 AM, Jeffy Chen  wrote:

Removal the IOMMUs cannot be done reliably.


nit: Perhaps "Removal of IOMMUs"?


ok


This is similar to exynos iommu driver.

Signed-off-by: Jeffy Chen 
---

Changes in v3:
Also remove remove() and module_exit() as Tomasz suggested.

Changes in v2: None

  drivers/iommu/rockchip-iommu.c | 21 +
  1 file changed, 1 insertion(+), 20 deletions(-)


nit: Typo in subject: s/Prohibiat/Prohibit/


ok, will do that.

Otherwise,

Reviewed-by: Tomasz Figa 

Best regards,
Tomasz

Re: [PATCH v3 01/12] iommu/rockchip: Prohibiat unbind and remove