Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Improve DMA chaining for ahash requests
From: Romain Perier @ 2016-10-03 15:48 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161003151739.11615-1-romain.perier@free-electrons.com>

Hello,

Le 03/10/2016 17:17, Romain Perier a ?crit :
> This series contain performance improvement regarding ahash requests.
> So far, ahash requests were systematically not chained at the DMA level.
> However, in some case, like this is the case by using IPSec, some ahash
> requests can be processed directly by the engine, and don't have
> intermediaire partial update states.
>
> This series firstly re-work the way outer IVs are copied from the SRAM
> into the dma pool. To do so, we introduce a common dma pool for all type
> of requests that contains outer results (like IV or digest). Then, for
> ahash requests that can be processed directly by the engine, outer
> results are copied from the SRAM into the common dma pool. These requests
> are then allowed to be chained at the DMA level.
>
>
> Benchmarking results with iperf throught IPSec
> ==============================================
> 		ESP			AH
>
> Before		343 Mbits/s		492 Mbits/s
> After		392 Mbits/s		557 Mbits/s
> Improvement	+14%			+13%
>
> Romain Perier (2):
>    crypto: marvell - Use an unique pool to copy results of requests
>    crypto: marvell - Don't break chain for computable last ahash requests
>
>   drivers/crypto/marvell/cesa.c   |  4 ---
>   drivers/crypto/marvell/cesa.h   |  5 ++-
>   drivers/crypto/marvell/cipher.c |  6 ++--
>   drivers/crypto/marvell/hash.c   | 79 +++++++++++++++++++++++++++++++++--------
>   drivers/crypto/marvell/tdma.c   | 17 +++++----
>   5 files changed, 78 insertions(+), 33 deletions(-)
>

After an internal discussion, we can handle things differently. Instead 
of allocating a new mv_cesa_op_ctx in the op_pool, we can just allocate 
a new descriptor which points to the first op ctx of the chain, and then 
copy outer data.

Please ignore this series.

Regards,

-- 
Romain Perier, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply

* [PATCH] drm/sun4i: Check that the plane coordinates are not negative
From: Boris Brezillon @ 2016-10-03 16:18 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161003125811.GH5228@lukather>

On Mon, 3 Oct 2016 14:58:11 +0200
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> Hi Boris,
> 
> On Fri, Sep 30, 2016 at 06:08:26PM +0200, Boris Brezillon wrote:
> > On Fri, 30 Sep 2016 16:33:20 +0200
> > Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> >   
> > > Our planes cannot be set at negative coordinates. Make sure we reject such
> > > configuration.
> > > 
> > > Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > ---
> > >  drivers/gpu/drm/sun4i/sun4i_layer.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
> > > index f0035bf5efea..f5463c4c2cde 100644
> > > --- a/drivers/gpu/drm/sun4i/sun4i_layer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
> > > @@ -29,6 +29,9 @@ struct sun4i_plane_desc {
> > >  static int sun4i_backend_layer_atomic_check(struct drm_plane *plane,
> > >  					    struct drm_plane_state *state)
> > >  {
> > > +	if ((state->crtc_x < 0) || (state->crtc_y < 0))
> > > +		return -EINVAL;
> > > +  
> > 
> > Hm, I think it's a perfectly valid use case from the DRM framework and
> > DRM user PoV: you may want to place your plane at a negative CRTC
> > offset (which means part of the plane will be hidden).
> > 
> > Maybe I'm wrong, but it seems you can support that by adapting the
> > start address of your framebuffer pointer and the layer size.  
> 
> Indeed, that would probably work. This is even somewhat what we've
> been using to implement the VGA hack we use on the CHIP.
> 
> Can you send that patch?

Actually, Ville suggested a slightly different approach: use the
->src and ->dst in drm_plane_state.

^ permalink raw reply

* [PATCH v2 1/2] dt-bindings: add bindings doc for ZTE VOU display controller
From: Rob Herring @ 2016-10-03 17:44 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1474727185-24180-2-git-send-email-shawn.guo@linaro.org>

On Sat, Sep 24, 2016 at 10:26:24PM +0800, Shawn Guo wrote:
> It adds initial bindings doc for ZTE VOU display controller.  HDMI is
> the only supported output device right now.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  .../devicetree/bindings/display/zte,vou.txt        | 86 ++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/zte,vou.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/zte,vou.txt b/Documentation/devicetree/bindings/display/zte,vou.txt
> new file mode 100644
> index 000000000000..d03ba4c4810c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/zte,vou.txt
> @@ -0,0 +1,86 @@
> +ZTE VOU Display Controller
> +
> +This is a display controller found on ZTE ZX296718 SoC.  It includes multiple
> +Graphic Layer (GL) and Video Layer (VL), two Mixers/Channels, and a few blocks
> +handling scaling, color space conversion etc.  VOU also integrates the support
> +for typical output devices, like HDMI, TV Encoder, VGA, and RGB LCD.
> +
> +* Master VOU node
> +
> +It must be the parent node of all the sub-device nodes.
> +
> +Required properties:
> + - compatible: should be "zte,zx296718-vou"
> + - #address-cells: should be <1>
> + - #size-cells: should be <1>
> + - reg: Physical base address and length of the whole VOU IO region
> + - ranges: to allow probing of sub-devices
> +
> +* VOU DPC device
> +
> +Required properties:
> + - compatible: should be "zte,zx296718-dpc"
> + - reg: Physical base address and length of DPC register regions, one for each
> +   entry in 'reg-names'
> + - reg-names: The names of register regions. The following regions are required:
> +	"osd"
> +	"timing_ctrl"
> +	"dtrc"
> +	"vou_ctrl"
> +	"otfppu"
> + - interrupts: VOU DPC interrupt number to CPU
> + - clocks: A list of phandle + clock-specifier pairs, one for each entry
> +   in 'clock-names'
> + - clock-names: A list of clock names.  The following clocks are required:
> +	"aclk"
> +	"ppu_wclk"
> +	"main_wclk"
> +	"aux_wclk"
> +
> +* HDMI output device
> +
> +Required properties:
> + - compatible: should be "zte,zx296718-hdmi"
> + - reg: Physical base address and length of the HDMI device IO region
> + - interrupts : HDMI interrupt number to CPU
> + - clocks: A list of phandle + clock-specifier pairs, one for each entry
> +   in 'clock-names'
> + - clock-names: A list of clock names.  The following clocks are required:
> +	"osc_cec"
> +	"osc_clk"
> +	"xclk"
> +
> +Example:
> +
> +vou: vou at 1440000 {
> +	compatible = "zte,zx296718-vou";
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	reg = <0x1440000 0x10000>;
> +	ranges;

You still have overlapping addresses. Explicitly list the sub ranges in 
reg here used by the VOU driver if the driver usage doesn't overlap. If 
there is overlap (2 drivers accessing the same range), then you need 
some APIs between the components (or possibly regmap).

Also, don't do an empty ranges here. Fill it in so the child nodes are 
just offsets of 0x1440000

> +
> +	dpc: dpc at 1440000 {
> +		compatible = "zte,zx296718-dpc";
> +		reg = <0x1440000 0x1000>, <0x1441000 0x1000>,
> +		      <0x1445000 0x1000>, <0x1446000 0x1000>,
> +		      <0x144a000 0x1000>;
> +		reg-names = "osd", "timing_ctrl",
> +			    "dtrc", "vou_ctrl",
> +			    "otfppu";
> +		interrupts = <GIC_SPI 81 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&topcrm VOU_ACLK>, <&topcrm VOU_PPU_WCLK>,
> +			 <&topcrm VOU_MAIN_WCLK>, <&topcrm VOU_AUX_WCLK>;
> +		clock-names = "aclk", "ppu_wclk",
> +			      "main_wclk", "aux_wclk";
> +	};
> +
> +	hdmi: hdmi at 144c000 {
> +		compatible = "zte,zx296718-hdmi";
> +		reg = <0x144c000 0x4000>;
> +		interrupts = <GIC_SPI 82 IRQ_TYPE_EDGE_RISING>;
> +		clocks = <&topcrm HDMI_OSC_CEC>,
> +			 <&topcrm HDMI_OSC_CLK>,
> +			 <&topcrm HDMI_XCLK>;
> +		clock-names = "osc_cec", "osc_clk", "xclk";
> +	};
> +};
> -- 
> 1.9.1
> 

^ permalink raw reply

* [PATCH v1 1/2] ARM: dts: vfxxx: Enable DMA for DSPI on Vybrid
From: Stefan Agner @ 2016-10-03 18:05 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <63fc108e2df00d2297a9b7014955f203bc802f34.1475498805.git.maitysanchayan@gmail.com>

On 2016-10-03 05:50, Sanchayan Maity wrote:
> Enable DMA for DSPI on Vybrid.

Hm, we have that in 4.4 already, is that meant for 4.1?

> 
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
>  arch/arm/boot/dts/vf-colibri.dtsi | 4 ++++
>  arch/arm/boot/dts/vfxxx.dtsi      | 6 ++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/vf-colibri.dtsi
> b/arch/arm/boot/dts/vf-colibri.dtsi
> index b741709..21bfef9 100644
> --- a/arch/arm/boot/dts/vf-colibri.dtsi
> +++ b/arch/arm/boot/dts/vf-colibri.dtsi
> @@ -108,6 +108,10 @@
>  	status = "okay";
>  };
>  
> +&edma1 {
> +	status = "okay";
> +};
> +
>  &esdhc1 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_esdhc1>;
> diff --git a/arch/arm/boot/dts/vfxxx.dtsi b/arch/arm/boot/dts/vfxxx.dtsi
> index 2c13ec6..eac4213 100644
> --- a/arch/arm/boot/dts/vfxxx.dtsi
> +++ b/arch/arm/boot/dts/vfxxx.dtsi
> @@ -194,6 +194,9 @@
>  				clocks = <&clks VF610_CLK_DSPI0>;
>  				clock-names = "dspi";
>  				spi-num-chipselects = <6>;
> +				dmas = <&edma1 1 12>,
> +					<&edma1 1 13>;
> +				dma-names = "rx", "tx";
>  				status = "disabled";
>  			};
>  
> @@ -206,6 +209,9 @@
>  				clocks = <&clks VF610_CLK_DSPI1>;
>  				clock-names = "dspi";
>  				spi-num-chipselects = <4>;
> +				dmas = <&edma1 1 14>,
> +					<&edma1 1 15>;
> +				dma-names = "rx", "tx";
>  				status = "disabled";
>  			};

