Re: [U-Boot] [PATCH] ns16650: Make sure we have CONFIG_CLK set before using infrastructure

Hi Tom,

2016-09-23 10:11 GMT+09:00 Tom Rini trini@konsulko.com:

We cannot call on the CONFIG_CLK based clk_get_rate function unless CONFIG_CLK is set.

Signed-off-by: Tom Rini trini@konsulko.com

drivers/serial/ns16550.c | 7 +++++-- include/clk.h | 25 ++++++++++++------------- 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c index 3f6ea4d27591..765499dab646 100644 --- a/drivers/serial/ns16550.c +++ b/drivers/serial/ns16550.c @@ -13,6 +13,7 @@ #include <serial.h> #include <watchdog.h> #include <linux/types.h> +#include <linux/compiler.h> #include <asm/io.h>

DECLARE_GLOBAL_DATA_PTR; @@ -353,8 +354,8 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev) { struct ns16550_platdata *plat = dev->platdata; fdt_addr_t addr;

•   struct clk clk;
  

•   int err;
  

•   __maybe_unused struct clk clk;
  

•   __maybe_unused int err;
  
    /* try Processor Local Bus device first */
    addr = dev_get_addr(dev);
  

@@ -401,6 +402,7 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev) plat->reg_shift = fdtdec_get_int(gd->fdt_blob, dev->of_offset, "reg-shift", 0);

+#ifdef CONFIG_CLK err = clk_get_by_index(dev, 0, &clk); if (!err) { err = clk_get_rate(&clk); @@ -410,6 +412,7 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev) debug("ns16550 failed to get clock\n"); return err; } +#endif

    if (!plat->clock)
            plat->clock = fdtdec_get_int(gd->fdt_blob, dev->of_offset,

Why did you need this patch?

If CONFIG_CLK is not set, clk_get_by_index() just returns -ENOSYS.

So, the compiler optimization will drop the whole of if () ... else if ().

err = clk_get_by_index(dev, 0, &clk); if (!err) { err = clk_get_rate(&clk); if (!IS_ERR_VALUE(err)) plat->clock = err; } else if (err != -ENODEV && err != -ENOSYS) { debug("ns16550 failed to get clock\n"); return err; }

Why are you belt-and-braces guarding it with #ifdef CONFIG_CLK?

diff --git a/include/clk.h b/include/clk.h index 94c003714700..7273127bb4fb 100644 --- a/include/clk.h +++ b/include/clk.h @@ -98,19 +98,6 @@ int clk_get_by_index(struct udevice *dev, int index, struct clk *clk);

• @return 0 if OK, or a negative error code.

*/ int clk_get_by_name(struct udevice *dev, const char *name, struct clk *clk); -#else -static inline int clk_get_by_index(struct udevice *dev, int index,

•                              struct clk *clk)
  

-{

•   return -ENOSYS;
  

-}

-static inline int clk_get_by_name(struct udevice *dev, const char *name,

•                      struct clk *clk)
  

-{

•   return -ENOSYS;
  

-} -#endif

/**

• clk_request - Request a clock by provider-specific ID.

@@ -175,5 +162,17 @@ int clk_enable(struct clk *clk); int clk_disable(struct clk *clk);

int soc_clk_dump(void); +#else +static inline int clk_get_by_index(struct udevice *dev, int index,

•                              struct clk *clk)
  

+{

•   return -ENOSYS;
  

+}

+static inline int clk_get_by_name(struct udevice *dev, const char *name,

•                      struct clk *clk)
  

+{

•   return -ENOSYS;
  

+} +#endif #endif

This just made the situation worse; it will cause implicit declaration of clk_enable(), clk_set_rate(), etc unless we patch clk consumers around with #ifdef CONFIG_CLK.