| Message ID | ZhPMHdt6r/4D99Zg@duo.ucw.cz (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv3,1/2] dt-bindings: usb: typec: anx7688: start a binding document | expand |
On 08/04/2024 12:51, Pavel Machek wrote: > Add binding for anx7688 usb type-c bridge. I don't have a datasheet, > but I did best I could. > > Signed-off-by: Pavel Machek <pavel@ucw.cz> ... > + cabledet-gpios: > + maxItems: 1 > + description: GPIO controlling CABLE_DET (C3) pin. > + > + avdd10-supply: > + description: 1.0V power supply going to AVDD10 (A4, ...) pins > + > + dvdd10-supply: > + description: 1.0V power supply going to DVDD10 (D6, ...) pins > + > + avdd18-supply: > + description: 1.8V power supply going to AVDD18 (E3, ...) pins > + > + dvdd18-supply: > + description: 1.8V power supply going to DVDD18 (G4, ...) pins > + > + avdd33-supply: > + description: 3.3V power supply going to AVDD33 (C4, ...) pins > + > + i2c-supply: true > + vconn-supply: true There are no such supplies like i2c and vconn on the schematics. I think this represents some other part of component which was added here only for convenience. Best regards, Krzysztof
Hi! > > Add binding for anx7688 usb type-c bridge. I don't have a datasheet, > > but I did best I could. > > > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > ... > > > + cabledet-gpios: > > + maxItems: 1 > > + description: GPIO controlling CABLE_DET (C3) pin. > > + > > + avdd10-supply: > > + description: 1.0V power supply going to AVDD10 (A4, ...) pins > > + > > + dvdd10-supply: > > + description: 1.0V power supply going to DVDD10 (D6, ...) pins > > + > > + avdd18-supply: > > + description: 1.8V power supply going to AVDD18 (E3, ...) pins > > + > > + dvdd18-supply: > > + description: 1.8V power supply going to DVDD18 (G4, ...) pins > > + > > + avdd33-supply: > > + description: 3.3V power supply going to AVDD33 (C4, ...) pins > > + > > + i2c-supply: true > > + vconn-supply: true > > There are no such supplies like i2c and vconn on the schematics. > > I think this represents some other part of component which was added > here only for convenience. Can you give me pointer to documentation you are looking at? Best regards, Pavel
On 08/04/2024 13:21, Pavel Machek wrote: > Hi! > >>> Add binding for anx7688 usb type-c bridge. I don't have a datasheet, >>> but I did best I could. >>> >>> Signed-off-by: Pavel Machek <pavel@ucw.cz> >> >> ... >> >>> + cabledet-gpios: >>> + maxItems: 1 >>> + description: GPIO controlling CABLE_DET (C3) pin. >>> + >>> + avdd10-supply: >>> + description: 1.0V power supply going to AVDD10 (A4, ...) pins >>> + >>> + dvdd10-supply: >>> + description: 1.0V power supply going to DVDD10 (D6, ...) pins >>> + >>> + avdd18-supply: >>> + description: 1.8V power supply going to AVDD18 (E3, ...) pins >>> + >>> + dvdd18-supply: >>> + description: 1.8V power supply going to DVDD18 (G4, ...) pins >>> + >>> + avdd33-supply: >>> + description: 3.3V power supply going to AVDD33 (C4, ...) pins >>> + >>> + i2c-supply: true >>> + vconn-supply: true >> >> There are no such supplies like i2c and vconn on the schematics. >> >> I think this represents some other part of component which was added >> here only for convenience. > > Can you give me pointer to documentation you are looking at? The schematics you linked in the document at the beginning. Page 13. Do you see these pins there? I saw only VCONN1_EN, but that's not a supply. Best regards, Krzysztof
Hi Krzysztof, On Mon, Apr 08, 2024 at 01:17:32PM GMT, Krzysztof Kozlowski wrote: > On 08/04/2024 12:51, Pavel Machek wrote: > > Add binding for anx7688 usb type-c bridge. I don't have a datasheet, > > but I did best I could. > > > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > ... > > > + cabledet-gpios: > > + maxItems: 1 > > + description: GPIO controlling CABLE_DET (C3) pin. > > + > > + avdd10-supply: > > + description: 1.0V power supply going to AVDD10 (A4, ...) pins > > + > > + dvdd10-supply: > > + description: 1.0V power supply going to DVDD10 (D6, ...) pins > > + > > + avdd18-supply: > > + description: 1.8V power supply going to AVDD18 (E3, ...) pins > > + > > + dvdd18-supply: > > + description: 1.8V power supply going to DVDD18 (G4, ...) pins > > + > > + avdd33-supply: > > + description: 3.3V power supply going to AVDD33 (C4, ...) pins > > + > > + i2c-supply: true > > + vconn-supply: true > > There are no such supplies like i2c and vconn on the schematics. Which schematics? ANX7688 has VCONN1/2_EN GPIOs that control a switching of VCONN power supply to resective CCx pins. That's just a switch signal. Power for VCONN needs to come from somewhere and the driver needs to enable the regulator at the appropriate time only. On Pinephone it can't be an always on power supply and needs to be enabled only when used due to HW design of the circuit. (default without ANX driver initialized would be to shove 5V to both CC pins, which breaks Type-C spec) I2C supply is needed for I2C bus to work, apparently. There's nothing that says that I2C pull-ups supply has to come from supplies powering the chip. I2C I/O is open drain and the device needs to enable a bus supply in order to communicate. You can say that bus master should be managing the bus supply, but you'd still have a problem that each device may be behind a voltage translator, and logically, bus master driver should care only about its side of the bus then. Both side of level shifter need the pull-up power enabled. You can also make an argument that bus supply can be always on, but that causes several other issues on Pinephone due to shared nature of most resources like these. There are other devices on shared power rails, that need to be turned off during sleep, etc. Kind regards, o. > I think this represents some other part of component which was added > here only for convenience. > > > > Best regards, > Krzysztof >
On Mon, Apr 08, 2024 at 01:24:03PM GMT, Krzysztof Kozlowski wrote: > On 08/04/2024 13:21, Pavel Machek wrote: > > Hi! > > > >>> Add binding for anx7688 usb type-c bridge. I don't have a datasheet, > >>> but I did best I could. > >>> > >>> Signed-off-by: Pavel Machek <pavel@ucw.cz> > >> > >> ... > >> > >>> + cabledet-gpios: > >>> + maxItems: 1 > >>> + description: GPIO controlling CABLE_DET (C3) pin. > >>> + > >>> + avdd10-supply: > >>> + description: 1.0V power supply going to AVDD10 (A4, ...) pins > >>> + > >>> + dvdd10-supply: > >>> + description: 1.0V power supply going to DVDD10 (D6, ...) pins > >>> + > >>> + avdd18-supply: > >>> + description: 1.8V power supply going to AVDD18 (E3, ...) pins > >>> + > >>> + dvdd18-supply: > >>> + description: 1.8V power supply going to DVDD18 (G4, ...) pins > >>> + > >>> + avdd33-supply: > >>> + description: 3.3V power supply going to AVDD33 (C4, ...) pins > >>> + > >>> + i2c-supply: true > >>> + vconn-supply: true > >> > >> There are no such supplies like i2c and vconn on the schematics. > >> > >> I think this represents some other part of component which was added > >> here only for convenience. > > > > Can you give me pointer to documentation you are looking at? > > The schematics you linked in the document at the beginning. Page 13. Do > you see these pins there? I saw only VCONN1_EN, but that's not a supply. The supply is U1308. regards, o. > Best regards, > Krzysztof >
On 08/04/2024 13:52, Ondřej Jirman wrote: > On Mon, Apr 08, 2024 at 01:24:03PM GMT, Krzysztof Kozlowski wrote: >> On 08/04/2024 13:21, Pavel Machek wrote: >>> Hi! >>> >>>>> Add binding for anx7688 usb type-c bridge. I don't have a datasheet, >>>>> but I did best I could. >>>>> >>>>> Signed-off-by: Pavel Machek <pavel@ucw.cz> >>>> >>>> ... >>>> >>>>> + cabledet-gpios: >>>>> + maxItems: 1 >>>>> + description: GPIO controlling CABLE_DET (C3) pin. >>>>> + >>>>> + avdd10-supply: >>>>> + description: 1.0V power supply going to AVDD10 (A4, ...) pins >>>>> + >>>>> + dvdd10-supply: >>>>> + description: 1.0V power supply going to DVDD10 (D6, ...) pins >>>>> + >>>>> + avdd18-supply: >>>>> + description: 1.8V power supply going to AVDD18 (E3, ...) pins >>>>> + >>>>> + dvdd18-supply: >>>>> + description: 1.8V power supply going to DVDD18 (G4, ...) pins >>>>> + >>>>> + avdd33-supply: >>>>> + description: 3.3V power supply going to AVDD33 (C4, ...) pins >>>>> + >>>>> + i2c-supply: true >>>>> + vconn-supply: true >>>> >>>> There are no such supplies like i2c and vconn on the schematics. >>>> >>>> I think this represents some other part of component which was added >>>> here only for convenience. >>> >>> Can you give me pointer to documentation you are looking at? >> >> The schematics you linked in the document at the beginning. Page 13. Do >> you see these pins there? I saw only VCONN1_EN, but that's not a supply. > > The supply is U1308. That's not a supply to anx7688. Best regards, Krzysztof
On 08/04/2024 13:51, Ondřej Jirman wrote: > Hi Krzysztof, > > On Mon, Apr 08, 2024 at 01:17:32PM GMT, Krzysztof Kozlowski wrote: >> On 08/04/2024 12:51, Pavel Machek wrote: >>> Add binding for anx7688 usb type-c bridge. I don't have a datasheet, >>> but I did best I could. >>> >>> Signed-off-by: Pavel Machek <pavel@ucw.cz> >> >> ... >> >>> + cabledet-gpios: >>> + maxItems: 1 >>> + description: GPIO controlling CABLE_DET (C3) pin. >>> + >>> + avdd10-supply: >>> + description: 1.0V power supply going to AVDD10 (A4, ...) pins >>> + >>> + dvdd10-supply: >>> + description: 1.0V power supply going to DVDD10 (D6, ...) pins >>> + >>> + avdd18-supply: >>> + description: 1.8V power supply going to AVDD18 (E3, ...) pins >>> + >>> + dvdd18-supply: >>> + description: 1.8V power supply going to DVDD18 (G4, ...) pins >>> + >>> + avdd33-supply: >>> + description: 3.3V power supply going to AVDD33 (C4, ...) pins >>> + >>> + i2c-supply: true >>> + vconn-supply: true >> >> There are no such supplies like i2c and vconn on the schematics. > > Which schematics? > > ANX7688 has VCONN1/2_EN GPIOs that control a switching of VCONN power supply > to resective CCx pins. That's just a switch signal. Power for VCONN needs > to come from somewhere and the driver needs to enable the regulator at > the appropriate time only. > > On Pinephone it can't be an always on power supply and needs to be enabled > only when used due to HW design of the circuit. (default without ANX driver > initialized would be to shove 5V to both CC pins, which breaks Type-C spec) > > I2C supply is needed for I2C bus to work, apparently. There's nothing > that says that I2C pull-ups supply has to come from supplies powering the > chip. I2C I/O is open drain and the device needs to enable a bus supply > in order to communicate. No, that's misunderstanding of DT. These are not supplies to anx7688. Bus supply is not related to anx7688. > > You can say that bus master should be managing the bus supply, but you'd still > have a problem that each device may be behind a voltage translator, and > logically, bus master driver should care only about its side of the bus then. > Both side of level shifter need the pull-up power enabled. Again, that's nothing related to anx7688. If you want to add it here, why not to every device everywhere? > > You can also make an argument that bus supply can be always on, but that > causes several other issues on Pinephone due to shared nature of most > resources like these. There are other devices on shared power rails, that > need to be turned off during sleep, etc. > No, do not add non-existing properties on this device as workaround for other issues. Please drop these two supplies *and all other which do not exist* on anx7688. Best regards, Krzysztof
On Mon, Apr 08, 2024 at 01:59:12PM GMT, Krzysztof Kozlowski wrote: > On 08/04/2024 13:52, Ondřej Jirman wrote: > > On Mon, Apr 08, 2024 at 01:24:03PM GMT, Krzysztof Kozlowski wrote: > >> On 08/04/2024 13:21, Pavel Machek wrote: > >>> Hi! > >>> > >>>>> Add binding for anx7688 usb type-c bridge. I don't have a datasheet, > >>>>> but I did best I could. > >>>>> > >>>>> Signed-off-by: Pavel Machek <pavel@ucw.cz> > >>>> > >>>> ... > >>>> > >>>>> + cabledet-gpios: > >>>>> + maxItems: 1 > >>>>> + description: GPIO controlling CABLE_DET (C3) pin. > >>>>> + > >>>>> + avdd10-supply: > >>>>> + description: 1.0V power supply going to AVDD10 (A4, ...) pins > >>>>> + > >>>>> + dvdd10-supply: > >>>>> + description: 1.0V power supply going to DVDD10 (D6, ...) pins > >>>>> + > >>>>> + avdd18-supply: > >>>>> + description: 1.8V power supply going to AVDD18 (E3, ...) pins > >>>>> + > >>>>> + dvdd18-supply: > >>>>> + description: 1.8V power supply going to DVDD18 (G4, ...) pins > >>>>> + > >>>>> + avdd33-supply: > >>>>> + description: 3.3V power supply going to AVDD33 (C4, ...) pins > >>>>> + > >>>>> + i2c-supply: true > >>>>> + vconn-supply: true > >>>> > >>>> There are no such supplies like i2c and vconn on the schematics. > >>>> > >>>> I think this represents some other part of component which was added > >>>> here only for convenience. > >>> > >>> Can you give me pointer to documentation you are looking at? > >> > >> The schematics you linked in the document at the beginning. Page 13. Do > >> you see these pins there? I saw only VCONN1_EN, but that's not a supply. > > > > The supply is U1308. > > That's not a supply to anx7688. Yeah, I understand where the confusion is. The driver is not for anx7688 chip really. The driver is named anx7688, but that's mostly a historical accident at this point. I guess there can be a driver for anx7688 chip that can directly use the chip's resources from the host by directly manipulating its registers and implementing type-c functionality via eg. Linux's TCPM or TCPCI stack, etc. (eg. like fusb302 driver, or various tcpci subdrivers). But in this case the chip is driven by an optional on-chip microcontroller's firmware and *this driver* is specifically for *the Type-C port on Pinephone* and serves as an integration driver for quite a bunch of things that need to work together on Pinephone for all of the Type-C port's features to operate reasonably well (and one of those is some communication with anx7688 firmware that we use, and enabling power to this chip and other things as appropriate, based on the communication from the firmware). It handles the specific needs of the Pinephone's Type-C implementation, all of its quirks (of which there are many over several HW revisions) that can't be handled by the particular implementation of on-chip microcontroller firmware directly and need host side interaction. In an ideal world, many of the things this driver handles would be handled by embedded microcontroller on the board (like it is with some RK3399 based Google devices), but Pinephone has no such thing and this glue needs to be implemented somewhere in the kernel. Kind regards, o. > Best regards, > Krzysztof >
On 08/04/2024 14:48, Ondřej Jirman wrote: > On Mon, Apr 08, 2024 at 01:59:12PM GMT, Krzysztof Kozlowski wrote: >> On 08/04/2024 13:52, Ondřej Jirman wrote: >>> On Mon, Apr 08, 2024 at 01:24:03PM GMT, Krzysztof Kozlowski wrote: >>>> On 08/04/2024 13:21, Pavel Machek wrote: >>>>> Hi! >>>>> >>>>>>> Add binding for anx7688 usb type-c bridge. I don't have a datasheet, >>>>>>> but I did best I could. >>>>>>> >>>>>>> Signed-off-by: Pavel Machek <pavel@ucw.cz> >>>>>> >>>>>> ... >>>>>> >>>>>>> + cabledet-gpios: >>>>>>> + maxItems: 1 >>>>>>> + description: GPIO controlling CABLE_DET (C3) pin. >>>>>>> + >>>>>>> + avdd10-supply: >>>>>>> + description: 1.0V power supply going to AVDD10 (A4, ...) pins >>>>>>> + >>>>>>> + dvdd10-supply: >>>>>>> + description: 1.0V power supply going to DVDD10 (D6, ...) pins >>>>>>> + >>>>>>> + avdd18-supply: >>>>>>> + description: 1.8V power supply going to AVDD18 (E3, ...) pins >>>>>>> + >>>>>>> + dvdd18-supply: >>>>>>> + description: 1.8V power supply going to DVDD18 (G4, ...) pins >>>>>>> + >>>>>>> + avdd33-supply: >>>>>>> + description: 3.3V power supply going to AVDD33 (C4, ...) pins >>>>>>> + >>>>>>> + i2c-supply: true >>>>>>> + vconn-supply: true >>>>>> >>>>>> There are no such supplies like i2c and vconn on the schematics. >>>>>> >>>>>> I think this represents some other part of component which was added >>>>>> here only for convenience. >>>>> >>>>> Can you give me pointer to documentation you are looking at? >>>> >>>> The schematics you linked in the document at the beginning. Page 13. Do >>>> you see these pins there? I saw only VCONN1_EN, but that's not a supply. >>> >>> The supply is U1308. >> >> That's not a supply to anx7688. > > Yeah, I understand where the confusion is. The driver is not for anx7688 chip > really. The driver is named anx7688, but that's mostly a historical accident at > this point. > > I guess there can be a driver for anx7688 chip that can directly use the chip's > resources from the host by directly manipulating its registers and implementing > type-c functionality via eg. Linux's TCPM or TCPCI stack, etc. (eg. like > fusb302 driver, or various tcpci subdrivers). > > But in this case the chip is driven by an optional on-chip microcontroller's > firmware and *this driver* is specifically for *the Type-C port on Pinephone* We do not talk here about the driver, but bindings, so hardware. > and serves as an integration driver for quite a bunch of things that need to > work together on Pinephone for all of the Type-C port's features to operate > reasonably well (and one of those is some communication with anx7688 firmware > that we use, and enabling power to this chip and other things as appropriate, > based on the communication from the firmware). That's still looking like putting board design into particular device binding. > > It handles the specific needs of the Pinephone's Type-C implementation, all of > its quirks (of which there are many over several HW revisions) that can't be > handled by the particular implementation of on-chip microcontroller firmware > directly and need host side interaction. > > In an ideal world, many of the things this driver handles would be handled by > embedded microcontroller on the board (like it is with some RK3399 based Google > devices), but Pinephone has no such thing and this glue needs to be implemented > somewhere in the kernel. You might need multiple schemas, because this is for anx7688, not for Pinephone type-c implementation. However I still do not see yet a limitation of DTS requiring stuffing some other properties into anx7688 or creating some other, virtual entity. Best regards, Krzysztof
On Mon, Apr 08, 2024 at 03:27:00PM GMT, Krzysztof Kozlowski wrote: > On 08/04/2024 14:48, Ondřej Jirman wrote: > > Yeah, I understand where the confusion is. The driver is not for anx7688 chip > > really. The driver is named anx7688, but that's mostly a historical accident at > > this point. > > > > I guess there can be a driver for anx7688 chip that can directly use the chip's > > resources from the host by directly manipulating its registers and implementing > > type-c functionality via eg. Linux's TCPM or TCPCI stack, etc. (eg. like > > fusb302 driver, or various tcpci subdrivers). > > > > But in this case the chip is driven by an optional on-chip microcontroller's > > firmware and *this driver* is specifically for *the Type-C port on Pinephone* > > We do not talk here about the driver, but bindings, so hardware. Got it. Bindings should be the same regardless of what driver would be used, whether this OCM based one, or some future one based on the above mentioned TCPCI in-kernel implementation. Hardware is the same in both cases. Just trying to imagine how to actually solve the issues... Basic thing with the I2C regulator thing is that needs to be enabled as long as anx7688 needs to communicate over I2C. Other user of this power rail is touchscreen controller for its normal power supply, and it needs to be able to disable it during system suspend. Now for things to not fail during suspend/resume based on PM callbacks invocation order, anx7688 driver needs to enable this regulator too, as long as it needs it. I can put bus-supply to I2C controller node, and read it from the ANX7688 driver I guess, by going up a DT node. Whether that's going to be acceptable, I don't know. VCONN regulator I don't know where else to put either. It doesn't seem to belong anywhere. It's not something directly connected to Type-C connector, so not part of connector bindings, and there's nothing else I can see, other than anx7688 device which needs it for core functionality. ANX7688 chip desing doesn't have integrated VCONN mosfet switches so it always needs external supply + switches that are controlled by the chip itself. There's no sensible design where someone would not want this and the driver needs to get this regulator reference from somewhere. The switches are sort of an extension of the chip. kind regards, o. > > and serves as an integration driver for quite a bunch of things that need to > > work together on Pinephone for all of the Type-C port's features to operate > > reasonably well (and one of those is some communication with anx7688 firmware > > that we use, and enabling power to this chip and other things as appropriate, > > based on the communication from the firmware). > > That's still looking like putting board design into particular device > binding. > > > > > It handles the specific needs of the Pinephone's Type-C implementation, all of > > its quirks (of which there are many over several HW revisions) that can't be > > handled by the particular implementation of on-chip microcontroller firmware > > directly and need host side interaction. > > > > In an ideal world, many of the things this driver handles would be handled by > > embedded microcontroller on the board (like it is with some RK3399 based Google > > devices), but Pinephone has no such thing and this glue needs to be implemented > > somewhere in the kernel. > > You might need multiple schemas, because this is for anx7688, not for > Pinephone type-c implementation. > > However I still do not see yet a limitation of DTS requiring stuffing > some other properties into anx7688 or creating some other, virtual entity. > > > Best regards, > Krzysztof >
On 08/04/2024 17:17, Ondřej Jirman wrote: > On Mon, Apr 08, 2024 at 03:27:00PM GMT, Krzysztof Kozlowski wrote: >> On 08/04/2024 14:48, Ondřej Jirman wrote: >>> Yeah, I understand where the confusion is. The driver is not for anx7688 chip >>> really. The driver is named anx7688, but that's mostly a historical accident at >>> this point. >>> >>> I guess there can be a driver for anx7688 chip that can directly use the chip's >>> resources from the host by directly manipulating its registers and implementing >>> type-c functionality via eg. Linux's TCPM or TCPCI stack, etc. (eg. like >>> fusb302 driver, or various tcpci subdrivers). >>> >>> But in this case the chip is driven by an optional on-chip microcontroller's >>> firmware and *this driver* is specifically for *the Type-C port on Pinephone* >> >> We do not talk here about the driver, but bindings, so hardware. > > Got it. Bindings should be the same regardless of what driver would be used, > whether this OCM based one, or some future one based on the above mentioned > TCPCI in-kernel implementation. Hardware is the same in both cases. > > Just trying to imagine how to actually solve the issues... > > Basic thing with the I2C regulator thing is that needs to be enabled as long > as anx7688 needs to communicate over I2C. Other user of this power rail is > touchscreen controller for its normal power supply, and it needs to be able > to disable it during system suspend. This does not look like anything specific to this particular device... but even without this, please think how you want it to be solved. Same supply which has to be on always, because your anx7688 can talk over I2C, and in the same time sometimes off, so touchscreen can be shutdown. If this is regulator for the I2C bus, then I think we already had this discussion some time ago. I think it is not a property of the I2C device, but the controller. > > Now for things to not fail during suspend/resume based on PM callbacks > invocation order, anx7688 driver needs to enable this regulator too, as long > as it needs it. No, the I2C bus driver needs to manage it. Not one individual I2C device. Again, why anx7688 is specific? If you next phone has anx8867, using different driver, you also add there i2c-supply? And if it is nxp,ptn5100 as well? > > I can put bus-supply to I2C controller node, and read it from the ANX7688 driver > I guess, by going up a DT node. Whether that's going to be acceptable, I don't > know. > > > VCONN regulator I don't know where else to put either. It doesn't seem to belong > anywhere. It's not something directly connected to Type-C connector, so > not part of connector bindings, and there's nothing else I can see, other > than anx7688 device which needs it for core functionality. That sounds like a GPIO, not regulator. anx7688 has GPIOs, right? On Pinephone they go to regulator, but on FooPhone also using anx7688 they go somewhere else, so why this anx7688 assumes this is a regulator? You need to properly represent the hardware, not bend it to your one use-case for one hardware. > > ANX7688 chip desing doesn't have integrated VCONN mosfet switches so it always > needs external supply + switches that are controlled by the chip itself. There's > no sensible design where someone would not want this and the driver needs > to get this regulator reference from somewhere. The switches are sort of an > extension of the chip. Best regards, Krzysztof
On Mon, Apr 08, 2024 at 10:12:30PM GMT, Krzysztof Kozlowski wrote: > On 08/04/2024 17:17, Ondřej Jirman wrote: > > > > Now for things to not fail during suspend/resume based on PM callbacks > > invocation order, anx7688 driver needs to enable this regulator too, as long > > as it needs it. > > No, the I2C bus driver needs to manage it. Not one individual I2C > device. Again, why anx7688 is specific? If you next phone has anx8867, > using different driver, you also add there i2c-supply? And if it is > nxp,ptn5100 as well? Yes, that could work, if I2C core would manage this. > > > > I can put bus-supply to I2C controller node, and read it from the ANX7688 driver > > I guess, by going up a DT node. Whether that's going to be acceptable, I don't > > know. > > > > > > VCONN regulator I don't know where else to put either. It doesn't seem to belong > > anywhere. It's not something directly connected to Type-C connector, so > > not part of connector bindings, and there's nothing else I can see, other > > than anx7688 device which needs it for core functionality. > > That sounds like a GPIO, not regulator. anx7688 has GPIOs, right? On > Pinephone they go to regulator, but on FooPhone also using anx7688 they > go somewhere else, so why this anx7688 assumes this is a regulator? CC1/CC2_VCONN control pins are "GPIO" of anx7688, sort of. They have fixed purpose of switching external 5V regulator output to one of the CC pins on type-c port. I don't care what other purpose with some other firmware someone puts to those pins. It's irrelevant to the use case of anx7688 as a type-c controller/HDMI bridge, which we're describing here. VCONN regulator is an actual GPIO controlled regulator on the board, and needs to be controlled by the anx7688 driver. So that CC1/CC2_VCONN control pins driven by the firmware actually do what they're supposed to do. Not sure why it would be a business of anything else but anx7688 driver enabling this regulator, because only this driver knows and cares about this. If some other board doesn't have the need to manually enable the regulator, or doesn't have the regulator, it can simply be optional. There are also some other funky supplies in the bindings, that are not connected to the chip in any way, but need to be controlled by the driver: + vbus-supply: true + vbus-in-supply: true First one can be on the connector node instead, where the driver can fetch it from. The purpose of the second one is to link the Phone's PMIC's USB power input with the type-c controller (anx7688), to make sure the PMIC has information about how much power it can draw from external PSU. The second one can be replaced by rewriting the anx7688 driver so that it creates a power supply representing the USB PSU connected to the phone, and by linking to anx7688 DT node from x-powers,axp813-usb-power-supply via a power-supplies property, which would mean that USB input of the phone is supplied by the external USB PSU. PMIC driver can be modified to watch the power supply provided by anx7688 driver for information it detected via USB-PD and update its input current limit via a standard helper function. This is how eg. fusb302 works. Not sure if that's any better from the PoV of DT bindings, though. Because power-supplies = <&anx7688>; will not look any greater in DT bindings, IMO. It will just link the same nodes in the other direction. Anyway, the HW is that there's an external PSU (detected by type c controller) and internal USB power input, and they are connected and one has to respect the limits of the other. I guess I shouldn't be adding a device node for external PSU, since it's not part of the phone. But that's what's trully being linked on HW level. Kind regards, o. > > > > > ANX7688 chip desing doesn't have integrated VCONN mosfet switches so it always > > needs external supply + switches that are controlled by the chip itself. There's > > no sensible design where someone would not want this and the driver needs > > to get this regulator reference from somewhere. The switches are sort of an > > extension of the chip. > > > Best regards, > Krzysztof >
On 10/04/2024 04:20, Ondřej Jirman wrote: > On Mon, Apr 08, 2024 at 10:12:30PM GMT, Krzysztof Kozlowski wrote: >> On 08/04/2024 17:17, Ondřej Jirman wrote: >>> >>> Now for things to not fail during suspend/resume based on PM callbacks >>> invocation order, anx7688 driver needs to enable this regulator too, as long >>> as it needs it. >> >> No, the I2C bus driver needs to manage it. Not one individual I2C >> device. Again, why anx7688 is specific? If you next phone has anx8867, >> using different driver, you also add there i2c-supply? And if it is >> nxp,ptn5100 as well? > > Yes, that could work, if I2C core would manage this. Either I don't understand about which I2C regulator you speak or this is not I2C core regulator. This is a regulator to be managed by the I2C controller, not by I2C core. > >>> >>> I can put bus-supply to I2C controller node, and read it from the ANX7688 driver >>> I guess, by going up a DT node. Whether that's going to be acceptable, I don't >>> know. >>> >>> >>> VCONN regulator I don't know where else to put either. It doesn't seem to belong >>> anywhere. It's not something directly connected to Type-C connector, so >>> not part of connector bindings, and there's nothing else I can see, other >>> than anx7688 device which needs it for core functionality. >> >> That sounds like a GPIO, not regulator. anx7688 has GPIOs, right? On >> Pinephone they go to regulator, but on FooPhone also using anx7688 they >> go somewhere else, so why this anx7688 assumes this is a regulator? > > CC1/CC2_VCONN control pins are "GPIO" of anx7688, sort of. They have fixed > purpose of switching external 5V regulator output to one of the CC pins > on type-c port. I don't care what other purpose with some other firmware > someone puts to those pins. It's irrelevant to the use case of anx7688 > as a type-c controller/HDMI bridge, which we're describing here. > > VCONN regulator is an actual GPIO controlled regulator on the board, and > needs to be controlled by the anx7688 driver. So that CC1/CC2_VCONN control > pins driven by the firmware actually do what they're supposed to do. > > Not sure why it would be a business of anything else but anx7688 driver > enabling this regulator, because only this driver knows and cares about this. > If some other board doesn't have the need to manually enable the regulator, or > doesn't have the regulator, it can simply be optional. > > There are also some other funky supplies in the bindings, that are not connected > to the chip in any way, but need to be controlled by the driver: > > + vbus-supply: true > + vbus-in-supply: true Yeah, the vconn looks reasonable. Just provide description of the supply, so it will be obvious. > Best regards, Krzysztof
On Thu, Apr 11, 2024 at 09:59:35PM +0200, Krzysztof Kozlowski wrote: > On 10/04/2024 04:20, Ondřej Jirman wrote: > > On Mon, Apr 08, 2024 at 10:12:30PM GMT, Krzysztof Kozlowski wrote: > >> On 08/04/2024 17:17, Ondřej Jirman wrote: > >>> > >>> Now for things to not fail during suspend/resume based on PM callbacks > >>> invocation order, anx7688 driver needs to enable this regulator too, as long > >>> as it needs it. > >> > >> No, the I2C bus driver needs to manage it. Not one individual I2C > >> device. Again, why anx7688 is specific? If you next phone has anx8867, > >> using different driver, you also add there i2c-supply? And if it is > >> nxp,ptn5100 as well? > > > > Yes, that could work, if I2C core would manage this. > > Either I don't understand about which I2C regulator you speak or this is > not I2C core regulator. This is a regulator to be managed by the I2C > controller, not by I2C core. If it is a supply that pulls up the SDA/SCL lines, then it is generic enough to be handled by the core. For example, on Qualcomm platforms CCI lines also usually have external supply as a pull-up.
diff --git a/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml b/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml new file mode 100644 index 000000000000..48b9ae936cb5 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml @@ -0,0 +1,127 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/usb/analogix,anx7688.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +# Pin names can be deduced from +# https://files.pine64.org/doc/PinePhone/PinePhone%20v1.2b%20Released%20Schematic.pdf + +title: Analogix ANX7688 Type-C controller + +maintainers: + - Pavel Machek <pavel@ucw.cz> + +properties: + compatible: + enum: + - analogix,anx7688 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + reset-gpios: + maxItems: 1 + description: GPIO controlling RESET_N (B7) pin. + + enable-gpios: + maxItems: 1 + description: GPIO controlling POWER_EN (D2) pin. + + cabledet-gpios: + maxItems: 1 + description: GPIO controlling CABLE_DET (C3) pin. + + avdd10-supply: + description: 1.0V power supply going to AVDD10 (A4, ...) pins + + dvdd10-supply: + description: 1.0V power supply going to DVDD10 (D6, ...) pins + + avdd18-supply: + description: 1.8V power supply going to AVDD18 (E3, ...) pins + + dvdd18-supply: + description: 1.8V power supply going to DVDD18 (G4, ...) pins + + avdd33-supply: + description: 3.3V power supply going to AVDD33 (C4, ...) pins + + i2c-supply: true + vconn-supply: true + hdmi-vt-supply: true + vbus-supply: true + vbus-in-supply: true + + connector: + type: object + $ref: /schemas/connector/usb-connector.yaml + + description: + Properties for usb c connector. + + properties: + compatible: + const: usb-c-connector + +required: + - compatible + - reg + - connector + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/gpio/gpio.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + typec@2c { + compatible = "analogix,anx7688"; + reg = <0x2c>; + interrupts = <8 IRQ_TYPE_EDGE_FALLING>; + interrupt-parent = <&gpio0>; + + enable-gpios = <&pio 3 10 GPIO_ACTIVE_LOW>; /* PD10 */ + reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */ + cabledet-gpios = <&r_pio 0 8 GPIO_ACTIVE_HIGH>; /* PL8 */ + + avdd10-supply = <®_anx1v0>; + dvdd10-supply = <®_anx1v0>; + avdd18-supply = <®_ldo_io1>; + dvdd18-supply = <®_ldo_io1>; + avdd33-supply = <®_dcdc1>; + i2c-supply = <®_ldo_io0>; + vconn-supply = <®_vconn5v0>; + hdmi-vt-supply = <®_dldo1>; + + vbus-supply = <®_usb_5v>; + vbus-in-supply = <&usb_power_supply>; + + typec_con: connector { + compatible = "usb-c-connector"; + power-role = "dual"; + data-role = "dual"; + try-power-role = "source"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + typec_con_ep: endpoint { + remote-endpoint = <&usbotg_hs_ep>; + }; + }; + }; + }; + }; + }; +...
Add binding for anx7688 usb type-c bridge. I don't have a datasheet, but I did best I could. Signed-off-by: Pavel Machek <pavel@ucw.cz> --- v2: implement review feedback v3: fix single character pointed by robot