linux-kernel - RE: [PATCH] usb: typec: tcpm: Report back negotiated PPS voltage and current

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2E89032DDAA8B9408CB92943514A0337019831A3F9@SW-EX-MBX01.diasemi.com>
Date:   Fri, 28 Sep 2018 18:15:34 +0000
From:   Adam Thomson <Adam.Thomson.Opensource@...semi.com>
To:     Guenter Roeck <linux@...ck-us.net>,
        Adam Thomson <Adam.Thomson.Opensource@...semi.com>
CC:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Support Opensource <Support.Opensource@...semi.com>
Subject: RE: [PATCH] usb: typec: tcpm: Report back negotiated PPS voltage
 and current

On 28 September 2018 17:04, Guenter Roeck wrote:

> On Fri, Sep 28, 2018 at 04:33:49PM +0100, Adam Thomson wrote:
> > Currently when requesting a specific voltage or current through
> > the psy interface, for PPS, when reading back from that interface
> > the values will always be the same as previously given, if the
> > request was successful. However PPS only allows for 20mV voltage
> > steps and 50mA current steps, and the psy class expects microvolt
> > and micro amp requests, so inbetween values can be provided through
> > this interface. Really when reading back the true values negotiated
> > should be given, and not the ones originally asked for.
> >
> > To report the actual values negotiated with the Source, the values
> > stored are now rounded down to the relevant step units prior to
> > building the PPS request, so that those values are later correctly
> > reported through the psy interface. In addition this improves the
> > adjustments made to meet the operating power requirements of the
> > platform, which previously could have been slightly out due to not
> > using valid PPS units of voltage and current.
> >
> > Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@...semi.com>
> > ---
> > Changes are based on usb-testing (ae8a2ca8a221)
> >
> >  drivers/usb/typec/tcpm/tcpm.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index 4f1f421..33b3981 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -2433,6 +2433,10 @@ static int tcpm_pd_build_pps_request(struct tcpm_port
> *port, u32 *rdo)
> >
> >  	flags = RDO_USB_COMM | RDO_NO_SUSPEND;
> >
> > +	/* Round down output voltage and current to align with PPS valid steps */
> > +	out_mv = out_mv - (out_mv % RDO_PROG_VOLT_MV_STEP);
> > +	op_ma = op_ma - (op_ma % RDO_PROG_CURR_MA_STEP);
> > +
> 
> The original intent was to have out_mv (ie out_volt) set to valid values.
> Some remnants of that are still in the code.
> 
> 	out_mv += RDO_PROG_VOLT_MV_STEP - (out_mv %
> RDO_PROG_VOLT_MV_STEP);
> 
> 	port->pps_data.out_volt =
> 		min(pdo_pps_apdo_max_voltage(pdo), port->pps_data.out_volt);
> 
> Somehow that got lost over time. Personally I think it would be better to set
> port->pps_data.out_vol and port->pps_data.op_curr to valid step sizes instead
> of post-adjusting the values when used.

Actually, that's a fair point. Those are used beforehand in
'tcpm_pd_select_pps_apdo()' so should be updated prior to that being called,
although in reality this wouldn't change the behaviour of the code.

I'll move the adjustment to valid step sizes (out_volt and op_curr) to the
respective functions tcpm_pps_set_out_volt() and tcpm_pps_set_op_curr().

Thanks
Adam

> 
> Thanks,
> Guenter
> 
> >  	op_mw = (op_ma * out_mv) / 1000;
> >  	if (op_mw < port->operating_snk_mw) {
> >  		/*
> > --
> > 1.9.1
> >