[PATCH v5 0/5] Support EFI partition on NVIDIA Tegra devices

[PATCH v5 0/5] Support EFI partition on NVIDIA Tegra devices

[Dmitry Osipenko]

This series adds the most minimal EFI partition support for NVIDIA Tegra
consumer devices, like Android tablets and game consoles, making theirs
eMMC accessible out-of-the-box using downstream bootloader and mainline
Linux kernel.  eMMC now works on Acer A500 tablet and Ouya game console
that are already well supported in mainline and internal storage is the
only biggest thing left to support.

Changelog:

v5: - Implemented alternative_gpt_sector() blk/mmc callback that was
      suggested by Christoph Hellwig in a comment to v4.

    - mmc_bdev_to_card() now checks blk fops instead of the major number,
      like it was suggested by Christoph Hellwig in a comment to v4.

    - Emailed Rob Herring, which was asked by Ulf Hansson in a comment
      to v4. Although the of-match change is gone now in v5, the matching
      is transformed into the new SDHCI quirk of the Tegra driver.

v4: - Rebased on top of recent linux-next.

v3: - Removed unnecessary v1 hunk that was left by accident in efi.c of v2.

v2: - This is continuation of [1] where Davidlohr Bueso suggested that it
      should be better to avoid supporting in mainline the custom gpt_sector
      kernel cmdline parameter that downstream Android kernels use.  We can
      do this for the devices that are already mainlined, so I dropped the
      cmdline from the v2 and left only the variant with a fixed GPT address.

[1] https://lore.kernel.org/linux-efi/20210327212100.3834-3-digetx@gmail.com/T/

Dmitry Osipenko (5):
  block: Add alternative_gpt_sector() operation
  mmc: block: Support alternative_gpt_sector() operation
  mmc: core: Add raw_boot_mult field to mmc_ext_csd
  mmc: sdhci-tegra: Implement alternative_gpt_sector()
  partitions/efi: Support non-standard GPT location

 block/partitions/efi.c         | 13 +++++++++++
 drivers/mmc/core/block.c       | 30 ++++++++++++++++++++++++
 drivers/mmc/core/mmc.c         |  2 ++
 drivers/mmc/host/sdhci-tegra.c | 42 ++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h         |  1 +
 include/linux/mmc/card.h       |  1 +
 include/linux/mmc/host.h       |  4 ++++
 7 files changed, 93 insertions(+)

-- 
2.32.0

[PATCH v5 1/5] block: Add alternative_gpt_sector() operation

[Dmitry Osipenko]

Add alternative_gpt_sector() block device operation which specifies
alternative location of a GPT entry. This allows us to support Android
devices which have GPT entry at a non-standard location and can't be
repartitioned easily.

Suggested-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 include/linux/blkdev.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 111a3911c4d2..a662a8f5065f 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1848,6 +1848,7 @@ struct block_device_operations {
 	int (*report_zones)(struct gendisk *, sector_t sector,
 			unsigned int nr_zones, report_zones_cb cb, void *data);
 	char *(*devnode)(struct gendisk *disk, umode_t *mode);
+	int (*alternative_gpt_sector)(struct block_device *, sector_t *);
 	struct module *owner;
 	const struct pr_ops *pr_ops;
 };
-- 
2.32.0

Re: [PATCH v5 1/5] block: Add alternative_gpt_sector() operation

[Christoph Hellwig]

On Wed, Aug 18, 2021 at 03:55:43AM +0300, Dmitry Osipenko wrote:
> Add alternative_gpt_sector() block device operation which specifies
> alternative location of a GPT entry. This allows us to support Android
> devices which have GPT entry at a non-standard location and can't be
> repartitioned easily.

Probably worth adding a little comment explaining this rather odd
operation.

Re: [PATCH v5 1/5] block: Add alternative_gpt_sector() operation

[Dmitry Osipenko]

18.08.2021 08:20, Christoph Hellwig пишет:
> On Wed, Aug 18, 2021 at 03:55:43AM +0300, Dmitry Osipenko wrote:
>> Add alternative_gpt_sector() block device operation which specifies
>> alternative location of a GPT entry. This allows us to support Android
>> devices which have GPT entry at a non-standard location and can't be
>> repartitioned easily.
> 
> Probably worth adding a little comment explaining this rather odd
> operation.
> 

I'll add the comment in v6.

[PATCH v5 2/5] mmc: block: Support alternative_gpt_sector() operation

[Dmitry Osipenko]

Support generic alternative_gpt_sector() block device operation by
invoking a new platform-specific MMC host hook that is also named
alternative_gpt_sector().

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/mmc/core/block.c | 30 ++++++++++++++++++++++++++++++
 include/linux/mmc/host.h |  4 ++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 672cc505ce37..8ad1841f0dbd 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -178,6 +178,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
 			       int disable_multi,
 			       struct mmc_queue *mq);
 static void mmc_blk_hsq_req_done(struct mmc_request *mrq);
