* Re: [PATCH] mmc: don't print 'MMC:' if there are no MMC devices
2024-11-13 5:30 ` [PATCH] mmc: don't print 'MMC:' if there are no MMC devices Caleb Connolly
@ 2024-11-13 7:43 ` Ilias Apalodimas
2024-11-13 9:48 ` Dragan Simic
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Ilias Apalodimas @ 2024-11-13 7:43 UTC (permalink / raw)
To: Caleb Connolly
Cc: Christian Marangi, Dragan Simic, Jaehoon Chung, Jerome Forissier,
Jonas Karlman, Marek Vasut, Peng Fan, Peter Robinson,
Rasmus Villemoes, Simon Glass, Sughosh Ganu, Tom Rini, u-boot
On Wed, 13 Nov 2024 at 07:30, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>
> It may be the case that MMC support is enabled even though the board
> we're booting on doesn't have any MMC devices. Move the print over to
> the print_mmc_devices() function where we can only print it if we
> actually have MMC devices.
>
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
> common/board_r.c | 1 -
> drivers/mmc/mmc-uclass.c | 4 +++-
> drivers/mmc/mmc_legacy.c | 5 +++++
> 3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/common/board_r.c b/common/board_r.c
> index 62228a723e12..232a8fd19f03 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -384,9 +384,8 @@ static int initr_onenand(void)
>
> #ifdef CONFIG_MMC
> static int initr_mmc(void)
> {
> - puts("MMC: ");
> mmc_initialize(gd->bd);
> return 0;
> }
> #endif
> diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c
> index c8db4f811c2f..56fe29249c36 100644
> --- a/drivers/mmc/mmc-uclass.c
> +++ b/drivers/mmc/mmc-uclass.c
> @@ -384,9 +384,11 @@ void print_mmc_devices(char separator)
> dev;
> uclass_next_device(&dev), first = false) {
> struct mmc *m = mmc_get_mmc_dev(dev);
>
> - if (!first) {
> + if (first) {
> + printf("MMC: ");
> + } else {
> printf("%c", separator);
> if (separator != '\n')
> puts(" ");
> }
> diff --git a/drivers/mmc/mmc_legacy.c b/drivers/mmc/mmc_legacy.c
> index 8f8ba34be717..f4a049a4a4d4 100644
> --- a/drivers/mmc/mmc_legacy.c
> +++ b/drivers/mmc/mmc_legacy.c
> @@ -98,8 +98,9 @@ void print_mmc_devices(char separator)
> {
> struct mmc *m;
> struct list_head *entry;
> char *mmc_type;
> + bool first = true;
>
> list_for_each(entry, &mmc_devices) {
> m = list_entry(entry, struct mmc, link);
>
> @@ -107,8 +108,12 @@ void print_mmc_devices(char separator)
> mmc_type = IS_SD(m) ? "SD" : "eMMC";
> else
> mmc_type = NULL;
>
> + if (first) {
> + printf("MMC: ");
> + first = false;
> + }
> printf("%s: %d", m->cfg->name, m->block_dev.devnum);
> if (mmc_type)
> printf(" (%s)", mmc_type);
>
> --
> 2.47.0
>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] mmc: don't print 'MMC:' if there are no MMC devices
2024-11-13 5:30 ` [PATCH] mmc: don't print 'MMC:' if there are no MMC devices Caleb Connolly
2024-11-13 7:43 ` Ilias Apalodimas
@ 2024-11-13 9:48 ` Dragan Simic
2024-11-13 13:08 ` Caleb Connolly
2024-11-13 14:24 ` Tom Rini
2024-11-14 6:54 ` Jaehoon Chung
3 siblings, 1 reply; 11+ messages in thread
From: Dragan Simic @ 2024-11-13 9:48 UTC (permalink / raw)
To: Caleb Connolly
Cc: Christian Marangi, Ilias Apalodimas, Jaehoon Chung,
Jerome Forissier, Jonas Karlman, Marek Vasut, Peng Fan,
Peter Robinson, Rasmus Villemoes, Simon Glass, Sughosh Ganu,
Tom Rini, u-boot
Hello Caleb,
Thanks for the patch. Please, see a comment below.
On 2024-11-13 06:30, Caleb Connolly wrote:
> It may be the case that MMC support is enabled even though the board
> we're booting on doesn't have any MMC devices. Move the print over to
> the print_mmc_devices() function where we can only print it if we
> actually have MMC devices.
>
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
> common/board_r.c | 1 -
> drivers/mmc/mmc-uclass.c | 4 +++-
> drivers/mmc/mmc_legacy.c | 5 +++++
> 3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/common/board_r.c b/common/board_r.c
> index 62228a723e12..232a8fd19f03 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -384,9 +384,8 @@ static int initr_onenand(void)
>
> #ifdef CONFIG_MMC
> static int initr_mmc(void)
> {
> - puts("MMC: ");
> mmc_initialize(gd->bd);
> return 0;
> }
> #endif
> diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c
> index c8db4f811c2f..56fe29249c36 100644
> --- a/drivers/mmc/mmc-uclass.c
> +++ b/drivers/mmc/mmc-uclass.c
> @@ -384,9 +384,11 @@ void print_mmc_devices(char separator)
> dev;
> uclass_next_device(&dev), first = false) {
> struct mmc *m = mmc_get_mmc_dev(dev);
>
> - if (!first) {
> + if (first) {
> + printf("MMC: ");
> + } else {
> printf("%c", separator);
> if (separator != '\n')
> puts(" ");
> }
> diff --git a/drivers/mmc/mmc_legacy.c b/drivers/mmc/mmc_legacy.c
> index 8f8ba34be717..f4a049a4a4d4 100644
> --- a/drivers/mmc/mmc_legacy.c
> +++ b/drivers/mmc/mmc_legacy.c
> @@ -98,8 +98,9 @@ void print_mmc_devices(char separator)
> {
> struct mmc *m;
> struct list_head *entry;
> char *mmc_type;
> + bool first = true;
>
> list_for_each(entry, &mmc_devices) {
> m = list_entry(entry, struct mmc, link);
>
> @@ -107,8 +108,12 @@ void print_mmc_devices(char separator)
> mmc_type = IS_SD(m) ? "SD" : "eMMC";
> else
> mmc_type = NULL;
>
> + if (first) {
> + printf("MMC: ");
> + first = false;
> + }
Can't we simply check for "entry == &mmc_devices->next" here and
avoid the introduction of "first"?
> printf("%s: %d", m->cfg->name, m->block_dev.devnum);
> if (mmc_type)
> printf(" (%s)", mmc_type);
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] mmc: don't print 'MMC:' if there are no MMC devices
2024-11-13 9:48 ` Dragan Simic
@ 2024-11-13 13:08 ` Caleb Connolly
2024-11-13 13:15 ` Dragan Simic
0 siblings, 1 reply; 11+ messages in thread
From: Caleb Connolly @ 2024-11-13 13:08 UTC (permalink / raw)
To: Dragan Simic
Cc: Christian Marangi, Ilias Apalodimas, Jaehoon Chung,
Jerome Forissier, Jonas Karlman, Marek Vasut, Peng Fan,
Peter Robinson, Rasmus Villemoes, Simon Glass, Sughosh Ganu,
Tom Rini, u-boot
On 13/11/2024 10:48, Dragan Simic wrote:
> Hello Caleb,
>
> Thanks for the patch. Please, see a comment below.
>
> On 2024-11-13 06:30, Caleb Connolly wrote:
>> It may be the case that MMC support is enabled even though the board
>> we're booting on doesn't have any MMC devices. Move the print over to
>> the print_mmc_devices() function where we can only print it if we
>> actually have MMC devices.
>>
>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>> ---
>> common/board_r.c | 1 -
>> drivers/mmc/mmc-uclass.c | 4 +++-
>> drivers/mmc/mmc_legacy.c | 5 +++++
>> 3 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/board_r.c b/common/board_r.c
>> index 62228a723e12..232a8fd19f03 100644
>> --- a/common/board_r.c
>> +++ b/common/board_r.c
>> @@ -384,9 +384,8 @@ static int initr_onenand(void)
>>
>> #ifdef CONFIG_MMC
>> static int initr_mmc(void)
>> {
>> - puts("MMC: ");
>> mmc_initialize(gd->bd);
>> return 0;
>> }
>> #endif
>> diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c
>> index c8db4f811c2f..56fe29249c36 100644
>> --- a/drivers/mmc/mmc-uclass.c
>> +++ b/drivers/mmc/mmc-uclass.c
>> @@ -384,9 +384,11 @@ void print_mmc_devices(char separator)
>> dev;
>> uclass_next_device(&dev), first = false) {
>> struct mmc *m = mmc_get_mmc_dev(dev);
>>
>> - if (!first) {
>> + if (first) {
>> + printf("MMC: ");
>> + } else {
>> printf("%c", separator);
>> if (separator != '\n')
>> puts(" ");
>> }
>> diff --git a/drivers/mmc/mmc_legacy.c b/drivers/mmc/mmc_legacy.c
>> index 8f8ba34be717..f4a049a4a4d4 100644
>> --- a/drivers/mmc/mmc_legacy.c
>> +++ b/drivers/mmc/mmc_legacy.c
>> @@ -98,8 +98,9 @@ void print_mmc_devices(char separator)
>> {
>> struct mmc *m;
>> struct list_head *entry;
>> char *mmc_type;
>> + bool first = true;
>>
>> list_for_each(entry, &mmc_devices) {
>> m = list_entry(entry, struct mmc, link);
>>
>> @@ -107,8 +108,12 @@ void print_mmc_devices(char separator)
>> mmc_type = IS_SD(m) ? "SD" : "eMMC";
>> else
>> mmc_type = NULL;
>>
>> + if (first) {
>> + printf("MMC: ");
>> + first = false;
>> + }
>
> Can't we simply check for "entry == &mmc_devices->next" here and
> avoid the introduction of "first"?
That would be clever, yeah I can do this in v2.
I hvaen't found a way to test mmc_legacy thus far, does anyone know how
I could?
>
>> printf("%s: %d", m->cfg->name, m->block_dev.devnum);
>> if (mmc_type)
>> printf(" (%s)", mmc_type);
--
// Caleb (they/them)
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] mmc: don't print 'MMC:' if there are no MMC devices
2024-11-13 13:08 ` Caleb Connolly
@ 2024-11-13 13:15 ` Dragan Simic
0 siblings, 0 replies; 11+ messages in thread
From: Dragan Simic @ 2024-11-13 13:15 UTC (permalink / raw)
To: Caleb Connolly
Cc: Christian Marangi, Ilias Apalodimas, Jaehoon Chung,
Jerome Forissier, Jonas Karlman, Marek Vasut, Peng Fan,
Peter Robinson, Rasmus Villemoes, Simon Glass, Sughosh Ganu,
Tom Rini, u-boot
Hello Caleb,
On 2024-11-13 14:08, Caleb Connolly wrote:
> On 13/11/2024 10:48, Dragan Simic wrote:
>> Thanks for the patch. Please, see a comment below.
>>
>> On 2024-11-13 06:30, Caleb Connolly wrote:
>>> It may be the case that MMC support is enabled even though the board
>>> we're booting on doesn't have any MMC devices. Move the print over to
>>> the print_mmc_devices() function where we can only print it if we
>>> actually have MMC devices.
>>>
>>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>>> ---
>>> common/board_r.c | 1 -
>>> drivers/mmc/mmc-uclass.c | 4 +++-
>>> drivers/mmc/mmc_legacy.c | 5 +++++
>>> 3 files changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/common/board_r.c b/common/board_r.c
>>> index 62228a723e12..232a8fd19f03 100644
>>> --- a/common/board_r.c
>>> +++ b/common/board_r.c
>>> @@ -384,9 +384,8 @@ static int initr_onenand(void)
>>>
>>> #ifdef CONFIG_MMC
>>> static int initr_mmc(void)
>>> {
>>> - puts("MMC: ");
>>> mmc_initialize(gd->bd);
>>> return 0;
>>> }
>>> #endif
>>> diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c
>>> index c8db4f811c2f..56fe29249c36 100644
>>> --- a/drivers/mmc/mmc-uclass.c
>>> +++ b/drivers/mmc/mmc-uclass.c
>>> @@ -384,9 +384,11 @@ void print_mmc_devices(char separator)
>>> dev;
>>> uclass_next_device(&dev), first = false) {
>>> struct mmc *m = mmc_get_mmc_dev(dev);
>>>
>>> - if (!first) {
>>> + if (first) {
>>> + printf("MMC: ");
>>> + } else {
>>> printf("%c", separator);
>>> if (separator != '\n')
>>> puts(" ");
>>> }
>>> diff --git a/drivers/mmc/mmc_legacy.c b/drivers/mmc/mmc_legacy.c
>>> index 8f8ba34be717..f4a049a4a4d4 100644
>>> --- a/drivers/mmc/mmc_legacy.c
>>> +++ b/drivers/mmc/mmc_legacy.c
>>> @@ -98,8 +98,9 @@ void print_mmc_devices(char separator)
>>> {
>>> struct mmc *m;
>>> struct list_head *entry;
>>> char *mmc_type;
>>> + bool first = true;
>>>
>>> list_for_each(entry, &mmc_devices) {
>>> m = list_entry(entry, struct mmc, link);
>>>
>>> @@ -107,8 +108,12 @@ void print_mmc_devices(char separator)
>>> mmc_type = IS_SD(m) ? "SD" : "eMMC";
>>> else
>>> mmc_type = NULL;
>>>
>>> + if (first) {
>>> + printf("MMC: ");
>>> + first = false;
>>> + }
>>
>> Can't we simply check for "entry == &mmc_devices->next" here and
>> avoid the introduction of "first"?
>
> That would be clever, yeah I can do this in v2.
Thanks, please make sure to Cc me in the v2 submission.
> I hvaen't found a way to test mmc_legacy thus far, does anyone know how
> I could?
>
>>> printf("%s: %d", m->cfg->name, m->block_dev.devnum);
>>> if (mmc_type)
>>> printf(" (%s)", mmc_type);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mmc: don't print 'MMC:' if there are no MMC devices
2024-11-13 5:30 ` [PATCH] mmc: don't print 'MMC:' if there are no MMC devices Caleb Connolly
2024-11-13 7:43 ` Ilias Apalodimas
2024-11-13 9:48 ` Dragan Simic
@ 2024-11-13 14:24 ` Tom Rini
2024-11-13 14:47 ` Caleb Connolly
2024-11-14 6:54 ` Jaehoon Chung
3 siblings, 1 reply; 11+ messages in thread
From: Tom Rini @ 2024-11-13 14:24 UTC (permalink / raw)
To: Caleb Connolly
Cc: Christian Marangi, Dragan Simic, Ilias Apalodimas, Jaehoon Chung,
Jerome Forissier, Jonas Karlman, Marek Vasut, Peng Fan,
Peter Robinson, Rasmus Villemoes, Simon Glass, Sughosh Ganu,
u-boot
[-- Attachment #1: Type: text/plain, Size: 743 bytes --]
On Wed, Nov 13, 2024 at 06:30:08AM +0100, Caleb Connolly wrote:
> It may be the case that MMC support is enabled even though the board
> we're booting on doesn't have any MMC devices. Move the print over to
> the print_mmc_devices() function where we can only print it if we
> actually have MMC devices.
>
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
I'm not sure I like this. What we do / don't find on startup is part of
the not-exactly-API. It's true that if we don't print an MMC line at
all, and we should have MMC, the user (and any scripts that parse
console output) but now we're also increasing the code size a little bit
too. I can be convinced this is a good idea, but I'm not there yet.
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mmc: don't print 'MMC:' if there are no MMC devices
2024-11-13 14:24 ` Tom Rini
@ 2024-11-13 14:47 ` Caleb Connolly
2024-11-14 7:34 ` Jaehoon Chung
2024-11-14 18:02 ` Tom Rini
0 siblings, 2 replies; 11+ messages in thread
From: Caleb Connolly @ 2024-11-13 14:47 UTC (permalink / raw)
To: Tom Rini
Cc: Christian Marangi, Dragan Simic, Ilias Apalodimas, Jaehoon Chung,
Jerome Forissier, Jonas Karlman, Marek Vasut, Peng Fan,
Peter Robinson, Rasmus Villemoes, Simon Glass, Sughosh Ganu,
u-boot
On 13/11/2024 15:24, Tom Rini wrote:
> On Wed, Nov 13, 2024 at 06:30:08AM +0100, Caleb Connolly wrote:
>
>> It may be the case that MMC support is enabled even though the board
>> we're booting on doesn't have any MMC devices. Move the print over to
>> the print_mmc_devices() function where we can only print it if we
>> actually have MMC devices.
>>
>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>
> I'm not sure I like this. What we do / don't find on startup is part of
> the not-exactly-API. It's true that if we don't print an MMC line at
> all, and we should have MMC, the user (and any scripts that parse
> console output) but now we're also increasing the code size a little bit
> too. I can be convinced this is a good idea, but I'm not there yet.
Hmm, fair enough. I'll offer some more context, maybe there's a smarter
approach here I'm not seeing.
The main place this shows up is on Qualcomm boards. Since all Qualcomm
armv8 targets are supported with qcom_defconfig (just by adjusting which
DTB is used), we can't know at build time whether the board has MMC.
I guess my thinking behind this patch comes from a bigger picture desire
to get UFS and MMC more aligned. The number of devices with UFS is
definitely going up, and I would argue that U-Boot's inconsistent
treatment of these two storage classes (obviously a result of their
relative age and support in the codebase) is really unintuitive and
weird for users (nevermind that the "scsi" command is used for UFS
devices, cute though it is).
I'm really wary to open this whole can of worms, since I guess it would
require some larger efforts and collaboration to fix. But maybe this
patch (or one like it) would be better suited in the context of some
larger effort to unify storage backends?
Kind regards,
>
--
// Caleb (they/them)
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] mmc: don't print 'MMC:' if there are no MMC devices
2024-11-13 14:47 ` Caleb Connolly
@ 2024-11-14 7:34 ` Jaehoon Chung
2024-11-14 18:02 ` Tom Rini
1 sibling, 0 replies; 11+ messages in thread
From: Jaehoon Chung @ 2024-11-14 7:34 UTC (permalink / raw)
To: 'Caleb Connolly', 'Tom Rini'
Cc: 'Christian Marangi', 'Dragan Simic',
'Ilias Apalodimas', 'Jerome Forissier',
'Jonas Karlman', 'Marek Vasut',
'Peng Fan', 'Peter Robinson',
'Rasmus Villemoes', 'Simon Glass',
'Sughosh Ganu', u-boot
Hi,
> -----Original Message-----
> From: Caleb Connolly <caleb.connolly@linaro.org>
> Sent: Wednesday, November 13, 2024 11:47 PM
> To: Tom Rini <trini@konsulko.com>
> Cc: Christian Marangi <ansuelsmth@gmail.com>; Dragan Simic <dsimic@manjaro.org>; Ilias Apalodimas
> <ilias.apalodimas@linaro.org>; Jaehoon Chung <jh80.chung@samsung.com>; Jerome Forissier
> <jerome.forissier@linaro.org>; Jonas Karlman <jonas@kwiboo.se>; Marek Vasut
> <marek.vasut+renesas@mailbox.org>; Peng Fan <peng.fan@nxp.com>; Peter Robinson <pbrobinson@gmail.com>;
> Rasmus Villemoes <rasmus.villemoes@prevas.dk>; Simon Glass <sjg@chromium.org>; Sughosh Ganu
> <sughosh.ganu@linaro.org>; u-boot@lists.denx.de
> Subject: Re: [PATCH] mmc: don't print 'MMC:' if there are no MMC devices
>
>
>
> On 13/11/2024 15:24, Tom Rini wrote:
> > On Wed, Nov 13, 2024 at 06:30:08AM +0100, Caleb Connolly wrote:
> >
> >> It may be the case that MMC support is enabled even though the board
> >> we're booting on doesn't have any MMC devices. Move the print over to
> >> the print_mmc_devices() function where we can only print it if we
> >> actually have MMC devices.
> >>
> >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> >
> > I'm not sure I like this. What we do / don't find on startup is part of
> > the not-exactly-API. It's true that if we don't print an MMC line at
> > all, and we should have MMC, the user (and any scripts that parse
> > console output) but now we're also increasing the code size a little bit
> > too. I can be convinced this is a good idea, but I'm not there yet.
>
> Hmm, fair enough. I'll offer some more context, maybe there's a smarter
> approach here I'm not seeing.
>
> The main place this shows up is on Qualcomm boards. Since all Qualcomm
> armv8 targets are supported with qcom_defconfig (just by adjusting which
> DTB is used), we can't know at build time whether the board has MMC.
I'm also not sure if it has to apply this at this time.
Partially, I understood what you said about your case.
But In my case, the printing MMC information was useful to do debug at booting time.
(correct dtb or not, or wrong configuration, etc)
Best Regards,
Jaehoon Chung
>
> I guess my thinking behind this patch comes from a bigger picture desire
> to get UFS and MMC more aligned. The number of devices with UFS is
> definitely going up, and I would argue that U-Boot's inconsistent
> treatment of these two storage classes (obviously a result of their
> relative age and support in the codebase) is really unintuitive and
> weird for users (nevermind that the "scsi" command is used for UFS
> devices, cute though it is).
>
> I'm really wary to open this whole can of worms, since I guess it would
> require some larger efforts and collaboration to fix. But maybe this
> patch (or one like it) would be better suited in the context of some
> larger effort to unify storage backends?
>
> Kind regards,
> >
>
> --
> // Caleb (they/them)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mmc: don't print 'MMC:' if there are no MMC devices
2024-11-13 14:47 ` Caleb Connolly
2024-11-14 7:34 ` Jaehoon Chung
@ 2024-11-14 18:02 ` Tom Rini
2024-11-14 18:05 ` Caleb Connolly
1 sibling, 1 reply; 11+ messages in thread
From: Tom Rini @ 2024-11-14 18:02 UTC (permalink / raw)
To: Caleb Connolly
Cc: Christian Marangi, Dragan Simic, Ilias Apalodimas, Jaehoon Chung,
Jerome Forissier, Jonas Karlman, Marek Vasut, Peng Fan,
Peter Robinson, Rasmus Villemoes, Simon Glass, Sughosh Ganu,
u-boot
[-- Attachment #1: Type: text/plain, Size: 2464 bytes --]
On Wed, Nov 13, 2024 at 03:47:16PM +0100, Caleb Connolly wrote:
>
>
> On 13/11/2024 15:24, Tom Rini wrote:
> > On Wed, Nov 13, 2024 at 06:30:08AM +0100, Caleb Connolly wrote:
> >
> >> It may be the case that MMC support is enabled even though the board
> >> we're booting on doesn't have any MMC devices. Move the print over to
> >> the print_mmc_devices() function where we can only print it if we
> >> actually have MMC devices.
> >>
> >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> >
> > I'm not sure I like this. What we do / don't find on startup is part of
> > the not-exactly-API. It's true that if we don't print an MMC line at
> > all, and we should have MMC, the user (and any scripts that parse
> > console output) but now we're also increasing the code size a little bit
> > too. I can be convinced this is a good idea, but I'm not there yet.
>
> Hmm, fair enough. I'll offer some more context, maybe there's a smarter
> approach here I'm not seeing.
>
> The main place this shows up is on Qualcomm boards. Since all Qualcomm
> armv8 targets are supported with qcom_defconfig (just by adjusting which
> DTB is used), we can't know at build time whether the board has MMC.
>
> I guess my thinking behind this patch comes from a bigger picture desire
> to get UFS and MMC more aligned. The number of devices with UFS is
> definitely going up, and I would argue that U-Boot's inconsistent
> treatment of these two storage classes (obviously a result of their
> relative age and support in the codebase) is really unintuitive and
> weird for users (nevermind that the "scsi" command is used for UFS
> devices, cute though it is).
>
> I'm really wary to open this whole can of worms, since I guess it would
> require some larger efforts and collaboration to fix. But maybe this
> patch (or one like it) would be better suited in the context of some
> larger effort to unify storage backends?
I get it now, thanks. And yeah, this is part of our inconsistency in
printing information. I'd rather leave things alone here (assuming the
MMC: printout is reasonable in the case of no devices, similar to how
the Net: printout is reasonable in that case) and wait for a longer term
/ high level solution wherein for example as we go down the device tree
we give some consistent level of information for everything (and more or
less info depending on verbosity configured perhaps).
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mmc: don't print 'MMC:' if there are no MMC devices
2024-11-14 18:02 ` Tom Rini
@ 2024-11-14 18:05 ` Caleb Connolly
0 siblings, 0 replies; 11+ messages in thread
From: Caleb Connolly @ 2024-11-14 18:05 UTC (permalink / raw)
To: Tom Rini
Cc: Christian Marangi, Dragan Simic, Ilias Apalodimas, Jaehoon Chung,
Jerome Forissier, Jonas Karlman, Marek Vasut, Peng Fan,
Peter Robinson, Rasmus Villemoes, Simon Glass, Sughosh Ganu,
u-boot
Hi Tom,
On 14/11/2024 19:02, Tom Rini wrote:
> On Wed, Nov 13, 2024 at 03:47:16PM +0100, Caleb Connolly wrote:
>>
>>
>> On 13/11/2024 15:24, Tom Rini wrote:
>>> On Wed, Nov 13, 2024 at 06:30:08AM +0100, Caleb Connolly wrote:
>>>
>>>> It may be the case that MMC support is enabled even though the board
>>>> we're booting on doesn't have any MMC devices. Move the print over to
>>>> the print_mmc_devices() function where we can only print it if we
>>>> actually have MMC devices.
>>>>
>>>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>>>
>>> I'm not sure I like this. What we do / don't find on startup is part of
>>> the not-exactly-API. It's true that if we don't print an MMC line at
>>> all, and we should have MMC, the user (and any scripts that parse
>>> console output) but now we're also increasing the code size a little bit
>>> too. I can be convinced this is a good idea, but I'm not there yet.
>>
>> Hmm, fair enough. I'll offer some more context, maybe there's a smarter
>> approach here I'm not seeing.
>>
>> The main place this shows up is on Qualcomm boards. Since all Qualcomm
>> armv8 targets are supported with qcom_defconfig (just by adjusting which
>> DTB is used), we can't know at build time whether the board has MMC.
>>
>> I guess my thinking behind this patch comes from a bigger picture desire
>> to get UFS and MMC more aligned. The number of devices with UFS is
>> definitely going up, and I would argue that U-Boot's inconsistent
>> treatment of these two storage classes (obviously a result of their
>> relative age and support in the codebase) is really unintuitive and
>> weird for users (nevermind that the "scsi" command is used for UFS
>> devices, cute though it is).
>>
>> I'm really wary to open this whole can of worms, since I guess it would
>> require some larger efforts and collaboration to fix. But maybe this
>> patch (or one like it) would be better suited in the context of some
>> larger effort to unify storage backends?
>
> I get it now, thanks. And yeah, this is part of our inconsistency in
> printing information. I'd rather leave things alone here (assuming the
> MMC: printout is reasonable in the case of no devices, similar to how
> the Net: printout is reasonable in that case) and wait for a longer term
> / high level solution wherein for example as we go down the device tree
> we give some consistent level of information for everything (and more or
> less info depending on verbosity configured perhaps).
>
Thanks for taking a look. Something like what you're suggesting sounds
good to me, and I agree it's fine to leave things as-is until we have
such a plan.
Kind regards,
--
// Caleb (they/them)
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] mmc: don't print 'MMC:' if there are no MMC devices
2024-11-13 5:30 ` [PATCH] mmc: don't print 'MMC:' if there are no MMC devices Caleb Connolly
` (2 preceding siblings ...)
2024-11-13 14:24 ` Tom Rini
@ 2024-11-14 6:54 ` Jaehoon Chung
3 siblings, 0 replies; 11+ messages in thread
From: Jaehoon Chung @ 2024-11-14 6:54 UTC (permalink / raw)
To: 'Caleb Connolly', 'Christian Marangi',
'Dragan Simic', 'Ilias Apalodimas',
'Jerome Forissier', 'Jonas Karlman',
'Marek Vasut', 'Peng Fan',
'Peter Robinson', 'Rasmus Villemoes',
'Simon Glass', 'Sughosh Ganu', 'Tom Rini'
Cc: u-boot
Hi,
> -----Original Message-----
> From: Caleb Connolly <caleb.connolly@linaro.org>
> Sent: Wednesday, November 13, 2024 2:30 PM
> To: Caleb Connolly <caleb.connolly@linaro.org>; Christian Marangi <ansuelsmth@gmail.com>; Dragan Simic
> <dsimic@manjaro.org>; Ilias Apalodimas <ilias.apalodimas@linaro.org>; Jaehoon Chung
> <jh80.chung@samsung.com>; Jerome Forissier <jerome.forissier@linaro.org>; Jonas Karlman
> <jonas@kwiboo.se>; Marek Vasut <marek.vasut+renesas@mailbox.org>; Peng Fan <peng.fan@nxp.com>; Peter
> Robinson <pbrobinson@gmail.com>; Rasmus Villemoes <rasmus.villemoes@prevas.dk>; Simon Glass
> <sjg@chromium.org>; Sughosh Ganu <sughosh.ganu@linaro.org>; Tom Rini <trini@konsulko.com>
> Cc: u-boot@lists.denx.de
> Subject: [PATCH] mmc: don't print 'MMC:' if there are no MMC devices
>
> It may be the case that MMC support is enabled even though the board
> we're booting on doesn't have any MMC devices. Move the print over to
> the print_mmc_devices() function where we can only print it if we
> actually have MMC devices.
>
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
> common/board_r.c | 1 -
> drivers/mmc/mmc-uclass.c | 4 +++-
> drivers/mmc/mmc_legacy.c | 5 +++++
> 3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/common/board_r.c b/common/board_r.c
> index 62228a723e12..232a8fd19f03 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -384,9 +384,8 @@ static int initr_onenand(void)
>
> #ifdef CONFIG_MMC
> static int initr_mmc(void)
> {
> - puts("MMC: ");
> mmc_initialize(gd->bd);
> return 0;
> }
> #endif
> diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c
> index c8db4f811c2f..56fe29249c36 100644
> --- a/drivers/mmc/mmc-uclass.c
> +++ b/drivers/mmc/mmc-uclass.c
> @@ -384,9 +384,11 @@ void print_mmc_devices(char separator)
> dev;
> uclass_next_device(&dev), first = false) {
> struct mmc *m = mmc_get_mmc_dev(dev);
>
> - if (!first) {
> + if (first) {
> + printf("MMC: ");
> + } else {
Frankly, I don't know if it needs to print "MMC; " at here.
It's already knowing mmc with mmc command. In board_r, To clarify which device is showing "MMC", but not here.
Best Regards,
Jaehoon Chung
> printf("%c", separator);
> if (separator != '\n')
> puts(" ");
> }
> diff --git a/drivers/mmc/mmc_legacy.c b/drivers/mmc/mmc_legacy.c
> index 8f8ba34be717..f4a049a4a4d4 100644
> --- a/drivers/mmc/mmc_legacy.c
> +++ b/drivers/mmc/mmc_legacy.c
> @@ -98,8 +98,9 @@ void print_mmc_devices(char separator)
> {
> struct mmc *m;
> struct list_head *entry;
> char *mmc_type;
> + bool first = true;
>
> list_for_each(entry, &mmc_devices) {
> m = list_entry(entry, struct mmc, link);
>
> @@ -107,8 +108,12 @@ void print_mmc_devices(char separator)
> mmc_type = IS_SD(m) ? "SD" : "eMMC";
> else
> mmc_type = NULL;
>
> + if (first) {
> + printf("MMC: ");
> + first = false;
> + }
> printf("%s: %d", m->cfg->name, m->block_dev.devnum);
> if (mmc_type)
> printf(" (%s)", mmc_type);
>
> --
> 2.47.0
^ permalink raw reply [flat|nested] 11+ messages in thread