Hi Marcel,
>=20
> > > +
> > > +static int mrvl_setup(struct hci_uart *hu) {
> > > + struct mrvl_data *mrvl =3D hu->priv;
> > > +
> > > + mrvl_init_fw_data(hu);
> > > + set_bit(HCI_UART_DNLD_FW, &mrvl->flags);
> > > +
> > > + return hci_uart_dnld_fw(hu);
> > > +}
> >
> > So this is clearly the wrong spot. When ->setup is called it is
> > expected that HCI is ready. You are misusing it here.
> >
>=20
> Sure. We will move this to mrvl_open() where HCI is not yet initialized.
We tried moving firmware download to mrvl_open(), but it's not feasible. "h=
u->proto" is not yet initialized at that time. So when the data/ack is rece=
ived during firmware download, we can't have Marvell specific handling. Als=
o, I can see other vendor's (broadcomm, Intel) have done firmware download =
in setup handler.
I will send V8 patch shortly. Please check.
Regards,
Amitkumar
Hi Marcel,
> From: Marcel Holtmann [mailto:[email protected]]
> Sent: Friday, April 22, 2016 6:19 PM
> To: Amitkumar Karwar
> Cc: Linux Bluetooth; LKML; Ganapathi Bhat; Cathy Luo
> Subject: Re: [PATCH v7] Bluetooth: hci_uart: Support firmware download
> for Marvell
>=20
> Hi Amitkumar,
>=20
> >>>> +
> >>>> +static int mrvl_setup(struct hci_uart *hu) {
> >>>> + struct mrvl_data *mrvl =3D hu->priv;
> >>>> +
> >>>> + mrvl_init_fw_data(hu);
> >>>> + set_bit(HCI_UART_DNLD_FW, &mrvl->flags);
> >>>> +
> >>>> + return hci_uart_dnld_fw(hu);
> >>>> +}
> >>>
> >>> So this is clearly the wrong spot. When ->setup is called it is
> >>> expected that HCI is ready. You are misusing it here.
> >>>
> >>
> >> Sure. We will move this to mrvl_open() where HCI is not yet
> initialized.
> >
> > We tried moving firmware download to mrvl_open(), but it's not
> feasible. "hu->proto" is not yet initialized at that time. So when the
> data/ack is received during firmware download, we can't have Marvell
> specific handling. Also, I can see other vendor's (broadcomm, Intel)
> have done firmware download in setup handler.
>=20
> firmware download in ->setup() is fine as long as it uses HCI commands.
> If it does not use HCI commands, then we need to come up with something
> new.
>=20
> The problem here is that for all intense and purposes once ->setup() is
> called, then assumption is that you are in an HCI capable transport and
> it is ready.
>=20
I understand your concern. We are now planning to add new handler called "-=
>prepare" where we can do firmware download. We are working on the change. =
Meanwhile let us know if you have any suggestion.
---------hci_ldisc.c---------
@@ -641,6 +641,10 @@ static int hci_uart_set_proto(struct hci_uart *hu, int=
id)
hu->proto =3D p;
set_bit(HCI_UART_PROTO_READY, &hu->flags);
+ err =3D p->prepare(hu);
+ if (err)
+ return err;
+
err =3D hci_uart_register_dev(hu);
if (err) {
---------------------------
Regards,
Amitkumar
Hi Marcel,
> From: Marcel Holtmann [mailto:[email protected]]
> Sent: Friday, April 22, 2016 6:19 PM
> To: Amitkumar Karwar
> Cc: Linux Bluetooth; LKML; Ganapathi Bhat; Cathy Luo
> Subject: Re: [PATCH v7] Bluetooth: hci_uart: Support firmware download
> for Marvell
>=20
> Hi Amitkumar,
>=20
> >>>> +
> >>>> +static int mrvl_setup(struct hci_uart *hu) {
> >>>> + struct mrvl_data *mrvl =3D hu->priv;
> >>>> +
> >>>> + mrvl_init_fw_data(hu);
> >>>> + set_bit(HCI_UART_DNLD_FW, &mrvl->flags);
> >>>> +
> >>>> + return hci_uart_dnld_fw(hu);
> >>>> +}
> >>>
> >>> So this is clearly the wrong spot. When ->setup is called it is
> >>> expected that HCI is ready. You are misusing it here.
> >>>
> >>
> >> Sure. We will move this to mrvl_open() where HCI is not yet
> initialized.
> >
> > We tried moving firmware download to mrvl_open(), but it's not
> feasible. "hu->proto" is not yet initialized at that time. So when the
> data/ack is received during firmware download, we can't have Marvell
> specific handling. Also, I can see other vendor's (broadcomm, Intel)
> have done firmware download in setup handler.
>=20
> firmware download in ->setup() is fine as long as it uses HCI commands.
> If it does not use HCI commands, then we need to come up with something
> new.
>=20
> The problem here is that for all intense and purposes once ->setup() is
> called, then assumption is that you are in an HCI capable transport and
> it is ready.
>=20
We have maintained a flag(in mrvl->flags) which remains on until firmware d=
ownload gets completed. We drop HCI frames in "->enqueue" if firmware downl=
oad is in progress. I don't see any possible issues if firmware download is=
included in ->setup().
request_firmware() API expects a device pointer (hdev->dev). We won't get v=
alid pointer until HCI is ready.
Also, we are downloading firmware data chunks through normal Tx path(schedu=
ling hu->write_work etc) where hdev comes into picture.
Let me know your thoughts.
Regards,
Amitkumar
Hi Amitkumar,
>>>> +
>>>> +static int mrvl_setup(struct hci_uart *hu) {
>>>> + struct mrvl_data *mrvl = hu->priv;
>>>> +
>>>> + mrvl_init_fw_data(hu);
>>>> + set_bit(HCI_UART_DNLD_FW, &mrvl->flags);
>>>> +
>>>> + return hci_uart_dnld_fw(hu);
>>>> +}
>>>
>>> So this is clearly the wrong spot. When ->setup is called it is
>>> expected that HCI is ready. You are misusing it here.
>>>
>>
>> Sure. We will move this to mrvl_open() where HCI is not yet initialized.
>
> We tried moving firmware download to mrvl_open(), but it's not feasible. "hu->proto" is not yet initialized at that time. So when the data/ack is received during firmware download, we can't have Marvell specific handling. Also, I can see other vendor's (broadcomm, Intel) have done firmware download in setup handler.
firmware download in ->setup() is fine as long as it uses HCI commands. If it does not use HCI commands, then we need to come up with something new.
The problem here is that for all intense and purposes once ->setup() is called, then assumption is that you are in an HCI capable transport and it is ready.
Regards
Marcel