[PATCH 1/2] drm/etnaviv: fail probe if core or bus clock are absent

[PATCH 1/2] drm/etnaviv: fail probe if core or bus clock are absent

[Lucas Stach]

The devicetree documentation states that those are required properties,
so the driver should refuse to probe if those are absent to be
consistent. This will also allow to drop some error checking from the
clock enable/disable paths.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index b851809d29b3..ec14aaaf6dd7 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1684,12 +1684,12 @@ static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
 	gpu->clk_bus = devm_clk_get(&pdev->dev, "bus");
 	DBG("clk_bus: %p", gpu->clk_bus);
 	if (IS_ERR(gpu->clk_bus))
-		gpu->clk_bus = NULL;
+		return PTR_ERR(gpu->clk_bus);
 
 	gpu->clk_core = devm_clk_get(&pdev->dev, "core");
 	DBG("clk_core: %p", gpu->clk_core);
 	if (IS_ERR(gpu->clk_core))
-		gpu->clk_core = NULL;
+		return PTR_ERR(gpu->clk_core);
 
 	gpu->clk_shader = devm_clk_get(&pdev->dev, "shader");
 	DBG("clk_shader: %p", gpu->clk_shader);
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] net: fec: don't ack masked interrupt events

[Lucas Stach]

The FEC doesn't have a real interrupt status register, that takes
into account the mask status of the IRQ. The driver reads the raw
interrupt event register, which also reports events for masked
IRQs.

The driver needs to apply the current mask itself, to avoid acking
IRQs that are currently masked, as NAPI relies on the masking to
hide the IRQs. The current behavior of just acking all interrupts
regardless of their mask status opens the driver up the "rotting
packet" race-window, as described in the original NAPI-HOWTO, which
has been observed in the wild.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 01f7e811739b..498264969e89 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1572,7 +1572,8 @@ fec_enet_interrupt(int irq, void *dev_id)
 	uint int_events;
 	irqreturn_t ret = IRQ_NONE;
 
-	int_events = readl(fep->hwp + FEC_IEVENT);
+	int_events = readl_relaxed(fep->hwp + FEC_IEVENT) &
+	             readl_relaxed(fep->hwp + FEC_IMASK);
 	writel(int_events, fep->hwp + FEC_IEVENT);
 	fec_enet_collect_events(fep, int_events);
 
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] net: fec: don't ack masked interrupt events

[Lucas Stach]

Sorry, please ignore the FEC patches. Those are test patches still
residing in my to-send folder. Sorry for the noise.

Am Freitag, den 26.08.2016, 17:49 +0200 schrieb Lucas Stach:
> The FEC doesn't have a real interrupt status register, that takes
> into account the mask status of the IRQ. The driver reads the raw
> interrupt event register, which also reports events for masked
> IRQs.
> 
> The driver needs to apply the current mask itself, to avoid acking
> IRQs that are currently masked, as NAPI relies on the masking to
> hide the IRQs. The current behavior of just acking all interrupts
> regardless of their mask status opens the driver up the "rotting
> packet" race-window, as described in the original NAPI-HOWTO, which
> has been observed in the wild.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 01f7e811739b..498264969e89 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1572,7 +1572,8 @@ fec_enet_interrupt(int irq, void *dev_id)
>  	uint int_events;
>  	irqreturn_t ret = IRQ_NONE;
>  
> -	int_events = readl(fep->hwp + FEC_IEVENT);
> +	int_events = readl_relaxed(fep->hwp + FEC_IEVENT) &
> +	             readl_relaxed(fep->hwp + FEC_IMASK);
>  	writel(int_events, fep->hwp + FEC_IEVENT);
>  	fec_enet_collect_events(fep, int_events);
>  


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] net: fec: don't ack masked interrupt events

[Russell King - ARM Linux]

On Fri, Aug 26, 2016 at 05:53:38PM +0200, Lucas Stach wrote:
> Sorry, please ignore the FEC patches. Those are test patches still
> residing in my to-send folder. Sorry for the noise.

This patch actually looks correct: you are indeed correct that the
driver can end up with a packet sitting waiting to be received.
All it will take is a correctly timed MII interrupt event with a
NAPI just coming to the end of its processing.

So, I'd encourage this to be applied - but obviously when sent to
netdev rather than the DRI mailing list!