^ permalink raw reply

* [PATCH v5] devicetree: bindings: uart: Add new compatible string for ZynqMP
From: Rob Herring @ 2016-10-03 18:42 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1474898801-36571-1-git-send-email-navam@xilinx.com>

On Mon, Sep 26, 2016 at 07:36:41PM +0530, Nava kishore Manne wrote:
> From: Nava kishore Manne <nava.manne@xilinx.com>
> 
> This patch Adds the new compatible string for ZynqMP SoC.
> 
> Signed-off-by: Nava kishore Manne <navam@xilinx.com>
> ---
> Changes for v5:
> 		-Fixed some minor comments.

Not a useful changelog. The point of these comments is to remind the 
reviewers of what they commented on.

> Changes for v4:
>                 -Modified the ChangeLog comment.
> Changes for v3:
>                 -Added changeLog comment.
> Changes for v2:
>                 -None
> 
>  Documentation/devicetree/bindings/serial/cdns,uart.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/serial/cdns,uart.txt b/Documentation/devicetree/bindings/serial/cdns,uart.txt
> index a3eb154..30c885c 100644
> --- a/Documentation/devicetree/bindings/serial/cdns,uart.txt
> +++ b/Documentation/devicetree/bindings/serial/cdns,uart.txt
> @@ -1,7 +1,9 @@
>  Binding for Cadence UART Controller
>  
>  Required properties:
> -- compatible : should be "cdns,uart-r1p8", or "xlnx,xuartps"
> +- compatible :
> +  Use "cdns,uart-r1p8", or "xlnx,xuartps" for Zynq-7xxx SoC.

This is still not right. It was wrong before, but you are touching it so 
make it right. 

OR doesn't match the dts files. It is AND and the opposite order.

xlnx,xuartps wasn't the best naming, but it's in use so we'll have to 
live with it for zynq-7xxx.

> +  Use "cdns,uart-r1p12" for Zynq Ultrascale+ MPSoC.

What I meant here was this should have something like "xlnx,mpsoc-uart" 
as the first compatible with "cdns,uart-r1p12" as the second. Not sure 
if "mpsoc" is the best name here. 

Rob

^ permalink raw reply

* [PATCH 0/4] staging: Fix build of VCHIQ driver.
From: Eric Anholt @ 2016-10-03 18:52 UTC (permalink / raw)
  To: linux-arm-kernel

This is a series of fixes for the vchiq driver that Greg recently
added to his staging-testing branch.

A full tree with nasty patches to add the other drivers necessary to
do "vcdbg log msg" is available at:

https://github.com/anholt/linux/tree/vchiq

Eric Anholt (4):
  staging/vchi: Convert to current get_user_pages() arguments.
  staging/vchi: Update for rename of page_cache_release() to put_page().
  ARM: bcm2835: Add #define for VCHIQ property message.
  drivers/vchi: Remove dependency on CONFIG_BROKEN.

 drivers/staging/vc04_services/Kconfig                              | 2 +-
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 6 +++---
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c      | 5 ++---
 include/soc/bcm2835/raspberrypi-firmware.h                         | 2 ++
 4 files changed, 8 insertions(+), 7 deletions(-)

