Hi Alex,
On 10 August 2016 at 07:41, Alexander Graf agraf@suse.de wrote:
On 10 Aug 2016, at 15:33, Simon Glass sjg@chromium.org wrote:
Hi Alex,
On 10 August 2016 at 07:25, Alexander Graf agraf@suse.de wrote:
Am 10.08.2016 um 15:16 schrieb Simon Glass sjg@chromium.org:
Hi Alex,
On 10 August 2016 at 07:02, Alexander Graf agraf@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 agraf@suse.de wrote: >> >> On 08 Aug 2016, at 23:44, Simon Glass sjg@chromium.org wrote: >> >> Hi Alexander, >> >>> On 5 August 2016 at 06:49, Alexander Graf agraf@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 agraf@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:
- 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.
Yes that's right. But it is mostly just a simple case of enabling the option. For a few boards there might be an MMC driver that needs converting, but we can approach those one by one.
That approach sounds terribly wrong tbh. If it’s so simple, why not just mark a flag day where CONFIG_BLK becomes mandatory and fix all fallout? Then you’ll have much more help at your disposal than a few percent of my overloaded days ;).
We all have plenty of things to do. I can't see how we can do a flag day. Things need to convert bit by bit as people want the new features that it enables. We can't just turn off all the non-complying boards.
If we add new features based on legacy code, there is no incentive for people to switch. I had the same discussion on SPI flash. it really won't save you are lot of time (you are overestimating the impact), and it's much better for U-Boot as a whole.
Regards, Simon