diff mbox series

[2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

Message ID 20220203164629.1711958-3-vladimir.zapolskiy@linaro.org
State New
Headers show
Series i2c: qcom-cci: fixes and updates | expand

Commit Message

Vladimir Zapolskiy Feb. 3, 2022, 4:46 p.m. UTC
Quite regularly I2C bus lines on QCOM CCI controller require an external
pull-up to a regulator powered line, to be able to define all such
cases an additional vbus-supply property of a bus subnode is wanted.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Robert Foss Feb. 4, 2022, 11:06 a.m. UTC | #1
On Thu, 3 Feb 2022 at 17:46, Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> Quite regularly I2C bus lines on QCOM CCI controller require an external
> pull-up to a regulator powered line, to be able to define all such
> cases an additional vbus-supply property of a bus subnode is wanted.
>
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> index 924ad8c03464..9f5b321748f1 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> @@ -60,6 +60,11 @@ PROPERTIES:
>         Definition: Desired I2C bus clock frequency in Hz, defaults to 100
>                     kHz if omitted.
>
> +- vbus-supply:
> +       Usage: optional
> +       Value type: phandle
> +       Definition: Regulator that provides power to SCL/SDA lines
> +
>  Example:
>
>         cci@a0c000 {
> --
> 2.33.0
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>
Bjorn Andersson Feb. 4, 2022, 6:05 p.m. UTC | #2
On Thu 03 Feb 08:46 PST 2022, Vladimir Zapolskiy wrote:

> Quite regularly I2C bus lines on QCOM CCI controller require an external
> pull-up to a regulator powered line, to be able to define all such
> cases an additional vbus-supply property of a bus subnode is wanted.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> index 924ad8c03464..9f5b321748f1 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> @@ -60,6 +60,11 @@ PROPERTIES:
>  	Definition: Desired I2C bus clock frequency in Hz, defaults to 100
>  		    kHz if omitted.
>  
> +- vbus-supply:

I don't think "vbus" is an appropriate name for his. Perhaps "vddio" or
something like that would be better.

But there's a bigger question here, this is not a supply for the
i2c master, it's simply a supply for pulling up the bus. So it's not
entirely correct to specify it as a supply for the CCI node (which is
also the reason why the name isn't obvious).

Typically we don't don't mention the bus-supply because it happens to be
pulled up either by io-supply for the block, or by some always-on
regulator in the system.

Looping in Linus and Mark in hope they have seen this need elsewhere.

Regards,
Bjorn

> +	Usage: optional
> +	Value type: phandle
> +	Definition: Regulator that provides power to SCL/SDA lines
> +
>  Example:
>  
>  	cci@a0c000 {
> -- 
> 2.33.0
>
Mark Brown Feb. 4, 2022, 6:42 p.m. UTC | #3
On Fri, Feb 04, 2022 at 10:05:47AM -0800, Bjorn Andersson wrote:
> On Thu 03 Feb 08:46 PST 2022, Vladimir Zapolskiy wrote:

> > +- vbus-supply:

> I don't think "vbus" is an appropriate name for his. Perhaps "vddio" or
> something like that would be better.

> But there's a bigger question here, this is not a supply for the
> i2c master, it's simply a supply for pulling up the bus. So it's not
> entirely correct to specify it as a supply for the CCI node (which is
> also the reason why the name isn't obvious).

Does the device (controller?) not have a supply that the I2C bus is
referenced to?  If so that supply should be named.

> Typically we don't don't mention the bus-supply because it happens to be
> pulled up either by io-supply for the block, or by some always-on
> regulator in the system.

If the bus is being pulled up to some supply other than the supply that
the bus is referenced to that doesn't sound like the greatest electrical
engineering ever...  without any context it's hard to comment about this
particular system.
Bjorn Andersson Feb. 4, 2022, 7:02 p.m. UTC | #4
On Fri 04 Feb 10:42 PST 2022, Mark Brown wrote:

> On Fri, Feb 04, 2022 at 10:05:47AM -0800, Bjorn Andersson wrote:
> > On Thu 03 Feb 08:46 PST 2022, Vladimir Zapolskiy wrote:
> 
> > > +- vbus-supply:
> 
> > I don't think "vbus" is an appropriate name for his. Perhaps "vddio" or
> > something like that would be better.
> 
> > But there's a bigger question here, this is not a supply for the
> > i2c master, it's simply a supply for pulling up the bus. So it's not
> > entirely correct to specify it as a supply for the CCI node (which is
> > also the reason why the name isn't obvious).
> 
> Does the device (controller?) not have a supply that the I2C bus is
> referenced to?  If so that supply should be named.
> 

No, for some reason the regulator in question is not connected to either
the master or the client devices, it's only used for pull up of a few
of the i2c busses.

> > Typically we don't don't mention the bus-supply because it happens to be
> > pulled up either by io-supply for the block, or by some always-on
> > regulator in the system.
> 
> If the bus is being pulled up to some supply other than the supply that
> the bus is referenced to that doesn't sound like the greatest electrical
> engineering ever...  without any context it's hard to comment about this
> particular system.

That's what the schematics says...

Regards,
Bjorn
Mark Brown Feb. 4, 2022, 7:32 p.m. UTC | #5
On Fri, Feb 04, 2022 at 11:02:04AM -0800, Bjorn Andersson wrote:
> On Fri 04 Feb 10:42 PST 2022, Mark Brown wrote:
> > On Fri, Feb 04, 2022 at 10:05:47AM -0800, Bjorn Andersson wrote:

> > > Typically we don't don't mention the bus-supply because it happens to be
> > > pulled up either by io-supply for the block, or by some always-on
> > > regulator in the system.

> > If the bus is being pulled up to some supply other than the supply that
> > the bus is referenced to that doesn't sound like the greatest electrical
> > engineering ever...  without any context it's hard to comment about this
> > particular system.

> That's what the schematics says...

Oh, good.  I forsee no problems here.  Probably this is something that
should be in the I2C core if it's going to be dynamically managed,
though just setting the supply as always on is probably more expedient.
Vladimir Zapolskiy Feb. 7, 2022, 2:08 p.m. UTC | #6
Hi Bjorn, Mark,

On 2/4/22 9:32 PM, Mark Brown wrote:
> On Fri, Feb 04, 2022 at 11:02:04AM -0800, Bjorn Andersson wrote:
>> On Fri 04 Feb 10:42 PST 2022, Mark Brown wrote:
>>> On Fri, Feb 04, 2022 at 10:05:47AM -0800, Bjorn Andersson wrote:
> 
>>>> Typically we don't don't mention the bus-supply because it happens to be
>>>> pulled up either by io-supply for the block, or by some always-on
>>>> regulator in the system.
> 
>>> If the bus is being pulled up to some supply other than the supply that
>>> the bus is referenced to that doesn't sound like the greatest electrical
>>> engineering ever...  without any context it's hard to comment about this
>>> particular system.
> 
>> That's what the schematics says...
> 
> Oh, good.  I forsee no problems here.  Probably this is something that
> should be in the I2C core if it's going to be dynamically managed,
> though just setting the supply as always on is probably more expedient.
> 

vbus-supply property has been added recently to another I2C master controller,
see commit c021087c43c8 ("dt-binding: i2c: mt65xx: add vbus-supply property").
It serves right the same purpose, and its handling is going to be done in i2c
core, however since the latter is not yet completed, I would propose to add
the property to i2c-bus subnodes of QCOM CCI and its support in the driver,
later on both the property and its generic support would be better to see in
i2c core.

--
Best wishes,
Vladimir
Mark Brown Feb. 7, 2022, 2:39 p.m. UTC | #7
On Mon, Feb 07, 2022 at 04:08:01PM +0200, Vladimir Zapolskiy wrote:
> On 2/4/22 9:32 PM, Mark Brown wrote:

> > Oh, good.  I forsee no problems here.  Probably this is something that
> > should be in the I2C core if it's going to be dynamically managed,
> > though just setting the supply as always on is probably more expedient.

> vbus-supply property has been added recently to another I2C master controller,
> see commit c021087c43c8 ("dt-binding: i2c: mt65xx: add vbus-supply property").

Note that some devices do have supplies that I/O is referenced against
and it's not clear that this isn't what's goin on here.

> It serves right the same purpose, and its handling is going to be done in i2c
> core, however since the latter is not yet completed, I would propose to add
> the property to i2c-bus subnodes of QCOM CCI and its support in the driver,
> later on both the property and its generic support would be better to see in
> i2c core.

The bindings are ABI, it doesn't seem like a good idea to add new ABI as
a temporary bodge.
Vladimir Zapolskiy Feb. 7, 2022, 6:31 p.m. UTC | #8
On 2/7/22 4:39 PM, Mark Brown wrote:
> On Mon, Feb 07, 2022 at 04:08:01PM +0200, Vladimir Zapolskiy wrote:
>> On 2/4/22 9:32 PM, Mark Brown wrote:
> 
>>> Oh, good.  I forsee no problems here.  Probably this is something that
>>> should be in the I2C core if it's going to be dynamically managed,
>>> though just setting the supply as always on is probably more expedient.
> 
>> vbus-supply property has been added recently to another I2C master controller,
>> see commit c021087c43c8 ("dt-binding: i2c: mt65xx: add vbus-supply property").
> 
> Note that some devices do have supplies that I/O is referenced against
> and it's not clear that this isn't what's goin on here.
>
>> It serves right the same purpose, and its handling is going to be done in i2c
>> core, however since the latter is not yet completed, I would propose to add
>> the property to i2c-bus subnodes of QCOM CCI and its support in the driver,
>> later on both the property and its generic support would be better to see in
>> i2c core.
> 
> The bindings are ABI, it doesn't seem like a good idea to add new ABI as
> a temporary bodge.

The bindings are supposed to describe hardware, thus it's natural to extend
them, I believe there is a trilemma in this particular case:
1) add optional vbus-supply property to all I2C master controllers or I2C
    busses in case of multiple I2C busses managed by a single controller,
2) add optional vbus-supply property to all I2C slave devices,
3) ignore peculiarities of particular (multiple in fact) PCB designs and
    a necessity of adding a regulator finely described as a pull-up for I2C
    bus lines.

My assumption is that a decision should be generic for all similar cases,
Wolfram, could you share your point of view on the subject?

--
Best wishes,
Vladimir
Mark Brown Feb. 8, 2022, 12:55 p.m. UTC | #9
On Mon, Feb 07, 2022 at 08:31:30PM +0200, Vladimir Zapolskiy wrote:
> On 2/7/22 4:39 PM, Mark Brown wrote:

> > The bindings are ABI, it doesn't seem like a good idea to add new ABI as
> > a temporary bodge.

> The bindings are supposed to describe hardware, thus it's natural to extend
> them, I believe there is a trilemma in this particular case:
> 1) add optional vbus-supply property to all I2C master controllers or I2C
>    busses in case of multiple I2C busses managed by a single controller,
> 2) add optional vbus-supply property to all I2C slave devices,

If you add a named supply to all I2C controllers or devices then if any
of them have an actual vbus supply there will be a namespace collision.

> 3) ignore peculiarities of particular (multiple in fact) PCB designs and
>    a necessity of adding a regulator finely described as a pull-up for I2C
>    bus lines.

There's also the option of representing this as a separate thing on or
part of the bus.
Dmitry Baryshkov Feb. 10, 2022, 3:33 p.m. UTC | #10
On Tue, 8 Feb 2022 at 16:16, Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Feb 07, 2022 at 08:31:30PM +0200, Vladimir Zapolskiy wrote:
> > On 2/7/22 4:39 PM, Mark Brown wrote:
>
> > > The bindings are ABI, it doesn't seem like a good idea to add new ABI as
> > > a temporary bodge.

It's not a temporary bodge. The i2c-core piece was reverted, but not
the mediatek driver code/bindings.
Vladimir has provided a replacement for the i2c-core code handling the
vbus-regulator. When thee code will be back, the code from i2c-cci can
be removed. The bindings will be the same.

>
> > The bindings are supposed to describe hardware, thus it's natural to extend
> > them, I believe there is a trilemma in this particular case:
> > 1) add optional vbus-supply property to all I2C master controllers or I2C
> >    busses in case of multiple I2C busses managed by a single controller,
> > 2) add optional vbus-supply property to all I2C slave devices,
>
> If you add a named supply to all I2C controllers or devices then if any
> of them have an actual vbus supply there will be a namespace collision.
>
> > 3) ignore peculiarities of particular (multiple in fact) PCB designs and
> >    a necessity of adding a regulator finely described as a pull-up for I2C
> >    bus lines.
>
> There's also the option of representing this as a separate thing on or
> part of the bus.

