| Message ID | 20220203164629.1711958-3-vladimir.zapolskiy@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: qcom-cci: fixes and updates | expand |
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>
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 >
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.
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
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.
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
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.
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
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.
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.
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?
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.
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.
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?
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.
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 --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 {
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(+)