[RFC 0/5] updates for i3c error printing

[RFC 0/5] updates for i3c error printing

[Ben Dooks]

During work on an i3c compatible system some of the
probing does not produce much useful error output. This
series is an attempt to add error reporting where it
would be useful to track down the culprit.

Ben Dooks (5):
  i3c: show error with node for invalid reg property
  i3c: add error print to show device failing during populate bus
  i3c: show node when printing unsupported 10-bit i2c dev
  i3c: show error messages in of_i3c_master_add_i3c_boardinfo
  i3c: dw; add print if cannot get resources

 drivers/i3c/master.c               | 28 +++++++++++++++++++++-------
 drivers/i3c/master/dw-i3c-master.c | 12 +++++++++---
 2 files changed, 30 insertions(+), 10 deletions(-)

-- 
2.40.1

[RFC 1/5] i3c: show error with node for invalid reg property

[Ben Dooks]

When adding i3c devices via fdt, it is useful to get an error if the
reg node is wrong (say, you thought it was an i2c device and only
needed one value here) and have the error show the OF node which was
the cause of the problem (in case of many devices in the system),

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/i3c/master.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 08aeb69a7800..559fc2781a81 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -2094,8 +2094,10 @@ static int of_i3c_master_add_dev(struct i3c_master_controller *master,
 		return -EINVAL;
 
 	ret = of_property_read_u32_array(node, "reg", reg, ARRAY_SIZE(reg));
-	if (ret)
+	if (ret) {
+		dev_err(&master->dev, "%pOF: invalid reg property", node);
 		return ret;
+	}
 
 	/*
 	 * The manufacturer ID can't be 0. If reg[1] == 0 that means we're
-- 
2.40.1

[RFC 2/5] i3c: add error print to show device failing during populate bus

[Ben Dooks]

The of_populate_i3c_bus() does not produce much helpful output when
a device fails to add, so addd an explicit dev_err() showing the
device node that failed and the error code it failed with. This should
make finding bad device-tree entries easier.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/i3c/master.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 559fc2781a81..6316f3fc914a 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -2125,6 +2125,8 @@ static int of_populate_i3c_bus(struct i3c_master_controller *master)
 	for_each_available_child_of_node(i3cbus_np, node) {
 		ret = of_i3c_master_add_dev(master, node);
 		if (ret) {
+			dev_err(dev, "%pOF: failed to add device (%d)\n",
+				node, ret);
 			of_node_put(node);
 			return ret;
 		}
-- 
2.40.1

[RFC 3/5] i3c: show node when printing unsupported 10-bit i2c dev

[Ben Dooks]

When an i2c device is errored due to not being supported, the OF node
is not show (unlike errors from of_i2c_get_board_info). Make the failed
node explict in the of_i3c_master_add_i2c_boardinfo() to aid in tracking
down incorrect entries in the device tree.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/i3c/master.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 6316f3fc914a..bc42669f5c6d 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -2023,7 +2023,7 @@ of_i3c_master_add_i2c_boardinfo(struct i3c_master_controller *master,
 	 * DEFSLVS command.
 	 */
 	if (boardinfo->base.flags & I2C_CLIENT_TEN) {
-		dev_err(dev, "I2C device with 10 bit address not supported.");
+		dev_err(dev, "%pOF: I2C device with 10 bit address not supported.", node);
 		return -ENOTSUPP;
 	}
 
-- 
2.40.1

[RFC 4/5] i3c: show error messages in of_i3c_master_add_i3c_boardinfo

[Ben Dooks]

If of_i3c_master_add_i3c_boardinfo() fails, then there's no much to say
what the issue was (most of the error returns are -EINVAL), so add some
printing of the errors using dev_err() showing the node that failed and
what the issue was.

This should help with finding which device-tree node was causing the
issue and also mirrors the i2c case where it shows the node and the
error.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/i3c/master.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index bc42669f5c6d..2a9ebb1d9d57 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -2050,32 +2050,42 @@ of_i3c_master_add_i3c_boardinfo(struct i3c_master_controller *master,
 		return -ENOMEM;
 
 	if (reg[0]) {
-		if (reg[0] > I3C_MAX_ADDR)
+		if (reg[0] > I3C_MAX_ADDR) {
+			dev_err(dev, "%pOF: address too big\n", node);
 			return -EINVAL;
+		}
 
 		addrstatus = i3c_bus_get_addr_slot_status(&master->bus,
 							  reg[0]);
-		if (addrstatus != I3C_ADDR_SLOT_FREE)
+		if (addrstatus != I3C_ADDR_SLOT_FREE) {
+			dev_err(dev, "%pOF: slot in use\n", node);
 			return -EINVAL;
+		}
 	}
 
 	boardinfo->static_addr = reg[0];
 
 	if (!of_property_read_u32(node, "assigned-address", &init_dyn_addr)) {
-		if (init_dyn_addr > I3C_MAX_ADDR)
+		if (init_dyn_addr > I3C_MAX_ADDR) {
+			dev_err(dev, "%pOF: cannot assign address\n", node);
 			return -EINVAL;
+		}
 
 		addrstatus = i3c_bus_get_addr_slot_status(&master->bus,
 							  init_dyn_addr);
-		if (addrstatus != I3C_ADDR_SLOT_FREE)
+		if (addrstatus != I3C_ADDR_SLOT_FREE) {
+			dev_err(dev, "%pOF: slot in use\n", node);
 			return -EINVAL;
+		}
 	}
 
 	boardinfo->pid = ((u64)reg[1] << 32) | reg[2];
 
 	if ((boardinfo->pid & GENMASK_ULL(63, 48)) ||
-	    I3C_PID_RND_LOWER_32BITS(boardinfo->pid))
+	    I3C_PID_RND_LOWER_32BITS(boardinfo->pid)) {
+		dev_err(dev, "%pOF: bad PID\n", node);
 		return -EINVAL;
+	}
 
 	boardinfo->init_dyn_addr = init_dyn_addr;
 	boardinfo->of_node = of_node_get(node);
-- 
2.40.1

[RFC 5/5] i3c: dw; add print if cannot get resources

[Ben Dooks]

The devm_reset_control_get_optional_exclusive() call does
not print any errors, neiterh does the clk_prepare_enable
or devm_request_irq() call.

Add some basic error printing to make the probe failures
easier to debug.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/i3c/master/dw-i3c-master.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 9332ae5f6419..ffc84ff6225c 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -1429,12 +1429,16 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
 
 	master->core_rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
 								    "core_rst");
