mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH 1/3] ARM: at91: microchip-ksz9477-evb: support PBL console
@ 2021-06-02 10:25 Ahmad Fatoum
  2021-06-02 10:25 ` [PATCH 2/3] ARM: at91: mmc-xload: allow overriding card capacity Ahmad Fatoum
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Ahmad Fatoum @ 2021-06-02 10:25 UTC (permalink / raw)
  To: barebox; +Cc: mgr, ore, Ahmad Fatoum

First stage PBL does FAT file system accesses, so there is more that
could go wrong compared with second stage PBL that just does extraction.

Enable PBL console to get some error messages out if things don't work
out.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/boards/microchip-ksz9477-evb/lowlevel.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boards/microchip-ksz9477-evb/lowlevel.c b/arch/arm/boards/microchip-ksz9477-evb/lowlevel.c
index 93ae4819750c..7c5a69af05de 100644
--- a/arch/arm/boards/microchip-ksz9477-evb/lowlevel.c
+++ b/arch/arm/boards/microchip-ksz9477-evb/lowlevel.c
@@ -41,8 +41,9 @@ SAMA5_ENTRY_FUNCTION(start_sama5d3_xplained_ung8071_xload_mmc, r4)
 
 	sama5d3_udelay_init(MASTER_CLOCK);
 	sama5d3_xplained_ddrconf();
-	if (IS_ENABLED(CONFIG_DEBUG_LL))
-		dbgu_init();
+
+	dbgu_init();
+	pbl_set_putc(at91_dbgu_putc, IOMEM(AT91_BASE_DBGU1));
 
 	sama5d3_atmci_start_image(0, MASTER_CLOCK, 0);
 }
-- 
2.29.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/3] ARM: at91: mmc-xload: allow overriding card capacity
  2021-06-02 10:25 [PATCH 1/3] ARM: at91: microchip-ksz9477-evb: support PBL console Ahmad Fatoum
@ 2021-06-02 10:25 ` Ahmad Fatoum
  2021-06-02 10:30   ` Ahmad Fatoum
  2021-06-03  5:34   ` Alexander Dahl
  2021-06-02 10:25 ` [PATCH 3/3] ARM: at91: xload-mmc: add prominent note about PBL MMC limitation Ahmad Fatoum
  2021-06-02 10:29 ` [PATCH 1/3] ARM: at91: microchip-ksz9477-evb: support PBL console Ahmad Fatoum
  2 siblings, 2 replies; 9+ messages in thread
From: Ahmad Fatoum @ 2021-06-02 10:25 UTC (permalink / raw)
  To: barebox; +Cc: mgr, ore, Ahmad Fatoum

The PBL MMC driver works with the assumption that the BootROM has left
the SD-Card in transfer mode. There seems to be no definitive way
to find out whether a running card is high capacity (> 2G) or not,
but we need this info when reading, because default capacities accept
their read offset in bytes while high capacity deal in 512 byte blocks.

For i.MX, this is elegantly solved by just reading from sector 0.
For AT91, we chainload barebox from FAT, so we need to seek around.
So far, we just had an assumption buried in the driver that we are
always highcapacity.

Export a knob to change this, so we can move the hardcoding from
driver to board code, where it's more prominent. This is done
in a follow-up commit.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/mach-at91/include/mach/xload.h | 3 +++
 drivers/mci/atmel-sdhci-pbl.c           | 8 +++++++-
 drivers/mci/atmel_mci_pbl.c             | 8 +++++++-
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-at91/include/mach/xload.h b/arch/arm/mach-at91/include/mach/xload.h
index bbc70af2108a..038f32554568 100644
--- a/arch/arm/mach-at91/include/mach/xload.h
+++ b/arch/arm/mach-at91/include/mach/xload.h
@@ -12,4 +12,7 @@ int at91_sdhci_bio_init(struct pbl_bio *bio, void __iomem *base);
 int at91_mci_bio_init(struct pbl_bio *bio, void __iomem *base,
 		      unsigned int clock, unsigned int slot);
 
+void at91_mci_set_highcapacity(struct pbl_bio *bio, bool highcap);
+void at91_sdhci_set_highcapacity(struct pbl_bio *bio, bool highcap);
+
 #endif /* __MACH_XLOAD_H */
diff --git a/drivers/mci/atmel-sdhci-pbl.c b/drivers/mci/atmel-sdhci-pbl.c
index 626e4008fe85..317f26f8af0d 100644
--- a/drivers/mci/atmel-sdhci-pbl.c
+++ b/drivers/mci/atmel-sdhci-pbl.c
@@ -99,6 +99,12 @@ static int at91_sdhci_bio_read(struct pbl_bio *bio, off_t start,
 	return blocks_done;
 }
 
+void at91_sdhci_set_highcapacity(struct pbl_bio *bio, bool highcap)
+{
+	struct at91_sdhci_priv *priv = bio->priv;
+	priv->highcapacity_card = highcap;
+}
+
 static struct at91_sdhci_priv atmel_sdcard;
 
 int at91_sdhci_bio_init(struct pbl_bio *bio, void __iomem *base)
@@ -122,7 +128,7 @@ int at91_sdhci_bio_init(struct pbl_bio *bio, void __iomem *base)
 	ret = at91_sdhci_set_ios(host, &ios);
 
 	 // FIXME can we determine this without leaving SD transfer mode?
