| Message ID | 20210818005547.14497-5-digetx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support EFI partition on NVIDIA Tegra devices | expand |
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?
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.
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
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
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.
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
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
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
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
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)
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(+)