[U-Boot] [PATCH 0/2] api: extend accessible set of block device attributes

[U-Boot] [PATCH 0/2] api: extend accessible set of block device attributes

[Che-Liang Chiou]

struct device_info in api_public.h defined its own subset of attributes
of block_dev_desc, which limits the capability of external apps.

This patch set let external apps access the same set of block device
attributes as U-Boot.

Che-Liang Chiou (2):
  Flatten and solidify block_dev_desc layout
  api: storage: Share attributes with block_dev_desc

 api/api_storage.c        |   17 +++++++++++++++--
 disk/part_dos.c          |    2 +-
 disk/part_efi.c          |    4 +---
 drivers/mmc/mmc.c        |    4 ++--
 drivers/mmc/pxa_mmc.c    |    2 +-
 examples/api/demo.c      |   15 +++++++++++++--
 include/api_public.h     |    8 +-------
 include/block_dev_attr.h |   39 +++++++++++++++++++++++++++++++++++++++
 include/part.h           |   18 +++---------------
 9 files changed, 76 insertions(+), 33 deletions(-)
 create mode 100644 include/block_dev_attr.h

-- 
1.7.3.1

[U-Boot] [PATCH 1/2] Flatten and solidify block_dev_desc layout

[Che-Liang Chiou]

The block_dev_desc struct has #ifdef on lba48 and variable-size on lba
and so its layout varies from config to config.  At least part_efi.c has
partially complained about this.

This patch makes lba48 be always defined and lba be fixed to largest
size that an LBA would need so that the block_dev_desc layout would be
an invariant with respect to configurations.

Doing so would waste a few extra bytes per struct block_dev_desc, which
I believe is not critical.

Signed-off-by: Che-Liang Chiou <clchiou@chromium.org>
---
 disk/part_dos.c       |    2 +-
 disk/part_efi.c       |    4 +---
 drivers/mmc/mmc.c     |    4 ++--
 drivers/mmc/pxa_mmc.c |    2 +-
 include/part.h        |    4 +---
 5 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/disk/part_dos.c b/disk/part_dos.c
