Re: [U-Boot] [PATCH v4 01/11] serial: ns16550: Support clocks via phandle

On 4.8.2016 11:59, Paul Burton wrote:

On 04/08/16 10:50, Michal Simek wrote:

...

...

@@ -352,6 +353,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;

  /* try Processor Local Bus device first */ addr = dev_get_addr(dev);

@@ -397,9 +400,19 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev) "reg-offset", 0); plat->reg_shift = fdtdec_get_int(gd->fdt_blob, dev->of_offset, "reg-shift", 0);

• plat->clock = fdtdec_get_int(gd->fdt_blob, dev->of_offset,

•                 "clock-frequency",
  

•                 CONFIG_SYS_NS16550_CLK);
  

• err = clk_get_by_index(dev, 0, &clk);
• if (!err) {

•    plat->clock = clk_get_rate(&clk);
  

• } else if (err != -ENODEV) {

•    debug("ns16550 failed to get clock\n");
  

•    return err;
  

• }
• if (!plat->clock)

•    plat->clock = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
  

•                     "clock-frequency",
  

•                     CONFIG_SYS_NS16550_CLK);
  

  if (!plat->clock) { debug("ns16550 clock not defined\n"); return -EINVAL;

NACK. It is missing dependency on clk uclass which is not enabled by default on Microblaze. Maybe also on others. You should add some ifs around.

Thanks, Michal

I'm not sure #ifdef'ing in this code is the nicest solution. How about if we allow -ENOSYS through the error handling above like -ENODEV, and provide the dummy clk_get_by_* implementations when CONFIG_CLK is disabled? As in:

diff --git a/include/clk.h b/include/clk.h index 161bc28..328f364 100644 --- a/include/clk.h +++ b/include/clk.h @@ -59,7 +59,7 @@ struct clk { unsigned long id; };

-#if CONFIG_IS_ENABLED(OF_CONTROL) +#if CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(CONFIG_CLK) struct phandle_2_cell; int clk_get_by_index_platdata(struct udevice *dev, int index, struct phandle_2_cell *cells, struct clk *clk);

No problem with it from my side. Just to make sure that you can build it on other archs and there is no big overhead.

Thanks, Michal