On Tue, 2009-02-03 at 09:04 +0200, Mike Rapoport wrote:
> Different SPI controllers pose different alignment requirements on DMAable
> buffers. It's possible to alter the SPI controller driver to use it's own
> properly aligned buffers for DMA and copy the data it gets from the client
> into these buffers. However, this approach also impacts overall performance.
> Adding ability to set DMA buffers alignment for SPI interface in compile
> time prevents unnecessary memcpy calls.
I'd still rather that alignment constraints were handled in the
*controller* code, where the constraint actually is. If this path is
followed, then every SPI-connected device for a platform would have to
have specific hacks for every platform, which is quite unmaintainable.
Is there a good way to fit the alignment constraints into the SPI
controller code?
Dan
> Signed-off-by: Mike Rapoport <mike@xxxxxxxxxxxxxx>
> ---
> drivers/net/wireless/Kconfig | 10 ++++++++++
> drivers/net/wireless/libertas/if_spi.c | 27 ++++++++++++++++-----------
> 2 files changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/wireless/Kconfig b/drivers/net/wireless/Kconfig
> index fe819a7..3bbb7b2 100644
> --- a/drivers/net/wireless/Kconfig
> +++ b/drivers/net/wireless/Kconfig
> @@ -157,6 +157,16 @@ config LIBERTAS_SPI
> ---help---
> A driver for Marvell Libertas 8686 SPI devices.
>
> +config LIBERTAS_SPI_DMA_ALIGN
> + int "DMA buffers alignment for SPI interface"
> + depends on LIBERTAS_SPI
> + default 4 if !ARCH_PXA
> + default 8 if ARCH_PXA
> + ---help---
> + Different SPI controllers pose different alignment
> + requirements on DMAable buffers. Allow proper setting of
> + this value in compile-time.
> +
> config LIBERTAS_DEBUG
> bool "Enable full debugging output in the Libertas module."
> depends on LIBERTAS
> diff --git a/drivers/net/wireless/libertas/if_spi.c b/drivers/net/wireless/libertas/if_spi.c
> index 07311e7..3499948 100644
> --- a/drivers/net/wireless/libertas/if_spi.c
> +++ b/drivers/net/wireless/libertas/if_spi.c
> @@ -33,10 +33,12 @@
> #include "dev.h"
> #include "if_spi.h"
>
> +#define DMA_ALIGN CONFIG_LIBERTAS_SPI_DMA_ALIGN
> +
> struct if_spi_packet {
> - struct list_head list;
> - u16 blen;
> - u8 buffer[0] __attribute__((aligned(4)));
> + struct list_head list;
> + u16 blen;
> + u8 buffer[0] __attribute__((aligned(DMA_ALIGN)));
> };
>
> struct if_spi_card {
> @@ -73,7 +75,7 @@ struct if_spi_card {
> struct semaphore spi_ready;
> struct semaphore spi_thread_terminated;
>
> - u8 cmd_buffer[IF_SPI_CMD_BUF_SIZE];
> + u8 cmd_buffer[IF_SPI_CMD_BUF_SIZE] __attribute__((aligned(DMA_ALIGN)));
>
> /* A buffer of incoming packets from libertas core.
> * Since we can't sleep in hw_host_to_card, we have to buffer
> @@ -665,10 +667,13 @@ static int if_spi_c2h_cmd(struct if_spi_card *card)
> BUILD_BUG_ON(IF_SPI_CMD_BUF_SIZE < LBS_CMD_BUFFER_SIZE);
> BUILD_BUG_ON(IF_SPI_CMD_BUF_SIZE < LBS_UPLD_SIZE);
>
> - /* It's just annoying if the buffer size isn't a multiple of 4, because
> - * then we might have len < IF_SPI_CMD_BUF_SIZE but
> - * ALIGN(len, 4) > IF_SPI_CMD_BUF_SIZE */
> - BUILD_BUG_ON(IF_SPI_CMD_BUF_SIZE % 4 != 0);
> + /*
> + * It's just annoying if the buffer size isn't a multiple of
> + * DMA_ALIGN, because then we might have
> + * len < IF_SPI_CMD_BUF_SIZE but
> + * ALIGN(len, DMA_ALIGN) > IF_SPI_CMD_BUF_SIZE
> + */
> + BUILD_BUG_ON(IF_SPI_CMD_BUF_SIZE % DMA_ALIGN != 0);
>
> lbs_deb_enter(LBS_DEB_SPI);
>
> @@ -691,7 +696,7 @@ static int if_spi_c2h_cmd(struct if_spi_card *card)
>
> /* Read the data from the WLAN module into our command buffer */
> err = spu_read(card, IF_SPI_CMD_RDWRPORT_REG,
> - card->cmd_buffer, ALIGN(len, 4));
> + card->cmd_buffer, ALIGN(len, DMA_ALIGN));
> if (err)
> goto out;
>
> @@ -747,7 +752,7 @@ static int if_spi_c2h_data(struct if_spi_card *card)
> data = skb_put(skb, len);
>
> /* Read the data from the WLAN module into our skb... */
> - err = spu_read(card, IF_SPI_DATA_RDWRPORT_REG, data, ALIGN(len, 4));
> + err = spu_read(card, IF_SPI_DATA_RDWRPORT_REG, data, ALIGN(len, DMA_ALIGN));
> if (err)
> goto free_skb;
>
> @@ -940,7 +945,7 @@ static int if_spi_host_to_card(struct lbs_private *priv,
> err = -EINVAL;
> goto out;
> }
> - blen = ALIGN(nb, 4);
> + blen = ALIGN(nb, DMA_ALIGN);
> packet = kzalloc(sizeof(struct if_spi_packet) + blen, GFP_ATOMIC);
> if (!packet) {
> err = -ENOMEM;
> --
> 1.5.6.4
>
>
> --
> Sincerely yours,
> Mike.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