-- 
2.9.3

^ permalink raw reply

* [PATCH 1/4] staging/vchi: Convert to current get_user_pages() arguments.
From: Eric Anholt @ 2016-10-03 18:52 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161003185209.27733-1-eric@anholt.net>

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 2 +-
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c      | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index c29040fdf9a7..18f268ee6d4b 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -420,7 +420,7 @@ create_pagelist(char __user *buf, size_t count, unsigned short type,
 		*need_release = 0; /* do not try and release vmalloc pages */
 	} else {
 		down_read(&task->mm->mmap_sem);
-		actual_pages = get_user_pages(task, task->mm,
+		actual_pages = get_user_pages(
 				          (unsigned long)buf & ~(PAGE_SIZE - 1),
 					  num_pages,
 					  (type == PAGELIST_READ) /*Write */ ,
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index e11c0e07471b..56c2c48f6801 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1473,8 +1473,7 @@ dump_phys_mem(void *virt_addr, uint32_t num_bytes)
 	}
 
 	down_read(&current->mm->mmap_sem);
-	rc = get_user_pages(current,      /* task */
-		current->mm,              /* mm */
+	rc = get_user_pages(
 		(unsigned long)virt_addr, /* start */
 		num_pages,                /* len */
 		0,                        /* write */
-- 
2.9.3

^ permalink raw reply related

* [PATCH 2/4] staging/vchi: Update for rename of page_cache_release() to put_page().
From: Eric Anholt @ 2016-10-03 18:52 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161003185209.27733-1-eric@anholt.net>

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 4 ++--
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c      | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index 18f268ee6d4b..4cb5bff23728 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -439,7 +439,7 @@ create_pagelist(char __user *buf, size_t count, unsigned short type,
 			while (actual_pages > 0)
 			{
 				actual_pages--;
-				page_cache_release(pages[actual_pages]);
+				put_page(pages[actual_pages]);
 			}
 			kfree(pagelist);
 			if (actual_pages == 0)
@@ -578,7 +578,7 @@ free_pagelist(PAGELIST_T *pagelist, int actual)
 				offset = 0;
 				set_page_dirty(pg);
 			}
-			page_cache_release(pg);
+			put_page(pg);
 		}
 	}
 
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 56c2c48f6801..47df1af2219d 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1512,7 +1512,7 @@ dump_phys_mem(void *virt_addr, uint32_t num_bytes)
 		kunmap(page);
 
 	for (page_idx = 0; page_idx < num_pages; page_idx++)
-		page_cache_release(pages[page_idx]);
+		put_page(pages[page_idx]);
 
 	kfree(pages);
 }
-- 
2.9.3

^ permalink raw reply related

* [PATCH 3/4] ARM: bcm2835: Add #define for VCHIQ property message.
From: Eric Anholt @ 2016-10-03 18:52 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161003185209.27733-1-eric@anholt.net>

This comes from the downstream tree and is needed for the new VCHIQ
driver in staging.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 include/soc/bcm2835/raspberrypi-firmware.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h
index 3fb357193f09..a06baffdf580 100644
--- a/include/soc/bcm2835/raspberrypi-firmware.h
+++ b/include/soc/bcm2835/raspberrypi-firmware.h
@@ -109,6 +109,8 @@ enum rpi_firmware_property_tag {
 	RPI_FIRMWARE_FRAMEBUFFER_SET_OVERSCAN =               0x0004800a,
 	RPI_FIRMWARE_FRAMEBUFFER_SET_PALETTE =                0x0004800b,
 
+	RPI_FIRMWARE_VCHIQ_INIT =                             0x00048010,
+
 	RPI_FIRMWARE_GET_COMMAND_LINE =                       0x00050001,
 	RPI_FIRMWARE_GET_DMA_CHANNELS =                       0x00060001,
 };
-- 
2.9.3

^ permalink raw reply related

* [PATCH 4/4] drivers/vchi: Remove dependency on CONFIG_BROKEN.
From: Eric Anholt @ 2016-10-03 18:52 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161003185209.27733-1-eric@anholt.net>

The driver builds now.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/staging/vc04_services/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vc04_services/Kconfig b/drivers/staging/vc04_services/Kconfig
index 9676fb29075a..db8e1beb89f9 100644
--- a/drivers/staging/vc04_services/Kconfig
+++ b/drivers/staging/vc04_services/Kconfig
@@ -1,6 +1,6 @@
 config BCM2708_VCHIQ
 	tristate "Videocore VCHIQ"
-	depends on RASPBERRYPI_FIRMWARE && BROKEN
+	depends on RASPBERRYPI_FIRMWARE
 	default y
 	help
 		Kernel to VideoCore communication interface for the
-- 
2.9.3

^ permalink raw reply related

* [PATCH 4/8] pinctrl: aspeed-g5: Fix pin association of SPI1 function
From: Rob Herring @ 2016-10-03 18:57 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <bdd34f8c4bfabbc1d3cd05a66ac8734da514b1e5.1474986045.git-series.andrew@aj.id.au>

On Wed, Sep 28, 2016 at 12:20:16AM +0930, Andrew Jeffery wrote:
> The SPI1 function was associated with the wrong pins: The functions that
> those pins provide is either an SPI debug or passthrough function
> coupled to SPI1. Make the SPI1 mux function configure the relevant pins
> and associate new SPI1DEBUG and SPI1PASSTHRU functions with the pins
> that were already defined.
> 
> The notation used in the datasheet's multi-function pin table for the SoC is
> often creative: in this case the SYS* signals are enabled by a single bit,
> which is nothing unusual on its own, but in this case the bit was also
> participating in a multi-bit bitfield and therefore represented multiple
> functions. This fact was overlooked in the original patch.
> 
> Fixes: 56e57cb6c07f (pinctrl: Add pinctrl-aspeed-g5 driver)
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  Documentation/devicetree/bindings/pinctrl/pinctrl-aspeed.txt |  4 +-

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c                   | 86 ++++++-
>  2 files changed, 81 insertions(+), 9 deletions(-)

^ permalink raw reply

* [PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts
From: Eric Anholt @ 2016-10-03 18:58 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <0c943ee9-d152-79c4-2ad7-8a88c2d9c960@tronnes.org>

Noralf Tr?nnes <noralf@tronnes.org> writes:

> Den 29.09.2016 07:37, skrev Stefan Wahren:
>>> Noralf Tr?nnes <noralf@tronnes.org> hat am 29. September 2016 um 00:22
>>> geschrieben:
>>>
>>>
>>>
>>> Den 29.09.2016 00:00, skrev Eric Anholt:
>>>> Noralf Tr?nnes <noralf@tronnes.org> writes:
>>>>
>>>>> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
>>>>> the driver has no way to fill/drain the FIFO to stop the interrupts.
>>>>> In this case the controller has to be disabled and the transfer
>>>>> completed to avoid hang.
>>>>>
>>>>> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
>>>>> the controller is disabled in the transfer function after completion.
>>>>> Unite the code paths and do disabling inside the interrupt routine.
>>>>>
>>>>> Clear interrupt status bits in the united completion path instead of
>>>>> trying to do it on every interrupt which isn't necessary.
>>>>> Only CLKT, ERR and DONE can be cleared that way.
>>>>>
>>>>> Add the status value to the error value in case of TXW/RXR errors to
>>>>> distinguish them from the other S_LEN error.
>>>> I was surprised that not writing the TXW/RXR bits on handling their
>>>> interrupts was OK, given that we were doing so before, but it's a level
>>>> interrupt and those bits are basically ignored on write.
>>>>
>>>> This patch and 3, 4, and 6 are:
>>>>
>>>> Reviewed-by: Eric Anholt <eric@anholt.net>
>>>>
>>>> Patch 5 is:
>>>>
>>>> Acked-by: Eric Anholt <eric@anholt.net>
>>>>
>>>> Note for future debug: The I2C_C_CLEAR on errors will take some time to
>>>> resolve -- if you were in non-idle state and I2C_C_READ, it sets an
>>>> abort_rx flag and runs through the state machine to send a NACK and a
>>>> STOP, I think.  Since we're setting CLEAR without I2CEN, that NACK will
>>>> be hanging around queued up for next time we start the engine.
>>> Maybe you're able to explain the issues I had with reset:
>>> https://github.com/raspberrypi/linux/issues/1653
>>>
>>> Should we put your note into the commit message?
>>> It will most likely be lost if it just stays in this email.
>> I prefer to have this kind of information as a code comment.
>
> Eric, does this look good to you as a code comment:
>
> /*
>   * Note about I2C_C_CLEAR on error:
>   * The I2C_C_CLEAR on errors will take some time to resolve -- if you 
> were in
>   * non-idle state and I2C_C_READ, it sets an abort_rx flag and runs through
>   * the state machine to send a NACK and a STOP. Since we're setting CLEAR
>   * without I2CEN, that NACK will be hanging around queued up for next time
>   * we start the engine.
>   */
>
>
> If it is, I'll resend the series with this change and add all the ack's 
> and r-b's.

Looks good to me.  Thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161003/29b87d31/attachment.sig>

^ permalink raw reply

* [PATCH] bus: qcom-ebi2: depend on HAS_IOMEM
From: Stephen Boyd @ 2016-10-03 19:04 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1475445239-25006-1-git-send-email-linus.walleij@linaro.org>

On 10/02/2016 02:53 PM, Linus Walleij wrote:
> After being asked to not depend on ARCH_QCOM* or similar,
> unsurprisingly compilation fails on UM as it has no I/O
> memory:

We could have depends on ARCH_QCOM || COMPILE_TEST and then life is
good. The HAS_IOMEM would of course stick around for compile testing.

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

^ permalink raw reply

* [PATCH 7/8] pinctrl: aspeed-g4: Add mux configuration for all pins
From: Rob Herring @ 2016-10-03 19:08 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <e0d8fa6cd444972e6f048f98da98f0439e6ca39b.1474986045.git-series.andrew@aj.id.au>

On Wed, Sep 28, 2016 at 12:20:19AM +0930, Andrew Jeffery wrote:
> The patch introducing the g4 pinctrl driver implemented a smattering of
> pins to flesh out the implementation of the core and provide bare-bones
> support for some OpenPOWER platforms. Now, update the bindings document
> to reflect the complete functionality and implement the necessary pin
> configuration tables in the driver.

We prefer bindings to be complete if possible where as drivers can be 
expanded over time.

> 
> Cc: Timothy Pearson <tpearson@raptorengineering.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  Documentation/devicetree/bindings/pinctrl/pinctrl-aspeed.txt |   19 +-

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/pinctrl/aspeed/pinctrl-aspeed-g4.c                   | 1094 ++++++-
>  2 files changed, 1093 insertions(+), 20 deletions(-)

^ permalink raw reply

* [RFC] arm64: Enforce observed order for spinlock and data
From: bdegraaf at codeaurora.org @ 2016-10-03 19:20 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161001181101.GA17554@remoulade>

On 2016-10-01 14:11, Mark Rutland wrote:
> Hi Brent,
> 
> Evidently my questions weren't sufficiently clear; even with your 
> answers it's
> not clear to me what precise issue you're attempting to solve. I've 
> tried to be
> more specific this time.
> 
> At a high-level, can you clarify whether you're attempting to solve is:
> 
> (a) a functional correctness issue (e.g. data corruption)
> (b) a performance issue
> 
> And whether this was seen in practice, or found through code 
> inspection?
> 
> On Sat, Oct 01, 2016 at 12:11:36PM -0400, bdegraaf at codeaurora.org 
> wrote:
>> On 2016-09-30 15:32, Mark Rutland wrote:
>> >On Fri, Sep 30, 2016 at 01:40:57PM -0400, Brent DeGraaf wrote:
>> >>+	 * so LSE needs an explicit barrier here as well.  Without this, the
>> >>+	 * changed contents of the area protected by the spinlock could be
>> >>+	 * observed prior to the lock.
>> >>+	 */
>> >
>> >By whom? We generally expect that if data is protected by a lock, you take
>> >the lock before accessing it. If you expect concurrent lockless readers,
>> >then there's a requirement on the writer side to explicitly provide the
>> >ordering it requires -- spinlocks are not expected to provide that.
>> 
>> More details are in my response to Robin, but there is an API arm64 
>> supports
>> in spinlock.h which is used by lockref to determine whether a lock is 
>> free or
>> not.  For that code to work properly without adding these barriers, 
>> that API
>> needs to take the lock.
> 
> Can you please provide a concrete example of a case where things go 
> wrong,
> citing the code (or access pattern) in question? e.g. as in the commit 
> messages
> for:
> 
>   8e86f0b409a44193 ("arm64: atomics: fix use of acquire + release for
> full barrier semantics)
>   859b7a0e89120505 ("mm/slub: fix lockups on PREEMPT && !SMP kernels")
> 
> (for the latter, I messed up the register -> var mapping in one 
> paragraph, but
> the style and reasoning is otherwise sound).
> 
> In the absence of a concrete example as above, it's very difficult to 
> reason
> about what the underlying issue is, and what a valid fix would be for 
> said
> issue.
> 
>> >What pattern of accesses are made by readers and writers such that there is
>> >a problem?
> 
> Please note here I was asking specifically w.r.t. the lockref code, 
> e.g. which
> loads could see stale data, and what does the code do based on this 
> value such
> that there is a problem.
> 
>> I added the barriers to the readers/writers because I do not know 
>> these are
>> not similarly abused.  There is a lot of driver code out there, and 
>> ensuring
>> order is the safest way to be sure we don't get burned by something 
>> similar
>> to the lockref access.
> 
> Making the architecture-provided primitives overly strong harms 
> performance and
> efficiency (in general), makes the code harder to maintain and optimise 
> in
> future, and only masks the issue (which could crop up on other 
> architectures,
> for instance).
> 
> Thanks,
> Mark.


Thinking about this, as the reader/writer code has no known "abuse" 
case, I'll
remove it from the patchset, then provide a v2 patchset with a detailed
explanation for the lockref problem using the commits you provided as an 
example,
as well as performance consideration.

Brent

^ permalink raw reply

* [PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts
From: Eric Anholt @ 2016-10-03 19:42 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <d93a0590-1783-b913-9362-007628cb17d6@tronnes.org>

Noralf Tr?nnes <noralf@tronnes.org> writes:

> Den 29.09.2016 00:00, skrev Eric Anholt:
>> Noralf Tr?nnes <noralf@tronnes.org> writes:
>>
>>> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
>>> the driver has no way to fill/drain the FIFO to stop the interrupts.
>>> In this case the controller has to be disabled and the transfer
>>> completed to avoid hang.
>>>
>>> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
>>> the controller is disabled in the transfer function after completion.
>>> Unite the code paths and do disabling inside the interrupt routine.
>>>
>>> Clear interrupt status bits in the united completion path instead of
>>> trying to do it on every interrupt which isn't necessary.
>>> Only CLKT, ERR and DONE can be cleared that way.
>>>
>>> Add the status value to the error value in case of TXW/RXR errors to
>>> distinguish them from the other S_LEN error.
>> I was surprised that not writing the TXW/RXR bits on handling their
>> interrupts was OK, given that we were doing so before, but it's a level
>> interrupt and those bits are basically ignored on write.
>>
>> This patch and 3, 4, and 6 are:
>>
>> Reviewed-by: Eric Anholt <eric@anholt.net>
>>
>> Patch 5 is:
>>
>> Acked-by: Eric Anholt <eric@anholt.net>
>>
>> Note for future debug: The I2C_C_CLEAR on errors will take some time to
>> resolve -- if you were in non-idle state and I2C_C_READ, it sets an
>> abort_rx flag and runs through the state machine to send a NACK and a
>> STOP, I think.  Since we're setting CLEAR without I2CEN, that NACK will
>> be hanging around queued up for next time we start the engine.
>
> Maybe you're able to explain the issues I had with reset:
> https://github.com/raspberrypi/linux/issues/1653

One of the questions I think you might have is "what state does the
controller end up in after the various interrupts?"

ERR:
- produced if we get a nack that's not at the end of a read.

- Proceeds to repeated start if BCM2835_I2C_C_ST is queued, otherwise
  stop.

CLKT:
- Triggered by a counter outside of the state machine when stretching
  happens and then times out.

- Sets cs_override, which causes proceeding through the state machine as
  if the clock wasn't getting stretched, until the end of the next byte
  sent/received.

- According to Wolfram we shouldn't be timing out on clock stretching
  for i2c, just on the transfer as a whole
  (https://patchwork.kernel.org/patch/9148431/), so I wrote
  https://github.com/anholt/linux/commit/894200276239d2e4c60b378bdc52164fcb13af8d.
  However, I don't see an obvious way to get back to IDLE while the
  slave is still stretching, without triggering the clock stretching
  timeout path.

DONE:
- Signaled at STOP, and just moves to IDLE state which keeps scl/sda
  high and waits for a BCM2835_I2C_C_ST while we're not clearing the
  FIFOs (if you do signal start while the fifos are clearing, the start
  will hang around until the fifo clear is done).  This is the only way
  to get to IDLE.

I'm don't think I have an answer to the "what should I do?" question you
had, but hopefully this helps.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161003/0b6762c3/attachment.sig>

^ permalink raw reply

* [PATCH] i2c: bcm2835: Set up the clock stretching timeout at boot.
From: Eric Anholt @ 2016-10-03 19:50 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20160722072740.GG1605@katana>

Wolfram Sang <wsa@the-dreams.de> writes:

> On Sun, Jul 03, 2016 at 06:02:32PM -0700, Eric Anholt wrote:
>> Wolfram Sang <wsa@the-dreams.de> writes:
>> 
>> >> +	/*
>> >> +	 * SMBUS says "Devices participating in a transfer will
>> >> +	 * timeout when any clock low exceeds the value of
>> >> +	 * T_TIMEOUT,MIN of 25 ms."
>> >> +	 */
>> >
>> > SMBus has that timeout, but I2C doesn't. How about disabling the timeout
>> > simply? Or using the max value if you want to keep the timeout
>> > detection?
>> 
>> Disabling the timeout seems fine to me.  We still have a 1-second
>> timeout around the entire transfer.  I'll be back on my DSI branch this
>> week and test it out then.
>
> Did it work?

Sorry for the long-delayed feedback: It turned out that the reason I was
getting timeouts and looking into i2c in the first place was that the
firmware was driving that controller behind my back, so I couldn't do
useful testing anyway.

I put together a patch
(https://github.com/anholt/linux/commit/894200276239d2e4c60b378bdc52164fcb13af8d)
but I'm a bit concerned by it: I don't see a way to get the controller
back to its idle state without continuing through the I2C state machine,
and if the clock is still being stretched it doesn't continue unless CLK
is triggered.

What is supposed to happen when adap->timeout times out while the clock
is being stretched?  Should we be able to try starting a fresh new I2C
transaction cleanly?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161003/880af017/attachment.sig>

^ permalink raw reply

* [PATCH -next v2] arm*/efi: efi_init error handling fix
From: Matt Fleming @ 2016-10-03 19:51 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1474371593-10250-1-git-send-email-xieyisheng1@huawei.com>

On Tue, 20 Sep, at 07:39:53PM, Yisheng Xie wrote:
> There's an early memmap leak in efi_init error path, fix it.
> 
> Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
> ---
>  drivers/firmware/efi/arm-init.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
> index 9f8d996..7c2fe57 100644
> --- a/drivers/firmware/efi/arm-init.c
> +++ b/drivers/firmware/efi/arm-init.c
> @@ -245,8 +245,10 @@ void __init efi_init(void)
>  	     "Unexpected EFI_MEMORY_DESCRIPTOR version %ld",
>  	      efi.memmap.desc_version);
>  
> -	if (uefi_init() < 0)
> +	if (uefi_init() < 0) {
> +		efi_memmap_unmap();
>  		return;
> +	}
>  
>  	reserve_regions();
>  	efi_memattr_init();

Thanks, applied to 'next'.

^ permalink raw reply

* [PATCH v4 0/7] i2c: bcm2835: Bring in changes from downstream
From: Noralf Trønnes @ 2016-10-03 20:06 UTC (permalink / raw)
  To: linux-arm-kernel

This patchset tries to bring in the lessons learned in the downstream
driver i2c-bcm2708. The downstream clock stretcing timeout patch has
been left out since clock stretching is broken/unreliable on this
controller, so no point in setting it.

Changes since version 3:
- Add comment about I2C_C_CLEAR on error

Noralf.


Noralf Tr?nnes (7):
  i2c: bcm2835: Fix hang for writing messages larger than 16 bytes
  i2c: bcm2835: Protect against unexpected TXW/RXR interrupts
  i2c: bcm2835: Use dev_dbg logging on transfer errors
  i2c: bcm2835: Can't support I2C_M_IGNORE_NAK
  i2c: bcm2835: Add support for Repeated Start Condition
  i2c: bcm2835: Support i2c-dev ioctl I2C_TIMEOUT
  i2c: bcm2835: Add support for dynamic clock

 drivers/i2c/busses/i2c-bcm2835.c | 218 +++++++++++++++++++++++++--------------
 1 file changed, 139 insertions(+), 79 deletions(-)

--
2.8.2

^ permalink raw reply

* [PATCH v4 1/7] i2c: bcm2835: Fix hang for writing messages larger than 16 bytes
From: Noralf Trønnes @ 2016-10-03 20:06 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1475525174-28604-1-git-send-email-noralf@tronnes.org>

Writing messages larger than the FIFO size results in a hang, rendering
the machine unusable. This is because the RXD status flag is set on the
first interrupt which results in bcm2835_drain_rxfifo() stealing bytes
from the buffer. The controller continues to trigger interrupts waiting
for the missing bytes, but bcm2835_fill_txfifo() has none to give.
In this situation wait_for_completion_timeout() apparently is unable to
stop the madness.

The BCM2835 ARM Peripherals datasheet has this to say about the flags:
  TXD: is set when the FIFO has space for at least one byte of data.
  RXD: is set when the FIFO contains at least one byte of data.
  TXW: is set during a write transfer and the FIFO is less than full.
  RXR: is set during a read transfer and the FIFO is or more full.

Implementing the logic from the downstream i2c-bcm2708 driver solved
the hang problem.

Signed-off-by: Noralf Tr?nnes <noralf@tronnes.org>
Reviewed-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/i2c/busses/i2c-bcm2835.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index d4f3239..f283b71 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -64,6 +64,7 @@ struct bcm2835_i2c_dev {
 	int irq;
 	struct i2c_adapter adapter;
 	struct completion completion;
+	struct i2c_msg *curr_msg;
 	u32 msg_err;
 	u8 *msg_buf;
 	size_t msg_buf_remaining;
@@ -126,14 +127,13 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 		return IRQ_HANDLED;
 	}
 
-	if (val & BCM2835_I2C_S_RXD) {
-		bcm2835_drain_rxfifo(i2c_dev);
-		if (!(val & BCM2835_I2C_S_DONE))
-			return IRQ_HANDLED;
-	}
-
 	if (val & BCM2835_I2C_S_DONE) {
-		if (i2c_dev->msg_buf_remaining)
+		if (i2c_dev->curr_msg->flags & I2C_M_RD) {
+			bcm2835_drain_rxfifo(i2c_dev);
+			val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S);
+		}
+
+		if ((val & BCM2835_I2C_S_RXD) || i2c_dev->msg_buf_remaining)
 			i2c_dev->msg_err = BCM2835_I2C_S_LEN;
 		else
 			i2c_dev->msg_err = 0;
@@ -141,11 +141,16 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 		return IRQ_HANDLED;
 	}
 
-	if (val & BCM2835_I2C_S_TXD) {
+	if (val & BCM2835_I2C_S_TXW) {
 		bcm2835_fill_txfifo(i2c_dev);
 		return IRQ_HANDLED;
 	}
 
+	if (val & BCM2835_I2C_S_RXR) {
+		bcm2835_drain_rxfifo(i2c_dev);
+		return IRQ_HANDLED;
+	}
+
 	return IRQ_NONE;
 }
 
@@ -155,6 +160,7 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
 	u32 c;
 	unsigned long time_left;
 
+	i2c_dev->curr_msg = msg;
 	i2c_dev->msg_buf = msg->buf;
 	i2c_dev->msg_buf_remaining = msg->len;
 	reinit_completion(&i2c_dev->completion);
-- 
2.8.2

^ permalink raw reply related

* [PATCH v4 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts
From: Noralf Trønnes @ 2016-10-03 20:06 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1475525174-28604-1-git-send-email-noralf@tronnes.org>

If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
the driver has no way to fill/drain the FIFO to stop the interrupts.
In this case the controller has to be disabled and the transfer
completed to avoid hang.

(CLKT | ERR) and DONE interrupts are completed in their own paths, and
the controller is disabled in the transfer function after completion.
Unite the code paths and do disabling inside the interrupt routine.

Clear interrupt status bits in the united completion path instead of
trying to do it on every interrupt which isn't necessary.
Only CLKT, ERR and DONE can be cleared that way.

Add the status value to the error value in case of TXW/RXR errors to
distinguish them from the other S_LEN error.

Signed-off-by: Noralf Tr?nnes <noralf@tronnes.org>
Reviewed-by: Eric Anholt <eric@anholt.net>
---

Changes since v3:
- Add comment about I2C_C_CLEAR on error

 drivers/i2c/busses/i2c-bcm2835.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index f283b71..d2ba1a4 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -50,8 +50,6 @@
 #define BCM2835_I2C_S_CLKT	BIT(9)
 #define BCM2835_I2C_S_LEN	BIT(10) /* Fake bit for SW error reporting */

-#define BCM2835_I2C_BITMSK_S	0x03FF
-
 #define BCM2835_I2C_CDIV_MIN	0x0002
 #define BCM2835_I2C_CDIV_MAX	0xFFFE

@@ -111,20 +109,26 @@ static void bcm2835_drain_rxfifo(struct bcm2835_i2c_dev *i2c_dev)
 	}
 }

+/*
+ * Note about I2C_C_CLEAR on error:
+ * The I2C_C_CLEAR on errors will take some time to resolve -- if you were in
+ * non-idle state and I2C_C_READ, it sets an abort_rx flag and runs through
+ * the state machine to send a NACK and a STOP. Since we're setting CLEAR
+ * without I2CEN, that NACK will be hanging around queued up for next time
+ * we start the engine.
+ */
+
 static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 {
 	struct bcm2835_i2c_dev *i2c_dev = data;
 	u32 val, err;

 	val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S);
-	val &= BCM2835_I2C_BITMSK_S;
-	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_S, val);

 	err = val & (BCM2835_I2C_S_CLKT | BCM2835_I2C_S_ERR);
 	if (err) {
 		i2c_dev->msg_err = err;
-		complete(&i2c_dev->completion);
-		return IRQ_HANDLED;
+		goto complete;
 	}

 	if (val & BCM2835_I2C_S_DONE) {
@@ -137,21 +141,38 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 			i2c_dev->msg_err = BCM2835_I2C_S_LEN;
 		else
 			i2c_dev->msg_err = 0;
-		complete(&i2c_dev->completion);
-		return IRQ_HANDLED;
+		goto complete;
 	}

 	if (val & BCM2835_I2C_S_TXW) {
+		if (!i2c_dev->msg_buf_remaining) {
+			i2c_dev->msg_err = val | BCM2835_I2C_S_LEN;
+			goto complete;
+		}
+
 		bcm2835_fill_txfifo(i2c_dev);
 		return IRQ_HANDLED;
 	}

 	if (val & BCM2835_I2C_S_RXR) {
+		if (!i2c_dev->msg_buf_remaining) {
+			i2c_dev->msg_err = val | BCM2835_I2C_S_LEN;
+			goto complete;
+		}
+
 		bcm2835_drain_rxfifo(i2c_dev);
 		return IRQ_HANDLED;
 	}

 	return IRQ_NONE;
+
+complete:
+	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, BCM2835_I2C_C_CLEAR);
+	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_S, BCM2835_I2C_S_CLKT |
+			   BCM2835_I2C_S_ERR | BCM2835_I2C_S_DONE);
+	complete(&i2c_dev->completion);
+
+	return IRQ_HANDLED;
 }

 static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
