diff mbox series

[v1] spi: cadence_qspi: Probe fail if QSPI clock is not set

Message ID 20200805101838.180134-1-chee.hong.ang@intel.com
State New
Delegated to: Simon Goldschmidt
Headers show
Series [v1] spi: cadence_qspi: Probe fail if QSPI clock is not set | expand

Commit Message

Ang, Chee Hong Aug. 5, 2020, 10:18 a.m. UTC
If the QSPI clock is not set (read as 0), QSPI driver probe shall fail
and prevent further QSPI access.

Signed-off-by: Chee Hong Ang <chee.hong.ang@intel.com>
---
 drivers/spi/cadence_qspi.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Vignesh Raghavendra Aug. 5, 2020, 11:12 a.m. UTC | #1
Hi,

On 05/08/20 3:48 pm, Chee Hong Ang wrote:
> If the QSPI clock is not set (read as 0), QSPI driver probe shall fail
> and prevent further QSPI access.
> 
> Signed-off-by: Chee Hong Ang <chee.hong.ang@intel.com>
> ---
>   drivers/spi/cadence_qspi.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> index 1e85749209..3bb5c7031d 100644
> --- a/drivers/spi/cadence_qspi.c
> +++ b/drivers/spi/cadence_qspi.c
> @@ -170,6 +170,9 @@ static int cadence_spi_probe(struct udevice *bus)
>   	struct clk clk;
>   	int ret;
>   
> +	if (!CONFIG_CQSPI_REF_CLK)
> +		return -ENODEV;
> +

This seems broken to me...

Most platforms have moved away from passing clk freq via CONFIG_ and are
using more scalable DT or platdata approach where freq is obtained via
clk_get_*() APIs... See few lines below:

         if (plat->ref_clk_hz == 0) {
                 ret = clk_get_by_index(bus, 0, &clk);
                 if (ret) {
#ifdef CONFIG_CQSPI_REF_CLK
                         plat->ref_clk_hz = CONFIG_CQSPI_REF_CLK;
#else
                         return ret;
#endif
                 } else {
                         plat->ref_clk_hz = clk_get_rate(&clk);
                         clk_free(&clk);
                         if (IS_ERR_VALUE(plat->ref_clk_hz))
                                 return plat->ref_clk_hz;
                 }
         }

So there is no need to make CONFIG_CQSPI_REF_CLK mandatory.
Probe may fail only when CONFIG_CQSPI_REF_CLK is undefined and there is 
no reference to a valid clk device either.


>   	priv->regbase = plat->regbase;
>   	priv->ahbbase = plat->ahbbase;
>   
> 

Regards
Vignesh
Ang, Chee Hong Aug. 5, 2020, 11:26 a.m. UTC | #2
> Hi,
> 
> On 05/08/20 3:48 pm, Chee Hong Ang wrote:
> > If the QSPI clock is not set (read as 0), QSPI driver probe shall fail
> > and prevent further QSPI access.
> >
> > Signed-off-by: Chee Hong Ang <chee.hong.ang@intel.com>
> > ---
> >   drivers/spi/cadence_qspi.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> > index 1e85749209..3bb5c7031d 100644
> > --- a/drivers/spi/cadence_qspi.c
> > +++ b/drivers/spi/cadence_qspi.c
> > @@ -170,6 +170,9 @@ static int cadence_spi_probe(struct udevice *bus)
> >   	struct clk clk;
> >   	int ret;
> >
> > +	if (!CONFIG_CQSPI_REF_CLK)
> > +		return -ENODEV;
> > +
> 
> This seems broken to me...
> 
> Most platforms have moved away from passing clk freq via CONFIG_ and are
> using more scalable DT or platdata approach where freq is obtained via

I think the proper way to resolve this is to change our clock driver to DM. Thanks.

> clk_get_*() APIs... See few lines below:
> 
>          if (plat->ref_clk_hz == 0) {
>                  ret = clk_get_by_index(bus, 0, &clk);
>                  if (ret) {
> #ifdef CONFIG_CQSPI_REF_CLK
>                          plat->ref_clk_hz = CONFIG_CQSPI_REF_CLK; #else
>                          return ret;
> #endif
>                  } else {
>                          plat->ref_clk_hz = clk_get_rate(&clk);
>                          clk_free(&clk);
>                          if (IS_ERR_VALUE(plat->ref_clk_hz))
>                                  return plat->ref_clk_hz;
>                  }
>          }
> 
> So there is no need to make CONFIG_CQSPI_REF_CLK mandatory.
> Probe may fail only when CONFIG_CQSPI_REF_CLK is undefined and there is
> no reference to a valid clk device either.
> 
> 
> >   	priv->regbase = plat->regbase;
> >   	priv->ahbbase = plat->ahbbase;
> >
> >
> 
> Regards
> Vignesh
diff mbox series

Patch

diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
index 1e85749209..3bb5c7031d 100644
--- a/drivers/spi/cadence_qspi.c
+++ b/drivers/spi/cadence_qspi.c
@@ -170,6 +170,9 @@  static int cadence_spi_probe(struct udevice *bus)
 	struct clk clk;
 	int ret;
 
+	if (!CONFIG_CQSPI_REF_CLK)
+		return -ENODEV;
+
 	priv->regbase = plat->regbase;
 	priv->ahbbase = plat->ahbbase;