[PATCH 1/2] platform: starfive: correct system clock device tree node

[PATCH 1/2] platform: starfive: correct system clock device tree node

[Nam Cao]

Starfive names the system clock device tree node "starfive,jh7110-clkgen"
in all their git repositories. However, a different name is used in
upstream U-Boot (and also Linux): "starfive,jh7110-syscrg". Since
OpenSBI gets the device tree from U-Boot, this inconsistency leads the
problem that OpenSBI doesn't know the system clock device exists.

Correct this name to keep the consistency.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 platform/generic/starfive/jh7110.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/platform/generic/starfive/jh7110.c b/platform/generic/starfive/jh7110.c
index dcd6306..4b22175 100644
--- a/platform/generic/starfive/jh7110.c
+++ b/platform/generic/starfive/jh7110.c
@@ -252,7 +252,7 @@ static int starfive_jh7110_inst_init(void *fdt)
 		jh7110_inst.pmu_reg_base = addr;
 	}
 
-	noff = fdt_node_offset_by_compatible(fdt, -1, "starfive,jh7110-clkgen");
+	noff = fdt_node_offset_by_compatible(fdt, -1, "starfive,jh7110-syscrg");
 	if (-1 < noff) {
 		rc = fdt_get_node_addr_size(fdt, noff, 0, &addr, NULL);
 		if (rc)
-- 
2.39.2

[PATCH 2/2] platform: starfive: get I2C offset address from clocks property

[Nam Cao]

The current code gets the I2C offset address using the device tree node
name: it get the I2C device index from the 4th character in the node
name (for example, "i2c5" -> i2c device 5). However, the device tree
node's name in U-Boot is actually just "i2c" without the number, so the
current code cannot be used with the device tree from U-Boot.

Get the I2C offset address from the "clocks" property instead.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 platform/generic/starfive/jh7110.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/platform/generic/starfive/jh7110.c b/platform/generic/starfive/jh7110.c
index 4b22175..846b068 100644
--- a/platform/generic/starfive/jh7110.c
+++ b/platform/generic/starfive/jh7110.c
@@ -29,7 +29,7 @@ struct pmic {
 struct jh7110 {
 	u64 pmu_reg_base;
 	u64 clk_reg_base;
-	u32 i2c_index;
+	u32 i2c_clk_offset;
 };
 
 static struct pmic pmic_inst;
@@ -163,10 +163,7 @@ static void pmic_i2c_clk_enable(void)
 	unsigned long clock_base;
 	unsigned int val;
 
-	clock_base = jh7110_inst.clk_reg_base +
-		I2C_APB_CLK_OFFSET +
-		(jh7110_inst.i2c_index << 2);
-
+	clock_base = jh7110_inst.clk_reg_base + jh7110_inst.i2c_clk_offset;
 	val = readl((void *)clock_base);
 
 	if (!val)
@@ -241,7 +238,8 @@ static struct fdt_reset fdt_reset_pmic = {
 static int starfive_jh7110_inst_init(void *fdt)
 {
 	int noff, rc = 0;
-	const char *name;
+	const fdt32_t *val;
+	int len;
 	u64 addr;
 
 	noff = fdt_node_offset_by_compatible(fdt, -1, "starfive,jh7110-pmu");
@@ -261,9 +259,12 @@ static int starfive_jh7110_inst_init(void *fdt)
 	}
 
 	if (pmic_inst.adapter) {
-		name = fdt_get_name(fdt, pmic_inst.adapter->id, NULL);
-		if (!sbi_strncmp(name, "i2c", 3))
-			jh7110_inst.i2c_index = name[3] - '0';
+		val = fdt_getprop(fdt, pmic_inst.adapter->id, "clocks", &len);
+		/* The clocks property looks like this: clocks = <&syscrg JH7110_SYSCLK_I2C5_APB>;
+		 * So check that the length is 8 bytes, and get the offset from the second value
+		 */
+		if (val && len == 8)
+			jh7110_inst.i2c_clk_offset = fdt32_to_cpu(val[1]) << 2;
 		else
 			rc = SBI_EINVAL;
 	}
-- 
2.39.2

[PATCH 1/2] platform: starfive: correct system clock device tree node

[Xiang W]

? 2024-01-17???? 12:16 +0100?Nam Cao???
> Starfive names the system clock device tree node "starfive,jh7110-clkgen"
> in all their git repositories. However, a different name is used in
> upstream U-Boot (and also Linux): "starfive,jh7110-syscrg". Since
> OpenSBI gets the device tree from U-Boot, this inconsistency leads the
> problem that OpenSBI doesn't know the system clock device exists.
> 
> Correct this name to keep the consistency.
This sounds like a problem of u-boot. Directly replacing compatible strings is 
not a good way. It is best to try another compatible string after the search
fails.

Regards,
Xiang W
> 
> Signed-off-by: Nam Cao <namcao@linutronix.de>
> ---
> ?platform/generic/starfive/jh7110.c | 2 +-
> ?1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/platform/generic/starfive/jh7110.c b/platform/generic/starfive/jh7110.c
> index dcd6306..4b22175 100644
> --- a/platform/generic/starfive/jh7110.c
> +++ b/platform/generic/starfive/jh7110.c
> @@ -252,7 +252,7 @@ static int starfive_jh7110_inst_init(void *fdt)
> ?		jh7110_inst.pmu_reg_base = addr;
> ?	}
> ?
> -	noff = fdt_node_offset_by_compatible(fdt, -1, "starfive,jh7110-clkgen");
> +	noff = fdt_node_offset_by_compatible(fdt, -1, "starfive,jh7110-syscrg");
> ?	if (-1 < noff) {
> ?		rc = fdt_get_node_addr_size(fdt, noff, 0, &addr, NULL);
> ?		if (rc)
> -- 
> 2.39.2
> 
> 

[PATCH 1/2] platform: starfive: correct system clock device tree node

[David Abdurachmanov]

On Wed, Jan 17, 2024 at 1:33?PM Xiang W <wxjstz@126.com> wrote:
>
> ? 2024-01-17???? 12:16 +0100?Nam Cao???
> > Starfive names the system clock device tree node "starfive,jh7110-clkgen"
> > in all their git repositories. However, a different name is used in
> > upstream U-Boot (and also Linux): "starfive,jh7110-syscrg". Since
> > OpenSBI gets the device tree from U-Boot, this inconsistency leads the
> > problem that OpenSBI doesn't know the system clock device exists.
> >
> > Correct this name to keep the consistency.
> This sounds like a problem of u-boot. Directly replacing compatible strings is
> not a good way. It is best to try another compatible string after the search
> fails.

At a quick glance there are no approved bindings for
starfive,jh7110-clkgen. There are bindings for starfive,jh7110-syscrg
in the Linux tree. IIUC v1 patch to add bindings referenced
starfive,jh7110-clkgen compatible, but that's no what landed. U-Boot
is supposed to regularly sync their DTS from the kernel tree.

Most likely OpenSBI stuff was merged before bindings got officially
approved in the Linux tree.

Cheers,
david

>
> Regards,
> Xiang W
> >
> > Signed-off-by: Nam Cao <namcao@linutronix.de>
> > ---
> >  platform/generic/starfive/jh7110.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/platform/generic/starfive/jh7110.c b/platform/generic/starfive/jh7110.c
> > index dcd6306..4b22175 100644
> > --- a/platform/generic/starfive/jh7110.c
> > +++ b/platform/generic/starfive/jh7110.c
> > @@ -252,7 +252,7 @@ static int starfive_jh7110_inst_init(void *fdt)
> >               jh7110_inst.pmu_reg_base = addr;
> >       }
> >
> > -     noff = fdt_node_offset_by_compatible(fdt, -1, "starfive,jh7110-clkgen");
> > +     noff = fdt_node_offset_by_compatible(fdt, -1, "starfive,jh7110-syscrg");
> >       if (-1 < noff) {
> >               rc = fdt_get_node_addr_size(fdt, noff, 0, &addr, NULL);
> >               if (rc)
> > --
> > 2.39.2
> >
> >
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 1/2] platform: starfive: correct system clock device tree node

[Nam Cao]

On Wed, 17 Jan 2024 13:47:13 +0200 David Abdurachmanov <david.abdurachmanov@gmail.com> wrote:
> On Wed, Jan 17, 2024 at 1:33?PM Xiang W <wxjstz@126.com> wrote:
> >
> > ? 2024-01-17???? 12:16 +0100?Nam Cao???  
> > > Starfive names the system clock device tree node "starfive,jh7110-clkgen"
> > > in all their git repositories. However, a different name is used in
> > > upstream U-Boot (and also Linux): "starfive,jh7110-syscrg". Since
> > > OpenSBI gets the device tree from U-Boot, this inconsistency leads the
> > > problem that OpenSBI doesn't know the system clock device exists.
> > >
> > > Correct this name to keep the consistency.  
> > This sounds like a problem of u-boot. Directly replacing compatible strings is
> > not a good way. It is best to try another compatible string after the search
> > fails.  
> 
> At a quick glance there are no approved bindings for
> starfive,jh7110-clkgen. There are bindings for starfive,jh7110-syscrg
> in the Linux tree. IIUC v1 patch to add bindings referenced
> starfive,jh7110-clkgen compatible, but that's no what landed. U-Boot
> is supposed to regularly sync their DTS from the kernel tree.
> 
> Most likely OpenSBI stuff was merged before bindings got officially
> approved in the Linux tree.

This is also what I think: this stuff is merged to OpenSBI too early.

However, this patch breaks OpenSBI's compatibility with Starfive's U-Boot.
So the question is whether we should keep the compatibility with both
upstream U-Boot and Starfive's U-Boot, or just the upstream one. I would
say being compatible with upstream U-Boot is sufficient.

That said, if anyone insists on keeping the compatibility with Starfive's
U-Boot too, I can send a v2.

Best regards,
Nam

[PATCH 2/2] platform: starfive: get I2C offset address from clocks property

[Xiang W]

? 2024-01-17???? 12:16 +0100?Nam Cao???
> The current code gets the I2C offset address using the device tree node
> name: it get the I2C device index from the 4th character in the node
> name (for example, "i2c5" -> i2c device 5). However, the device tree
> node's name in U-Boot is actually just "i2c" without the number, so the
> current code cannot be used with the device tree from U-Boot.
> 
> Get the I2C offset address from the "clocks" property instead.
> 
> Signed-off-by: Nam Cao <namcao@linutronix.de>
> ---
> ?platform/generic/starfive/jh7110.c | 19 ++++++++++---------
> ?1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/platform/generic/starfive/jh7110.c b/platform/generic/starfive/jh7110.c
> index 4b22175..846b068 100644
> --- a/platform/generic/starfive/jh7110.c
> +++ b/platform/generic/starfive/jh7110.c
> @@ -29,7 +29,7 @@ struct pmic {
> ?struct jh7110 {
> ?	u64 pmu_reg_base;
> ?	u64 clk_reg_base;
> -	u32 i2c_index;
> +	u32 i2c_clk_offset;
> ?};
> ?
> ?static struct pmic pmic_inst;
> @@ -163,10 +163,7 @@ static void pmic_i2c_clk_enable(void)
> ?	unsigned long clock_base;
> ?	unsigned int val;
> ?
> -	clock_base = jh7110_inst.clk_reg_base +
> -		I2C_APB_CLK_OFFSET +
The I2C_APB_CLK_OFFSET is no longer needed and can be removed.

Regards,
Xiang W
> -		(jh7110_inst.i2c_index << 2);
> -
> +	clock_base = jh7110_inst.clk_reg_base + jh7110_inst.i2c_clk_offset;
> ?	val = readl((void *)clock_base);
> ?
> ?	if (!val)
> @@ -241,7 +238,8 @@ static struct fdt_reset fdt_reset_pmic = {
> ?static int starfive_jh7110_inst_init(void *fdt)
> ?{
> ?	int noff, rc = 0;
> -	const char *name;
> +	const fdt32_t *val;
> +	int len;
> ?	u64 addr;
> ?
> ?	noff = fdt_node_offset_by_compatible(fdt, -1, "starfive,jh7110-pmu");
> @@ -261,9 +259,12 @@ static int starfive_jh7110_inst_init(void *fdt)
> ?	}
> ?
> ?	if (pmic_inst.adapter) {
> -		name = fdt_get_name(fdt, pmic_inst.adapter->id, NULL);
> -		if (!sbi_strncmp(name, "i2c", 3))
> -			jh7110_inst.i2c_index = name[3] - '0';
> +		val = fdt_getprop(fdt, pmic_inst.adapter->id, "clocks", &len);
> +		/* The clocks property looks like this: clocks = <&syscrg JH7110_SYSCLK_I2C5_APB>;
> +		 * So check that the length is 8 bytes, and get the offset from the second value
> +		 */
> +		if (val && len == 8)
> +			jh7110_inst.i2c_clk_offset = fdt32_to_cpu(val[1]) << 2;
> ?		else
> ?			rc = SBI_EINVAL;
> ?	}
> -- 
> 2.39.2
> 
>