RE: [PATCH v3] usb: typec: get the vbus source and charge values from the devicetree

[Jun Li]

Hi
> -----Original Message-----
> From: Angus Ainslie <angus@xxxxxxxx>
> Sent: 2018å9æ13æ 19:10
> To: Peter Chen <hzpeterchen@xxxxxxxxx>
> Cc: linux@xxxxxxxxxxxx; Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>; Greg
> Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; lkml
> <linux-kernel@xxxxxxxxxxxxxxx>; Peter Chen <peter.chen@xxxxxxx>; Jun Li
> <jun.li@xxxxxxx>; Guenter Roeck <linux@xxxxxxxxxxxx>
> 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@xxxxxxxxxxxx>
> > 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.