All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/4] clk: rs9: Check for vendor/device ID
@ 2023-01-10 10:00 Alexander Stein
  2023-01-10 10:00 ` [PATCH v2 2/4] dt-bindings: clk: rs9: Add 9FGV0441 Alexander Stein
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Alexander Stein @ 2023-01-10 10:00 UTC (permalink / raw)
  To: Geert Uytterhoeven, Michael Turquette, Stephen Boyd, Rob Herring,
	Krzysztof Kozlowski, Marek Vasut
  Cc: Alexander Stein, linux-renesas-soc, linux-clk, devicetree

This is in preparation to support additional devices which have different
IDs as well as a slightly different register layout.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes in v2:
* Use dev_err_probe to include return statement in one line

 drivers/clk/clk-renesas-pcie.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
index e6247141d0c05..bba09a88c2ccc 100644
--- a/drivers/clk/clk-renesas-pcie.c
+++ b/drivers/clk/clk-renesas-pcie.c
@@ -45,6 +45,13 @@
 #define RS9_REG_DID				0x6
 #define RS9_REG_BCP				0x7
 
+#define RS9_REG_VID_IDT				0x01
+
+#define RS9_REG_DID_TYPE_FGV			(0x0 << RS9_REG_DID_TYPE_SHIFT)
+#define RS9_REG_DID_TYPE_DBV			(0x1 << RS9_REG_DID_TYPE_SHIFT)
+#define RS9_REG_DID_TYPE_DMV			(0x2 << RS9_REG_DID_TYPE_SHIFT)
+#define RS9_REG_DID_TYPE_SHIFT			0x6
+
 /* Supported Renesas 9-series models. */
 enum rs9_model {
 	RENESAS_9FGV0241,
@@ -54,6 +61,7 @@ enum rs9_model {
 struct rs9_chip_info {
 	const enum rs9_model	model;
 	unsigned int		num_clks;
+	u8			did;
 };
 
 struct rs9_driver_data {
@@ -270,6 +278,7 @@ static int rs9_probe(struct i2c_client *client)
 {
 	unsigned char name[5] = "DIF0";
 	struct rs9_driver_data *rs9;
+	unsigned int vid, did;
 	struct clk_hw *hw;
 	int i, ret;
 
@@ -306,6 +315,20 @@ static int rs9_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
+	ret = regmap_read(rs9->regmap, RS9_REG_VID, &vid);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_read(rs9->regmap, RS9_REG_DID, &did);
+	if (ret < 0)
+		return ret;
+
+	if (vid != RS9_REG_VID_IDT || did != rs9->chip_info->did)
+		return dev_err_probe(&client->dev, -ENODEV,
+				     "Incorrect VID/DID: %#02x, %#02x. Expected %#02x, %#02x\n",
+				     vid, did, RS9_REG_VID_IDT,
+				     rs9->chip_info->did);
+
 	/* Register clock */
 	for (i = 0; i < rs9->chip_info->num_clks; i++) {
 		snprintf(name, 5, "DIF%d", i);
@@ -349,6 +372,7 @@ static int __maybe_unused rs9_resume(struct device *dev)
 static const struct rs9_chip_info renesas_9fgv0241_info = {
 	.model		= RENESAS_9FGV0241,
 	.num_clks	= 2,
+	.did		= RS9_REG_DID_TYPE_FGV | 0x02,
 };
 
 static const struct i2c_device_id rs9_id[] = {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 2/4] dt-bindings: clk: rs9: Add 9FGV0441
  2023-01-10 10:00 [PATCH v2 1/4] clk: rs9: Check for vendor/device ID Alexander Stein
@ 2023-01-10 10:00 ` Alexander Stein
  2023-01-10 10:29   ` Marek Vasut
  2023-01-10 10:00 ` [PATCH v2 3/4] clk: rs9: Support device specific dif bit calculation Alexander Stein
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Alexander Stein @ 2023-01-10 10:00 UTC (permalink / raw)
  To: Geert Uytterhoeven, Michael Turquette, Stephen Boyd, Rob Herring,
	Krzysztof Kozlowski, Marek Vasut
  Cc: Alexander Stein, linux-renesas-soc, linux-clk, devicetree,
	Krzysztof Kozlowski

