On Fri, Aug 12, 2016 at 08:19:42PM +0200, Alexander Graf wrote: > > > On 12.08.16 19:20, Simon Glass wrote: > > Hi Alex, > > > > On 10 August 2016 at 13:01, Alexander Graf <ag...@suse.de> wrote: > >> > >>> On 10 Aug 2016, at 18:25, Tom Rini <tr...@konsulko.com> wrote: > >>> > >>> On Wed, Aug 10, 2016 at 03:25:16PM +0200, Alexander Graf wrote: > >>>> > >>>> > >>>>> Am 10.08.2016 um 15:16 schrieb Simon Glass <s...@chromium.org>: > >>>>> > >>>>> Hi Alex, > >>>>> > >>>>>> On 10 August 2016 at 07:02, Alexander Graf <ag...@suse.de> wrote: > >>>>>>> On 08/10/2016 02:56 PM, Simon Glass wrote: > >>>>>>> > >>>>>>> +Tom > >>>>>>> > >>>>>>> Hi Alex, > >>>>>>> > >>>>>>> On 10 August 2016 at 01:47, Alexander Graf <ag...@suse.de> wrote: > >>>>>>>>> > >>>>>>>>> On 08 Aug 2016, at 23:44, Simon Glass <s...@chromium.org> wrote: > >>>>>>>>> > >>>>>>>>> Hi Alexander, > >>>>>>>>> > >>>>>>>>>> On 5 August 2016 at 06:49, Alexander Graf <ag...@suse.de> wrote: > >>>>>>>>>> > >>>>>>>>>> When using CONFIG_BLK, there were 2 issues: > >>>>>>>>>> > >>>>>>>>>> 1) The name we generate the device with has to match the > >>>>>>>>>> name we set in efi_set_bootdev() > >>>>>>>>>> > >>>>>>>>>> 2) The device we pass into our block functions was wrong, > >>>>>>>>>> we should not rediscover it but just use the already known > >>>>>>>>>> pointer. > >>>>>>>>>> > >>>>>>>>>> This patch fixes both issues. > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Alexander Graf <ag...@suse.de> > >>>>>>>>>> --- > >>>>>>>>>> cmd/bootefi.c | 23 ++++++++++++++++++----- > >>>>>>>>>> lib/efi_loader/efi_disk.c | 18 +++++++++++------- > >>>>>>>>>> 2 files changed, 29 insertions(+), 12 deletions(-) > >>>>>>> [...] > >>>>>>> > >>>>>>>>>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > >>>>>>>>>> index c434c92..e00a747 100644 > >>>>>>>>>> --- a/lib/efi_loader/efi_disk.c > >>>>>>>>>> +++ b/lib/efi_loader/efi_disk.c > >>>>>>>>>> @@ -31,6 +31,8 @@ struct efi_disk_obj { > >>>>>>>>>> struct efi_device_path_file_path *dp; > >>>>>>>>>> /* Offset into disk for simple partitions */ > >>>>>>>>>> lbaint_t offset; > >>>>>>>>>> + /* Internal block device */ > >>>>>>>>>> + const struct blk_desc *desc; > >>>>>>>>> > >>>>>>>>> Rather than storing this, can you store the udevice? > >>>>>>>> > >>>>>>>> I could, but then I would diverge between the CONFIG_BLK and > >>>>>>>> non-CONFIG_BLK path again, which would turn the code into an #ifdef > >>>>>>>> mess > >>>>>>>> (read: hard to maintain), because the whole device creation path > >>>>>>>> relies on > >>>>>>>> struct blk_desc * today and doesn’t pass the udevice anywhere. > >>>>>>>> > >>>>>>>> Do you feel strongly about this? To give you an idea how messy it > >>>>>>>> gets, > >>>>>>>> the diff is below. > >>>>>>> > >>>>>>> Actually I'd like to make this feature depend on CONFIG_BLK. If we add > >>>>>>> new features that don't use driver model, and then use the legacy data > >>>>>>> structures such that converting to driver model becomes harder, we'll > >>>>>>> never be done. > >>>>>>> > >>>>>>> I did mention this at the beginning and it seems to have come to pass. > >>>>>>> > >>>>>>> In order of preference from my side: > >>>>>>> > >>>>>>> 1. Make EFI_LOADER depend on BLK > >>>>>> > >>>>>> > >>>>>> If we make EFI_LOADER depend on BLK, doesn't that break all systems > >>>>>> that > >>>>>> need storage that isn't converted to device model today? Like the SATA > >>>>>> breakage on Xilinx systems, just at a much bigger scale? > >>>>> > >>>>> No it just means that these platforms need to move to BLK before they > >>>>> can use the EFI loader. Given the embryonic nature of this feature, > >>>>> that seems reasonable, and the impact would be small. It will also > >>>>> encourage conversion and keep the code cleaner. > >>>> > >>>> No, it will simply make my life harder because I would have to sit > >>>> down and vonvert every single board to BLK that I need EFI enabled. > >>> > >>> Seems like as good a place as any to jump in, of the boards that you > >>> need EFI enabled on, what isn't converted today? > >> > >> I want to make EFI the default boot path in openSUSE, which means any > >> board that anyone out there wants to run openSUSE on would be on the list. > >> Anything that keeps them from running EFI on a random board is a road > >> block that I’d rather not have if I can avoid it. > > > > Of course, I fully understand that. However as mentioned in this > > patch, you are creating future problems. > > I don't see how I am creating future problems, really. Passing a > udevice* instead of the struct blk_desc* internally doesn't improve the > code really at the end of the day. > > > Can you address Tom's question? I take it that it boots on Raspberry > > Pi (which I'd like to try actually - are there instructions > > somewhere?). We can easily convert that over. Anything else? > > For a list of currently available "upstream" openSUSE images, see > https://build.opensuse.org/package/view_file/openSUSE:Factory:ARM/JeOS/pre_checkin.sh?expand=1 > > Every one of those is on the short-term list. Any other board that > people want to run on is potentially on the mid-term to long-term list.
OK, that is a lot of boards and such. And yes, I see both of these features / projects as important to the long-term health of U-Boot. So, Alex, when we've got storage converted fully to DM, you're willing to do further clean-ups to make it DM-better, yes? Thanks! -- Tom
signature.asc
Description: Digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot