[PATCH 13/25] spi: cadence_qspi: Migrate CONFIG_CQSPI_REF_CLK to Kconfig

[Pratyush Yadav]

On 31/03/22 01:41PM, Tom Rini wrote:
> On Thu, Mar 31, 2022 at 10:00:52PM +0530, Pratyush Yadav wrote:
> > +Vignesh
> > 
> > Hi Tom,
> > 
> > On 30/03/22 06:07PM, Tom Rini wrote:
> > > This is a little tricky since SoCFPGA has code to determine this as
> > > runtime.  Introduce a guard variable for platforms to select if they
> > > have a static value to use.  Then for ARCH_SOCFPGA, call
> > > cm_get_qspi_controller_clk_hz() and otherwise continue the previous
> > > behavior.
> > > 
> > > Cc: Jagan Teki <jagan at amarulasolutions.com>
> > > Signed-off-by: Tom Rini <trini at konsulko.com>
> > > ---
> > >  arch/arm/mach-socfpga/misc_soc64.c     | 1 +
> > >  configs/j7200_evm_a72_defconfig        | 2 ++
> > >  configs/j7200_evm_r5_defconfig         | 2 ++
> > >  configs/j721e_evm_a72_defconfig        | 2 ++
> > >  configs/j721e_evm_r5_defconfig         | 2 ++
> > >  configs/j721e_hs_evm_a72_defconfig     | 2 ++
> > >  configs/j721e_hs_evm_r5_defconfig      | 2 ++
> > >  configs/j721s2_evm_a72_defconfig       | 2 ++
> > >  configs/j721s2_evm_r5_defconfig        | 2 ++
> > >  configs/k2g_evm_defconfig              | 2 ++
> > >  configs/k2g_hs_evm_defconfig           | 2 ++
> > >  configs/stv0991_defconfig              | 2 ++
> > >  drivers/spi/Kconfig                    | 8 ++++++++
> > >  drivers/spi/cadence_qspi.c             | 4 +++-
> > >  drivers/spi/cadence_qspi.h             | 1 +
> > >  include/configs/j721e_evm.h            | 1 -
> > >  include/configs/j721s2_evm.h           | 1 -
> > >  include/configs/k2g_evm.h              | 4 ----
> > >  include/configs/socfpga_common.h       | 9 ---------
> > >  include/configs/socfpga_soc64_common.h | 5 -----
> > >  include/configs/stv0991.h              | 8 --------
> > >  21 files changed, 35 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/arch/arm/mach-socfpga/misc_soc64.c b/arch/arm/mach-socfpga/misc_soc64.c
> > > index 7b973a79e807..2acdfad07b35 100644
> > > --- a/arch/arm/mach-socfpga/misc_soc64.c
> > > +++ b/arch/arm/mach-socfpga/misc_soc64.c
> > > @@ -16,6 +16,7 @@
> > >  #include <errno.h>
> > >  #include <init.h>
> > >  #include <log.h>
> > > +#include <mach/clock_manager.h>
> > >  
> > >  DECLARE_GLOBAL_DATA_PTR;
> > >  
> > > diff --git a/configs/j7200_evm_a72_defconfig b/configs/j7200_evm_a72_defconfig
> > > index eb1d7d46b82a..3d0d1977ff99 100644
> > > --- a/configs/j7200_evm_a72_defconfig
> > > +++ b/configs/j7200_evm_a72_defconfig
> > > @@ -173,6 +173,8 @@ CONFIG_SOC_TI=y
> > >  CONFIG_SPI=y
> > >  CONFIG_DM_SPI=y
> > >  CONFIG_CADENCE_QSPI=y
> > > +CONFIG_HAS_CQSPI_REF_CLK=y
> > > +CONFIG_CQSPI_REF_CLK=133333333
> > >  CONFIG_SYSRESET=y
> > >  CONFIG_SPL_SYSRESET=y
> > >  CONFIG_SYSRESET_TI_SCI=y
> > > diff --git a/configs/j7200_evm_r5_defconfig b/configs/j7200_evm_r5_defconfig
> > > index e500a27bb692..0f4b006b80b5 100644
> > > --- a/configs/j7200_evm_r5_defconfig
> > > +++ b/configs/j7200_evm_r5_defconfig
> > > @@ -134,6 +134,8 @@ CONFIG_SOC_TI=y
> > >  CONFIG_SPI=y
> > >  CONFIG_DM_SPI=y
> > >  CONFIG_CADENCE_QSPI=y
> > > +CONFIG_HAS_CQSPI_REF_CLK=y
> > > +CONFIG_CQSPI_REF_CLK=133333333
> > 
> > This clock is configured to 166 MHz via device tree. I would prefer TI 
> > platforms to use device tree and not set CONFIG_HAS_CQSPI_REF_CLK at 
> > all. The clock _is_ 133 MHz by default but I think it is better if we 
> > just return an error when clk_get fails.
> 
> For these migration patches, it's important to not include functional
> changes as the same time in case there's problems later on (see for
> example the SYS_IMMR fix I merged today).  So please send a follow-up to
> this patch or something vs master to disable the current behavior on the
> TI platforms.

Okay. Makes sense.

> 
> > >  CONFIG_SYSRESET=y
> > >  CONFIG_SPL_SYSRESET=y
> > >  CONFIG_SYSRESET_TI_SCI=y
> > [...]
> > > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> > > index db680618ee9b..7209bb43a776 100644
> > > --- a/drivers/spi/cadence_qspi.c
> > > +++ b/drivers/spi/cadence_qspi.c
> > > @@ -188,8 +188,10 @@ static int cadence_spi_probe(struct udevice *bus)
> > >  	if (plat->ref_clk_hz == 0) {
> > >  		ret = clk_get_by_index(bus, 0, &clk);
> > >  		if (ret) {
> > > -#ifdef CONFIG_CQSPI_REF_CLK
> > > +#ifdef CONFIG_HAS_CQSPI_REF_CLK
> > >  			plat->ref_clk_hz = CONFIG_CQSPI_REF_CLK;
> > > +#elif defined(CONFIG_ARCH_SOCFPGA)
> > > +			plat->ref_clk_hz = cm_get_qspi_controller_clk_hz();
> > 
> > While you are here, please change all this to use if (IS_ENABLED()) 
> > instead.
> 
> In this case IS_ENABLED() does not increase readibity of the code nor
> increase static code coverage testing.

I disagree. I think it definitely improves readibity. In general I find 
IS_ENABLED() is easier to read compared to ifdefs because the if-else 
flow feels more natural than the ifdef-elif flow, which has weird 
indentation.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.