On Tue, 2018-07-31 at 12:32 +0200, Marcel Holtmann wrote:
> Hi Sean,
>
> >>> All suggestions seem reasonable for me in order to make code style
> >>> aligned with the other drivers and code better to read,
> >>> and it looks like no any big problem, so I'll start to work on the next
> >>> version immediately.
> >>
> >> no rush, but if you can get this back to me quickly, we might be still
> >> able to get this driver included.
> >>
> >>> And I also add a few explanations inline about questions about the
> >>> diagnostic packet and how hci->shutdown is being made.
> >>>
> >>> On Mon, 2018-07-30 at 15:40 +0200, Marcel Holtmann wrote:
> >
> >
> > [ ... ]
> >
> >>>> Do we want these as HCI vendor events. Or actually as vendor /
> >>>> diagnostic packets. There is a hci_recv_diag.
> >>>>
> >>>
> >>> These are actually Hci vendor events. not for diagnostic purpose. They
> >>> hdr->evt == 0xe4 are all the part of chip initialization.
> >>
> >> Ok, then leave it as is.
> >>
> >>>
> >>>> So let me ask this a different way, do you have support for LMP / LL
> >>>> tracing in your chips? If yes, then how is that enabled and how does it
> >>>> work? Or any general debug data that can be switched on. That is what we
> >>>> have a diagnostic channel for that is also fed into btmon.
> >>>>
> >>>
> >>> I'm not really sure about them because I didn't see any diagnostic
> >>> packets handling in vendor driver.
> >>
> >> You can see examples for this in btusb.c, hci_bcm.c and bpa10x.c where the
> >> hardware has a side channel. In case of Broadcom, they are LL or LMP trace
> >> packets, but it could be also debug messages or something else. Other
> >> vendors use HCI vendor events for that which is also fine. Just wanted to
> >> know what it is.
> >>
> >> And if your hardware supports LL or LMP traces to be enabled, then
> >> implement hdev->set_diag() callback. You can then enable it via
> >> /sys/kernel/debug/bluetooth/hci0/vendor_diag
> >>
> >
> > Thanks for sharing the information to me.
> >
> > If I get the permission and details about adding support for these debug
> > trace packets, I am willing to add them.
> >
> >>>
> >>>>> +
> >>>>> + /* Each HCI event would go through the core. */
> >>>>> + return hci_recv_frame(hdev, skb);
> >>>>> +}
> >>>>> +
> >
> > [ ... ]
> >
> >>>>> +
> >>>>> +static int mtk_btuart_shutdown(struct hci_dev *hdev)
> >>>>> +{
> >>>>> + struct mtk_btuart_dev *bdev = hci_get_drvdata(hdev);
> >>>>> + struct device *dev = &bdev->serdev->dev;
> >>>>> + u8 param = 0x0;
> >>>>> +
> >>>>> + /* Disable the device. */
> >>>>> + mtk_hci_wmt_sync(hdev, MTK_WMT_FUNC_CTRL, 0x0, sizeof(param),
> >>>>> ¶m);
> >>>>> +
> >>>>> + /* Shutdown the clock and power domain the device requires. */
> >>>>> + clk_disable_unprepare(bdev->clk);
> >>>>> + pm_runtime_put_sync(dev);
> >>>>> + pm_runtime_disable(dev);
> >>>>
> >>>> Don’t these belong into ->close method? And the enable ones into ->open?
> >>>> I really think that ->setup and ->shutdown should just do HCI commands
> >>>> and leave the others to ->open and ->close. Since ->open and ->close are
> >>>> suppose to set up and terminate the transport.
> >>>>
> >>>
> >>> Yes, ->open and ->close are already done just for setup and terminate the
> >>> transport. I've noted the part during earlier version discussion.
> >>>
> >>> Because mt7622 is a SoC integrating Bluetooth hardware inside, these
> >>> operations for clk* and pm clk_* and pm_* you saw here are all for
> >>> controlling clocks and powers Bluetooth circuits requires on the SoC, not
> >>> for the transports.
> >>>
> >>> As for clocks for the transport, they're already being taken care of by
> >>> the serial driver.
> >>
> >> With transport, I meant the Bluetooth transport. So I really thing they
> >> belong into ->open and ->close. Within ->setup and ->shutdown, I would
> >> just expect HCI commands.
> >>
> >
> > At the moment, it's not easy that clk_* and pm_* are moved to ->open and
> > ->close
> >
> > because firmware download would depend on the full cycle of hardware power
> > down and then up.
> >
> > And another advantage is that when users call hdev down and the up, the
> > device can do the real hardware reset, not just the software reset via hci
> > command.
>
> But when you call down/up cycle, then you will go through ->close and ->open.
> So I don’t see the problem here.
>
> With the quirk for always calling ->setup, you get the ->open + ->setup on
> hdev up and ->shutdown + ->close on hdev down.
>
Yes, it seems no any problem. really thanks point out me with patience
So I've moved these clk_* and pm_* operations into ->open and ->close in newer
v7.
> Regards
>
> Marcel
>
