[U-Boot] [PATCH v1 0/4] mmc: omap_hsmmc: Reduce the footprint of the driver and fix am335x clock

[U-Boot] [PATCH v1 0/4] mmc: omap_hsmmc: Reduce the footprint of the driver and fix am335x clock

[Jean-Jacques Hiblot]

This series aims at reducing the footprint of the omap_hsmmc driver in
the SPL (1.5 kB gain in the case of the SPL for the am335x evm).
It also fixes an issue with the am335x_evm by setting a default maximum
frequency if none is defined in the dts.

tested on am335x_evm, bbb, bbb (vboot), and dra7 evm


Jean-Jacques Hiblot (4):
  mmc: omap_hsmmc: do not embed struct mmc in struct omap_hsmmc_plat
  mmc: omap_hsmmc: compile out write support if not needed
  mmc: omap_hsmmc: compile out ADMA support for am33xx and omap34xx
  mmc: omap_hsmmc: use a default 52MHz max clock rate if none is
    specified

 arch/arm/include/asm/omap_mmc.h |  2 +-
 drivers/mmc/omap_hsmmc.c        | 35 ++++++++++++++++++++++++-----------
 2 files changed, 25 insertions(+), 12 deletions(-)

-- 
1.9.1

[U-Boot] [PATCH v1 1/4] mmc: omap_hsmmc: do not embed struct mmc in struct omap_hsmmc_plat

[Jean-Jacques Hiblot]

The area for struct mmc can be allocated dynamically. It greatly reduces
the size of struct omap_hsmmc_plat. This is useful in cases where the board
level code declares one or two struct omap_hsmmc_plat because it doesn't
use the Driver Model.

This saves around 740 bytes for the am335x_evm SPL.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---

 arch/arm/include/asm/omap_mmc.h | 2 +-
 drivers/mmc/omap_hsmmc.c        | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/omap_mmc.h b/arch/arm/include/asm/omap_mmc.h
index 3d70148..42ce8dc 100644
--- a/arch/arm/include/asm/omap_mmc.h
+++ b/arch/arm/include/asm/omap_mmc.h
@@ -67,7 +67,7 @@ struct hsmmc {
 struct omap_hsmmc_plat {
 	struct mmc_config cfg;
 	struct hsmmc *base_addr;
-	struct mmc mmc;
+	struct mmc *mmc;
 	bool cd_inverted;
 	u32 controller_flags;
 	const char *hw_rev;
diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
index 02970f2..e0b679a 100644
--- a/drivers/mmc/omap_hsmmc.c
+++ b/drivers/mmc/omap_hsmmc.c
@@ -1858,8 +1858,8 @@ static int omap_hsmmc_ofdata_to_platdata(struct udevice *dev)
 static int omap_hsmmc_bind(struct udevice *dev)
 {
 	struct omap_hsmmc_plat *plat = dev_get_platdata(dev);
-
-	return mmc_bind(dev, &plat->mmc, &plat->cfg);
+	plat->mmc = calloc(1, sizeof(struct mmc));
+	return mmc_bind(dev, plat->mmc, &plat->cfg);
 }
 #endif
 static int omap_hsmmc_probe(struct udevice *dev)
@@ -1882,7 +1882,7 @@ static int omap_hsmmc_probe(struct udevice *dev)
 #endif
 
 #ifdef CONFIG_BLK
-	mmc = &plat->mmc;
+	mmc = plat->mmc;
 #else
 	mmc = mmc_create(cfg, priv);
 	if (mmc == NULL)
-- 
1.9.1

[U-Boot] [PATCH v1 2/4] mmc: omap_hsmmc: compile out write support if not needed

[Jean-Jacques Hiblot]

This reduces the size of the binary by about 196 bytes.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---

 drivers/mmc/omap_hsmmc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
index e0b679a..8b57edc 100644
--- a/drivers/mmc/omap_hsmmc.c
+++ b/drivers/mmc/omap_hsmmc.c
@@ -1181,8 +1181,9 @@ static int mmc_read_data(struct hsmmc *mmc_base, char *buf, unsigned int size)
 	return 0;
 }
 
+#if CONFIG_IS_ENABLED(MMC_WRITE)
 static int mmc_write_data(struct hsmmc *mmc_base, const char *buf,
-				unsigned int size)
+			  unsigned int size)
 {
 	unsigned int *input_buf = (unsigned int *)buf;
 	unsigned int mmc_stat;
@@ -1235,7 +1236,13 @@ static int mmc_write_data(struct hsmmc *mmc_base, const char *buf,
 	}
 	return 0;
 }
-
+#else
+static int mmc_write_data(struct hsmmc *mmc_base, const char *buf,
+			  unsigned int size)
+{
+	return -ENOTSUPP;
+}
+#endif
 static void omap_hsmmc_stop_clock(struct hsmmc *mmc_base)
 {
 	writel(readl(&mmc_base->sysctl) & ~CEN_ENABLE, &mmc_base->sysctl);
-- 
1.9.1

[U-Boot] [PATCH v1 3/4] mmc: omap_hsmmc: compile out ADMA support for am33xx and omap34xx

[Jean-Jacques Hiblot]

This reduces the size of the binary by about 600 bytes.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---

 drivers/mmc/omap_hsmmc.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
index 8b57edc..31c1f66 100644
--- a/drivers/mmc/omap_hsmmc.c
+++ b/drivers/mmc/omap_hsmmc.c
@@ -48,6 +48,10 @@
 #include <dm.h>
 #include <power/regulator.h>
 
+#if !defined(CONFIG_AM33XX) && !defined(CONFIG_OMAP34XX)
+#define ADMA_SUPPORT
+#endif
+
 DECLARE_GLOBAL_DATA_PTR;
 
 /* simplify defines to OMAP_HSMMC_USE_GPIO */
@@ -93,7 +97,7 @@ struct omap_hsmmc_data {
 	enum bus_mode mode;
 #endif
 	u8 controller_flags;
-#ifndef CONFIG_OMAP34XX
+#ifdef ADMA_SUPPORT
 	struct omap_hsmmc_adma_desc *adma_desc_table;
 	uint desc_slot;
 #endif
@@ -117,7 +121,7 @@ struct omap_mmc_of_data {
 	u8 controller_flags;
 };
 
-#ifndef CONFIG_OMAP34XX
+#ifdef ADMA_SUPPORT
 struct omap_hsmmc_adma_desc {
 	u8 attr;
 	u8 reserved;
@@ -741,7 +745,7 @@ static int omap_hsmmc_init_setup(struct mmc *mmc)
 			return -ETIMEDOUT;
 		}
 	}
-#ifndef CONFIG_OMAP34XX
+#ifdef ADMA_SUPPORT
 	reg_val = readl(&mmc_base->hl_hwinfo);
 	if (reg_val & MADMA_EN)
 		priv->controller_flags |= OMAP_HSMMC_USE_ADMA;
@@ -834,7 +838,7 @@ static void mmc_reset_controller_fsm(struct hsmmc *mmc_base, u32 bit)
 	}
 }
 
-#ifndef CONFIG_OMAP34XX
+#ifdef ADMA_SUPPORT
 static void omap_hsmmc_adma_desc(struct mmc *mmc, char *buf, u16 len, bool end)
 {
 	struct omap_hsmmc_data *priv = omap_hsmmc_get_data(mmc);
@@ -1037,7 +1041,7 @@ static int omap_hsmmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
 		else
 			flags |= (DP_DATA | DDIR_WRITE);
 
-#ifndef CONFIG_OMAP34XX
+#ifdef ADMA_SUPPORT
 		if ((priv->controller_flags & OMAP_HSMMC_USE_ADMA) &&
 		    !mmc_is_tuning_cmd(cmd->cmdidx)) {
 			omap_hsmmc_prepare_data(mmc, data);
@@ -1082,7 +1086,7 @@ static int omap_hsmmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
 		}
 	}
 