-	priv->highcapacity_card = 1;
+	at91_sdhci_set_highcapacity(bio, true);
 
 	return 0;
 }
diff --git a/drivers/mci/atmel_mci_pbl.c b/drivers/mci/atmel_mci_pbl.c
index 767d6f3ce2d7..82e45fd4e89e 100644
--- a/drivers/mci/atmel_mci_pbl.c
+++ b/drivers/mci/atmel_mci_pbl.c
@@ -82,6 +82,12 @@ static int at91_mci_bio_read(struct pbl_bio *bio, off_t start,
 	return blocks_done;
 }
 
+void at91_mci_set_highcapacity(struct pbl_bio *bio, bool highcap)
+{
+	struct atmel_mci_priv *priv = bio->priv;
+	priv->highcapacity_card = highcap;
+}
+
 int at91_mci_bio_init(struct pbl_bio *bio, void __iomem *base,
 		      unsigned int clock, unsigned int slot)
 {
@@ -110,7 +116,7 @@ int at91_mci_bio_init(struct pbl_bio *bio, void __iomem *base,
 
 	atmci_common_set_ios(host, &ios);
 
-	priv->highcapacity_card = 1;
+	at91_mci_set_highcapacity(bio, true);
 
 	return 0;
 }
-- 
2.29.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 3/3] ARM: at91: xload-mmc: add prominent note about PBL MMC limitation
  2021-06-02 10:25 [PATCH 1/3] ARM: at91: microchip-ksz9477-evb: support PBL console Ahmad Fatoum
  2021-06-02 10:25 ` [PATCH 2/3] ARM: at91: mmc-xload: allow overriding card capacity Ahmad Fatoum
@ 2021-06-02 10:25 ` Ahmad Fatoum
  2021-06-02 10:29 ` [PATCH 1/3] ARM: at91: microchip-ksz9477-evb: support PBL console Ahmad Fatoum
  2 siblings, 0 replies; 9+ messages in thread
From: Ahmad Fatoum @ 2021-06-02 10:25 UTC (permalink / raw)
  To: barebox; +Cc: mgr, ore, Ahmad Fatoum

The PBL driver has an assumption about only being used with
high capacity cards. When using a default capacity card, the sama5d3
MCI controller hang when doing unaligned accesses.

This heuristic could add some support default capacity card support
without risking unaligned accesses:

  at91_mci_set_highcapacity(false);

  sector0 = read_sector(0);
  if (!is_mbr(sector0, &fatsect)
         abort();

  fatsector1 = read_sector(fatsect);
  at91_mci_set_highcapacity(false);

  if (is_fat(fatsect))
	goto default_cap;
  else
	goto high_cap;

This, of course, fails if fatsect * 512 on a high capacity card
happens to have a FAT signature. As it's unclear whether supporting
<= 2 GiB is worth the effort, hardcode it in xload-mmc.c and add
a note about the limitation to conserve future debugging effort.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/mach-at91/xload-mmc.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-at91/xload-mmc.c b/arch/arm/mach-at91/xload-mmc.c
index 1b641f3a47ac..89766cf6b910 100644
--- a/arch/arm/mach-at91/xload-mmc.c
+++ b/arch/arm/mach-at91/xload-mmc.c
@@ -76,8 +76,18 @@ void __noreturn sama5d2_sdhci_start_image(u32 r4)
 	if (ret)
 		goto out_panic;
 
-	/* TODO: eMMC boot partition handling: they are not FAT-formatted */
+	/*
+	 * There seems to be no definitive way to find out whether we are
+	 * connected to a default or high capacity card without resetting
+	 * the card. The PBL driver assumes however that the card is already
+	 * in transfer mode. For now assume all cards to be high capacity.
+	 * If support for cards smaller or equal to 2GiB becomes relevant,
+	 * this assumption can be revisited.
+	 */
+	pr_debug("Assuming high capacity card\n");
+	at91_sdhci_set_highcapacity(&bio, true);
 
+	/* TODO: eMMC boot partition handling: they are not FAT-formatted */
 	at91_fat_start_image(&bio, buf, SZ_16M, r4);
 
 out_panic:
@@ -128,6 +138,18 @@ void __noreturn sama5d3_atmci_start_image(u32 boot_src, unsigned int clock,
 	if (ret)
 		goto out_panic;
 
+	/*
+	 * There seems to be no definitive way to find out whether we are
+	 * connected to a default or high capacity card without resetting
+	 * the card. The PBL driver assumes however that the card is already
+	 * in transfer mode. For now assume all cards to be high capacity.
+	 * If support for cards smaller or equal to 2GiB becomes relevant,
+	 * this assumption can be revisited.
+	 */
+	pr_debug("Assuming high capacity card\n");
+	at91_mci_set_highcapacity(&bio, true);
+
+	/* TODO: eMMC boot partition handling: they are not FAT-formatted */
 	at91_fat_start_image(&bio, buf, SZ_16M, boot_src);
 
 out_panic:
-- 
2.29.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] ARM: at91: microchip-ksz9477-evb: support PBL console
  2021-06-02 10:25 [PATCH 1/3] ARM: at91: microchip-ksz9477-evb: support PBL console Ahmad Fatoum
  2021-06-02 10:25 ` [PATCH 2/3] ARM: at91: mmc-xload: allow overriding card capacity Ahmad Fatoum
  2021-06-02 10:25 ` [PATCH 3/3] ARM: at91: xload-mmc: add prominent note about PBL MMC limitation Ahmad Fatoum
@ 2021-06-02 10:29 ` Ahmad Fatoum
  2 siblings, 0 replies; 9+ messages in thread
