[PATCH 0/2] clk: add assigned-clock-rates-u64

[PATCH 0/2] clk: add assigned-clock-rates-u64

[Peng Fan (OSS)]

i.MX95 PLL VCO supports rates that exceeds UINT32_MAX, and the
i.MX95 System Controller Management Firmware(SCMI) server exports
PLL VCO for SCMI Agents to configure. So introduce
assigned-clock-rates-u64 to support rates that exceeds UINT32_MAX.
And introduce of_property_for_each_u64 to iterate each u64 rate.

The PR to add assigned-clock-rates-u64 to dt-schema:
https://github.com/devicetree-org/dt-schema/pull/140

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Peng Fan (2):
      of: property: add of_property_for_each_u64
      clk: clk-conf: support assigned-clock-rates-u64

 drivers/clk/clk-conf.c | 106 ++++++++++++++++++++++++++++++++++---------------
 drivers/of/property.c  |  23 +++++++++++
 include/linux/of.h     |  24 +++++++++++
 3 files changed, 121 insertions(+), 32 deletions(-)
---
base-commit: 76db4c64526c5e8ba0f56ad3d890dce8f9b00bbc
change-id: 20240621-clk-u64-70c4333f0f80

Best regards,
-- 
Peng Fan <peng.fan@nxp.com>

[PATCH 1/2] of: property: add of_property_for_each_u64

[Peng Fan (OSS)]

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)
+{
+	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);		\

-- 
2.37.1

[PATCH 2/2] clk: clk-conf: support assigned-clock-rates-u64

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

i.MX95 System Management Control Firmware(SCMI) manages the clock
function, it exposes PLL VCO which could support up to 5GHz rate that
exceeds UINT32_MAX. So add assigned-clock-rates-u64 support
to set rate that exceeds UINT32_MAX.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/clk/clk-conf.c | 106 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 74 insertions(+), 32 deletions(-)

diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
index 1a4e6340f95c..2bb00c859eb4 100644
--- a/drivers/clk/clk-conf.c
+++ b/drivers/clk/clk-conf.c
@@ -78,49 +78,91 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 	return rc;
 }
 
-static int __set_clk_rates(struct device_node *node, bool clk_supplier)
+static int __set_clk_rate(struct device_node *node, bool clk_supplier, int index,
+			  unsigned long rate)
 {
 	struct of_phandle_args clkspec;
+	struct clk *clk;
+	int rc;
+
+	rc = of_parse_phandle_with_args(node, "assigned-clocks",
+			"#clock-cells",	index, &clkspec);
+	if (rc < 0)
+		return rc;
+
+	if (clkspec.np == node && !clk_supplier) {
+		of_node_put(clkspec.np);
+		return 1;
+	}
+
+	clk = of_clk_get_from_provider(&clkspec);
+	of_node_put(clkspec.np);
+	if (IS_ERR(clk)) {
+		if (PTR_ERR(clk) != -EPROBE_DEFER)
+			pr_warn("clk: couldn't get clock %d for %pOF\n",
+				index, node);
+		return PTR_ERR(clk);
+	}
+
+	rc = clk_set_rate(clk, rate);
+	if (rc < 0)
+		pr_err("clk: couldn't set %s clk rate to %lu (%d), current rate: %lu\n",
+		       __clk_get_name(clk), rate, rc, clk_get_rate(clk));
+	clk_put(clk);
+
+	return 0;
+}
+
+static int __set_clk_rates(struct device_node *node, bool clk_supplier)
+{
 	struct property	*prop;
 	const __be32 *cur;
 	int rc, index = 0;
-	struct clk *clk;
-	u32 rate;
+	u64 rate;
+	u32 rate_32;
+	bool is_rate_32 = false;
 
-	of_property_for_each_u32(node, "assigned-clock-rates", prop, cur, rate) {
-		if (rate) {
-			rc = of_parse_phandle_with_args(node, "assigned-clocks",
-					"#clock-cells",	index, &clkspec);
-			if (rc < 0) {
-				/* skip empty (null) phandles */
-				if (rc == -ENOENT)
-					continue;
-				else
-					return rc;
-			}
-			if (clkspec.np == node && !clk_supplier) {
-				of_node_put(clkspec.np);
-				return 0;
-			}
+	if (!of_find_property(node, "assigned-clock-rates-u64", NULL))
+		is_rate_32 = true;
 
-			clk = of_clk_get_from_provider(&clkspec);
-			of_node_put(clkspec.np);
-			if (IS_ERR(clk)) {
-				if (PTR_ERR(clk) != -EPROBE_DEFER)
-					pr_warn("clk: couldn't get clock %d for %pOF\n",
-						index, node);
-				return PTR_ERR(clk);
+	if (is_rate_32) {
+		of_property_for_each_u32(node, "assigned-clock-rates", prop, cur, rate_32) {
+			if (rate_32) {
+				rc = __set_clk_rate(node, clk_supplier, index, rate_32);
+
+				if (rc == 1 && !clk_supplier)
+					return 0;
+
+				if (rc < 0) {
+					/* skip empty (null) phandles */
+					if (rc == -ENOENT)
+						continue;
+					else
+						return rc;
+				}
 			}
+			index++;
+		}
+	} else {
+		of_property_for_each_u64(node, "assigned-clock-rates-u64", prop, cur, rate) {
+			if (rate) {
+				rc = __set_clk_rate(node, clk_supplier, index, rate);
 
-			rc = clk_set_rate(clk, rate);
-			if (rc < 0)
-				pr_err("clk: couldn't set %s clk rate to %u (%d), current rate: %lu\n",
-				       __clk_get_name(clk), rate, rc,
-				       clk_get_rate(clk));
-			clk_put(clk);
+				if (rc == 1 && !clk_supplier)
+					return 0;
+
+				if (rc < 0) {
+					/* skip empty (null) phandles */
+					if (rc == -ENOENT)
+						continue;
+					else
+						return rc;
+				}
+			}
+			index++;
 		}
-		index++;
 	}
+
 	return 0;
 }
 

-- 
2.37.1

Re: [PATCH 1/2] of: property: add of_property_for_each_u64

[Rob Herring]

+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/

RE: [PATCH 1/2] of: property: add of_property_for_each_u64

[Peng Fan]

> 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/

RE: [PATCH 1/2] of: property: add of_property_for_each_u64

[Peng Fan]

> 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/

Re: [PATCH 1/2] of: property: add of_property_for_each_u64

[Luca Ceresoli]

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

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/2] of: property: add of_property_for_each_u64

[Luca Ceresoli]

Hello Peng,

On Fri, 28 Jun 2024 16:16:17 +0200
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

[...]

> > > > > +#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.

Ok, that slipped to Wednesday, but here it is:
https://lore.kernel.org/all/20240703-of_property_for_each_u32-v1-1-42c1fc0b82aa@bootlin.com/

I think you can reuse the technique I used in that patch to write
of_property_for_each_u64(np, propname, u), taking only 3 parameters.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com