index b5bcb37..a0938db 100644
--- a/disk/part_dos.c
+++ b/disk/part_dos.c
@@ -119,7 +119,7 @@ static void print_partition_extended (block_dev_desc_t *dev_desc, int ext_part_s
 		return;
 	}
 	if(i==DOS_PBR) {
-		printf ("    1\t\t         0\t%10ld\t%2x\n",
+		printf ("    1\t\t         0\t%10lld\t%2x\n",
 			dev_desc->lba, buffer[DOS_PBR_MEDIA_TYPE_OFFSET]);
 		return;
 	}
diff --git a/disk/part_efi.c b/disk/part_efi.c
index 0a513c6..e779dc1 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -22,10 +22,8 @@
  */
 
 /*
- * Problems with CONFIG_SYS_64BIT_LBA:
- *
  * struct disk_partition.start in include/part.h is sized as ulong.
- * When CONFIG_SYS_64BIT_LBA is activated, lbaint_t changes from ulong to uint64_t.
+ * struct block_dev_desc.lba in the same header is sized as uint64_t.
  * For now, it is cast back to ulong at assignment.
  *
  * This limits the maximum size of addressable storage to < 2 Terra Bytes
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 391bc2b..c17e495 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -265,7 +265,7 @@ mmc_write_blocks(struct mmc *mmc, ulong start, lbaint_t blkcnt, const void*src)
 	int timeout = 1000;
 
 	if ((start + blkcnt) > mmc->block_dev.lba) {
-		printf("MMC: block number 0x%lx exceeds max(0x%lx)\n",
+		printf("MMC: block number 0x%lx exceeds max(0x%llx)\n",
 			start + blkcnt, mmc->block_dev.lba);
 		return 0;
 	}
@@ -393,7 +393,7 @@ static ulong mmc_bread(int dev_num, ulong start, lbaint_t blkcnt, void *dst)
 		return 0;
 
 	if ((start + blkcnt) > mmc->block_dev.lba) {
-		printf("MMC: block number 0x%lx exceeds max(0x%lx)\n",
+		printf("MMC: block number 0x%lx exceeds max(0x%llx)\n",
 			start + blkcnt, mmc->block_dev.lba);
 		return 0;
 	}
diff --git a/drivers/mmc/pxa_mmc.c b/drivers/mmc/pxa_mmc.c
index 48e21ef..67c33d4 100644
--- a/drivers/mmc/pxa_mmc.c
+++ b/drivers/mmc/pxa_mmc.c
@@ -541,7 +541,7 @@ static void mmc_decode_csd(uint32_t * resp)
 	mmc_dev.removable = 0;
 	mmc_dev.block_read = mmc_bread;
 
-	printf("Detected: %lu blocks of %lu bytes (%luMB) ",
+	printf("Detected: %llu blocks of %lu bytes (%lluMB) ",
 		mmc_dev.lba,
 		mmc_dev.blksz,
 		mmc_dev.lba * mmc_dev.blksz / (1024 * 1024));
diff --git a/include/part.h b/include/part.h
index 1827767..be0a22e 100644
--- a/include/part.h
+++ b/include/part.h
@@ -33,10 +33,8 @@ typedef struct block_dev_desc {
 	unsigned char	lun;		/* target LUN */
 	unsigned char	type;		/* device type */
 	unsigned char	removable;	/* removable device */
-#ifdef CONFIG_LBA48
 	unsigned char	lba48;		/* device can use 48bit addr (ATA/ATAPI v7) */
-#endif
-	lbaint_t		lba;		/* number of blocks */
+	uint64_t	lba;		/* number of blocks */
 	unsigned long	blksz;		/* block size */
 	char		vendor [40+1];	/* IDE model, SCSI Vendor */
 	char		product[20+1];	/* IDE Serial no, SCSI product */
-- 
1.7.3.1

[U-Boot] [PATCH 2/2] api: storage: Share attributes with block_dev_desc

[Che-Liang Chiou]

struct device_info in api_public.h defined its own subset of attributes
of block_dev_desc, which limits the capability of external apps.

This patch let struct device_info and block_dev_desc share the same set
of attributes so that an external app has equal amount of information of
block device compared to U-Boot.

The export.h and _export.h have somewhat addressed the same issue. That
is, sharing declarations between U-Boot and external apps.

Signed-off-by: Che-Liang Chiou <clchiou@chromium.org>
---
 api/api_storage.c        |   17 +++++++++++++++--
 examples/api/demo.c      |   15 +++++++++++++--
 include/api_public.h     |    8 +-------
 include/block_dev_attr.h |   39 +++++++++++++++++++++++++++++++++++++++
 include/part.h           |   16 +++-------------
 5 files changed, 71 insertions(+), 24 deletions(-)
 create mode 100644 include/block_dev_attr.h

diff --git a/api/api_storage.c b/api/api_storage.c
index c535712..ab4cad5 100644
--- a/api/api_storage.c
+++ b/api/api_storage.c
@@ -165,8 +165,21 @@ static int dev_stor_get(int type, int first, int *more, struct device_info *di)
 				debugf("device instance exists, but is not active..");
 				found = 0;
 			} else {
-				di->di_stor.block_count = dd->lba;
-				di->di_stor.block_size = dd->blksz;
+#define COPY_ATTR(a) di->di_stor.a = dd->a
+				COPY_ATTR(if_type);
+				COPY_ATTR(dev);
+				COPY_ATTR(part_type);
+				COPY_ATTR(target);
+				COPY_ATTR(lun);
+				COPY_ATTR(type);
+				COPY_ATTR(removable);
+				COPY_ATTR(lba48);
+				COPY_ATTR(lba);
+				COPY_ATTR(blksz);
+#undef COPY_ATTR
+				strcpy(di->di_stor.vendor, dd->vendor);
+				strcpy(di->di_stor.product, dd->product);
+				strcpy(di->di_stor.revision, dd->revision);
 			}
 		}
 
