| Message ID | 20211023140647.7661-1-heinrich.schuchardt@canonical.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [1/1] blk: simplify blk_get_devnum_by_typename() | expand |
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
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
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
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
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 >
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!
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
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
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
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 >
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 >>
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 > >
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
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;
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(-)