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

Hi Matthias,

On 2018-07-26 00:01, Matthias Kaehlcke wrote:

On Tue, Jul 24, 2018 at 09:25:16PM +0530, Balakrishna Godavarthi wrote:

quoted

Hi Matthias,

On 2018-07-24 01:24, Matthias Kaehlcke wrote:

quoted

On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:

quoted

+	 * 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.

quoted

quoted

quoted

+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.

quoted

        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.

quoted

quoted

quoted

+	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.