> 
> Am Freitag, den 26.08.2016, 17:49 +0200 schrieb Lucas Stach:
> > The FEC doesn't have a real interrupt status register, that takes
> > into account the mask status of the IRQ. The driver reads the raw
> > interrupt event register, which also reports events for masked
> > IRQs.
> > 
> > The driver needs to apply the current mask itself, to avoid acking
> > IRQs that are currently masked, as NAPI relies on the masking to
> > hide the IRQs. The current behavior of just acking all interrupts
> > regardless of their mask status opens the driver up the "rotting
> > packet" race-window, as described in the original NAPI-HOWTO, which
> > has been observed in the wild.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/net/ethernet/freescale/fec_main.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> > index 01f7e811739b..498264969e89 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -1572,7 +1572,8 @@ fec_enet_interrupt(int irq, void *dev_id)
> >  	uint int_events;
> >  	irqreturn_t ret = IRQ_NONE;
> >  
> > -	int_events = readl(fep->hwp + FEC_IEVENT);
> > +	int_events = readl_relaxed(fep->hwp + FEC_IEVENT) &
> > +	             readl_relaxed(fep->hwp + FEC_IMASK);
> >  	writel(int_events, fep->hwp + FEC_IEVENT);
> >  	fec_enet_collect_events(fep, int_events);
> >  
> 
> 

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/etnaviv: fold various clock enable/disable functions into top ones

[Lucas Stach]