This is a 4-channel variant of 9FGV series.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
Changes in v2:
* Add Krzysztof's A-b

 .../devicetree/bindings/clock/renesas,9series.yaml          | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/renesas,9series.yaml b/Documentation/devicetree/bindings/clock/renesas,9series.yaml
index 6b6cec3fba528..3afdebdb52ad4 100644
--- a/Documentation/devicetree/bindings/clock/renesas,9series.yaml
+++ b/Documentation/devicetree/bindings/clock/renesas,9series.yaml
@@ -16,6 +16,11 @@ description: |
   - 9FGV0241:
     0 -- DIF0
     1 -- DIF1
+  - 9FGV0441:
+    0 -- DIF0
+    1 -- DIF1
+    2 -- DIF2
+    3 -- DIF3
 
 maintainers:
   - Marek Vasut <marex@denx.de>
@@ -24,6 +29,7 @@ properties:
   compatible:
     enum:
       - renesas,9fgv0241
+      - renesas,9fgv0441
 
   reg:
     description: I2C device address
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 3/4] clk: rs9: Support device specific dif bit calculation
  2023-01-10 10:00 [PATCH v2 1/4] clk: rs9: Check for vendor/device ID Alexander Stein
  2023-01-10 10:00 ` [PATCH v2 2/4] dt-bindings: clk: rs9: Add 9FGV0441 Alexander Stein
@ 2023-01-10 10:00 ` Alexander Stein
  2023-01-10 10:31   ` Marek Vasut
  2023-01-10 10:00 ` [PATCH v2 4/4] clk: rs9: Add support for 9FGV0441 Alexander Stein
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Alexander Stein @ 2023-01-10 10:00 UTC (permalink / raw)
  To: Geert Uytterhoeven, Michael Turquette, Stephen Boyd, Rob Herring,
	Krzysztof Kozlowski, Marek Vasut
  Cc: Alexander Stein, linux-renesas-soc, linux-clk, devicetree

The calculation DIFx is BIT(n) +1 is only true for 9FGV0241. With
additional devices this is getting more complicated.
Support a base bit for the DIF calculation, currently only devices
with consecutive bits are supported, e.g. the 6-channel device needs
additional logic.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes in v2:
* Use a common function instead of callback for calculating the DIF bit

 drivers/clk/clk-renesas-pcie.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
index bba09a88c2ccc..6b19186228238 100644
--- a/drivers/clk/clk-renesas-pcie.c
+++ b/drivers/clk/clk-renesas-pcie.c
@@ -18,7 +18,6 @@
 #include <linux/regmap.h>
 
 #define RS9_REG_OE				0x0
-#define RS9_REG_OE_DIF_OE(n)			BIT((n) + 1)
 #define RS9_REG_SS				0x1
 #define RS9_REG_SS_AMP_0V6			0x0
 #define RS9_REG_SS_AMP_0V7			0x1
@@ -31,9 +30,6 @@
 #define RS9_REG_SS_SSC_MASK			(3 << 3)
 #define RS9_REG_SS_SSC_LOCK			BIT(5)
 #define RS9_REG_SR				0x2
-#define RS9_REG_SR_2V0_DIF(n)			0
-#define RS9_REG_SR_3V0_DIF(n)			BIT((n) + 1)
-#define RS9_REG_SR_DIF_MASK(n)		BIT((n) + 1)
 #define RS9_REG_REF				0x3
 #define RS9_REG_REF_OE				BIT(4)
 #define RS9_REG_REF_OD				BIT(5)
@@ -160,17 +156,27 @@ static const struct regmap_config rs9_regmap_config = {
 	.reg_read = rs9_regmap_i2c_read,
 };
 
