| Message ID | 20230404180515.49821-2-marex@denx.de |
|---|---|
| State | New |
| Delegated to: | Jaehoon Chung |
| Headers | show |
| Series | [v3,1/2] spl: mmc: Pass eMMC HW partition and SW partition to spl_mmc_get_uboot_raw_sector() | expand |
On Tuesday 04 April 2023 20:05:15 Marek Vasut wrote: > The eMMC HW partition 0 and 7 both mean USER HW partition. This is not truth! > Use this as > a mean of propagating A/B copy selection within USER HW partition. The > spl_mmc_get_uboot_raw_sector() can detect that a USER HW partition is > in use and based on whether it is 0 or 7, select appropriate sector to > load from. > > Signed-off-by: Fedor Ross <fedor.ross@ifm.com> > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Jaehoon Chung <jh80.chung@samsung.com> > Cc: Peng Fan <peng.fan@nxp.com> > --- > V2: Initialize the part variable, else it is passed uninitialized to > mmc_load_image_raw_sector(..raw_sect + spl_mmc_raw_uboot_offset(part)); > and that prevents pine64 from booting. Interestingly enough, there > is no warning emitted, which sounds like a compiler bug. > V3: Update on u-boot/master > --- > common/spl/spl_mmc.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c > index f1f87d78ba7..865c395b9fe 100644 > --- a/common/spl/spl_mmc.c > +++ b/common/spl/spl_mmc.c > @@ -412,7 +412,7 @@ int spl_mmc_load(struct spl_image_info *spl_image, > static struct mmc *mmc; > u32 boot_mode; > int err = 0; > - __maybe_unused int hw_part = 0; > + __maybe_unused int part = 0, hw_part = 0; > int mmc_dev; > > /* Perform peripheral init only once for an mmc device */ > @@ -437,11 +437,12 @@ int spl_mmc_load(struct spl_image_info *spl_image, > switch (boot_mode) { > case MMCSD_MODE_EMMCBOOT: > hw_part = spl_mmc_emmc_boot_partition(mmc); > + part = hw_part == 7 ? 0 : hw_part; ... and so this change will break accessing eMMC part 7 / GP4. > > if (CONFIG_IS_ENABLED(MMC_TINY)) > - err = mmc_switch_part(mmc, hw_part); > + err = mmc_switch_part(mmc, part); > else > - err = blk_dselect_hwpart(mmc_get_blk_desc(mmc), hw_part); > + err = blk_dselect_hwpart(mmc_get_blk_desc(mmc), part); > > if (err) { > #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT > @@ -471,7 +472,7 @@ int spl_mmc_load(struct spl_image_info *spl_image, > #endif > #ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR > err = mmc_load_image_raw_sector(spl_image, bootdev, mmc, > - raw_sect + spl_mmc_raw_uboot_offset(hw_part)); > + raw_sect + spl_mmc_raw_uboot_offset(part)); > if (!err) > return err; > #endif > -- > 2.39.2 >
On 4/4/23 21:25, Pali Rohár wrote: > On Tuesday 04 April 2023 20:05:15 Marek Vasut wrote: >> The eMMC HW partition 0 and 7 both mean USER HW partition. > > This is not truth! Can you please provide further details to back your claim ? This kind of screaming feedback with zero additional information is worthless, sorry. Before you proceed with any further replies, please have a look at JEDEC JESD84-B51 section 7.4.69 PARTITION_CONFIG Bit[5:3] BOOT_PARTITION_ENABLE , this is what is being discussed here. Value 1/2 is either BOOT HW partition, value 0/7 is treated as USER HW partition by the boot code.
On Wednesday 05 April 2023 00:11:17 Marek Vasut wrote: > On 4/4/23 21:25, Pali Rohár wrote: > > On Tuesday 04 April 2023 20:05:15 Marek Vasut wrote: > > > The eMMC HW partition 0 and 7 both mean USER HW partition. > > > > This is not truth! > > Can you please provide further details to back your claim ? Yes, see a patch with explanation which I meanwhile sent: https://patchwork.ozlabs.org/project/uboot/patch/20230404202805.8523-1-pali@kernel.org/ > This kind of screaming feedback with zero additional information is > worthless, sorry. Before you proceed with any further replies, please have a > look at JEDEC JESD84-B51 section 7.4.69 PARTITION_CONFIG Bit[5:3] > BOOT_PARTITION_ENABLE , this is what is being discussed here. Value 1/2 is > either BOOT HW partition, value 0/7 is treated as USER HW partition by the > boot code.
On 4/5/23 00:16, Pali Rohár wrote: > On Wednesday 05 April 2023 00:11:17 Marek Vasut wrote: >> On 4/4/23 21:25, Pali Rohár wrote: >>> On Tuesday 04 April 2023 20:05:15 Marek Vasut wrote: >>>> The eMMC HW partition 0 and 7 both mean USER HW partition. >>> >>> This is not truth! >> >> Can you please provide further details to back your claim ? > > Yes, see a patch with explanation which I meanwhile sent: > https://patchwork.ozlabs.org/project/uboot/patch/20230404202805.8523-1-pali@kernel.org/ This very much adds a comment that looks like the content of the second paragraph of my reply, see below. It is a good thing you mention the aforementioned patch, since exactly one line past the changes you implemented is the following piece of code: https://source.denx.de/u-boot/u-boot/-/blob/master/common/spl/spl_mmc.c#L382 " int default_spl_mmc_emmc_boot_partition(struct mmc *mmc) ... part = (mmc->part_config >> 3) & PART_ACCESS_MASK; if (part == 7) part = 0; " Which maps 7 to 0 . >> This kind of screaming feedback with zero additional information is >> worthless, sorry. Before you proceed with any further replies, please have a >> look at JEDEC JESD84-B51 section 7.4.69 PARTITION_CONFIG Bit[5:3] >> BOOT_PARTITION_ENABLE , this is what is being discussed here. Value 1/2 is >> either BOOT HW partition, value 0/7 is treated as USER HW partition by the >> boot code. Please read the paragraph above, I do not see any reply to it and I think that might put the conversation back on track.
On Wednesday 05 April 2023 00:33:02 Marek Vasut wrote: > On 4/5/23 00:16, Pali Rohár wrote: > > On Wednesday 05 April 2023 00:11:17 Marek Vasut wrote: > > > On 4/4/23 21:25, Pali Rohár wrote: > > > > On Tuesday 04 April 2023 20:05:15 Marek Vasut wrote: > > > > > The eMMC HW partition 0 and 7 both mean USER HW partition. > > > > > > > > This is not truth! > > > > > > Can you please provide further details to back your claim ? > > > > Yes, see a patch with explanation which I meanwhile sent: > > https://patchwork.ozlabs.org/project/uboot/patch/20230404202805.8523-1-pali@kernel.org/ > > This very much adds a comment that looks like the content of the second > paragraph of my reply, see below. > > It is a good thing you mention the aforementioned patch, since exactly one > line past the changes you implemented is the following piece of code: > > https://source.denx.de/u-boot/u-boot/-/blob/master/common/spl/spl_mmc.c#L382 > > " > int default_spl_mmc_emmc_boot_partition(struct mmc *mmc) > ... > part = (mmc->part_config >> 3) & PART_ACCESS_MASK; > if (part == 7) > part = 0; > " > > Which maps 7 to 0 . > > > > This kind of screaming feedback with zero additional information is > > > worthless, sorry. Before you proceed with any further replies, please have a > > > look at JEDEC JESD84-B51 section 7.4.69 PARTITION_CONFIG Bit[5:3] > > > BOOT_PARTITION_ENABLE , this is what is being discussed here. Value 1/2 is > > > either BOOT HW partition, value 0/7 is treated as USER HW partition by the > > > boot code. > > Please read the paragraph above, I do not see any reply to it and I think > that might put the conversation back on track. I have read it and the reply with explanation is already there in my first email and also in the linked email. What you have written above in not truth.
On 4/5/23 00:44, Pali Rohár wrote: > On Wednesday 05 April 2023 00:33:02 Marek Vasut wrote: >> On 4/5/23 00:16, Pali Rohár wrote: >>> On Wednesday 05 April 2023 00:11:17 Marek Vasut wrote: >>>> On 4/4/23 21:25, Pali Rohár wrote: >>>>> On Tuesday 04 April 2023 20:05:15 Marek Vasut wrote: >>>>>> The eMMC HW partition 0 and 7 both mean USER HW partition. >>>>> >>>>> This is not truth! >>>> >>>> Can you please provide further details to back your claim ? >>> >>> Yes, see a patch with explanation which I meanwhile sent: >>> https://patchwork.ozlabs.org/project/uboot/patch/20230404202805.8523-1-pali@kernel.org/ >> >> This very much adds a comment that looks like the content of the second >> paragraph of my reply, see below. >> >> It is a good thing you mention the aforementioned patch, since exactly one >> line past the changes you implemented is the following piece of code: >> >> https://source.denx.de/u-boot/u-boot/-/blob/master/common/spl/spl_mmc.c#L382 >> >> " >> int default_spl_mmc_emmc_boot_partition(struct mmc *mmc) >> ... >> part = (mmc->part_config >> 3) & PART_ACCESS_MASK; >> if (part == 7) >> part = 0; >> " >> >> Which maps 7 to 0 . >> >>>> This kind of screaming feedback with zero additional information is >>>> worthless, sorry. Before you proceed with any further replies, please have a >>>> look at JEDEC JESD84-B51 section 7.4.69 PARTITION_CONFIG Bit[5:3] >>>> BOOT_PARTITION_ENABLE , this is what is being discussed here. Value 1/2 is >>>> either BOOT HW partition, value 0/7 is treated as USER HW partition by the >>>> boot code. >> >> Please read the paragraph above, I do not see any reply to it and I think >> that might put the conversation back on track. > > I have read it and the reply with explanation is already there in my > first email and also in the linked email. > > What you have written above in not truth. Very well, I think I will defer further judgement to the MMC maintainer and stop participating in this thread. Constructive feedback is welcome.
On Wednesday 05 April 2023 01:03:49 Marek Vasut wrote: > On 4/5/23 00:44, Pali Rohár wrote: > > On Wednesday 05 April 2023 00:33:02 Marek Vasut wrote: > > > On 4/5/23 00:16, Pali Rohár wrote: > > > > On Wednesday 05 April 2023 00:11:17 Marek Vasut wrote: > > > > > On 4/4/23 21:25, Pali Rohár wrote: > > > > > > On Tuesday 04 April 2023 20:05:15 Marek Vasut wrote: > > > > > > > The eMMC HW partition 0 and 7 both mean USER HW partition. > > > > > > > > > > > > This is not truth! > > > > > > > > > > Can you please provide further details to back your claim ? > > > > > > > > Yes, see a patch with explanation which I meanwhile sent: > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230404202805.8523-1-pali@kernel.org/ > > > > > > This very much adds a comment that looks like the content of the second > > > paragraph of my reply, see below. > > > > > > It is a good thing you mention the aforementioned patch, since exactly one > > > line past the changes you implemented is the following piece of code: > > > > > > https://source.denx.de/u-boot/u-boot/-/blob/master/common/spl/spl_mmc.c#L382 > > > > > > " > > > int default_spl_mmc_emmc_boot_partition(struct mmc *mmc) > > > ... > > > part = (mmc->part_config >> 3) & PART_ACCESS_MASK; > > > if (part == 7) > > > part = 0; > > > " > > > > > > Which maps 7 to 0 . > > > > > > > > This kind of screaming feedback with zero additional information is > > > > > worthless, sorry. Before you proceed with any further replies, please have a > > > > > look at JEDEC JESD84-B51 section 7.4.69 PARTITION_CONFIG Bit[5:3] > > > > > BOOT_PARTITION_ENABLE , this is what is being discussed here. Value 1/2 is > > > > > either BOOT HW partition, value 0/7 is treated as USER HW partition by the > > > > > boot code. > > > > > > Please read the paragraph above, I do not see any reply to it and I think > > > that might put the conversation back on track. > > > > I have read it and the reply with explanation is already there in my > > first email and also in the linked email. > > > > What you have written above in not truth. > > Very well, I think I will defer further judgement to the MMC maintainer and > stop participating in this thread. Constructive feedback is welcome. I see that this is your standard trolling reaction for a very long time if you do not have any relevant argument; or when you are lazy to read replies from other people.
On Wed, Apr 05, 2023 at 01:09:35AM +0200, Pali Rohár wrote: > On Wednesday 05 April 2023 01:03:49 Marek Vasut wrote: > > On 4/5/23 00:44, Pali Rohár wrote: > > > On Wednesday 05 April 2023 00:33:02 Marek Vasut wrote: > > > > On 4/5/23 00:16, Pali Rohár wrote: > > > > > On Wednesday 05 April 2023 00:11:17 Marek Vasut wrote: > > > > > > On 4/4/23 21:25, Pali Rohár wrote: > > > > > > > On Tuesday 04 April 2023 20:05:15 Marek Vasut wrote: > > > > > > > > The eMMC HW partition 0 and 7 both mean USER HW partition. > > > > > > > > > > > > > > This is not truth! > > > > > > > > > > > > Can you please provide further details to back your claim ? > > > > > > > > > > Yes, see a patch with explanation which I meanwhile sent: > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230404202805.8523-1-pali@kernel.org/ Please note that I'm not sure the comments in this patch are a correct reflection of what the code does, or is supposed to be doing. I need to go and study everything there again. > > > > This very much adds a comment that looks like the content of the second > > > > paragraph of my reply, see below. > > > > > > > > It is a good thing you mention the aforementioned patch, since exactly one > > > > line past the changes you implemented is the following piece of code: > > > > > > > > https://source.denx.de/u-boot/u-boot/-/blob/master/common/spl/spl_mmc.c#L382 > > > > > > > > " > > > > int default_spl_mmc_emmc_boot_partition(struct mmc *mmc) > > > > ... > > > > part = (mmc->part_config >> 3) & PART_ACCESS_MASK; > > > > if (part == 7) > > > > part = 0; > > > > " > > > > > > > > Which maps 7 to 0 . > > > > > > > > > > This kind of screaming feedback with zero additional information is > > > > > > worthless, sorry. Before you proceed with any further replies, please have a > > > > > > look at JEDEC JESD84-B51 section 7.4.69 PARTITION_CONFIG Bit[5:3] > > > > > > BOOT_PARTITION_ENABLE , this is what is being discussed here. Value 1/2 is > > > > > > either BOOT HW partition, value 0/7 is treated as USER HW partition by the > > > > > > boot code. > > > > > > > > Please read the paragraph above, I do not see any reply to it and I think > > > > that might put the conversation back on track. > > > > > > I have read it and the reply with explanation is already there in my > > > first email and also in the linked email. > > > > > > What you have written above in not truth. > > > > Very well, I think I will defer further judgement to the MMC maintainer and > > stop participating in this thread. Constructive feedback is welcome. > > I see that this is your standard trolling reaction for a very long time > if you do not have any relevant argument; or when you are lazy to read > replies from other people. That's uncalled for and rude, Pali. If you don't have anything further to add, technically, there's no need nor justification for such language.
diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c index f1f87d78ba7..865c395b9fe 100644 --- a/common/spl/spl_mmc.c +++ b/common/spl/spl_mmc.c @@ -412,7 +412,7 @@ int spl_mmc_load(struct spl_image_info *spl_image, static struct mmc *mmc; u32 boot_mode; int err = 0; - __maybe_unused int hw_part = 0; + __maybe_unused int part = 0, hw_part = 0; int mmc_dev; /* Perform peripheral init only once for an mmc device */ @@ -437,11 +437,12 @@ int spl_mmc_load(struct spl_image_info *spl_image, switch (boot_mode) { case MMCSD_MODE_EMMCBOOT: hw_part = spl_mmc_emmc_boot_partition(mmc); + part = hw_part == 7 ? 0 : hw_part; if (CONFIG_IS_ENABLED(MMC_TINY)) - err = mmc_switch_part(mmc, hw_part); + err = mmc_switch_part(mmc, part); else - err = blk_dselect_hwpart(mmc_get_blk_desc(mmc), hw_part); + err = blk_dselect_hwpart(mmc_get_blk_desc(mmc), part); if (err) { #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT @@ -471,7 +472,7 @@ int spl_mmc_load(struct spl_image_info *spl_image, #endif #ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR err = mmc_load_image_raw_sector(spl_image, bootdev, mmc, - raw_sect + spl_mmc_raw_uboot_offset(hw_part)); + raw_sect + spl_mmc_raw_uboot_offset(part)); if (!err) return err; #endif