diff mbox series

[5/9] i2c: qcom-cci: initialize CCI controller after registration of adapters

Message ID 20220203164705.1712027-1-vladimir.zapolskiy@linaro.org (mailing list archive)
State Not Applicable
Headers show
Series i2c: qcom-cci: fixes and updates | expand

Commit Message

Vladimir Zapolskiy Feb. 3, 2022, 4:47 p.m. UTC
The change is wanted to postpone initialization of busses on CCI controller
by cci_init() and cci_reset() till adapters are registered, the later is
needed for adding I2C bus devices and get correspondent vbus regulators.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

Comments

Loic Poulain Feb. 3, 2022, 5:29 p.m. UTC | #1
Hi Vladimir,

On Thu, 3 Feb 2022 at 17:47, Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> The change is wanted to postpone initialization of busses on CCI controller
> by cci_init() and cci_reset() till adapters are registered, the later is
> needed for adding I2C bus devices and get correspondent vbus regulators.

This is odd, I don't think it's a good idea to register an adapter
which is not yet initialized. Could you elaborate on why you need to
do this, if you can't access the controller without this regulator
enabled, maybe it is more than vbus supply, and, in that case, it
should be enabled from your probe function.

Regards,
Loic





>
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index cf54f1cb4c57..eebf9603d3d1 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -630,14 +630,6 @@ static int cci_probe(struct platform_device *pdev)
>         val = readl(cci->base + CCI_HW_VERSION);
>         dev_dbg(dev, "CCI HW version = 0x%08x", val);
>
> -       ret = cci_reset(cci);
> -       if (ret < 0)
> -               goto error;
> -
> -       ret = cci_init(cci);
> -       if (ret < 0)
> -               goto error;
> -
>         for (i = 0; i < cci->data->num_masters; i++) {
>                 if (!cci->master[i].cci)
>                         continue;
> @@ -649,6 +641,14 @@ static int cci_probe(struct platform_device *pdev)
>                 }
>         }
>
> +       ret = cci_reset(cci);
> +       if (ret < 0)
> +               goto error_i2c;
> +
> +       ret = cci_init(cci);
> +       if (ret < 0)
> +               goto error_i2c;
> +
>         pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
>         pm_runtime_use_autosuspend(dev);
>         pm_runtime_set_active(dev);
> @@ -663,7 +663,6 @@ static int cci_probe(struct platform_device *pdev)
>                         of_node_put(cci->master[i].adap.dev.of_node);
>                 }
>         }
> -error:
>         disable_irq(cci->irq);
>  disable_clocks:
>         cci_disable_clocks(cci);
> --
> 2.33.0
>
Vladimir Zapolskiy Feb. 3, 2022, 6:45 p.m. UTC | #2
Hi Loic,

On 2/3/22 7:29 PM, Loic Poulain wrote:
> Hi Vladimir,
> 
> On Thu, 3 Feb 2022 at 17:47, Vladimir Zapolskiy
> <vladimir.zapolskiy@linaro.org> wrote:
>>
>> The change is wanted to postpone initialization of busses on CCI controller
>> by cci_init() and cci_reset() till adapters are registered, the later is
>> needed for adding I2C bus devices and get correspondent vbus regulators.
> 
> This is odd, I don't think it's a good idea to register an adapter
> which is not yet initialized. Could you elaborate on why you need to
> do this, if you can't access the controller without this regulator
> enabled, maybe it is more than vbus supply, and, in that case, it
> should be enabled from your probe function.

thank you for review, the controller can be accessed without a vbus regulator,
but I2C devices connected to the master controller can not.

The registration of a master controller device done by i2c_add_adapter()
should be safe to defer IMO, because there shall be no communication on
the bus at this point, there are no slaves before probe completion, thus
cci_init()/cci_reset() can be safely called afterwards.

The rationale of the change is to merge two loops over busses, see change 6/9,
keeping two loops extremely complicates the proper resource management.

--
Best wishes,
Vladimir

