[U-Boot] [PATCH] mmc: Tinification of the mmc code

[Marek Vasut <marex@denx.de>]

On 06/10/2016 03:16 AM, Simon Glass wrote:
> Hi Marek,

Hi,

> On 9 June 2016 at 18:12, Marek Vasut <marex@denx.de> wrote:
>> On 06/10/2016 02:34 AM, Simon Glass wrote:
>>> Hi Marek,
>>
>> Hi!
>>
>>> On 26 May 2016 at 12:41, Marek Vasut <marex@denx.de> wrote:
>>>> Add new configuration option CONFIG_MMC_TINY which strips away all
>>>> memory allocation within the MMC code and code for handling multiple
>>>> cards. This allows extremely space-constrained SPL code use the MMC
>>>> framework.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Cc: Tom Rini <trini@konsulko.com>
>>>> Cc: Simon Glass <sjg@chromium.org>
>>>> ---
>>>>  common/spl/spl_mmc.c |  4 ++++
>>>>  drivers/mmc/Makefile |  2 ++
>>>>  drivers/mmc/mmc.c    | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>  include/mmc.h        |  1 +
>>>>  4 files changed, 65 insertions(+), 1 deletion(-)
>>>
>>> Can CONFIG_MMC_TINY be a Kconfig? Also I suggest CONFIG_SPL_MMC_TINY.
>>
>> It can, but how do I assure it's enabled only for SPL build ?
> 
> depends on SPL
> 
> and in the code:
> 
> #if CONFIG_IS_ENABLED(MMC_TINY)
> 
> will do it.

Ah right, thanks.

>>>> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
>>>> index ae42221..51f0f24 100644
>>>> --- a/common/spl/spl_mmc.c
>>>> +++ b/common/spl/spl_mmc.c
>>>> @@ -300,7 +300,11 @@ int spl_mmc_load_image(u32 boot_device)
>>>>                         if (part == 7)
>>>>                                 part = 0;
>>>>
>>>> +#ifdef CONFIG_MMC_TINY
>>>
>>> if (CONFIG_IS_ENABLED(MMC_TINY))
>>>
>>> to avoid #ifdef
>>
>> The compiler complains about missing symbols blk_dselect_hwpart() and
>> such, so I will opt for the ifdef .
> 
> That's odd. It should not care about things which are not compiled in.

Must've been some oddity indeed.