From: Ahmad Fatoum @ 2021-06-02 10:29 UTC (permalink / raw)
  To: Oleksij Rempel; +Cc: mgr, barebox

Hello Oleksij,

On 02.06.21 12:25, Ahmad Fatoum wrote:
> First stage PBL does FAT file system accesses, so there is more that
> could go wrong compared with second stage PBL that just does extraction.
> 
> Enable PBL console to get some error messages out if things don't work
> out.

I don't have the HW handy to test this. Would be great if you could
boot this up once to see nothing broke. Thanks!

> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  arch/arm/boards/microchip-ksz9477-evb/lowlevel.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boards/microchip-ksz9477-evb/lowlevel.c b/arch/arm/boards/microchip-ksz9477-evb/lowlevel.c
> index 93ae4819750c..7c5a69af05de 100644
> --- a/arch/arm/boards/microchip-ksz9477-evb/lowlevel.c
> +++ b/arch/arm/boards/microchip-ksz9477-evb/lowlevel.c
> @@ -41,8 +41,9 @@ SAMA5_ENTRY_FUNCTION(start_sama5d3_xplained_ung8071_xload_mmc, r4)
>  
>  	sama5d3_udelay_init(MASTER_CLOCK);
>  	sama5d3_xplained_ddrconf();
> -	if (IS_ENABLED(CONFIG_DEBUG_LL))
> -		dbgu_init();
> +
> +	dbgu_init();
> +	pbl_set_putc(at91_dbgu_putc, IOMEM(AT91_BASE_DBGU1));
>  
>  	sama5d3_atmci_start_image(0, MASTER_CLOCK, 0);
>  }
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] ARM: at91: mmc-xload: allow overriding card capacity
  2021-06-02 10:25 ` [PATCH 2/3] ARM: at91: mmc-xload: allow overriding card capacity Ahmad Fatoum
@ 2021-06-02 10:30   ` Ahmad Fatoum
  2021-06-03  5:34   ` Alexander Dahl
  1 sibling, 0 replies; 9+ messages in thread
From: Ahmad Fatoum @ 2021-06-02 10:30 UTC (permalink / raw)
  To: barebox; +Cc: mgr, ore

I see now that I botched, the title "mmc-xload" instead of "xload-mmc".
@Sascha, please tell me if I should resend.

On 02.06.21 12:25, Ahmad Fatoum wrote:
> The PBL MMC driver works with the assumption that the BootROM has left
> the SD-Card in transfer mode. There seems to be no definitive way
> to find out whether a running card is high capacity (> 2G) or not,
> but we need this info when reading, because default capacities accept
> their read offset in bytes while high capacity deal in 512 byte blocks.
> 
> For i.MX, this is elegantly solved by just reading from sector 0.
> For AT91, we chainload barebox from FAT, so we need to seek around.
> So far, we just had an assumption buried in the driver that we are
> always highcapacity.
> 
> Export a knob to change this, so we can move the hardcoding from
> driver to board code, where it's more prominent. This is done
> in a follow-up commit.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  arch/arm/mach-at91/include/mach/xload.h | 3 +++
>  drivers/mci/atmel-sdhci-pbl.c           | 8 +++++++-
>  drivers/mci/atmel_mci_pbl.c             | 8 +++++++-
>  3 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/include/mach/xload.h b/arch/arm/mach-at91/include/mach/xload.h
> index bbc70af2108a..038f32554568 100644
> --- a/arch/arm/mach-at91/include/mach/xload.h
> +++ b/arch/arm/mach-at91/include/mach/xload.h
> @@ -12,4 +12,7 @@ int at91_sdhci_bio_init(struct pbl_bio *bio, void __iomem *base);
>  int at91_mci_bio_init(struct pbl_bio *bio, void __iomem *base,
>  		      unsigned int clock, unsigned int slot);
>  
> +void at91_mci_set_highcapacity(struct pbl_bio *bio, bool highcap);
> +void at91_sdhci_set_highcapacity(struct pbl_bio *bio, bool highcap);
> +
>  #endif /* __MACH_XLOAD_H */
> diff --git a/drivers/mci/atmel-sdhci-pbl.c b/drivers/mci/atmel-sdhci-pbl.c
> index 626e4008fe85..317f26f8af0d 100644
> --- a/drivers/mci/atmel-sdhci-pbl.c
> +++ b/drivers/mci/atmel-sdhci-pbl.c
> @@ -99,6 +99,12 @@ static int at91_sdhci_bio_read(struct pbl_bio *bio, off_t start,
>  	return blocks_done;
>  }
>  
> +void at91_sdhci_set_highcapacity(struct pbl_bio *bio, bool highcap)
> +{
> +	struct at91_sdhci_priv *priv = bio->priv;
> +	priv->highcapacity_card = highcap;
> +}
> +
>  static struct at91_sdhci_priv atmel_sdcard;
>  
>  int at91_sdhci_bio_init(struct pbl_bio *bio, void __iomem *base)
> @@ -122,7 +128,7 @@ int at91_sdhci_bio_init(struct pbl_bio *bio, void __iomem *base)
>  	ret = at91_sdhci_set_ios(host, &ios);
>  
>  	 // FIXME can we determine this without leaving SD transfer mode?
> -	priv->highcapacity_card = 1;
> +	at91_sdhci_set_highcapacity(bio, true);
>  
>  	return 0;
>  }
> diff --git a/drivers/mci/atmel_mci_pbl.c b/drivers/mci/atmel_mci_pbl.c
> index 767d6f3ce2d7..82e45fd4e89e 100644
> --- a/drivers/mci/atmel_mci_pbl.c
> +++ b/drivers/mci/atmel_mci_pbl.c
> @@ -82,6 +82,12 @@ static int at91_mci_bio_read(struct pbl_bio *bio, off_t start,
>  	return blocks_done;
>  }
>  
> +void at91_mci_set_highcapacity(struct pbl_bio *bio, bool highcap)
> +{
> +	struct atmel_mci_priv *priv = bio->priv;
> +	priv->highcapacity_card = highcap;
> +}
> +
>  int at91_mci_bio_init(struct pbl_bio *bio, void __iomem *base,
>  		      unsigned int clock, unsigned int slot)
>  {
> @@ -110,7 +116,7 @@ int at91_mci_bio_init(struct pbl_bio *bio, void __iomem *base,
>  
>  	atmci_common_set_ios(host, &ios);
>  
> -	priv->highcapacity_card = 1;
> +	at91_mci_set_highcapacity(bio, true);
>  
>  	return 0;
>  }
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] ARM: at91: mmc-xload: allow overriding card capacity
  2021-06-02 10:25 ` [PATCH 2/3] ARM: at91: mmc-xload: allow overriding card capacity Ahmad Fatoum
  2021-06-02 10:30   ` Ahmad Fatoum
@ 2021-06-03  5:34   ` Alexander Dahl
  2021-06-03  6:28     ` Ahmad Fatoum
  1 sibling, 1 reply; 9+ messages in thread
From: Alexander Dahl @ 2021-06-03  5:34 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox, mgr, ore

Hello Ahmad,

just glanced over this series by accident, something catched my eye …

Am Wed, Jun 02, 2021 at 12:25:23PM +0200 schrieb Ahmad Fatoum:
> The PBL MMC driver works with the assumption that the BootROM has left
> the SD-Card in transfer mode. There seems to be no definitive way
> to find out whether a running card is high capacity (> 2G) or not,
> but we need this info when reading, because default capacities accept
> their read offset in bytes while high capacity deal in 512 byte blocks.

I'm a little surprised there's not.  Once like ten years ago I had to
write a SD card driver for a small microcontroller.  In the firmware I
switched to high capacity mode just based on the Card Capacity Status
(CCS) bit in the Operation Conditions Register (OCR) of the SD card.
Got that after sending advanced command 41 (send op cond) to the card.
Not sure if it's that easy, or if that command was only sent under
certain conditions, but I can not remember just guessing high capacity
based on some heuristics nor hard code it.  We certainly used low
capacity (like 32 MB for example) and high capacity cards (4G or more)
with that system.

You probably already looked for a reliable way to detect this.  I was
just curious why this needs to be hardcoded.

Have a nice day
Alex

> For i.MX, this is elegantly solved by just reading from sector 0.
> For AT91, we chainload barebox from FAT, so we need to seek around.
> So far, we just had an assumption buried in the driver that we are
> always highcapacity.
> 
> Export a knob to change this, so we can move the hardcoding from
> driver to board code, where it's more prominent. This is done
> in a follow-up commit.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  arch/arm/mach-at91/include/mach/xload.h | 3 +++
>  drivers/mci/atmel-sdhci-pbl.c           | 8 +++++++-
>  drivers/mci/atmel_mci_pbl.c             | 8 +++++++-
>  3 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/include/mach/xload.h b/arch/arm/mach-at91/include/mach/xload.h
> index bbc70af2108a..038f32554568 100644
> --- a/arch/arm/mach-at91/include/mach/xload.h
> +++ b/arch/arm/mach-at91/include/mach/xload.h
> @@ -12,4 +12,7 @@ int at91_sdhci_bio_init(struct pbl_bio *bio, void __iomem *base);
>  int at91_mci_bio_init(struct pbl_bio *bio, void __iomem *base,
>  		      unsigned int clock, unsigned int slot);
>  
> +void at91_mci_set_highcapacity(struct pbl_bio *bio, bool highcap);
> +void at91_sdhci_set_highcapacity(struct pbl_bio *bio, bool highcap);
> +
>  #endif /* __MACH_XLOAD_H */
> diff --git a/drivers/mci/atmel-sdhci-pbl.c b/drivers/mci/atmel-sdhci-pbl.c
> index 626e4008fe85..317f26f8af0d 100644
> --- a/drivers/mci/atmel-sdhci-pbl.c
> +++ b/drivers/mci/atmel-sdhci-pbl.c
> @@ -99,6 +99,12 @@ static int at91_sdhci_bio_read(struct pbl_bio *bio, off_t start,
>  	return blocks_done;
>  }
>  
> +void at91_sdhci_set_highcapacity(struct pbl_bio *bio, bool highcap)
> +{
> +	struct at91_sdhci_priv *priv = bio->priv;
> +	priv->highcapacity_card = highcap;
> +}
> +
>  static struct at91_sdhci_priv atmel_sdcard;
>  
>  int at91_sdhci_bio_init(struct pbl_bio *bio, void __iomem *base)
> @@ -122,7 +128,7 @@ int at91_sdhci_bio_init(struct pbl_bio *bio, void __iomem *base)
>  	ret = at91_sdhci_set_ios(host, &ios);
>  
>  	 // FIXME can we determine this without leaving SD transfer mode?
> -	priv->highcapacity_card = 1;
> +	at91_sdhci_set_highcapacity(bio, true);
>  
>  	return 0;
>  }
> diff --git a/drivers/mci/atmel_mci_pbl.c b/drivers/mci/atmel_mci_pbl.c
> index 767d6f3ce2d7..82e45fd4e89e 100644
> --- a/drivers/mci/atmel_mci_pbl.c
> +++ b/drivers/mci/atmel_mci_pbl.c
> @@ -82,6 +82,12 @@ static int at91_mci_bio_read(struct pbl_bio *bio, off_t start,
>  	return blocks_done;
>  }
>  
> +void at91_mci_set_highcapacity(struct pbl_bio *bio, bool highcap)
> +{
> +	struct atmel_mci_priv *priv = bio->priv;
> +	priv->highcapacity_card = highcap;
> +}
> +
>  int at91_mci_bio_init(struct pbl_bio *bio, void __iomem *base,
>  		      unsigned int clock, unsigned int slot)
>  {
> @@ -110,7 +116,7 @@ int at91_mci_bio_init(struct pbl_bio *bio, void __iomem *base,
>  
>  	atmci_common_set_ios(host, &ios);
>  
> -	priv->highcapacity_card = 1;
> +	at91_mci_set_highcapacity(bio, true);
>  
>  	return 0;
>  }
> -- 
> 2.29.2
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] ARM: at91: mmc-xload: allow overriding card capacity
  2021-06-03  5:34   ` Alexander Dahl