4) (which you have implemented in your patch). Add support for  the
vbus-supplies property for the I2C CCI controllers.

This is the option I'd vote for.
Mark Brown Feb. 10, 2022, 3:44 p.m. UTC | #11
On Thu, Feb 10, 2022 at 06:33:09PM +0300, Dmitry Baryshkov wrote:
> On Tue, 8 Feb 2022 at 16:16, Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Feb 07, 2022 at 08:31:30PM +0200, Vladimir Zapolskiy wrote:
> > > On 2/7/22 4:39 PM, Mark Brown wrote:

> > > > The bindings are ABI, it doesn't seem like a good idea to add new ABI as
> > > > a temporary bodge.

> It's not a temporary bodge. The i2c-core piece was reverted, but not
> the mediatek driver code/bindings.
> Vladimir has provided a replacement for the i2c-core code handling the
> vbus-regulator. When thee code will be back, the code from i2c-cci can
> be removed. The bindings will be the same.

I would hope it's a temporary thing given the namespace collision
issues...

> > There's also the option of representing this as a separate thing on or
> > part of the bus.

> 4) (which you have implemented in your patch). Add support for  the
> vbus-supplies property for the I2C CCI controllers.

> This is the option I'd vote for.

Do these controllers actually have a supply called vbus?
Dmitry Baryshkov Feb. 10, 2022, 5:32 p.m. UTC | #12
On Thu, 10 Feb 2022 at 18:45, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Feb 10, 2022 at 06:33:09PM +0300, Dmitry Baryshkov wrote:
> > On Tue, 8 Feb 2022 at 16:16, Mark Brown <broonie@kernel.org> wrote:
> > > On Mon, Feb 07, 2022 at 08:31:30PM +0200, Vladimir Zapolskiy wrote:
> > > > On 2/7/22 4:39 PM, Mark Brown wrote:
>
> > > > > The bindings are ABI, it doesn't seem like a good idea to add new ABI as
> > > > > a temporary bodge.
>
> > It's not a temporary bodge. The i2c-core piece was reverted, but not
> > the mediatek driver code/bindings.
> > Vladimir has provided a replacement for the i2c-core code handling the
> > vbus-regulator. When thee code will be back, the code from i2c-cci can
> > be removed. The bindings will be the same.
>
> I would hope it's a temporary thing given the namespace collision
> issues...