-	if (IS_ERR(master->core_rst))
+	if (IS_ERR(master->core_rst)) {
+		dev_err(&pdev->dev, "cannot get core_rst\n");
 		return PTR_ERR(master->core_rst);
+	}
 
 	ret = clk_prepare_enable(master->core_clk);
-	if (ret)
+	if (ret) {
+		dev_err(&pdev->dev, "cannot enable core_clk\n");
 		goto err_disable_core_clk;
+	}
 
 	reset_control_deassert(master->core_rst);
 
@@ -1446,8 +1450,10 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
 	ret = devm_request_irq(&pdev->dev, irq,
 			       dw_i3c_master_irq_handler, 0,
 			       dev_name(&pdev->dev), master);
-	if (ret)
+	if (ret) {
+		dev_err(&pdev->dev, "cannot get irq\n");
 		goto err_assert_rst;
+	}
 
 	platform_set_drvdata(pdev, master);
 
-- 
2.40.1

Re: [RFC 0/5] updates for i3c error printing

[Ben Dooks]

On 21/06/2023 17:20, Ben Dooks wrote:
> During work on an i3c compatible system some of the
> probing does not produce much useful error output. This
> series is an attempt to add error reporting where it
> would be useful to track down the culprit.
> 
> Ben Dooks (5):
>    i3c: show error with node for invalid reg property
>    i3c: add error print to show device failing during populate bus
>    i3c: show node when printing unsupported 10-bit i2c dev
>    i3c: show error messages in of_i3c_master_add_i3c_boardinfo
>    i3c: dw; add print if cannot get resources
> 
>   drivers/i3c/master.c               | 28 +++++++++++++++++++++-------
>   drivers/i3c/master/dw-i3c-master.c | 12 +++++++++---
>   2 files changed, 30 insertions(+), 10 deletions(-)

Has anyone had a chance to review these?

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Re: [RFC 5/5] i3c: dw; add print if cannot get resources

[Alexandre Belloni]

Hello,

Please fix the typo in the subject.

On 21/06/2023 17:20:05+0100, Ben Dooks wrote:
> The devm_reset_control_get_optional_exclusive() call does
> not print any errors, neiterh does the clk_prepare_enable
Also fix this one.

> or devm_request_irq() call.
> 
> Add some basic error printing to make the probe failures
> easier to debug.

I guess all those dev_err could be dev_dbg so we don't litter the driver
with strings that will only ever be useful during bring up.

> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/i3c/master/dw-i3c-master.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 9332ae5f6419..ffc84ff6225c 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -1429,12 +1429,16 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
>  
>  	master->core_rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
>  								    "core_rst");
> -	if (IS_ERR(master->core_rst))
> +	if (IS_ERR(master->core_rst)) {
> +		dev_err(&pdev->dev, "cannot get core_rst\n");
>  		return PTR_ERR(master->core_rst);
> +	}
>  
>  	ret = clk_prepare_enable(master->core_clk);
> -	if (ret)
> +	if (ret) {
> +		dev_err(&pdev->dev, "cannot enable core_clk\n");
>  		goto err_disable_core_clk;
> +	}
>  
>  	reset_control_deassert(master->core_rst);
>  
> @@ -1446,8 +1450,10 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
>  	ret = devm_request_irq(&pdev->dev, irq,
>  			       dw_i3c_master_irq_handler, 0,
>  			       dev_name(&pdev->dev), master);
> -	if (ret)
> +	if (ret) {
> +		dev_err(&pdev->dev, "cannot get irq\n");
>  		goto err_assert_rst;
> +	}
>  
>  	platform_set_drvdata(pdev, master);
>  
> -- 
> 2.40.1
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC 5/5] i3c: dw; add print if cannot get resources

[Ben Dooks]

On 04/07/2023 22:37, Alexandre Belloni wrote:
> Hello,
> 
> Please fix the typo in the subject.
> 
> On 21/06/2023 17:20:05+0100, Ben Dooks wrote:
>> The devm_reset_control_get_optional_exclusive() call does
>> not print any errors, neiterh does the clk_prepare_enable
> Also fix this one.
> 
>> or devm_request_irq() call.
>>
>> Add some basic error printing to make the probe failures
>> easier to debug.
> 
> I guess all those dev_err could be dev_dbg so we don't litter the driver
> with strings that will only ever be useful during bring up.

I think dev_err_probe() is probably the right one as we aren't going
to want to warn on any sort of probe defer errors. However most drivers
are printing errors if there are resources missing so I think an error
is the best way to report issues.

I'll correct the spelling when the discussions about the correct way to
print errors is.

>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>>   drivers/i3c/master/dw-i3c-master.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
>> index 9332ae5f6419..ffc84ff6225c 100644
>> --- a/drivers/i3c/master/dw-i3c-master.c
>> +++ b/drivers/i3c/master/dw-i3c-master.c
>> @@ -1429,12 +1429,16 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
>>   
>>   	master->core_rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
>>   								    "core_rst");
>> -	if (IS_ERR(master->core_rst))
>> +	if (IS_ERR(master->core_rst)) {
>> +		dev_err(&pdev->dev, "cannot get core_rst\n");
>>   		return PTR_ERR(master->core_rst);
>> +	}
>>   
>>   	ret = clk_prepare_enable(master->core_clk);
>> -	if (ret)
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "cannot enable core_clk\n");
>>   		goto err_disable_core_clk;
>> +	}
>>   
>>   	reset_control_deassert(master->core_rst);
>>   
>> @@ -1446,8 +1450,10 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
>>   	ret = devm_request_irq(&pdev->dev, irq,
>>   			       dw_i3c_master_irq_handler, 0,
>>   			       dev_name(&pdev->dev), master);
>> -	if (ret)
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "cannot get irq\n");
>>   		goto err_assert_rst;
>> +	}
>>   
>>   	platform_set_drvdata(pdev, master);
>>   
>> -- 
>> 2.40.1
>>
> 

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html