| Message ID | 20241220222612.1757884-7-trini@konsulko.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Rework the BLK symbol usage in Kconfig | expand |
On Fri, 20 Dec 2024 at 22:27, Tom Rini <trini@konsulko.com> wrote: > Now that block drivers are all selecting the BLK symbol, there's no need > for other options to be select'ing BLK so that other required > functionality can be enabled. Remove these places. > > Signed-off-by: Tom Rini <trini@konsulko.com> > Reviewed-by: Peter Robinson <pbrobinson@gmail.com> > --- > arch/arm/Kconfig | 4 ---- > arch/arm/mach-exynos/Kconfig | 4 ---- > arch/arm/mach-imx/mx5/Kconfig | 2 +- > arch/arm/mach-s5pc1xx/Kconfig | 1 - > 4 files changed, 1 insertion(+), 10 deletions(-) > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index ea256f687867..86b37e8d5f8d 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -1371,7 +1371,6 @@ config ARCH_VEXPRESS64 > select PL01X_SERIAL > select OF_CONTROL > select CLK > - select BLK > select MTD_NOR_FLASH if MTD > select FLASH_CFI_DRIVER if MTD > select ENV_IS_IN_FLASH if MTD > @@ -1981,7 +1980,6 @@ config ARCH_STM32 > > config ARCH_STI > bool "Support STMicroelectronics SoCs" > - select BLK > select CPU_V7A > select DM > select DM_RESET > @@ -2028,7 +2026,6 @@ config ARCH_STM32MP > > config ARCH_ROCKCHIP > bool "Support Rockchip SoCs" > - select BLK > select BINMAN if SPL_OPTEE || SPL > select DM > select DM_GPIO > @@ -2116,7 +2113,6 @@ config TARGET_POMELO > select AHCI > select SCSI_AHCI > select AHCI_PCI > - select BLK > select PCI > select DM_PCI > select SCSI > diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig > index 7e6959510087..28193039cb84 100644 > --- a/arch/arm/mach-exynos/Kconfig > +++ b/arch/arm/mach-exynos/Kconfig > @@ -18,7 +18,6 @@ config ARCH_EXYNOS4 > bool "Exynos4 SoC family" > select BOARD_EARLY_INIT_F > select CPU_V7A > - select BLK > select MMC > help > Samsung Exynos4 SoC family are based on ARM Cortex-A9 CPU. There > @@ -39,7 +38,6 @@ config ARCH_EXYNOS5 > imply USB_ETHER_ASIX > imply USB_ETHER_RTL8152 > imply USB_ETHER_SMSC95XX > - select BLK > select MMC > > help > @@ -51,7 +49,6 @@ config ARCH_EXYNOS7 > bool "Exynos7 SoC family" > select ARM64 > select BOARD_EARLY_INIT_F > - select BLK > select MMC > help > Samsung Exynos7 SoC family are based on ARM Cortex-A57 CPU or > @@ -61,7 +58,6 @@ config ARCH_EXYNOS7 > config ARCH_EXYNOS9 > bool "Exynos9 SoC family" > select ARM64 > - select BLK > select MMC > help > Samsung Exynos9 SoC family are based on ARMv8 Cortex CPU. There > are > diff --git a/arch/arm/mach-imx/mx5/Kconfig b/arch/arm/mach-imx/mx5/Kconfig > index 4d1e07b14d32..0dfd68496875 100644 > --- a/arch/arm/mach-imx/mx5/Kconfig > +++ b/arch/arm/mach-imx/mx5/Kconfig > @@ -26,7 +26,7 @@ config TARGET_KP_IMX53 > select DM_I2C > select DM_PMIC > select DM_SERIAL > - select BLK > + select DM_MMC > select DM_REGULATOR > select MMC > select MX53 > diff --git a/arch/arm/mach-s5pc1xx/Kconfig b/arch/arm/mach-s5pc1xx/Kconfig > index b15b2e7b6a01..d8b85f80e631 100644 > --- a/arch/arm/mach-s5pc1xx/Kconfig > +++ b/arch/arm/mach-s5pc1xx/Kconfig > @@ -7,7 +7,6 @@ choice > config TARGET_S5P_GONI > bool "S5P Goni board" > select OF_CONTROL > - select BLK > select MISC_COMMON > select MMC > > -- > 2.43.0 > >
Hi Tom, On 12/20/24 11:22 PM, Tom Rini wrote: > Now that block drivers are all selecting the BLK symbol, there's no need > for other options to be select'ing BLK so that other required > functionality can be enabled. Remove these places. > We have multiple commands depending on the BLK symbol. BOOTSTD also depends on it, but I assume we should be able to network boot without HW block drivers? CMD_UFETCH wouldn't be usable without those drivers as well. Should we do something about that by making them not depend on BLK e.g. use CONFIG_IS_ENABLED in the right places? Not sure if all devicess based on those archs have at least one HW block driver enabled. I guess checking if all .config before and after that change are identical would help us figure out if this could introduce a regression? Cheers, Quentin
On Tue, Jan 14, 2025 at 02:53:48PM +0100, Quentin Schulz wrote: > Hi Tom, > > On 12/20/24 11:22 PM, Tom Rini wrote: > > Now that block drivers are all selecting the BLK symbol, there's no need > > for other options to be select'ing BLK so that other required > > functionality can be enabled. Remove these places. > > > > We have multiple commands depending on the BLK symbol. Yes. > BOOTSTD also depends on it, but I assume we should be able to network boot > without HW block drivers? Correct. That's part of the motivation for this series (which I wasn't clear enough about on its own). Without something like this series if we remove the BLK dependency from BOOTSTD then some other platforms fail to build or grow a bunch in size (as BOOTSTD is default y and now it's enabled on those platforms). > CMD_UFETCH wouldn't be usable without those drivers as well. > > Should we do something about that by making them not depend on BLK e.g. use > CONFIG_IS_ENABLED in the right places? Not sure if all devicess based on > those archs have at least one HW block driver enabled. I guess checking if > all .config before and after that change are identical would help us figure > out if this could introduce a regression? There's a few options, depending on what the command is. For CMD_UFETCH it's likely that a small restructure would be needed to not try and print block devices while preserving the rest of the formatting. For CMD_LSBLK / CMD_CLONE it's an unfortunate short-hand for "some block device exists" which is functionally what those commands require. Thanks for reviewing the whole series.
Hi Tom, On 1/14/25 5:59 PM, Tom Rini wrote: > On Tue, Jan 14, 2025 at 02:53:48PM +0100, Quentin Schulz wrote: >> Hi Tom, >> >> On 12/20/24 11:22 PM, Tom Rini wrote: >>> Now that block drivers are all selecting the BLK symbol, there's no need >>> for other options to be select'ing BLK so that other required >>> functionality can be enabled. Remove these places. >>> >> >> We have multiple commands depending on the BLK symbol. > > Yes. > >> BOOTSTD also depends on it, but I assume we should be able to network boot >> without HW block drivers? > > Correct. That's part of the motivation for this series (which I wasn't > clear enough about on its own). Without something like this series if we > remove the BLK dependency from BOOTSTD then some other platforms fail to > build or grow a bunch in size (as BOOTSTD is default y and now it's > enabled on those platforms). > >> CMD_UFETCH wouldn't be usable without those drivers as well. >> >> Should we do something about that by making them not depend on BLK e.g. use >> CONFIG_IS_ENABLED in the right places? Not sure if all devicess based on >> those archs have at least one HW block driver enabled. I guess checking if >> all .config before and after that change are identical would help us figure >> out if this could introduce a regression? > > There's a few options, depending on what the command is. For CMD_UFETCH > it's likely that a small restructure would be needed to not try and My point is that I believe this patch is too hastily removing the select BLK because some symbols have "depends on BLK" and by removing the select, we make those symbols unselectable. This can cascade if other symbols depend on those now unselectable symbols. Also, removing the select BLK from architecture/target symbols can introduce regressions if BLK really is required? I think a reasonable (albeit cumbersome) solution is to migrate all those selects to the impacted defconfigs so that effectively no change is made for existing devices. If maintainers want to remove BLK, they could then do it later. This also makes sure that BOOTSTD and CMD_UFETCH (and others) are still enabled, since BLK would still be enabled, just from a different location. Then another patch series could remove BOOTSTD dependency on BLK by adapting the code, same for CMD_UFETCH (and others). Does this make sense? Am I missing something? > print block devices while preserving the rest of the formatting. For > CMD_LSBLK / CMD_CLONE it's an unfortunate short-hand for "some block > device exists" which is functionally what those commands require. > Yes, we need to keep the depends on BLK for those. Cheers, Quentin
On Wed, Jan 15, 2025 at 06:49:45PM +0100, Quentin Schulz wrote: > Hi Tom, > > On 1/14/25 5:59 PM, Tom Rini wrote: > > On Tue, Jan 14, 2025 at 02:53:48PM +0100, Quentin Schulz wrote: > > > Hi Tom, > > > > > > On 12/20/24 11:22 PM, Tom Rini wrote: > > > > Now that block drivers are all selecting the BLK symbol, there's no need > > > > for other options to be select'ing BLK so that other required > > > > functionality can be enabled. Remove these places. > > > > > > > > > > We have multiple commands depending on the BLK symbol. > > > > Yes. > > > > > BOOTSTD also depends on it, but I assume we should be able to network boot > > > without HW block drivers? > > > > Correct. That's part of the motivation for this series (which I wasn't > > clear enough about on its own). Without something like this series if we > > remove the BLK dependency from BOOTSTD then some other platforms fail to > > build or grow a bunch in size (as BOOTSTD is default y and now it's > > enabled on those platforms). > > > > > CMD_UFETCH wouldn't be usable without those drivers as well. > > > > > > Should we do something about that by making them not depend on BLK e.g. use > > > CONFIG_IS_ENABLED in the right places? Not sure if all devicess based on > > > those archs have at least one HW block driver enabled. I guess checking if > > > all .config before and after that change are identical would help us figure > > > out if this could introduce a regression? > > > > There's a few options, depending on what the command is. For CMD_UFETCH > > it's likely that a small restructure would be needed to not try and > > My point is that I believe this patch is too hastily removing the select BLK > because some symbols have "depends on BLK" and by removing the select, we > make those symbols unselectable. This can cascade if other symbols depend on > those now unselectable symbols. > > Also, removing the select BLK from architecture/target symbols can introduce > regressions if BLK really is required? > > I think a reasonable (albeit cumbersome) solution is to migrate all those > selects to the impacted defconfigs so that effectively no change is made for > existing devices. If maintainers want to remove BLK, they could then do it > later. > > This also makes sure that BOOTSTD and CMD_UFETCH (and others) are still > enabled, since BLK would still be enabled, just from a different location. > Then another patch series could remove BOOTSTD dependency on BLK by adapting > the code, same for CMD_UFETCH (and others). > > Does this make sense? Am I missing something? What you're missing, I believe, is that this patch exists because prior to the first patch in the series and also going back to when BLK was optional, if something higher level like ARCH_ROCKCHIP didn't select BLK then it wouldn't be able to prompt about its MMC driver. This is similar to all of the high level places that "select DM" as that used to be meaningful but now it's not. As part of testing the series I did my world build before/after and the only change is that as noted in the cover letter, espresso7420 now has MMC again. Does this help? Thanks.
Hi Tom, On 1/15/25 9:20 PM, Tom Rini wrote: > On Wed, Jan 15, 2025 at 06:49:45PM +0100, Quentin Schulz wrote: >> Hi Tom, >> >> On 1/14/25 5:59 PM, Tom Rini wrote: >>> On Tue, Jan 14, 2025 at 02:53:48PM +0100, Quentin Schulz wrote: >>>> Hi Tom, >>>> >>>> On 12/20/24 11:22 PM, Tom Rini wrote: >>>>> Now that block drivers are all selecting the BLK symbol, there's no need >>>>> for other options to be select'ing BLK so that other required >>>>> functionality can be enabled. Remove these places. >>>>> >>>> >>>> We have multiple commands depending on the BLK symbol. >>> >>> Yes. >>> >>>> BOOTSTD also depends on it, but I assume we should be able to network boot >>>> without HW block drivers? >>> >>> Correct. That's part of the motivation for this series (which I wasn't >>> clear enough about on its own). Without something like this series if we >>> remove the BLK dependency from BOOTSTD then some other platforms fail to >>> build or grow a bunch in size (as BOOTSTD is default y and now it's >>> enabled on those platforms). >>> >>>> CMD_UFETCH wouldn't be usable without those drivers as well. >>>> >>>> Should we do something about that by making them not depend on BLK e.g. use >>>> CONFIG_IS_ENABLED in the right places? Not sure if all devicess based on >>>> those archs have at least one HW block driver enabled. I guess checking if >>>> all .config before and after that change are identical would help us figure >>>> out if this could introduce a regression? >>> >>> There's a few options, depending on what the command is. For CMD_UFETCH >>> it's likely that a small restructure would be needed to not try and >> >> My point is that I believe this patch is too hastily removing the select BLK >> because some symbols have "depends on BLK" and by removing the select, we >> make those symbols unselectable. This can cascade if other symbols depend on >> those now unselectable symbols. >> >> Also, removing the select BLK from architecture/target symbols can introduce >> regressions if BLK really is required? >> >> I think a reasonable (albeit cumbersome) solution is to migrate all those >> selects to the impacted defconfigs so that effectively no change is made for >> existing devices. If maintainers want to remove BLK, they could then do it >> later. >> >> This also makes sure that BOOTSTD and CMD_UFETCH (and others) are still >> enabled, since BLK would still be enabled, just from a different location. >> Then another patch series could remove BOOTSTD dependency on BLK by adapting >> the code, same for CMD_UFETCH (and others). >> >> Does this make sense? Am I missing something? > > What you're missing, I believe, is that this patch exists because prior > to the first patch in the series and also going back to when BLK was > optional, if something higher level like ARCH_ROCKCHIP didn't select BLK > then it wouldn't be able to prompt about its MMC driver. This is similar > to all of the high level places that "select DM" as that used to be > meaningful but now it's not. As part of testing the series I did my > world build before/after and the only change is that as noted in the > cover letter, espresso7420 now has MMC again. Does this help? Thanks. > Please add this to the patch commit log as well. The cover letter content doesn't make it to the git history :) But with the world build you've done, I'm now confident what I was worried about will not happen, therefore: Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de> Thanks! Quentin
On Thu, Jan 16, 2025 at 10:21:36AM +0100, Quentin Schulz wrote: > Hi Tom, > > On 1/15/25 9:20 PM, Tom Rini wrote: > > On Wed, Jan 15, 2025 at 06:49:45PM +0100, Quentin Schulz wrote: > > > Hi Tom, > > > > > > On 1/14/25 5:59 PM, Tom Rini wrote: > > > > On Tue, Jan 14, 2025 at 02:53:48PM +0100, Quentin Schulz wrote: > > > > > Hi Tom, > > > > > > > > > > On 12/20/24 11:22 PM, Tom Rini wrote: > > > > > > Now that block drivers are all selecting the BLK symbol, there's no need > > > > > > for other options to be select'ing BLK so that other required > > > > > > functionality can be enabled. Remove these places. > > > > > > > > > > > > > > > > We have multiple commands depending on the BLK symbol. > > > > > > > > Yes. > > > > > > > > > BOOTSTD also depends on it, but I assume we should be able to network boot > > > > > without HW block drivers? > > > > > > > > Correct. That's part of the motivation for this series (which I wasn't > > > > clear enough about on its own). Without something like this series if we > > > > remove the BLK dependency from BOOTSTD then some other platforms fail to > > > > build or grow a bunch in size (as BOOTSTD is default y and now it's > > > > enabled on those platforms). > > > > > > > > > CMD_UFETCH wouldn't be usable without those drivers as well. > > > > > > > > > > Should we do something about that by making them not depend on BLK e.g. use > > > > > CONFIG_IS_ENABLED in the right places? Not sure if all devicess based on > > > > > those archs have at least one HW block driver enabled. I guess checking if > > > > > all .config before and after that change are identical would help us figure > > > > > out if this could introduce a regression? > > > > > > > > There's a few options, depending on what the command is. For CMD_UFETCH > > > > it's likely that a small restructure would be needed to not try and > > > > > > My point is that I believe this patch is too hastily removing the select BLK > > > because some symbols have "depends on BLK" and by removing the select, we > > > make those symbols unselectable. This can cascade if other symbols depend on > > > those now unselectable symbols. > > > > > > Also, removing the select BLK from architecture/target symbols can introduce > > > regressions if BLK really is required? > > > > > > I think a reasonable (albeit cumbersome) solution is to migrate all those > > > selects to the impacted defconfigs so that effectively no change is made for > > > existing devices. If maintainers want to remove BLK, they could then do it > > > later. > > > > > > This also makes sure that BOOTSTD and CMD_UFETCH (and others) are still > > > enabled, since BLK would still be enabled, just from a different location. > > > Then another patch series could remove BOOTSTD dependency on BLK by adapting > > > the code, same for CMD_UFETCH (and others). > > > > > > Does this make sense? Am I missing something? > > > > What you're missing, I believe, is that this patch exists because prior > > to the first patch in the series and also going back to when BLK was > > optional, if something higher level like ARCH_ROCKCHIP didn't select BLK > > then it wouldn't be able to prompt about its MMC driver. This is similar > > to all of the high level places that "select DM" as that used to be > > meaningful but now it's not. As part of testing the series I did my > > world build before/after and the only change is that as noted in the > > cover letter, espresso7420 now has MMC again. Does this help? Thanks. > > > > Please add this to the patch commit log as well. The cover letter content > doesn't make it to the git history :) > > But with the world build you've done, I'm now confident what I was worried > about will not happen, therefore: > > Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de> Thanks, I'll reword this slightly to include the above when applying.
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index ea256f687867..86b37e8d5f8d 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -1371,7 +1371,6 @@ config ARCH_VEXPRESS64 select PL01X_SERIAL select OF_CONTROL select CLK - select BLK select MTD_NOR_FLASH if MTD select FLASH_CFI_DRIVER if MTD select ENV_IS_IN_FLASH if MTD @@ -1981,7 +1980,6 @@ config ARCH_STM32 config ARCH_STI bool "Support STMicroelectronics SoCs" - select BLK select CPU_V7A select DM select DM_RESET @@ -2028,7 +2026,6 @@ config ARCH_STM32MP config ARCH_ROCKCHIP bool "Support Rockchip SoCs" - select BLK select BINMAN if SPL_OPTEE || SPL select DM select DM_GPIO @@ -2116,7 +2113,6 @@ config TARGET_POMELO select AHCI select SCSI_AHCI select AHCI_PCI - select BLK select PCI select DM_PCI select SCSI diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig index 7e6959510087..28193039cb84 100644 --- a/arch/arm/mach-exynos/Kconfig +++ b/arch/arm/mach-exynos/Kconfig @@ -18,7 +18,6 @@ config ARCH_EXYNOS4 bool "Exynos4 SoC family" select BOARD_EARLY_INIT_F select CPU_V7A - select BLK select MMC help Samsung Exynos4 SoC family are based on ARM Cortex-A9 CPU. There @@ -39,7 +38,6 @@ config ARCH_EXYNOS5 imply USB_ETHER_ASIX imply USB_ETHER_RTL8152 imply USB_ETHER_SMSC95XX - select BLK select MMC help @@ -51,7 +49,6 @@ config ARCH_EXYNOS7 bool "Exynos7 SoC family" select ARM64 select BOARD_EARLY_INIT_F - select BLK select MMC help Samsung Exynos7 SoC family are based on ARM Cortex-A57 CPU or @@ -61,7 +58,6 @@ config ARCH_EXYNOS7 config ARCH_EXYNOS9 bool "Exynos9 SoC family" select ARM64 - select BLK select MMC help Samsung Exynos9 SoC family are based on ARMv8 Cortex CPU. There are diff --git a/arch/arm/mach-imx/mx5/Kconfig b/arch/arm/mach-imx/mx5/Kconfig index 4d1e07b14d32..0dfd68496875 100644 --- a/arch/arm/mach-imx/mx5/Kconfig +++ b/arch/arm/mach-imx/mx5/Kconfig @@ -26,7 +26,7 @@ config TARGET_KP_IMX53 select DM_I2C select DM_PMIC select DM_SERIAL - select BLK + select DM_MMC select DM_REGULATOR select MMC select MX53 diff --git a/arch/arm/mach-s5pc1xx/Kconfig b/arch/arm/mach-s5pc1xx/Kconfig index b15b2e7b6a01..d8b85f80e631 100644 --- a/arch/arm/mach-s5pc1xx/Kconfig +++ b/arch/arm/mach-s5pc1xx/Kconfig @@ -7,7 +7,6 @@ choice config TARGET_S5P_GONI bool "S5P Goni board" select OF_CONTROL - select BLK select MISC_COMMON select MMC
Now that block drivers are all selecting the BLK symbol, there's no need for other options to be select'ing BLK so that other required functionality can be enabled. Remove these places. Signed-off-by: Tom Rini <trini@konsulko.com> --- arch/arm/Kconfig | 4 ---- arch/arm/mach-exynos/Kconfig | 4 ---- arch/arm/mach-imx/mx5/Kconfig | 2 +- arch/arm/mach-s5pc1xx/Kconfig | 1 - 4 files changed, 1 insertion(+), 10 deletions(-)