Re: [PATCH v5 38/44] freescale: Fix odd use of ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE

Tom Rini
2023-03-02T20:42:56Z

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

On Wed, Feb 22, 2023 at 09:34:19AM -0700, Simon Glass wrote:

> This is not a CONFIG option so we should not be using IS_ENABLED() on it,
> particularly not when it is not defined to anything, so shows up as
> calling IS_ENABLED() with no arguments.
>
> Just check it normally.
>
> For fsl_esdhc.c set it to 0 at the top of the file if it is not defined.
> This sort-of matches the behaviour in fsl_esdhc_mmc.c which seem to have
> a similar problem.
>
> This fixes a build error with split config on T2080QDS.
>
> Note: This should be handled in the devicetree.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> No size changes on affected boards:
>
> aarch64: w+ ls1088aqds ls1088aqds_qspi ls1088aqds_qspi_SECURE_BOOT ls1088aqds_sdcard_ifc ls1088aqds_sdcard_qspi ls1088aqds_tfa ls1088ardb_qspi ls1088ardb_qspi_SECURE_BOOT ls1088ardb_sdcard_qspi ls1088ardb_sdcard_qspi_SECURE_BOOT ls1088ardb_tfa ls1088ardb_tfa_SECURE_BOOT ls2080aqds ls2080aqds_nand ls2080aqds_qspi ls2080aqds_sdcard ls2080aqds_SECURE_BOOT ls2088aqds_tfa
> arm: w+ am57xx_hs_evm_usb
> powerpc: w+ P2041RDB P2041RDB_NAND P2041RDB_SDCARD P2041RDB_SPIFLASH T1024RDB T1024RDB_NAND T1024RDB_SDCARD T1024RDB_SPIFLASH T1042D4RDB T1042D4RDB_NAND T1042D4RDB_SDCARD T1042D4RDB_SPIFLASH T2080QDS T2080QDS_NAND T2080QDS_SDCARD T2080QDS_SECURE_BOOT T2080QDS_SPIFLASH T2080QDS_SRIO_PCIE_BOOT T2080RDB T2080RDB_NAND T2080RDB_revD T2080RDB_revD_NAND T2080RDB_revD_SDCARD T2080RDB_revD_SPIFLASH T2080RDB_SDCARD T2080RDB_SPIFLASH kmcent2
>
> Changes in v5:
> - Change approach and expand notes after more investigation
>
> arch/powerpc/include/asm/config_mpc85xx.h | 6 +++---
> drivers/mmc/fsl_esdhc.c | 6 +++++-
> drivers/mmc/fsl_esdhc_imx.c | 4 +++-
> 3 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/config_mpc85xx.h b/arch/powerpc/include/asm/config_mpc85xx.h
> index d990ecff96f..358ccef7f1d 100644
> --- a/arch/powerpc/include/asm/config_mpc85xx.h
> +++ b/arch/powerpc/include/asm/config_mpc85xx.h
> @@ -134,7 +134,7 @@
> #define CFG_FM_PLAT_CLK_DIV 1
> #define CFG_SYS_FM1_CLK CFG_FM_PLAT_CLK_DIV
> #define CFG_SYS_FM_MURAM_SIZE 0x30000
> -#define ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE
> +#define ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE 0
> #define QE_MURAM_SIZE 0x6000UL
> #define MAX_QE_RISC 1
> #define QE_NUM_OF_SNUM 28
> @@ -147,7 +147,7 @@
> #define CFG_SYS_FM1_CLK 0
> #define CFG_QBMAN_CLK_DIV 1
> #define CFG_SYS_FM_MURAM_SIZE 0x30000
> -#define ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE
> +#define ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE 0
> #define QE_MURAM_SIZE 0x6000UL
> #define MAX_QE_RISC 1
> #define QE_NUM_OF_SNUM 28
> @@ -166,7 +166,7 @@
> #define CFG_SYS_PME_CLK CFG_PME_PLAT_CLK_DIV
> #define CFG_SYS_FM1_CLK 0
> #define CFG_SYS_FM_MURAM_SIZE 0x28000
> -#define ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE
> +#define ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE 0
>
>
> #elif defined(CONFIG_ARCH_C29X)
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index d5066666698..ecb9c3b30c9 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -34,6 +34,10 @@
>
> DECLARE_GLOBAL_DATA_PTR;
>
> +#ifndef ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE
> +#define ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE 0
> +#endif
> +
> struct fsl_esdhc {
> uint dsaddr; /* SDMA system address register */
> uint blkattr; /* Block attributes register */
> @@ -325,7 +329,7 @@ static int esdhc_setup_data(struct fsl_esdhc_priv *priv, struct mmc *mmc,
> (timeout == 4 || timeout == 8 || timeout == 12))
> timeout++;
>
> - if (IS_ENABLED(ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE))
> + if (ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE)
> timeout = 0xE;
>
> esdhc_clrsetbits32(&regs->sysctl, SYSCTL_TIMEOUT_MASK, timeout << 16);
> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
> index 66caf683f74..9d9ffd0990a 100644
> --- a/drivers/mmc/fsl_esdhc_imx.c
> +++ b/drivers/mmc/fsl_esdhc_imx.c
> @@ -43,6 +43,8 @@
> #ifndef ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE
> #ifdef CONFIG_FSL_USDHC
> #define ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE 1
> +#else
> +#define ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE 0
> #endif
> #endif
>
> @@ -375,7 +377,7 @@ static int esdhc_setup_data(struct fsl_esdhc_priv *priv, struct mmc *mmc,
> (timeout == 4 || timeout == 8 || timeout == 12))
> timeout++;
>
> - if (IS_ENABLED(ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE))
> + if (ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE)
> timeout = 0xE;
>
> esdhc_clrsetbits32(&regs->sysctl, SYSCTL_TIMEOUT_MASK, timeout << 16);

Looking at commit 52faec31827e ("mmc: fsl_esdhc: replace most #ifdefs by
IS_ENABLED()") this is going backwards. The original logic was to define
ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE to set timeout to 0xE as
"ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE" is a boolean, not a value-holder.

--
Tom

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