| Message ID | 20180911145931.32441-1-angus@akkea.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] usb: typec: get the vbus source and charge values from the devicetree | expand |
I cant put my finger on it but this seems wrong. As i said both src and sink should never be true at the same time. I also din’t understand why turning off src should power off your board. Ultimately my concern is that we may be just painting over the real problem, and that would be really bad to do with dt properties. Note that sending to groeck7@ makes it hard for me to see your emails and to reply. Using linux@roeck-us.net would be much better. Guenter Sent from my iPhone > On Sep 11, 2018, at 7:59 AM, Angus Ainslie (Purism) <angus@akkea.ca> wrote: > > If the board is being powered by USB disabling the source and sink > can remove power from the board. Allow the source and sink to be > initallized based on devicetree values. > > Changed since V2: > > Change the devicetree documentation. > Change the devicetree property names. > > Changed since V1: > > use devicetree values instead of hardcoded initialization. > > Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca> > --- > .../devicetree/bindings/usb/typec-tcpci.txt | 6 ++++++ > drivers/usb/typec/tcpm.c | 14 +++++++++++--- > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/usb/typec-tcpci.txt b/Documentation/devicetree/bindings/usb/typec-tcpci.txt > index 0dd1469e7318..b07418ae6482 100644 > --- a/Documentation/devicetree/bindings/usb/typec-tcpci.txt > +++ b/Documentation/devicetree/bindings/usb/typec-tcpci.txt > @@ -15,6 +15,12 @@ Required sub-node: > of connector node are specified in > Documentation/devicetree/bindings/connector/usb-connector.txt > > +Optional properties for usb-c-connector sub-node: > +- init-vbus-source: set the initalization value for vbus-source to true. > + If this property is not present the initial value will be false. > +- init-vbus-sink: set the initalization value for vbus-sink to true. > + If this property is not present the initial value will be false. > + > Example: > > ptn5110@50 { > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c > index ca7bedb46f7f..10c14ece3858 100644 > --- a/drivers/usb/typec/tcpm.c > +++ b/drivers/usb/typec/tcpm.c > @@ -2462,9 +2462,7 @@ static int tcpm_init_vbus(struct tcpm_port *port) > { > int ret; > > - ret = port->tcpc->set_vbus(port->tcpc, false, false); > - port->vbus_source = false; > - port->vbus_charge = false; > + ret = port->tcpc->set_vbus(port->tcpc, port->vbus_source, port->vbus_charge); > return ret; > } > > @@ -4266,6 +4264,16 @@ static int tcpm_fw_get_caps(struct tcpm_port *port, > return -EINVAL; > port->port_type = port->typec_caps.type; > > + if (fwnode_property_present(fwnode, "init-vbus-source")) > + port->vbus_source = true; > + else > + port->vbus_source = false; > + > + if (fwnode_property_present(fwnode, "init-vbus-sink")) > + port->vbus_charge = true; > + else > + port->vbus_charge = false; > + > if (port->port_type == TYPEC_PORT_SNK) > goto sink; > > -- > 2.17.1 >
On 2018-09-11 09:33, Guenter Roeck wrote: > I cant put my finger on it but this seems wrong. As i said both src > and sink should never be true at the same time. I also din’t > understand why turning off src should power off your board. Ultimately > my concern is that we may be just painting over the real problem, and > that would be really bad to do with dt properties. > I agree that this doesn't seem like the correct way of solving the problem. On this HW (Emcraft iMX8M BSB) I think the PTN5110 chip has been connected correctly so I'm assuming that it is some quirk of the PTN5110. I didn't design the HW or the chip. This is a workaround for "quirky" hardware and there may be others that don't behave exactly as expected. > Note that sending to groeck7@ makes it hard for me to see your emails > and to reply. Using linux@roeck-us.net would be much better. > Ok, I think that email was grabbed from the driver source. > Guenter > > Sent from my iPhone > >> On Sep 11, 2018, at 7:59 AM, Angus Ainslie (Purism) <angus@akkea.ca> >> wrote: >> >> If the board is being powered by USB disabling the source and sink >> can remove power from the board. Allow the source and sink to be >> initallized based on devicetree values. >> >> Changed since V2: >> >> Change the devicetree documentation. >> Change the devicetree property names. >> >> Changed since V1: >> >> use devicetree values instead of hardcoded initialization. >> >> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca> >> --- >> .../devicetree/bindings/usb/typec-tcpci.txt | 6 ++++++ >> drivers/usb/typec/tcpm.c | 14 +++++++++++--- >> 2 files changed, 17 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/usb/typec-tcpci.txt >> b/Documentation/devicetree/bindings/usb/typec-tcpci.txt >> index 0dd1469e7318..b07418ae6482 100644 >> --- a/Documentation/devicetree/bindings/usb/typec-tcpci.txt >> +++ b/Documentation/devicetree/bindings/usb/typec-tcpci.txt >> @@ -15,6 +15,12 @@ Required sub-node: >> of connector node are specified in >> Documentation/devicetree/bindings/connector/usb-connector.txt >> >> +Optional properties for usb-c-connector sub-node: >> +- init-vbus-source: set the initalization value for vbus-source to >> true. >> + If this property is not present the initial value will be false. >> +- init-vbus-sink: set the initalization value for vbus-sink to true. >> + If this property is not present the initial value will be false. >> + >> Example: >> >> ptn5110@50 { >> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c >> index ca7bedb46f7f..10c14ece3858 100644 >> --- a/drivers/usb/typec/tcpm.c >> +++ b/drivers/usb/typec/tcpm.c >> @@ -2462,9 +2462,7 @@ static int tcpm_init_vbus(struct tcpm_port >> *port) >> { >> int ret; >> >> - ret = port->tcpc->set_vbus(port->tcpc, false, false); >> - port->vbus_source = false; >> - port->vbus_charge = false; >> + ret = port->tcpc->set_vbus(port->tcpc, port->vbus_source, >> port->vbus_charge); >> return ret; >> } >> >> @@ -4266,6 +4264,16 @@ static int tcpm_fw_get_caps(struct tcpm_port >> *port, >> return -EINVAL; >> port->port_type = port->typec_caps.type; >> >> + if (fwnode_property_present(fwnode, "init-vbus-source")) >> + port->vbus_source = true; >> + else >> + port->vbus_source = false; >> + >> + if (fwnode_property_present(fwnode, "init-vbus-sink")) >> + port->vbus_charge = true; >> + else >> + port->vbus_charge = false; >> + >> if (port->port_type == TYPEC_PORT_SNK) >> goto sink; >> >> -- >> 2.17.1 >>
On Wed, Sep 12, 2018 at 10:08:58AM -0600, Angus Ainslie wrote: > On 2018-09-11 09:33, Guenter Roeck wrote: > >I cant put my finger on it but this seems wrong. As i said both src > >and sink should never be true at the same time. I also din’t > >understand why turning off src should power off your board. Ultimately > >my concern is that we may be just painting over the real problem, and > >that would be really bad to do with dt properties. > > > > I agree that this doesn't seem like the correct way of solving the problem. > On this HW (Emcraft iMX8M BSB) I think the PTN5110 chip has been connected > correctly so I'm assuming that it is some quirk of the PTN5110. > > I didn't design the HW or the chip. This is a workaround for "quirky" > hardware and there may be others that don't behave exactly as expected. > I wouldn't be that sure about that. It may as well be that the tcpc driver and/or the tcpm driver are doing something wrong when initializing. I didn't really understand the logs you sent out earlier. It looked like the system would loose power if the TCPC_CMD_DISABLE_SRC_VBUS command is sent. That doesn't really make sense to me since it indicates that the chip sources power to the remote, and turning that off should not result in a local loss of power. Note that the chip is supposed to be able to report if it is sourcing vbus and if VBUS is present, in the POWER_STATUS register. Another question is the content of the ROLE_CONTROL register when the system boots, and the DEVICE_CAPABILITIES settings. Overall I suspect that we don't handle startup for your system correctly in the tcpc driver. The ideal solution would be to find a solution which does not require any devicetree properties, but to do that we'll need to get a better understanding about your system's requirements. Guenter
On Thu, Sep 13, 2018 at 12:35 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On Wed, Sep 12, 2018 at 10:08:58AM -0600, Angus Ainslie wrote: > > On 2018-09-11 09:33, Guenter Roeck wrote: > > >I cant put my finger on it but this seems wrong. As i said both src > > >and sink should never be true at the same time. I also din’t > > >understand why turning off src should power off your board. Ultimately > > >my concern is that we may be just painting over the real problem, and > > >that would be really bad to do with dt properties. > > > > > > > I agree that this doesn't seem like the correct way of solving the problem. > > On this HW (Emcraft iMX8M BSB) I think the PTN5110 chip has been connected > > correctly so I'm assuming that it is some quirk of the PTN5110. > > > > I didn't design the HW or the chip. This is a workaround for "quirky" > > hardware and there may be others that don't behave exactly as expected. > > > > I wouldn't be that sure about that. It may as well be that the tcpc driver > and/or the tcpm driver are doing something wrong when initializing. > > I didn't really understand the logs you sent out earlier. It looked like > the system would loose power if the TCPC_CMD_DISABLE_SRC_VBUS command is > sent. That doesn't really make sense to me since it indicates that the > chip sources power to the remote, and turning that off should not result > in a local loss of power. > > Note that the chip is supposed to be able to report if it is sourcing vbus > and if VBUS is present, in the POWER_STATUS register. Another question is > the content of the ROLE_CONTROL register when the system boots, and the > DEVICE_CAPABILITIES settings. > > Overall I suspect that we don't handle startup for your system correctly > in the tcpc driver. The ideal solution would be to find a solution which > does not require any devicetree properties, but to do that we'll need > to get a better understanding about your system's requirements. > > Guenter Hi Angus, Would you please check if below patch can fix your issue? staging: typec: don't do vbus source disable for dead battery In PTN5110 design, DisableSourceVBUS command also disables the sink enable signal because the EN_SNK can be used to source higher voltage, and, there is only one TCPC command to disable sourcing voltage without telling whether to disable 5V or the high voltage, and to keep the design simple they designed the PTN5110 to disable both. with this fact, we use the flag drive_vbus to check if the source vbus enable was issued, if yes we then do vbus source disable, in dead battery case, we never did vbus source enable, so will not issue vbus source disable command. Acked-by: Peter Chen <peter.chen@nxp.com> Signed-off-by: Li Jun <jun.li@nxp.com> --- drivers/staging/typec/tcpci.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c index 2d4fbb8aac5e..7352207224b5 100644 --- a/drivers/staging/typec/tcpci.c +++ b/drivers/staging/typec/tcpci.c @@ -381,9 +381,8 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, bool source, bool sink) struct tcpci *tcpci = tcpc_to_tcpci(tcpc); int ret; - /* Disable both source and sink first before enabling anything */ - - if (!source) { + /* Only disable source if it was enabled */ + if (!source && tcpci->drive_vbus) { ret = regmap_write(tcpci->regmap, TCPC_COMMAND, TCPC_CMD_DISABLE_SRC_VBUS); if (ret < 0)
On 2018-09-13 01:27, Peter Chen wrote: > On Thu, Sep 13, 2018 at 12:35 AM Guenter Roeck <linux@roeck-us.net> > wrote: >> >> On Wed, Sep 12, 2018 at 10:08:58AM -0600, Angus Ainslie wrote: >> > On 2018-09-11 09:33, Guenter Roeck wrote: >> > >I cant put my finger on it but this seems wrong. As i said both src >> > >and sink should never be true at the same time. I also din’t >> > >understand why turning off src should power off your board. Ultimately >> > >my concern is that we may be just painting over the real problem, and >> > >that would be really bad to do with dt properties. >> > > >> > >> > I agree that this doesn't seem like the correct way of solving the problem. >> > On this HW (Emcraft iMX8M BSB) I think the PTN5110 chip has been connected >> > correctly so I'm assuming that it is some quirk of the PTN5110. >> > >> > I didn't design the HW or the chip. This is a workaround for "quirky" >> > hardware and there may be others that don't behave exactly as expected. >> > >> >> I wouldn't be that sure about that. It may as well be that the tcpc >> driver >> and/or the tcpm driver are doing something wrong when initializing. >> >> I didn't really understand the logs you sent out earlier. It looked >> like >> the system would loose power if the TCPC_CMD_DISABLE_SRC_VBUS command >> is >> sent. That doesn't really make sense to me since it indicates that >> the >> chip sources power to the remote, and turning that off should not >> result >> in a local loss of power. >> >> Note that the chip is supposed to be able to report if it is sourcing >> vbus >> and if VBUS is present, in the POWER_STATUS register. Another question >> is >> the content of the ROLE_CONTROL register when the system boots, and >> the >> DEVICE_CAPABILITIES settings. >> >> Overall I suspect that we don't handle startup for your system >> correctly >> in the tcpc driver. The ideal solution would be to find a solution >> which >> does not require any devicetree properties, but to do that we'll need >> to get a better understanding about your system's requirements. >> >> Guenter > > Hi Angus, > > Would you please check if below patch can fix your issue? > > staging: typec: don't do vbus source disable for dead battery > > In PTN5110 design, DisableSourceVBUS command also disables the sink > enable signal because the EN_SNK can be used to source higher voltage, > and, there is only one TCPC command to disable sourcing voltage without > telling whether to disable 5V or the high voltage, and to keep the > design simple they designed the PTN5110 to disable both. with this > fact, we use the flag drive_vbus to check if the source vbus enable was > issued, if yes we then do vbus source disable, in dead battery case, > we never did vbus source enable, so will not issue vbus source disable > command. > Thanks Peter, this sounds like the missing piece of information and I think some form of the code below will fix that. There is still the issue that my board will need some way of controlling the initial state of vbus-sink. @Guenter: would my initial patch be acceptable to set the default state of vbus-source and vbus-sink. Would you like some code to sanity check that both were not enabled at the same time ? > Acked-by: Peter Chen <peter.chen@nxp.com> > Signed-off-by: Li Jun <jun.li@nxp.com> > --- > drivers/staging/typec/tcpci.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/typec/tcpci.c > b/drivers/staging/typec/tcpci.c > index 2d4fbb8aac5e..7352207224b5 100644 > --- a/drivers/staging/typec/tcpci.c > +++ b/drivers/staging/typec/tcpci.c > @@ -381,9 +381,8 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, > bool source, bool sink) > struct tcpci *tcpci = tcpc_to_tcpci(tcpc); > int ret; > > - /* Disable both source and sink first before enabling anything */ > - > - if (!source) { > + /* Only disable source if it was enabled */ > + if (!source && tcpci->drive_vbus) { > ret = regmap_write(tcpci->regmap, TCPC_COMMAND, > TCPC_CMD_DISABLE_SRC_VBUS); > if (ret < 0) The version of struct tcpci doesn't have a drive_vbus. Where should drive_vbus get set and cleared ? Is this a more complete version of what you intended ? diff --git a/drivers/usb/typec/tcpci.c b/drivers/usb/typec/tcpci.c index ac6b418b15f1..d6168163df7b 100644 --- a/drivers/usb/typec/tcpci.c +++ b/drivers/usb/typec/tcpci.c @@ -28,6 +28,7 @@ struct tcpci { struct regmap *regmap; bool controls_vbus; + bool drive_vbus; struct tcpc_dev tcpc; struct tcpci_data *data; @@ -277,7 +278,9 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, bool source, bool sink) /* Disable both source and sink first before enabling anything */ - if (!source) { + if (!source && tcpci->drive_vbus) { + tcpci->drive_vbus = false; + ret = regmap_write(tcpci->regmap, TCPC_COMMAND, TCPC_CMD_DISABLE_SRC_VBUS); if (ret < 0) @@ -292,6 +295,8 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, bool source, bool sink) } if (source) { + tcpci->drive_vbus = true; + ret = regmap_write(tcpci->regmap, TCPC_COMMAND, TCPC_CMD_SRC_VBUS_DEFAULT); if (ret < 0) @@ -503,6 +508,7 @@ struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data) tcpci->dev = dev; tcpci->data = data; tcpci->regmap = data->regmap; + tcpci->drive_vbus = false; tcpci->tcpc.init = tcpci_init; tcpci->tcpc.get_vbus = tcpci_get_vbus;
On Thu, Sep 13, 2018 at 05:10:09AM -0600, Angus Ainslie wrote: > > > >staging: typec: don't do vbus source disable for dead battery > > > >In PTN5110 design, DisableSourceVBUS command also disables the sink > >enable signal because the EN_SNK can be used to source higher voltage, > >and, there is only one TCPC command to disable sourcing voltage without > >telling whether to disable 5V or the high voltage, and to keep the > >design simple they designed the PTN5110 to disable both. with this > >fact, we use the flag drive_vbus to check if the source vbus enable was > >issued, if yes we then do vbus source disable, in dead battery case, > >we never did vbus source enable, so will not issue vbus source disable > >command. > > > > Thanks Peter, this sounds like the missing piece of information and I think > some form of the code below will fix that. > > There is still the issue that my board will need some way of controlling the > initial state of vbus-sink. > > @Guenter: would my initial patch be acceptable to set the default state of > vbus-source and vbus-sink. Would you like some code to sanity check that > both were not enabled at the same time ? > Seems to me this is indeed a chip specific problem. I don't think a fix belongs into the tcpm code. As mentioned before, the current status (ie drive_vbus) should be readable from the chip. I am not sure though if we should add a PTN5110 specific quirk to the driver to handle the situation. I am concerned that fixing this as suggested below for PTN5110 may cause trouble with other chips. Maybe not, but I'd rather be cautious. Thanks, Guenter > >Acked-by: Peter Chen <peter.chen@nxp.com> > >Signed-off-by: Li Jun <jun.li@nxp.com> > >--- > > drivers/staging/typec/tcpci.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > >diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c > >index 2d4fbb8aac5e..7352207224b5 100644 > >--- a/drivers/staging/typec/tcpci.c > >+++ b/drivers/staging/typec/tcpci.c > >@@ -381,9 +381,8 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, > >bool source, bool sink) > > struct tcpci *tcpci = tcpc_to_tcpci(tcpc); > > int ret; > > > >- /* Disable both source and sink first before enabling anything */ > >- > >- if (!source) { > >+ /* Only disable source if it was enabled */ > >+ if (!source && tcpci->drive_vbus) { > > ret = regmap_write(tcpci->regmap, TCPC_COMMAND, > > TCPC_CMD_DISABLE_SRC_VBUS); > > if (ret < 0) > > The version of struct tcpci doesn't have a drive_vbus. Where should > drive_vbus get set and cleared ? > > Is this a more complete version of what you intended ? > > diff --git a/drivers/usb/typec/tcpci.c b/drivers/usb/typec/tcpci.c > index ac6b418b15f1..d6168163df7b 100644 > --- a/drivers/usb/typec/tcpci.c > +++ b/drivers/usb/typec/tcpci.c > @@ -28,6 +28,7 @@ struct tcpci { > struct regmap *regmap; > > bool controls_vbus; > + bool drive_vbus; > > struct tcpc_dev tcpc; > struct tcpci_data *data; > @@ -277,7 +278,9 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, bool > source, bool sink) > > /* Disable both source and sink first before enabling anything */ > > - if (!source) { > + if (!source && tcpci->drive_vbus) { > + tcpci->drive_vbus = false; > + > ret = regmap_write(tcpci->regmap, TCPC_COMMAND, > TCPC_CMD_DISABLE_SRC_VBUS); > if (ret < 0) > @@ -292,6 +295,8 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, bool > source, bool sink) > } > > if (source) { > + tcpci->drive_vbus = true; > + > ret = regmap_write(tcpci->regmap, TCPC_COMMAND, > TCPC_CMD_SRC_VBUS_DEFAULT); > if (ret < 0) > @@ -503,6 +508,7 @@ struct tcpci *tcpci_register_port(struct device *dev, > struct tcpci_data *data) > tcpci->dev = dev; > tcpci->data = data; > tcpci->regmap = data->regmap; > + tcpci->drive_vbus = false; > > tcpci->tcpc.init = tcpci_init; > tcpci->tcpc.get_vbus = tcpci_get_vbus; > > >
Hi > -----Original Message----- > From: Angus Ainslie <angus@akkea.ca> > Sent: 2018年9月13日 19:10 > To: Peter Chen <hzpeterchen@gmail.com> > Cc: linux@roeck-us.net; Heikki Krogerus <heikki.krogerus@linux.intel.com>; Greg > Kroah-Hartman <gregkh@linuxfoundation.org>; linux-usb@vger.kernel.org; lkml > <linux-kernel@vger.kernel.org>; Peter Chen <peter.chen@nxp.com>; Jun Li > <jun.li@nxp.com>; Guenter Roeck <linux@roeck-us.net> > Subject: Re: [PATCH v3] usb: typec: get the vbus source and charge values from the > devicetree > > On 2018-09-13 01:27, Peter Chen wrote: > > On Thu, Sep 13, 2018 at 12:35 AM Guenter Roeck <linux@roeck-us.net> > > wrote: > >> > >> On Wed, Sep 12, 2018 at 10:08:58AM -0600, Angus Ainslie wrote: > >> > On 2018-09-11 09:33, Guenter Roeck wrote: > >> > >I cant put my finger on it but this seems wrong. As i said both > >> > >src and sink should never be true at the same time. I also din’t > >> > >understand why turning off src should power off your board. > >> > >Ultimately my concern is that we may be just painting over the > >> > >real problem, and that would be really bad to do with dt properties. > >> > > > >> > > >> > I agree that this doesn't seem like the correct way of solving the problem. > >> > On this HW (Emcraft iMX8M BSB) I think the PTN5110 chip has been > >> > connected correctly so I'm assuming that it is some quirk of the PTN5110. > >> > > >> > I didn't design the HW or the chip. This is a workaround for "quirky" > >> > hardware and there may be others that don't behave exactly as expected. > >> > > >> > >> I wouldn't be that sure about that. It may as well be that the tcpc > >> driver and/or the tcpm driver are doing something wrong when > >> initializing. > >> > >> I didn't really understand the logs you sent out earlier. It looked > >> like the system would loose power if the TCPC_CMD_DISABLE_SRC_VBUS > >> command is sent. That doesn't really make sense to me since it > >> indicates that the chip sources power to the remote, and turning that > >> off should not result in a local loss of power. > >> > >> Note that the chip is supposed to be able to report if it is sourcing > >> vbus and if VBUS is present, in the POWER_STATUS register. Another > >> question is the content of the ROLE_CONTROL register when the system > >> boots, and the DEVICE_CAPABILITIES settings. > >> > >> Overall I suspect that we don't handle startup for your system > >> correctly in the tcpc driver. The ideal solution would be to find a > >> solution which does not require any devicetree properties, but to do > >> that we'll need to get a better understanding about your system's > >> requirements. > >> > >> Guenter > > > > Hi Angus, > > > > Would you please check if below patch can fix your issue? > > > > staging: typec: don't do vbus source disable for dead battery > > > > In PTN5110 design, DisableSourceVBUS command also disables the sink > > enable signal because the EN_SNK can be used to source higher voltage, > > and, there is only one TCPC command to disable sourcing voltage > > without telling whether to disable 5V or the high voltage, and to keep > > the design simple they designed the PTN5110 to disable both. with this > > fact, we use the flag drive_vbus to check if the source vbus enable > > was issued, if yes we then do vbus source disable, in dead battery > > case, we never did vbus source enable, so will not issue vbus source > > disable command. > > > > Thanks Peter, this sounds like the missing piece of information and I think some form of > the code below will fix that. > > There is still the issue that my board will need some way of controlling the initial state of > vbus-sink. > > @Guenter: would my initial patch be acceptable to set the default state of vbus-source and > vbus-sink. Would you like some code to sanity check that both were not enabled at the > same time ? I agree Guenter using a dt property is not the proper way here(maybe valid for your HW with only typec power supply), I think a right way is to change tcpm to handle this kind case(like the existing vbus_never_low flag) so we will have a common handling if (vbus_on && CC_state_is_RP) dead_battery = true; bypass tcpm_init_vbus or only enable sink.
Hi > -----Original Message----- > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > Sent: 2018年9月14日 1:35 > To: Angus Ainslie <angus@akkea.ca> > Cc: Peter Chen <hzpeterchen@gmail.com>; Heikki Krogerus > <heikki.krogerus@linux.intel.com>; Greg Kroah-Hartman > <gregkh@linuxfoundation.org>; linux-usb@vger.kernel.org; lkml > <linux-kernel@vger.kernel.org>; Peter Chen <peter.chen@nxp.com>; Jun Li > <jun.li@nxp.com> > Subject: Re: [PATCH v3] usb: typec: get the vbus source and charge values from the > devicetree > > On Thu, Sep 13, 2018 at 05:10:09AM -0600, Angus Ainslie wrote: > > > > > >staging: typec: don't do vbus source disable for dead battery > > > > > >In PTN5110 design, DisableSourceVBUS command also disables the sink > > >enable signal because the EN_SNK can be used to source higher > > >voltage, and, there is only one TCPC command to disable sourcing > > >voltage without telling whether to disable 5V or the high voltage, > > >and to keep the design simple they designed the PTN5110 to disable > > >both. with this fact, we use the flag drive_vbus to check if the > > >source vbus enable was issued, if yes we then do vbus source disable, > > >in dead battery case, we never did vbus source enable, so will not > > >issue vbus source disable command. > > > > > > > Thanks Peter, this sounds like the missing piece of information and I > > think some form of the code below will fix that. > > > > There is still the issue that my board will need some way of > > controlling the initial state of vbus-sink. > > > > @Guenter: would my initial patch be acceptable to set the default > > state of vbus-source and vbus-sink. Would you like some code to sanity > > check that both were not enabled at the same time ? > > > Seems to me this is indeed a chip specific problem. I don't think a fix belongs into the tcpm > code. As mentioned before, the current status (ie drive_vbus) should be readable from the > chip. I am not sure though if we should add a > PTN5110 specific quirk to the driver to handle the situation. I am concerned that fixing this > as suggested below for PTN5110 may cause trouble with other chips. Maybe not, but I'd > rather be cautious. Yes, this is a chip specific problem, the reason DisableSourceVBUS command also disables the sink enable signal because the EN_SNK of PTN5110 can be used to source higher voltage And, there is only one TCPC command to disable sourcing voltage without telling whether to disable 5V or the high voltage, and to keep the design simple, TPN5110 is designed to disable both. The patch below is to not issue a vbus disable command if it was not enabled, from my point view it should be OK and has the benefit to save one command. Li Jun > > Thanks, > Guenter > > > >Acked-by: Peter Chen <peter.chen@nxp.com> > > >Signed-off-by: Li Jun <jun.li@nxp.com> > > >--- > > > drivers/staging/typec/tcpci.c | 5 ++--- > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > >diff --git a/drivers/staging/typec/tcpci.c > > >b/drivers/staging/typec/tcpci.c index 2d4fbb8aac5e..7352207224b5 > > >100644 > > >--- a/drivers/staging/typec/tcpci.c > > >+++ b/drivers/staging/typec/tcpci.c > > >@@ -381,9 +381,8 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, > > >bool source, bool sink) > > > struct tcpci *tcpci = tcpc_to_tcpci(tcpc); > > > int ret; > > > > > >- /* Disable both source and sink first before enabling anything */ > > >- > > >- if (!source) { > > >+ /* Only disable source if it was enabled */ if (!source && > > >+ tcpci->drive_vbus) { > > > ret = regmap_write(tcpci->regmap, TCPC_COMMAND, > > > TCPC_CMD_DISABLE_SRC_VBUS); > > > if (ret < 0) > > > > The version of struct tcpci doesn't have a drive_vbus. Where should > > drive_vbus get set and cleared ? > > > > Is this a more complete version of what you intended ? > > > > diff --git a/drivers/usb/typec/tcpci.c b/drivers/usb/typec/tcpci.c > > index ac6b418b15f1..d6168163df7b 100644 > > --- a/drivers/usb/typec/tcpci.c > > +++ b/drivers/usb/typec/tcpci.c > > @@ -28,6 +28,7 @@ struct tcpci { > > struct regmap *regmap; > > > > bool controls_vbus; > > + bool drive_vbus; > > > > struct tcpc_dev tcpc; > > struct tcpci_data *data; > > @@ -277,7 +278,9 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, > > bool source, bool sink) > > > > /* Disable both source and sink first before enabling anything > > */ > > > > - if (!source) { > > + if (!source && tcpci->drive_vbus) { > > + tcpci->drive_vbus = false; > > + > > ret = regmap_write(tcpci->regmap, TCPC_COMMAND, > > TCPC_CMD_DISABLE_SRC_VBUS); > > if (ret < 0) > > @@ -292,6 +295,8 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, > > bool source, bool sink) > > } > > > > if (source) { > > + tcpci->drive_vbus = true; > > + > > ret = regmap_write(tcpci->regmap, TCPC_COMMAND, > > TCPC_CMD_SRC_VBUS_DEFAULT); > > if (ret < 0) > > @@ -503,6 +508,7 @@ struct tcpci *tcpci_register_port(struct device > > *dev, struct tcpci_data *data) > > tcpci->dev = dev; > > tcpci->data = data; > > tcpci->regmap = data->regmap; > > + tcpci->drive_vbus = false; > > > > tcpci->tcpc.init = tcpci_init; > > tcpci->tcpc.get_vbus = tcpci_get_vbus; > > > > > >
diff --git a/Documentation/devicetree/bindings/usb/typec-tcpci.txt b/Documentation/devicetree/bindings/usb/typec-tcpci.txt index 0dd1469e7318..b07418ae6482 100644 --- a/Documentation/devicetree/bindings/usb/typec-tcpci.txt +++ b/Documentation/devicetree/bindings/usb/typec-tcpci.txt @@ -15,6 +15,12 @@ Required sub-node: of connector node are specified in Documentation/devicetree/bindings/connector/usb-connector.txt +Optional properties for usb-c-connector sub-node: +- init-vbus-source: set the initalization value for vbus-source to true. + If this property is not present the initial value will be false. +- init-vbus-sink: set the initalization value for vbus-sink to true. + If this property is not present the initial value will be false. + Example: ptn5110@50 { diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c index ca7bedb46f7f..10c14ece3858 100644 --- a/drivers/usb/typec/tcpm.c +++ b/drivers/usb/typec/tcpm.c @@ -2462,9 +2462,7 @@ static int tcpm_init_vbus(struct tcpm_port *port) { int ret; - ret = port->tcpc->set_vbus(port->tcpc, false, false); - port->vbus_source = false; - port->vbus_charge = false; + ret = port->tcpc->set_vbus(port->tcpc, port->vbus_source, port->vbus_charge); return ret; } @@ -4266,6 +4264,16 @@ static int tcpm_fw_get_caps(struct tcpm_port *port, return -EINVAL; port->port_type = port->typec_caps.type; + if (fwnode_property_present(fwnode, "init-vbus-source")) + port->vbus_source = true; + else + port->vbus_source = false; + + if (fwnode_property_present(fwnode, "init-vbus-sink")) + port->vbus_charge = true; + else + port->vbus_charge = false; + if (port->port_type == TYPEC_PORT_SNK) goto sink;
If the board is being powered by USB disabling the source and sink can remove power from the board. Allow the source and sink to be initallized based on devicetree values. Changed since V2: Change the devicetree documentation. Change the devicetree property names. Changed since V1: use devicetree values instead of hardcoded initialization. Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca> --- .../devicetree/bindings/usb/typec-tcpci.txt | 6 ++++++ drivers/usb/typec/tcpm.c | 14 +++++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-)