The driver doesn't ever enable individual clocks alone, so there
is no need to scatter the clock enable/disable sequences through
multiple functions. Fold them into the top one.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 91 ++++++++---------------------------
 1 file changed, 20 insertions(+), 71 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index ec14aaaf6dd7..6a4de07bd6e6 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -868,63 +868,6 @@ int etnaviv_gpu_debugfs(struct etnaviv_gpu *gpu, struct seq_file *m)
 #endif
 
 /*
- * Power Management:
- */
-static int enable_clk(struct etnaviv_gpu *gpu)
-{
-	int ret;
-
-	if (gpu->clk_core) {
-		ret = clk_prepare_enable(gpu->clk_core);
-		if (ret)
-			return ret;
-	}
-
-	if (gpu->clk_shader) {
-		ret = clk_prepare_enable(gpu->clk_shader);
-		if (ret)
-			goto disable_clk_core;
-	}
-
-	return 0;
-
-disable_clk_core:
-	clk_disable_unprepare(gpu->clk_core);
-	return ret;
-}
-
-static int disable_clk(struct etnaviv_gpu *gpu)
-{
-	if (gpu->clk_core)
-		clk_disable_unprepare(gpu->clk_core);
-	if (gpu->clk_shader)
-		clk_disable_unprepare(gpu->clk_shader);
-
-	return 0;
-}
-
-static int enable_axi(struct etnaviv_gpu *gpu)
-{
-	int ret;
-
-	if (gpu->clk_bus) {
-		ret = clk_prepare_enable(gpu->clk_bus);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
-}
-
-static int disable_axi(struct etnaviv_gpu *gpu)
-{
-	if (gpu->clk_bus)
-		clk_disable_unprepare(gpu->clk_bus);
-
-	return 0;
-}
-
-/*
  * Hangcheck detection for locked gpu:
  */
 static void recover_worker(struct work_struct *work)
@@ -1484,30 +1427,36 @@ static int etnaviv_gpu_clk_enable(struct etnaviv_gpu *gpu)
 {
 	int ret;
 
-	ret = enable_clk(gpu);
+	ret = clk_prepare_enable(gpu->clk_bus);
 	if (ret)
 		return ret;
 
-	ret = enable_axi(gpu);
-	if (ret) {
-		disable_clk(gpu);
-		return ret;
+	ret = clk_prepare_enable(gpu->clk_core);
+	if (ret)
+		goto disable_clk_bus;
+
+	if (gpu->clk_shader) {
+		ret = clk_prepare_enable(gpu->clk_shader);
+		if (ret)
+			goto disable_clk_core;
 	}
 
 	return 0;
+
+disable_clk_core:
+	clk_disable_unprepare(gpu->clk_core);
+disable_clk_bus:
+	clk_disable_unprepare(gpu->clk_bus);
+
+	return ret;
 }
 
 static int etnaviv_gpu_clk_disable(struct etnaviv_gpu *gpu)
 {
-	int ret;
-
-	ret = disable_axi(gpu);
-	if (ret)
-		return ret;
-
-	ret = disable_clk(gpu);
-	if (ret)
-		return ret;
+	if (gpu->clk_shader)
+		clk_disable_unprepare(gpu->clk_shader);
+	clk_disable_unprepare(gpu->clk_core);
+	clk_disable_unprepare(gpu->clk_bus);
 
 	return 0;
 }
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] net: fec: optimize IRQ handler

[Lucas Stach]

fep->work_rx and fep->work_tx are both non-zero, as long as the NAPI
softirq hasn't finished its work. So if the current IRQ does not
signal any RX or TX completion, but some unrelated event, the path to
schedule the NAPI context is still entered.

The handler works correctly as in this case napi_schedule_prep() will
reject the scheduling attempt, but the flow can still be optimized by
not trying to schedule if the IRQ doesn't signal RX or TX completion.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 498264969e89..fee5783a4dc6 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1575,14 +1575,14 @@ fec_enet_interrupt(int irq, void *dev_id)
 	int_events = readl_relaxed(fep->hwp + FEC_IEVENT) &
 	             readl_relaxed(fep->hwp + FEC_IMASK);
 	writel(int_events, fep->hwp + FEC_IEVENT);
-	fec_enet_collect_events(fep, int_events);
 
-	if ((fep->work_tx || fep->work_rx) && fep->link) {
+	if ((int_events & (FEC_ENET_RXF | FEC_ENET_TXF)) && fep->link) {
 		ret = IRQ_HANDLED;
 
 		if (napi_schedule_prep(&fep->napi)) {
 			/* Disable the NAPI interrupts */
 			writel(FEC_NAPI_IMASK, fep->hwp + FEC_IMASK);
+			fec_enet_collect_events(fep, int_events);
 			__napi_schedule(&fep->napi);
 		}
 	}
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/etnaviv: fail probe if core or bus clock are absent

[Russell King - ARM Linux]

On Fri, Aug 26, 2016 at 05:49:54PM +0200, Lucas Stach wrote:
> The devicetree documentation states that those are required properties,
> so the driver should refuse to probe if those are absent to be
> consistent. This will also allow to drop some error checking from the
> clock enable/disable paths.

NAK.

Thanks for reviewing the existing DT files before proposing this change
and noticing that you're going to wilfully end up breaking existing users.
A simple grep would have sufficed.

The DT binding doc is wrong: there is only one documented clock on Dove
and that's for the GPU core.  (The Dove documentation as far as clocks
go is very poor.)  So, what's Dove supposed to do - make up some
ficticious clock?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/etnaviv: fail probe if core or bus clock are absent

[Lucas Stach]

Am Freitag, den 26.08.2016, 17:10 +0100 schrieb Russell King - ARM
Linux:
> On Fri, Aug 26, 2016 at 05:49:54PM +0200, Lucas Stach wrote:
> > The devicetree documentation states that those are required properties,
> > so the driver should refuse to probe if those are absent to be
> > consistent. This will also allow to drop some error checking from the
> > clock enable/disable paths.
> 
> NAK.
> 
> Thanks for reviewing the existing DT files before proposing this change
> and noticing that you're going to wilfully end up breaking existing users.
> A simple grep would have sufficed.
> 
Gah, thanks for pointing this out.

> The DT binding doc is wrong: there is only one documented clock on Dove
> and that's for the GPU core.  (The Dove documentation as far as clocks
> go is very poor.)  So, what's Dove supposed to do - make up some
> ficticious clock?
> 
Core, bus and shader are all module input clocks. If the SoC integration
provides the same clock for all inputs, the DT should reflect this by
supplying the same clock for all 3 inputs.

I'm going to change this patch to keep things working for the Dove DTs,
but I think they really should be changed to supply all 3 input clocks.
I'm sorry for not noticing this when you proposed the Dove GPU DT
support.

Regards,
Lucas



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/etnaviv: fail probe if core or bus clock are absent

[Russell King - ARM Linux]

On Mon, Aug 29, 2016 at 12:47:20PM +0200, Lucas Stach wrote:
> Core, bus and shader are all module input clocks. If the SoC integration
> provides the same clock for all inputs, the DT should reflect this by
> supplying the same clock for all 3 inputs.

You're making an assertion that we don't know is true.  There is no
evidence that the GC600 has three input clocks.  Just because iMX
Vivante GPUs have three clocks does not mean that all Vivante IP has
three clock inputs.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel