| Message ID | 20240621-clk-u64-v1-1-d28a611b2621@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | clk: add assigned-clock-rates-u64 | expand |
+Luca On Fri, Jun 21, 2024 at 08:36:39PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > Preparing for assigned-clock-rates-u64 support, add function > of_property_for_each_u64 to iterate each u64 value > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/of/property.c | 23 +++++++++++++++++++++++ > include/linux/of.h | 24 ++++++++++++++++++++++++ > 2 files changed, 47 insertions(+) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 164d77cb9445..b89c3ab01d44 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -548,6 +548,29 @@ const __be32 *of_prop_next_u32(struct property *prop, const __be32 *cur, > } > EXPORT_SYMBOL_GPL(of_prop_next_u32); > > +const __be32 *of_prop_next_u64(struct property *prop, const __be32 *cur, > + u64 *pu) struct property can be const > +{ > + const void *curv = cur; > + > + if (!prop) > + return NULL; > + > + if (!cur) { > + curv = prop->value; > + goto out_val; > + } > + > + curv += sizeof(*cur) * 2; > + if (curv >= prop->value + prop->length) > + return NULL; > + > +out_val: > + *pu = of_read_number(curv, 2); > + return curv; > +} > +EXPORT_SYMBOL_GPL(of_prop_next_u64); > + > const char *of_prop_next_string(struct property *prop, const char *cur) > { > const void *curv = cur; > diff --git a/include/linux/of.h b/include/linux/of.h > index 13cf7a43b473..464eca6a4636 100644 > --- a/include/linux/of.h > +++ b/include/linux/of.h > @@ -439,6 +439,18 @@ extern int of_detach_node(struct device_node *); > */ > const __be32 *of_prop_next_u32(struct property *prop, const __be32 *cur, > u32 *pu); > + > +/* > + * struct property *prop; > + * const __be32 *p; > + * u64 u; > + * > + * of_property_for_each_u64(np, "propname", prop, p, u) > + * printk("U64 value: %llx\n", u); > + */ > +const __be32 *of_prop_next_u64(struct property *prop, const __be32 *cur, > + u64 *pu); > + > /* > * struct property *prop; > * const char *s; > @@ -834,6 +846,12 @@ static inline const __be32 *of_prop_next_u32(struct property *prop, > return NULL; > } > > +static inline const __be32 *of_prop_next_u64(struct property *prop, > + const __be32 *cur, u64 *pu) > +{ > + return NULL; > +} > + > static inline const char *of_prop_next_string(struct property *prop, > const char *cur) > { > @@ -1437,6 +1455,12 @@ static inline int of_property_read_s32(const struct device_node *np, > p; \ > p = of_prop_next_u32(prop, p, &u)) > > +#define of_property_for_each_u64(np, propname, prop, p, u) \ > + for (prop = of_find_property(np, propname, NULL), \ > + p = of_prop_next_u64(prop, NULL, &u); \ > + p; \ > + p = of_prop_next_u64(prop, p, &u)) I think we want to define this differently to avoid exposing struct property and the property data directly. Like this: #define of_property_for_each_u64(np, propname, u) \ for (struct property *_prop = of_find_property(np, propname, NULL), const __be32 *_p = of_prop_next_u64(_prop, NULL, &u); _p; _p = of_prop_next_u64(_prop, _p, &u)) See this discussion for context[1]. Rob [1] https://lore.kernel.org/all/20240624232122.3cfe03f8@booty/
> Subject: Re: [PATCH 1/2] of: property: add of_property_for_each_u64 > > +Luca > > On Fri, Jun 21, 2024 at 08:36:39PM +0800, Peng Fan (OSS) wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > Preparing for assigned-clock-rates-u64 support, add function > > of_property_for_each_u64 to iterate each u64 value > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > drivers/of/property.c | 23 +++++++++++++++++++++++ > > include/linux/of.h | 24 ++++++++++++++++++++++++ > > 2 files changed, 47 insertions(+) > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c index > > 164d77cb9445..b89c3ab01d44 100644 > > --- a/drivers/of/property.c > > +++ b/drivers/of/property.c > > @@ -548,6 +548,29 @@ const __be32 *of_prop_next_u32(struct > property > > *prop, const __be32 *cur, } > EXPORT_SYMBOL_GPL(of_prop_next_u32); > > > > +const __be32 *of_prop_next_u64(struct property *prop, const > __be32 *cur, > > + u64 *pu) > > struct property can be const Fix in v2. BTW, I am thinking something as below: const __be64 *of_prop_next_u64(const struct property *prop, const __be64 *cur, u64 *pu) { const void *curv = cur; if (!prop) return NULL; if (!cur) { curv = prop->value; goto out_val; } curv += sizeof(*cur); if (curv >= prop->value + prop->length) return NULL; out_val: *pu = be64_to_cpup(curv); return curv; } EXPORT_SYMBOL_GPL(of_prop_next_u64); > > > +{ > > + const void *curv = cur; > > + > > + if (!prop) > > + return NULL; > > + > > + if (!cur) { > > + curv = prop->value; > > + goto out_val; > > + } > > + > > + curv += sizeof(*cur) * 2; > > + if (curv >= prop->value + prop->length) > > + return NULL; > > + > > +out_val: > > + *pu = of_read_number(curv, 2); > > + return curv; > > +} > > +EXPORT_SYMBOL_GPL(of_prop_next_u64); > > + > > const char *of_prop_next_string(struct property *prop, const char > > *cur) { > > const void *curv = cur; > > diff --git a/include/linux/of.h b/include/linux/of.h index > > 13cf7a43b473..464eca6a4636 100644 > > --- a/include/linux/of.h > > +++ b/include/linux/of.h > > @@ -439,6 +439,18 @@ extern int of_detach_node(struct > device_node *); > > */ > > const __be32 *of_prop_next_u32(struct property *prop, const > __be32 *cur, > > u32 *pu); > > + > > +/* > > + * struct property *prop; > > + * const __be32 *p; > > + * u64 u; > > + * > > + * of_property_for_each_u64(np, "propname", prop, p, u) > > + * printk("U64 value: %llx\n", u); > > + */ > > +const __be32 *of_prop_next_u64(struct property *prop, const > __be32 *cur, > > + u64 *pu); > > + > > /* > > * struct property *prop; > > * const char *s; > > @@ -834,6 +846,12 @@ static inline const __be32 > *of_prop_next_u32(struct property *prop, > > return NULL; > > } > > > > +static inline const __be32 *of_prop_next_u64(struct property *prop, > > + const __be32 *cur, u64 *pu) > > +{ > > + return NULL; > > +} > > + > > static inline const char *of_prop_next_string(struct property *prop, > > const char *cur) > > { > > @@ -1437,6 +1455,12 @@ static inline int > of_property_read_s32(const struct device_node *np, > > p; \ > > p = of_prop_next_u32(prop, p, &u)) > > > > +#define of_property_for_each_u64(np, propname, prop, p, u) \ > > + for (prop = of_find_property(np, propname, NULL), \ > > + p = of_prop_next_u64(prop, NULL, &u); \ > > + p; \ > > + p = of_prop_next_u64(prop, p, &u)) > > I think we want to define this differently to avoid exposing struct > property and the property data directly. Like this: > > #define of_property_for_each_u64(np, propname, u) \ > for (struct property *_prop = of_find_property(np, propname, NULL), > const __be32 *_p = of_prop_next_u64(_prop, NULL, &u); > _p; > _p = of_prop_next_u64(_prop, _p, &u)) > Sure, I will fix in v2. Thanks, Peng. > See this discussion for context[1]. > > Rob > > [1] https://lore.kernel.org/all/20240624232122.3cfe03f8@booty/
> Subject: RE: [PATCH 1/2] of: property: add of_property_for_each_u64 > > > Subject: Re: [PATCH 1/2] of: property: add of_property_for_each_u64 > > > > +Luca > > > > On Fri, Jun 21, 2024 at 08:36:39PM +0800, Peng Fan (OSS) wrote: > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > Preparing for assigned-clock-rates-u64 support, add function > > > of_property_for_each_u64 to iterate each u64 value > > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > --- > > > drivers/of/property.c | 23 +++++++++++++++++++++++ > > > include/linux/of.h | 24 ++++++++++++++++++++++++ > > > 2 files changed, 47 insertions(+) > > > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c index > > > 164d77cb9445..b89c3ab01d44 100644 > > > --- a/drivers/of/property.c > > > +++ b/drivers/of/property.c > > > @@ -548,6 +548,29 @@ const __be32 *of_prop_next_u32(struct > > property > > > *prop, const __be32 *cur, } > > EXPORT_SYMBOL_GPL(of_prop_next_u32); > > > > > > +const __be32 *of_prop_next_u64(struct property *prop, const > > __be32 *cur, > > > + u64 *pu) > > > > struct property can be const > > Fix in v2. BTW, I am thinking something as below: > > const __be64 *of_prop_next_u64(const struct property *prop, const > __be64 *cur, > u64 *pu) > { > const void *curv = cur; > > if (!prop) > return NULL; > > if (!cur) { > curv = prop->value; > goto out_val; > } > > curv += sizeof(*cur); > if (curv >= prop->value + prop->length) > return NULL; > > out_val: > *pu = be64_to_cpup(curv); > return curv; > } > EXPORT_SYMBOL_GPL(of_prop_next_u64); > > > > > > +{ > > > + const void *curv = cur; > > > + > > > + if (!prop) > > > + return NULL; > > > + > > > + if (!cur) { > > > + curv = prop->value; > > > + goto out_val; > > > + } > > > + > > > + curv += sizeof(*cur) * 2; > > > + if (curv >= prop->value + prop->length) > > > + return NULL; > > > + > > > +out_val: > > > + *pu = of_read_number(curv, 2); > > > + return curv; > > > +} > > > +EXPORT_SYMBOL_GPL(of_prop_next_u64); > > > + > > > const char *of_prop_next_string(struct property *prop, const char > > > *cur) { > > > const void *curv = cur; > > > diff --git a/include/linux/of.h b/include/linux/of.h index > > > 13cf7a43b473..464eca6a4636 100644 > > > --- a/include/linux/of.h > > > +++ b/include/linux/of.h > > > @@ -439,6 +439,18 @@ extern int of_detach_node(struct > > device_node *); > > > */ > > > const __be32 *of_prop_next_u32(struct property *prop, const > > __be32 *cur, > > > u32 *pu); > > > + > > > +/* > > > + * struct property *prop; > > > + * const __be32 *p; > > > + * u64 u; > > > + * > > > + * of_property_for_each_u64(np, "propname", prop, p, u) > > > + * printk("U64 value: %llx\n", u); > > > + */ > > > +const __be32 *of_prop_next_u64(struct property *prop, const > > __be32 *cur, > > > + u64 *pu); > > > + > > > /* > > > * struct property *prop; > > > * const char *s; > > > @@ -834,6 +846,12 @@ static inline const __be32 > > *of_prop_next_u32(struct property *prop, > > > return NULL; > > > } > > > > > > +static inline const __be32 *of_prop_next_u64(struct property > *prop, > > > + const __be32 *cur, u64 *pu) > > > +{ > > > + return NULL; > > > +} > > > + > > > static inline const char *of_prop_next_string(struct property *prop, > > > const char *cur) > > > { > > > @@ -1437,6 +1455,12 @@ static inline int > > of_property_read_s32(const struct device_node *np, > > > p; \ > > > p = of_prop_next_u32(prop, p, &u)) > > > > > > +#define of_property_for_each_u64(np, propname, prop, p, u) \ > > > + for (prop = of_find_property(np, propname, NULL), \ > > > + p = of_prop_next_u64(prop, NULL, &u); \ > > > + p; \ > > > + p = of_prop_next_u64(prop, p, &u)) > > > > I think we want to define this differently to avoid exposing struct > > property and the property data directly. Like this: > > > > #define of_property_for_each_u64(np, propname, u) \ > > for (struct property *_prop = of_find_property(np, propname, NULL), > > const __be32 *_p = of_prop_next_u64(_prop, NULL, &u); > > _p; > > _p = of_prop_next_u64(_prop, _p, &u)) This will trigger a compilation error, because C not allow declare two variables with different types as for loop expression 1. Need to think about other methods. Thanks, Peng. > > > > Sure, I will fix in v2. > > Thanks, > Peng. > > > See this discussion for context[1]. > > > > Rob > > > > [1] https://lore.kernel.org/all/20240624232122.3cfe03f8@booty/
Hello Peng, On Fri, 28 Jun 2024 13:10:34 +0000 Peng Fan <peng.fan@nxp.com> wrote: > > Subject: RE: [PATCH 1/2] of: property: add of_property_for_each_u64 > > > > > Subject: Re: [PATCH 1/2] of: property: add of_property_for_each_u64 > > > > > > +Luca > > > > > > On Fri, Jun 21, 2024 at 08:36:39PM +0800, Peng Fan (OSS) wrote: > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > > > Preparing for assigned-clock-rates-u64 support, add function > > > > of_property_for_each_u64 to iterate each u64 value > > > > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > > --- > > > > drivers/of/property.c | 23 +++++++++++++++++++++++ > > > > include/linux/of.h | 24 ++++++++++++++++++++++++ > > > > 2 files changed, 47 insertions(+) > > > > > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c index > > > > 164d77cb9445..b89c3ab01d44 100644 > > > > --- a/drivers/of/property.c > > > > +++ b/drivers/of/property.c > > > > @@ -548,6 +548,29 @@ const __be32 *of_prop_next_u32(struct > > > property > > > > *prop, const __be32 *cur, } > > > EXPORT_SYMBOL_GPL(of_prop_next_u32); > > > > > > > > +const __be32 *of_prop_next_u64(struct property *prop, const > > > __be32 *cur, > > > > + u64 *pu) > > > > > > struct property can be const > > > > Fix in v2. BTW, I am thinking something as below: > > > > const __be64 *of_prop_next_u64(const struct property *prop, const > > __be64 *cur, > > u64 *pu) > > { > > const void *curv = cur; > > > > if (!prop) > > return NULL; > > > > if (!cur) { > > curv = prop->value; > > goto out_val; > > } > > > > curv += sizeof(*cur); > > if (curv >= prop->value + prop->length) > > return NULL; > > > > out_val: > > *pu = be64_to_cpup(curv); > > return curv; > > } > > EXPORT_SYMBOL_GPL(of_prop_next_u64); > > > > > > > > > +{ > > > > + const void *curv = cur; > > > > + > > > > + if (!prop) > > > > + return NULL; > > > > + > > > > + if (!cur) { > > > > + curv = prop->value; > > > > + goto out_val; > > > > + } > > > > + > > > > + curv += sizeof(*cur) * 2; > > > > + if (curv >= prop->value + prop->length) > > > > + return NULL; > > > > + > > > > +out_val: > > > > + *pu = of_read_number(curv, 2); > > > > + return curv; > > > > +} > > > > +EXPORT_SYMBOL_GPL(of_prop_next_u64); > > > > + > > > > const char *of_prop_next_string(struct property *prop, const char > > > > *cur) { > > > > const void *curv = cur; > > > > diff --git a/include/linux/of.h b/include/linux/of.h index > > > > 13cf7a43b473..464eca6a4636 100644 > > > > --- a/include/linux/of.h > > > > +++ b/include/linux/of.h > > > > @@ -439,6 +439,18 @@ extern int of_detach_node(struct > > > device_node *); > > > > */ > > > > const __be32 *of_prop_next_u32(struct property *prop, const > > > __be32 *cur, > > > > u32 *pu); > > > > + > > > > +/* > > > > + * struct property *prop; > > > > + * const __be32 *p; > > > > + * u64 u; > > > > + * > > > > + * of_property_for_each_u64(np, "propname", prop, p, u) > > > > + * printk("U64 value: %llx\n", u); > > > > + */ > > > > +const __be32 *of_prop_next_u64(struct property *prop, const > > > __be32 *cur, > > > > + u64 *pu); > > > > + > > > > /* > > > > * struct property *prop; > > > > * const char *s; > > > > @@ -834,6 +846,12 @@ static inline const __be32 > > > *of_prop_next_u32(struct property *prop, > > > > return NULL; > > > > } > > > > > > > > +static inline const __be32 *of_prop_next_u64(struct property > > *prop, > > > > + const __be32 *cur, u64 *pu) > > > > +{ > > > > + return NULL; > > > > +} > > > > + > > > > static inline const char *of_prop_next_string(struct property *prop, > > > > const char *cur) > > > > { > > > > @@ -1437,6 +1455,12 @@ static inline int > > > of_property_read_s32(const struct device_node *np, > > > > p; \ > > > > p = of_prop_next_u32(prop, p, &u)) > > > > > > > > +#define of_property_for_each_u64(np, propname, prop, p, u) \ > > > > + for (prop = of_find_property(np, propname, NULL), \ > > > > + p = of_prop_next_u64(prop, NULL, &u); \ > > > > + p; \ > > > > + p = of_prop_next_u64(prop, p, &u)) > > > > > > I think we want to define this differently to avoid exposing struct > > > property and the property data directly. Like this: > > > > > > #define of_property_for_each_u64(np, propname, u) \ > > > for (struct property *_prop = of_find_property(np, propname, NULL), > > > const __be32 *_p = of_prop_next_u64(_prop, NULL, &u); > > > _p; > > > _p = of_prop_next_u64(_prop, _p, &u)) > > This will trigger a compilation error, because C not allow > declare two variables with different types as for loop expression 1. > Need to think about other methods. I have a working draft here where I solved it somehow, let me just find the proper branch and send it. Perhaps next week, but I'm striving to do that by Mon-Tue. Luca
diff --git a/drivers/of/property.c b/drivers/of/property.c index 164d77cb9445..b89c3ab01d44 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -548,6 +548,29 @@ const __be32 *of_prop_next_u32(struct property *prop, const __be32 *cur, } EXPORT_SYMBOL_GPL(of_prop_next_u32); +const __be32 *of_prop_next_u64(struct property *prop, const __be32 *cur, + u64 *pu) +{ + const void *curv = cur; + + if (!prop) + return NULL; + + if (!cur) { + curv = prop->value; + goto out_val; + } + + curv += sizeof(*cur) * 2; + if (curv >= prop->value + prop->length) + return NULL; + +out_val: + *pu = of_read_number(curv, 2); + return curv; +} +EXPORT_SYMBOL_GPL(of_prop_next_u64); + const char *of_prop_next_string(struct property *prop, const char *cur) { const void *curv = cur; diff --git a/include/linux/of.h b/include/linux/of.h index 13cf7a43b473..464eca6a4636 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -439,6 +439,18 @@ extern int of_detach_node(struct device_node *); */ const __be32 *of_prop_next_u32(struct property *prop, const __be32 *cur, u32 *pu); + +/* + * struct property *prop; + * const __be32 *p; + * u64 u; + * + * of_property_for_each_u64(np, "propname", prop, p, u) + * printk("U64 value: %llx\n", u); + */ +const __be32 *of_prop_next_u64(struct property *prop, const __be32 *cur, + u64 *pu); + /* * struct property *prop; * const char *s; @@ -834,6 +846,12 @@ static inline const __be32 *of_prop_next_u32(struct property *prop, return NULL; } +static inline const __be32 *of_prop_next_u64(struct property *prop, + const __be32 *cur, u64 *pu) +{ + return NULL; +} + static inline const char *of_prop_next_string(struct property *prop, const char *cur) { @@ -1437,6 +1455,12 @@ static inline int of_property_read_s32(const struct device_node *np, p; \ p = of_prop_next_u32(prop, p, &u)) +#define of_property_for_each_u64(np, propname, prop, p, u) \ + for (prop = of_find_property(np, propname, NULL), \ + p = of_prop_next_u64(prop, NULL, &u); \ + p; \ + p = of_prop_next_u64(prop, p, &u)) + #define of_property_for_each_string(np, propname, prop, s) \ for (prop = of_find_property(np, propname, NULL), \ s = of_prop_next_string(prop, NULL); \