diff --git a/examples/api/demo.c b/examples/api/demo.c
index 65e7491..0c65ae9 100644
--- a/examples/api/demo.c
+++ b/examples/api/demo.c
@@ -294,7 +294,18 @@ void test_dump_di(int handle)
 
 	} else if (di->type & DEV_TYP_STOR) {
 		printf("  type\t\t= %s\n", test_stor_typ(di->type));
-		printf("  blk size\t\t= %d\n", (unsigned int)di->di_stor.block_size);
-		printf("  blk count\t\t= %d\n", (unsigned int)di->di_stor.block_count);
+		printf("  if_type\t\t= %d\n", di->di_stor.if_type);
+		printf("  dev\t\t= %d\n", di->di_stor.dev);
+		printf("  part_type\t\t= %d\n", di->di_stor.part_type);
+		printf("  target\t\t= %d\n", di->di_stor.target);
+		printf("  lun\t\t= %d\n", di->di_stor.lun);
+		printf("  device type\t\t= %d\n", di->di_stor.type);
+		printf("  removable\t\t= %d\n", di->di_stor.removable);
+		printf("  lba48\t\t= %d\n", di->di_stor.lba48);
+		printf("  blk size\t\t= %d\n", (unsigned int)di->di_stor.blksz);
+		printf("  blk count\t\t= %d\n", (unsigned int)di->di_stor.lba);
+		printf("  vendor\t\t= %s\n", di->di_stor.vendor);
+		printf("  product\t\t= %s\n", di->di_stor.product);
+		printf("  revision\t\t= %s\n", di->di_stor.revision);
 	}
 }
diff --git a/include/api_public.h b/include/api_public.h
index 5940d81..245904f 100644
--- a/include/api_public.h
+++ b/include/api_public.h
@@ -111,12 +111,7 @@ struct sys_info {
 	int			mr_no;	/* number of memory regions */
 };
 
-#undef CONFIG_SYS_64BIT_LBA
-#ifdef CONFIG_SYS_64BIT_LBA
-typedef	u_int64_t lbasize_t;
-#else
 typedef unsigned long lbasize_t;
-#endif
 typedef unsigned long lbastart_t;
 
 #define DEV_TYP_NONE	0x0000