@@ -181,8 +202,9 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,

 	time_left = wait_for_completion_timeout(&i2c_dev->completion,
 						BCM2835_I2C_TIMEOUT);
-	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, BCM2835_I2C_C_CLEAR);
 	if (!time_left) {
+		bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C,
+				   BCM2835_I2C_C_CLEAR);
 		dev_err(i2c_dev->dev, "i2c transfer timed out\n");
 		return -ETIMEDOUT;
 	}
--
2.8.2

^ permalink raw reply related

* [PATCH v4 3/7] i2c: bcm2835: Use dev_dbg logging on transfer errors
From: Noralf Trønnes @ 2016-10-03 20:06 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1475525174-28604-1-git-send-email-noralf@tronnes.org>

Writing to an AT24C32 generates on average 2x i2c transfer errors per
32-byte page write. Which amounts to a lot for a 4k write. This is due
to the fact that the chip doesn't respond during it's internal write
cycle when the at24 driver tries and retries the next write.
Only a handful drivers use dev_err() on transfer error, so switch to
dev_dbg() instead.

Signed-off-by: Noralf Tr?nnes <noralf@tronnes.org>
Reviewed-by: Eric Anholt <eric@anholt.net>
---
 drivers/i2c/busses/i2c-bcm2835.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index d2ba1a4..54d510a 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -216,7 +216,7 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
 	    (msg->flags & I2C_M_IGNORE_NAK))
 		return 0;
 