+static struct mmc_card *mmc_bdev_to_card(struct block_device *bdev);
 
 static struct mmc_blk_data *mmc_blk_get(struct gendisk *disk)
 {
@@ -801,6 +802,20 @@ static int mmc_blk_compat_ioctl(struct block_device *bdev, fmode_t mode,
 }
 #endif
 
+static int mmc_blk_alternative_gpt_sector(struct block_device *bdev,
+					  sector_t *sector)
+{
+	struct mmc_card *card = mmc_bdev_to_card(bdev);
+
+	if (!card)
+		return -ENODEV;
+
+	if (!card->host->ops->alternative_gpt_sector)
+		return -EOPNOTSUPP;
+
+	return card->host->ops->alternative_gpt_sector(card, sector);
+}
+
 static const struct block_device_operations mmc_bdops = {
 	.open			= mmc_blk_open,
 	.release		= mmc_blk_release,
@@ -810,8 +825,23 @@ static const struct block_device_operations mmc_bdops = {
 #ifdef CONFIG_COMPAT
 	.compat_ioctl		= mmc_blk_compat_ioctl,
 #endif
+	.alternative_gpt_sector	= mmc_blk_alternative_gpt_sector,
 };
 
+static struct mmc_card *mmc_bdev_to_card(struct block_device *bdev)
+{
+	struct mmc_blk_data *md;
+
+	if (bdev->bd_disk->fops != &mmc_bdops)
+		return NULL;
+
+	md = mmc_blk_get(bdev->bd_disk);
+	if (!md)
+		return NULL;
+
+	return md->queue.card;
+}
+
 static int mmc_blk_part_switch_pre(struct mmc_card *card,
 				   unsigned int part_type)
 {
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0abd47e9ef9b..18281c444bf6 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -190,6 +190,10 @@ struct mmc_host_ops {
 
 	/* Initialize an SD express card, mandatory for MMC_CAP2_SD_EXP. */
 	int	(*init_sd_express)(struct mmc_host *host, struct mmc_ios *ios);
+
+	/* Get platform-specific GPT entry location */
+	int	(*alternative_gpt_sector)(struct mmc_card *card,
+					  sector_t *sector);
 };
 
 struct mmc_cqe_ops {
-- 
2.32.0

Re: [PATCH v5 2/5] mmc: block: Support alternative_gpt_sector() operation

[Christoph Hellwig]

On Wed, Aug 18, 2021 at 03:55:44AM +0300, Dmitry Osipenko wrote:
> +static int mmc_blk_alternative_gpt_sector(struct block_device *bdev,
> +					  sector_t *sector)
> +{
> +	struct mmc_card *card = mmc_bdev_to_card(bdev);
> +
> +	if (!card)
> +		return -ENODEV;
> +
> +	if (!card->host->ops->alternative_gpt_sector)
> +		return -EOPNOTSUPP;
> +
> +	return card->host->ops->alternative_gpt_sector(card, sector);
> +}
> +

> +static struct mmc_card *mmc_bdev_to_card(struct block_device *bdev)
> +{
> +	struct mmc_blk_data *md;
> +
> +	if (bdev->bd_disk->fops != &mmc_bdops)
> +		return NULL;

No need for this check bow that it is only called through mmc_bdops.

> +
> +	md = mmc_blk_get(bdev->bd_disk);
> +	if (!md)
> +		return NULL;
> +
> +	return md->queue.card;
> +}

This reference seems to never be dropped anywhere.

Re: [PATCH v5 2/5] mmc: block: Support alternative_gpt_sector() operation

[Dmitry Osipenko]

18.08.2021 08:28, Christoph Hellwig пишет:
> On Wed, Aug 18, 2021 at 03:55:44AM +0300, Dmitry Osipenko wrote:
>> +static int mmc_blk_alternative_gpt_sector(struct block_device *bdev,
>> +					  sector_t *sector)
>> +{
>> +	struct mmc_card *card = mmc_bdev_to_card(bdev);
>> +
>> +	if (!card)
>> +		return -ENODEV;
>> +
>> +	if (!card->host->ops->alternative_gpt_sector)
>> +		return -EOPNOTSUPP;
>> +
>> +	return card->host->ops->alternative_gpt_sector(card, sector);
>> +}
>> +
> 
>> +static struct mmc_card *mmc_bdev_to_card(struct block_device *bdev)
>> +{
>> +	struct mmc_blk_data *md;
>> +
>> +	if (bdev->bd_disk->fops != &mmc_bdops)
>> +		return NULL;
> 
> No need for this check bow that it is only called through mmc_bdops.

Alright

>> +
>> +	md = mmc_blk_get(bdev->bd_disk);
>> +	if (!md)
>> +		return NULL;
>> +
>> +	return md->queue.card;
>> +}
> 
> This reference seems to never be dropped anywhere.
> 

Indeed, I missed that it bumps the refcnt.

[PATCH v5 3/5] mmc: core: Add raw_boot_mult field to mmc_ext_csd

[Dmitry Osipenko]

Bootloader of NVIDIA Tegra devices linearizes the boot0/boot1/main
partitions into a single virtual space, and thus, all partition addresses
are shifted by the size of boot0 + boot1 partitions.  The offset needs to
be known in order to find the EFI entry on eMMC storage of Tegra devices.
Add raw_boot_mult field to mmc_ext_csd which allows to get size of the
boot partitions.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/mmc/core/mmc.c   | 2 ++
 include/linux/mmc/card.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 838726b68ff3..29e58ffae379 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -418,6 +418,8 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
 		ext_csd[EXT_CSD_ERASE_TIMEOUT_MULT];
 	card->ext_csd.raw_hc_erase_grp_size =
 		ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE];
+	card->ext_csd.raw_boot_mult =
+		ext_csd[EXT_CSD_BOOT_MULT];
 	if (card->ext_csd.rev >= 3) {
 		u8 sa_shift = ext_csd[EXT_CSD_S_A_TIMEOUT];
 		card->ext_csd.part_config = ext_csd[EXT_CSD_PART_CONFIG];
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 74e6c0624d27..37f975875102 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -109,6 +109,7 @@ struct mmc_ext_csd {
 	u8			raw_hc_erase_gap_size;	/* 221 */
 	u8			raw_erase_timeout_mult;	/* 223 */
 	u8			raw_hc_erase_grp_size;	/* 224 */
+	u8			raw_boot_mult;		/* 226 */
 	u8			raw_sec_trim_mult;	/* 229 */
 	u8			raw_sec_erase_mult;	/* 230 */
 	u8			raw_sec_feature_support;/* 231 */
-- 
2.32.0

[PATCH v5 4/5] mmc: sdhci-tegra: Implement alternative_gpt_sector()

[Dmitry Osipenko]

Tegra20/30/114/124 Android devices place GPT at a non-standard location.
Implement alternative_gpt_sector() callback of the MMC host ops which
specifies that GPT location for the partition scanner.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/mmc/host/sdhci-tegra.c | 42 ++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 387ce9cdbd7c..24a713689d5b 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -116,6 +116,8 @@
  */
 #define NVQUIRK_HAS_TMCLK				BIT(10)
 
+#define NVQUIRK_HAS_ANDROID_GPT_SECTOR			BIT(11)
+
 /* SDMMC CQE Base Address for Tegra Host Ver 4.1 and Higher */
 #define SDHCI_TEGRA_CQE_BASE_ADDR			0xF000
 
@@ -1361,6 +1363,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra20 = {
 	.pdata = &sdhci_tegra20_pdata,
 	.dma_mask = DMA_BIT_MASK(32),
 	.nvquirks = NVQUIRK_FORCE_SDHCI_SPEC_200 |
+		    NVQUIRK_HAS_ANDROID_GPT_SECTOR |
 		    NVQUIRK_ENABLE_BLOCK_GAP_DET,
 };
 
@@ -1390,6 +1393,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra30 = {
 	.nvquirks = NVQUIRK_ENABLE_SDHCI_SPEC_300 |
 		    NVQUIRK_ENABLE_SDR50 |
 		    NVQUIRK_ENABLE_SDR104 |
+		    NVQUIRK_HAS_ANDROID_GPT_SECTOR |
 		    NVQUIRK_HAS_PADCALIB,
 };
 
@@ -1422,6 +1426,7 @@ static const struct sdhci_pltfm_data sdhci_tegra114_pdata = {
 static const struct sdhci_tegra_soc_data soc_data_tegra114 = {
 	.pdata = &sdhci_tegra114_pdata,
 	.dma_mask = DMA_BIT_MASK(32),
+	.nvquirks = NVQUIRK_HAS_ANDROID_GPT_SECTOR,
 };
 
 static const struct sdhci_pltfm_data sdhci_tegra124_pdata = {
@@ -1438,6 +1443,7 @@ static const struct sdhci_pltfm_data sdhci_tegra124_pdata = {
 static const struct sdhci_tegra_soc_data soc_data_tegra124 = {
 	.pdata = &sdhci_tegra124_pdata,
 	.dma_mask = DMA_BIT_MASK(34),
+	.nvquirks = NVQUIRK_HAS_ANDROID_GPT_SECTOR,
 };
 
 static const struct sdhci_ops tegra210_sdhci_ops = {
@@ -1590,6 +1596,38 @@ static int sdhci_tegra_add_host(struct sdhci_host *host)
 	return ret;
 }
 
+static int sdhci_tegra_alternative_gpt_sector(struct mmc_card *card,
+					      sector_t *gpt_sector)
+{
+	unsigned int boot_sectors_num;
+
+	/* filter out unrelated cards */
+	if (card->ext_csd.rev < 3 ||
+	    !mmc_card_mmc(card) ||
+	    !mmc_card_is_blockaddr(card) ||
+	     mmc_card_is_removable(card->host))
+		return -ENOENT;
+
+	/*
+	 * eMMC storage has two special boot partitions in addition to the
+	 * main one.  NVIDIA's bootloader linearizes eMMC boot0->boot1->main
+	 * accesses, this means that the partition table addresses are shifted
+	 * by the size of boot partitions.  In accordance with the eMMC
+	 * specification, the boot partition size is calculated as follows:
+	 *
+	 *	boot partition size = 128K byte x BOOT_SIZE_MULT
+	 *
+	 * Calculate number of sectors occupied by the both boot partitions.
+	 */
+	boot_sectors_num = card->ext_csd.raw_boot_mult * SZ_128K /
+			   SZ_512 * MMC_NUM_BOOT_PARTITION;
+
+	/* Defined by NVIDIA and used by Android devices. */
+	*gpt_sector = card->ext_csd.sectors - boot_sectors_num - 1;
+
+	return 0;
+}
+
 static int sdhci_tegra_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
@@ -1616,6 +1654,10 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
 	tegra_host->pad_control_available = false;
 	tegra_host->soc_data = soc_data;
 
+	if (soc_data->nvquirks & NVQUIRK_HAS_ANDROID_GPT_SECTOR)
+		host->mmc_host_ops.alternative_gpt_sector =
+				sdhci_tegra_alternative_gpt_sector;
+
 	if (soc_data->nvquirks & NVQUIRK_NEEDS_PAD_CONTROL) {
 		rc = tegra_sdhci_init_pinctrl_info(&pdev->dev, tegra_host);
 		if (rc == 0)
-- 
2.32.0

Re: [PATCH v5 4/5] mmc: sdhci-tegra: Implement alternative_gpt_sector()

[Christoph Hellwig]

On Wed, Aug 18, 2021 at 03:55:46AM +0300, Dmitry Osipenko wrote:
> Tegra20/30/114/124 Android devices place GPT at a non-standard location.
> Implement alternative_gpt_sector() callback of the MMC host ops which
> specifies that GPT location for the partition scanner.

Just curious:  how do the android kernels deal with this mess?

Re: [PATCH v5 4/5] mmc: sdhci-tegra: Implement alternative_gpt_sector()

[Dmitry Osipenko]

18.08.2021 08:30, Christoph Hellwig пишет:
> On Wed, Aug 18, 2021 at 03:55:46AM +0300, Dmitry Osipenko wrote:
>> Tegra20/30/114/124 Android devices place GPT at a non-standard location.
>> Implement alternative_gpt_sector() callback of the MMC host ops which
>> specifies that GPT location for the partition scanner.
> 
> Just curious:  how do the android kernels deal with this mess?

They either hack MMC core to cut off the last sector from blk dev or use
non-standard gpt_sector=<sector> kernel cmdline argument that is passed
by downstream bootloaders.

Re: [PATCH v5 4/5] mmc: sdhci-tegra: Implement alternative_gpt_sector()

[Ulf Hansson]

On Wed, 18 Aug 2021 at 02:57, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> Tegra20/30/114/124 Android devices place GPT at a non-standard location.
> Implement alternative_gpt_sector() callback of the MMC host ops which
> specifies that GPT location for the partition scanner.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/mmc/host/sdhci-tegra.c | 42 ++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index 387ce9cdbd7c..24a713689d5b 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -116,6 +116,8 @@
>   */
>  #define NVQUIRK_HAS_TMCLK                              BIT(10)
>
> +#define NVQUIRK_HAS_ANDROID_GPT_SECTOR                 BIT(11)
> +
>  /* SDMMC CQE Base Address for Tegra Host Ver 4.1 and Higher */
>  #define SDHCI_TEGRA_CQE_BASE_ADDR                      0xF000
>
> @@ -1361,6 +1363,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra20 = {
>         .pdata = &sdhci_tegra20_pdata,
>         .dma_mask = DMA_BIT_MASK(32),
>         .nvquirks = NVQUIRK_FORCE_SDHCI_SPEC_200 |
> +                   NVQUIRK_HAS_ANDROID_GPT_SECTOR |
>                     NVQUIRK_ENABLE_BLOCK_GAP_DET,
>  };
>
> @@ -1390,6 +1393,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra30 = {
>         .nvquirks = NVQUIRK_ENABLE_SDHCI_SPEC_300 |
>                     NVQUIRK_ENABLE_SDR50 |
>                     NVQUIRK_ENABLE_SDR104 |
> +                   NVQUIRK_HAS_ANDROID_GPT_SECTOR |
>                     NVQUIRK_HAS_PADCALIB,
>  };
>
> @@ -1422,6 +1426,7 @@ static const struct sdhci_pltfm_data sdhci_tegra114_pdata = {
>  static const struct sdhci_tegra_soc_data soc_data_tegra114 = {
>         .pdata = &sdhci_tegra114_pdata,
>         .dma_mask = DMA_BIT_MASK(32),
> +       .nvquirks = NVQUIRK_HAS_ANDROID_GPT_SECTOR,
>  };
>
>  static const struct sdhci_pltfm_data sdhci_tegra124_pdata = {
> @@ -1438,6 +1443,7 @@ static const struct sdhci_pltfm_data sdhci_tegra124_pdata = {
>  static const struct sdhci_tegra_soc_data soc_data_tegra124 = {
>         .pdata = &sdhci_tegra124_pdata,
>         .dma_mask = DMA_BIT_MASK(34),
> +       .nvquirks = NVQUIRK_HAS_ANDROID_GPT_SECTOR,
>  };
>
>  static const struct sdhci_ops tegra210_sdhci_ops = {
> @@ -1590,6 +1596,38 @@ static int sdhci_tegra_add_host(struct sdhci_host *host)
>         return ret;
>  }
>
> +static int sdhci_tegra_alternative_gpt_sector(struct mmc_card *card,
> +                                             sector_t *gpt_sector)
> +{
> +       unsigned int boot_sectors_num;
> +
> +       /* filter out unrelated cards */
> +       if (card->ext_csd.rev < 3 ||
> +           !mmc_card_mmc(card) ||
> +           !mmc_card_is_blockaddr(card) ||
> +            mmc_card_is_removable(card->host))
> +               return -ENOENT;
> +
> +       /*
> +        * eMMC storage has two special boot partitions in addition to the
> +        * main one.  NVIDIA's bootloader linearizes eMMC boot0->boot1->main
> +        * accesses, this means that the partition table addresses are shifted
> +        * by the size of boot partitions.  In accordance with the eMMC
> +        * specification, the boot partition size is calculated as follows:
> +        *
> +        *      boot partition size = 128K byte x BOOT_SIZE_MULT
> +        *
> +        * Calculate number of sectors occupied by the both boot partitions.
> +        */
> +       boot_sectors_num = card->ext_csd.raw_boot_mult * SZ_128K /
> +                          SZ_512 * MMC_NUM_BOOT_PARTITION;
> +
> +       /* Defined by NVIDIA and used by Android devices. */
> +       *gpt_sector = card->ext_csd.sectors - boot_sectors_num - 1;
> +
> +       return 0;
> +}

I suggest you move this code into the mmc core/block layer instead (it
better belongs there).

Additionally, let's add a new host cap, MMC_CAP_ALTERNATIVE_GPT, to
let the core know when it should use the code above.

> +
>  static int sdhci_tegra_probe(struct platform_device *pdev)
>  {
>         const struct of_device_id *match;
> @@ -1616,6 +1654,10 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
>         tegra_host->pad_control_available = false;
>         tegra_host->soc_data = soc_data;
>
> +       if (soc_data->nvquirks & NVQUIRK_HAS_ANDROID_GPT_SECTOR)
> +               host->mmc_host_ops.alternative_gpt_sector =
> +                               sdhci_tegra_alternative_gpt_sector;
> +
>         if (soc_data->nvquirks & NVQUIRK_NEEDS_PAD_CONTROL) {
>                 rc = tegra_sdhci_init_pinctrl_info(&pdev->dev, tegra_host);
>                 if (rc == 0)
> --
> 2.32.0
>

Kind regards
Uffe

Re: [PATCH v5 4/5] mmc: sdhci-tegra: Implement alternative_gpt_sector()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4818 bytes --]

On Wed, Aug 18, 2021 at 11:55:05AM +0200, Ulf Hansson wrote:
> On Wed, 18 Aug 2021 at 02:57, Dmitry Osipenko <digetx@gmail.com> wrote:
> >
> > Tegra20/30/114/124 Android devices place GPT at a non-standard location.
> > Implement alternative_gpt_sector() callback of the MMC host ops which
> > specifies that GPT location for the partition scanner.
> >
> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > ---
> >  drivers/mmc/host/sdhci-tegra.c | 42 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >
> > diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> > index 387ce9cdbd7c..24a713689d5b 100644
> > --- a/drivers/mmc/host/sdhci-tegra.c
> > +++ b/drivers/mmc/host/sdhci-tegra.c
> > @@ -116,6 +116,8 @@
> >   */
> >  #define NVQUIRK_HAS_TMCLK                              BIT(10)
> >
> > +#define NVQUIRK_HAS_ANDROID_GPT_SECTOR                 BIT(11)
> > +
> >  /* SDMMC CQE Base Address for Tegra Host Ver 4.1 and Higher */
> >  #define SDHCI_TEGRA_CQE_BASE_ADDR                      0xF000
> >
> > @@ -1361,6 +1363,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra20 = {
> >         .pdata = &sdhci_tegra20_pdata,
> >         .dma_mask = DMA_BIT_MASK(32),
> >         .nvquirks = NVQUIRK_FORCE_SDHCI_SPEC_200 |
> > +                   NVQUIRK_HAS_ANDROID_GPT_SECTOR |
> >                     NVQUIRK_ENABLE_BLOCK_GAP_DET,
> >  };
> >
> > @@ -1390,6 +1393,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra30 = {
> >         .nvquirks = NVQUIRK_ENABLE_SDHCI_SPEC_300 |
> >                     NVQUIRK_ENABLE_SDR50 |
> >                     NVQUIRK_ENABLE_SDR104 |
> > +                   NVQUIRK_HAS_ANDROID_GPT_SECTOR |
> >                     NVQUIRK_HAS_PADCALIB,
> >  };
> >
> > @@ -1422,6 +1426,7 @@ static const struct sdhci_pltfm_data sdhci_tegra114_pdata = {
> >  static const struct sdhci_tegra_soc_data soc_data_tegra114 = {
> >         .pdata = &sdhci_tegra114_pdata,
> >         .dma_mask = DMA_BIT_MASK(32),
> > +       .nvquirks = NVQUIRK_HAS_ANDROID_GPT_SECTOR,
> >  };
> >
> >  static const struct sdhci_pltfm_data sdhci_tegra124_pdata = {
> > @@ -1438,6 +1443,7 @@ static const struct sdhci_pltfm_data sdhci_tegra124_pdata = {
> >  static const struct sdhci_tegra_soc_data soc_data_tegra124 = {
> >         .pdata = &sdhci_tegra124_pdata,
> >         .dma_mask = DMA_BIT_MASK(34),
> > +       .nvquirks = NVQUIRK_HAS_ANDROID_GPT_SECTOR,
> >  };
> >
> >  static const struct sdhci_ops tegra210_sdhci_ops = {
> > @@ -1590,6 +1596,38 @@ static int sdhci_tegra_add_host(struct sdhci_host *host)
> >         return ret;
> >  }
> >
> > +static int sdhci_tegra_alternative_gpt_sector(struct mmc_card *card,
> > +                                             sector_t *gpt_sector)
> > +{
> > +       unsigned int boot_sectors_num;
> > +
> > +       /* filter out unrelated cards */
> > +       if (card->ext_csd.rev < 3 ||
> > +           !mmc_card_mmc(card) ||
> > +           !mmc_card_is_blockaddr(card) ||
> > +            mmc_card_is_removable(card->host))
> > +               return -ENOENT;
> > +
> > +       /*
> > +        * eMMC storage has two special boot partitions in addition to the
> > +        * main one.  NVIDIA's bootloader linearizes eMMC boot0->boot1->main
> > +        * accesses, this means that the partition table addresses are shifted
> > +        * by the size of boot partitions.  In accordance with the eMMC
> > +        * specification, the boot partition size is calculated as follows:
> > +        *
> > +        *      boot partition size = 128K byte x BOOT_SIZE_MULT
> > +        *
> > +        * Calculate number of sectors occupied by the both boot partitions.
> > +        */
> > +       boot_sectors_num = card->ext_csd.raw_boot_mult * SZ_128K /
> > +                          SZ_512 * MMC_NUM_BOOT_PARTITION;
> > +
> > +       /* Defined by NVIDIA and used by Android devices. */
> > +       *gpt_sector = card->ext_csd.sectors - boot_sectors_num - 1;
> > +
> > +       return 0;
> > +}
> 
> I suggest you move this code into the mmc core/block layer instead (it
> better belongs there).
> 
> Additionally, let's add a new host cap, MMC_CAP_ALTERNATIVE_GPT, to
> let the core know when it should use the code above.

Couldn't a generic "alternative GPT" mean pretty much anything? As far
as I know this is very specific to a series of Tegra chips and firmware
running on them. On some of these devices you can even replace the OEM
firmware by something custom that's less quirky.

I'm not aware of anyone else employing this kind of quirk, so I don't
want anyone to get any ideas that this is a good thing. Putting it into
the core runs the risk of legitimizing this.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 4/5] mmc: sdhci-tegra: Implement alternative_gpt_sector()

[Dmitry Osipenko]

18.08.2021 16:35, Thierry Reding пишет:
>>> +static int sdhci_tegra_alternative_gpt_sector(struct mmc_card *card,
>>> +                                             sector_t *gpt_sector)
>>> +{
>>> +       unsigned int boot_sectors_num;
>>> +
>>> +       /* filter out unrelated cards */
>>> +       if (card->ext_csd.rev < 3 ||
>>> +           !mmc_card_mmc(card) ||
>>> +           !mmc_card_is_blockaddr(card) ||
>>> +            mmc_card_is_removable(card->host))
>>> +               return -ENOENT;
>>> +
>>> +       /*
>>> +        * eMMC storage has two special boot partitions in addition to the
>>> +        * main one.  NVIDIA's bootloader linearizes eMMC boot0->boot1->main
>>> +        * accesses, this means that the partition table addresses are shifted
>>> +        * by the size of boot partitions.  In accordance with the eMMC
>>> +        * specification, the boot partition size is calculated as follows:
>>> +        *
>>> +        *      boot partition size = 128K byte x BOOT_SIZE_MULT
>>> +        *
>>> +        * Calculate number of sectors occupied by the both boot partitions.
>>> +        */
>>> +       boot_sectors_num = card->ext_csd.raw_boot_mult * SZ_128K /
>>> +                          SZ_512 * MMC_NUM_BOOT_PARTITION;
>>> +
>>> +       /* Defined by NVIDIA and used by Android devices. */
>>> +       *gpt_sector = card->ext_csd.sectors - boot_sectors_num - 1;
>>> +
>>> +       return 0;
>>> +}
>> I suggest you move this code into the mmc core/block layer instead (it
>> better belongs there).
>>
>> Additionally, let's add a new host cap, MMC_CAP_ALTERNATIVE_GPT, to
>> let the core know when it should use the code above.
> Couldn't a generic "alternative GPT" mean pretty much anything? As far
> as I know this is very specific to a series of Tegra chips and firmware
> running on them. On some of these devices you can even replace the OEM
> firmware by something custom that's less quirky.
> 
> I'm not aware of anyone else employing this kind of quirk, so I don't
> want anyone to get any ideas that this is a good thing. Putting it into
> the core runs the risk of legitimizing this.

I also think it's better to keep it internal to Tegra. Ulf, could you
please clarify why do you want to have it moved into the core? Are you
aware of any other platforms that want exactly the same quirk? Thierry
should be correct that it's relevant only to Tegra SoCs.

Regarding the 'legitimizing', it's not a bad thing to me at all. If we
want to run more devices with a mainline kernel, then such quirks are
inevitable.

Re: [PATCH v5 4/5] mmc: sdhci-tegra: Implement alternative_gpt_sector()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3067 bytes --]

On Wed, Aug 18, 2021 at 07:15:43PM +0300, Dmitry Osipenko wrote:
> 18.08.2021 16:35, Thierry Reding пишет:
> >>> +static int sdhci_tegra_alternative_gpt_sector(struct mmc_card *card,
> >>> +                                             sector_t *gpt_sector)
> >>> +{
> >>> +       unsigned int boot_sectors_num;
> >>> +
> >>> +       /* filter out unrelated cards */
> >>> +       if (card->ext_csd.rev < 3 ||
> >>> +           !mmc_card_mmc(card) ||
> >>> +           !mmc_card_is_blockaddr(card) ||
> >>> +            mmc_card_is_removable(card->host))
> >>> +               return -ENOENT;
> >>> +
> >>> +       /*
> >>> +        * eMMC storage has two special boot partitions in addition to the
> >>> +        * main one.  NVIDIA's bootloader linearizes eMMC boot0->boot1->main
> >>> +        * accesses, this means that the partition table addresses are shifted
> >>> +        * by the size of boot partitions.  In accordance with the eMMC
> >>> +        * specification, the boot partition size is calculated as follows:
> >>> +        *
> >>> +        *      boot partition size = 128K byte x BOOT_SIZE_MULT
> >>> +        *
> >>> +        * Calculate number of sectors occupied by the both boot partitions.
> >>> +        */
> >>> +       boot_sectors_num = card->ext_csd.raw_boot_mult * SZ_128K /
> >>> +                          SZ_512 * MMC_NUM_BOOT_PARTITION;
> >>> +
> >>> +       /* Defined by NVIDIA and used by Android devices. */
> >>> +       *gpt_sector = card->ext_csd.sectors - boot_sectors_num - 1;
> >>> +
> >>> +       return 0;
> >>> +}
> >> I suggest you move this code into the mmc core/block layer instead (it
> >> better belongs there).
> >>
> >> Additionally, let's add a new host cap, MMC_CAP_ALTERNATIVE_GPT, to
> >> let the core know when it should use the code above.
> > Couldn't a generic "alternative GPT" mean pretty much anything? As far
> > as I know this is very specific to a series of Tegra chips and firmware
> > running on them. On some of these devices you can even replace the OEM
> > firmware by something custom that's less quirky.
> > 
> > I'm not aware of anyone else employing this kind of quirk, so I don't
> > want anyone to get any ideas that this is a good thing. Putting it into
> > the core runs the risk of legitimizing this.
> 
> I also think it's better to keep it internal to Tegra. Ulf, could you
> please clarify why do you want to have it moved into the core? Are you
> aware of any other platforms that want exactly the same quirk? Thierry
> should be correct that it's relevant only to Tegra SoCs.
> 
> Regarding the 'legitimizing', it's not a bad thing to me at all. If we
> want to run more devices with a mainline kernel, then such quirks are
> inevitable.

That's not what I meant. What I meant to say is that putting it into the
core might give somebody else the idea that this is actually a good
thing and therefore they might end up implementing a similar quirk. That
is definitely not something we want to encourage.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 4/5] mmc: sdhci-tegra: Implement alternative_gpt_sector()

[Ulf Hansson]

On Wed, 18 Aug 2021 at 15:35, Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Wed, Aug 18, 2021 at 11:55:05AM +0200, Ulf Hansson wrote:
> > On Wed, 18 Aug 2021 at 02:57, Dmitry Osipenko <digetx@gmail.com> wrote:
> > >
> > > Tegra20/30/114/124 Android devices place GPT at a non-standard location.
> > > Implement alternative_gpt_sector() callback of the MMC host ops which
> > > specifies that GPT location for the partition scanner.
> > >
> > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > > ---
> > >  drivers/mmc/host/sdhci-tegra.c | 42 ++++++++++++++++++++++++++++++++++
> > >  1 file changed, 42 insertions(+)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> > > index 387ce9cdbd7c..24a713689d5b 100644
> > > --- a/drivers/mmc/host/sdhci-tegra.c
> > > +++ b/drivers/mmc/host/sdhci-tegra.c
> > > @@ -116,6 +116,8 @@
> > >   */
> > >  #define NVQUIRK_HAS_TMCLK                              BIT(10)
> > >
> > > +#define NVQUIRK_HAS_ANDROID_GPT_SECTOR                 BIT(11)
> > > +
> > >  /* SDMMC CQE Base Address for Tegra Host Ver 4.1 and Higher */
> > >  #define SDHCI_TEGRA_CQE_BASE_ADDR                      0xF000
> > >
> > > @@ -1361,6 +1363,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra20 = {
> > >         .pdata = &sdhci_tegra20_pdata,
> > >         .dma_mask = DMA_BIT_MASK(32),
> > >         .nvquirks = NVQUIRK_FORCE_SDHCI_SPEC_200 |
> > > +                   NVQUIRK_HAS_ANDROID_GPT_SECTOR |
> > >                     NVQUIRK_ENABLE_BLOCK_GAP_DET,
> > >  };
> > >
> > > @@ -1390,6 +1393,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra30 = {
> > >         .nvquirks = NVQUIRK_ENABLE_SDHCI_SPEC_300 |
> > >                     NVQUIRK_ENABLE_SDR50 |
> > >                     NVQUIRK_ENABLE_SDR104 |
> > > +                   NVQUIRK_HAS_ANDROID_GPT_SECTOR |
> > >                     NVQUIRK_HAS_PADCALIB,
> > >  };
> > >
> > > @@ -1422,6 +1426,7 @@ static const struct sdhci_pltfm_data sdhci_tegra114_pdata = {
> > >  static const struct sdhci_tegra_soc_data soc_data_tegra114 = {
> > >         .pdata = &sdhci_tegra114_pdata,
> > >         .dma_mask = DMA_BIT_MASK(32),
> > > +       .nvquirks = NVQUIRK_HAS_ANDROID_GPT_SECTOR,
> > >  };
> > >
> > >  static const struct sdhci_pltfm_data sdhci_tegra124_pdata = {
> > > @@ -1438,6 +1443,7 @@ static const struct sdhci_pltfm_data sdhci_tegra124_pdata = {
> > >  static const struct sdhci_tegra_soc_data soc_data_tegra124 = {
> > >         .pdata = &sdhci_tegra124_pdata,
> > >         .dma_mask = DMA_BIT_MASK(34),
> > > +       .nvquirks = NVQUIRK_HAS_ANDROID_GPT_SECTOR,
> > >  };
> > >
> > >  static const struct sdhci_ops tegra210_sdhci_ops = {
> > > @@ -1590,6 +1596,38 @@ static int sdhci_tegra_add_host(struct sdhci_host *host)
> > >         return ret;
> > >  }
> > >
> > > +static int sdhci_tegra_alternative_gpt_sector(struct mmc_card *card,
> > > +                                             sector_t *gpt_sector)
> > > +{
> > > +       unsigned int boot_sectors_num;
> > > +
> > > +       /* filter out unrelated cards */
> > > +       if (card->ext_csd.rev < 3 ||
> > > +           !mmc_card_mmc(card) ||
> > > +           !mmc_card_is_blockaddr(card) ||
> > > +            mmc_card_is_removable(card->host))
> > > +               return -ENOENT;
> > > +
> > > +       /*
> > > +        * eMMC storage has two special boot partitions in addition to the
> > > +        * main one.  NVIDIA's bootloader linearizes eMMC boot0->boot1->main
> > > +        * accesses, this means that the partition table addresses are shifted
> > > +        * by the size of boot partitions.  In accordance with the eMMC
> > > +        * specification, the boot partition size is calculated as follows:
> > > +        *
> > > +        *      boot partition size = 128K byte x BOOT_SIZE_MULT
> > > +        *
> > > +        * Calculate number of sectors occupied by the both boot partitions.
> > > +        */
> > > +       boot_sectors_num = card->ext_csd.raw_boot_mult * SZ_128K /
> > > +                          SZ_512 * MMC_NUM_BOOT_PARTITION;
> > > +
> > > +       /* Defined by NVIDIA and used by Android devices. */
> > > +       *gpt_sector = card->ext_csd.sectors - boot_sectors_num - 1;
> > > +
> > > +       return 0;
> > > +}
> >
> > I suggest you move this code into the mmc core/block layer instead (it
> > better belongs there).
> >
> > Additionally, let's add a new host cap, MMC_CAP_ALTERNATIVE_GPT, to
> > let the core know when it should use the code above.
>
> Couldn't a generic "alternative GPT" mean pretty much anything? As far
> as I know this is very specific to a series of Tegra chips and firmware
> running on them. On some of these devices you can even replace the OEM
> firmware by something custom that's less quirky.

Good point!

Perhaps naming the cap MMC_CAP_TEGRA_GPT would make this more clear.

>
> I'm not aware of anyone else employing this kind of quirk, so I don't
> want anyone to get any ideas that this is a good thing. Putting it into
> the core runs the risk of legitimizing this.

I certainly don't want to legitimize this. But no matter what, that is
exactly what we are doing, anyways.

In summary, I still prefer code to be put in their proper layers, and
there aren't any host specific things going on here, except for
parsing a compatible string.

Kind regards
Uffe

Re: [PATCH v5 4/5] mmc: sdhci-tegra: Implement alternative_gpt_sector()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6775 bytes --]

On Thu, Aug 19, 2021 at 03:20:57PM +0200, Ulf Hansson wrote:
> On Wed, 18 Aug 2021 at 15:35, Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Wed, Aug 18, 2021 at 11:55:05AM +0200, Ulf Hansson wrote:
> > > On Wed, 18 Aug 2021 at 02:57, Dmitry Osipenko <digetx@gmail.com> wrote:
> > > >
> > > > Tegra20/30/114/124 Android devices place GPT at a non-standard location.
> > > > Implement alternative_gpt_sector() callback of the MMC host ops which
> > > > specifies that GPT location for the partition scanner.
> > > >
> > > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > > > ---
> > > >  drivers/mmc/host/sdhci-tegra.c | 42 ++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 42 insertions(+)
> > > >
> > > > diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> > > > index 387ce9cdbd7c..24a713689d5b 100644
> > > > --- a/drivers/mmc/host/sdhci-tegra.c
> > > > +++ b/drivers/mmc/host/sdhci-tegra.c
> > > > @@ -116,6 +116,8 @@
> > > >   */
> > > >  #define NVQUIRK_HAS_TMCLK                              BIT(10)
> > > >
> > > > +#define NVQUIRK_HAS_ANDROID_GPT_SECTOR                 BIT(11)
> > > > +
> > > >  /* SDMMC CQE Base Address for Tegra Host Ver 4.1 and Higher */
> > > >  #define SDHCI_TEGRA_CQE_BASE_ADDR                      0xF000
> > > >
> > > > @@ -1361,6 +1363,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra20 = {
> > > >         .pdata = &sdhci_tegra20_pdata,
> > > >         .dma_mask = DMA_BIT_MASK(32),
> > > >         .nvquirks = NVQUIRK_FORCE_SDHCI_SPEC_200 |
> > > > +                   NVQUIRK_HAS_ANDROID_GPT_SECTOR |
> > > >                     NVQUIRK_ENABLE_BLOCK_GAP_DET,
> > > >  };
> > > >
> > > > @@ -1390,6 +1393,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra30 = {
> > > >         .nvquirks = NVQUIRK_ENABLE_SDHCI_SPEC_300 |
> > > >                     NVQUIRK_ENABLE_SDR50 |
> > > >                     NVQUIRK_ENABLE_SDR104 |
> > > > +                   NVQUIRK_HAS_ANDROID_GPT_SECTOR |
> > > >                     NVQUIRK_HAS_PADCALIB,
> > > >  };
> > > >
> > > > @@ -1422,6 +1426,7 @@ static const struct sdhci_pltfm_data sdhci_tegra114_pdata = {
> > > >  static const struct sdhci_tegra_soc_data soc_data_tegra114 = {
> > > >         .pdata = &sdhci_tegra114_pdata,
> > > >         .dma_mask = DMA_BIT_MASK(32),
> > > > +       .nvquirks = NVQUIRK_HAS_ANDROID_GPT_SECTOR,
> > > >  };
> > > >
> > > >  static const struct sdhci_pltfm_data sdhci_tegra124_pdata = {
> > > > @@ -1438,6 +1443,7 @@ static const struct sdhci_pltfm_data sdhci_tegra124_pdata = {
> > > >  static const struct sdhci_tegra_soc_data soc_data_tegra124 = {
> > > >         .pdata = &sdhci_tegra124_pdata,
> > > >         .dma_mask = DMA_BIT_MASK(34),
> > > > +       .nvquirks = NVQUIRK_HAS_ANDROID_GPT_SECTOR,
> > > >  };
> > > >
> > > >  static const struct sdhci_ops tegra210_sdhci_ops = {
> > > > @@ -1590,6 +1596,38 @@ static int sdhci_tegra_add_host(struct sdhci_host *host)
> > > >         return ret;
> > > >  }
> > > >
> > > > +static int sdhci_tegra_alternative_gpt_sector(struct mmc_card *card,
> > > > +                                             sector_t *gpt_sector)
> > > > +{
> > > > +       unsigned int boot_sectors_num;
> > > > +
> > > > +       /* filter out unrelated cards */
> > > > +       if (card->ext_csd.rev < 3 ||
> > > > +           !mmc_card_mmc(card) ||
> > > > +           !mmc_card_is_blockaddr(card) ||
> > > > +            mmc_card_is_removable(card->host))
> > > > +               return -ENOENT;
> > > > +
> > > > +       /*
> > > > +        * eMMC storage has two special boot partitions in addition to the
> > > > +        * main one.  NVIDIA's bootloader linearizes eMMC boot0->boot1->main
> > > > +        * accesses, this means that the partition table addresses are shifted
> > > > +        * by the size of boot partitions.  In accordance with the eMMC
> > > > +        * specification, the boot partition size is calculated as follows:
> > > > +        *
> > > > +        *      boot partition size = 128K byte x BOOT_SIZE_MULT
> > > > +        *
> > > > +        * Calculate number of sectors occupied by the both boot partitions.
> > > > +        */
> > > > +       boot_sectors_num = card->ext_csd.raw_boot_mult * SZ_128K /
> > > > +                          SZ_512 * MMC_NUM_BOOT_PARTITION;
> > > > +
> > > > +       /* Defined by NVIDIA and used by Android devices. */
> > > > +       *gpt_sector = card->ext_csd.sectors - boot_sectors_num - 1;
> > > > +
> > > > +       return 0;
> > > > +}
> > >
> > > I suggest you move this code into the mmc core/block layer instead (it
> > > better belongs there).
> > >
> > > Additionally, let's add a new host cap, MMC_CAP_ALTERNATIVE_GPT, to
> > > let the core know when it should use the code above.
> >
> > Couldn't a generic "alternative GPT" mean pretty much anything? As far
> > as I know this is very specific to a series of Tegra chips and firmware
> > running on them. On some of these devices you can even replace the OEM
> > firmware by something custom that's less quirky.
> 
> Good point!
> 
> Perhaps naming the cap MMC_CAP_TEGRA_GPT would make this more clear.

Yeah, that sounds like a better name. Or if people are hung up on
"alternative", perhaps MMC_CAP_ALTERNATIVE_GPT_TEGRA.

> > I'm not aware of anyone else employing this kind of quirk, so I don't
> > want anyone to get any ideas that this is a good thing. Putting it into
> > the core runs the risk of legitimizing this.
> 
> I certainly don't want to legitimize this. But no matter what, that is
> exactly what we are doing, anyways.

I think there's a difference between supporting a quirk and legitimizing
it. I certainly would hate for anyone to come across this "feature" and
then go: "Oh, this is neat, let's implement this on our new platform!".

> In summary, I still prefer code to be put in their proper layers, and
> there aren't any host specific things going on here, except for
> parsing a compatible string.

Fair enough. Perhaps if we put enough warnings in the comments
surrounding this and are vigilant enough during code review we can
prevent this from proliferating. Obviously, once somebody implements
this in their flash/boot stack, it can become difficult to change it,
so by the time we get to review the kernel bits it might already be
set in stone.

Then again, like you hinted at already, once we support it, we support
it. So no real harm is done if anyone copies this.

I don't exactly know how this came about in the first place, but it's
pretty exotic, so I doubt that anyone else will come up with something
like this anytime soon.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 4/5] mmc: sdhci-tegra: Implement alternative_gpt_sector()

[Ulf Hansson]

On Thu, 19 Aug 2021 at 19:19, Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Thu, Aug 19, 2021 at 03:20:57PM +0200, Ulf Hansson wrote:
> > On Wed, 18 Aug 2021 at 15:35, Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > On Wed, Aug 18, 2021 at 11:55:05AM +0200, Ulf Hansson wrote:
> > > > On Wed, 18 Aug 2021 at 02:57, Dmitry Osipenko <digetx@gmail.com> wrote:
> > > > >
> > > > > Tegra20/30/114/124 Android devices place GPT at a non-standard location.
> > > > > Implement alternative_gpt_sector() callback of the MMC host ops which
> > > > > specifies that GPT location for the partition scanner.
> > > > >
> > > > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > > > > ---
> > > > >  drivers/mmc/host/sdhci-tegra.c | 42 ++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 42 insertions(+)
> > > > >
> > > > > diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> > > > > index 387ce9cdbd7c..24a713689d5b 100644
> > > > > --- a/drivers/mmc/host/sdhci-tegra.c
> > > > > +++ b/drivers/mmc/host/sdhci-tegra.c
> > > > > @@ -116,6 +116,8 @@
> > > > >   */
> > > > >  #define NVQUIRK_HAS_TMCLK                              BIT(10)
> > > > >
> > > > > +#define NVQUIRK_HAS_ANDROID_GPT_SECTOR                 BIT(11)
> > > > > +
> > > > >  /* SDMMC CQE Base Address for Tegra Host Ver 4.1 and Higher */
> > > > >  #define SDHCI_TEGRA_CQE_BASE_ADDR                      0xF000
> > > > >
> > > > > @@ -1361,6 +1363,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra20 = {
> > > > >         .pdata = &sdhci_tegra20_pdata,
> > > > >         .dma_mask = DMA_BIT_MASK(32),
> > > > >         .nvquirks = NVQUIRK_FORCE_SDHCI_SPEC_200 |
> > > > > +                   NVQUIRK_HAS_ANDROID_GPT_SECTOR |
> > > > >                     NVQUIRK_ENABLE_BLOCK_GAP_DET,
> > > > >  };
> > > > >
> > > > > @@ -1390,6 +1393,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra30 = {
> > > > >         .nvquirks = NVQUIRK_ENABLE_SDHCI_SPEC_300 |
> > > > >                     NVQUIRK_ENABLE_SDR50 |
> > > > >                     NVQUIRK_ENABLE_SDR104 |
> > > > > +                   NVQUIRK_HAS_ANDROID_GPT_SECTOR |
> > > > >                     NVQUIRK_HAS_PADCALIB,
> > > > >  };
> > > > >
> > > > > @@ -1422,6 +1426,7 @@ static const struct sdhci_pltfm_data sdhci_tegra114_pdata = {
> > > > >  static const struct sdhci_tegra_soc_data soc_data_tegra114 = {
> > > > >         .pdata = &sdhci_tegra114_pdata,
> > > > >         .dma_mask = DMA_BIT_MASK(32),
> > > > > +       .nvquirks = NVQUIRK_HAS_ANDROID_GPT_SECTOR,
> > > > >  };
> > > > >
> > > > >  static const struct sdhci_pltfm_data sdhci_tegra124_pdata = {
> > > > > @@ -1438,6 +1443,7 @@ static const struct sdhci_pltfm_data sdhci_tegra124_pdata = {
> > > > >  static const struct sdhci_tegra_soc_data soc_data_tegra124 = {
> > > > >         .pdata = &sdhci_tegra124_pdata,
> > > > >         .dma_mask = DMA_BIT_MASK(34),
> > > > > +       .nvquirks = NVQUIRK_HAS_ANDROID_GPT_SECTOR,
> > > > >  };
> > > > >
> > > > >  static const struct sdhci_ops tegra210_sdhci_ops = {
> > > > > @@ -1590,6 +1596,38 @@ static int sdhci_tegra_add_host(struct sdhci_host *host)
> > > > >         return ret;
> > > > >  }
> > > > >
> > > > > +static int sdhci_tegra_alternative_gpt_sector(struct mmc_card *card,
> > > > > +                                             sector_t *gpt_sector)
> > > > > +{
> > > > > +       unsigned int boot_sectors_num;
> > > > > +
> > > > > +       /* filter out unrelated cards */
> > > > > +       if (card->ext_csd.rev < 3 ||
> > > > > +           !mmc_card_mmc(card) ||
> > > > > +           !mmc_card_is_blockaddr(card) ||
> > > > > +            mmc_card_is_removable(card->host))
> > > > > +               return -ENOENT;
> > > > > +
> > > > > +       /*
> > > > > +        * eMMC storage has two special boot partitions in addition to the
> > > > > +        * main one.  NVIDIA's bootloader linearizes eMMC boot0->boot1->main
> > > > > +        * accesses, this means that the partition table addresses are shifted
> > > > > +        * by the size of boot partitions.  In accordance with the eMMC
> > > > > +        * specification, the boot partition size is calculated as follows:
> > > > > +        *
> > > > > +        *      boot partition size = 128K byte x BOOT_SIZE_MULT
> > > > > +        *
> > > > > +        * Calculate number of sectors occupied by the both boot partitions.
> > > > > +        */
> > > > > +       boot_sectors_num = card->ext_csd.raw_boot_mult * SZ_128K /
> > > > > +                          SZ_512 * MMC_NUM_BOOT_PARTITION;
> > > > > +
> > > > > +       /* Defined by NVIDIA and used by Android devices. */
> > > > > +       *gpt_sector = card->ext_csd.sectors - boot_sectors_num - 1;
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > >
> > > > I suggest you move this code into the mmc core/block layer instead (it
> > > > better belongs there).
> > > >
> > > > Additionally, let's add a new host cap, MMC_CAP_ALTERNATIVE_GPT, to
> > > > let the core know when it should use the code above.
> > >
> > > Couldn't a generic "alternative GPT" mean pretty much anything? As far
> > > as I know this is very specific to a series of Tegra chips and firmware
> > > running on them. On some of these devices you can even replace the OEM
> > > firmware by something custom that's less quirky.
> >
> > Good point!
> >
> > Perhaps naming the cap MMC_CAP_TEGRA_GPT would make this more clear.
>
> Yeah, that sounds like a better name. Or if people are hung up on
> "alternative", perhaps MMC_CAP_ALTERNATIVE_GPT_TEGRA.

That works too. Dmitry can pick what he prefers.

>
> > > I'm not aware of anyone else employing this kind of quirk, so I don't
> > > want anyone to get any ideas that this is a good thing. Putting it into
> > > the core runs the risk of legitimizing this.
> >
> > I certainly don't want to legitimize this. But no matter what, that is
> > exactly what we are doing, anyways.
>
> I think there's a difference between supporting a quirk and legitimizing
> it. I certainly would hate for anyone to come across this "feature" and
> then go: "Oh, this is neat, let's implement this on our new platform!".
>
> > In summary, I still prefer code to be put in their proper layers, and
> > there aren't any host specific things going on here, except for
> > parsing a compatible string.
>
> Fair enough. Perhaps if we put enough warnings in the comments
> surrounding this and are vigilant enough during code review we can
> prevent this from proliferating. Obviously, once somebody implements
> this in their flash/boot stack, it can become difficult to change it,
> so by the time we get to review the kernel bits it might already be
> set in stone.

Sure, good idea. Some recommendations in the form of comments in the
code would be nice.

>
> Then again, like you hinted at already, once we support it, we support
> it. So no real harm is done if anyone copies this.
>
> I don't exactly know how this came about in the first place, but it's
> pretty exotic, so I doubt that anyone else will come up with something
> like this anytime soon.

Hopefully, but who knows. :-)

In the end, I think a lot of these homebrewed flash layouts, have been
invented to store bootbinararies in a robust way, tolerating data loss
and sudden power failures.

That said, let me take the opportunity to highlight the work
ARM/Linaro is doing on EBBR [1]. We should have done that many years
ago, but better late than never.

Kind regards
Uffe

[1] EBBR - Embedded Base Boot Requirements
https://github.com/ARM-software/ebbr

[PATCH v5 5/5] partitions/efi: Support non-standard GPT location

[Dmitry Osipenko]

Support looking up GPT at a non-standard location specified by a block
device driver.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 block/partitions/efi.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/block/partitions/efi.c b/block/partitions/efi.c
index aaa3dc487cb5..b9509f445b3c 100644
--- a/block/partitions/efi.c
+++ b/block/partitions/efi.c
@@ -585,6 +585,8 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
 	gpt_header *pgpt = NULL, *agpt = NULL;
 	gpt_entry *pptes = NULL, *aptes = NULL;
 	legacy_mbr *legacymbr;
+	struct gendisk *disk = state->disk;
+	const struct block_device_operations *fops = disk->fops;
 	sector_t total_sectors = get_capacity(state->disk);
 	u64 lastlba;
 
@@ -619,6 +621,17 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
         if (!good_agpt && force_gpt)
                 good_agpt = is_gpt_valid(state, lastlba, &agpt, &aptes);
 
+	if (!good_agpt && force_gpt && fops->alternative_gpt_sector) {
+		struct block_device *bdev = disk->part0;
+		sector_t agpt_sector;
+		int err;
+
+		err = fops->alternative_gpt_sector(bdev, &agpt_sector);
+		if (!err)
+			good_agpt = is_gpt_valid(state, agpt_sector,
+						 &agpt, &aptes);
+	}
+
         /* The obviously unsuccessful case */
         if (!good_pgpt && !good_agpt)
                 goto fail;
-- 
2.32.0

Re: [PATCH v5 5/5] partitions/efi: Support non-standard GPT location

[Christoph Hellwig]

On Wed, Aug 18, 2021 at 03:55:47AM +0300, Dmitry Osipenko wrote:
> Support looking up GPT at a non-standard location specified by a block
> device driver.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  block/partitions/efi.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/block/partitions/efi.c b/block/partitions/efi.c
> index aaa3dc487cb5..b9509f445b3c 100644
> --- a/block/partitions/efi.c
> +++ b/block/partitions/efi.c
> @@ -585,6 +585,8 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
>  	gpt_header *pgpt = NULL, *agpt = NULL;
>  	gpt_entry *pptes = NULL, *aptes = NULL;
>  	legacy_mbr *legacymbr;
> +	struct gendisk *disk = state->disk;
> +	const struct block_device_operations *fops = disk->fops;
>  	sector_t total_sectors = get_capacity(state->disk);
>  	u64 lastlba;
>  
> @@ -619,6 +621,17 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
>          if (!good_agpt && force_gpt)
>                  good_agpt = is_gpt_valid(state, lastlba, &agpt, &aptes);
>  
> +	if (!good_agpt && force_gpt && fops->alternative_gpt_sector) {
> +		struct block_device *bdev = disk->part0;
> +		sector_t agpt_sector;
> +		int err;
> +
> +		err = fops->alternative_gpt_sector(bdev, &agpt_sector);

Please call the method with the disk as the argument.  I've been moving
the block layer to generally pass the gendisk whenever we're dealing
with the whole device, as that makes the intent very clear.

Also do we really need the force_gpt check?  That is we always require
the user to pass a command line argument to use this?

Re: [PATCH v5 5/5] partitions/efi: Support non-standard GPT location

[Dmitry Osipenko]

18.08.2021 08:25, Christoph Hellwig пишет:
> On Wed, Aug 18, 2021 at 03:55:47AM +0300, Dmitry Osipenko wrote:
>> Support looking up GPT at a non-standard location specified by a block
>> device driver.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  block/partitions/efi.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/block/partitions/efi.c b/block/partitions/efi.c
>> index aaa3dc487cb5..b9509f445b3c 100644
>> --- a/block/partitions/efi.c
>> +++ b/block/partitions/efi.c
>> @@ -585,6 +585,8 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
>>  	gpt_header *pgpt = NULL, *agpt = NULL;
>>  	gpt_entry *pptes = NULL, *aptes = NULL;
>>  	legacy_mbr *legacymbr;
>> +	struct gendisk *disk = state->disk;
>> +	const struct block_device_operations *fops = disk->fops;
>>  	sector_t total_sectors = get_capacity(state->disk);
>>  	u64 lastlba;
>>  
>> @@ -619,6 +621,17 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
>>          if (!good_agpt && force_gpt)
>>                  good_agpt = is_gpt_valid(state, lastlba, &agpt, &aptes);
>>  
>> +	if (!good_agpt && force_gpt && fops->alternative_gpt_sector) {
>> +		struct block_device *bdev = disk->part0;
>> +		sector_t agpt_sector;
>> +		int err;
>> +
>> +		err = fops->alternative_gpt_sector(bdev, &agpt_sector);
> 
> Please call the method with the disk as the argument.  I've been moving
> the block layer to generally pass the gendisk whenever we're dealing
> with the whole device, as that makes the intent very clear.

I'll change it in v6.

> Also do we really need the force_gpt check?  That is we always require
> the user to pass a command line argument to use this?

User indeed must pass the 'gpt' argument to kernel cmdline. That's what
all those Android devices do. Should be okay to keep that requirement.