@@ -138,8 +133,7 @@ struct device_info {
 
 	union {
 		struct {
-			lbasize_t	block_count;	/* no of blocks */
-			unsigned long	block_size;	/* size of one block */
+			#include "block_dev_attr.h"
 		} storage;
 
 		struct {
diff --git a/include/block_dev_attr.h b/include/block_dev_attr.h
new file mode 100644
index 0000000..07a76d8
--- /dev/null
+++ b/include/block_dev_attr.h
@@ -0,0 +1,39 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+/*
+ * These are attributes for struct like block_dev_desc.  Include this header
+ * inside a struct definition to populate that struct with these attributes.
+ */
+
+int		if_type;	/* type of the interface */
+int		dev;		/* device number */
+unsigned char	part_type;	/* partition type */
+unsigned char	target;		/* target SCSI ID */
+unsigned char	lun;		/* target LUN */
+unsigned char	type;		/* device type */
+unsigned char	removable;	/* removable device */
+unsigned char	lba48;		/* device can use 48bit addr (ATA/ATAPI v7) */
+uint64_t	lba;		/* number of blocks */
+unsigned long	blksz;		/* block size */
+char		vendor[40+1];	/* IDE model, SCSI Vendor */
+char		product[20+1];	/* IDE Serial no, SCSI product */
+char		revision[8+1];	/* firmware revision */
diff --git a/include/part.h b/include/part.h
index be0a22e..2d2b599 100644
--- a/include/part.h
+++ b/include/part.h
@@ -26,19 +26,9 @@
 #include <ide.h>
 
 typedef struct block_dev_desc {
-	int		if_type;	/* type of the interface */
-	int		dev;		/* device number */
-	unsigned char	part_type;	/* partition type */
-	unsigned char	target;		/* target SCSI ID */
-	unsigned char	lun;		/* target LUN */
-	unsigned char	type;		/* device type */
-	unsigned char	removable;	/* removable device */
-	unsigned char	lba48;		/* device can use 48bit addr (ATA/ATAPI v7) */
-	uint64_t	lba;		/* number of blocks */
-	unsigned long	blksz;		/* block size */
-	char		vendor [40+1];	/* IDE model, SCSI Vendor */
-	char		product[20+1];	/* IDE Serial no, SCSI product */
-	char		revision[8+1];	/* firmware revision */
+
+	#include "block_dev_attr.h"
+
 	unsigned long	(*block_read)(int dev,
 				      unsigned long start,
 				      lbaint_t blkcnt,
-- 
1.7.3.1

[U-Boot] [PATCH 1/2] Flatten and solidify block_dev_desc layout

[Che-Liang Chiou]

The block_dev_desc struct has #ifdef on lba48 and variable-size on lba
and so its layout varies from config to config.  At least part_efi.c has
partially complained about this.

This patch makes lba48 be always defined and lba be fixed to largest
size that an LBA would need so that the block_dev_desc layout would be
an invariant with respect to configurations.

Doing so would waste a few extra bytes per struct block_dev_desc, which
I believe is not critical.

Signed-off-by: Che-Liang Chiou <clchiou@chromium.org>
---

Fix a minor checkpatch violation.

 disk/part_dos.c       |    2 +-
 disk/part_efi.c       |    4 +---
 drivers/mmc/mmc.c     |    4 ++--
 drivers/mmc/pxa_mmc.c |    2 +-
 include/part.h        |    4 +---
 5 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/disk/part_dos.c b/disk/part_dos.c
index b5bcb37..a0938db 100644
--- a/disk/part_dos.c
+++ b/disk/part_dos.c
@@ -119,7 +119,7 @@ static void print_partition_extended (block_dev_desc_t *dev_desc, int ext_part_s
 		return;
 	}
 	if(i==DOS_PBR) {
-		printf ("    1\t\t         0\t%10ld\t%2x\n",
+		printf("    1\t\t         0\t%10lld\t%2x\n",
 			dev_desc->lba, buffer[DOS_PBR_MEDIA_TYPE_OFFSET]);
 		return;
 	}
diff --git a/disk/part_efi.c b/disk/part_efi.c
index 0a513c6..e779dc1 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -22,10 +22,8 @@
  */
 
 /*
- * Problems with CONFIG_SYS_64BIT_LBA:
- *
  * struct disk_partition.start in include/part.h is sized as ulong.
- * When CONFIG_SYS_64BIT_LBA is activated, lbaint_t changes from ulong to uint64_t.
+ * struct block_dev_desc.lba in the same header is sized as uint64_t.
  * For now, it is cast back to ulong at assignment.
  *
  * This limits the maximum size of addressable storage to < 2 Terra Bytes
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 391bc2b..c17e495 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -265,7 +265,7 @@ mmc_write_blocks(struct mmc *mmc, ulong start, lbaint_t blkcnt, const void*src)
 	int timeout = 1000;
 
 	if ((start + blkcnt) > mmc->block_dev.lba) {
-		printf("MMC: block number 0x%lx exceeds max(0x%lx)\n",
+		printf("MMC: block number 0x%lx exceeds max(0x%llx)\n",
 			start + blkcnt, mmc->block_dev.lba);
 		return 0;
 	}
@@ -393,7 +393,7 @@ static ulong mmc_bread(int dev_num, ulong start, lbaint_t blkcnt, void *dst)
 		return 0;
 
 	if ((start + blkcnt) > mmc->block_dev.lba) {
-		printf("MMC: block number 0x%lx exceeds max(0x%lx)\n",
+		printf("MMC: block number 0x%lx exceeds max(0x%llx)\n",
 			start + blkcnt, mmc->block_dev.lba);
 		return 0;
 	}
diff --git a/drivers/mmc/pxa_mmc.c b/drivers/mmc/pxa_mmc.c
index 48e21ef..67c33d4 100644
--- a/drivers/mmc/pxa_mmc.c
+++ b/drivers/mmc/pxa_mmc.c
@@ -541,7 +541,7 @@ static void mmc_decode_csd(uint32_t * resp)
 	mmc_dev.removable = 0;
 	mmc_dev.block_read = mmc_bread;
 
-	printf("Detected: %lu blocks of %lu bytes (%luMB) ",
+	printf("Detected: %llu blocks of %lu bytes (%lluMB) ",
 		mmc_dev.lba,
 		mmc_dev.blksz,
 		mmc_dev.lba * mmc_dev.blksz / (1024 * 1024));
diff --git a/include/part.h b/include/part.h
index 1827767..be0a22e 100644
--- a/include/part.h
+++ b/include/part.h
@@ -33,10 +33,8 @@ typedef struct block_dev_desc {
 	unsigned char	lun;		/* target LUN */
 	unsigned char	type;		/* device type */
 	unsigned char	removable;	/* removable device */
-#ifdef CONFIG_LBA48
 	unsigned char	lba48;		/* device can use 48bit addr (ATA/ATAPI v7) */
-#endif
-	lbaint_t		lba;		/* number of blocks */
+	uint64_t	lba;		/* number of blocks */
 	unsigned long	blksz;		/* block size */
 	char		vendor [40+1];	/* IDE model, SCSI Vendor */
 	char		product[20+1];	/* IDE Serial no, SCSI product */
-- 
1.7.3.1

[U-Boot] [PATCH 0/2] api: extend accessible set of block device attributes

[Detlev Zundel]

Hi,

> struct device_info in api_public.h defined its own subset of attributes
> of block_dev_desc, which limits the capability of external apps.
>
> This patch set let external apps access the same set of block device
> attributes as U-Boot.

Generally speaking, we are intentionally limiting our API to external
applications.  It should be easier to extend U-Boot itself (and
everybody will profit from the new code under a free license) rather
than writing proprietary applications.

Can you pleas tell us what the intention of the extension is?  We surely
will not accept patches gradually exposing all of the U-Boot internals
through the API and paving the way for proprietary applications.  So
every extension has to be balanced and discussed.

Thanks
  Detlev

-- 
Insider comment on Microsoft releasing Linux Hyper-V driver code under GPLv2:
             "It looks like hell just froze over."
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH 1/2] Flatten and solidify block_dev_desc layout

[Wolfgang Denk]

Dear Che-Liang Chiou,

In message <1319178708-10881-2-git-send-email-clchiou@chromium.org> you wrote:
> The block_dev_desc struct has #ifdef on lba48 and variable-size on lba
> and so its layout varies from config to config.  At least part_efi.c has
> partially complained about this.
> 
> This patch makes lba48 be always defined and lba be fixed to largest
> size that an LBA would need so that the block_dev_desc layout would be
> an invariant with respect to configurations.
> 
> Doing so would waste a few extra bytes per struct block_dev_desc, which
> I believe is not critical.

How much exactly is "a few bytes"?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
As long as we're going to reinvent the wheel again, we might as  well
try making it round this time.                        - Mike Dennison

[U-Boot] [PATCH 0/2] api: extend accessible set of block device attributes

[Che-liang Chiou]

Hi Detlev,

Oops, I did not know it is intentionally to keep the external apps API
as it is now.

I am working on an open source secure bootloader based on U-Boot.
Mostly I wrote boot logic (by boot logic I mean prompting user and
listing available devices sort of things). If you see U-Boot as a
platform, you can think of my project as an application running on top
of U-Boot, except that my application is statically linked with
U-Boot.

The boot logic is running on ARM and x86. Through the project I have
learned that certain APIs would be helpful, such as enumerating all
available block device and drawing bitmaps on screen regardless of
which driver (video or LCD) you are using.

It was probably a misleading finding when I searched the code base: I
saw only the external apps API implemented an API for enumerating
available block device. So I thought improving the external apps API
was the right place to go.

Alternatively (if not go the external apps API), we could have like
common/cmd_enum_block_dev.c that does what I am planning to do, and I
am happy to do this. What do you think?

Regards,
Che-Liang

On Sat, Oct 22, 2011 at 12:06 AM, Detlev Zundel <dzu@denx.de> wrote:
> Hi,
>
>> struct device_info in api_public.h defined its own subset of attributes
>> of block_dev_desc, which limits the capability of external apps.
>>
>> This patch set let external apps access the same set of block device
>> attributes as U-Boot.
>
> Generally speaking, we are intentionally limiting our API to external
> applications. ?It should be easier to extend U-Boot itself (and
> everybody will profit from the new code under a free license) rather
> than writing proprietary applications.
>
> Can you pleas tell us what the intention of the extension is? ?We surely
> will not accept patches gradually exposing all of the U-Boot internals
> through the API and paving the way for proprietary applications. ?So
> every extension has to be balanced and discussed.
>
> Thanks
> ?Detlev
>
> --
> Insider comment on Microsoft releasing Linux Hyper-V driver code under GPLv2:
> ? ? ? ? ? ? "It looks like hell just froze over."
> --
> DENX Software Engineering GmbH, ? ? ?MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, ?Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de
>

[U-Boot] [PATCH 1/2] Flatten and solidify block_dev_desc layout

[Che-liang Chiou]

Dear Wolfgang Denk,

I guess I have to put this patchset on hold. I will get you back if we
could proceed with this patchset.

Regards,
Che-Liang

On Sat, Oct 22, 2011 at 3:09 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Che-Liang Chiou,
>
> In message <1319178708-10881-2-git-send-email-clchiou@chromium.org> you wrote:
>> The block_dev_desc struct has #ifdef on lba48 and variable-size on lba
>> and so its layout varies from config to config. ?At least part_efi.c has
>> partially complained about this.
>>
>> This patch makes lba48 be always defined and lba be fixed to largest
>> size that an LBA would need so that the block_dev_desc layout would be
>> an invariant with respect to configurations.
>>
>> Doing so would waste a few extra bytes per struct block_dev_desc, which
>> I believe is not critical.
>
> How much exactly is "a few bytes"?
>
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> As long as we're going to reinvent the wheel again, we might as ?well
> try making it round this time. ? ? ? ? ? ? ? ? ? ? ? ?- Mike Dennison
>

[U-Boot] [PATCH 0/2] api: extend accessible set of block device attributes

[Detlev Zundel]

Hi Che-liang,

> I am working on an open source secure bootloader based on U-Boot.
> Mostly I wrote boot logic (by boot logic I mean prompting user and
> listing available devices sort of things). If you see U-Boot as a
> platform, you can think of my project as an application running on top
> of U-Boot, except that my application is statically linked with
> U-Boot.

Usually the (old or new) API is used to separate the application from
the GPLed U-Boot codebase to allow for proprietary applications.  If you
plan to link your application statically to U-Boot anyway, then I think
that using the (external) API is not a good way to move forward.

> The boot logic is running on ARM and x86. Through the project I have
> learned that certain APIs would be helpful, such as enumerating all
> available block device and drawing bitmaps on screen regardless of
> which driver (video or LCD) you are using.

Yes, I think what you actually are looking for is a proper device model
in U-Boot.  Actually we are in dire need for something like this in the
whole of U-Boots codebase and we talk about it for a very long time (see
for example [1]).  This would ease progress in multiple
areas, where currently we hack around this.  See for example the
SERIAL_MULTI code base or NET_MULTI.  Both are isolated solutions for
the same problem.  As you note, video and block devices will also
profit - USB will be another instance.

> It was probably a misleading finding when I searched the code base: I
> saw only the external apps API implemented an API for enumerating
> available block device. So I thought improving the external apps API
> was the right place to go.
>
> Alternatively (if not go the external apps API), we could have like
> common/cmd_enum_block_dev.c that does what I am planning to do, and I
> am happy to do this. What do you think?

I think we should aim at a pretty generic device interface.  Currently I
would think that this also needs to fit into the "configure U-Boot
through the device tree" topic.  The one will profit from the other.

As far as I know, Marek (on CC) is working towards this device tree
model also.  Coordinating the efforts will be a good thing.

Thanks!
  Detlev

-- 
Perfecting oneself is as much unlearning as it is learning. 
                                        -- Edsger Dijkstra 
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH 1/2] Flatten and solidify block_dev_desc layout

[Detlev Zundel]

Hi Che-liang,

> I guess I have to put this patchset on hold. I will get you back if we
> could proceed with this patchset.

Please don't top-post.  The mails really are more difficult to read in
context.

> Regards,
> Che-Liang
>
> On Sat, Oct 22, 2011 at 3:09 AM, Wolfgang Denk <wd@denx.de> wrote:
>> Dear Che-Liang Chiou,
>>
>> In message <1319178708-10881-2-git-send-email-clchiou@chromium.org> you wrote:
>>> The block_dev_desc struct has #ifdef on lba48 and variable-size on lba
>>> and so its layout varies from config to config. ?At least part_efi.c has
>>> partially complained about this.
>>>
>>> This patch makes lba48 be always defined and lba be fixed to largest
>>> size that an LBA would need so that the block_dev_desc layout would be
>>> an invariant with respect to configurations.
>>>
>>> Doing so would waste a few extra bytes per struct block_dev_desc, which
>>> I believe is not critical.
>>
>> How much exactly is "a few bytes"?

As far as I can see, the difference is 4 bytes _and_ it is a runtime
data structure, so it should not make any difference for the code size.
Che-liang can surely correct me if I'm wrong.

Moreover it seems we need to do something comparable sooner or later.
If we want to support large block devices and the partition code uses
block devices, that code needs to be prepared to work with that. So in
general I'm in favor of doing something like this.

On the other hand, the patch changes the datatype of a field which gets
used in lots of places - Che-liang, did you run a MAKEALL with this
patch and check that no more warnings/errors are introduced?

Cheers
  Detlev

-- 
The latest code  looks a bit similar to the old  [linux] big-reader-locks  hack
(which got dropped for good many eons ago and with which i deny any involvement
with, such as having authored it. [oh, did i say that out loud? crap.]), imple-
mented cleanly and properly.     -- Ingo Molnar <20090428124033.GA1655@elte.hu>
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH 2/2] api: storage: Share attributes with block_dev_desc

[Mike Frysinger]

On Friday 21 October 2011 02:31:48 Che-Liang Chiou wrote:
> struct device_info in api_public.h defined its own subset of attributes
> of block_dev_desc, which limits the capability of external apps.
> 
> This patch let struct device_info and block_dev_desc share the same set
> of attributes so that an external app has equal amount of information of
> block device compared to U-Boot.
> 
> The export.h and _export.h have somewhat addressed the same issue. That
> is, sharing declarations between U-Boot and external apps.

NAK: any changes to api_public.h that is not backwards ABI compatible must 
increment API_SIG_VERSION.  you're changing the exported struct which is an 
ABI breakage.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120108/b565350d/attachment.pgp>