Which collision? CCI doesn't have a separate vbus power input (and
probably never will).

>
> > > There's also the option of representing this as a separate thing on or
> > > part of the bus.
>
> > 4) (which you have implemented in your patch). Add support for  the
> > vbus-supplies property for the I2C CCI controllers.
>
> > This is the option I'd vote for.
>
> Do these controllers actually have a supply called vbus?

No. It's a separate entity, a regulator-controller pull-up for the bus.
So far we'd like to hear better suggestions. Using regulator-always-on
doesn't sound like a good idea, it will increase unnecessary power
drain.
Mark Brown Feb. 10, 2022, 5:36 p.m. UTC | #13
On Thu, Feb 10, 2022 at 08:32:09PM +0300, Dmitry Baryshkov wrote:
> On Thu, 10 Feb 2022 at 18:45, Mark Brown <broonie@kernel.org> wrote:

> > I would hope it's a temporary thing given the namespace collision
> > issues...

> Which collision? CCI doesn't have a separate vbus power input (and
> probably never will).

That "probably" there is doing some work, and if you're doing something
at the I2C core level (as it seems should be done) it needs to cope with
all possible controllers and devices.

> > Do these controllers actually have a supply called vbus?

> No. It's a separate entity, a regulator-controller pull-up for the bus.
> So far we'd like to hear better suggestions. Using regulator-always-on
> doesn't sound like a good idea, it will increase unnecessary power
> drain.

Please see my suggestions elsewhere in the thread.
Dmitry Baryshkov Feb. 10, 2022, 6:21 p.m. UTC | #14
On Thu, 10 Feb 2022 at 20:36, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Feb 10, 2022 at 08:32:09PM +0300, Dmitry Baryshkov wrote:
> > On Thu, 10 Feb 2022 at 18:45, Mark Brown <broonie@kernel.org> wrote:
>
> > > I would hope it's a temporary thing given the namespace collision
> > > issues...
>
> > Which collision? CCI doesn't have a separate vbus power input (and
> > probably never will).
>
> That "probably" there is doing some work, and if you're doing something
> at the I2C core level (as it seems should be done) it needs to cope with
> all possible controllers and devices.
>
> > > Do these controllers actually have a supply called vbus?
>
> > No. It's a separate entity, a regulator-controller pull-up for the bus.
> > So far we'd like to hear better suggestions. Using regulator-always-on
> > doesn't sound like a good idea, it will increase unnecessary power
> > drain.
>
> Please see my suggestions elsewhere in the thread.

Please excuse me. I missed the e-mail suggesting to move support for
that into the core level.
I'd second a request to handle the adapter->bus_regulator in the core code.
Would you be ok with the 'external-sda-scl-supply' property? Would you
demand that it's completely handled by the core layer (including DT
parsing) or should we let a driver parse the DT property?
Mark Brown Feb. 10, 2022, 6:26 p.m. UTC | #15
On Thu, Feb 10, 2022 at 09:21:42PM +0300, Dmitry Baryshkov wrote:

> Please excuse me. I missed the e-mail suggesting to move support for
> that into the core level.

No problem.

> I'd second a request to handle the adapter->bus_regulator in the core code.
> Would you be ok with the 'external-sda-scl-supply' property? Would you
> demand that it's completely handled by the core layer (including DT
> parsing) or should we let a driver parse the DT property?

I'm not super worried about how it's implemented so long as the binding
is good for the long term - if doing it in a driver helps get things
done that's fixable later on without breaking ABI.
Dmitry Baryshkov Feb. 10, 2022, 7:02 p.m. UTC | #16
On Thu, 10 Feb 2022 at 21:26, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Feb 10, 2022 at 09:21:42PM +0300, Dmitry Baryshkov wrote:
>
> > I'd second a request to handle the adapter->bus_regulator in the core code.
> > Would you be ok with the 'external-sda-scl-supply' property? Would you
> > demand that it's completely handled by the core layer (including DT
> > parsing) or should we let a driver parse the DT property?
>
> I'm not super worried about how it's implemented so long as the binding
> is good for the long term - if doing it in a driver helps get things
> done that's fixable later on without breaking ABI.

So, 'external-sda-scl-supply'?
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
index 924ad8c03464..9f5b321748f1 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
@@ -60,6 +60,11 @@  PROPERTIES:
 	Definition: Desired I2C bus clock frequency in Hz, defaults to 100
 		    kHz if omitted.
 
+- vbus-supply:
+	Usage: optional
+	Value type: phandle
+	Definition: Regulator that provides power to SCL/SDA lines
+
 Example:
 
 	cci@a0c000 {