[U-Boot] [PATCH 2/3] mmc: split fsl_esdhc driver for i.MX
Y.b. Lu
yangbo.lu at nxp.com
Thu Mar 14 08:02:49 UTC 2019
Hi Stefano,
> -----Original Message-----
> From: Stefano Babic <sbabic at denx.de>
> Sent: Wednesday, March 13, 2019 7:53 PM
> To: Y.b. Lu <yangbo.lu at nxp.com>; u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH 2/3] mmc: split fsl_esdhc driver for i.MX
>
> Hi Y.B lu,
>
> On 21/02/19 08:55, Y.b. Lu wrote:
> > The fsl_esdhc driver was for Freescale eSDHC on MPC83XX/MPC85XX
> > initially. The later QoriQ series PowerPC processors (which were
> > evolutions of MPC83XX/MPC85XX), QorIQ series ARM processors, and i.MX
> > series processors were using this driver for their eSDHCs too.
> >
> > For the two series processors, the eSDHCs are becoming more and more
> > different. We should have split it into two drivers, like them
> > (sdhci-of-esdhc.c/sdhci-esdhc-imx.c) in linux kernel.
> >
> > This patch is just to create a fsl_esdhc_imx driver which is a copy of
> > fsl_esdhc driver for i.MX processors. We will convert i.MX processors
> > to use fsl_esdhc_imx, and clean up the two drivers separately in the
> > future patches.
> >
> > Signed-off-by: Yangbo Lu <yangbo.lu at nxp.com>
> > ---
> > drivers/mmc/Kconfig | 6 ++++++
> > drivers/mmc/Makefile | 1 +
> > drivers/mmc/{fsl_esdhc.c => fsl_esdhc_imx.c} | 5 +++--
> > include/{fsl_esdhc.h => fsl_esdhc_imx.h} | 11 ++++++-----
> > 4 files changed, 16 insertions(+), 7 deletions(-) copy
> > drivers/mmc/{fsl_esdhc.c => fsl_esdhc_imx.c} (99%) copy
> > include/{fsl_esdhc.h => fsl_esdhc_imx.h} (97%)
> >
>
> IMHO we can do better - if we split the code, we should be able to factorize
> common code (if any) into a separate file and move the specific parts into
> processor specific files. And at the same time, clean up what is not required
> (for example, CONFIG_MCF5441x should not appear in imx code, and so on).
[Y.b. Lu] It's ideal to reuse common code. However I could find little which could be re-used.
As you see in the driver, there is conditional compiling in almost each function. (Some macros are specific for esdhc, and some are for esdhc_imx)
And the function is usually for a very basic function which is not worth to be split for common part.
Even some functions which are not in conditional compiling are specific for esdhc or esdhc_imx.
Besides, there will be two different registers structures for esdhc/esdhc_imx, since they are having more and more different registers and bits.
So I suggest just to split them, and there will be two totally different driver after cleaning up.
Regarding to MCF5441x, that's really a surprise to me. eSDHC actually is IP of only Freescale/NXP processors.
I don’t know any detail about it. But we can keep it in esdhc driver after splitting.
Do you think it's ok?
>
> > diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig index
> > 04a4e7716f..09bc02fe9c 100644
> > --- a/drivers/mmc/Kconfig
> > +++ b/drivers/mmc/Kconfig
> > @@ -641,6 +641,12 @@ config FSL_ESDHC
> > This selects support for the eSDHC (enhanced secure digital host
> > controller) found on numerous Freescale/NXP SoCs.
> >
> > +config FSL_ESDHC_IMX
> > + bool "Freescale/NXP i.MX eSDHC controller support"
>
> We need a depend clause (ARCH_MX6, ARCH_MX5, ..)
[Y.b. Lu] Yes... Will add them.
>
> > + help
> > + This selects support for the i.MX eSDHC (enhanced secure digital host
> > + controller) found on numerous Freescale/NXP SoCs.
> > +
> > endmenu
> >
> > config SYS_FSL_ERRATUM_ESDHC111
> > diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile index
> > 7892c468f0..1287ad4cc1 100644
> > --- a/drivers/mmc/Makefile
> > +++ b/drivers/mmc/Makefile
> > @@ -25,6 +25,7 @@ obj-$(CONFIG_MMC_DW_K3) +=
> hi6220_dw_mmc.o
> > obj-$(CONFIG_MMC_DW_ROCKCHIP) += rockchip_dw_mmc.o
> > obj-$(CONFIG_MMC_DW_SOCFPGA) += socfpga_dw_mmc.o
> > obj-$(CONFIG_FSL_ESDHC) += fsl_esdhc.o
> > +obj-$(CONFIG_FSL_ESDHC_IMX) += fsl_esdhc_imx.o
> > obj-$(CONFIG_FTSDC010) += ftsdc010_mci.o
> > obj-$(CONFIG_GENERIC_ATMEL_MCI) += gen_atmel_mci.o
> > obj-$(CONFIG_MMC_MESON_GX) += meson_gx_mmc.o diff --git
> > a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc_imx.c similarity
> > index 99% copy from drivers/mmc/fsl_esdhc.c copy to
> > drivers/mmc/fsl_esdhc_imx.c index 21fa2ab1d4..9c823e86e2 100644
> > --- a/drivers/mmc/fsl_esdhc.c
> > +++ b/drivers/mmc/fsl_esdhc_imx.c
> > @@ -2,6 +2,7 @@
> > /*
> > * Copyright 2007, 2010-2011 Freescale Semiconductor, Inc
> > * Andy Fleming
> > + * Copyright 2019 NXP
> > *
> > * Based vaguely on the pxa mmc code:
> > * (C) Copyright 2003
> > @@ -18,7 +19,7 @@
> > #include <part.h>
> > #include <power/regulator.h>
> > #include <malloc.h>
> > -#include <fsl_esdhc.h>
> > +#include <fsl_esdhc_imx.h>
> > #include <fdt_support.h>
> > #include <asm/io.h>
> > #include <dm.h>
> > @@ -110,7 +111,7 @@ struct esdhc_soc_data {
> > * @non_removable: 0: removable; 1: non-removable
> > * @wp_enable: 1: enable checking wp; 0: no check
> > * @vs18_enable: 1: use 1.8V voltage; 0: use 3.3V
> > - * @flags: ESDHC_FLAG_xx in include/fsl_esdhc.h
> > + * @flags: ESDHC_FLAG_xx in include/fsl_esdhc_imx.h
> > * @caps: controller capabilities
> > * @tuning_step: tuning step setting in tuning_ctrl register
> > * @start_tuning_tap: the start point for tuning in tuning_ctrl
> > register diff --git a/include/fsl_esdhc.h b/include/fsl_esdhc_imx.h
> > similarity index 97% copy from include/fsl_esdhc.h copy to
> > include/fsl_esdhc_imx.h index 8dbd5249a7..e05b24e7e8 100644
> > --- a/include/fsl_esdhc.h
> > +++ b/include/fsl_esdhc_imx.h
> > @@ -4,10 +4,11 @@
> > *-------------------------------------------------------------------
> > *
> > * Copyright 2007-2008,2010-2011 Freescale Semiconductor, Inc
> > + * Copyright 2019 NXP
> > */
> >
> > -#ifndef __FSL_ESDHC_H__
> > -#define __FSL_ESDHC_H__
> > +#ifndef __FSL_ESDHC_IMX_H__
> > +#define __FSL_ESDHC_IMX_H__
> >
> > #include <linux/bitops.h>
> > #include <linux/errno.h>
> > @@ -258,15 +259,15 @@ struct fsl_esdhc_cfg { #error "Endianess is not
> > defined: please fix to continue"
> > #endif
> >
> > -#ifdef CONFIG_FSL_ESDHC
> > +#ifdef CONFIG_FSL_ESDHC_IMX
> > int fsl_esdhc_mmc_init(bd_t *bis);
> > int fsl_esdhc_initialize(bd_t *bis, struct fsl_esdhc_cfg *cfg); void
> > fdt_fixup_esdhc(void *blob, bd_t *bd); #else static inline int
> > fsl_esdhc_mmc_init(bd_t *bis) { return -ENOSYS; } static inline void
> > fdt_fixup_esdhc(void *blob, bd_t *bd) {} -#endif /* CONFIG_FSL_ESDHC
> > */
> > +#endif /* CONFIG_FSL_ESDHC_IMX */
> > void __noreturn mmc_boot(void);
> > void mmc_spl_load_image(uint32_t offs, unsigned int size, void
> > *vdst);
> >
> > -#endif /* __FSL_ESDHC_H__ */
> > +#endif /* __FSL_ESDHC_IMX_H__ */
> >
>
> I tend to go further and to cleanup the new file removing what is not required.
> Do it in a separate patch, but belonging to the same series, so that is easier to
> review - thanks !
[Y.b. Lu] I assume you meant both driver and header files.
That will take some time for me.
>
> Best regards,
> Stefano Babic
>
> --
> ===================================================================
> ==
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
> ===================================================================
> ==
More information about the U-Boot
mailing list