+static u8 rs9_calc_dif(const struct rs9_driver_data *rs9, int idx)
+{
+	enum rs9_model model = rs9->chip_info->model;
+
+	if (model == RENESAS_9FGV0241)
+		return BIT(idx) + 1;
+
+	return 0;
+}
+
 static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx)
 {
 	struct i2c_client *client = rs9->client;
+	u8 dif = rs9_calc_dif(rs9, idx);
 	unsigned char name[5] = "DIF0";
 	struct device_node *np;
 	int ret;
 	u32 sr;
 
 	/* Set defaults */
-	rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx);
-	rs9->clk_dif_sr |= RS9_REG_SR_3V0_DIF(idx);
+	rs9->clk_dif_sr |= dif;
 
 	snprintf(name, 5, "DIF%d", idx);
 	np = of_get_child_by_name(client->dev.of_node, name);
@@ -182,11 +188,9 @@ static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx)
 	of_node_put(np);
 	if (!ret) {
 		if (sr == 2000000) {		/* 2V/ns */
-			rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx);
-			rs9->clk_dif_sr |= RS9_REG_SR_2V0_DIF(idx);
+			rs9->clk_dif_sr &= ~dif;
 		} else if (sr == 3000000) {	/* 3V/ns (default) */
-			rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx);
-			rs9->clk_dif_sr |= RS9_REG_SR_3V0_DIF(idx);
+			rs9->clk_dif_sr |= dif;
 		} else
 			ret = dev_err_probe(&client->dev, -EINVAL,
 					    "Invalid renesas,slew-rate value\n");
@@ -257,11 +261,13 @@ static void rs9_update_config(struct rs9_driver_data *rs9)
 	}
 
 	for (i = 0; i < rs9->chip_info->num_clks; i++) {
-		if (rs9->clk_dif_sr & RS9_REG_SR_3V0_DIF(i))
+		u8 dif = rs9_calc_dif(rs9, i);
+
+		if (rs9->clk_dif_sr & dif)
 			continue;
 
-		regmap_update_bits(rs9->regmap, RS9_REG_SR, RS9_REG_SR_3V0_DIF(i),
-				   rs9->clk_dif_sr & RS9_REG_SR_3V0_DIF(i));
+		regmap_update_bits(rs9->regmap, RS9_REG_SR, dif,
+				   rs9->clk_dif_sr & dif);
 	}
 }
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 4/4] clk: rs9: Add support for 9FGV0441
  2023-01-10 10:00 [PATCH v2 1/4] clk: rs9: Check for vendor/device ID Alexander Stein
  2023-01-10 10:00 ` [PATCH v2 2/4] dt-bindings: clk: rs9: Add 9FGV0441 Alexander Stein
  2023-01-10 10:00 ` [PATCH v2 3/4] clk: rs9: Support device specific dif bit calculation Alexander Stein
@ 2023-01-10 10:00 ` Alexander Stein
  2023-01-10 10:32   ` Marek Vasut
  2023-01-10 10:28 ` [PATCH v2 1/4] clk: rs9: Check for vendor/device ID Marek Vasut
  2023-02-13  7:28 ` Alexander Stein
  4 siblings, 1 reply; 13+ messages in thread
From: Alexander Stein @ 2023-01-10 10:00 UTC (permalink / raw)
  To: Geert Uytterhoeven, Michael Turquette, Stephen Boyd, Rob Herring,
	Krzysztof Kozlowski, Marek Vasut
  Cc: Alexander Stein, linux-renesas-soc, linux-clk, devicetree

This model is similar to 9FGV0241, but the DIFx bits start at bit 0.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes in v2:
* Adjust to common DIF calculation function

 drivers/clk/clk-renesas-pcie.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
index 6b19186228238..a4ebb224181bf 100644
--- a/drivers/clk/clk-renesas-pcie.c
+++ b/drivers/clk/clk-renesas-pcie.c
@@ -6,6 +6,7 @@
  *   - 9FGV/9DBV/9DMV/9FGL/9DML/9QXL/9SQ
  * Currently supported:
  *   - 9FGV0241
+ *   - 9FGV0441
  *
  * Copyright (C) 2022 Marek Vasut <marex@denx.de>
  */
@@ -51,6 +52,7 @@
 /* Supported Renesas 9-series models. */
 enum rs9_model {
 	RENESAS_9FGV0241,
+	RENESAS_9FGV0441,
 };
 
 /* Structure to describe features of a particular 9-series model */
@@ -65,7 +67,7 @@ struct rs9_driver_data {
 	struct regmap		*regmap;
 	const struct rs9_chip_info *chip_info;
 	struct clk		*pin_xin;
-	struct clk_hw		*clk_dif[2];
+	struct clk_hw		*clk_dif[4];
 	u8			pll_amplitude;
 	u8			pll_ssc;
 	u8			clk_dif_sr;
@@ -162,6 +164,8 @@ static u8 rs9_calc_dif(const struct rs9_driver_data *rs9, int idx)
 
 	if (model == RENESAS_9FGV0241)
 		return BIT(idx) + 1;
+	else if (model == RENESAS_9FGV0441)
+		return BIT(idx);
 
 	return 0;
 }
@@ -381,14 +385,22 @@ static const struct rs9_chip_info renesas_9fgv0241_info = {
 	.did		= RS9_REG_DID_TYPE_FGV | 0x02,
 };
 
+static const struct rs9_chip_info renesas_9fgv0441_info = {
+	.model		= RENESAS_9FGV0441,
+	.num_clks	= 4,
+	.did		= RS9_REG_DID_TYPE_FGV | 0x04,
+};
+
 static const struct i2c_device_id rs9_id[] = {
 	{ "9fgv0241", .driver_data = RENESAS_9FGV0241 },
+	{ "9fgv0441", .driver_data = RENESAS_9FGV0441 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, rs9_id);
 
 static const struct of_device_id clk_rs9_of_match[] = {
 	{ .compatible = "renesas,9fgv0241", .data = &renesas_9fgv0241_info },
+	{ .compatible = "renesas,9fgv0441", .data = &renesas_9fgv0441_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, clk_rs9_of_match);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/4] clk: rs9: Check for vendor/device ID
  2023-01-10 10:00 [PATCH v2 1/4] clk: rs9: Check for vendor/device ID Alexander Stein
                   ` (2 preceding siblings ...)
  2023-01-10 10:00 ` [PATCH v2 4/4] clk: rs9: Add support for 9FGV0441 Alexander Stein
@ 2023-01-10 10:28 ` Marek Vasut
  2023-02-13  7:28 ` Alexander Stein
  4 siblings, 0 replies; 13+ messages in thread
From: Marek Vasut @ 2023-01-10 10:28 UTC (permalink / raw)
  To: Alexander Stein, Geert Uytterhoeven, Michael Turquette,
	Stephen Boyd, Rob Herring, Krzysztof Kozlowski
  Cc: linux-renesas-soc, linux-clk, devicetree

On 1/10/23 11:00, Alexander Stein wrote:
> This is in preparation to support additional devices which have different
> IDs as well as a slightly different register layout.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Reviewed-by: Marek Vasut <marex@denx.de>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] dt-bindings: clk: rs9: Add 9FGV0441
  2023-01-10 10:00 ` [PATCH v2 2/4] dt-bindings: clk: rs9: Add 9FGV0441 Alexander Stein
@ 2023-01-10 10:29   ` Marek Vasut
  0 siblings, 0 replies; 13+ messages in thread
From: Marek Vasut @ 2023-01-10 10:29 UTC (permalink / raw)
  To: Alexander Stein, Geert Uytterhoeven, Michael Turquette,
	Stephen Boyd, Rob Herring, Krzysztof Kozlowski
  Cc: linux-renesas-soc, linux-clk, devicetree, Krzysztof Kozlowski

On 1/10/23 11:00, Alexander Stein wrote:
> This is a 4-channel variant of 9FGV series.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Reviewed-by: Marek Vasut <marex@denx.de>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] clk: rs9: Support device specific dif bit calculation
  2023-01-10 10:00 ` [PATCH v2 3/4] clk: rs9: Support device specific dif bit calculation Alexander Stein
@ 2023-01-10 10:31   ` Marek Vasut
  2023-01-10 13:22     ` Alexander Stein
  0 siblings, 1 reply; 13+ messages in thread
From: Marek Vasut @ 2023-01-10 10:31 UTC (permalink / raw)
  To: Alexander Stein, Geert Uytterhoeven, Michael Turquette,
	Stephen Boyd, Rob Herring, Krzysztof Kozlowski
  Cc: linux-renesas-soc, linux-clk, devicetree

On 1/10/23 11:00, Alexander Stein wrote:

[...]

>   static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx)
>   {
>   	struct i2c_client *client = rs9->client;
> +	u8 dif = rs9_calc_dif(rs9, idx);
>   	unsigned char name[5] = "DIF0";
>   	struct device_node *np;
>   	int ret;
>   	u32 sr;
>   
>   	/* Set defaults */
> -	rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx);

Are you sure this line ^ should be dropped ?
Shouldn't the bitfield be cleared first and modified second?

> -	rs9->clk_dif_sr |= RS9_REG_SR_3V0_DIF(idx);
> +	rs9->clk_dif_sr |= dif;
>   
>   	snprintf(name, 5, "DIF%d", idx);
>   	np = of_get_child_by_name(client->dev.of_node, name);

[...]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 4/4] clk: rs9: Add support for 9FGV0441
  2023-01-10 10:00 ` [PATCH v2 4/4] clk: rs9: Add support for 9FGV0441 Alexander Stein
@ 2023-01-10 10:32   ` Marek Vasut
  0 siblings, 0 replies; 13+ messages in thread
From: Marek Vasut @ 2023-01-10 10:32 UTC (permalink / raw)
  To: Alexander Stein, Geert Uytterhoeven, Michael Turquette,
	Stephen Boyd, Rob Herring, Krzysztof Kozlowski
  Cc: linux-renesas-soc, linux-clk, devicetree

On 1/10/23 11:00, Alexander Stein wrote:
> This model is similar to 9FGV0241, but the DIFx bits start at bit 0.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Reviewed-by: Marek Vasut <marex@denx.de>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] clk: rs9: Support device specific dif bit calculation
  2023-01-10 10:31   ` Marek Vasut
@ 2023-01-10 13:22     ` Alexander Stein
  2023-01-10 13:37       ` Marek Vasut
  0 siblings, 1 reply; 13+ messages in thread
From: Alexander Stein @ 2023-01-10 13:22 UTC (permalink / raw)
  To: Geert Uytterhoeven, Michael Turquette, Stephen Boyd, Rob Herring,
	Krzysztof Kozlowski, Marek Vasut
  Cc: linux-renesas-soc, linux-clk, devicetree

Hi Marek,

thanks for your feedback.

Am Dienstag, 10. Januar 2023, 11:31:49 CET schrieb Marek Vasut:
> On 1/10/23 11:00, Alexander Stein wrote:
> 
> [...]
> 
> >   static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx)
> >   {
> >   
> >   	struct i2c_client *client = rs9->client;
> > 
> > +	u8 dif = rs9_calc_dif(rs9, idx);
> > 
> >   	unsigned char name[5] = "DIF0";
> >   	struct device_node *np;
> >   	int ret;
> >   	u32 sr;
> >   	
> >   	/* Set defaults */
> > 
> > -	rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx);
> 
> Are you sure this line ^ should be dropped ?
> Shouldn't the bitfield be cleared first and modified second?

Well, I had in my mind that this function is called upon probe with clk_dif_sr 
being cleared anyway, so this does essentially nothing. And the DIF bit is set 
unconditionally, so what is the point of masking it before?

Best regards,
Alexander

> > -	rs9->clk_dif_sr |= RS9_REG_SR_3V0_DIF(idx);
> > +	rs9->clk_dif_sr |= dif;
> > 
> >   	snprintf(name, 5, "DIF%d", idx);
> >   	np = of_get_child_by_name(client->dev.of_node, name);
> 
> [...]





^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] clk: rs9: Support device specific dif bit calculation
  2023-01-10 13:22     ` Alexander Stein
@ 2023-01-10 13:37       ` Marek Vasut
  2023-01-10 13:47         ` Alexander Stein
  0 siblings, 1 reply; 13+ messages in thread
From: Marek Vasut @ 2023-01-10 13:37 UTC (permalink / raw)
  To: Alexander Stein, Geert Uytterhoeven, Michael Turquette,
	Stephen Boyd, Rob Herring, Krzysztof Kozlowski
  Cc: linux-renesas-soc, linux-clk, devicetree

On 1/10/23 14:22, Alexander Stein wrote:
> Hi Marek,

Hi,

> thanks for your feedback.
> 
> Am Dienstag, 10. Januar 2023, 11:31:49 CET schrieb Marek Vasut:
>> On 1/10/23 11:00, Alexander Stein wrote:
>>
>> [...]
>>
>>>    static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx)
>>>    {
>>>    
>>>    	struct i2c_client *client = rs9->client;
>>>
>>> +	u8 dif = rs9_calc_dif(rs9, idx);
>>>
>>>    	unsigned char name[5] = "DIF0";
>>>    	struct device_node *np;
>>>    	int ret;
>>>    	u32 sr;
>>>    	
>>>    	/* Set defaults */
>>>
>>> -	rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx);
>>
>> Are you sure this line ^ should be dropped ?
>> Shouldn't the bitfield be cleared first and modified second?
> 
> Well, I had in my mind that this function is called upon probe with clk_dif_sr
> being cleared anyway, so this does essentially nothing. And the DIF bit is set
> unconditionally, so what is the point of masking it before?

Good point, but then, what's the point of ORRing either ? Just do a 
plain assignment.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] clk: rs9: Support device specific dif bit calculation
  2023-01-10 13:37       ` Marek Vasut
@ 2023-01-10 13:47         ` Alexander Stein
  2023-01-10 13:51           ` Marek Vasut
  0 siblings, 1 reply; 13+ messages in thread
From: Alexander Stein @ 2023-01-10 13:47 UTC (permalink / raw)
  To: Geert Uytterhoeven, Michael Turquette, Stephen Boyd, Rob Herring,
	Krzysztof Kozlowski, Marek Vasut
  Cc: linux-renesas-soc, linux-clk, devicetree

Hello Marek,

Am Dienstag, 10. Januar 2023, 14:37:19 CET schrieb Marek Vasut:
> On 1/10/23 14:22, Alexander Stein wrote:
> > Hi Marek,
> 
> Hi,
> 
> > thanks for your feedback.
> > 
> > Am Dienstag, 10. Januar 2023, 11:31:49 CET schrieb Marek Vasut:
> >> On 1/10/23 11:00, Alexander Stein wrote:
> >> 
> >> [...]
> >> 
> >>>    static int rs9_get_output_config(struct rs9_driver_data *rs9, int
> >>>    idx)
> >>>    {
> >>>    
> >>>    	struct i2c_client *client = rs9->client;
> >>> 
> >>> +	u8 dif = rs9_calc_dif(rs9, idx);
> >>> 
> >>>    	unsigned char name[5] = "DIF0";
> >>>    	struct device_node *np;
> >>>    	int ret;
> >>>    	u32 sr;
> >>>    	
> >>>    	/* Set defaults */
> >>> 
> >>> -	rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx);
> >> 
> >> Are you sure this line ^ should be dropped ?
> >> Shouldn't the bitfield be cleared first and modified second?
> > 
> > Well, I had in my mind that this function is called upon probe with
> > clk_dif_sr being cleared anyway, so this does essentially nothing. And
> > the DIF bit is set unconditionally, so what is the point of masking it
> > before?
> 
> Good point, but then, what's the point of ORRing either ? Just do a
> plain assignment.

OR-ring is necessary as this function is called for each DIF output (see idx 
parameter), so plain assignment will clear the previously set bits.

Best regards,
Alexander




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] clk: rs9: Support device specific dif bit calculation
  2023-01-10 13:47         ` Alexander Stein
@ 2023-01-10 13:51           ` Marek Vasut
  0 siblings, 0 replies; 13+ messages in thread
From: Marek Vasut @ 2023-01-10 13:51 UTC (permalink / raw)
  To: Alexander Stein, Geert Uytterhoeven, Michael Turquette,
	Stephen Boyd, Rob Herring, Krzysztof Kozlowski
  Cc: linux-renesas-soc, linux-clk, devicetree

On 1/10/23 14:47, Alexander Stein wrote:
> Hello Marek,
> 
> Am Dienstag, 10. Januar 2023, 14:37:19 CET schrieb Marek Vasut:
>> On 1/10/23 14:22, Alexander Stein wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> thanks for your feedback.
>>>
>>> Am Dienstag, 10. Januar 2023, 11:31:49 CET schrieb Marek Vasut:
>>>> On 1/10/23 11:00, Alexander Stein wrote:
>>>>
>>>> [...]
>>>>
>>>>>     static int rs9_get_output_config(struct rs9_driver_data *rs9, int
>>>>>     idx)
>>>>>     {
>>>>>     
>>>>>     	struct i2c_client *client = rs9->client;
>>>>>
>>>>> +	u8 dif = rs9_calc_dif(rs9, idx);
>>>>>
>>>>>     	unsigned char name[5] = "DIF0";
>>>>>     	struct device_node *np;
>>>>>     	int ret;
>>>>>     	u32 sr;
>>>>>     	
>>>>>     	/* Set defaults */
>>>>>
>>>>> -	rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx);
>>>>
>>>> Are you sure this line ^ should be dropped ?
>>>> Shouldn't the bitfield be cleared first and modified second?
>>>
>>> Well, I had in my mind that this function is called upon probe with
>>> clk_dif_sr being cleared anyway, so this does essentially nothing. And
>>> the DIF bit is set unconditionally, so what is the point of masking it
>>> before?
>>
>> Good point, but then, what's the point of ORRing either ? Just do a
>> plain assignment.
> 
> OR-ring is necessary as this function is called for each DIF output (see idx
> parameter), so plain assignment will clear the previously set bits.

Ah, got it.

Reviewed-by: Marek Vasut <marex@denx.de>

Thanks !

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/4] clk: rs9: Check for vendor/device ID
  2023-01-10 10:00 [PATCH v2 1/4] clk: rs9: Check for vendor/device ID Alexander Stein
                   ` (3 preceding siblings ...)
  2023-01-10 10:28 ` [PATCH v2 1/4] clk: rs9: Check for vendor/device ID Marek Vasut
