* [PATCH 1/1] blk: simplify blk_get_devnum_by_typename()
@ 2021-10-23 14:06 Heinrich Schuchardt
2021-10-24 19:54 ` Simon Glass
0 siblings, 1 reply; 14+ messages in thread
From: Heinrich Schuchardt @ 2021-10-23 14:06 UTC (permalink / raw)
To: Simon Glass
Cc: Patrick Delaunay, Ilias Apalodimas, u-boot, Heinrich Schuchardt
The block descriptor contains the if_type. There is no need to first look
up the uclass for the if_type and then to check the parent device's uclass
to know if the device has the correct if_type.
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
drivers/block/blk-uclass.c | 35 +----------------------------------
1 file changed, 1 insertion(+), 34 deletions(-)
diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index 83682dcc18..b32067f957 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -33,22 +33,6 @@ static const char *if_typename_str[IF_TYPE_COUNT] = {
[IF_TYPE_PVBLOCK] = "pvblock",
};
-static enum uclass_id if_type_uclass_id[IF_TYPE_COUNT] = {
- [IF_TYPE_IDE] = UCLASS_IDE,
- [IF_TYPE_SCSI] = UCLASS_SCSI,
- [IF_TYPE_ATAPI] = UCLASS_INVALID,
- [IF_TYPE_USB] = UCLASS_MASS_STORAGE,
- [IF_TYPE_DOC] = UCLASS_INVALID,
- [IF_TYPE_MMC] = UCLASS_MMC,
- [IF_TYPE_SD] = UCLASS_INVALID,
- [IF_TYPE_SATA] = UCLASS_AHCI,
- [IF_TYPE_HOST] = UCLASS_ROOT,
- [IF_TYPE_NVME] = UCLASS_NVME,
- [IF_TYPE_EFI] = UCLASS_EFI,
- [IF_TYPE_VIRTIO] = UCLASS_VIRTIO,
- [IF_TYPE_PVBLOCK] = UCLASS_PVBLOCK,
-};
-
static enum if_type if_typename_to_iftype(const char *if_typename)
{
int i;
@@ -62,11 +46,6 @@ static enum if_type if_typename_to_iftype(const char *if_typename)
return IF_TYPE_UNKNOWN;
}
-static enum uclass_id if_type_to_uclass_id(enum if_type if_type)
-{
- return if_type_uclass_id[if_type];
-}
-
const char *blk_get_if_type_name(enum if_type if_type)
{
return if_typename_str[if_type];
@@ -93,7 +72,6 @@ struct blk_desc *blk_get_devnum_by_type(enum if_type if_type, int devnum)
*/
struct blk_desc *blk_get_devnum_by_typename(const char *if_typename, int devnum)
{
- enum uclass_id uclass_id;
enum if_type if_type;
struct udevice *dev;
struct uclass *uc;
@@ -105,12 +83,6 @@ struct blk_desc *blk_get_devnum_by_typename(const char *if_typename, int devnum)
if_typename);
return NULL;
}
- uclass_id = if_type_to_uclass_id(if_type);
- if (uclass_id == UCLASS_INVALID) {
- debug("%s: Unknown uclass for interface type'\n",
- if_typename_str[if_type]);
- return NULL;
- }
ret = uclass_get(UCLASS_BLK, &uc);
if (ret)
@@ -122,13 +94,8 @@ struct blk_desc *blk_get_devnum_by_typename(const char *if_typename, int devnum)
if_type, devnum, dev->name, desc->if_type, desc->devnum);
if (desc->devnum != devnum)
continue;
-
- /* Find out the parent device uclass */
- if (device_get_uclass_id(dev->parent) != uclass_id) {
- debug("%s: parent uclass %d, this dev %d\n", __func__,
- device_get_uclass_id(dev->parent), uclass_id);
+ if (desc->if_type != if_type)
continue;
- }
if (device_probe(dev))
return NULL;
--
2.32.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 1/1] blk: simplify blk_get_devnum_by_typename()
2021-10-23 14:06 [PATCH 1/1] blk: simplify blk_get_devnum_by_typename() Heinrich Schuchardt
@ 2021-10-24 19:54 ` Simon Glass
2021-10-25 7:54 ` Heinrich Schuchardt
0 siblings, 1 reply; 14+ messages in thread
From: Simon Glass @ 2021-10-24 19:54 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Patrick Delaunay, Ilias Apalodimas, U-Boot Mailing List
Hi Heinrich,
On Sat, 23 Oct 2021 at 08:06, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> The block descriptor contains the if_type. There is no need to first look
> up the uclass for the if_type and then to check the parent device's uclass
> to know if the device has the correct if_type.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> drivers/block/blk-uclass.c | 35 +----------------------------------
> 1 file changed, 1 insertion(+), 34 deletions(-)
This seems to be heading in the wrong direction though.
The IF_TYPE should really go away and be replaced with the UCLASS ID, I think.
At present we have lots of calls to blk_create_device_f() which
specify the type. I think we should drop the IF_TYPE_.. arg to that
function and have it figured out from the uclass, in the interim.
But why do we need IF_TYPE now?
Regards,
Simon
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] blk: simplify blk_get_devnum_by_typename()
2021-10-24 19:54 ` Simon Glass
@ 2021-10-25 7:54 ` Heinrich Schuchardt
2021-10-25 8:00 ` Heinrich Schuchardt
0 siblings, 1 reply; 14+ messages in thread
From: Heinrich Schuchardt @ 2021-10-25 7:54 UTC (permalink / raw)
To: Simon Glass; +Cc: Patrick Delaunay, Ilias Apalodimas, U-Boot Mailing List
On 10/24/21 21:54, Simon Glass wrote:
> Hi Heinrich,
>
> On Sat, 23 Oct 2021 at 08:06, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> The block descriptor contains the if_type. There is no need to first look
>> up the uclass for the if_type and then to check the parent device's uclass
>> to know if the device has the correct if_type.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>> drivers/block/blk-uclass.c | 35 +----------------------------------
>> 1 file changed, 1 insertion(+), 34 deletions(-)
>
> This seems to be heading in the wrong direction though.
>
> The IF_TYPE should really go away and be replaced with the UCLASS ID, I think.
>
> At present we have lots of calls to blk_create_device_f() which
> specify the type. I think we should drop the IF_TYPE_.. arg to that
> function and have it figured out from the uclass, in the interim.
>
> But why do we need IF_TYPE now?
I admit that this patch is just an intermediate step.
We can replace IF_TYPE by ULASS ID once SPL block devices are always
using the driver model. Have we reached this point yet? I have not seen
drivers/block/blk_legacy.c being deleted.
Removing if_type_uclass_id[] is anyway on the right path.
Best regards
Heinrich
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] blk: simplify blk_get_devnum_by_typename()
2021-10-25 7:54 ` Heinrich Schuchardt
@ 2021-10-25 8:00 ` Heinrich Schuchardt
2021-10-25 15:18 ` Simon Glass
0 siblings, 1 reply; 14+ messages in thread
From: Heinrich Schuchardt @ 2021-10-25 8:00 UTC (permalink / raw)
To: Simon Glass; +Cc: Patrick Delaunay, Ilias Apalodimas, U-Boot Mailing List
On 10/25/21 09:54, Heinrich Schuchardt wrote:
> On 10/24/21 21:54, Simon Glass wrote:
>> Hi Heinrich,
>>
>> On Sat, 23 Oct 2021 at 08:06, Heinrich Schuchardt
>> <heinrich.schuchardt@canonical.com> wrote:
>>>
>>> The block descriptor contains the if_type. There is no need to first
>>> look
>>> up the uclass for the if_type and then to check the parent device's
>>> uclass
>>> to know if the device has the correct if_type.
>>>
>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>> ---
>>> drivers/block/blk-uclass.c | 35 +----------------------------------
>>> 1 file changed, 1 insertion(+), 34 deletions(-)
>>
>> This seems to be heading in the wrong direction though.
>>
>> The IF_TYPE should really go away and be replaced with the UCLASS ID,
>> I think.
>>
>> At present we have lots of calls to blk_create_device_f() which
>> specify the type. I think we should drop the IF_TYPE_.. arg to that
>> function and have it figured out from the uclass, in the interim.
>>
>> But why do we need IF_TYPE now?
>
> I admit that this patch is just an intermediate step.
>
> We can replace IF_TYPE by ULASS ID once SPL block devices are always
> using the driver model. Have we reached this point yet? I have not seen
> drivers/block/blk_legacy.c being deleted.
qemu_malta64, qemu_malta64el, qemu_malta, qemu_maltael are examples of
defconfigs requiring drivers/block/blk_legacy.c
Best regards
Heinrich
>
> Removing if_type_uclass_id[] is anyway on the right path.
>
> Best regards
>
> Heinrich
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] blk: simplify blk_get_devnum_by_typename()
2021-10-25 8:00 ` Heinrich Schuchardt
@ 2021-10-25 15:18 ` Simon Glass
2021-10-25 15:44 ` Heinrich Schuchardt
2021-10-25 15:46 ` Tom Rini
0 siblings, 2 replies; 14+ messages in thread
From: Simon Glass @ 2021-10-25 15:18 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Patrick Delaunay, Ilias Apalodimas, U-Boot Mailing List, Tom Rini
Hi Heinrich,
On Mon, 25 Oct 2021 at 02:00, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 10/25/21 09:54, Heinrich Schuchardt wrote:
> > On 10/24/21 21:54, Simon Glass wrote:
> >> Hi Heinrich,
> >>
> >> On Sat, 23 Oct 2021 at 08:06, Heinrich Schuchardt
> >> <heinrich.schuchardt@canonical.com> wrote:
> >>>
> >>> The block descriptor contains the if_type. There is no need to first
> >>> look
> >>> up the uclass for the if_type and then to check the parent device's
> >>> uclass
> >>> to know if the device has the correct if_type.
> >>>
> >>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>> ---
> >>> drivers/block/blk-uclass.c | 35 +----------------------------------
> >>> 1 file changed, 1 insertion(+), 34 deletions(-)
> >>
> >> This seems to be heading in the wrong direction though.
> >>
> >> The IF_TYPE should really go away and be replaced with the UCLASS ID,
> >> I think.
> >>
> >> At present we have lots of calls to blk_create_device_f() which
> >> specify the type. I think we should drop the IF_TYPE_.. arg to that
> >> function and have it figured out from the uclass, in the interim.
> >>
> >> But why do we need IF_TYPE now?
> >
> > I admit that this patch is just an intermediate step.
> >
> > We can replace IF_TYPE by ULASS ID once SPL block devices are always
> > using the driver model. Have we reached this point yet? I have not seen
> > drivers/block/blk_legacy.c being deleted.
>
> qemu_malta64, qemu_malta64el, qemu_malta, qemu_maltael are examples of
> defconfigs requiring drivers/block/blk_legacy.c
Yes, we seem to have BLK on only for MMC and USB, but malta64 (for
example) uses IDE.
The problem seems to be HAVE_BLOCK_DEVICE which should not be used.
+Tom Rini
Should we remove HAVE_BLOCK_DEVICE at this point, or make it depend on BROKEN?
>
> Best regards
>
> Heinrich
>
> >
> > Removing if_type_uclass_id[] is anyway on the right path.
Are you sure? Instead, could we use the uclass ID in more places?
Regards,
Simon
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] blk: simplify blk_get_devnum_by_typename()
2021-10-25 15:18 ` Simon Glass
@ 2021-10-25 15:44 ` Heinrich Schuchardt
2021-10-25 17:29 ` Simon Glass
2021-10-25 15:46 ` Tom Rini
1 sibling, 1 reply; 14+ messages in thread
From: Heinrich Schuchardt @ 2021-10-25 15:44 UTC (permalink / raw)
To: Simon Glass
Cc: Patrick Delaunay, Ilias Apalodimas, U-Boot Mailing List, Tom Rini
On 10/25/21 17:18, Simon Glass wrote:
> Hi Heinrich,
>
> On Mon, 25 Oct 2021 at 02:00, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> On 10/25/21 09:54, Heinrich Schuchardt wrote:
>>> On 10/24/21 21:54, Simon Glass wrote:
>>>> Hi Heinrich,
>>>>
>>>> On Sat, 23 Oct 2021 at 08:06, Heinrich Schuchardt
>>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>>
>>>>> The block descriptor contains the if_type. There is no need to first
>>>>> look
>>>>> up the uclass for the if_type and then to check the parent device's
>>>>> uclass
>>>>> to know if the device has the correct if_type.
>>>>>
>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>>> ---
>>>>> drivers/block/blk-uclass.c | 35 +----------------------------------
>>>>> 1 file changed, 1 insertion(+), 34 deletions(-)
>>>>
>>>> This seems to be heading in the wrong direction though.
>>>>
>>>> The IF_TYPE should really go away and be replaced with the UCLASS ID,
>>>> I think.
>>>>
>>>> At present we have lots of calls to blk_create_device_f() which
>>>> specify the type. I think we should drop the IF_TYPE_.. arg to that
>>>> function and have it figured out from the uclass, in the interim.
>>>>
>>>> But why do we need IF_TYPE now?
>>>
>>> I admit that this patch is just an intermediate step.
>>>
>>> We can replace IF_TYPE by ULASS ID once SPL block devices are always
>>> using the driver model. Have we reached this point yet? I have not seen
>>> drivers/block/blk_legacy.c being deleted.
>>
>> qemu_malta64, qemu_malta64el, qemu_malta, qemu_maltael are examples of
>> defconfigs requiring drivers/block/blk_legacy.c
>
> Yes, we seem to have BLK on only for MMC and USB, but malta64 (for
> example) uses IDE.
>
> The problem seems to be HAVE_BLOCK_DEVICE which should not be used.
>
> +Tom Rini
>
> Should we remove HAVE_BLOCK_DEVICE at this point, or make it depend on BROKEN?
>
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> Removing if_type_uclass_id[] is anyway on the right path.
>
> Are you sure? Instead, could we use the uclass ID in more places?
Yes, you want to get rid of if_type. This means the table will become
obsolete.
Once the legacy drivers are removed we can replace all occurrences of
if_type by uclass_id. That uclass_id we should not take from blk_desc
but from udevice.
Best regards
Heinrich
>
> Regards,
> Simon
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] blk: simplify blk_get_devnum_by_typename()
2021-10-25 15:44 ` Heinrich Schuchardt
@ 2021-10-25 17:29 ` Simon Glass
2021-10-25 18:41 ` Heinrich Schuchardt
0 siblings, 1 reply; 14+ messages in thread
From: Simon Glass @ 2021-10-25 17:29 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Patrick Delaunay, Ilias Apalodimas, U-Boot Mailing List, Tom Rini
Hi Heinrich,
On Mon, 25 Oct 2021 at 09:44, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 10/25/21 17:18, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, 25 Oct 2021 at 02:00, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> On 10/25/21 09:54, Heinrich Schuchardt wrote:
> >>> On 10/24/21 21:54, Simon Glass wrote:
> >>>> Hi Heinrich,
> >>>>
> >>>> On Sat, 23 Oct 2021 at 08:06, Heinrich Schuchardt
> >>>> <heinrich.schuchardt@canonical.com> wrote:
> >>>>>
> >>>>> The block descriptor contains the if_type. There is no need to first
> >>>>> look
> >>>>> up the uclass for the if_type and then to check the parent device's
> >>>>> uclass
> >>>>> to know if the device has the correct if_type.
> >>>>>
> >>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>>>> ---
> >>>>> drivers/block/blk-uclass.c | 35 +----------------------------------
> >>>>> 1 file changed, 1 insertion(+), 34 deletions(-)
> >>>>
> >>>> This seems to be heading in the wrong direction though.
> >>>>
> >>>> The IF_TYPE should really go away and be replaced with the UCLASS ID,
> >>>> I think.
> >>>>
> >>>> At present we have lots of calls to blk_create_device_f() which
> >>>> specify the type. I think we should drop the IF_TYPE_.. arg to that
> >>>> function and have it figured out from the uclass, in the interim.
> >>>>
> >>>> But why do we need IF_TYPE now?
> >>>
> >>> I admit that this patch is just an intermediate step.
> >>>
> >>> We can replace IF_TYPE by ULASS ID once SPL block devices are always
> >>> using the driver model. Have we reached this point yet? I have not seen
> >>> drivers/block/blk_legacy.c being deleted.
> >>
> >> qemu_malta64, qemu_malta64el, qemu_malta, qemu_maltael are examples of
> >> defconfigs requiring drivers/block/blk_legacy.c
> >
> > Yes, we seem to have BLK on only for MMC and USB, but malta64 (for
> > example) uses IDE.
> >
> > The problem seems to be HAVE_BLOCK_DEVICE which should not be used.
> >
> > +Tom Rini
> >
> > Should we remove HAVE_BLOCK_DEVICE at this point, or make it depend on BROKEN?
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>> Removing if_type_uclass_id[] is anyway on the right path.
> >
> > Are you sure? Instead, could we use the uclass ID in more places?
>
> Yes, you want to get rid of if_type. This means the table will become
> obsolete.
Yes.
> Once the legacy drivers are removed we can replace all occurrences of
> if_type by uclass_id. That uclass_id we should not take from blk_desc
> but from udevice.
How do we get from a blk_desc to a udevice, though?
Could you instead look at moving from using blk_desc to using udevice?
If we had that, I think I can see your point with this patch. At
present, it looks like a backwards step because you are reducing the
usage of the uclass and in fact removing it altogether.
Can you see what I am getting at? Let's move towards migration
incrementally, not destroy the bridges we have built towards the goal.
Regards,
Simon
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] blk: simplify blk_get_devnum_by_typename()
2021-10-25 17:29 ` Simon Glass
@ 2021-10-25 18:41 ` Heinrich Schuchardt
2021-10-25 19:03 ` Simon Glass
0 siblings, 1 reply; 14+ messages in thread
From: Heinrich Schuchardt @ 2021-10-25 18:41 UTC (permalink / raw)
To: Simon Glass
Cc: Patrick Delaunay, Ilias Apalodimas, U-Boot Mailing List, Tom Rini
On 10/25/21 19:29, Simon Glass wrote:
> Hi Heinrich,
>
> On Mon, 25 Oct 2021 at 09:44, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> On 10/25/21 17:18, Simon Glass wrote:
>>> Hi Heinrich,
>>>
>>> On Mon, 25 Oct 2021 at 02:00, Heinrich Schuchardt
>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>
>>>> On 10/25/21 09:54, Heinrich Schuchardt wrote:
>>>>> On 10/24/21 21:54, Simon Glass wrote:
>>>>>> Hi Heinrich,
>>>>>>
>>>>>> On Sat, 23 Oct 2021 at 08:06, Heinrich Schuchardt
>>>>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>>>>
>>>>>>> The block descriptor contains the if_type. There is no need to first
>>>>>>> look
>>>>>>> up the uclass for the if_type and then to check the parent device's
>>>>>>> uclass
>>>>>>> to know if the device has the correct if_type.
>>>>>>>
>>>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>>>>> ---
>>>>>>> drivers/block/blk-uclass.c | 35 +----------------------------------
>>>>>>> 1 file changed, 1 insertion(+), 34 deletions(-)
>>>>>>
>>>>>> This seems to be heading in the wrong direction though.
>>>>>>
>>>>>> The IF_TYPE should really go away and be replaced with the UCLASS ID,
>>>>>> I think.
>>>>>>
>>>>>> At present we have lots of calls to blk_create_device_f() which
>>>>>> specify the type. I think we should drop the IF_TYPE_.. arg to that
>>>>>> function and have it figured out from the uclass, in the interim.
>>>>>>
>>>>>> But why do we need IF_TYPE now?
>>>>>
>>>>> I admit that this patch is just an intermediate step.
>>>>>
>>>>> We can replace IF_TYPE by ULASS ID once SPL block devices are always
>>>>> using the driver model. Have we reached this point yet? I have not seen
>>>>> drivers/block/blk_legacy.c being deleted.
>>>>
>>>> qemu_malta64, qemu_malta64el, qemu_malta, qemu_maltael are examples of
>>>> defconfigs requiring drivers/block/blk_legacy.c
>>>
>>> Yes, we seem to have BLK on only for MMC and USB, but malta64 (for
>>> example) uses IDE.
>>>
>>> The problem seems to be HAVE_BLOCK_DEVICE which should not be used.
>>>
>>> +Tom Rini
>>>
>>> Should we remove HAVE_BLOCK_DEVICE at this point, or make it depend on BROKEN?
>>>
>>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>>
>>>>> Removing if_type_uclass_id[] is anyway on the right path.
>>>
>>> Are you sure? Instead, could we use the uclass ID in more places?
>>
>> Yes, you want to get rid of if_type. This means the table will become
>> obsolete.
>
> Yes.
>
>> Once the legacy drivers are removed we can replace all occurrences of
>> if_type by uclass_id. That uclass_id we should not take from blk_desc
>> but from udevice.
>
> How do we get from a blk_desc to a udevice, though?
>
> Could you instead look at moving from using blk_desc to using udevice?
> If we had that, I think I can see your point with this patch. At
> present, it looks like a backwards step because you are reducing the
> usage of the uclass and in fact removing it altogether.
>
> Can you see what I am getting at? Let's move towards migration
> incrementally, not destroy the bridges we have built towards the goal.
Once you move from if_type to uclass_id you will simply replace
if (desc->if_type != if_type)
by
if (device_get_uclass_id(dev->uclass->uclass_id) != uclass_id)
Except for this line this patch brings you to the final code.
Best regards
Heinrich
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] blk: simplify blk_get_devnum_by_typename()
2021-10-25 18:41 ` Heinrich Schuchardt
@ 2021-10-25 19:03 ` Simon Glass
2021-10-25 19:37 ` Heinrich Schuchardt
0 siblings, 1 reply; 14+ messages in thread
From: Simon Glass @ 2021-10-25 19:03 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Patrick Delaunay, Ilias Apalodimas, U-Boot Mailing List, Tom Rini
Hi Heinrich,
On Mon, 25 Oct 2021 at 12:41, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 10/25/21 19:29, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, 25 Oct 2021 at 09:44, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> On 10/25/21 17:18, Simon Glass wrote:
> >>> Hi Heinrich,
> >>>
> >>> On Mon, 25 Oct 2021 at 02:00, Heinrich Schuchardt
> >>> <heinrich.schuchardt@canonical.com> wrote:
> >>>>
> >>>> On 10/25/21 09:54, Heinrich Schuchardt wrote:
> >>>>> On 10/24/21 21:54, Simon Glass wrote:
> >>>>>> Hi Heinrich,
> >>>>>>
> >>>>>> On Sat, 23 Oct 2021 at 08:06, Heinrich Schuchardt
> >>>>>> <heinrich.schuchardt@canonical.com> wrote:
> >>>>>>>
> >>>>>>> The block descriptor contains the if_type. There is no need to first
> >>>>>>> look
> >>>>>>> up the uclass for the if_type and then to check the parent device's
> >>>>>>> uclass
> >>>>>>> to know if the device has the correct if_type.
> >>>>>>>
> >>>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>>>>>> ---
> >>>>>>> drivers/block/blk-uclass.c | 35 +----------------------------------
> >>>>>>> 1 file changed, 1 insertion(+), 34 deletions(-)
> >>>>>>
> >>>>>> This seems to be heading in the wrong direction though.
> >>>>>>
> >>>>>> The IF_TYPE should really go away and be replaced with the UCLASS ID,
> >>>>>> I think.
> >>>>>>
> >>>>>> At present we have lots of calls to blk_create_device_f() which
> >>>>>> specify the type. I think we should drop the IF_TYPE_.. arg to that
> >>>>>> function and have it figured out from the uclass, in the interim.
> >>>>>>
> >>>>>> But why do we need IF_TYPE now?
> >>>>>
> >>>>> I admit that this patch is just an intermediate step.
> >>>>>
> >>>>> We can replace IF_TYPE by ULASS ID once SPL block devices are always
> >>>>> using the driver model. Have we reached this point yet? I have not seen
> >>>>> drivers/block/blk_legacy.c being deleted.
> >>>>
> >>>> qemu_malta64, qemu_malta64el, qemu_malta, qemu_maltael are examples of
> >>>> defconfigs requiring drivers/block/blk_legacy.c
> >>>
> >>> Yes, we seem to have BLK on only for MMC and USB, but malta64 (for
> >>> example) uses IDE.
> >>>
> >>> The problem seems to be HAVE_BLOCK_DEVICE which should not be used.
> >>>
> >>> +Tom Rini
> >>>
> >>> Should we remove HAVE_BLOCK_DEVICE at this point, or make it depend on BROKEN?
> >>>
> >>>>
> >>>> Best regards
> >>>>
> >>>> Heinrich
> >>>>
> >>>>>
> >>>>> Removing if_type_uclass_id[] is anyway on the right path.
> >>>
> >>> Are you sure? Instead, could we use the uclass ID in more places?
> >>
> >> Yes, you want to get rid of if_type. This means the table will become
> >> obsolete.
> >
> > Yes.
> >
> >> Once the legacy drivers are removed we can replace all occurrences of
> >> if_type by uclass_id. That uclass_id we should not take from blk_desc
> >> but from udevice.
> >
> > How do we get from a blk_desc to a udevice, though?
> >
> > Could you instead look at moving from using blk_desc to using udevice?
> > If we had that, I think I can see your point with this patch. At
> > present, it looks like a backwards step because you are reducing the
> > usage of the uclass and in fact removing it altogether.
> >
> > Can you see what I am getting at? Let's move towards migration
> > incrementally, not destroy the bridges we have built towards the goal.
>
> Once you move from if_type to uclass_id you will simply replace
>
> if (desc->if_type != if_type)
>
> by
>
> if (device_get_uclass_id(dev->uclass->uclass_id) != uclass_id)
>
> Except for this line this patch brings you to the final code.
device_get_uclass_id() BTW
But where does uclass_id come from?! You are removing the function
that produces it!
How about, instead, you update blk_create_devicef() to drop the
if_type parameter and just use the device's uclass? That would
actually head forwards towards migration, rather than away from it.
Regards,
Simon
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] blk: simplify blk_get_devnum_by_typename()
2021-10-25 19:03 ` Simon Glass
@ 2021-10-25 19:37 ` Heinrich Schuchardt
2021-10-25 19:45 ` Heinrich Schuchardt
2021-10-25 19:46 ` Simon Glass
0 siblings, 2 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2021-10-25 19:37 UTC (permalink / raw)
To: Simon Glass
Cc: Patrick Delaunay, Ilias Apalodimas, U-Boot Mailing List, Tom Rini
On 10/25/21 21:03, Simon Glass wrote:
> Hi Heinrich,
>
> On Mon, 25 Oct 2021 at 12:41, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>>
>>
>> On 10/25/21 19:29, Simon Glass wrote:
>>> Hi Heinrich,
>>>
>>> On Mon, 25 Oct 2021 at 09:44, Heinrich Schuchardt
>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>
>>>> On 10/25/21 17:18, Simon Glass wrote:
>>>>> Hi Heinrich,
>>>>>
>>>>> On Mon, 25 Oct 2021 at 02:00, Heinrich Schuchardt
>>>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>>>
>>>>>> On 10/25/21 09:54, Heinrich Schuchardt wrote:
>>>>>>> On 10/24/21 21:54, Simon Glass wrote:
>>>>>>>> Hi Heinrich,
>>>>>>>>
>>>>>>>> On Sat, 23 Oct 2021 at 08:06, Heinrich Schuchardt
>>>>>>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>>>>>>
>>>>>>>>> The block descriptor contains the if_type. There is no need to first
>>>>>>>>> look
>>>>>>>>> up the uclass for the if_type and then to check the parent device's
>>>>>>>>> uclass
>>>>>>>>> to know if the device has the correct if_type.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>>>>>>> ---
>>>>>>>>> drivers/block/blk-uclass.c | 35 +----------------------------------
>>>>>>>>> 1 file changed, 1 insertion(+), 34 deletions(-)
>>>>>>>>
>>>>>>>> This seems to be heading in the wrong direction though.
>>>>>>>>
>>>>>>>> The IF_TYPE should really go away and be replaced with the UCLASS ID,
>>>>>>>> I think.
>>>>>>>>
>>>>>>>> At present we have lots of calls to blk_create_device_f() which
>>>>>>>> specify the type. I think we should drop the IF_TYPE_.. arg to that
>>>>>>>> function and have it figured out from the uclass, in the interim.
>>>>>>>>
>>>>>>>> But why do we need IF_TYPE now?
>>>>>>>
>>>>>>> I admit that this patch is just an intermediate step.
>>>>>>>
>>>>>>> We can replace IF_TYPE by ULASS ID once SPL block devices are always
>>>>>>> using the driver model. Have we reached this point yet? I have not seen
>>>>>>> drivers/block/blk_legacy.c being deleted.
>>>>>>
>>>>>> qemu_malta64, qemu_malta64el, qemu_malta, qemu_maltael are examples of
>>>>>> defconfigs requiring drivers/block/blk_legacy.c
>>>>>
>>>>> Yes, we seem to have BLK on only for MMC and USB, but malta64 (for
>>>>> example) uses IDE.
>>>>>
>>>>> The problem seems to be HAVE_BLOCK_DEVICE which should not be used.
>>>>>
>>>>> +Tom Rini
>>>>>
>>>>> Should we remove HAVE_BLOCK_DEVICE at this point, or make it depend on BROKEN?
>>>>>
>>>>>>
>>>>>> Best regards
>>>>>>
>>>>>> Heinrich
>>>>>>
>>>>>>>
>>>>>>> Removing if_type_uclass_id[] is anyway on the right path.
>>>>>
>>>>> Are you sure? Instead, could we use the uclass ID in more places?
>>>>
>>>> Yes, you want to get rid of if_type. This means the table will become
>>>> obsolete.
>>>
>>> Yes.
>>>
>>>> Once the legacy drivers are removed we can replace all occurrences of
>>>> if_type by uclass_id. That uclass_id we should not take from blk_desc
>>>> but from udevice.
>>>
>>> How do we get from a blk_desc to a udevice, though?
>>>
>>> Could you instead look at moving from using blk_desc to using udevice?
>>> If we had that, I think I can see your point with this patch. At
>>> present, it looks like a backwards step because you are reducing the
>>> usage of the uclass and in fact removing it altogether.
>>>
>>> Can you see what I am getting at? Let's move towards migration
>>> incrementally, not destroy the bridges we have built towards the goal.
>>
>> Once you move from if_type to uclass_id you will simply replace
>>
>> if (desc->if_type != if_type)
>>
>> by
>>
>> if (device_get_uclass_id(dev->uclass->uclass_id) != uclass_id)
>>
>> Except for this line this patch brings you to the final code.
>
> device_get_uclass_id() BTW
>
> But where does uclass_id come from?! You are removing the function
> that produces it!
In future you will pass uclass_id as parameter instead of if_type
because if_type will be replaced by uclass_id!
>
> How about, instead, you update blk_create_devicef() to drop the
> if_type parameter and just use the device's uclass? That would
> actually head forwards towards migration, rather than away from it.
>
> Regards,
> Simon
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] blk: simplify blk_get_devnum_by_typename()
2021-10-25 19:37 ` Heinrich Schuchardt
@ 2021-10-25 19:45 ` Heinrich Schuchardt
2021-10-25 19:46 ` Simon Glass
1 sibling, 0 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2021-10-25 19:45 UTC (permalink / raw)
To: Simon Glass
Cc: Patrick Delaunay, Ilias Apalodimas, U-Boot Mailing List, Tom Rini
On 10/25/21 21:37, Heinrich Schuchardt wrote:
>
>
> On 10/25/21 21:03, Simon Glass wrote:
>> Hi Heinrich,
>>
>> On Mon, 25 Oct 2021 at 12:41, Heinrich Schuchardt
>> <heinrich.schuchardt@canonical.com> wrote:
>>>
>>>
>>>
>>> On 10/25/21 19:29, Simon Glass wrote:
>>>> Hi Heinrich,
>>>>
>>>> On Mon, 25 Oct 2021 at 09:44, Heinrich Schuchardt
>>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>>
>>>>> On 10/25/21 17:18, Simon Glass wrote:
>>>>>> Hi Heinrich,
>>>>>>
>>>>>> On Mon, 25 Oct 2021 at 02:00, Heinrich Schuchardt
>>>>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>>>>
>>>>>>> On 10/25/21 09:54, Heinrich Schuchardt wrote:
>>>>>>>> On 10/24/21 21:54, Simon Glass wrote:
>>>>>>>>> Hi Heinrich,
>>>>>>>>>
>>>>>>>>> On Sat, 23 Oct 2021 at 08:06, Heinrich Schuchardt
>>>>>>>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>>>>>>>
>>>>>>>>>> The block descriptor contains the if_type. There is no need to
>>>>>>>>>> first
>>>>>>>>>> look
>>>>>>>>>> up the uclass for the if_type and then to check the parent
>>>>>>>>>> device's
>>>>>>>>>> uclass
>>>>>>>>>> to know if the device has the correct if_type.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Heinrich Schuchardt
>>>>>>>>>> <heinrich.schuchardt@canonical.com>
>>>>>>>>>> ---
>>>>>>>>>> drivers/block/blk-uclass.c | 35
>>>>>>>>>> +----------------------------------
>>>>>>>>>> 1 file changed, 1 insertion(+), 34 deletions(-)
>>>>>>>>>
>>>>>>>>> This seems to be heading in the wrong direction though.
>>>>>>>>>
>>>>>>>>> The IF_TYPE should really go away and be replaced with the
>>>>>>>>> UCLASS ID,
>>>>>>>>> I think.
>>>>>>>>>
>>>>>>>>> At present we have lots of calls to blk_create_device_f() which
>>>>>>>>> specify the type. I think we should drop the IF_TYPE_.. arg to
>>>>>>>>> that
>>>>>>>>> function and have it figured out from the uclass, in the interim.
>>>>>>>>>
>>>>>>>>> But why do we need IF_TYPE now?
>>>>>>>>
>>>>>>>> I admit that this patch is just an intermediate step.
>>>>>>>>
>>>>>>>> We can replace IF_TYPE by ULASS ID once SPL block devices are
>>>>>>>> always
>>>>>>>> using the driver model. Have we reached this point yet? I have
>>>>>>>> not seen
>>>>>>>> drivers/block/blk_legacy.c being deleted.
>>>>>>>
>>>>>>> qemu_malta64, qemu_malta64el, qemu_malta, qemu_maltael are
>>>>>>> examples of
>>>>>>> defconfigs requiring drivers/block/blk_legacy.c
>>>>>>
>>>>>> Yes, we seem to have BLK on only for MMC and USB, but malta64 (for
>>>>>> example) uses IDE.
>>>>>>
>>>>>> The problem seems to be HAVE_BLOCK_DEVICE which should not be used.
>>>>>>
>>>>>> +Tom Rini
>>>>>>
>>>>>> Should we remove HAVE_BLOCK_DEVICE at this point, or make it
>>>>>> depend on BROKEN?
>>>>>>
>>>>>>>
>>>>>>> Best regards
>>>>>>>
>>>>>>> Heinrich
>>>>>>>
>>>>>>>>
>>>>>>>> Removing if_type_uclass_id[] is anyway on the right path.
>>>>>>
>>>>>> Are you sure? Instead, could we use the uclass ID in more places?
>>>>>
>>>>> Yes, you want to get rid of if_type. This means the table will become
>>>>> obsolete.
>>>>
>>>> Yes.
>>>>
>>>>> Once the legacy drivers are removed we can replace all occurrences of
>>>>> if_type by uclass_id. That uclass_id we should not take from blk_desc
>>>>> but from udevice.
>>>>
>>>> How do we get from a blk_desc to a udevice, though?
>>>>
>>>> Could you instead look at moving from using blk_desc to using udevice?
>>>> If we had that, I think I can see your point with this patch. At
>>>> present, it looks like a backwards step because you are reducing the
>>>> usage of the uclass and in fact removing it altogether.
>>>>
>>>> Can you see what I am getting at? Let's move towards migration
>>>> incrementally, not destroy the bridges we have built towards the goal.
>>>
>>> Once you move from if_type to uclass_id you will simply replace
>>>
>>> if (desc->if_type != if_type)
>>>
>>> by
>>>
>>> if (device_get_uclass_id(dev->uclass->uclass_id) != uclass_id)
>>>
>>> Except for this line this patch brings you to the final code.
>>
>> device_get_uclass_id() BTW
>>
>> But where does uclass_id come from?! You are removing the function
>> that produces it!
>
> In future you will pass uclass_id as parameter instead of if_type
> because if_type will be replaced by uclass_id!
uclass_get_by_name() should be able to find the uclass_id if you pass in
an interface name.
Best regards
Heinrich
>
>>
>> How about, instead, you update blk_create_devicef() to drop the
>> if_type parameter and just use the device's uclass? That would
>> actually head forwards towards migration, rather than away from it.
>>
>> Regards,
>> Simon
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] blk: simplify blk_get_devnum_by_typename()
2021-10-25 19:37 ` Heinrich Schuchardt
2021-10-25 19:45 ` Heinrich Schuchardt
@ 2021-10-25 19:46 ` Simon Glass
2021-11-13 9:05 ` Heinrich Schuchardt
1 sibling, 1 reply; 14+ messages in thread
From: Simon Glass @ 2021-10-25 19:46 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Patrick Delaunay, Ilias Apalodimas, U-Boot Mailing List, Tom Rini
Hi Heinrich,
On Mon, 25 Oct 2021 at 13:37, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 10/25/21 21:03, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, 25 Oct 2021 at 12:41, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >>
> >>
> >> On 10/25/21 19:29, Simon Glass wrote:
> >>> Hi Heinrich,
> >>>
> >>> On Mon, 25 Oct 2021 at 09:44, Heinrich Schuchardt
> >>> <heinrich.schuchardt@canonical.com> wrote:
> >>>>
> >>>> On 10/25/21 17:18, Simon Glass wrote:
> >>>>> Hi Heinrich,
> >>>>>
> >>>>> On Mon, 25 Oct 2021 at 02:00, Heinrich Schuchardt
> >>>>> <heinrich.schuchardt@canonical.com> wrote:
> >>>>>>
> >>>>>> On 10/25/21 09:54, Heinrich Schuchardt wrote:
> >>>>>>> On 10/24/21 21:54, Simon Glass wrote:
> >>>>>>>> Hi Heinrich,
> >>>>>>>>
> >>>>>>>> On Sat, 23 Oct 2021 at 08:06, Heinrich Schuchardt
> >>>>>>>> <heinrich.schuchardt@canonical.com> wrote:
> >>>>>>>>>
> >>>>>>>>> The block descriptor contains the if_type. There is no need to first
> >>>>>>>>> look
> >>>>>>>>> up the uclass for the if_type and then to check the parent device's
> >>>>>>>>> uclass
> >>>>>>>>> to know if the device has the correct if_type.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>>>>>>>> ---
> >>>>>>>>> drivers/block/blk-uclass.c | 35 +----------------------------------
> >>>>>>>>> 1 file changed, 1 insertion(+), 34 deletions(-)
> >>>>>>>>
> >>>>>>>> This seems to be heading in the wrong direction though.
> >>>>>>>>
> >>>>>>>> The IF_TYPE should really go away and be replaced with the UCLASS ID,
> >>>>>>>> I think.
> >>>>>>>>
> >>>>>>>> At present we have lots of calls to blk_create_device_f() which
> >>>>>>>> specify the type. I think we should drop the IF_TYPE_.. arg to that
> >>>>>>>> function and have it figured out from the uclass, in the interim.
> >>>>>>>>
> >>>>>>>> But why do we need IF_TYPE now?
> >>>>>>>
> >>>>>>> I admit that this patch is just an intermediate step.
> >>>>>>>
> >>>>>>> We can replace IF_TYPE by ULASS ID once SPL block devices are always
> >>>>>>> using the driver model. Have we reached this point yet? I have not seen
> >>>>>>> drivers/block/blk_legacy.c being deleted.
> >>>>>>
> >>>>>> qemu_malta64, qemu_malta64el, qemu_malta, qemu_maltael are examples of
> >>>>>> defconfigs requiring drivers/block/blk_legacy.c
> >>>>>
> >>>>> Yes, we seem to have BLK on only for MMC and USB, but malta64 (for
> >>>>> example) uses IDE.
> >>>>>
> >>>>> The problem seems to be HAVE_BLOCK_DEVICE which should not be used.
> >>>>>
> >>>>> +Tom Rini
> >>>>>
> >>>>> Should we remove HAVE_BLOCK_DEVICE at this point, or make it depend on BROKEN?
> >>>>>
> >>>>>>
> >>>>>> Best regards
> >>>>>>
> >>>>>> Heinrich
> >>>>>>
> >>>>>>>
> >>>>>>> Removing if_type_uclass_id[] is anyway on the right path.
> >>>>>
> >>>>> Are you sure? Instead, could we use the uclass ID in more places?
> >>>>
> >>>> Yes, you want to get rid of if_type. This means the table will become
> >>>> obsolete.
> >>>
> >>> Yes.
> >>>
> >>>> Once the legacy drivers are removed we can replace all occurrences of
> >>>> if_type by uclass_id. That uclass_id we should not take from blk_desc
> >>>> but from udevice.
> >>>
> >>> How do we get from a blk_desc to a udevice, though?
> >>>
> >>> Could you instead look at moving from using blk_desc to using udevice?
> >>> If we had that, I think I can see your point with this patch. At
> >>> present, it looks like a backwards step because you are reducing the
> >>> usage of the uclass and in fact removing it altogether.
> >>>
> >>> Can you see what I am getting at? Let's move towards migration
> >>> incrementally, not destroy the bridges we have built towards the goal.
> >>
> >> Once you move from if_type to uclass_id you will simply replace
> >>
> >> if (desc->if_type != if_type)
> >>
> >> by
> >>
> >> if (device_get_uclass_id(dev->uclass->uclass_id) != uclass_id)
> >>
> >> Except for this line this patch brings you to the final code.
> >
> > device_get_uclass_id() BTW
> >
> > But where does uclass_id come from?! You are removing the function
> > that produces it!
>
> In future you will pass uclass_id as parameter instead of if_type
> because if_type will be replaced by uclass_id!
Perhaps we can discuss this on the contributor call as we are not
getting anywhere.
You are removing the ability to translate between the old value and
the new value, but still using the old value in the function call. I
have already suggested how you could contribute to the migration
effort if you have time. Rather than increasing reliance on if_type,
we should be removing use of it, e.g. dropping it from blk_desc, etc.
- Simon
>
> >
> > How about, instead, you update blk_create_devicef() to drop the
> > if_type parameter and just use the device's uclass? That would
> > actually head forwards towards migration, rather than away from it.
> >
> > Regards,
> > Simon
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] blk: simplify blk_get_devnum_by_typename()
2021-10-25 19:46 ` Simon Glass
@ 2021-11-13 9:05 ` Heinrich Schuchardt
0 siblings, 0 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2021-11-13 9:05 UTC (permalink / raw)
To: Simon Glass
Cc: Patrick Delaunay, Ilias Apalodimas, U-Boot Mailing List, Tom Rini
On 10/25/21 21:46, Simon Glass wrote:
> Hi Heinrich,
>
> On Mon, 25 Oct 2021 at 13:37, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>>
>>
>> On 10/25/21 21:03, Simon Glass wrote:
>>> Hi Heinrich,
>>>
>>> On Mon, 25 Oct 2021 at 12:41, Heinrich Schuchardt
>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>
>>>>
>>>>
>>>> On 10/25/21 19:29, Simon Glass wrote:
>>>>> Hi Heinrich,
>>>>>
>>>>> On Mon, 25 Oct 2021 at 09:44, Heinrich Schuchardt
>>>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>>>
>>>>>> On 10/25/21 17:18, Simon Glass wrote:
>>>>>>> Hi Heinrich,
>>>>>>>
>>>>>>> On Mon, 25 Oct 2021 at 02:00, Heinrich Schuchardt
>>>>>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>>>>>
>>>>>>>> On 10/25/21 09:54, Heinrich Schuchardt wrote:
>>>>>>>>> On 10/24/21 21:54, Simon Glass wrote:
>>>>>>>>>> Hi Heinrich,
>>>>>>>>>>
>>>>>>>>>> On Sat, 23 Oct 2021 at 08:06, Heinrich Schuchardt
>>>>>>>>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> The block descriptor contains the if_type. There is no need to first
>>>>>>>>>>> look
>>>>>>>>>>> up the uclass for the if_type and then to check the parent device's
>>>>>>>>>>> uclass
>>>>>>>>>>> to know if the device has the correct if_type.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>>>>>>>>> ---
>>>>>>>>>>> drivers/block/blk-uclass.c | 35 +----------------------------------
>>>>>>>>>>> 1 file changed, 1 insertion(+), 34 deletions(-)
>>>>>>>>>>
>>>>>>>>>> This seems to be heading in the wrong direction though.
>>>>>>>>>>
>>>>>>>>>> The IF_TYPE should really go away and be replaced with the UCLASS ID,
>>>>>>>>>> I think.
>>>>>>>>>>
>>>>>>>>>> At present we have lots of calls to blk_create_device_f() which
>>>>>>>>>> specify the type. I think we should drop the IF_TYPE_.. arg to that
>>>>>>>>>> function and have it figured out from the uclass, in the interim.
>>>>>>>>>>
>>>>>>>>>> But why do we need IF_TYPE now?
>>>>>>>>>
>>>>>>>>> I admit that this patch is just an intermediate step.
>>>>>>>>>
>>>>>>>>> We can replace IF_TYPE by ULASS ID once SPL block devices are always
>>>>>>>>> using the driver model. Have we reached this point yet? I have not seen
>>>>>>>>> drivers/block/blk_legacy.c being deleted.
>>>>>>>>
>>>>>>>> qemu_malta64, qemu_malta64el, qemu_malta, qemu_maltael are examples of
>>>>>>>> defconfigs requiring drivers/block/blk_legacy.c
>>>>>>>
>>>>>>> Yes, we seem to have BLK on only for MMC and USB, but malta64 (for
>>>>>>> example) uses IDE.
>>>>>>>
>>>>>>> The problem seems to be HAVE_BLOCK_DEVICE which should not be used.
>>>>>>>
>>>>>>> +Tom Rini
>>>>>>>
>>>>>>> Should we remove HAVE_BLOCK_DEVICE at this point, or make it depend on BROKEN?
>>>>>>>
>>>>>>>>
>>>>>>>> Best regards
>>>>>>>>
>>>>>>>> Heinrich
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Removing if_type_uclass_id[] is anyway on the right path.
>>>>>>>
>>>>>>> Are you sure? Instead, could we use the uclass ID in more places?
>>>>>>
>>>>>> Yes, you want to get rid of if_type. This means the table will become
>>>>>> obsolete.
>>>>>
>>>>> Yes.
>>>>>
>>>>>> Once the legacy drivers are removed we can replace all occurrences of
>>>>>> if_type by uclass_id. That uclass_id we should not take from blk_desc
>>>>>> but from udevice.
>>>>>
>>>>> How do we get from a blk_desc to a udevice, though?
>>>>>
>>>>> Could you instead look at moving from using blk_desc to using udevice?
>>>>> If we had that, I think I can see your point with this patch. At
>>>>> present, it looks like a backwards step because you are reducing the
>>>>> usage of the uclass and in fact removing it altogether.
>>>>>
>>>>> Can you see what I am getting at? Let's move towards migration
>>>>> incrementally, not destroy the bridges we have built towards the goal.
>>>>
>>>> Once you move from if_type to uclass_id you will simply replace
>>>>
>>>> if (desc->if_type != if_type)
>>>>
>>>> by
>>>>
>>>> if (device_get_uclass_id(dev->uclass->uclass_id) != uclass_id)
>>>>
>>>> Except for this line this patch brings you to the final code.
>>>
>>> device_get_uclass_id() BTW
>>>
>>> But where does uclass_id come from?! You are removing the function
>>> that produces it!
>>
>> In future you will pass uclass_id as parameter instead of if_type
>> because if_type will be replaced by uclass_id!
>
> Perhaps we can discuss this on the contributor call as we are not
> getting anywhere.
>
> You are removing the ability to translate between the old value and
> the new value, but still using the old value in the function call. I
> have already suggested how you could contribute to the migration
> effort if you have time. Rather than increasing reliance on if_type,
> we should be removing use of it, e.g. dropping it from blk_desc, etc.
>
>
> - Simon
>
>>
>>>
>>> How about, instead, you update blk_create_devicef() to drop the
>>> if_type parameter and just use the device's uclass? That would
>>> actually head forwards towards migration, rather than away from it.
>>>
>>> Regards,
>>> Simon
>>>
Without this patch the following fails:
make sandconfig
make menuconfig
# set CONFIG_EFI_SELFTEST=y
make -j$(nproc)
./u-boot -T
setenv efi_selftest block device
bootefi selftest
ls efi 0:1
load efi 0:1 $fdt_addr_r hello.txt
Best regards
Heinrich
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] blk: simplify blk_get_devnum_by_typename()
2021-10-25 15:18 ` Simon Glass
2021-10-25 15:44 ` Heinrich Schuchardt
@ 2021-10-25 15:46 ` Tom Rini
1 sibling, 0 replies; 14+ messages in thread
From: Tom Rini @ 2021-10-25 15:46 UTC (permalink / raw)
To: Simon Glass, Daniel Schwierzeck
Cc: Heinrich Schuchardt, Patrick Delaunay, Ilias Apalodimas,
U-Boot Mailing List
[-- Attachment #1: Type: text/plain, Size: 2205 bytes --]
On Mon, Oct 25, 2021 at 09:18:49AM -0600, Simon Glass wrote:
> Hi Heinrich,
>
> On Mon, 25 Oct 2021 at 02:00, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
> >
> > On 10/25/21 09:54, Heinrich Schuchardt wrote:
> > > On 10/24/21 21:54, Simon Glass wrote:
> > >> Hi Heinrich,
> > >>
> > >> On Sat, 23 Oct 2021 at 08:06, Heinrich Schuchardt
> > >> <heinrich.schuchardt@canonical.com> wrote:
> > >>>
> > >>> The block descriptor contains the if_type. There is no need to first
> > >>> look
> > >>> up the uclass for the if_type and then to check the parent device's
> > >>> uclass
> > >>> to know if the device has the correct if_type.
> > >>>
> > >>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > >>> ---
> > >>> drivers/block/blk-uclass.c | 35 +----------------------------------
> > >>> 1 file changed, 1 insertion(+), 34 deletions(-)
> > >>
> > >> This seems to be heading in the wrong direction though.
> > >>
> > >> The IF_TYPE should really go away and be replaced with the UCLASS ID,
> > >> I think.
> > >>
> > >> At present we have lots of calls to blk_create_device_f() which
> > >> specify the type. I think we should drop the IF_TYPE_.. arg to that
> > >> function and have it figured out from the uclass, in the interim.
> > >>
> > >> But why do we need IF_TYPE now?
> > >
> > > I admit that this patch is just an intermediate step.
> > >
> > > We can replace IF_TYPE by ULASS ID once SPL block devices are always
> > > using the driver model. Have we reached this point yet? I have not seen
> > > drivers/block/blk_legacy.c being deleted.
> >
> > qemu_malta64, qemu_malta64el, qemu_malta, qemu_maltael are examples of
> > defconfigs requiring drivers/block/blk_legacy.c
>
> Yes, we seem to have BLK on only for MMC and USB, but malta64 (for
> example) uses IDE.
>
> The problem seems to be HAVE_BLOCK_DEVICE which should not be used.
>
> +Tom Rini
>
> Should we remove HAVE_BLOCK_DEVICE at this point, or make it depend on BROKEN?
Daniel, do you know what's up with malta and what needs to be migrated
to DM, possibly, at this point still, wrt storage devices? Thanks!
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-11-13 9:05 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-23 14:06 [PATCH 1/1] blk: simplify blk_get_devnum_by_typename() Heinrich Schuchardt
2021-10-24 19:54 ` Simon Glass
2021-10-25 7:54 ` Heinrich Schuchardt
2021-10-25 8:00 ` Heinrich Schuchardt
2021-10-25 15:18 ` Simon Glass
2021-10-25 15:44 ` Heinrich Schuchardt
2021-10-25 17:29 ` Simon Glass
2021-10-25 18:41 ` Heinrich Schuchardt
2021-10-25 19:03 ` Simon Glass
2021-10-25 19:37 ` Heinrich Schuchardt
2021-10-25 19:45 ` Heinrich Schuchardt
2021-10-25 19:46 ` Simon Glass
2021-11-13 9:05 ` Heinrich Schuchardt
2021-10-25 15:46 ` Tom Rini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.