-	dev_err(i2c_dev->dev, "i2c transfer failed: %x\n", i2c_dev->msg_err);
+	dev_dbg(i2c_dev->dev, "i2c transfer failed: %x\n", i2c_dev->msg_err);
 
 	if (i2c_dev->msg_err & BCM2835_I2C_S_ERR)
 		return -EREMOTEIO;
-- 
2.8.2

^ permalink raw reply related

* [PATCH v4 4/7] i2c: bcm2835: Can't support I2C_M_IGNORE_NAK
From: Noralf Trønnes @ 2016-10-03 20:06 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1475525174-28604-1-git-send-email-noralf@tronnes.org>

The controller can't support this flag, so remove it.

Documentation/i2c/i2c-protocol states that all of the message is sent:

I2C_M_IGNORE_NAK:
    Normally message is interrupted immediately if there is [NA] from the
    client. Setting this flag treats any [NA] as [A], and all of
    message is sent.

>From the BCM2835 ARM Peripherals datasheet:

    The ERR field is set when the slave fails to acknowledge either
    its address or a data byte written to it.

So when the controller doesn't receive an ack, it sets ERR and raises
an interrupt. In other words, the whole message is not sent.

Signed-off-by: Noralf Tr?nnes <noralf@tronnes.org>
Reviewed-by: Eric Anholt <eric@anholt.net>
---
 drivers/i2c/busses/i2c-bcm2835.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 54d510a..565ef69 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -212,10 +212,6 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
 	if (likely(!i2c_dev->msg_err))
 		return 0;
 
