Re: [U-Boot] [PATCH v7 02/12] serial: ns16550: Support clocks via phandle

Hello,

I found that this commit breaks serial ns16550 driver on ARC. I have investigated the issue and found that it is because ARC timer node in device tree doesn't refer to any clock devices.

In such case clk_get_by_index() returns -ENOENT error, but neither -ENODEV nor -ENOSYS (as CONFIG_CLK is enabled for ARC). So the error is not ignored and we exit with error instead of trying to get "clock-frequency" property from the device tree.

Thus I wonder why we ignore only ENOSYS and ENODEV errors and exit if any other error code appears? As shown in my example such behavior can lead to breakages.  What should we do if ENOENT occurs?

Thanks.

On Thu, 2016-09-08 at 07:47 +0100, Paul Burton wrote:

Previously ns16550 compatible UARTs probed via device tree have needed their device tree nodes to contain a clock-frequency property. An alternative to this commonly used with Linux is to reference a clock via a phandle. This patch allows U-Boot to support that, retrieving the clock frequency by probing the appropriate clock device.

For example, a system might choose to provide the UART base clock as a reference to a clock common to multiple devices:

sys_clk: clock {     compatible = "fixed-clock";     #clock-cells = <0>;     clock-frequency = <10000000>;   };

uart0: uart@10000000 {     compatible = "ns16550a";     reg = <0x10000000 0x1000>;     clocks = <&sys_clk>;   };

uart1: uart@10000000 {     compatible = "ns16550a";     reg = <0x10001000 0x1000>;     clocks = <&sys_clk>;   };

This removes the need for the frequency information to be duplicated in multiple nodes and allows the device tree to be more descriptive of the system.

Signed-off-by: Paul Burton paul.burton@imgtec.com Reviewed-by: Simon Glass sjg@chromium.org

---

Changes in v7:

• Check clk_get_rate return for error values

Changes in v6:

• Ignore -ENOSYS from clk_get_by_index too, for systems with CONFIG_CLK=n

Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2:

• Propogate non-ENODEV errors from clk_get_by_index

drivers/serial/ns16550.c | 21 ++++++++++++++++++---  1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c index 88fca15..3f6ea4d 100644 --- a/drivers/serial/ns16550.c +++ b/drivers/serial/ns16550.c @@ -5,6 +5,7 @@   */    #include <common.h> +#include <clk.h>  #include <dm.h>  #include <errno.h>  #include <fdtdec.h> @@ -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,21 @@ 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) {

• 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;
  

• }
• 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;