>>>> +                       err = mmc_switch_part(mmc, part);
>>>> +#else
>>>>                         err = blk_dselect_hwpart(mmc_get_blk_desc(mmc), part);
>>>> +#endif
>>>>                         if (err) {
>>>>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>>>>                                 puts("spl: mmc partition switch failed\n");
>>>> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
>>>> index 3da4817..4d986cb 100644
>>>> --- a/drivers/mmc/Makefile
>>>> +++ b/drivers/mmc/Makefile
>>>> @@ -10,8 +10,10 @@ obj-$(CONFIG_GENERIC_MMC) += mmc-uclass.o
>>>>  endif
>>>>
>>>>  ifndef CONFIG_BLK
>>>> +ifndef CONFIG_MMC_TINY
>>>>  obj-$(CONFIG_GENERIC_MMC) += mmc_legacy.o
>>>>  endif
>>>> +endif
>>>>
>>>>  obj-$(CONFIG_ARM_PL180_MMCI) += arm_pl180_mmci.o
>>>>  obj-$(CONFIG_ATMEL_SDHCI) += atmel_sdhci.o
>>>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>>>> index d687345..1f240ed 100644
>>>> --- a/drivers/mmc/mmc.c
>>>> +++ b/drivers/mmc/mmc.c
>>>> @@ -21,6 +21,29 @@
>>>>  #include <div64.h>
>>>>  #include "mmc_private.h"
>>>>
>>>> +#if defined(CONFIG_MMC_TINY)
>>>> +static struct mmc mmc_static;
>>>> +struct mmc *find_mmc_device(int dev_num)
>>>> +{
>>>> +       return &mmc_static;
>>>> +}
>>>> +
>>>> +void mmc_do_preinit(void)
>>>> +{
>>>> +       struct mmc *m = &mmc_static;
>>>> +#ifdef CONFIG_FSL_ESDHC_ADAPTER_IDENT
>>>> +       mmc_set_preinit(m, 1);
>>>> +#endif
>>>> +       if (m->preinit)
>>>> +               mmc_start_init(m);
>>>> +}
>>>> +
>>>> +struct blk_desc *mmc_get_blk_desc(struct mmc *mmc)
>>>> +{
>>>> +       return &mmc->block_dev;
>>>> +}
>>>> +#endif
>>>> +
>>>>  __weak int board_mmc_getwp(struct mmc *mmc)
>>>>  {
>>>>         return -1;
>>>> @@ -238,7 +261,11 @@ static ulong mmc_bread(struct blk_desc *block_dev, lbaint_t start,
>>>>         if (!mmc)
>>>>                 return 0;
>>>>
>>>> +#ifdef CONFIG_MMC_TINY
>>>> +       err = mmc_switch_part(mmc, block_dev->hwpart);
>>>> +#else
>>>>         err = blk_dselect_hwpart(block_dev, block_dev->hwpart);
>>>> +#endif
>>>>         if (err < 0)
>>>>                 return 0;
>>>>
>>>> @@ -568,7 +595,7 @@ static int mmc_set_capacity(struct mmc *mmc, int part_num)
>>>>         return 0;
>>>>  }
>>>>
>>>> -static int mmc_switch_part(struct mmc *mmc, unsigned int part_num)
>>>> +int mmc_switch_part(struct mmc *mmc, unsigned int part_num)
>>>>  {
>>>>         int ret;
>>>>
>>>> @@ -1585,6 +1612,34 @@ int mmc_unbind(struct udevice *dev)
>>>>         return 0;
>>>>  }
>>>>
>>>> +#elif defined(CONFIG_MMC_TINY)
>>>> +static struct mmc mmc_static = {
>>>> +       .dsr_imp                = 0,
>>>> +       .dsr                    = 0xffffffff,
>>>> +       .block_dev = {
>>>> +               .if_type        = IF_TYPE_MMC,
>>>> +               .removable      = 1,
>>>> +               .devnum         = 0,
>>>> +               .block_read     = mmc_bread,
>>>> +               .block_write    = mmc_bwrite,
>>>> +               .block_erase    = mmc_berase,
>>>> +               .part_type      = 0,
>>>> +       },
>>>> +};
>>>> +
>>>> +struct mmc *mmc_create(const struct mmc_config *cfg, void *priv)
>>>> +{
>>>> +       struct mmc *mmc = &mmc_static;
>>>> +
>>>> +       mmc->cfg = cfg;
>>>> +       mmc->priv = priv;
>>>> +
>>>> +       return mmc;
>>>> +}
>>>> +
>>>> +void mmc_destroy(struct mmc *mmc)
>>>> +{
>>>> +}
>>>>  #else
>>>>  struct mmc *mmc_create(const struct mmc_config *cfg, void *priv)
>>>>  {
>>>> @@ -1834,8 +1889,10 @@ int mmc_initialize(bd_t *bis)
>>>>         initialized = 1;
>>>>
>>>>  #ifndef CONFIG_BLK
>>>> +#ifndef CONFIG_MMC_TINY
>>>>         mmc_list_init();
>>>>  #endif
>>>> +#endif
>>>>         ret = mmc_probe(bis);
>>>>         if (ret)
>>>>                 return ret;
>>>> diff --git a/include/mmc.h b/include/mmc.h
>>>> index a5c6573..08a59c2 100644
>>>> --- a/include/mmc.h
>>>> +++ b/include/mmc.h
>>>> @@ -444,6 +444,7 @@ struct mmc *find_mmc_device(int dev_num);
>>>>  int mmc_set_dev(int dev_num);
>>>>  void print_mmc_devices(char separator);
>>>>  int get_mmc_num(void);
>>>> +int mmc_switch_part(struct mmc *mmc, unsigned int part_num);
>>>>  int mmc_hwpart_config(struct mmc *mmc, const struct mmc_hwpart_conf *conf,
>>>>                       enum mmc_hwpart_conf_mode mode);
>>>>  int mmc_getcd(struct mmc *mmc);
>>>> --
>>>> 2.7.0
>>>>
>>>
>>> This is partially undoing the legacy block device work. How much does
>>> this patch save?
>>
>> It does save enough to make my SPL fit on my device, which is a few kiB.
>> I didn't measure it precisely because the block stuff starts requiring
>> malloc support (which my SPL does not have) and pulls in more and more
>> code which blows the SPL size.
> 
> The legacy block support should not require malloc().

But it does require convoluted list handling, which takes a lot of space.

> Can you give me instructions on how to try all this (perhaps point me
> to a tree?). I'd like to dig into it a little.

Sent off-list.

-- 
Best regards,
Marek Vasut