From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Roese <sr@denx.de>
Date: Fri, 25 Jul 2014 16:50:54 +0200
Subject: [U-Boot] [PATCH v5 1/1] ARM: kirkwood: add mvsdio driver
In-Reply-To: <1406294864-6839-1-git-send-email-drEagle@doukki.net>
References: <1406294864-6839-1-git-send-email-drEagle@doukki.net>
Message-ID: <53D26ECE.2050104@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 25.07.2014 15:27, Gerald Kerma wrote:
> This patch add Marvell kirkwood MVSDIO/MMC driver
> and enable it for Sheevaplugs and OpenRD boards.
>
> Signed-off-by: Gerald Kerma <drEagle@doukki.net>

Please don't send this patch as base64 encoded. It makes it impossible 
to apply this patch or run checkpatch on it.

Did you run it though checkpatch btw?

> ---
>
>   Changes in v5:
>   - purge uneeded code
>   - remove some extra debug
>
>   Changes in v4:
>   - rename drivers files to MVSDIO
>   - fix MMC clock init which now use dev ID
>   - clean debug strings
>   - remove MVSDIO_TWEAK_NOSDHS quirk
>   - remove dead code
>
>   Changes in v3:
>   - Add MVSDIO_TWEAK_NOSDHS quirk
>   - Minor clean
>
>   Changes in v2:
>   - Fix some typo and missing lines from patch import
>
>   arch/arm/include/asm/arch-kirkwood/kirkwood.h |   1 +
>   board/Marvell/openrd/openrd.c                 |  11 +
>   board/Marvell/sheevaplug/sheevaplug.c         |  11 +
>   drivers/mmc/Makefile                          |   1 +
>   drivers/mmc/mvsdio.c                          | 353 ++++++++++++++++++++++++++

I suggest to rename the driver to mvebu_mmc.c instead. This MVEBU is the 
generally agreed upon abbreviation for Marvell SoC's (Orion, Kirkwood, 
Armada XP, 370 ...).

>   include/configs/openrd.h                      |   8 +
>   include/configs/sheevaplug.h                  |  17 +-
>   include/mvsdio.h                              | 279 ++++++++++++++++++++
>   8 files changed, 679 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/mmc/mvsdio.c
>   create mode 100644 include/mvsdio.h
>
> diff --git a/arch/arm/include/asm/arch-kirkwood/kirkwood.h b/arch/arm/include/asm/arch-kirkwood/kirkwood.h
> index bc207f5..3ea51d7 100644
> --- a/arch/arm/include/asm/arch-kirkwood/kirkwood.h
> +++ b/arch/arm/include/asm/arch-kirkwood/kirkwood.h
> @@ -39,6 +39,7 @@
>   #define KW_EGIGA0_BASE			(KW_REGISTER(0x72000))
>   #define KW_EGIGA1_BASE			(KW_REGISTER(0x76000))
>   #define KW_SATA_BASE			(KW_REGISTER(0x80000))
> +#define KW_SDIO_BASE			(KW_REGISTER(0x90000))
>
>   /* Kirkwood Sata controller has two ports */
>   #define KW_SATA_PORT0_OFFSET		0x2000
> diff --git a/board/Marvell/openrd/openrd.c b/board/Marvell/openrd/openrd.c
> index a005a2f..6e9dca6 100644
> --- a/board/Marvell/openrd/openrd.c
> +++ b/board/Marvell/openrd/openrd.c
> @@ -17,6 +17,9 @@
>   #include <asm/arch/kirkwood.h>
>   #include <asm/arch/mpp.h>
>   #include "openrd.h"
> +#ifdef CONFIG_MVSDIO_MMC
> +# include <mvsdio.h>
> +#endif /* CONFIG_MVSDIO_MMC */
>
>   DECLARE_GLOBAL_DATA_PTR;
>
> @@ -159,3 +162,11 @@ void reset_phy(void)
>   #endif
>   }
>   #endif /* CONFIG_RESET_PHY_R */
> +
> +#ifdef CONFIG_MRVL_MMC
> +int board_mmc_init(bd_t *bis)
> +{
> +	mrvl_mmc_init(bis);
> +	return 0;
> +}
> +#endif /* CONFIG_MRVL_MMC */

Doesn't it make sense to move this code to init / enable the MMC driver 
into the arch/cpu/... SoC code? Instead of adding it to all the board 
specific files?

> diff --git a/board/Marvell/sheevaplug/sheevaplug.c b/board/Marvell/sheevaplug/sheevaplug.c
> index 87e49f4..c33f55c 100644
> --- a/board/Marvell/sheevaplug/sheevaplug.c
> +++ b/board/Marvell/sheevaplug/sheevaplug.c
> @@ -12,6 +12,9 @@
>   #include <asm/arch/kirkwood.h>
>   #include <asm/arch/mpp.h>
>   #include "sheevaplug.h"
> +#ifdef CONFIG_MVSDIO_MMC
> +# include <mvsdio.h>
> +#endif /* CONFIG_MVSDIO_MMC */
>
>   DECLARE_GLOBAL_DATA_PTR;
>
> @@ -131,3 +134,11 @@ void reset_phy(void)
>   	printf("88E1116 Initialized on %s\n", name);
>   }
>   #endif /* CONFIG_RESET_PHY_R */
> +
> +#ifdef CONFIG_MRVL_MMC
> +int board_mmc_init(bd_t *bis)
> +{
> +	mrvl_mmc_init(bis);
> +	return 0;
> +}
> +#endif /* CONFIG_MRVL_MMC */

Here we have this code again. I suggest to move it to the kirkwood cpu.c 
file instead.

Thanks,
Stefan