[PATCH 1/3] mmc: Convert hs400_tuning flag from u8 to bool
This hs400_tuning is a flag, make it bool. No functional change. This will be useful in the following patch, which adds another more generic flag, where the compiler can better use the space now reserved for the u8 to store more flags in it.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org --- Cc: Hai Pham hai.pham.ud@renesas.com Cc: Jaehoon Chung jh80.chung@samsung.com Cc: Nobuhiro Iwamatsu iwamatsu@nigauri.org Cc: Paul Barker paul.barker.ct@bp.renesas.com Cc: Peng Fan peng.fan@nxp.com Cc: Sean Anderson seanga2@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Yoshihiro Shimoda yoshihiro.shimoda.uh@renesas.com --- drivers/mmc/mmc.c | 4 ++-- include/mmc.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 83f9ae8bb7d..07d87c993a6 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -2027,9 +2027,9 @@ static int mmc_select_hs400(struct mmc *mmc) mmc_set_clock(mmc, mmc->tran_speed, false);
/* execute tuning if needed */ - mmc->hs400_tuning = 1; + mmc->hs400_tuning = true; err = mmc_execute_tuning(mmc, MMC_CMD_SEND_TUNING_BLOCK_HS200); - mmc->hs400_tuning = 0; + mmc->hs400_tuning = false; if (err) { debug("tuning failed\n"); return err; diff --git a/include/mmc.h b/include/mmc.h index 2b9a6aa8ee0..92cffc6a19a 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -736,7 +736,7 @@ struct mmc { * accessing the boot partitions */ u32 quirks; - u8 hs400_tuning; + bool hs400_tuning;
enum bus_mode user_speed_mode; /* input speed mode from user */ };
Set generic mmc->tuning flag when performing tuning to indicate this condition to drivers. Drivers may use this to bypass various checks during tuning.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org --- Cc: Hai Pham hai.pham.ud@renesas.com Cc: Jaehoon Chung jh80.chung@samsung.com Cc: Nobuhiro Iwamatsu iwamatsu@nigauri.org Cc: Paul Barker paul.barker.ct@bp.renesas.com Cc: Peng Fan peng.fan@nxp.com Cc: Sean Anderson seanga2@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Yoshihiro Shimoda yoshihiro.shimoda.uh@renesas.com --- drivers/mmc/mmc-uclass.c | 8 +++++++- include/mmc.h | 1 + 2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c index 328456831dd..304bd5eaee2 100644 --- a/drivers/mmc/mmc-uclass.c +++ b/drivers/mmc/mmc-uclass.c @@ -124,7 +124,13 @@ static int dm_mmc_execute_tuning(struct udevice *dev, uint opcode)
int mmc_execute_tuning(struct mmc *mmc, uint opcode) { - return dm_mmc_execute_tuning(mmc->dev, opcode); + int ret; + + mmc->tuning = true; + ret = dm_mmc_execute_tuning(mmc->dev, opcode); + mmc->tuning = false; + + return ret; } #endif
diff --git a/include/mmc.h b/include/mmc.h index 92cffc6a19a..47ccf5f45a1 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -736,6 +736,7 @@ struct mmc { * accessing the boot partitions */ u32 quirks; + bool tuning; bool hs400_tuning;
enum bus_mode user_speed_mode; /* input speed mode from user */
On 20/02/2024 08:37, Marek Vasut wrote:
Set generic mmc->tuning flag when performing tuning to indicate this condition to drivers. Drivers may use this to bypass various checks during tuning.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org
Cc: Hai Pham hai.pham.ud@renesas.com Cc: Jaehoon Chung jh80.chung@samsung.com Cc: Nobuhiro Iwamatsu iwamatsu@nigauri.org Cc: Paul Barker paul.barker.ct@bp.renesas.com Cc: Peng Fan peng.fan@nxp.com Cc: Sean Anderson seanga2@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Yoshihiro Shimoda yoshihiro.shimoda.uh@renesas.com
drivers/mmc/mmc-uclass.c | 8 +++++++- include/mmc.h | 1 + 2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c index 328456831dd..304bd5eaee2 100644 --- a/drivers/mmc/mmc-uclass.c +++ b/drivers/mmc/mmc-uclass.c @@ -124,7 +124,13 @@ static int dm_mmc_execute_tuning(struct udevice *dev, uint opcode)
int mmc_execute_tuning(struct mmc *mmc, uint opcode) {
- return dm_mmc_execute_tuning(mmc->dev, opcode);
- int ret;
- mmc->tuning = true;
- ret = dm_mmc_execute_tuning(mmc->dev, opcode);
- mmc->tuning = false;
- return ret;
} #endif
diff --git a/include/mmc.h b/include/mmc.h index 92cffc6a19a..47ccf5f45a1 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -736,6 +736,7 @@ struct mmc { * accessing the boot partitions */ u32 quirks;
bool tuning; bool hs400_tuning;
enum bus_mode user_speed_mode; /* input speed mode from user */
Reviewed-by: Paul Barker paul.barker.ct@bp.renesas.com Tested-by: Paul Barker paul.barker.ct@bp.renesas.com (tested on RZ/G2L with commit ad50a8151387 from https://source.denx.de/u-boot/custodians/u-boot-sh)
Do not access SCC when sending commands during tuning operation as that will disrupt the tuning operation. The tuning operation is adjusting the SCC settings itself in execute_tuning callback.
When renesas_sdhi_execute_tuning() is called by the MMC core code, a loop which consists of renesas_sdhi_prepare_tuning(), mmc_send_tuning() and renesas_sdhi_compare_scc_data() iterates over each SCC tuning tap.
The renesas_sdhi_prepare_tuning() configures the SCC tuning tap number into hardware, mmc_send_tuning() triggers transfer of tuning block which depends on the bus mode for which the bus is currently being tuned, this information is supplied by the MMC core code, and finally renesas_sdhi_compare_scc_data() tests the received tuning block for validity.
Because renesas_sdhi_prepare_tuning() configures the SCC tuning tap into the hardware to fit the tuning operation, mmc_send_tuning() which triggers command transfer using renesas_sdhi_send_cmd() must not manipulate with the SCC in any way. Currently renesas_sdhi_send_cmd() does unconditionally call renesas_sdhi_check_scc_error(), which may adjust the SCC tuning tap position by writing RENESAS_SDHI_SCC_TAPSET, which would overwrite the required tuning configuration set by renesas_sdhi_prepare_tuning() and disrupt the tuning operation.
Fix this by skipping the renesas_sdhi_check_scc_error() call in case the MMC subsystem is in tuning state. This way, the SCC settings are left unmodified by command transfer during tuning operation.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org --- Cc: Hai Pham hai.pham.ud@renesas.com Cc: Jaehoon Chung jh80.chung@samsung.com Cc: Nobuhiro Iwamatsu iwamatsu@nigauri.org Cc: Paul Barker paul.barker.ct@bp.renesas.com Cc: Peng Fan peng.fan@nxp.com Cc: Sean Anderson seanga2@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Yoshihiro Shimoda yoshihiro.shimoda.uh@renesas.com --- drivers/mmc/renesas-sdhi.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/renesas-sdhi.c b/drivers/mmc/renesas-sdhi.c index 9770b6bb5e1..316b75b35fe 100644 --- a/drivers/mmc/renesas-sdhi.c +++ b/drivers/mmc/renesas-sdhi.c @@ -798,9 +798,12 @@ static int renesas_sdhi_send_cmd(struct udevice *dev, struct mmc_cmd *cmd, #if CONFIG_IS_ENABLED(MMC_UHS_SUPPORT) || \ CONFIG_IS_ENABLED(MMC_HS200_SUPPORT) || \ CONFIG_IS_ENABLED(MMC_HS400_SUPPORT) + struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); struct tmio_sd_priv *priv = dev_get_priv(dev); + struct mmc *mmc = upriv->mmc;
- renesas_sdhi_check_scc_error(dev); + if (!mmc->tuning) + renesas_sdhi_check_scc_error(dev);
if (cmd->cmdidx == MMC_CMD_SEND_STATUS) renesas_sdhi_adjust_hs400_mode_enable(priv);
On 20/02/2024 08:37, Marek Vasut wrote:
Do not access SCC when sending commands during tuning operation as that will disrupt the tuning operation. The tuning operation is adjusting the SCC settings itself in execute_tuning callback.
When renesas_sdhi_execute_tuning() is called by the MMC core code, a loop which consists of renesas_sdhi_prepare_tuning(), mmc_send_tuning() and renesas_sdhi_compare_scc_data() iterates over each SCC tuning tap.
The renesas_sdhi_prepare_tuning() configures the SCC tuning tap number into hardware, mmc_send_tuning() triggers transfer of tuning block which depends on the bus mode for which the bus is currently being tuned, this information is supplied by the MMC core code, and finally renesas_sdhi_compare_scc_data() tests the received tuning block for validity.
Because renesas_sdhi_prepare_tuning() configures the SCC tuning tap into the hardware to fit the tuning operation, mmc_send_tuning() which triggers command transfer using renesas_sdhi_send_cmd() must not manipulate with the SCC in any way. Currently renesas_sdhi_send_cmd() does unconditionally call renesas_sdhi_check_scc_error(), which may adjust the SCC tuning tap position by writing RENESAS_SDHI_SCC_TAPSET, which would overwrite the required tuning configuration set by renesas_sdhi_prepare_tuning() and disrupt the tuning operation.
Fix this by skipping the renesas_sdhi_check_scc_error() call in case the MMC subsystem is in tuning state. This way, the SCC settings are left unmodified by command transfer during tuning operation.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org
Cc: Hai Pham hai.pham.ud@renesas.com Cc: Jaehoon Chung jh80.chung@samsung.com Cc: Nobuhiro Iwamatsu iwamatsu@nigauri.org Cc: Paul Barker paul.barker.ct@bp.renesas.com Cc: Peng Fan peng.fan@nxp.com Cc: Sean Anderson seanga2@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Yoshihiro Shimoda yoshihiro.shimoda.uh@renesas.com
drivers/mmc/renesas-sdhi.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/renesas-sdhi.c b/drivers/mmc/renesas-sdhi.c index 9770b6bb5e1..316b75b35fe 100644 --- a/drivers/mmc/renesas-sdhi.c +++ b/drivers/mmc/renesas-sdhi.c @@ -798,9 +798,12 @@ static int renesas_sdhi_send_cmd(struct udevice *dev, struct mmc_cmd *cmd, #if CONFIG_IS_ENABLED(MMC_UHS_SUPPORT) || \ CONFIG_IS_ENABLED(MMC_HS200_SUPPORT) || \ CONFIG_IS_ENABLED(MMC_HS400_SUPPORT)
- struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); struct tmio_sd_priv *priv = dev_get_priv(dev);
- struct mmc *mmc = upriv->mmc;
- renesas_sdhi_check_scc_error(dev);
if (!mmc->tuning)
renesas_sdhi_check_scc_error(dev);if (cmd->cmdidx == MMC_CMD_SEND_STATUS) renesas_sdhi_adjust_hs400_mode_enable(priv);
Reviewed-by: Paul Barker paul.barker.ct@bp.renesas.com Tested-by: Paul Barker paul.barker.ct@bp.renesas.com (tested on RZ/G2L with commit ad50a8151387 from https://source.denx.de/u-boot/custodians/u-boot-sh)
On 20/02/2024 08:37, Marek Vasut wrote:
This hs400_tuning is a flag, make it bool. No functional change. This will be useful in the following patch, which adds another more generic flag, where the compiler can better use the space now reserved for the u8 to store more flags in it.
The minimum size for a bool is one byte so there likely won't be any improvement in struct size from using bool instead of u8 for `hs400_tuning` here and `tuning` added in the next patch. I still think it's a good change to make though, bool is the right type for an on/off flag.
So I think the commit message needs a little clarification. Other than that,
Reviewed-by: Paul Barker paul.barker.ct@bp.renesas.com Tested-by: Paul Barker paul.barker.ct@bp.renesas.com (tested on RZ/G2L with commit ad50a8151387 from https://source.denx.de/u-boot/custodians/u-boot-sh)
Thanks,
On 2/20/24 11:57, Paul Barker wrote:
On 20/02/2024 08:37, Marek Vasut wrote:
This hs400_tuning is a flag, make it bool. No functional change. This will be useful in the following patch, which adds another more generic flag, where the compiler can better use the space now reserved for the u8 to store more flags in it.
The minimum size for a bool is one byte so there likely won't be any improvement in struct size from using bool instead of u8 for `hs400_tuning` here and `tuning` added in the next patch. I still think it's a good change to make though, bool is the right type for an on/off flag.
The compiler does not do boolean packing in structures ?
On 20/02/2024 11:27, Marek Vasut wrote:
On 2/20/24 11:57, Paul Barker wrote:
On 20/02/2024 08:37, Marek Vasut wrote:
This hs400_tuning is a flag, make it bool. No functional change. This will be useful in the following patch, which adds another more generic flag, where the compiler can better use the space now reserved for the u8 to store more flags in it.
The minimum size for a bool is one byte so there likely won't be any improvement in struct size from using bool instead of u8 for `hs400_tuning` here and `tuning` added in the next patch. I still think it's a good change to make though, bool is the right type for an on/off flag.
The compiler does not do boolean packing in structures ?
The compiler will only pack booleans if you explicitly say that only one bit of memory is needed, e.g.:
bool tuning:1; bool hs400_tuning:1;
Otherwise the assumption is that you may wish to take the address of each field and so each one must have a distinct address in memory.
Thanks,
On 2/20/24 3:41 PM, Paul Barker wrote:
On 20/02/2024 11:27, Marek Vasut wrote:
On 2/20/24 11:57, Paul Barker wrote:
On 20/02/2024 08:37, Marek Vasut wrote:
This hs400_tuning is a flag, make it bool. No functional change. This will be useful in the following patch, which adds another more generic flag, where the compiler can better use the space now reserved for the u8 to store more flags in it.
The minimum size for a bool is one byte so there likely won't be any improvement in struct size from using bool instead of u8 for `hs400_tuning` here and `tuning` added in the next patch. I still think it's a good change to make though, bool is the right type for an on/off flag.
The compiler does not do boolean packing in structures ?
The compiler will only pack booleans if you explicitly say that only one bit of memory is needed, e.g.:
bool tuning:1; bool hs400_tuning:1;Otherwise the assumption is that you may wish to take the address of each field and so each one must have a distinct address in memory.
Fixed in V2, thanks !
participants (3)
-
Marek Vasut -
Marek Vasut -
Paul Barker