>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>> ---
>>   drivers/i2c/busses/i2c-qcom-cci.c | 17 ++++++++---------
>>   1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
>> index cf54f1cb4c57..eebf9603d3d1 100644
>> --- a/drivers/i2c/busses/i2c-qcom-cci.c
>> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
>> @@ -630,14 +630,6 @@ static int cci_probe(struct platform_device *pdev)
>>          val = readl(cci->base + CCI_HW_VERSION);
>>          dev_dbg(dev, "CCI HW version = 0x%08x", val);
>>
>> -       ret = cci_reset(cci);
>> -       if (ret < 0)
>> -               goto error;
>> -
>> -       ret = cci_init(cci);
>> -       if (ret < 0)
>> -               goto error;
>> -
>>          for (i = 0; i < cci->data->num_masters; i++) {
>>                  if (!cci->master[i].cci)
>>                          continue;
>> @@ -649,6 +641,14 @@ static int cci_probe(struct platform_device *pdev)
>>                  }
>>          }
>>
>> +       ret = cci_reset(cci);
>> +       if (ret < 0)
>> +               goto error_i2c;
>> +
>> +       ret = cci_init(cci);
>> +       if (ret < 0)
>> +               goto error_i2c;
>> +
>>          pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
>>          pm_runtime_use_autosuspend(dev);
>>          pm_runtime_set_active(dev);
>> @@ -663,7 +663,6 @@ static int cci_probe(struct platform_device *pdev)
>>                          of_node_put(cci->master[i].adap.dev.of_node);
>>                  }
>>          }
>> -error:
>>          disable_irq(cci->irq);
>>   disable_clocks:
>>          cci_disable_clocks(cci);
>> --
>> 2.33.0
>>
Loic Poulain Feb. 4, 2022, 11:18 a.m. UTC | #3
On Thu, 3 Feb 2022 at 19:45, Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> Hi Loic,
>
> On 2/3/22 7:29 PM, Loic Poulain wrote:
> > Hi Vladimir,
> >
> > On Thu, 3 Feb 2022 at 17:47, Vladimir Zapolskiy
> > <vladimir.zapolskiy@linaro.org> wrote:
> >>
> >> The change is wanted to postpone initialization of busses on CCI controller
> >> by cci_init() and cci_reset() till adapters are registered, the later is
> >> needed for adding I2C bus devices and get correspondent vbus regulators.
> >
> > This is odd, I don't think it's a good idea to register an adapter
> > which is not yet initialized. Could you elaborate on why you need to
> > do this, if you can't access the controller without this regulator
> > enabled, maybe it is more than vbus supply, and, in that case, it
> > should be enabled from your probe function.
>
> thank you for review, the controller can be accessed without a vbus regulator,
> but I2C devices connected to the master controller can not.
>
> The registration of a master controller device done by i2c_add_adapter()
> should be safe to defer IMO, because there shall be no communication on
> the bus at this point, there are no slaves before probe completion, thus
> cci_init()/cci_reset() can be safely called afterwards.
>
> The rationale of the change is to merge two loops over busses, see change 6/9,
> keeping two loops extremely complicates the proper resource management.

OK, I see, I'm sure it works, but I still think that registering the
adapter should be the last step, without making assumptions on when
the i2c core is going to use it. Maybe the point here is the initial
bad design/implementation of cci_reset and cci_init.

cci_reset() is a global reset and then should not depend on subnode
initialization in order to be executed early in the probe.You can e.g
add an 'irq_complete' completion to the cci struct. The CCI_IRQ_MASK_0
bit should probably be added here as well.

cci_init() should be subnode/master specific (cci_init_master) so that
you call it in the registering loop, updating master specific timings
and irq-mask bits.

Thoughs?

Regards,
Loic
Bjorn Andersson Feb. 4, 2022, 6:31 p.m. UTC | #4
On Thu 03 Feb 08:47 PST 2022, Vladimir Zapolskiy wrote:

> The change is wanted to postpone initialization of busses on CCI controller
> by cci_init() and cci_reset() till adapters are registered, the later is
> needed for adding I2C bus devices and get correspondent vbus regulators.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index cf54f1cb4c57..eebf9603d3d1 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -630,14 +630,6 @@ static int cci_probe(struct platform_device *pdev)
>  	val = readl(cci->base + CCI_HW_VERSION);
>  	dev_dbg(dev, "CCI HW version = 0x%08x", val);
>  
> -	ret = cci_reset(cci);
> -	if (ret < 0)
> -		goto error;
> -
> -	ret = cci_init(cci);
> -	if (ret < 0)
> -		goto error;
> -
>  	for (i = 0; i < cci->data->num_masters; i++) {
>  		if (!cci->master[i].cci)
>  			continue;
> @@ -649,6 +641,14 @@ static int cci_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	ret = cci_reset(cci);

i2c_add_adapter() will register and probe child devices, which might
want to access the bus in their probe functions. Don't you break that by
initializing the master after the children?

Regards,
Bjorn

> +	if (ret < 0)
> +		goto error_i2c;
> +
> +	ret = cci_init(cci);
> +	if (ret < 0)
> +		goto error_i2c;
> +
>  	pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
>  	pm_runtime_use_autosuspend(dev);
>  	pm_runtime_set_active(dev);
> @@ -663,7 +663,6 @@ static int cci_probe(struct platform_device *pdev)
>  			of_node_put(cci->master[i].adap.dev.of_node);
>  		}
>  	}
> -error:
>  	disable_irq(cci->irq);
>  disable_clocks:
>  	cci_disable_clocks(cci);
> -- 
> 2.33.0
>
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index cf54f1cb4c57..eebf9603d3d1 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -630,14 +630,6 @@  static int cci_probe(struct platform_device *pdev)
 	val = readl(cci->base + CCI_HW_VERSION);
 	dev_dbg(dev, "CCI HW version = 0x%08x", val);
 
-	ret = cci_reset(cci);
-	if (ret < 0)
-		goto error;
-
-	ret = cci_init(cci);
-	if (ret < 0)
-		goto error;
-
 	for (i = 0; i < cci->data->num_masters; i++) {
 		if (!cci->master[i].cci)
 			continue;
@@ -649,6 +641,14 @@  static int cci_probe(struct platform_device *pdev)
 		}
 	}
 
+	ret = cci_reset(cci);
+	if (ret < 0)
+		goto error_i2c;
+
+	ret = cci_init(cci);
+	if (ret < 0)
+		goto error_i2c;
+
 	pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_set_active(dev);
@@ -663,7 +663,6 @@  static int cci_probe(struct platform_device *pdev)
 			of_node_put(cci->master[i].adap.dev.of_node);
 		}
 	}
-error:
 	disable_irq(cci->irq);
 disable_clocks:
 	cci_disable_clocks(cci);