Re: [RFC 3/3] Bluetooth: Add AMP initialization

Hi Marcel, Andrei,

quoted

quoted

quoted

quoted

However since we are talking about controller init. Can someone please
first propose a list of how we init AMP vs BR/EDR/LE controllers instead
of just dumping code here. I think it needs some clear strategy on how
to do it.

AMP controller is initialized following way in Code Aurora code:

Common part:

<------8<--------------------------------------------------
|  /* Mandatory initialization */
|
|  /* Reset */
|  if (!test_bit(HCI_QUIRK_NO_RESET, &hdev->quirks)) {
|          set_bit(HCI_RESET, &hdev->flags);
|          hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
|  }

this check for QUIRK_NO_RESET is pointless. That is just there for old
Bluetooth 1.0b controllers. Or broken hardware. Let's assume the AMP
controllers are fine and just issue OP_RESET.

quoted

|  /* Read Local Version */
|  hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
|
|  /* Read HCI Flow Control Mode */
|  hci_send_cmd(hdev, HCI_OP_READ_FLOW_CONTROL_MODE, 0, NULL);
|
|  /* Read Buffer Size (ACL mtu, max pkt, etc.) */
|  hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
|
|  /* Read Data Block Size (ACL mtu, max pkt, etc.) */
|  hci_send_cmd(hdev, HCI_OP_READ_DATA_BLOCK_SIZE, 0, NULL);
<------8<------------------------------------------------------

This is not a generic sequence. We will not be reading flow control or
data block size commands. It will heavily fail on older controllers.

What about following code (basically BR/EDR initialization is the same as
now and only amp_init is new). I use now virtual AMP device so any init is
good, I need Read Version and Read AMP info commands.

static void bredr_init(struct hci_dev *hdev)
{
      struct hci_cp_delete_stored_link_key cp;
      __le16 param;
      __u8 flt_type;

      /* Mandatory initialization */
      /* Reset */
      if (!test_bit(HCI_QUIRK_NO_RESET, &hdev->quirks)) {
              set_bit(HCI_RESET, &hdev->flags);
              hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
      }

      /* Read Local Supported Features */
      hci_send_cmd(hdev, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);

      /* Read Local Version */
      hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);

      /* Read Buffer Size (ACL mtu, max pkt, etc.) */
      hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);

      /* Read BD Address */
      hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);

      /* Read Class of Device */
      hci_send_cmd(hdev, HCI_OP_READ_CLASS_OF_DEV, 0, NULL);

      /* Read Local Name */
      hci_send_cmd(hdev, HCI_OP_READ_LOCAL_NAME, 0, NULL);

      /* Read Voice Setting */
      hci_send_cmd(hdev, HCI_OP_READ_VOICE_SETTING, 0, NULL);

      /* Optional initialization */
      /* Clear Event Filters */
      flt_type = HCI_FLT_CLEAR_ALL;
      hci_send_cmd(hdev, HCI_OP_SET_EVENT_FLT, 1, &flt_type);

      /* Connection accept timeout ~20 secs */
      param = cpu_to_le16(0x7d00);
      hci_send_cmd(hdev, HCI_OP_WRITE_CA_TIMEOUT, 2, &param);

      bacpy(&cp.bdaddr, BDADDR_ANY);
      cp.delete_all = 1;
      hci_send_cmd(hdev, HCI_OP_DELETE_STORED_LINK_KEY, sizeof(cp), &cp);
}

static void amp_init(struct hci_dev *hdev)
{
      /* Mandatory initialization */
      /* Reset */
      hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);

      /* Read Local Version */
      hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);

      /* Read Buffer Size (ACL mtu, max pkt, etc.) */
      hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);

      /* Read AMP Info */
      hci_send_cmd(hdev, HCI_OP_READ_LOCAL_AMP_INFO, 0, NULL);
}

actually I think the READ_AMP_INFO is wrong here. It should be only
executed based on an AMP_Get_Info_Request during a connection setup
procedure.

You can read the AMP info over HCI upon receiving a request by A2MP, but there are reasons to issue the READ_AMP_INFO command in advance.  For example, you need the Controller_Type for responding to the AMP_Discover_Request.  Also you may (tenuously) want the know the Max_PDU_Size in advance for selecting SDU sizes (w.r.t. segmentation).

Certainly READ_AMP_INFO is a command intended to be issued during controller initialization, the majority of the values should not change during the lifetime of the controller, and only once that command has been issued will the Controller commence sending AMP_Status_Change_Events to communicate changes in status.  At a minimum READ_AMP_INFO must be issued for each AMP_Discover_Request (and AMP_Get_Info_Request) received; but if AMP discovery is slower, then transitioning a channel to an AMP link will be slower.

And even the READ_BUFFER_SIZE is the wrong command since by default the
AMP controller is in block based flow control mode. So you would need to
read the flow control mode first actually.

Correct, although reading the flow control mode on an AMP controller after a HCI_Reset *should* always return "data block mode" (as that is the default).  The READ_BUFFER_SIZE should only be used if HCI_Write_Flow_Control_Mode has been issued first (and successfully enabled packet based flow control).  Instead I believe we should issue the HCI_Read_Data_Block_Size command, where the correct information is then provided for the host stack (with its "new" data block flow control model for AMP controllers).  I don't think there is any requirement forcing an AMP controller to implement packet based flow control, so if there is no data block flow control model in the host yet then initialization has to be prepared to fail upon trying to configure packet based flow control mode.

Also for AMP Controller initialization, the HCI_Set_Event_Mask_Page_2 command should be issued to ensure the AMP related events are provided by the controller (by default they are turned off).  A suitable HCI_Set_Event_Mask command may also be needed if using enhanced flush on the AMP controller.  Event filters also apply to AMP controllers IIRC, so if following the BR/EDR example you may want to clear them too (although I imagine such initialization could be added when required).

Regards,
James


________________________________
Subject to local law, communications with Accenture and its affiliates including telephone calls and emails (including content), may be monitored by our systems for the purposes of security and the assessment of internal compliance with Accenture policy.
______________________________________________________________________________________

www.accenture.com