-#ifndef CONFIG_OMAP34XX
+#ifdef ADMA_SUPPORT
 	if ((priv->controller_flags & OMAP_HSMMC_USE_ADMA) && data &&
 	    !mmc_is_tuning_cmd(cmd->cmdidx)) {
 		u32 sz_mb, timeout;
-- 
1.9.1

[U-Boot] [PATCH v1 4/4] mmc: omap_hsmmc: use a default 52MHz max clock rate if none is specified

[Jean-Jacques Hiblot]

mmc_of_parse() doesn't set a default value if none is available in DT.
In that case, use a default 52MHz clock rate.

Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com>
Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>

---

 drivers/mmc/omap_hsmmc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
index 31c1f66..f61fae9 100644
--- a/drivers/mmc/omap_hsmmc.c
+++ b/drivers/mmc/omap_hsmmc.c
@@ -1836,6 +1836,8 @@ static int omap_hsmmc_ofdata_to_platdata(struct udevice *dev)
 	if (ret < 0)
 		return ret;
 
+	if (!cfg->f_max)
+		cfg->f_max = 52000000;
 	cfg->host_caps |= MMC_MODE_HS_52MHz | MMC_MODE_HS;
 	cfg->f_min = 400000;
 	cfg->voltages = MMC_VDD_32_33 | MMC_VDD_33_34 | MMC_VDD_165_195;
-- 
1.9.1

[U-Boot] [PATCH v1 3/4] mmc: omap_hsmmc: compile out ADMA support for am33xx and omap34xx

[Tom Rini]

On Thu, Feb 22, 2018 at 11:25:47AM +0100, Jean-Jacques Hiblot wrote:
> This reduces the size of the binary by about 600 bytes.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
> 
>  drivers/mmc/omap_hsmmc.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
> index 8b57edc..31c1f66 100644
> --- a/drivers/mmc/omap_hsmmc.c
> +++ b/drivers/mmc/omap_hsmmc.c
> @@ -48,6 +48,10 @@
>  #include <dm.h>
>  #include <power/regulator.h>
>  
> +#if !defined(CONFIG_AM33XX) && !defined(CONFIG_OMAP34XX)
> +#define ADMA_SUPPORT
> +#endif
> +
>  DECLARE_GLOBAL_DATA_PTR;
>  
>  /* simplify defines to OMAP_HSMMC_USE_GPIO */
> @@ -93,7 +97,7 @@ struct omap_hsmmc_data {
>  	enum bus_mode mode;
>  #endif
>  	u8 controller_flags;
> -#ifndef CONFIG_OMAP34XX
> +#ifdef ADMA_SUPPORT

How about we add CONFIG_MMC_OMAP_HS_ADMA, and select it on !AM33XX &&
!OMAP34XX, and then test that here?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180222/e9e731b1/attachment.sig>

[U-Boot] [PATCH v1 2/4] mmc: omap_hsmmc: compile out write support if not needed

[Tom Rini]

On Thu, Feb 22, 2018 at 11:25:46AM +0100, Jean-Jacques Hiblot wrote:

> This reduces the size of the binary by about 196 bytes.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180222/e670edc2/attachment.sig>

[U-Boot] [PATCH v1 1/4] mmc: omap_hsmmc: do not embed struct mmc in struct omap_hsmmc_plat

[Tom Rini]

On Thu, Feb 22, 2018 at 11:25:45AM +0100, Jean-Jacques Hiblot wrote:

> The area for struct mmc can be allocated dynamically. It greatly reduces
> the size of struct omap_hsmmc_plat. This is useful in cases where the board
> level code declares one or two struct omap_hsmmc_plat because it doesn't
> use the Driver Model.
> 
> This saves around 740 bytes for the am335x_evm SPL.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180222/3b155aa6/attachment.sig>

[U-Boot] [PATCH v1 4/4] mmc: omap_hsmmc: use a default 52MHz max clock rate if none is specified

[Tom Rini]

On Thu, Feb 22, 2018 at 11:25:48AM +0100, Jean-Jacques Hiblot wrote:

> mmc_of_parse() doesn't set a default value if none is available in DT.
> In that case, use a default 52MHz clock rate.
> 
> Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> 

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180222/a18da4a4/attachment.sig>

[U-Boot] [PATCH v1 0/4] mmc: omap_hsmmc: Reduce the footprint of the driver and fix am335x clock

[Adam Ford]

On Thu, Feb 22, 2018 at 4:25 AM, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>
>
> This series aims at reducing the footprint of the omap_hsmmc driver in
> the SPL (1.5 kB gain in the case of the SPL for the am335x evm).
> It also fixes an issue with the am335x_evm by setting a default maximum
> frequency if none is defined in the dts.
>
> tested on am335x_evm, bbb, bbb (vboot), and dra7 evm
>

This series fixes 2d7482cf793f ("mmc: omap_hsmmc: use mmc_of_parse to
populate mmc_config") which made my DM3730 unable to probe the MMC.

Tested-by: Adam Ford <aford173@gmail.com> #omap3_logic

>
>
> Jean-Jacques Hiblot (4):
>   mmc: omap_hsmmc: do not embed struct mmc in struct omap_hsmmc_plat
>   mmc: omap_hsmmc: compile out write support if not needed
>   mmc: omap_hsmmc: compile out ADMA support for am33xx and omap34xx
>   mmc: omap_hsmmc: use a default 52MHz max clock rate if none is
>     specified
>
>  arch/arm/include/asm/omap_mmc.h |  2 +-
>  drivers/mmc/omap_hsmmc.c        | 35 ++++++++++++++++++++++++-----------
>  2 files changed, 25 insertions(+), 12 deletions(-)
>
> --
> 1.9.1
>

[U-Boot] [PATCH v1 3/4] mmc: omap_hsmmc: compile out ADMA support for am33xx and omap34xx

[Jaehoon Chung]

On 02/23/2018 12:47 AM, Tom Rini wrote:
> On Thu, Feb 22, 2018 at 11:25:47AM +0100, Jean-Jacques Hiblot wrote:
>> This reduces the size of the binary by about 600 bytes.
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> ---
>>
>>  drivers/mmc/omap_hsmmc.c | 16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
>> index 8b57edc..31c1f66 100644
>> --- a/drivers/mmc/omap_hsmmc.c
>> +++ b/drivers/mmc/omap_hsmmc.c
>> @@ -48,6 +48,10 @@
>>  #include <dm.h>
>>  #include <power/regulator.h>
>>  
>> +#if !defined(CONFIG_AM33XX) && !defined(CONFIG_OMAP34XX)
>> +#define ADMA_SUPPORT
>> +#endif
>> +
>>  DECLARE_GLOBAL_DATA_PTR;
>>  
>>  /* simplify defines to OMAP_HSMMC_USE_GPIO */
>> @@ -93,7 +97,7 @@ struct omap_hsmmc_data {
>>  	enum bus_mode mode;
>>  #endif
>>  	u8 controller_flags;
>> -#ifndef CONFIG_OMAP34XX
>> +#ifdef ADMA_SUPPORT
> 
> How about we add CONFIG_MMC_OMAP_HS_ADMA, and select it on !AM33XX &&
> !OMAP34XX, and then test that here?  Thanks!

I agreed Tom's suggestion. 
If you can send v2 with it, I will pick these series with Tom's Reviewed's tag and Adam's Tested tag.

Best Regards,
Jaehoon Chung

> 

[U-Boot] [PATCH v1 1/4] mmc: omap_hsmmc: do not embed struct mmc in struct omap_hsmmc_plat

[Simon Glass]

Hi Jean-Jacques,

On 22 February 2018 at 03:25, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
> The area for struct mmc can be allocated dynamically. It greatly reduces
> the size of struct omap_hsmmc_plat. This is useful in cases where the board
> level code declares one or two struct omap_hsmmc_plat because it doesn't
> use the Driver Model.
>
> This saves around 740 bytes for the am335x_evm SPL.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
>
>  arch/arm/include/asm/omap_mmc.h | 2 +-
>  drivers/mmc/omap_hsmmc.c        | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

I would like to understand why this saves memory though. Presumably
the pointer has to point to a real struct anyway, which uses memory.
So how does this help?

- Simon

[U-Boot] [PATCH v1 1/4] mmc: omap_hsmmc: do not embed struct mmc in struct omap_hsmmc_plat

[Jean-Jacques Hiblot]

On 23/02/2018 21:59, Simon Glass wrote:
> Hi Jean-Jacques,
>
> On 22 February 2018 at 03:25, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>> The area for struct mmc can be allocated dynamically. It greatly reduces
>> the size of struct omap_hsmmc_plat. This is useful in cases where the board
>> level code declares one or two struct omap_hsmmc_plat because it doesn't
>> use the Driver Model.
>>
>> This saves around 740 bytes for the am335x_evm SPL.
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> ---
>>
>>   arch/arm/include/asm/omap_mmc.h | 2 +-
>>   drivers/mmc/omap_hsmmc.c        | 6 +++---
>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> I would like to understand why this saves memory though. Presumably
> the pointer has to point to a real struct anyway, which uses memory.
> So how does this help?
struct omap_hsmmc_plat are initialized variables so they are part of the 
binary. With this patch the memory is dynamically allocated so that it's 
not taking space in the binary.

JJ
>
> - Simon
>