Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
Hi Matthias,
On 2018-08-01 21:46, Matthias Kaehlcke wrote:
On Wed, Aug 01, 2018 at 07:29:29PM +0530, Balakrishna Godavarthi wrote:
Hi Matthias,
On 2018-07-31 21:33, Matthias Kaehlcke wrote:
> On Tue, Jul 31, 2018 at 08:08:40PM +0530, Balakrishna Godavarthi wrote:
> > Hi Matthias,
> >
> > On 2018-07-31 01:37, Matthias Kaehlcke wrote:
> > > On Fri, Jul 27, 2018 at 05:09:02PM +0530, Balakrishna Godavarthi wrote:
> > > > Hi Matthias,
> > > >
> > > > On 2018-07-27 01:21, Matthias Kaehlcke wrote:
> > > > > On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi
wrote:
> > > > > > 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.
> > > > >
> > > > > Thanks for the clarification. Couldn't you assure the device is in a
> > > > > defined state by calling qca_power_shutdown() as one of the first
> > > > > things in qca_wcn3990_init()?
> > > >
> > > > [Bala]: we have reasons behind writing qca_power_setup(true) at the
> > > > start.
> > > >
> > > > 1. the reason to add iteration here, is to handle BT fails
> > > > cases
> > > > either due to communication failure of wcn3900 or due to regulator
> > > > issues.
> > > >before calling qca_setup(), we have our regulator turned
> > > > on and
> > > > in qca_setup i.e. init routine if we added power_shutdown as first
> > > > statement
> > > > before
> > > >communicating with chip then regulator will be off and
> > > > again we
> > > > need to call function to ON regulators.
> > > >so it could be some thing like this
> > > >
> > > >init(){
> > > >
> > > >for () {
> > > > shutdown() // regs are off
> > > > poweron(true) // regs are on.
> > > > if(!start communicating with chip()) {
> > > > break;
> > > > }
> > > >
> > > >}
> > > >}
> > > >as the reason to add the iteration handling is to
> > > > overcome 1% of
> > > > fail cases, so every time when we call it will turn off the regs and
> > > > turn it
> > > > back. which require an turning in off regs and on it back for 99% pass
> > > >
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
Hi Matthias,
On 2018-08-01 21:46, Matthias Kaehlcke wrote:
On Wed, Aug 01, 2018 at 07:29:29PM +0530, Balakrishna Godavarthi wrote:
Hi Matthias,
On 2018-07-31 21:33, Matthias Kaehlcke wrote:
> On Tue, Jul 31, 2018 at 08:08:40PM +0530, Balakrishna Godavarthi wrote:
> > Hi Matthias,
> >
> > On 2018-07-31 01:37, Matthias Kaehlcke wrote:
> > > On Fri, Jul 27, 2018 at 05:09:02PM +0530, Balakrishna Godavarthi wrote:
> > > > Hi Matthias,
> > > >
> > > > On 2018-07-27 01:21, Matthias Kaehlcke wrote:
> > > > > On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi
wrote:
> > > > > > 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.
> > > > >
> > > > > Thanks for the clarification. Couldn't you assure the device is in a
> > > > > defined state by calling qca_power_shutdown() as one of the first
> > > > > things in qca_wcn3990_init()?
> > > >
> > > > [Bala]: we have reasons behind writing qca_power_setup(true) at the
> > > > start.
> > > >
> > > > 1. the reason to add iteration here, is to handle BT fails
> > > > cases
> > > > either due to communication failure of wcn3900 or due to regulator
> > > > issues.
> > > >before calling qca_setup(), we have our regulator turned
> > > > on and
> > > > in qca_setup i.e. init routine if we added power_shutdown as first
> > > > statement
> > > > before
> > > >communicating with chip then regulator will be off and
> > > > again we
> > > > need to call function to ON regulators.
> > > >so it could be some thing like this
> > > >
> > > >init(){
> > > >
> > > >for () {
> > > > shutdown() // regs are off
> > > > poweron(true) // regs are on.
> > > > if(!start communicating with chip()) {
> > > > break;
> > > > }
> > > >
> > > >}
> > > >}
> > > >as the reason to add the iteration handling is to
> > > > overcome 1% of
> > > > fail cases, so every time when we call it will turn off the regs and
> > > > turn it
> > > > back. which require an turning in off regs and on it back for 99% pass
> > > >
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
On Wed, Aug 01, 2018 at 07:29:29PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
>
> On 2018-07-31 21:33, Matthias Kaehlcke wrote:
> > On Tue, Jul 31, 2018 at 08:08:40PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Matthias,
> > >
> > > On 2018-07-31 01:37, Matthias Kaehlcke wrote:
> > > > On Fri, Jul 27, 2018 at 05:09:02PM +0530, Balakrishna Godavarthi wrote:
> > > > > Hi Matthias,
> > > > >
> > > > > On 2018-07-27 01:21, Matthias Kaehlcke wrote:
> > > > > > On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi
> > > > > > wrote:
> > > > > > > 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.
> > > > > >
> > > > > > Thanks for the clarification. Couldn't you assure the device is in a
> > > > > > defined state by calling qca_power_shutdown() as one of the first
> > > > > > things in qca_wcn3990_init()?
> > > > >
> > > > > [Bala]: we have reasons behind writing qca_power_setup(true) at the
> > > > > start.
> > > > >
> > > > > 1. the reason to add iteration here, is to handle BT fails
> > > > > cases
> > > > > either due to communication failure of wcn3900 or due to regulator
> > > > > issues.
> > > > >before calling qca_setup(), we have our regulator turned
> > > > > on and
> > > > > in qca_setup i.e. init routine if we added power_shutdown as first
> > > > > statement
> > > > > before
> > > > >communicating with chip then regulator will be off and
> > > > > again we
> > > > > need to call function to ON regulators.
> > > > >so it could be some thing like this
> > > > >
> > > > >init(){
> > > > >
> > > > >for () {
> > > > > shutdown() // regs are off
> > > > > poweron(true) // regs are on.
> > > > > if(!start communicating with chip()) {
> > > > > break;
> > > > > }
> > > > >
> > > > >
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
On Wed, Aug 01, 2018 at 07:29:29PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
>
> On 2018-07-31 21:33, Matthias Kaehlcke wrote:
> > On Tue, Jul 31, 2018 at 08:08:40PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Matthias,
> > >
> > > On 2018-07-31 01:37, Matthias Kaehlcke wrote:
> > > > On Fri, Jul 27, 2018 at 05:09:02PM +0530, Balakrishna Godavarthi wrote:
> > > > > Hi Matthias,
> > > > >
> > > > > On 2018-07-27 01:21, Matthias Kaehlcke wrote:
> > > > > > On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi
> > > > > > wrote:
> > > > > > > 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.
> > > > > >
> > > > > > Thanks for the clarification. Couldn't you assure the device is in a
> > > > > > defined state by calling qca_power_shutdown() as one of the first
> > > > > > things in qca_wcn3990_init()?
> > > > >
> > > > > [Bala]: we have reasons behind writing qca_power_setup(true) at the
> > > > > start.
> > > > >
> > > > > 1. the reason to add iteration here, is to handle BT fails
> > > > > cases
> > > > > either due to communication failure of wcn3900 or due to regulator
> > > > > issues.
> > > > >before calling qca_setup(), we have our regulator turned
> > > > > on and
> > > > > in qca_setup i.e. init routine if we added power_shutdown as first
> > > > > statement
> > > > > before
> > > > >communicating with chip then regulator will be off and
> > > > > again we
> > > > > need to call function to ON regulators.
> > > > >so it could be some thing like this
> > > > >
> > > > >init(){
> > > > >
> > > > >for () {
> > > > > shutdown() // regs are off
> > > > > poweron(true) // regs are on.
> > > > > if(!start communicating with chip()) {
> > > > > break;
> > > > > }
> > > > >
> > > > >
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
Hi Matthias,
On 2018-07-31 21:33, Matthias Kaehlcke wrote:
On Tue, Jul 31, 2018 at 08:08:40PM +0530, Balakrishna Godavarthi wrote:
Hi Matthias,
On 2018-07-31 01:37, Matthias Kaehlcke wrote:
> On Fri, Jul 27, 2018 at 05:09:02PM +0530, Balakrishna Godavarthi wrote:
> > Hi Matthias,
> >
> > On 2018-07-27 01:21, Matthias Kaehlcke wrote:
> > > On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
> > > > 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.
> > >
> > > Thanks for the clarification. Couldn't you assure the device is in a
> > > defined state by calling qca_power_shutdown() as one of the first
> > > things in qca_wcn3990_init()?
> >
> > [Bala]: we have reasons behind writing qca_power_setup(true) at the
> > start.
> >
> > 1. the reason to add iteration here, is to handle BT fails
> > cases
> > either due to communication failure of wcn3900 or due to regulator
> > issues.
> >before calling qca_setup(), we have our regulator turned
> > on and
> > in qca_setup i.e. init routine if we added power_shutdown as first
> > statement
> > before
> >communicating with chip then regulator will be off and
> > again we
> > need to call function to ON regulators.
> >so it could be some thing like this
> >
> >init(){
> >
> >for () {
> > shutdown() // regs are off
> > poweron(true) // regs are on.
> > if(!start communicating with chip()) {
> > break;
> > }
> >
> >}
> >}
> >as the reason to add the iteration handling is to
> > overcome 1% of
> > fail cases, so every time when we call it will turn off the regs and
> > turn it
> > back. which require an turning in off regs and on it back for 99% pass
> > cases.
>
> But would turning off the regs really add a significant delay here?
> The setup is already really slow, with a 100ms delay in the
> loop (still wonder if booting the chip without loading firmware really
> takes that long) and later the firmware is loaded.
>
[Bala]: By default we will have an firmware loaded in ROM of wcn3990,
100 ms
delay will help wcn3990 boot up with default firmware.
Ok, thanks.
Once it is booted up with default firmware on ROM, we will
download
the firmware from the firmware files,
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
Hi Matthias,
On 2018-07-31 21:33, Matthias Kaehlcke wrote:
On Tue, Jul 31, 2018 at 08:08:40PM +0530, Balakrishna Godavarthi wrote:
Hi Matthias,
On 2018-07-31 01:37, Matthias Kaehlcke wrote:
> On Fri, Jul 27, 2018 at 05:09:02PM +0530, Balakrishna Godavarthi wrote:
> > Hi Matthias,
> >
> > On 2018-07-27 01:21, Matthias Kaehlcke wrote:
> > > On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
> > > > 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.
> > >
> > > Thanks for the clarification. Couldn't you assure the device is in a
> > > defined state by calling qca_power_shutdown() as one of the first
> > > things in qca_wcn3990_init()?
> >
> > [Bala]: we have reasons behind writing qca_power_setup(true) at the
> > start.
> >
> > 1. the reason to add iteration here, is to handle BT fails
> > cases
> > either due to communication failure of wcn3900 or due to regulator
> > issues.
> >before calling qca_setup(), we have our regulator turned
> > on and
> > in qca_setup i.e. init routine if we added power_shutdown as first
> > statement
> > before
> >communicating with chip then regulator will be off and
> > again we
> > need to call function to ON regulators.
> >so it could be some thing like this
> >
> >init(){
> >
> >for () {
> > shutdown() // regs are off
> > poweron(true) // regs are on.
> > if(!start communicating with chip()) {
> > break;
> > }
> >
> >}
> >}
> >as the reason to add the iteration handling is to
> > overcome 1% of
> > fail cases, so every time when we call it will turn off the regs and
> > turn it
> > back. which require an turning in off regs and on it back for 99% pass
> > cases.
>
> But would turning off the regs really add a significant delay here?
> The setup is already really slow, with a 100ms delay in the
> loop (still wonder if booting the chip without loading firmware really
> takes that long) and later the firmware is loaded.
>
[Bala]: By default we will have an firmware loaded in ROM of wcn3990,
100 ms
delay will help wcn3990 boot up with default firmware.
Ok, thanks.
Once it is booted up with default firmware on ROM, we will
download
the firmware from the firmware files,
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
On Tue, Jul 31, 2018 at 08:08:40PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
>
> On 2018-07-31 01:37, Matthias Kaehlcke wrote:
> > On Fri, Jul 27, 2018 at 05:09:02PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Matthias,
> > >
> > > On 2018-07-27 01:21, Matthias Kaehlcke wrote:
> > > > On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
> > > > > 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.
> > > >
> > > > Thanks for the clarification. Couldn't you assure the device is in a
> > > > defined state by calling qca_power_shutdown() as one of the first
> > > > things in qca_wcn3990_init()?
> > >
> > > [Bala]: we have reasons behind writing qca_power_setup(true) at the
> > > start.
> > >
> > > 1. the reason to add iteration here, is to handle BT fails
> > > cases
> > > either due to communication failure of wcn3900 or due to regulator
> > > issues.
> > >before calling qca_setup(), we have our regulator turned
> > > on and
> > > in qca_setup i.e. init routine if we added power_shutdown as first
> > > statement
> > > before
> > >communicating with chip then regulator will be off and
> > > again we
> > > need to call function to ON regulators.
> > >so it could be some thing like this
> > >
> > >init(){
> > >
> > >for () {
> > > shutdown() // regs are off
> > > poweron(true) // regs are on.
> > > if(!start communicating with chip()) {
> > > break;
> > > }
> > >
> > >}
> > >}
> > >as the reason to add the iteration handling is to
> > > overcome 1% of
> > > fail cases, so every time when we call it will turn off the regs and
> > > turn it
> > > back. which require an turning in off regs and on it back for 99% pass
> > > cases.
> >
> > But would turning off the regs really add a significant delay here?
> > The setup is already really slow, with a 100ms delay in the
> > loop (still wonder if booting the chip without loading firmware really
> > takes that long) and later the firmware is
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
On Tue, Jul 31, 2018 at 08:08:40PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
>
> On 2018-07-31 01:37, Matthias Kaehlcke wrote:
> > On Fri, Jul 27, 2018 at 05:09:02PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Matthias,
> > >
> > > On 2018-07-27 01:21, Matthias Kaehlcke wrote:
> > > > On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
> > > > > 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.
> > > >
> > > > Thanks for the clarification. Couldn't you assure the device is in a
> > > > defined state by calling qca_power_shutdown() as one of the first
> > > > things in qca_wcn3990_init()?
> > >
> > > [Bala]: we have reasons behind writing qca_power_setup(true) at the
> > > start.
> > >
> > > 1. the reason to add iteration here, is to handle BT fails
> > > cases
> > > either due to communication failure of wcn3900 or due to regulator
> > > issues.
> > >before calling qca_setup(), we have our regulator turned
> > > on and
> > > in qca_setup i.e. init routine if we added power_shutdown as first
> > > statement
> > > before
> > >communicating with chip then regulator will be off and
> > > again we
> > > need to call function to ON regulators.
> > >so it could be some thing like this
> > >
> > >init(){
> > >
> > >for () {
> > > shutdown() // regs are off
> > > poweron(true) // regs are on.
> > > if(!start communicating with chip()) {
> > > break;
> > > }
> > >
> > >}
> > >}
> > >as the reason to add the iteration handling is to
> > > overcome 1% of
> > > fail cases, so every time when we call it will turn off the regs and
> > > turn it
> > > back. which require an turning in off regs and on it back for 99% pass
> > > cases.
> >
> > But would turning off the regs really add a significant delay here?
> > The setup is already really slow, with a 100ms delay in the
> > loop (still wonder if booting the chip without loading firmware really
> > takes that long) and later the firmware is
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
Hi Matthias,
On 2018-07-31 01:37, Matthias Kaehlcke wrote:
On Fri, Jul 27, 2018 at 05:09:02PM +0530, Balakrishna Godavarthi wrote:
Hi Matthias,
On 2018-07-27 01:21, Matthias Kaehlcke wrote:
> On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
> > 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.
>
> Thanks for the clarification. Couldn't you assure the device is in a
> defined state by calling qca_power_shutdown() as one of the first
> things in qca_wcn3990_init()?
[Bala]: we have reasons behind writing qca_power_setup(true) at the
start.
1. the reason to add iteration here, is to handle BT fails
cases
either due to communication failure of wcn3900 or due to regulator
issues.
before calling qca_setup(), we have our regulator turned on
and
in qca_setup i.e. init routine if we added power_shutdown as first
statement
before
communicating with chip then regulator will be off and
again we
need to call function to ON regulators.
so it could be some thing like this
init(){
for () {
shutdown() // regs are off
poweron(true) // regs are on.
if(!start communicating with chip()) {
break;
}
}
}
as the reason to add the iteration handling is to overcome
1% of
fail cases, so every time when we call it will turn off the regs and
turn it
back. which require an turning in off regs and on it back for 99% pass
cases.
But would turning off the regs really add a significant delay here?
The setup is already really slow, with a 100ms delay in the
loop (still wonder if booting the chip without loading firmware really
takes that long) and later the firmware is loaded.
[Bala]: By default we will have an firmware loaded in ROM of wcn3990,
100 ms delay will help wcn3990 boot up with default firmware.
Once it is booted up with default firmware on ROM, we will
download the firmware from the firmware files, these firmware files
contains bug fixes of wcn3990. Once the firmware files are
loaded we will send the reset command to wcn3990.
wcn3990 will start working with latest firmware which is loaded.
If the chip needs to be in a defined state we should make sure to put
in into that state, unless there is a significant overhead wrt 'try
first and only reset in case of failure'. As a nice side effect the
code would be cleaner and we probably could get rid of the loop
completely, since it's supposed to address the case where the chip
wasn't properly reset on a
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
Hi Matthias,
On 2018-07-31 01:37, Matthias Kaehlcke wrote:
On Fri, Jul 27, 2018 at 05:09:02PM +0530, Balakrishna Godavarthi wrote:
Hi Matthias,
On 2018-07-27 01:21, Matthias Kaehlcke wrote:
> On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
> > 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.
>
> Thanks for the clarification. Couldn't you assure the device is in a
> defined state by calling qca_power_shutdown() as one of the first
> things in qca_wcn3990_init()?
[Bala]: we have reasons behind writing qca_power_setup(true) at the
start.
1. the reason to add iteration here, is to handle BT fails
cases
either due to communication failure of wcn3900 or due to regulator
issues.
before calling qca_setup(), we have our regulator turned on
and
in qca_setup i.e. init routine if we added power_shutdown as first
statement
before
communicating with chip then regulator will be off and
again we
need to call function to ON regulators.
so it could be some thing like this
init(){
for () {
shutdown() // regs are off
poweron(true) // regs are on.
if(!start communicating with chip()) {
break;
}
}
}
as the reason to add the iteration handling is to overcome
1% of
fail cases, so every time when we call it will turn off the regs and
turn it
back. which require an turning in off regs and on it back for 99% pass
cases.
But would turning off the regs really add a significant delay here?
The setup is already really slow, with a 100ms delay in the
loop (still wonder if booting the chip without loading firmware really
takes that long) and later the firmware is loaded.
[Bala]: By default we will have an firmware loaded in ROM of wcn3990,
100 ms delay will help wcn3990 boot up with default firmware.
Once it is booted up with default firmware on ROM, we will
download the firmware from the firmware files, these firmware files
contains bug fixes of wcn3990. Once the firmware files are
loaded we will send the reset command to wcn3990.
wcn3990 will start working with latest firmware which is loaded.
If the chip needs to be in a defined state we should make sure to put
in into that state, unless there is a significant overhead wrt 'try
first and only reset in case of failure'. As a nice side effect the
code would be cleaner and we probably could get rid of the loop
completely, since it's supposed to address the case where the chip
wasn't properly reset on a
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
On Fri, Jul 27, 2018 at 05:09:02PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
>
> On 2018-07-27 01:21, Matthias Kaehlcke wrote:
> > On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
> > > 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.
> >
> > Thanks for the clarification. Couldn't you assure the device is in a
> > defined state by calling qca_power_shutdown() as one of the first
> > things in qca_wcn3990_init()?
>
> [Bala]: we have reasons behind writing qca_power_setup(true) at the start.
>
> 1. the reason to add iteration here, is to handle BT fails cases
> either due to communication failure of wcn3900 or due to regulator issues.
>before calling qca_setup(), we have our regulator turned on and
> in qca_setup i.e. init routine if we added power_shutdown as first statement
> before
>communicating with chip then regulator will be off and again we
> need to call function to ON regulators.
>so it could be some thing like this
>
>init(){
>
>for () {
> shutdown() // regs are off
> poweron(true) // regs are on.
> if(!start communicating with chip()) {
> break;
> }
>
>}
>}
>as the reason to add the iteration handling is to overcome 1% of
> fail cases, so every time when we call it will turn off the regs and turn it
> back. which require an turning in off regs and on it back for 99% pass
> cases.
But would turning off the regs really add a significant delay here?
The setup is already really slow, with a 100ms delay in the
loop (still wonder if booting the chip without loading firmware really
takes that long) and later the firmware is loaded.
If the chip needs to be in a defined state we should make sure to put
in into that state, unless there is a significant overhead wrt 'try
first and only reset in case of failure'. As a nice side effect the
code would be cleaner and we probably could get rid of the loop
completely, since it's supposed to address the case where the chip
wasn't properly reset on a reboot.
> 2. this is the one of the main reason for adding
> qca_power_setup(true) in the init() function first.
>as we know that power management is so critical for long lasting
> of battery.
>now present implementation is when we off BT from UI i.e. hci0
> down, we
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
On Fri, Jul 27, 2018 at 05:09:02PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
>
> On 2018-07-27 01:21, Matthias Kaehlcke wrote:
> > On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
> > > 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.
> >
> > Thanks for the clarification. Couldn't you assure the device is in a
> > defined state by calling qca_power_shutdown() as one of the first
> > things in qca_wcn3990_init()?
>
> [Bala]: we have reasons behind writing qca_power_setup(true) at the start.
>
> 1. the reason to add iteration here, is to handle BT fails cases
> either due to communication failure of wcn3900 or due to regulator issues.
>before calling qca_setup(), we have our regulator turned on and
> in qca_setup i.e. init routine if we added power_shutdown as first statement
> before
>communicating with chip then regulator will be off and again we
> need to call function to ON regulators.
>so it could be some thing like this
>
>init(){
>
>for () {
> shutdown() // regs are off
> poweron(true) // regs are on.
> if(!start communicating with chip()) {
> break;
> }
>
>}
>}
>as the reason to add the iteration handling is to overcome 1% of
> fail cases, so every time when we call it will turn off the regs and turn it
> back. which require an turning in off regs and on it back for 99% pass
> cases.
But would turning off the regs really add a significant delay here?
The setup is already really slow, with a 100ms delay in the
loop (still wonder if booting the chip without loading firmware really
takes that long) and later the firmware is loaded.
If the chip needs to be in a defined state we should make sure to put
in into that state, unless there is a significant overhead wrt 'try
first and only reset in case of failure'. As a nice side effect the
code would be cleaner and we probably could get rid of the loop
completely, since it's supposed to address the case where the chip
wasn't properly reset on a reboot.
> 2. this is the one of the main reason for adding
> qca_power_setup(true) in the init() function first.
>as we know that power management is so critical for long lasting
> of battery.
>now present implementation is when we off BT from UI i.e. hci0
> down, we
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
Hi Matthias,
On 2018-07-27 01:21, Matthias Kaehlcke wrote:
On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
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.
Thanks for the clarification. Couldn't you assure the device is in a
defined state by calling qca_power_shutdown() as one of the first
things in qca_wcn3990_init()?
[Bala]: we have reasons behind writing qca_power_setup(true) at the
start.
1. the reason to add iteration here, is to handle BT fails cases
either due to communication failure of wcn3900 or due to regulator
issues.
before calling qca_setup(), we have our regulator turned on
and in qca_setup i.e. init routine if we added power_shutdown as first
statement before
communicating with chip then regulator will be off and again
we need to call function to ON regulators.
so it could be some thing like this
init(){
for () {
shutdown() // regs are off
poweron(true) // regs are on.
if(!start communicating with chip()) {
break;
}
}
}
as the reason to add the iteration handling is to overcome 1%
of fail cases, so every time when we call it will turn off the regs and
turn it back. which require an turning in off regs and on it back for
99% pass
cases.
2. this is the one of the main reason for adding
qca_power_setup(true) in the init() function first.
as we know that power management is so critical for long
lasting of battery.
now present implementation is when we off BT from UI i.e.
hci0 down, we put BT into an suspend or low power mode, as soon as we
turn ON the BT back from UI we make hci0 up.
the above is putting device into suspend state and bring it
back where the regulator are still on state. so we will have leakage
currents which can be minimal or may be in few mA.
to over come the above case, we want to do an cold on/off for
BT chip wcn3990. i.e. when bt is off from UI, we will off the regulators
and turn on it again once the BT is ON from UI.
every time we disable i.e. off BT from UI we will call
hdev->shutdown() i.e. completely powering off the chip.
so it require an reprogram again, when we turn ON BT from UI.
it will call qca_setup()--> init().. so here actually qca_power_on(true)
will turn on the chip and dump the fw files into it.
so that is also a reason behind to write power on first.
the above feature is under testing state, will post a patch
series once the driver code merged to bt-next.
Some more comments on the functions, for if the retry loop is kept:
+static int qca_wcn3990_init(struct hci_uart *hu, u32
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
Hi Matthias,
On 2018-07-27 01:21, Matthias Kaehlcke wrote:
On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
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.
Thanks for the clarification. Couldn't you assure the device is in a
defined state by calling qca_power_shutdown() as one of the first
things in qca_wcn3990_init()?
[Bala]: we have reasons behind writing qca_power_setup(true) at the
start.
1. the reason to add iteration here, is to handle BT fails cases
either due to communication failure of wcn3900 or due to regulator
issues.
before calling qca_setup(), we have our regulator turned on
and in qca_setup i.e. init routine if we added power_shutdown as first
statement before
communicating with chip then regulator will be off and again
we need to call function to ON regulators.
so it could be some thing like this
init(){
for () {
shutdown() // regs are off
poweron(true) // regs are on.
if(!start communicating with chip()) {
break;
}
}
}
as the reason to add the iteration handling is to overcome 1%
of fail cases, so every time when we call it will turn off the regs and
turn it back. which require an turning in off regs and on it back for
99% pass
cases.
2. this is the one of the main reason for adding
qca_power_setup(true) in the init() function first.
as we know that power management is so critical for long
lasting of battery.
now present implementation is when we off BT from UI i.e.
hci0 down, we put BT into an suspend or low power mode, as soon as we
turn ON the BT back from UI we make hci0 up.
the above is putting device into suspend state and bring it
back where the regulator are still on state. so we will have leakage
currents which can be minimal or may be in few mA.
to over come the above case, we want to do an cold on/off for
BT chip wcn3990. i.e. when bt is off from UI, we will off the regulators
and turn on it again once the BT is ON from UI.
every time we disable i.e. off BT from UI we will call
hdev->shutdown() i.e. completely powering off the chip.
so it require an reprogram again, when we turn ON BT from UI.
it will call qca_setup()--> init().. so here actually qca_power_on(true)
will turn on the chip and dump the fw files into it.
so that is also a reason behind to write power on first.
the above feature is under testing state, will post a patch
series once the driver code merged to bt-next.
Some more comments on the functions, for if the retry loop is kept:
+static int qca_wcn3990_init(struct hci_uart *hu, u32
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
> 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.
Thanks for the clarification. Couldn't you assure the device is in a
defined state by calling qca_power_shutdown() as one of the first
things in qca_wcn3990_init()?
Some more comments on the functions, for if the retry loop is kept:
> +static int qca_wcn3990_init(struct hci_uart *hu, u32 *soc_ver)
> +{
> + struct hci_dev *hdev = hu->hdev;
> + int i, ret = 1;
> +
> + /* WCN3990 is a discrete Bluetooth chip connected to APPS processor.
> + * 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++) {
> + /* This helper will turn ON chip if it is powered off.
> + * if the chip is already powered ON, function call will
> + * return zero.
> + */
> + ret = qca_power_setup(hu, true);
> + if (ret)
> + goto regs_off;
A failure here is not caused by a communication problem, so this
should probably be a 'return' instead of a 'goto'.
> +
> + /* Forcefully enable wcn3990 to enter in to boot mode. */
> + host_set_baudrate(hu, 2400);
> + ret = qca_send_vendor_pulse(hdev, QCA_WCN3990_FORCE_BOOT_PULSE);
> + if (ret)
> + goto regs_off;
> +
> + qca_set_speed(hu, QCA_INIT_SPEED);
> + ret = qca_send_vendor_pulse(hdev, QCA_WCN3990_POWERON_PULSE);
> + if (ret)
> + goto regs_off;
> +
> + /* Wait for 100 ms for SoC to boot */
> + msleep(100);
> +
> + /* Now the device is in ready state to communicate with host.
> + * To sync HOST with device we need to reopen port.
> + * Without this, we will have RTS and CTS synchronization
> + * issues.
> + */
> + serdev_device_close(hu->serdev);
> + ret = serdev_device_open(hu->serdev);
> + if (ret) {
> + bt_dev_err(hu->hdev, "failed to open port");
> + break;
return ret;
> + }
> +
> + hci_uart_set_flow_control(hu, false);
> + ret = qca_read_soc_version(hdev, soc_ver);
> + if (ret < 0 || soc_ver == 0)
> + bt_dev_err(hdev, "Failed to get version:%d", ret);
nit: add a space between ':' and '%d'
> +
> + if (!ret)
> +
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
> 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.
Thanks for the clarification. Couldn't you assure the device is in a
defined state by calling qca_power_shutdown() as one of the first
things in qca_wcn3990_init()?
Some more comments on the functions, for if the retry loop is kept:
> +static int qca_wcn3990_init(struct hci_uart *hu, u32 *soc_ver)
> +{
> + struct hci_dev *hdev = hu->hdev;
> + int i, ret = 1;
> +
> + /* WCN3990 is a discrete Bluetooth chip connected to APPS processor.
> + * 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++) {
> + /* This helper will turn ON chip if it is powered off.
> + * if the chip is already powered ON, function call will
> + * return zero.
> + */
> + ret = qca_power_setup(hu, true);
> + if (ret)
> + goto regs_off;
A failure here is not caused by a communication problem, so this
should probably be a 'return' instead of a 'goto'.
> +
> + /* Forcefully enable wcn3990 to enter in to boot mode. */
> + host_set_baudrate(hu, 2400);
> + ret = qca_send_vendor_pulse(hdev, QCA_WCN3990_FORCE_BOOT_PULSE);
> + if (ret)
> + goto regs_off;
> +
> + qca_set_speed(hu, QCA_INIT_SPEED);
> + ret = qca_send_vendor_pulse(hdev, QCA_WCN3990_POWERON_PULSE);
> + if (ret)
> + goto regs_off;
> +
> + /* Wait for 100 ms for SoC to boot */
> + msleep(100);
> +
> + /* Now the device is in ready state to communicate with host.
> + * To sync HOST with device we need to reopen port.
> + * Without this, we will have RTS and CTS synchronization
> + * issues.
> + */
> + serdev_device_close(hu->serdev);
> + ret = serdev_device_open(hu->serdev);
> + if (ret) {
> + bt_dev_err(hu->hdev, "failed to open port");
> + break;
return ret;
> + }
> +
> + hci_uart_set_flow_control(hu, false);
> + ret = qca_read_soc_version(hdev, soc_ver);
> + if (ret < 0 || soc_ver == 0)
> + bt_dev_err(hdev, "Failed to get version:%d", ret);
nit: add a space between ':' and '%d'
> +
> + if (!ret)
> +
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:
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
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:
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
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
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.
> > > +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.
> > > + if (device_property_read_bool(dev, prop_name))
> > > + device_property_read_u32(dev, prop_name, >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, >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", );
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
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
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.
> > > +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.
> > > + if (device_property_read_bool(dev, prop_name))
> > > + device_property_read_u32(dev, prop_name, >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, >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", );
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
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
Hi Matthias,
On 2018-07-24 01:24, Matthias Kaehlcke wrote:
On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:
Add support to set voltage/current of various regulators
to power up/down Bluetooth chip wcn3990.
Signed-off-by: Balakrishna Godavarthi
---
changes in v10:
* added support to read regulator currents from dts.
I commented on this below
* added support to try to connect with chip if it fails to respond
to initial commands
* updated review comments.
changes in v9:
* moved flow control to vendor and set_baudarte functions.
* removed parent regs.
changes in v8:
* closing qca buffer, if qca_power_setup fails
* chnaged ibs start timer function call location.
* updated review comments.
changes in v7:
* addressed review comments.
changes in v6:
* Hooked up qca_power to qca_serdev.
* renamed all the naming inconsistency functions with qca_*
* leveraged common code of ROME for wcn3990.
* created wrapper functions for re-usable blocks.
* updated function of _*regulator_enable and _*regualtor_disable.
* removed redundant comments and functions.
* addressed review comments.
Changes in v5:
* updated regulator vddpa min_uV to 1304000.
* addressed review comments.
Changes in v4:
* Segregated the changes of btqca from hci_qca
* rebased all changes on top of bluetooth-next.
* addressed review comments.
---
drivers/bluetooth/btqca.h | 4 +
drivers/bluetooth/hci_qca.c | 481
2 files changed, 439 insertions(+), 46 deletions(-)
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index a9c2779f3e07..9e2bbcf5c002 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -37,6 +37,10 @@
#define EDL_TAG_ID_HCI (17)
#define EDL_TAG_ID_DEEP_SLEEP (27)
+#define QCA_WCN3990_POWERON_PULSE 0xFC
+#define QCA_WCN3990_POWEROFF_PULSE 0xC0
+#define QCA_WCN3990_FORCE_BOOT_PULSE 0xC0
This is the same value as QCA_WCN3990_POWEROFF_PULSE. From the usage
it seems it's really just a power off pulse, so let's stick to this
name, instead of having two names for the same thing.
[Bala] : will update.
+static int qca_send_vendor_pulse(struct hci_dev *hdev, u8 cmd)
My understanding from earlier discussion is that these pulses are
limited to power on/off. If that is correct this should probably be
called qca_send_power_pulse().
[Bala]: will update the function name to qca_send_power_pulse().
+{
+ struct hci_uart *hu = hci_get_drvdata(hdev);
+ struct qca_data *qca = hu->priv;
+ struct sk_buff *skb;
+
+ /* These vendor pulses are single byte command which are sent
+* at required baudrate to WCN3990. on WCN3990, we have an external
s/on/On/
[Bala]: will update.
+ * circuit at Tx pin which decodes the pulse sent at specific
baudrate.
+ * For example, as WCN3990 supports RF COEX frequency for both
Wi-Fi/BT
+* and also, we use the same power inputs to turn ON and OFF for
nit: not sure how much value is added by (sometimes) using upper case
for certain things (ON, OFF, COLD, HOST, ...).
[Bala] : will use lower case.
+* Wi-Fi/BT. Powering up the power sources will not enable BT, until
+* we send a POWER ON pulse at 115200. This algorithm will help to
115200 what? bps I guess.
[Bala] : will add bps.
+static int qca_wcn3990_init(struct hci_uart *hu, u32 *soc_ver)
+{
+ struct hci_dev *hdev = hu->hdev;
+ int i, ret = 1;
Initialization not necessary, more details below.
+
+ /* WCN3990 is a discrete Bluetooth chip connected to APPS processor.
APPS is a Qualcomm specific term, and some QCA docs also call it
APSS. Just say 'SoC' which is universally understood.
[Bala]: will update to SoC.
+* 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.
Common practice is to start loops with index 0.
[Bala] : will init i=0
The check for ret is not needed. All jumps to 'regs_off' are done
when an error is detected. The loop is left when 'ret == 0' at the
bottom.
[Bala]: will update.
+ /* This helper will turn ON chip if it is powered off.
+* if the chip is already powered ON, function call will
+* return zero.
+*/
Comments are great when they add value, IMO this one doesn't and just
adds distraction. Most readers will assume that after
qca_power_setup(hu, true) the chip is powered on, regardless of the
previous power state.
[Bala] : will remove the comment.
+ ret =
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
Hi Matthias,
On 2018-07-24 01:24, Matthias Kaehlcke wrote:
On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:
Add support to set voltage/current of various regulators
to power up/down Bluetooth chip wcn3990.
Signed-off-by: Balakrishna Godavarthi
---
changes in v10:
* added support to read regulator currents from dts.
I commented on this below
* added support to try to connect with chip if it fails to respond
to initial commands
* updated review comments.
changes in v9:
* moved flow control to vendor and set_baudarte functions.
* removed parent regs.
changes in v8:
* closing qca buffer, if qca_power_setup fails
* chnaged ibs start timer function call location.
* updated review comments.
changes in v7:
* addressed review comments.
changes in v6:
* Hooked up qca_power to qca_serdev.
* renamed all the naming inconsistency functions with qca_*
* leveraged common code of ROME for wcn3990.
* created wrapper functions for re-usable blocks.
* updated function of _*regulator_enable and _*regualtor_disable.
* removed redundant comments and functions.
* addressed review comments.
Changes in v5:
* updated regulator vddpa min_uV to 1304000.
* addressed review comments.
Changes in v4:
* Segregated the changes of btqca from hci_qca
* rebased all changes on top of bluetooth-next.
* addressed review comments.
---
drivers/bluetooth/btqca.h | 4 +
drivers/bluetooth/hci_qca.c | 481
2 files changed, 439 insertions(+), 46 deletions(-)
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index a9c2779f3e07..9e2bbcf5c002 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -37,6 +37,10 @@
#define EDL_TAG_ID_HCI (17)
#define EDL_TAG_ID_DEEP_SLEEP (27)
+#define QCA_WCN3990_POWERON_PULSE 0xFC
+#define QCA_WCN3990_POWEROFF_PULSE 0xC0
+#define QCA_WCN3990_FORCE_BOOT_PULSE 0xC0
This is the same value as QCA_WCN3990_POWEROFF_PULSE. From the usage
it seems it's really just a power off pulse, so let's stick to this
name, instead of having two names for the same thing.
[Bala] : will update.
+static int qca_send_vendor_pulse(struct hci_dev *hdev, u8 cmd)
My understanding from earlier discussion is that these pulses are
limited to power on/off. If that is correct this should probably be
called qca_send_power_pulse().
[Bala]: will update the function name to qca_send_power_pulse().
+{
+ struct hci_uart *hu = hci_get_drvdata(hdev);
+ struct qca_data *qca = hu->priv;
+ struct sk_buff *skb;
+
+ /* These vendor pulses are single byte command which are sent
+* at required baudrate to WCN3990. on WCN3990, we have an external
s/on/On/
[Bala]: will update.
+ * circuit at Tx pin which decodes the pulse sent at specific
baudrate.
+ * For example, as WCN3990 supports RF COEX frequency for both
Wi-Fi/BT
+* and also, we use the same power inputs to turn ON and OFF for
nit: not sure how much value is added by (sometimes) using upper case
for certain things (ON, OFF, COLD, HOST, ...).
[Bala] : will use lower case.
+* Wi-Fi/BT. Powering up the power sources will not enable BT, until
+* we send a POWER ON pulse at 115200. This algorithm will help to
115200 what? bps I guess.
[Bala] : will add bps.
+static int qca_wcn3990_init(struct hci_uart *hu, u32 *soc_ver)
+{
+ struct hci_dev *hdev = hu->hdev;
+ int i, ret = 1;
Initialization not necessary, more details below.
+
+ /* WCN3990 is a discrete Bluetooth chip connected to APPS processor.
APPS is a Qualcomm specific term, and some QCA docs also call it
APSS. Just say 'SoC' which is universally understood.
[Bala]: will update to SoC.
+* 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.
Common practice is to start loops with index 0.
[Bala] : will init i=0
The check for ret is not needed. All jumps to 'regs_off' are done
when an error is detected. The loop is left when 'ret == 0' at the
bottom.
[Bala]: will update.
+ /* This helper will turn ON chip if it is powered off.
+* if the chip is already powered ON, function call will
+* return zero.
+*/
Comments are great when they add value, IMO this one doesn't and just
adds distraction. Most readers will assume that after
qca_power_setup(hu, true) the chip is powered on, regardless of the
previous power state.
[Bala] : will remove the comment.
+ ret =
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:
> Add support to set voltage/current of various regulators
> to power up/down Bluetooth chip wcn3990.
>
> Signed-off-by: Balakrishna Godavarthi
> ---
> changes in v10:
> * added support to read regulator currents from dts.
I commented on this below
> * added support to try to connect with chip if it fails to respond to
> initial commands
> * updated review comments.
>
> changes in v9:
> * moved flow control to vendor and set_baudarte functions.
> * removed parent regs.
>
> changes in v8:
> * closing qca buffer, if qca_power_setup fails
> * chnaged ibs start timer function call location.
> * updated review comments.
>
> changes in v7:
> * addressed review comments.
>
> changes in v6:
> * Hooked up qca_power to qca_serdev.
> * renamed all the naming inconsistency functions with qca_*
> * leveraged common code of ROME for wcn3990.
> * created wrapper functions for re-usable blocks.
> * updated function of _*regulator_enable and _*regualtor_disable.
> * removed redundant comments and functions.
> * addressed review comments.
>
> Changes in v5:
> * updated regulator vddpa min_uV to 1304000.
> * addressed review comments.
>
> Changes in v4:
> * Segregated the changes of btqca from hci_qca
> * rebased all changes on top of bluetooth-next.
> * addressed review comments.
>
> ---
> drivers/bluetooth/btqca.h | 4 +
> drivers/bluetooth/hci_qca.c | 481
> 2 files changed, 439 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index a9c2779f3e07..9e2bbcf5c002 100644
> --- a/drivers/bluetooth/btqca.h
> +++ b/drivers/bluetooth/btqca.h
> @@ -37,6 +37,10 @@
> #define EDL_TAG_ID_HCI (17)
> #define EDL_TAG_ID_DEEP_SLEEP(27)
>
> +#define QCA_WCN3990_POWERON_PULSE0xFC
> +#define QCA_WCN3990_POWEROFF_PULSE 0xC0
> +#define QCA_WCN3990_FORCE_BOOT_PULSE 0xC0
This is the same value as QCA_WCN3990_POWEROFF_PULSE. From the usage
it seems it's really just a power off pulse, so let's stick to this
name, instead of having two names for the same thing.
> +static int qca_send_vendor_pulse(struct hci_dev *hdev, u8 cmd)
My understanding from earlier discussion is that these pulses are
limited to power on/off. If that is correct this should probably be
called qca_send_power_pulse().
> +{
> + struct hci_uart *hu = hci_get_drvdata(hdev);
> + struct qca_data *qca = hu->priv;
> + struct sk_buff *skb;
> +
> + /* These vendor pulses are single byte command which are sent
> + * at required baudrate to WCN3990. on WCN3990, we have an external
s/on/On/
> + * circuit at Tx pin which decodes the pulse sent at specific baudrate.
> + * For example, as WCN3990 supports RF COEX frequency for both Wi-Fi/BT
> + * and also, we use the same power inputs to turn ON and OFF for
nit: not sure how much value is added by (sometimes) using upper case
for certain things (ON, OFF, COLD, HOST, ...).
> + * Wi-Fi/BT. Powering up the power sources will not enable BT, until
> + * we send a POWER ON pulse at 115200. This algorithm will help to
115200 what? bps I guess.
> +static int qca_wcn3990_init(struct hci_uart *hu, u32 *soc_ver)
> +{
> + struct hci_dev *hdev = hu->hdev;
> + int i, ret = 1;
Initialization not necessary, more details below.
> +
> + /* WCN3990 is a discrete Bluetooth chip connected to APPS processor.
APPS is a Qualcomm specific term, and some QCA docs also call it
APSS. Just say 'SoC' which is universally understood.
> + * 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?
Common practice is to start loops with index 0.
The check for ret is not needed. All jumps to 'regs_off' are done
when an error is detected. The loop is left when 'ret == 0' at the
bottom.
> + /* This helper will turn ON chip if it is powered off.
> + * if the chip is already powered ON, function call will
> + * return zero.
> + */
Comments are great when they add value, IMO this one doesn't and just
adds distraction. Most readers will assume that after
qca_power_setup(hu, true) the chip is powered on, regardless of the
previous power state.
> + ret = qca_power_setup(hu, true);
> + if (ret)
> + goto regs_off;
> +
> + /* Forcefully enable wcn3990 to enter in to boot mode. */
nit: Sometimes the comments and logs name the chip wcn3990, others
WCN3990. Personally I don't
Re: [PATCH v10 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990
On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:
> Add support to set voltage/current of various regulators
> to power up/down Bluetooth chip wcn3990.
>
> Signed-off-by: Balakrishna Godavarthi
> ---
> changes in v10:
> * added support to read regulator currents from dts.
I commented on this below
> * added support to try to connect with chip if it fails to respond to
> initial commands
> * updated review comments.
>
> changes in v9:
> * moved flow control to vendor and set_baudarte functions.
> * removed parent regs.
>
> changes in v8:
> * closing qca buffer, if qca_power_setup fails
> * chnaged ibs start timer function call location.
> * updated review comments.
>
> changes in v7:
> * addressed review comments.
>
> changes in v6:
> * Hooked up qca_power to qca_serdev.
> * renamed all the naming inconsistency functions with qca_*
> * leveraged common code of ROME for wcn3990.
> * created wrapper functions for re-usable blocks.
> * updated function of _*regulator_enable and _*regualtor_disable.
> * removed redundant comments and functions.
> * addressed review comments.
>
> Changes in v5:
> * updated regulator vddpa min_uV to 1304000.
> * addressed review comments.
>
> Changes in v4:
> * Segregated the changes of btqca from hci_qca
> * rebased all changes on top of bluetooth-next.
> * addressed review comments.
>
> ---
> drivers/bluetooth/btqca.h | 4 +
> drivers/bluetooth/hci_qca.c | 481
> 2 files changed, 439 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index a9c2779f3e07..9e2bbcf5c002 100644
> --- a/drivers/bluetooth/btqca.h
> +++ b/drivers/bluetooth/btqca.h
> @@ -37,6 +37,10 @@
> #define EDL_TAG_ID_HCI (17)
> #define EDL_TAG_ID_DEEP_SLEEP(27)
>
> +#define QCA_WCN3990_POWERON_PULSE0xFC
> +#define QCA_WCN3990_POWEROFF_PULSE 0xC0
> +#define QCA_WCN3990_FORCE_BOOT_PULSE 0xC0
This is the same value as QCA_WCN3990_POWEROFF_PULSE. From the usage
it seems it's really just a power off pulse, so let's stick to this
name, instead of having two names for the same thing.
> +static int qca_send_vendor_pulse(struct hci_dev *hdev, u8 cmd)
My understanding from earlier discussion is that these pulses are
limited to power on/off. If that is correct this should probably be
called qca_send_power_pulse().
> +{
> + struct hci_uart *hu = hci_get_drvdata(hdev);
> + struct qca_data *qca = hu->priv;
> + struct sk_buff *skb;
> +
> + /* These vendor pulses are single byte command which are sent
> + * at required baudrate to WCN3990. on WCN3990, we have an external
s/on/On/
> + * circuit at Tx pin which decodes the pulse sent at specific baudrate.
> + * For example, as WCN3990 supports RF COEX frequency for both Wi-Fi/BT
> + * and also, we use the same power inputs to turn ON and OFF for
nit: not sure how much value is added by (sometimes) using upper case
for certain things (ON, OFF, COLD, HOST, ...).
> + * Wi-Fi/BT. Powering up the power sources will not enable BT, until
> + * we send a POWER ON pulse at 115200. This algorithm will help to
115200 what? bps I guess.
> +static int qca_wcn3990_init(struct hci_uart *hu, u32 *soc_ver)
> +{
> + struct hci_dev *hdev = hu->hdev;
> + int i, ret = 1;
Initialization not necessary, more details below.
> +
> + /* WCN3990 is a discrete Bluetooth chip connected to APPS processor.
APPS is a Qualcomm specific term, and some QCA docs also call it
APSS. Just say 'SoC' which is universally understood.
> + * 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?
Common practice is to start loops with index 0.
The check for ret is not needed. All jumps to 'regs_off' are done
when an error is detected. The loop is left when 'ret == 0' at the
bottom.
> + /* This helper will turn ON chip if it is powered off.
> + * if the chip is already powered ON, function call will
> + * return zero.
> + */
Comments are great when they add value, IMO this one doesn't and just
adds distraction. Most readers will assume that after
qca_power_setup(hu, true) the chip is powered on, regardless of the
previous power state.
> + ret = qca_power_setup(hu, true);
> + if (ret)
> + goto regs_off;
> +
> + /* Forcefully enable wcn3990 to enter in to boot mode. */
nit: Sometimes the comments and logs name the chip wcn3990, others
WCN3990. Personally I don't
