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

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:

*clk);

Thanks,
     Paul

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