@ 2021-06-03  6:28     ` Ahmad Fatoum
  2021-06-03  6:53       ` Alexander Dahl
  0 siblings, 1 reply; 9+ messages in thread
From: Ahmad Fatoum @ 2021-06-03  6:28 UTC (permalink / raw)
  To: barebox, mgr, ore

Hei hei Alex,

On 03.06.21 07:34, Alexander Dahl wrote:
> Am Wed, Jun 02, 2021 at 12:25:23PM +0200 schrieb Ahmad Fatoum:
>> The PBL MMC driver works with the assumption that the BootROM has left
>> the SD-Card in transfer mode. There seems to be no definitive way
>> to find out whether a running card is high capacity (> 2G) or not,
>> but we need this info when reading, because default capacities accept
>> their read offset in bytes while high capacity deal in 512 byte blocks.
> 
> I'm a little surprised there's not.  Once like ten years ago I had to
> write a SD card driver for a small microcontroller.  In the firmware I
> switched to high capacity mode just based on the Card Capacity Status
> (CCS) bit in the Operation Conditions Register (OCR) of the SD card.
> Got that after sending advanced command 41 (send op cond) to the card.

barebox proper does that in sd_send_op_cond() as well.

> Not sure if it's that easy, or if that command was only sent under
> certain conditions, but I can not remember just guessing high capacity
> based on some heuristics nor hard code it. 

When you build at91_multi_defconfig, multiple images are generated, currently:

barebox-at91sam9x5ek.img
barebox-at91sam9263ek.img
barebox-microchip-ksz9477-evb.img
barebox-microchip-ksz9477-evb-xload-mmc.img
barebox-sama5d27-som1-ek.img
barebox-sama5d27-som1-ek-xload-mmc.img
barebox-groboards-sama5d27-giantboard.img
barebox-groboards-sama5d27-giantboard-xload-mmc.img
barebox-skov-arm9cpu.img

This patch here is for the xload-mmc.img's, which result from the barebox
prebootloader. The PBL sets up SDRAM and chainloads barebox proper from the
same boot medium that itself came from. Because of this limited scope, it can
reuse here the SD-card setup done by the BootROM. The BootROM leaves the
SD-Card in transfer mode, allowing the PBL to directly read blocks off the
SD-Card without a full MMC/SD-Card driver.

> We certainly used low
> capacity (like 32 MB for example) and high capacity cards (4G or more)
> with that system.
> 
> You probably already looked for a reliable way to detect this.  I was
> just curious why this needs to be hardcoded.

It's been a while since I wrote the sama5d27 PBL support (which the
sama5d3 support I am patching here is based on), but I recall that
the send op cond did not work in transfer mode, the card must be sent
into idle first, i.e. reset. I'd be happy if there happens to be indeed
a way to deduce high capacity mode without resetting and having to repeat
the SD-Card setup in the first boot stage.

There are of course alternatives to this:
  - build barebox twice with different configs for first and second stage:
    There is already code for this (chainload code that uses barebox proper
    driver that fully re-initializes card). The first stage config needs to
    be small to fit into SRAM, so we can no longer generate both stages from
    the same multi-image config as we already do.

  - provide full MMC support in PBL:
    The objective so far was to keep PBL code to the bare minimum. Doing
    full MMC setup there violates that.


