| Message ID | 20240220083755.34449-1-marek.vasut+renesas@mailbox.org |
|---|---|
| State | Superseded |
| Delegated to: | Jaehoon Chung |
| Headers | show |
| Series | [1/3] mmc: Convert hs400_tuning flag from u8 to bool | expand |
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 !
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 */ };
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(-)