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

[Balakrishna Godavarthi]

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.