From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?utf-8?Q?Beno=C3=AEt_Th=C3=A9baudeau?= <benoit.thebaudeau@advansee.com>
Date: Sat, 11 Aug 2012 19:59:02 +0200 (CEST)
Subject: [U-Boot] [PATCH] fsl_esdhc: Add no-snoop config for default init
In-Reply-To: <50269586.1060802@denx.de>
Message-ID: <746717980.2303098.1344707942239.JavaMail.root@advansee.com>
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

Hi Stefano,

On 08/11/2012 19:25:26, Stefano Babic wrote:
> On 11/08/2012 16:39, Beno?t Th?baudeau wrote:
> > Hi Stefano,
> > 
> 
> Hi,
> 
> > fsl_esdhc_mmc_init() exists, so it should give access to all
> > features.
> 
> This was left for compatibility with PowerPC SOCs.

OK.

> > Moreover,
> > it's shorter to #define a config option than to create custom
> > fsl_esdhc_mmc init
> > functions when only a single eSDHC instance is needed.
> 
> However, the number of CONFIG_ continues to increase, often not well
> documented.

OK.

> > Besides that, the cache snooping feature is not available on i.MX,
> > which means
> > that without CONFIG_SYS_FSL_ESDHC_NO_SNOOP or an equivalent, boards
> > using
> > fsl_esdhc_mmc_init() will make esdhc_init() access a reserved bit
> > of a reserved
> > register, which is not really recommended.
> 
> It should be masked by the i.MX code. You add
> CONFIG_SYS_FSL_ESDHC_NO_SNOOP and this is set inside the board
> configuration file. But this is not a board configuration option,
> rather
> a SOC option. Then it should the initialization code of i.MX that
> should
> hide this bit to the board itself.
> 
> Instead of this, I think it is better to change cpu_mmc_init() in
> arch/arm/cpu/armv7/imx-common/cpu.c, setting the structure and fixing
> the soop bit to 1, because i.MX has not this feature. Then all i.MX
> boards using ESDHC have the no_snoop bit set.

That could be a solution. However, that would have to be done for i.MX25 and
i.MX35 too (I have patches to add/fix eSDHC support for these that I will post
shortly), which means more duplicated code that should rather be centralized
somewhere.

> > On i.MX, the patch could also be improved by not making this option
> > available,
> > but behaving as if it were set.
> 
> Right. It should not be configurable. We know that the i.MXs have no
> snoop cabability.

The solution I would like would be to have some automatic ARCH_IMX (or ARCH_MXC)
config like on Linux. Then the fsl_esdhc driver could use this config to remove
all cache snooping stuff from its code and from struct fsl_esdhc_cfg.

The i.MX board files would have to be updated to no longer refer to the no_snoop
field. struct fsl_esdhc_cfg would then have only one field for i.MX, but this is
not really an issue.

ARCH_IMX could be defined in all imx-regs.h files. Or the fsl_esdhc driver could
simply use a #if testing all the supported CONFIG_MX*.

What do you think?

Best regards,
Beno?t