AFAIK, all barebox platforms, except for i.MX, went for the first alternative
of having multiple configs. barebox on i.MX doesn't have this issue, because
it reads barebox from offset 0 of the SD/MMC, which works equally well whether
booting off default or high capacity cards.

I like how the i.MX defconfig generates more than a hundred images in one go and
wanted AT91 to have something similar, but the fact that FAT needs to seek around
(offsets != 0) complicates this.
The trade off I took then was assuming high capacity cards and postpone the decision
on how to deal with default capacity cards in PBL into the future.

Cheers,
Ahmad

> 
> Have a nice day
> Alex
> 
>> For i.MX, this is elegantly solved by just reading from sector 0.
>> For AT91, we chainload barebox from FAT, so we need to seek around.
>> So far, we just had an assumption buried in the driver that we are
>> always highcapacity.
>>
>> Export a knob to change this, so we can move the hardcoding from
>> driver to board code, where it's more prominent. This is done
>> in a follow-up commit.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  arch/arm/mach-at91/include/mach/xload.h | 3 +++
>>  drivers/mci/atmel-sdhci-pbl.c           | 8 +++++++-
>>  drivers/mci/atmel_mci_pbl.c             | 8 +++++++-
>>  3 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-at91/include/mach/xload.h b/arch/arm/mach-at91/include/mach/xload.h
>> index bbc70af2108a..038f32554568 100644
>> --- a/arch/arm/mach-at91/include/mach/xload.h
>> +++ b/arch/arm/mach-at91/include/mach/xload.h
>> @@ -12,4 +12,7 @@ int at91_sdhci_bio_init(struct pbl_bio *bio, void __iomem *base);
>>  int at91_mci_bio_init(struct pbl_bio *bio, void __iomem *base,
>>  		      unsigned int clock, unsigned int slot);
>>  
>> +void at91_mci_set_highcapacity(struct pbl_bio *bio, bool highcap);
>> +void at91_sdhci_set_highcapacity(struct pbl_bio *bio, bool highcap);
>> +
>>  #endif /* __MACH_XLOAD_H */
>> diff --git a/drivers/mci/atmel-sdhci-pbl.c b/drivers/mci/atmel-sdhci-pbl.c
>> index 626e4008fe85..317f26f8af0d 100644
>> --- a/drivers/mci/atmel-sdhci-pbl.c
>> +++ b/drivers/mci/atmel-sdhci-pbl.c
>> @@ -99,6 +99,12 @@ static int at91_sdhci_bio_read(struct pbl_bio *bio, off_t start,
>>  	return blocks_done;
>>  }
>>  
>> +void at91_sdhci_set_highcapacity(struct pbl_bio *bio, bool highcap)
>> +{
>> +	struct at91_sdhci_priv *priv = bio->priv;
>> +	priv->highcapacity_card = highcap;
>> +}
>> +
>>  static struct at91_sdhci_priv atmel_sdcard;
>>  
>>  int at91_sdhci_bio_init(struct pbl_bio *bio, void __iomem *base)
>> @@ -122,7 +128,7 @@ int at91_sdhci_bio_init(struct pbl_bio *bio, void __iomem *base)
>>  	ret = at91_sdhci_set_ios(host, &ios);
>>  
>>  	 // FIXME can we determine this without leaving SD transfer mode?
>> -	priv->highcapacity_card = 1;
>> +	at91_sdhci_set_highcapacity(bio, true);
>>  
>>  	return 0;
>>  }
>> diff --git a/drivers/mci/atmel_mci_pbl.c b/drivers/mci/atmel_mci_pbl.c
>> index 767d6f3ce2d7..82e45fd4e89e 100644
>> --- a/drivers/mci/atmel_mci_pbl.c
>> +++ b/drivers/mci/atmel_mci_pbl.c
>> @@ -82,6 +82,12 @@ static int at91_mci_bio_read(struct pbl_bio *bio, off_t start,
>>  	return blocks_done;
>>  }
>>  
>> +void at91_mci_set_highcapacity(struct pbl_bio *bio, bool highcap)
>> +{
>> +	struct atmel_mci_priv *priv = bio->priv;
>> +	priv->highcapacity_card = highcap;
>> +}
>> +
>>  int at91_mci_bio_init(struct pbl_bio *bio, void __iomem *base,
>>  		      unsigned int clock, unsigned int slot)
>>  {
>> @@ -110,7 +116,7 @@ int at91_mci_bio_init(struct pbl_bio *bio, void __iomem *base,
>>  
>>  	atmci_common_set_ios(host, &ios);
>>  
>> -	priv->highcapacity_card = 1;
>> +	at91_mci_set_highcapacity(bio, true);
>>  
>>  	return 0;
>>  }
>> -- 
>> 2.29.2
>>
>>
>> _______________________________________________
>> barebox mailing list
>> barebox@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] ARM: at91: mmc-xload: allow overriding card capacity
  2021-06-03  6:28     ` Ahmad Fatoum
@ 2021-06-03  6:53       ` Alexander Dahl
  2021-06-03  7:01         ` Ahmad Fatoum
  0 siblings, 1 reply; 9+ messages in thread
From: Alexander Dahl @ 2021-06-03  6:53 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox, mgr, ore

Hello,

thanks for the surprisingly detailed explanation.

Am Thu, Jun 03, 2021 at 08:28:48AM +0200 schrieb Ahmad Fatoum:
> Hei hei Alex,
> 
> On 03.06.21 07:34, Alexander Dahl wrote:
> > Am Wed, Jun 02, 2021 at 12:25:23PM +0200 schrieb Ahmad Fatoum:
> >> The PBL MMC driver works with the assumption that the BootROM has left
> >> the SD-Card in transfer mode. There seems to be no definitive way
> >> to find out whether a running card is high capacity (> 2G) or not,
> >> but we need this info when reading, because default capacities accept
> >> their read offset in bytes while high capacity deal in 512 byte blocks.
> > 
> > I'm a little surprised there's not.  Once like ten years ago I had to
> > write a SD card driver for a small microcontroller.  In the firmware I
> > switched to high capacity mode just based on the Card Capacity Status
> > (CCS) bit in the Operation Conditions Register (OCR) of the SD card.
> > Got that after sending advanced command 41 (send op cond) to the card.
> 
> barebox proper does that in sd_send_op_cond() as well.
> 
> > Not sure if it's that easy, or if that command was only sent under
> > certain conditions, but I can not remember just guessing high capacity
> > based on some heuristics nor hard code it. 
> 
> When you build at91_multi_defconfig, multiple images are generated, currently:
> 
> barebox-at91sam9x5ek.img
> barebox-at91sam9263ek.img
> barebox-microchip-ksz9477-evb.img
> barebox-microchip-ksz9477-evb-xload-mmc.img
> barebox-sama5d27-som1-ek.img
> barebox-sama5d27-som1-ek-xload-mmc.img
> barebox-groboards-sama5d27-giantboard.img
> barebox-groboards-sama5d27-giantboard-xload-mmc.img
> barebox-skov-arm9cpu.img
> 
> This patch here is for the xload-mmc.img's, which result from the barebox
> prebootloader. The PBL sets up SDRAM and chainloads barebox proper from the
> same boot medium that itself came from. Because of this limited scope, it can
> reuse here the SD-card setup done by the BootROM. The BootROM leaves the
> SD-Card in transfer mode, allowing the PBL to directly read blocks off the
> SD-Card without a full MMC/SD-Card driver.

I see.

> > We certainly used low
> > capacity (like 32 MB for example) and high capacity cards (4G or more)
> > with that system.
> > 
> > You probably already looked for a reliable way to detect this.  I was
> > just curious why this needs to be hardcoded.
> 
> It's been a while since I wrote the sama5d27 PBL support (which the
> sama5d3 support I am patching here is based on), but I recall that
> the send op cond did not work in transfer mode, the card must be sent
> into idle first, i.e. reset. I'd be happy if there happens to be indeed
> a way to deduce high capacity mode without resetting and having to repeat
> the SD-Card setup in the first boot stage.

Makes sense.  And yes, IIRC correctly, that register read happens
while setting up the card.  If you can not or don't want to reset the
card again, it's probably tricky.

> There are of course alternatives to this:
>   - build barebox twice with different configs for first and second stage:
>     There is already code for this (chainload code that uses barebox proper
>     driver that fully re-initializes card). The first stage config needs to
>     be small to fit into SRAM, so we can no longer generate both stages from
>     the same multi-image config as we already do.
> 
>   - provide full MMC support in PBL:
>     The objective so far was to keep PBL code to the bare minimum. Doing
>     full MMC setup there violates that.
> 
> 
> AFAIK, all barebox platforms, except for i.MX, went for the first alternative
> of having multiple configs. barebox on i.MX doesn't have this issue, because
> it reads barebox from offset 0 of the SD/MMC, which works equally well whether
> booting off default or high capacity cards.
> 
> I like how the i.MX defconfig generates more than a hundred images in one go and
> wanted AT91 to have something similar, but the fact that FAT needs to seek around
> (offsets != 0) complicates this.
> The trade off I took then was assuming high capacity cards and postpone the decision
> on how to deal with default capacity cards in PBL into the future.

I just had the idea of just trying to read from different small
offsets and compare it with the block you get from offset 0.  For
example reading 500 bytes from offset 10 and reading 510 bytes from
offset 0 in byte mode should match the last 500 bytes, but probably in
block mode that wouldn't match.  But that's obviously highly dependent
on the card content and more an ugly hack, right?

Greets
Alex


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] ARM: at91: mmc-xload: allow overriding card capacity
  2021-06-03  6:53       ` Alexander Dahl