-	if ((i2c_dev->msg_err & BCM2835_I2C_S_ERR) &&
-	    (msg->flags & I2C_M_IGNORE_NAK))
-		return 0;
-
 	dev_dbg(i2c_dev->dev, "i2c transfer failed: %x\n", i2c_dev->msg_err);
 
 	if (i2c_dev->msg_err & BCM2835_I2C_S_ERR)
-- 
2.8.2

^ permalink raw reply related

* [PATCH v4 5/7] i2c: bcm2835: Add support for Repeated Start Condition
From: Noralf Trønnes @ 2016-10-03 20:06 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1475525174-28604-1-git-send-email-noralf@tronnes.org>

Documentation/i2c/i2c-protocol states that Combined transactions should
separate messages with a Start bit and end the whole transaction with a
Stop bit. This patch adds support for issuing only a Start between
messages instead of a Stop followed by a Start.

This implementation differs from downstream i2c-bcm2708 in 2 respects:
- it uses an interrupt to detect that the transfer is active instead
  of using polling. There is no interrupt for Transfer Active, but by
  not prefilling the FIFO it's possible to use the TXW interrupt.
- when resetting/disabling the controller between transfers it writes
  CLEAR to the control register instead of just zero.
  Using just zero gave many errors. This might be the reason why
  downstream had to disable this feature and make it available with a
  module parameter.

