Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990

From: Balakrishna Godavarthi
Date: Thu Jul 26 2018 - 10:21:17 EST


Hi Matthias,

On 2018-07-26 00:01, Matthias Kaehlcke wrote:
On Tue, Jul 24, 2018 at 09:25:16PM +0530, Balakrishna Godavarthi wrote:
Hi Matthias,

On 2018-07-24 01:24, Matthias Kaehlcke wrote:
> On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:
> > + * sometimes we will face communication synchronization issues,
> > + * like reading version command timeouts. In which HCI_SETUP fails,
> > + * to overcome these issues, we try to communicate by performing an
> > + * COLD power OFF and ON.
> > + */
> > + for (i = 1; i <= 10 && ret; i++) {
>
> Is it really that bad that more than say 3 iterations might be needed?
>
[Bala]: will restrict to 3 iterations.

Is 3x expected to be enough to 'guarantee' as successful
initialization? Just wondered about the 10x since it suddendly changed
from 1x. What is the failure rate without retries?

Could you provide more information about the 'communication
synchronization issues'? Is the root cause understood? Maybe there is
a better way than retries.


[Bala]: basically before sending a every patch series we run a stress test to the driver to detect the bugs.
in recent test results found one interesting bug that BT setups fails with version request timeouts,
after we do a reboot for the device.
we debugged the issue and found that wcn3900 is not responding to the version request commands
sent by HOST. this is because before reboot, wcn3990 is in on state i.e. we are communicating to device.
then we did a reboot and HOST is not sending a power off request to the regulators to turn off.
so after reboot wcn3990 is still in ON state where it will not respond to version request commands which in turn fails HCI_SETUP.
so we are sending the power off pulse and then sending the power on pulse.
coming back to 3x or 10x iteration this is to avoid any such synchronization issues.
i agreed for 3x because of stress test results. we have success rate of 99% for single iteration, where as 3x iterations will helps to handle 1% fails cases.

> > +static void qca_regulator_get_current(struct device *dev,
> > + struct qca_vreg *vregs)
> > +{
> > + char prop_name[32]; /* 32 is max size of property name */
> > +
> > + /* We have different platforms where the load value is controlled
> > + * via PMIC controllers. In such cases load required to power ON
> > + * Bluetooth chips are defined in the PMIC. We have option to set
> > + * operation mode like high or low power modes.
> > + * We do have some platforms where driver need to enable the load
> > for
> > + * WCN3990. Based on the current property value defined for the
> > + * regulators, driver will decide the regulator output load.
> > + * If the current property for the regulator is defined in the dts
> > + * we will read from dts tree, else from the default load values.
> > + */
>
> Let's make sure we all really understand why this is needed. You
> mentioned RPMh regulators earlier and said a special value of 1uA
> would be needed to enable high power mode. Later when I pointed to the
> RPMh regulator code you agreed that this special value wouldn't make
> any difference.
>
> Now the defaults are higher:
>
[Bala]: today i got the info from the power teams here, currently in the
downstream what we have is different wrt to the
patch "https://patchwork.kernel.org/patch/10524299/"; by David
Collins.
prior to his patch we have different architecture where 1uA will
change the mode to HPM mode.
which is not valid, so 1uA will not work any more. we have go with
actual current values.

Ok, in any case downstream drivers shouldn't impact the design of
upstream drivers. If there are incompatibilities the BT driver needs
to be hacked in the downstream tree.

coming back to reading current values from dts. we have reason for
it.
let us assume that later stages of wcn3990 if we have less current
values than default values.
instead of updating the driver again, we can assign the current no
in the dts, which we will read.

This is how it works.

if(current value for the reg is declared in dts tree)
consider the current value from the dts.
else
go with default value.

pls let me know if you any queries.

If I understand correctly you describe a hypothetical situation of a
future wcn3990 variant having lower power requirements. I'd say let's
deal with this when these chips actually exist and need to be
supported by Linux. As of now it seems there is no need for current
limits in the DT.


[Bala]: will remove current property for dts.
in previous mail you asked me a question for currents
"The currents of 300mA and 450mA seem high for Bluetooth, I'm not an
expert in this area though, they might be reasonable peak currents for
certain use cases."

yes we require 450mA and 300mA of current for rf and ch0 pins. setting regulator to required load will not pump load current to wcn3990
it depends on operations, typical the above are the max current drawn by the two pins.

> > + if (device_property_read_bool(dev, prop_name))
> > + device_property_read_u32(dev, prop_name, &vregs->load_uA);
>
> Why device_property_read_bool()?
>
[Bala]: if the current prop is present we read from dts. else we go with
default current no's.
if block is used to check whether the property is present in dts or
not.
this is required because before calling _regualtor_get_current() we
hold the default current in the vregs[].
if we skip the read bool here, if the current property is not
present then the function call of device_property_read_u32() will assign
zero the vregs[].
so we miss the default current values.

this how it work if we miss read_bool check

//vregs hold default current
device_property_read_u32(dev, prop_name, &vregs->load_uA);
the above will read the current property value from dts store in
the vregs.. if the property is missing in dts it will store zero.

Where does of_property_read_u32() set the value to zero when the
property does not exist?

A simple test in a probe function:

{
u32 v = 123;
of_property_read_u32(node, "no-such-property", &v);
printk("DBG: v = %d\n", v);
}

[ 7.598366] DBG: v = 123

And looking at the code, of_property_read_u32() ends up in a call to
of_property_read_variable_u32_array():

int of_property_read_variable_u32_array(const struct device_node *np,
const char *propname, u32 *out_values,
size_t sz_min, size_t sz_max)
{
size_t sz, count;
const __be32 *val = of_find_property_value_of_size(np, propname,
(sz_min * sizeof(*out_values)),
(sz_max * sizeof(*out_values)),
&sz);

if (IS_ERR(val))
return PTR_ERR(val);

...
}

i.e. 'out_values' is not modified when the property does not exit.

[Bala]: Thanks for clarifying me.

--
Regards
Balakrishna.