@ 2021-06-03  7:01         ` Ahmad Fatoum
  0 siblings, 0 replies; 9+ messages in thread
From: Ahmad Fatoum @ 2021-06-03  7:01 UTC (permalink / raw)
  To: barebox, mgr, ore

Hello Alex,

On 03.06.21 08:53, Alexander Dahl wrote:
> thanks for the surprisingly detailed explanation.

:-)

> Am Thu, Jun 03, 2021 at 08:28:48AM +0200 schrieb Ahmad Fatoum:
>> On 03.06.21 07:34, Alexander Dahl wrote:
>>> Am Wed, Jun 02, 2021 at 12:25:23PM +0200 schrieb Ahmad Fatoum:
>>>> The PBL MMC driver works with the assumption that the BootROM has left
>>>> the SD-Card in transfer mode. There seems to be no definitive way
>>>> to find out whether a running card is high capacity (> 2G) or not,
>>>> but we need this info when reading, because default capacities accept
>>>> their read offset in bytes while high capacity deal in 512 byte blocks.
>>>
>>> I'm a little surprised there's not.  Once like ten years ago I had to
>>> write a SD card driver for a small microcontroller.  In the firmware I
>>> switched to high capacity mode just based on the Card Capacity Status
>>> (CCS) bit in the Operation Conditions Register (OCR) of the SD card.
>>> Got that after sending advanced command 41 (send op cond) to the card.
>>
>> barebox proper does that in sd_send_op_cond() as well.
>>
>>> Not sure if it's that easy, or if that command was only sent under
>>> certain conditions, but I can not remember just guessing high capacity
>>> based on some heuristics nor hard code it. 
>>
>> When you build at91_multi_defconfig, multiple images are generated, currently:
>>
>> barebox-at91sam9x5ek.img
>> barebox-at91sam9263ek.img
>> barebox-microchip-ksz9477-evb.img
>> barebox-microchip-ksz9477-evb-xload-mmc.img
>> barebox-sama5d27-som1-ek.img
>> barebox-sama5d27-som1-ek-xload-mmc.img
>> barebox-groboards-sama5d27-giantboard.img
>> barebox-groboards-sama5d27-giantboard-xload-mmc.img
>> barebox-skov-arm9cpu.img
>>
>> This patch here is for the xload-mmc.img's, which result from the barebox
>> prebootloader. The PBL sets up SDRAM and chainloads barebox proper from the
>> same boot medium that itself came from. Because of this limited scope, it can
>> reuse here the SD-card setup done by the BootROM. The BootROM leaves the
>> SD-Card in transfer mode, allowing the PBL to directly read blocks off the
>> SD-Card without a full MMC/SD-Card driver.
> 
> I see.
> 
>>> We certainly used low
>>> capacity (like 32 MB for example) and high capacity cards (4G or more)
>>> with that system.
>>>
>>> You probably already looked for a reliable way to detect this.  I was
>>> just curious why this needs to be hardcoded.
>>
>> It's been a while since I wrote the sama5d27 PBL support (which the
>> sama5d3 support I am patching here is based on), but I recall that
>> the send op cond did not work in transfer mode, the card must be sent
>> into idle first, i.e. reset. I'd be happy if there happens to be indeed
>> a way to deduce high capacity mode without resetting and having to repeat
>> the SD-Card setup in the first boot stage.
> 
> Makes sense.  And yes, IIRC correctly, that register read happens
> while setting up the card.  If you can not or don't want to reset the
> card again, it's probably tricky.
> 
>> There are of course alternatives to this:
>>   - build barebox twice with different configs for first and second stage:
>>     There is already code for this (chainload code that uses barebox proper
>>     driver that fully re-initializes card). The first stage config needs to
>>     be small to fit into SRAM, so we can no longer generate both stages from
>>     the same multi-image config as we already do.
>>
>>   - provide full MMC support in PBL:
>>     The objective so far was to keep PBL code to the bare minimum. Doing
>>     full MMC setup there violates that.
>>
>>
>> AFAIK, all barebox platforms, except for i.MX, went for the first alternative
>> of having multiple configs. barebox on i.MX doesn't have this issue, because
>> it reads barebox from offset 0 of the SD/MMC, which works equally well whether
>> booting off default or high capacity cards.
>>
>> I like how the i.MX defconfig generates more than a hundred images in one go and
>> wanted AT91 to have something similar, but the fact that FAT needs to seek around
>> (offsets != 0) complicates this.
>> The trade off I took then was assuming high capacity cards and postpone the decision
>> on how to deal with default capacity cards in PBL into the future.
> 
> I just had the idea of just trying to read from different small
> offsets and compare it with the block you get from offset 0.  For
> example reading 500 bytes from offset 10 and reading 510 bytes from
> offset 0 in byte mode should match the last 500 bytes, but probably in
> block mode that wouldn't match.  But that's obviously highly dependent
> on the card content and more an ugly hack, right?

Tested that yesterday: reading from the 2G default capacity card at offset 1
caused a hang. So any heuristic will probably need to read 512-byte aligned
blocks.

Cheers,
Ahmad

> 
> Greets
> Alex
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-06-03  7:03 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-02 10:25 [PATCH 1/3] ARM: at91: microchip-ksz9477-evb: support PBL console Ahmad Fatoum
2021-06-02 10:25 ` [PATCH 2/3] ARM: at91: mmc-xload: allow overriding card capacity Ahmad Fatoum
2021-06-02 10:30   ` Ahmad Fatoum
2021-06-03  5:34   ` Alexander Dahl
2021-06-03  6:28     ` Ahmad Fatoum
2021-06-03  6:53       ` Alexander Dahl
2021-06-03  7:01         ` Ahmad Fatoum
2021-06-02 10:25 ` [PATCH 3/3] ARM: at91: xload-mmc: add prominent note about PBL MMC limitation Ahmad Fatoum
2021-06-02 10:29 ` [PATCH 1/3] ARM: at91: microchip-ksz9477-evb: support PBL console Ahmad Fatoum

mail archive of the barebox mailing list

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lore.barebox.org/barebox/0 barebox/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 barebox barebox/ https://lore.barebox.org/barebox \
		barebox@lists.infradead.org barebox@lists.infradead.org
	public-inbox-index barebox

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git