| Message ID | 20180928153349.DC14D3FAF0@swsrvapps-01.diasemi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: typec: tcpm: Report back negotiated PPS voltage and current | expand |
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@diasemi.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. Thanks, Guenter > op_mw = (op_ma * out_mv) / 1000; > if (op_mw < port->operating_snk_mw) { > /* > -- > 1.9.1 >
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@diasemi.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 > >
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); + op_mw = (op_ma * out_mv) / 1000; if (op_mw < port->operating_snk_mw) { /*
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@diasemi.com> --- Changes are based on usb-testing (ae8a2ca8a221) drivers/usb/typec/tcpm/tcpm.c | 4 ++++ 1 file changed, 4 insertions(+)