| Message ID | 1452727540-3249-2-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Simon, > @@ -12,6 +12,8 @@ > #include <dm/lists.h> > #include <dm/root.h> > > +DECLARE_GLOBAL_DATA_PTR; > + > ulong clk_get_rate(struct udevice *dev) > { > struct clk_ops *ops = clk_get_ops(dev); > @@ -62,6 +64,32 @@ int clk_get_id(struct udevice *dev, int args_count, uint32_t *args) > return ops->get_id(dev, args_count, args); > } > > +int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp, > + int *periphp) This function causes NULL pointer access if called with clk_devp == NULL. You can decrease the number of arguments if this function returns periph ID. > +{ > + struct fdtdec_phandle_args args; > + int ret; > + > + ret = fdtdec_parse_phandle_with_args(gd->fdt_blob, dev->of_offset, > + "clocks", "#clock-cells", 0, index, > + &args); > + if (ret) { > + debug("%s: fdtdec_parse_phandle_with_args failed: err=%d\n", > + __func__, ret); > + return ret; > + } > + > + ret = uclass_get_device_by_of_offset(UCLASS_CLK, args.node, clk_devp); > + if (ret) { > + debug("%s: uclass_get_device_by_of_offset failed: err=%d\n", > + __func__, ret); > + return ret; > + } > + *periphp = args.args_count > 0 ? args.args[0] : -1; Do you want to let this function fail against #clock-cells == 0? This code should be compiled only when OF_CONTROL is on.
Hi Masahiro, On 14 January 2016 at 03:11, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Hi Simon, > > > >> @@ -12,6 +12,8 @@ >> #include <dm/lists.h> >> #include <dm/root.h> >> >> +DECLARE_GLOBAL_DATA_PTR; >> + >> ulong clk_get_rate(struct udevice *dev) >> { >> struct clk_ops *ops = clk_get_ops(dev); >> @@ -62,6 +64,32 @@ int clk_get_id(struct udevice *dev, int args_count, uint32_t *args) >> return ops->get_id(dev, args_count, args); >> } >> >> +int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp, >> + int *periphp) > > > This function causes NULL pointer access > if called with clk_devp == NULL. Yes, don't do that. Do you think the function comment is unclear? > > > You can decrease the number of arguments > if this function returns periph ID. Right, but how to handle 0? > > >> +{ >> + struct fdtdec_phandle_args args; >> + int ret; >> + >> + ret = fdtdec_parse_phandle_with_args(gd->fdt_blob, dev->of_offset, >> + "clocks", "#clock-cells", 0, index, >> + &args); >> + if (ret) { >> + debug("%s: fdtdec_parse_phandle_with_args failed: err=%d\n", >> + __func__, ret); >> + return ret; >> + } >> + >> + ret = uclass_get_device_by_of_offset(UCLASS_CLK, args.node, clk_devp); >> + if (ret) { >> + debug("%s: uclass_get_device_by_of_offset failed: err=%d\n", >> + __func__, ret); >> + return ret; >> + } >> + *periphp = args.args_count > 0 ? args.args[0] : -1; > > > Do you want to let this function fail against #clock-cells == 0? At present it doesn't. That's why I didn't have it return the peripheral ID. What could we return to signify that the function succeeded but there was no peripheral ID? > > > > This code should be compiled only when OF_CONTROL is on. OK, I'll update it once we sort out the above. Regards, Simon
Hi Simon, 2016-01-15 22:21 GMT+09:00 Simon Glass <sjg@chromium.org>: > Hi Masahiro, > > On 14 January 2016 at 03:11, Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: >> Hi Simon, >> >> >> >>> @@ -12,6 +12,8 @@ >>> #include <dm/lists.h> >>> #include <dm/root.h> >>> >>> +DECLARE_GLOBAL_DATA_PTR; >>> + >>> ulong clk_get_rate(struct udevice *dev) >>> { >>> struct clk_ops *ops = clk_get_ops(dev); >>> @@ -62,6 +64,32 @@ int clk_get_id(struct udevice *dev, int args_count, uint32_t *args) >>> return ops->get_id(dev, args_count, args); >>> } >>> >>> +int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp, >>> + int *periphp) >> >> >> This function causes NULL pointer access >> if called with clk_devp == NULL. > > Yes, don't do that. Do you think the function comment is unclear? This would not be a problem in this case, but I thought NULL-pointer checking is a boilterplate when we return a pointer filled into a function argument. >> >> >> You can decrease the number of arguments >> if this function returns periph ID. > > Right, but how to handle 0? >= 0 means peripheral ID. < 0 means error code. >> >> >>> +{ >>> + struct fdtdec_phandle_args args; >>> + int ret; >>> + >>> + ret = fdtdec_parse_phandle_with_args(gd->fdt_blob, dev->of_offset, >>> + "clocks", "#clock-cells", 0, index, >>> + &args); >>> + if (ret) { >>> + debug("%s: fdtdec_parse_phandle_with_args failed: err=%d\n", >>> + __func__, ret); >>> + return ret; >>> + } >>> + >>> + ret = uclass_get_device_by_of_offset(UCLASS_CLK, args.node, clk_devp); >>> + if (ret) { >>> + debug("%s: uclass_get_device_by_of_offset failed: err=%d\n", >>> + __func__, ret); >>> + return ret; >>> + } >>> + *periphp = args.args_count > 0 ? args.args[0] : -1; >> >> >> Do you want to let this function fail against #clock-cells == 0? > > At present it doesn't. That's why I didn't have it return the > peripheral ID. What could we return to signify that the function > succeeded but there was no peripheral ID? I think any clock provider should have at least one ID. If "clock-cells == 0", this clock has ID 0. Please correct me if I am misunderstanding.
Hi Masahiro, On 18 January 2016 at 19:01, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Hi Simon, > > > 2016-01-15 22:21 GMT+09:00 Simon Glass <sjg@chromium.org>: >> Hi Masahiro, >> >> On 14 January 2016 at 03:11, Masahiro Yamada >> <yamada.masahiro@socionext.com> wrote: >>> Hi Simon, >>> >>> >>> >>>> @@ -12,6 +12,8 @@ >>>> #include <dm/lists.h> >>>> #include <dm/root.h> >>>> >>>> +DECLARE_GLOBAL_DATA_PTR; >>>> + >>>> ulong clk_get_rate(struct udevice *dev) >>>> { >>>> struct clk_ops *ops = clk_get_ops(dev); >>>> @@ -62,6 +64,32 @@ int clk_get_id(struct udevice *dev, int args_count, uint32_t *args) >>>> return ops->get_id(dev, args_count, args); >>>> } >>>> >>>> +int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp, >>>> + int *periphp) >>> >>> >>> This function causes NULL pointer access >>> if called with clk_devp == NULL. >> >> Yes, don't do that. Do you think the function comment is unclear? > > > This would not be a problem in this case, > but I thought NULL-pointer checking is a boilterplate > when we return a pointer filled into a function argument. > > > >>> >>> >>> You can decrease the number of arguments >>> if this function returns periph ID. >> >> Right, but how to handle 0? > > >>= 0 means peripheral ID. > > < 0 means error code. > > >>> >>> >>>> +{ >>>> + struct fdtdec_phandle_args args; >>>> + int ret; >>>> + >>>> + ret = fdtdec_parse_phandle_with_args(gd->fdt_blob, dev->of_offset, >>>> + "clocks", "#clock-cells", 0, index, >>>> + &args); >>>> + if (ret) { >>>> + debug("%s: fdtdec_parse_phandle_with_args failed: err=%d\n", >>>> + __func__, ret); >>>> + return ret; >>>> + } >>>> + >>>> + ret = uclass_get_device_by_of_offset(UCLASS_CLK, args.node, clk_devp); >>>> + if (ret) { >>>> + debug("%s: uclass_get_device_by_of_offset failed: err=%d\n", >>>> + __func__, ret); >>>> + return ret; >>>> + } >>>> + *periphp = args.args_count > 0 ? args.args[0] : -1; >>> >>> >>> Do you want to let this function fail against #clock-cells == 0? >> >> At present it doesn't. That's why I didn't have it return the >> peripheral ID. What could we return to signify that the function >> succeeded but there was no peripheral ID? > > > I think any clock provider should have at least one ID. > > If "clock-cells == 0", this clock has ID 0. > > Please correct me if I am misunderstanding. That sounds fine to me. I've sent an update to that one patch. Regards, Simon
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 8078b0f..a205b26 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -12,6 +12,8 @@ #include <dm/lists.h> #include <dm/root.h> +DECLARE_GLOBAL_DATA_PTR; + ulong clk_get_rate(struct udevice *dev) { struct clk_ops *ops = clk_get_ops(dev); @@ -62,6 +64,32 @@ int clk_get_id(struct udevice *dev, int args_count, uint32_t *args) return ops->get_id(dev, args_count, args); } +int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp, + int *periphp) +{ + struct fdtdec_phandle_args args; + int ret; + + ret = fdtdec_parse_phandle_with_args(gd->fdt_blob, dev->of_offset, + "clocks", "#clock-cells", 0, index, + &args); + if (ret) { + debug("%s: fdtdec_parse_phandle_with_args failed: err=%d\n", + __func__, ret); + return ret; + } + + ret = uclass_get_device_by_of_offset(UCLASS_CLK, args.node, clk_devp); + if (ret) { + debug("%s: uclass_get_device_by_of_offset failed: err=%d\n", + __func__, ret); + return ret; + } + *periphp = args.args_count > 0 ? args.args[0] : -1; + + return 0; +} + UCLASS_DRIVER(clk) = { .id = UCLASS_CLK, .name = "clk", diff --git a/include/clk.h b/include/clk.h index ce2db41..b77c788 100644 --- a/include/clk.h +++ b/include/clk.h @@ -150,4 +150,19 @@ static inline int fdt_clk_get(const void *fdt, int nodeoffset, int index, } #endif +/** + * clk_get_by_index() - look up a clock referenced by a device + * + * Parse a device's 'clocks' list, returning information on the indexed clock, + * ensuring that it is activated. + * + * @dev: Device containing the clock reference + * @index: Clock index to return (0 = first) + * @clk_devp: Returns clock device + * @periphp: Returns peripheral ID for the device to control. This is the + * first argument after the clock node phandle. + * @return 0 if OK, or -ve error code + */ +int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp, + int *periphp); #endif /* _CLK_H_ */
Add a method which can locate a clock for a device, given its index. This uses the normal device tree bindings to return the clock device and the first argument which is normally used as a peripheral ID in U-Boot. Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/clk/clk-uclass.c | 28 ++++++++++++++++++++++++++++ include/clk.h | 15 +++++++++++++++ 2 files changed, 43 insertions(+)