I have run thousands of transfers to a DS1307 (rtc), MMA8451 (accel)
and AT24C32 (eeprom) in parallel without problems.

Signed-off-by: Noralf Tr?nnes <noralf@tronnes.org>
Acked-by: Eric Anholt <eric@anholt.net>
---
 drivers/i2c/busses/i2c-bcm2835.c | 101 ++++++++++++++++++++++++---------------
 1 file changed, 63 insertions(+), 38 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 565ef69..241e08a 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -63,6 +63,7 @@ struct bcm2835_i2c_dev {
 	struct i2c_adapter adapter;
 	struct completion completion;
 	struct i2c_msg *curr_msg;
+	int num_msgs;
 	u32 msg_err;
 	u8 *msg_buf;
 	size_t msg_buf_remaining;
@@ -110,6 +111,45 @@ static void bcm2835_drain_rxfifo(struct bcm2835_i2c_dev *i2c_dev)
 }
 
 /*
+ * Repeated Start Condition (Sr)
+ * The BCM2835 ARM Peripherals datasheet mentions a way to trigger a Sr when it
+ * talks about reading from a slave with 10 bit address. This is achieved by
+ * issuing a write, poll the I2CS.TA flag and wait for it to be set, and then
+ * issue a read.
+ * A comment in https://github.com/raspberrypi/linux/issues/254 shows how the
+ * firmware actually does it using polling and says that it's a workaround for
+ * a problem in the state machine.
+ * It turns out that it is possible to use the TXW interrupt to know when the
+ * transfer is active, provided the FIFO has not been prefilled.
+ */
+
+static void bcm2835_i2c_start_transfer(struct bcm2835_i2c_dev *i2c_dev)
+{
+	u32 c = BCM2835_I2C_C_ST | BCM2835_I2C_C_I2CEN;
+	struct i2c_msg *msg = i2c_dev->curr_msg;
+	bool last_msg = (i2c_dev->num_msgs == 1);
+
+	if (!i2c_dev->num_msgs)
+		return;
+
+	i2c_dev->num_msgs--;
+	i2c_dev->msg_buf = msg->buf;
+	i2c_dev->msg_buf_remaining = msg->len;
+
+	if (msg->flags & I2C_M_RD)
+		c |= BCM2835_I2C_C_READ | BCM2835_I2C_C_INTR;
+	else
+		c |= BCM2835_I2C_C_INTT;
+
+	if (last_msg)
+		c |= BCM2835_I2C_C_INTD;
+
+	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_A, msg->addr);
+	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DLEN, msg->len);
+	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, c);
+}
+
+/*
  * Note about I2C_C_CLEAR on error:
  * The I2C_C_CLEAR on errors will take some time to resolve -- if you were in
  * non-idle state and I2C_C_READ, it sets an abort_rx flag and runs through
@@ -151,6 +191,12 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 		}
 
 		bcm2835_fill_txfifo(i2c_dev);
+
+		if (i2c_dev->num_msgs && !i2c_dev->msg_buf_remaining) {
+			i2c_dev->curr_msg++;
+			bcm2835_i2c_start_transfer(i2c_dev);
+		}
+
 		return IRQ_HANDLED;
 	}
 
@@ -175,30 +221,25 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
-				struct i2c_msg *msg)
+static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
+			    int num)
 {
-	u32 c;
+	struct bcm2835_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
 	unsigned long time_left;
+	int i;
 
-	i2c_dev->curr_msg = msg;
-	i2c_dev->msg_buf = msg->buf;
-	i2c_dev->msg_buf_remaining = msg->len;
-	reinit_completion(&i2c_dev->completion);
-
-	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, BCM2835_I2C_C_CLEAR);
+	for (i = 0; i < (num - 1); i++)
+		if (msgs[i].flags & I2C_M_RD) {
+			dev_warn_once(i2c_dev->dev,
+				      "only one read message supported, has to be last\n");
+			return -EOPNOTSUPP;
+		}
 
-	if (msg->flags & I2C_M_RD) {
-		c = BCM2835_I2C_C_READ | BCM2835_I2C_C_INTR;
-	} else {
-		c = BCM2835_I2C_C_INTT;
-		bcm2835_fill_txfifo(i2c_dev);
-	}
-	c |= BCM2835_I2C_C_ST | BCM2835_I2C_C_INTD | BCM2835_I2C_C_I2CEN;
+	i2c_dev->curr_msg = msgs;
+	i2c_dev->num_msgs = num;
+	reinit_completion(&i2c_dev->completion);
 
-	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_A, msg->addr);
-	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DLEN, msg->len);
-	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, c);
+	bcm2835_i2c_start_transfer(i2c_dev);
 
 	time_left = wait_for_completion_timeout(&i2c_dev->completion,
 						BCM2835_I2C_TIMEOUT);
@@ -209,31 +250,15 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
 		return -ETIMEDOUT;
 	}
 
-	if (likely(!i2c_dev->msg_err))
-		return 0;
+	if (!i2c_dev->msg_err)
+		return num;
 
 	dev_dbg(i2c_dev->dev, "i2c transfer failed: %x\n", i2c_dev->msg_err);
 
 	if (i2c_dev->msg_err & BCM2835_I2C_S_ERR)
 		return -EREMOTEIO;
-	else
-		return -EIO;
-}
-
-static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
-			    int num)
-{
-	struct bcm2835_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
-	int i;
-	int ret = 0;
-
-	for (i = 0; i < num; i++) {
-		ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
-		if (ret)
-			break;
-	}
 
-	return ret ?: i;
+	return -EIO;
 }
 
 static u32 bcm2835_i2c_func(struct i2c_adapter *adap)
-- 
2.8.2

^ permalink raw reply related

* [PATCH v4 6/7] i2c: bcm2835: Support i2c-dev ioctl I2C_TIMEOUT
From: Noralf Trønnes @ 2016-10-03 20:06 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1475525174-28604-1-git-send-email-noralf@tronnes.org>

Use i2c_adapter->timeout for the completion timeout value. The core
default is 1 second.

Signed-off-by: Noralf Tr?nnes <noralf@tronnes.org>
Reviewed-by: Eric Anholt <eric@anholt.net>
---
 drivers/i2c/busses/i2c-bcm2835.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 241e08a..d2085dd 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -53,8 +53,6 @@
 #define BCM2835_I2C_CDIV_MIN	0x0002
 #define BCM2835_I2C_CDIV_MAX	0xFFFE
 
-#define BCM2835_I2C_TIMEOUT (msecs_to_jiffies(1000))
-
 struct bcm2835_i2c_dev {
 	struct device *dev;
 	void __iomem *regs;
@@ -242,7 +240,7 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 	bcm2835_i2c_start_transfer(i2c_dev);
 
 	time_left = wait_for_completion_timeout(&i2c_dev->completion,
-						BCM2835_I2C_TIMEOUT);
+						adap->timeout);
 	if (!time_left) {
 		bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C,
 				   BCM2835_I2C_C_CLEAR);
-- 
2.8.2

^ permalink raw reply related


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