@ 2023-02-13  7:28 ` Alexander Stein
  4 siblings, 0 replies; 13+ messages in thread
From: Alexander Stein @ 2023-02-13  7:28 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Geert Uytterhoeven, Michael Turquette, Rob Herring,
	Krzysztof Kozlowski, Marek Vasut, linux-renesas-soc, linux-clk,
	devicetree

Hi Stephen,

want me to resend adding all Reviewed-By?

Best regards,
Alexander

Am Dienstag, 10. Januar 2023, 11:00:00 CET schrieb Alexander Stein:
> This is in preparation to support additional devices which have different
> IDs as well as a slightly different register layout.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Changes in v2:
> * Use dev_err_probe to include return statement in one line
> 
>  drivers/clk/clk-renesas-pcie.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
> index e6247141d0c05..bba09a88c2ccc 100644
> --- a/drivers/clk/clk-renesas-pcie.c
> +++ b/drivers/clk/clk-renesas-pcie.c
> @@ -45,6 +45,13 @@
>  #define RS9_REG_DID				0x6
>  #define RS9_REG_BCP				0x7
> 
> +#define RS9_REG_VID_IDT				0x01
> +
> +#define RS9_REG_DID_TYPE_FGV			(0x0 << 
RS9_REG_DID_TYPE_SHIFT)
> +#define RS9_REG_DID_TYPE_DBV			(0x1 << 
RS9_REG_DID_TYPE_SHIFT)
> +#define RS9_REG_DID_TYPE_DMV			(0x2 << 
RS9_REG_DID_TYPE_SHIFT)
> +#define RS9_REG_DID_TYPE_SHIFT			0x6
> +
>  /* Supported Renesas 9-series models. */
>  enum rs9_model {
>  	RENESAS_9FGV0241,
> @@ -54,6 +61,7 @@ enum rs9_model {
>  struct rs9_chip_info {
>  	const enum rs9_model	model;
>  	unsigned int		num_clks;
> +	u8			did;
>  };
> 
>  struct rs9_driver_data {
> @@ -270,6 +278,7 @@ static int rs9_probe(struct i2c_client *client)
>  {
>  	unsigned char name[5] = "DIF0";
>  	struct rs9_driver_data *rs9;
> +	unsigned int vid, did;
>  	struct clk_hw *hw;
>  	int i, ret;
> 
> @@ -306,6 +315,20 @@ static int rs9_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		return ret;
> 
> +	ret = regmap_read(rs9->regmap, RS9_REG_VID, &vid);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_read(rs9->regmap, RS9_REG_DID, &did);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (vid != RS9_REG_VID_IDT || did != rs9->chip_info->did)
> +		return dev_err_probe(&client->dev, -ENODEV,
> +				     "Incorrect VID/DID: %#02x, %#02x. 
Expected %#02x, %#02x\n",
> +				     vid, did, RS9_REG_VID_IDT,
> +				     rs9->chip_info->did);
> +
>  	/* Register clock */
>  	for (i = 0; i < rs9->chip_info->num_clks; i++) {
>  		snprintf(name, 5, "DIF%d", i);
> @@ -349,6 +372,7 @@ static int __maybe_unused rs9_resume(struct device *dev)
> static const struct rs9_chip_info renesas_9fgv0241_info = {
>  	.model		= RENESAS_9FGV0241,
>  	.num_clks	= 2,
> +	.did		= RS9_REG_DID_TYPE_FGV | 0x02,
>  };
> 
>  static const struct i2c_device_id rs9_id[] = {

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/



^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-02-13  7:28 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-10 10:00 [PATCH v2 1/4] clk: rs9: Check for vendor/device ID Alexander Stein
2023-01-10 10:00 ` [PATCH v2 2/4] dt-bindings: clk: rs9: Add 9FGV0441 Alexander Stein
2023-01-10 10:29   ` Marek Vasut
2023-01-10 10:00 ` [PATCH v2 3/4] clk: rs9: Support device specific dif bit calculation Alexander Stein
2023-01-10 10:31   ` Marek Vasut
2023-01-10 13:22     ` Alexander Stein
2023-01-10 13:37       ` Marek Vasut
2023-01-10 13:47         ` Alexander Stein
2023-01-10 13:51           ` Marek Vasut
2023-01-10 10:00 ` [PATCH v2 4/4] clk: rs9: Add support for 9FGV0441 Alexander Stein
2023-01-10 10:32   ` Marek Vasut
2023-01-10 10:28 ` [PATCH v2 1/4] clk: rs9: Check for vendor/device ID Marek Vasut
2023-02-13  7:28 ` Alexander Stein

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.