| Message ID | 20180314155514.3374-3-thierry.escande@linaro.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Bluetooth: hci_qca: Add serdev support | expand |
Hi Thierry, > Add binding document for serial bluetooth chips using Qualcomm protocol. > > Signed-off-by: Thierry Escande <thierry.escande@linaro.org> > --- > > v4: > - Move bt-disable-n-gpios to required properties section > - Add clocks and pinctrl-0 as required properties > > v3: no change > v2: no change > > .../devicetree/bindings/net/qualcomm-bluetooth.txt | 34 ++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt > > diff --git a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt > new file mode 100644 > index 000000000000..cdb14b96c229 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt > @@ -0,0 +1,34 @@ > +Qualcomm Bluetooth Chips > +--------------------- > + > +This documents the binding structure and common properties for serial > +attached Qualcomm devices. > + > +Serial attached Qualcomm devices shall be a child node of the host UART > +device the slave device is attached to. > + > +Required properties: > + - compatible: should contain one of the following: > + * "qcom,qca6174-bt" > + - bt-disable-n-gpios: gpio specifier, used to enable chip during probe > + - pinctrl-0: pin phandle for bt_en gpio > + - clocks: clock phandle for SUSCLK_32KHZ > + > +Example: > + > +serial@7570000 { > + pinctrl-names = "default", "sleep"; > + pinctrl-0 = <&blsp1_uart1_default>; > + pinctrl-1 = <&blsp1_uart1_sleep>; > + > + bluetooth { > + compatible = "qcom,qca6174-bt"; > + > + bt-disable-n-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>; can we use a common name here. I think that Nokia and Broadcom drivers define one. And if this is the enable/shutdown GPIO, we should name it consistently across all manufacturers. It essentially does the same on Bluetooth UART chips no matter what chip is behind them. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 14 Mar 09:13 PDT 2018, Marcel Holtmann wrote: > > + bt-disable-n-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>; > > can we use a common name here. I think that Nokia and Broadcom drivers > define one. And if this is the enable/shutdown GPIO, we should name it > consistently across all manufacturers. It essentially does the same on > Bluetooth UART chips no matter what chip is behind them. > Broadcomm has a device-wakup-gpios and Nokia has bluetooth-wakup-gpios. It might be that these behave in the same way, but from the description they only trigger the wakeup. The reason for the proposed naming here is that the pin is named "BT_DISABLE_N" in the datasheet. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjoern, >>> + bt-disable-n-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>; >> >> can we use a common name here. I think that Nokia and Broadcom drivers >> define one. And if this is the enable/shutdown GPIO, we should name it >> consistently across all manufacturers. It essentially does the same on >> Bluetooth UART chips no matter what chip is behind them. >> > > Broadcomm has a device-wakup-gpios and Nokia has bluetooth-wakup-gpios. > It might be that these behave in the same way, but from the description > they only trigger the wakeup. that is something that we might need to start fixing. I really prefer if we name the GPIOs a bit more consistent. > The reason for the proposed naming here is that the pin is named > "BT_DISABLE_N" in the datasheet. That is not a reason I buy. So the next board comes around that labels it in the data sheet BT_DISABLE_YEAH_SUPER_GREAT and you send me a patch to the driver to look for that name. If the GPIO does the same thing, I couldn’t care less what the data sheet says. That might be a comment in the DT file, but it should not pollute the driver code. A new board should not require driver changes, you just ship a new DT for that board and an existing driver hopefully just does the job. No matter how someone named a GPIO in a piece of paper. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 14 Mar 11:42 PDT 2018, Marcel Holtmann wrote: > Hi Bjoern, > > >>> + bt-disable-n-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>; > >> > >> can we use a common name here. I think that Nokia and Broadcom drivers > >> define one. And if this is the enable/shutdown GPIO, we should name it > >> consistently across all manufacturers. It essentially does the same on > >> Bluetooth UART chips no matter what chip is behind them. > >> > > > > Broadcomm has a device-wakup-gpios and Nokia has bluetooth-wakup-gpios. > > It might be that these behave in the same way, but from the description > > they only trigger the wakeup. > > that is something that we might need to start fixing. I really prefer > if we name the GPIOs a bit more consistent. > > > The reason for the proposed naming here is that the pin is named > > "BT_DISABLE_N" in the datasheet. > > That is not a reason I buy. So the next board comes around that labels > it in the data sheet BT_DISABLE_YEAH_SUPER_GREAT and you send me a > patch to the driver to look for that name. If the GPIO does the same > thing, I couldn’t care less what the data sheet says. That might be > a comment in the DT file, but it should not pollute the driver code. > BT_DISABLE_N is the name of this pin in the datasheet of the QCA chip, not on the board, so this name is the same regardless of what you name the line or gpio your board connect it to. > A new board should not require driver changes, you just ship a new DT > for that board and an existing driver hopefully just does the job. No > matter how someone named a GPIO in a piece of paper. > I totally agree with the fact that the board should not affect the naming of the gpio in the driver. But I do enjoy when we refer to pins by their real name - instead of having to guess which pin in the _chip_ specification the driver actually refer to. That said, what name would you prefer for this? Afaict this is not "wakeup" and there are a few extra steps between the disabled state and "bluetooth is enabled", so "enable" feels slightly wrong. And it probably should be "bluetooth" and not just "device" as this refers to a pin on a WiFi/BT combo chip. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, >>>>> + bt-disable-n-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>; >>>> >>>> can we use a common name here. I think that Nokia and Broadcom drivers >>>> define one. And if this is the enable/shutdown GPIO, we should name it >>>> consistently across all manufacturers. It essentially does the same on >>>> Bluetooth UART chips no matter what chip is behind them. >>>> >>> >>> Broadcomm has a device-wakup-gpios and Nokia has bluetooth-wakup-gpios. >>> It might be that these behave in the same way, but from the description >>> they only trigger the wakeup. >> >> that is something that we might need to start fixing. I really prefer >> if we name the GPIOs a bit more consistent. >> >>> The reason for the proposed naming here is that the pin is named >>> "BT_DISABLE_N" in the datasheet. >> >> That is not a reason I buy. So the next board comes around that labels >> it in the data sheet BT_DISABLE_YEAH_SUPER_GREAT and you send me a >> patch to the driver to look for that name. If the GPIO does the same >> thing, I couldn’t care less what the data sheet says. That might be >> a comment in the DT file, but it should not pollute the driver code. >> > > BT_DISABLE_N is the name of this pin in the datasheet of the QCA chip, > not on the board, so this name is the same regardless of what you name > the line or gpio your board connect it to. and QCA chip v1 and QCA chip v2 will use the same driver and same firmware loading mechanism. So why do we have to add a new GPIO naming if they decide to change the name in the data sheet. With Bluetooth it is pretty much all the same. Every UART chip has a shutdown/reset GPIO to enable/disable the chip behind the UART. >> A new board should not require driver changes, you just ship a new DT >> for that board and an existing driver hopefully just does the job. No >> matter how someone named a GPIO in a piece of paper. >> > > I totally agree with the fact that the board should not affect the > naming of the gpio in the driver. But I do enjoy when we refer to pins > by their real name - instead of having to guess which pin in the _chip_ > specification the driver actually refer to. > > > That said, what name would you prefer for this? > > Afaict this is not "wakeup" and there are a few extra steps between the > disabled state and "bluetooth is enabled", so "enable" feels slightly > wrong. And it probably should be "bluetooth" and not just "device" as > this refers to a pin on a WiFi/BT combo chip. The Broadcom side called it shutdown GPIO, it is essentially the shutdown/reset GPIO or power on/off GPIO. Personally I do not care what it is named, but it will be all the same for all Bluetooth chips. Take a poll from Broadcom, Intel, Realtek and Qualcomm and you can pick a reasonable common name. For the wakeup GPIOs, these are different and depend on if there is some low-power mode provided. You would need to check the data sheet to see if they provide more advanced low-power state handling. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Marcel, On 14/03/2018 20:51, Marcel Holtmann wrote: > Hi Bjorn, > >>>>>> + bt-disable-n-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>; >>>>> >>>>> can we use a common name here. I think that Nokia and Broadcom drivers >>>>> define one. And if this is the enable/shutdown GPIO, we should name it >>>>> consistently across all manufacturers. It essentially does the same on >>>>> Bluetooth UART chips no matter what chip is behind them. >>>>> >>>> >>>> Broadcomm has a device-wakup-gpios and Nokia has bluetooth-wakup-gpios. >>>> It might be that these behave in the same way, but from the description >>>> they only trigger the wakeup. >>> >>> that is something that we might need to start fixing. I really prefer >>> if we name the GPIOs a bit more consistent. >>> >>>> The reason for the proposed naming here is that the pin is named >>>> "BT_DISABLE_N" in the datasheet. >>> >>> That is not a reason I buy. So the next board comes around that labels >>> it in the data sheet BT_DISABLE_YEAH_SUPER_GREAT and you send me a >>> patch to the driver to look for that name. If the GPIO does the same >>> thing, I couldn’t care less what the data sheet says. That might be >>> a comment in the DT file, but it should not pollute the driver code. >>> >> >> BT_DISABLE_N is the name of this pin in the datasheet of the QCA chip, >> not on the board, so this name is the same regardless of what you name >> the line or gpio your board connect it to. > > and QCA chip v1 and QCA chip v2 will use the same driver and same firmware loading mechanism. So why do we have to add a new GPIO naming if they decide to change the name in the data sheet. With Bluetooth it is pretty much all the same. Every UART chip has a shutdown/reset GPIO to enable/disable the chip behind the UART. > >>> A new board should not require driver changes, you just ship a new DT >>> for that board and an existing driver hopefully just does the job. No >>> matter how someone named a GPIO in a piece of paper. >>> >> >> I totally agree with the fact that the board should not affect the >> naming of the gpio in the driver. But I do enjoy when we refer to pins >> by their real name - instead of having to guess which pin in the _chip_ >> specification the driver actually refer to. >> >> >> That said, what name would you prefer for this? >> >> Afaict this is not "wakeup" and there are a few extra steps between the >> disabled state and "bluetooth is enabled", so "enable" feels slightly >> wrong. And it probably should be "bluetooth" and not just "device" as >> this refers to a pin on a WiFi/BT combo chip. > > The Broadcom side called it shutdown GPIO, it is essentially the shutdown/reset GPIO or power on/off GPIO. Personally I do not care what it is named, but it will be all the same for all Bluetooth chips. Take a poll from Broadcom, Intel, Realtek and Qualcomm and you can pick a reasonable common name. The Nokia driver has "bluetooth-wakeup" gpio. The Broadcom one has "device-wakeup" and "shutdown". The "shutdown" gpio is set to its active state to power on the chip which sounds reversed logic. Same for the "bt-disable-n" gpio in the Qualcomm driver, configured as ACTIVE_HIGH, and which is set to 1 to enable it... So for consistency, naming it as "shutdown" to stick to the bcm driver but it should be configured as ACTIVE_LOW in the dts so we actually do a gpiod_set_value(0) to un-shutdown it. Does that sound ok? Regards, Thierry > For the wakeup GPIOs, these are different and depend on if there is some low-power mode provided. You would need to check the data sheet to see if they provide more advanced low-power state handling. > > Regards > > Marcel > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Thierry, On Thu, Mar 15, 2018 at 12:07:44PM +0100, Thierry Escande wrote: > > > BT_DISABLE_N is the name of this pin in the datasheet of the QCA chip, > > > not on the board, so this name is the same regardless of what you name > > > the line or gpio your board connect it to. > > > > and QCA chip v1 and QCA chip v2 will use the same driver and > > same firmware loading mechanism. So why do we have to add a new > > GPIO naming if they decide to change the name in the data sheet. > > With Bluetooth it is pretty much all the same. Every UART chip > > has a shutdown/reset GPIO to enable/disable the chip behind the > > UART. > > > > > > A new board should not require driver changes, you just ship a new DT > > > > for that board and an existing driver hopefully just does the job. No > > > > matter how someone named a GPIO in a piece of paper. > > > > > > > > > > I totally agree with the fact that the board should not affect the > > > naming of the gpio in the driver. But I do enjoy when we refer to pins > > > by their real name - instead of having to guess which pin in the _chip_ > > > specification the driver actually refer to. > > > > > > > > > That said, what name would you prefer for this? > > > > > > Afaict this is not "wakeup" and there are a few extra steps between the > > > disabled state and "bluetooth is enabled", so "enable" feels slightly > > > wrong. And it probably should be "bluetooth" and not just "device" as > > > this refers to a pin on a WiFi/BT combo chip. > > > > The Broadcom side called it shutdown GPIO, it is essentially the > > shutdown/reset GPIO or power on/off GPIO. Personally I do not > > care what it is named, but it will be all the same for all > > Bluetooth chips. Take a poll from Broadcom, Intel, Realtek and > > Qualcomm and you can pick a reasonable common name. > > The Nokia driver has "bluetooth-wakeup" gpio. The Broadcom one has > "device-wakeup" and "shutdown". The "shutdown" gpio is set to its active > state to power on the chip which sounds reversed logic. Same for the > "bt-disable-n" gpio in the Qualcomm driver, configured as ACTIVE_HIGH, and > which is set to 1 to enable it... > > So for consistency, naming it as "shutdown" to stick to the bcm driver but > it should be configured as ACTIVE_LOW in the dts so we actually do a > gpiod_set_value(0) to un-shutdown it. Does that sound ok? FWIW you picked the wrong gpio from the nokia bluetooth binding. The gpio for shutdown would be "reset". The "bluetooth-wakeup" is required for normal operation to exit idle mode. The "reset" name used by the nokia binding is quite common for DT: Documentation/devicetree/bindings $ git grep reset-gpios | wc -l 212 I guess it only makes sense when the device is actually being reset, though (i.e. for Nokia the settings are back to defaults and you need to re-upload the FW). -- Sebastian > Regards, > Thierry > > > For the wakeup GPIOs, these are different and depend on if there > > is some low-power mode provided. You would need to check the > > data sheet to see if they provide more advanced low-power state > > handling. > > > > Regards > > > > Marcel > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sebastian, >>>> BT_DISABLE_N is the name of this pin in the datasheet of the QCA chip, >>>> not on the board, so this name is the same regardless of what you name >>>> the line or gpio your board connect it to. >>> >>> and QCA chip v1 and QCA chip v2 will use the same driver and >>> same firmware loading mechanism. So why do we have to add a new >>> GPIO naming if they decide to change the name in the data sheet. >>> With Bluetooth it is pretty much all the same. Every UART chip >>> has a shutdown/reset GPIO to enable/disable the chip behind the >>> UART. >>> >>>>> A new board should not require driver changes, you just ship a new DT >>>>> for that board and an existing driver hopefully just does the job. No >>>>> matter how someone named a GPIO in a piece of paper. >>>>> >>>> >>>> I totally agree with the fact that the board should not affect the >>>> naming of the gpio in the driver. But I do enjoy when we refer to pins >>>> by their real name - instead of having to guess which pin in the _chip_ >>>> specification the driver actually refer to. >>>> >>>> >>>> That said, what name would you prefer for this? >>>> >>>> Afaict this is not "wakeup" and there are a few extra steps between the >>>> disabled state and "bluetooth is enabled", so "enable" feels slightly >>>> wrong. And it probably should be "bluetooth" and not just "device" as >>>> this refers to a pin on a WiFi/BT combo chip. >>> >>> The Broadcom side called it shutdown GPIO, it is essentially the >>> shutdown/reset GPIO or power on/off GPIO. Personally I do not >>> care what it is named, but it will be all the same for all >>> Bluetooth chips. Take a poll from Broadcom, Intel, Realtek and >>> Qualcomm and you can pick a reasonable common name. >> >> The Nokia driver has "bluetooth-wakeup" gpio. The Broadcom one has >> "device-wakeup" and "shutdown". The "shutdown" gpio is set to its active >> state to power on the chip which sounds reversed logic. Same for the >> "bt-disable-n" gpio in the Qualcomm driver, configured as ACTIVE_HIGH, and >> which is set to 1 to enable it... >> >> So for consistency, naming it as "shutdown" to stick to the bcm driver but >> it should be configured as ACTIVE_LOW in the dts so we actually do a >> gpiod_set_value(0) to un-shutdown it. Does that sound ok? > > FWIW you picked the wrong gpio from the nokia bluetooth binding. The > gpio for shutdown would be "reset". The "bluetooth-wakeup" is > required for normal operation to exit idle mode. The "reset" name > used by the nokia binding is quite common for DT: > > Documentation/devicetree/bindings $ git grep reset-gpios | wc -l > 212 > > I guess it only makes sense when the device is actually being > reset, though (i.e. for Nokia the settings are back to defaults > and you need to re-upload the FW). that is actually a good point. I like to differentiate between a shutdown GPIO (and we can argue about the inversion here) and the reset GPIO. If we loose the firmware and the programmed BD_ADDR, then it is a hard-reset, otherwise it is shutdown/power GPIO. So can we agree on how we name the hard-reset, shutdown, wakeup etc. GPIOs for Bluetooth UART devices? Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 15, 2018 at 02:46:45PM +0100, Sebastian Reichel wrote: > Hi Thierry, > > On Thu, Mar 15, 2018 at 12:07:44PM +0100, Thierry Escande wrote: > > > > BT_DISABLE_N is the name of this pin in the datasheet of the QCA chip, > > > > not on the board, so this name is the same regardless of what you name > > > > the line or gpio your board connect it to. > > > > > > and QCA chip v1 and QCA chip v2 will use the same driver and > > > same firmware loading mechanism. So why do we have to add a new > > > GPIO naming if they decide to change the name in the data sheet. > > > With Bluetooth it is pretty much all the same. Every UART chip > > > has a shutdown/reset GPIO to enable/disable the chip behind the > > > UART. > > > > > > > > A new board should not require driver changes, you just ship a new DT > > > > > for that board and an existing driver hopefully just does the job. No > > > > > matter how someone named a GPIO in a piece of paper. > > > > > > > > > > > > > I totally agree with the fact that the board should not affect the > > > > naming of the gpio in the driver. But I do enjoy when we refer to pins > > > > by their real name - instead of having to guess which pin in the _chip_ > > > > specification the driver actually refer to. > > > > > > > > > > > > That said, what name would you prefer for this? > > > > > > > > Afaict this is not "wakeup" and there are a few extra steps between the > > > > disabled state and "bluetooth is enabled", so "enable" feels slightly > > > > wrong. And it probably should be "bluetooth" and not just "device" as > > > > this refers to a pin on a WiFi/BT combo chip. > > > > > > The Broadcom side called it shutdown GPIO, it is essentially the > > > shutdown/reset GPIO or power on/off GPIO. Personally I do not > > > care what it is named, but it will be all the same for all > > > Bluetooth chips. Take a poll from Broadcom, Intel, Realtek and > > > Qualcomm and you can pick a reasonable common name. > > > > The Nokia driver has "bluetooth-wakeup" gpio. The Broadcom one has > > "device-wakeup" and "shutdown". The "shutdown" gpio is set to its active > > state to power on the chip which sounds reversed logic. Same for the > > "bt-disable-n" gpio in the Qualcomm driver, configured as ACTIVE_HIGH, and > > which is set to 1 to enable it... > > > > So for consistency, naming it as "shutdown" to stick to the bcm driver but > > it should be configured as ACTIVE_LOW in the dts so we actually do a > > gpiod_set_value(0) to un-shutdown it. Does that sound ok? > > FWIW you picked the wrong gpio from the nokia bluetooth binding. The > gpio for shutdown would be "reset". The "bluetooth-wakeup" is > required for normal operation to exit idle mode. The "reset" name > used by the nokia binding is quite common for DT: > > Documentation/devicetree/bindings $ git grep reset-gpios | wc -l > 212 > > I guess it only makes sense when the device is actually being > reset, though (i.e. for Nokia the settings are back to defaults > and you need to re-upload the FW). The standardish names are reset-gpios, enable-gpios, and powerdown-gpios. Pick one or some of those. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt new file mode 100644 index 000000000000..cdb14b96c229 --- /dev/null +++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt @@ -0,0 +1,34 @@ +Qualcomm Bluetooth Chips +--------------------- + +This documents the binding structure and common properties for serial +attached Qualcomm devices. + +Serial attached Qualcomm devices shall be a child node of the host UART +device the slave device is attached to. + +Required properties: + - compatible: should contain one of the following: + * "qcom,qca6174-bt" + - bt-disable-n-gpios: gpio specifier, used to enable chip during probe + - pinctrl-0: pin phandle for bt_en gpio + - clocks: clock phandle for SUSCLK_32KHZ + +Example: + +serial@7570000 { + pinctrl-names = "default", "sleep"; + pinctrl-0 = <&blsp1_uart1_default>; + pinctrl-1 = <&blsp1_uart1_sleep>; + + bluetooth { + compatible = "qcom,qca6174-bt"; + + bt-disable-n-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>; + + pinctrl-names = "default"; + pinctrl-0 = <&bt_en_pin_a>; + + clocks = <&divclk4>; + }; +};
Add binding document for serial bluetooth chips using Qualcomm protocol. Signed-off-by: Thierry Escande <thierry.escande@linaro.org> --- v4: - Move bt-disable-n-gpios to required properties section - Add clocks and pinctrl-0 as required properties v3: no change v2: no change .../devicetree/bindings/net/qualcomm-bluetooth.txt | 34 ++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt