[PATCH 0/2] ath10k: Add support to configure BB timing for external FEM

[PATCH 0/2] ath10k: Add support to configure BB timing for external FEM

[Bhagavathi Perumal S]

This adds support to choose correct Base Band(BB) timing values based on
the input Front End Module(FEM) hardware name from Device Tree entry and
configure it in hardware through WMI interface. Since the BB timing values
can vary between FEM hardwares and not same as default value used
in reference hardware.

Bhagavathi Perumal S (2):
  dt: bindings: add new dt entry to indentify external FEM
  ath10k: Add support to configure BB timing over wmi

 .../bindings/net/wireless/qcom,ath10k.txt          | 22 ++++++++++
 drivers/net/wireless/ath/ath10k/mac.c              | 47 ++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/wmi-ops.h          | 20 +++++++++
 drivers/net/wireless/ath/ath10k/wmi.c              | 23 +++++++++++
 drivers/net/wireless/ath/ath10k/wmi.h              | 26 ++++++++++++
 5 files changed, 138 insertions(+)

-- 
1.9.1

[PATCH 1/2] dt: bindings: add new dt entry to indentify external FEM

[Bhagavathi Perumal S]

This adds new dt entry ext-fem-name, it is used by ath10k driver
to select correct timing parameters and configure it in target wifi hardware.
The Front End Module(FEM) normally includes tx power amplifier(PA) and
rx low noise amplifier(LNA). The default timing parameters like tx end to
PA off timing values were fine tuned for internal FEM used in reference
design. And these timing values can not be same if ODM modifies hardware
design with different external FEM. This DT entry helps to choose correct
timing values in driver if different external FEM hardware used.

Signed-off-by: Bhagavathi Perumal S <bperumal@codeaurora.org>
---
 .../bindings/net/wireless/qcom,ath10k.txt          | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
index 7fd4e8c..fbaf309 100644
--- a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
+++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
@@ -56,6 +56,7 @@ Optional properties:
 				     the length can vary between hw versions.
 - <supply-name>-supply: handle to the regulator device tree node
 			   optional "supply-name" is "vdd-0.8-cx-mx".
+- ext-fem-name: name of external front end module used.
 
 Example (to supply the calibration data alone):
 
@@ -150,3 +151,24 @@ wifi@18000000 {
 			   <0 141 0 /* CE11 */ >;
 		vdd-0.8-cx-mx-supply = <&pm8998_l5>;
 };
+
+Example (to supply the external front end module name):
+
+In this example, the front end module is defined as a property of the ath10k
+device node.
+
+pci {
+	pcie@0 {
+		reg = <0 0 0 0 0>;
+		#interrupt-cells = <1>;
+		#size-cells = <2>;
+		#address-cells = <3>;
+		device_type = "pci";
+
+		ath10k@0,0 {
+			reg = <0 0 0 0 0>;
+			device_type = "pci";
+			ext-fem-name = "microsemi-lx5586";
+		};
+	};
+};
-- 
1.9.1

[PATCH 2/2] ath10k: Add support to configure BB timing over wmi

[Bhagavathi Perumal S]

Add wmi configuration cmd to configure base band(BB) power amplifier(PA)
off timing values in hardware. The default PA off timings were fine tuned
to make proper DFS radar detection in QCA reference design. If ODM uses
different PA in their design, then the same default PA off timing values
cannot be used, it requires different settling time to detect radar pulses
very sooner and avoid radar detection problems. In that case it provides
provision to select proper PA off timing values based on the PA hardware used.
The PA component is part of FEM hardware and new device tree entry
"ext-fem-name" is used to indentify the FEM hardware. And this wmi configuration
cmd is enabled via wmi service flag "WMI_SERVICE_BB_TIMING_CONFIG_SUPPORT".

Other way is to apply these values through calibration data, but recalibration
of all boards out there might not be feasible.

This change tested on firmware ver 10.2.4-1.0-00042 in QCA988X chipset.

Signed-off-by: Bhagavathi Perumal S <bperumal@codeaurora.org>
---
 drivers/net/wireless/ath/ath10k/mac.c     | 47 +++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/wmi-ops.h | 20 +++++++++++++
 drivers/net/wireless/ath/ath10k/wmi.c     | 23 +++++++++++++++
 drivers/net/wireless/ath/ath10k/wmi.h     | 26 +++++++++++++++++
 4 files changed, 116 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 3933dd9..a242c6b 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -22,6 +22,7 @@
 #include <net/mac80211.h>
 #include <linux/etherdevice.h>
 #include <linux/acpi.h>
+#include <linux/of.h>
 
 #include "hif.h"
 #include "core.h"
@@ -4637,11 +4638,44 @@ static int ath10k_set_antenna(struct ieee80211_hw *hw, u32 tx_ant, u32 rx_ant)
 	return ret;
 }
 
+static int __ath10k_fetch_bb_timing_dt(struct ath10k *ar,
+				       struct wmi_bb_timing_cfg_arg *bb_timing)
+{
+	struct device_node *node;
+	const char *fem_name;
+	int ret;
+
+	node = ar->dev->of_node;
+	if (!node)
+		return -ENOENT;
+
+	ret = of_property_read_string_index(node, "ext-fem-name", 0, &fem_name);
+	if (ret)
+		return -ENOENT;
+
+	/*
+	 * If external Front End module used in hardware, then default base band timing
+	 * parameter cannot be used since they were fine tuned for reference hardware,
+	 * so choosing different value suitable for that external FEM.
+	 */
+	if (!strcmp("microsemi-lx5586", fem_name)) {
+		bb_timing->bb_tx_timing = 0x00;
+		bb_timing->bb_xpa_timing = 0x0101;
+	} else {
+		return -ENOENT;
+	}
+
+	ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot bb_tx_timing 0x%x bb_xpa_timing 0x%x\n",
+		   bb_timing->bb_tx_timing, bb_timing->bb_xpa_timing);
+	return 0;
+}
+
 static int ath10k_start(struct ieee80211_hw *hw)
 {
 	struct ath10k *ar = hw->priv;
 	u32 param;
 	int ret = 0;
+	struct wmi_bb_timing_cfg_arg bb_timing = {0};
 
 	/*
 	 * This makes sense only when restarting hw. It is harmless to call
@@ -4788,6 +4822,19 @@ static int ath10k_start(struct ieee80211_hw *hw)
 		clear_bit(ATH10K_FLAG_BTCOEX, &ar->dev_flags);
 	}
 
+	if (test_bit(WMI_SERVICE_BB_TIMING_CONFIG_SUPPORT, ar->wmi.svc_map)) {
+		ret = __ath10k_fetch_bb_timing_dt(ar, &bb_timing);
+		if (!ret) {
+			ret = ath10k_wmi_pdev_bb_timing(ar, &bb_timing);
+			if (ret) {
+				ath10k_warn(ar,
+					    "failed to set bb timings: %d\n",
+					    ret);
+				goto err_core_stop;
+			}
+		}
+	}
+
 	ar->num_started_vdevs = 0;
 	ath10k_regd_update(ar);
 
diff --git a/drivers/net/wireless/ath/ath10k/wmi-ops.h b/drivers/net/wireless/ath/ath10k/wmi-ops.h
index 7fd63bb..7c7ee24 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-ops.h
+++ b/drivers/net/wireless/ath/ath10k/wmi-ops.h
@@ -216,6 +216,9 @@ struct wmi_ops {
 	struct sk_buff *(*gen_echo)(struct ath10k *ar, u32 value);
 	struct sk_buff *(*gen_pdev_get_tpc_table_cmdid)(struct ath10k *ar,
 							u32 param);
+	struct sk_buff *(*gen_bb_timing)
+			(struct ath10k *ar,
+			 const struct wmi_bb_timing_cfg_arg *arg);
 
 };
 
@@ -1555,4 +1558,21 @@ struct wmi_ops {
 				   ar->wmi.cmd->radar_found_cmdid);
 }
 
+static inline int
+ath10k_wmi_pdev_bb_timing(struct ath10k *ar,
+			  const struct wmi_bb_timing_cfg_arg *arg)
+{
+	struct sk_buff *skb;
+
+	if (!ar->wmi.ops->gen_bb_timing)
+		return -EOPNOTSUPP;
+
+	skb = ar->wmi.ops->gen_bb_timing(ar, arg);
+
+	if (IS_ERR(skb))
+		return PTR_ERR(skb);
+
+	return ath10k_wmi_cmd_send(ar, skb,
+				   ar->wmi.cmd->set_bb_timing_cmdid);
+}
 #endif
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 25e8fa7..7410362 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -539,6 +539,7 @@
 		WMI_10_2_PDEV_BSS_CHAN_INFO_REQUEST_CMDID,
 	.pdev_get_tpc_table_cmdid = WMI_CMD_UNSUPPORTED,
 	.radar_found_cmdid = WMI_CMD_UNSUPPORTED,
+	.set_bb_timing_cmdid = WMI_10_2_PDEV_SET_BB_TIMING_CONFIG_CMDID,
 };
 
 /* 10.4 WMI cmd track */
@@ -8785,6 +8786,27 @@ static u32 ath10k_wmi_prepare_peer_qos(u8 uapsd_queues, u8 sp)
 	return 0;
 }
 
+static struct sk_buff *
+ath10k_wmi_10_2_4_op_gen_bb_timing(struct ath10k *ar,
+				   const struct wmi_bb_timing_cfg_arg *arg)
+{
+	struct wmi_pdev_bb_timing_cfg_cmd *cmd;
+	struct sk_buff *skb;
+
+	skb = ath10k_wmi_alloc_skb(ar, sizeof(*cmd));
+	if (!skb)
+		return ERR_PTR(-ENOMEM);
+
+	cmd = (struct wmi_pdev_bb_timing_cfg_cmd *)skb->data;
+	cmd->bb_tx_timing = __cpu_to_le32(arg->bb_tx_timing);
+	cmd->bb_xpa_timing = __cpu_to_le32(arg->bb_xpa_timing);
+
+	ath10k_dbg(ar, ATH10K_DBG_WMI,
+		   "wmi pdev bb_tx_timing 0x%x bb_xpa_timing 0x%x\n",
+		   arg->bb_tx_timing, arg->bb_xpa_timing);
+	return skb;
+}
+
 static const struct wmi_ops wmi_ops = {
 	.rx = ath10k_wmi_op_rx,
 	.map_svc = wmi_main_svc_map,
@@ -9058,6 +9080,7 @@ static u32 ath10k_wmi_prepare_peer_qos(u8 uapsd_queues, u8 sp)
 	.gen_pdev_enable_adaptive_cca =
 		ath10k_wmi_op_gen_pdev_enable_adaptive_cca,
 	.get_vdev_subtype = ath10k_wmi_10_2_4_op_get_vdev_subtype,
+	.gen_bb_timing = ath10k_wmi_10_2_4_op_gen_bb_timing,
 	/* .gen_bcn_tmpl not implemented */
 	/* .gen_prb_tmpl not implemented */
 	/* .gen_p2p_go_bcn_ie not implemented */
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index f67c527..048305c 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -205,6 +205,7 @@ enum wmi_service {
 	WMI_SERVICE_SPOOF_MAC_SUPPORT,
 	WMI_SERVICE_TX_DATA_ACK_RSSI,
 	WMI_SERVICE_VDEV_DIFFERENT_BEACON_INTERVAL_SUPPORT,
+	WMI_SERVICE_BB_TIMING_CONFIG_SUPPORT,
 
 	/* keep last */
 	WMI_SERVICE_MAX,
@@ -244,6 +245,9 @@ enum wmi_10x_service {
 	WMI_10X_SERVICE_PEER_STATS,
 	WMI_10X_SERVICE_RESET_CHIP,
 	WMI_10X_SERVICE_HTT_MGMT_TX_COMP_VALID_FLAGS,
+	WMI_10X_SERVICE_VDEV_BCN_RATE_CONTROL,
+	WMI_10X_SERVICE_PER_PACKET_SW_ENCRYPT,
+	WMI_10X_SERVICE_BB_TIMING_CONFIG_SUPPORT,
 };
 
 enum wmi_main_service {
@@ -568,6 +572,8 @@ static inline void wmi_10x_svc_map(const __le32 *in, unsigned long *out,
 	       WMI_SERVICE_RESET_CHIP, len);
 	SVCMAP(WMI_10X_SERVICE_HTT_MGMT_TX_COMP_VALID_FLAGS,
 	       WMI_SERVICE_HTT_MGMT_TX_COMP_VALID_FLAGS, len);
+	SVCMAP(WMI_10X_SERVICE_BB_TIMING_CONFIG_SUPPORT,
+	       WMI_SERVICE_BB_TIMING_CONFIG_SUPPORT, len);
 }
 
 static inline void wmi_main_svc_map(const __le32 *in, unsigned long *out,
@@ -986,6 +992,7 @@ struct wmi_cmd_map {
 	u32 pdev_wds_entry_list_cmdid;
 	u32 tdls_set_offchan_mode_cmdid;
 	u32 radar_found_cmdid;
+	u32 set_bb_timing_cmdid;
 };
 
 /*
@@ -1601,6 +1608,8 @@ enum wmi_10_2_cmd_id {
 	WMI_10_2_SET_LTEU_CONFIG_CMDID,
 	WMI_10_2_SET_CCA_PARAMS,
 	WMI_10_2_PDEV_BSS_CHAN_INFO_REQUEST_CMDID,
+	WMI_10_2_FWTEST_CMDID,
+	WMI_10_2_PDEV_SET_BB_TIMING_CONFIG_CMDID,
 	WMI_10_2_PDEV_UTF_CMDID = WMI_10_2_END_CMDID - 1,
 };
 
@@ -7083,6 +7092,23 @@ struct wmi_pdev_chan_info_req_cmd {
 	__le32 reserved;
 } __packed;
 
+/* bb timing register configurations */
+struct wmi_bb_timing_cfg_arg {
+	/* Tx_end to pa off timing */
+	u32 bb_tx_timing;
+
+	/* Tx_end to external pa off timing */
+	u32 bb_xpa_timing;
+};
+
+struct wmi_pdev_bb_timing_cfg_cmd {
+	/* Tx_end to pa off timing */
+	__le32 bb_tx_timing;
+
+	/* Tx_end to external pa off timing */
+	__le32 bb_xpa_timing;
+} __packed;
+
 struct ath10k;
 struct ath10k_vif;
 struct ath10k_fw_stats_pdev;
-- 
1.9.1

Re: [PATCH 1/2] dt: bindings: add new dt entry to indentify external FEM

[Rob Herring]

On Thu, Oct 04, 2018 at 05:12:43PM +0530, Bhagavathi Perumal S wrote:
> This adds new dt entry ext-fem-name, it is used by ath10k driver
> to select correct timing parameters and configure it in target wifi hardware.
> The Front End Module(FEM) normally includes tx power amplifier(PA) and
> rx low noise amplifier(LNA). The default timing parameters like tx end to
> PA off timing values were fine tuned for internal FEM used in reference
> design. And these timing values can not be same if ODM modifies hardware
> design with different external FEM. This DT entry helps to choose correct
> timing values in driver if different external FEM hardware used.

'dt-bindings: net: ath10k: ...' for the subject please.

> 
> Signed-off-by: Bhagavathi Perumal S <bperumal@codeaurora.org>
> ---
>  .../bindings/net/wireless/qcom,ath10k.txt          | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
> index 7fd4e8c..fbaf309 100644
> --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
> +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
> @@ -56,6 +56,7 @@ Optional properties:
>  				     the length can vary between hw versions.
>  - <supply-name>-supply: handle to the regulator device tree node
>  			   optional "supply-name" is "vdd-0.8-cx-mx".
> +- ext-fem-name: name of external front end module used.

What are valid names? What if the OS doesn't recognize the name? Perhaps 
this should be a compatible string for the particular module instead? 
Then it could cover any differences, not just the FEM.

>  
>  Example (to supply the calibration data alone):
>  
> @@ -150,3 +151,24 @@ wifi@18000000 {
>  			   <0 141 0 /* CE11 */ >;
>  		vdd-0.8-cx-mx-supply = <&pm8998_l5>;
>  };
> +
> +Example (to supply the external front end module name):
> +
> +In this example, the front end module is defined as a property of the ath10k
> +device node.

Really need a whole new example for 1 property?

> +
> +pci {
> +	pcie@0 {
> +		reg = <0 0 0 0 0>;
> +		#interrupt-cells = <1>;
> +		#size-cells = <2>;
> +		#address-cells = <3>;
> +		device_type = "pci";
> +
> +		ath10k@0,0 {

wifi@0,0

> +			reg = <0 0 0 0 0>;
> +			device_type = "pci";
> +			ext-fem-name = "microsemi-lx5586";
> +		};
> +	};
> +};
> -- 
> 1.9.1
> 

Re: [PATCH 1/2] dt: bindings: add new dt entry to indentify external FEM

[bperumal]

On 2018-10-16 00:54, Rob Herring wrote:
> On Thu, Oct 04, 2018 at 05:12:43PM +0530, Bhagavathi Perumal S wrote:
>> This adds new dt entry ext-fem-name, it is used by ath10k driver
>> to select correct timing parameters and configure it in target wifi 
>> hardware.
>> The Front End Module(FEM) normally includes tx power amplifier(PA) and
>> rx low noise amplifier(LNA). The default timing parameters like tx end 
>> to
>> PA off timing values were fine tuned for internal FEM used in 
>> reference
>> design. And these timing values can not be same if ODM modifies 
>> hardware
>> design with different external FEM. This DT entry helps to choose 
>> correct
>> timing values in driver if different external FEM hardware used.
> 
> 'dt-bindings: net: ath10k: ...' for the subject please.
Sure, I will change and send v2 patch.

> 
>> 
>> Signed-off-by: Bhagavathi Perumal S <bperumal@codeaurora.org>
>> ---
>>  .../bindings/net/wireless/qcom,ath10k.txt          | 22 
>> ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt 
>> b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
>> index 7fd4e8c..fbaf309 100644
>> --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
>> +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
>> @@ -56,6 +56,7 @@ Optional properties:
>>  				     the length can vary between hw versions.
>>  - <supply-name>-supply: handle to the regulator device tree node
>>  			   optional "supply-name" is "vdd-0.8-cx-mx".
>> +- ext-fem-name: name of external front end module used.
> 
> What are valid names? What if the OS doesn't recognize the name? 
> Perhaps
Some valid FEM devices are "microsemi-lx5586", "sky85703-11" and 
"sky85803" etc.
Currently driver accepts only "microsemi-lx5586", Since this FEM 
requires different timing settings,
And it can be extended for other FEM devices in future.
If OS doesn't recognize the name, it uses predefined default timing 
settings.

> this should be a compatible string for the particular module instead?
> Then it could cover any differences, not just the FEM.
> 
No, it is specific to FEM device.

>> 
>>  Example (to supply the calibration data alone):
>> 
>> @@ -150,3 +151,24 @@ wifi@18000000 {
>>  			   <0 141 0 /* CE11 */ >;
>>  		vdd-0.8-cx-mx-supply = <&pm8998_l5>;
>>  };
>> +
>> +Example (to supply the external front end module name):
>> +
>> +In this example, the front end module is defined as a property of the 
>> ath10k
>> +device node.
> 
> Really need a whole new example for 1 property?
Will add this property in existing example.

> 
>> +
>> +pci {
>> +	pcie@0 {
>> +		reg = <0 0 0 0 0>;
>> +		#interrupt-cells = <1>;
>> +		#size-cells = <2>;
>> +		#address-cells = <3>;
>> +		device_type = "pci";
>> +
>> +		ath10k@0,0 {
> 
> wifi@0,0
Added in ath10k@0,0 to make consistent with existing property 
"qcom,ath10k-calibration-data" below,
"
pci {
         pcie@0 {
                 reg = <0 0 0 0 0>;
                 #interrupt-cells = <1>;
                 #size-cells = <2>;
                 #address-cells = <3>;
                 device_type = "pci";

                 ath10k@0,0 {
                         reg = <0 0 0 0 0>;
                         device_type = "pci";
                         qcom,ath10k-calibration-data = [ 01 02 03 ... ];
                 };
         };
};
"

If wifi@0,0 is preferable, then I will add new entry "wifi@0,0" and add 
ext-fem-name into it.

> 
>> +			reg = <0 0 0 0 0>;
>> +			device_type = "pci";
>> +			ext-fem-name = "microsemi-lx5586";
>> +		};
>> +	};
>> +};
>> --
>> 1.9.1
>> 

Thanks, Sorry for the delayed response,
Bhagavathi Perumal S.

Re: [PATCH 1/2] dt: bindings: add new dt entry to indentify external FEM

[Rob Herring]

On Tue, Oct 30, 2018 at 3:41 AM <bperumal@codeaurora.org> wrote:
>
> On 2018-10-16 00:54, Rob Herring wrote:
> > On Thu, Oct 04, 2018 at 05:12:43PM +0530, Bhagavathi Perumal S wrote:
> >> This adds new dt entry ext-fem-name, it is used by ath10k driver
> >> to select correct timing parameters and configure it in target wifi
> >> hardware.
> >> The Front End Module(FEM) normally includes tx power amplifier(PA) and
> >> rx low noise amplifier(LNA). The default timing parameters like tx end
> >> to
> >> PA off timing values were fine tuned for internal FEM used in
> >> reference
> >> design. And these timing values can not be same if ODM modifies
> >> hardware
> >> design with different external FEM. This DT entry helps to choose
> >> correct
> >> timing values in driver if different external FEM hardware used.
> >
> > 'dt-bindings: net: ath10k: ...' for the subject please.
> Sure, I will change and send v2 patch.
>
> >
> >>
> >> Signed-off-by: Bhagavathi Perumal S <bperumal@codeaurora.org>
> >> ---
> >>  .../bindings/net/wireless/qcom,ath10k.txt          | 22
> >> ++++++++++++++++++++++
> >>  1 file changed, 22 insertions(+)
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
> >> b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
> >> index 7fd4e8c..fbaf309 100644
> >> --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
> >> +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
> >> @@ -56,6 +56,7 @@ Optional properties:
> >>                                   the length can vary between hw versions.
> >>  - <supply-name>-supply: handle to the regulator device tree node
> >>                         optional "supply-name" is "vdd-0.8-cx-mx".
> >> +- ext-fem-name: name of external front end module used.
> >
> > What are valid names? What if the OS doesn't recognize the name?
> > Perhaps
> Some valid FEM devices are "microsemi-lx5586", "sky85703-11" and
> "sky85803" etc.
> Currently driver accepts only "microsemi-lx5586", Since this FEM
> requires different timing settings,

Assuming you keep this, then you need to enumerate what are valid
values in the binding. Otherwise, 'ext-fem-name = "rob"' is valid.

> And it can be extended for other FEM devices in future.
> If OS doesn't recognize the name, it uses predefined default timing
> settings.
>
> > this should be a compatible string for the particular module instead?
> > Then it could cover any differences, not just the FEM.
> >
> No, it is specific to FEM device.

You will be better off having either a specific compatible string or
using VID/PID as this is a PCI device. Then you can handle other
differences you may find between h/w versions without a DT change.

> >>
> >>  Example (to supply the calibration data alone):
> >>
> >> @@ -150,3 +151,24 @@ wifi@18000000 {
> >>                         <0 141 0 /* CE11 */ >;
> >>              vdd-0.8-cx-mx-supply = <&pm8998_l5>;
> >>  };
> >> +
> >> +Example (to supply the external front end module name):
> >> +
> >> +In this example, the front end module is defined as a property of the
> >> ath10k
> >> +device node.
> >
> > Really need a whole new example for 1 property?
> Will add this property in existing example.
>
> >
> >> +
> >> +pci {
> >> +    pcie@0 {
> >> +            reg = <0 0 0 0 0>;
> >> +            #interrupt-cells = <1>;
> >> +            #size-cells = <2>;
> >> +            #address-cells = <3>;
> >> +            device_type = "pci";
> >> +
> >> +            ath10k@0,0 {
> >
> > wifi@0,0
> Added in ath10k@0,0 to make consistent with existing property
> "qcom,ath10k-calibration-data" below,
> "

I don't see how that matters.

> pci {
>          pcie@0 {
>                  reg = <0 0 0 0 0>;
>                  #interrupt-cells = <1>;
>                  #size-cells = <2>;
>                  #address-cells = <3>;
>                  device_type = "pci";
>
>                  ath10k@0,0 {
>                          reg = <0 0 0 0 0>;
>                          device_type = "pci";
>                          qcom,ath10k-calibration-data = [ 01 02 03 ... ];
>                  };
>          };
> };
> "
>
> If wifi@0,0 is preferable, then I will add new entry "wifi@0,0" and add
> ext-fem-name into it.

Node names are supposed to reflect the class of device, not the model.
See the DT spec for a list.

>
> >
> >> +                    reg = <0 0 0 0 0>;
> >> +                    device_type = "pci";

Also, this is wrong. Only PCI bridges should have this property. dtc
will give you a warning on this.

> >> +                    ext-fem-name = "microsemi-lx5586";
> >> +            };
> >> +    };
> >> +};
> >> --
> >> 1.9.1
> >>
>
> Thanks, Sorry for the delayed response,
> Bhagavathi Perumal S.

Re: [PATCH 1/2] dt: bindings: add new dt entry to indentify external FEM

[Bhagavathi Perumal S]

On 2018-10-30 21:02, Rob Herring wrote:
> On Tue, Oct 30, 2018 at 3:41 AM <bperumal@codeaurora.org> wrote:
>> 
>> On 2018-10-16 00:54, Rob Herring wrote:
>> > On Thu, Oct 04, 2018 at 05:12:43PM +0530, Bhagavathi Perumal S wrote:
>> >> This adds new dt entry ext-fem-name, it is used by ath10k driver
>> >> to select correct timing parameters and configure it in target wifi
>> >> hardware.
>> >> The Front End Module(FEM) normally includes tx power amplifier(PA) and
>> >> rx low noise amplifier(LNA). The default timing parameters like tx end
>> >> to
>> >> PA off timing values were fine tuned for internal FEM used in
>> >> reference
>> >> design. And these timing values can not be same if ODM modifies
>> >> hardware
>> >> design with different external FEM. This DT entry helps to choose
>> >> correct
>> >> timing values in driver if different external FEM hardware used.
>> >
>> > 'dt-bindings: net: ath10k: ...' for the subject please.
>> Sure, I will change and send v2 patch.
>> 
>> >
>> >>
>> >> Signed-off-by: Bhagavathi Perumal S <bperumal@codeaurora.org>
>> >> ---
>> >>  .../bindings/net/wireless/qcom,ath10k.txt          | 22
>> >> ++++++++++++++++++++++
>> >>  1 file changed, 22 insertions(+)
>> >>
>> >> diff --git
>> >> a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
>> >> b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
>> >> index 7fd4e8c..fbaf309 100644
>> >> --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
>> >> +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
>> >> @@ -56,6 +56,7 @@ Optional properties:
>> >>                                   the length can vary between hw versions.
>> >>  - <supply-name>-supply: handle to the regulator device tree node
>> >>                         optional "supply-name" is "vdd-0.8-cx-mx".
>> >> +- ext-fem-name: name of external front end module used.
>> >
>> > What are valid names? What if the OS doesn't recognize the name?
>> > Perhaps
>> Some valid FEM devices are "microsemi-lx5586", "sky85703-11" and
>> "sky85803" etc.
>> Currently driver accepts only "microsemi-lx5586", Since this FEM
>> requires different timing settings,
> 
> Assuming you keep this, then you need to enumerate what are valid
> values in the binding. Otherwise, 'ext-fem-name = "rob"' is valid.

Sure, Will enumerate valid values in v2 patch.
> 
>> And it can be extended for other FEM devices in future.
>> If OS doesn't recognize the name, it uses predefined default timing
>> settings.
>> 
>> > this should be a compatible string for the particular module instead?
>> > Then it could cover any differences, not just the FEM.
>> >
>> No, it is specific to FEM device.
> 
> You will be better off having either a specific compatible string or
> using VID/PID as this is a PCI device. Then you can handle other
> differences you may find between h/w versions without a DT change.
> 

FEM is a part of wifi device, and not a separate PCI device. Different 
external
FEM can be used in a same wifi device, so it might not be feasible to 
define
a compatible string to cover any other differences. IMO we can go with
the property name instead of defining compatible string specific to
every FEM device.

>> >>
>> >>  Example (to supply the calibration data alone):
>> >>
>> >> @@ -150,3 +151,24 @@ wifi@18000000 {
>> >>                         <0 141 0 /* CE11 */ >;
>> >>              vdd-0.8-cx-mx-supply = <&pm8998_l5>;
>> >>  };
>> >> +
>> >> +Example (to supply the external front end module name):
>> >> +
>> >> +In this example, the front end module is defined as a property of the
>> >> ath10k
>> >> +device node.
>> >
>> > Really need a whole new example for 1 property?
>> Will add this property in existing example.
>> 
>> >
>> >> +
>> >> +pci {
>> >> +    pcie@0 {
>> >> +            reg = <0 0 0 0 0>;
>> >> +            #interrupt-cells = <1>;
>> >> +            #size-cells = <2>;
>> >> +            #address-cells = <3>;
>> >> +            device_type = "pci";
>> >> +
>> >> +            ath10k@0,0 {
>> >
>> > wifi@0,0
>> Added in ath10k@0,0 to make consistent with existing property
>> "qcom,ath10k-calibration-data" below,
>> "
> 
> I don't see how that matters.
> 
>> pci {
>>          pcie@0 {
>>                  reg = <0 0 0 0 0>;
>>                  #interrupt-cells = <1>;
>>                  #size-cells = <2>;
>>                  #address-cells = <3>;
>>                  device_type = "pci";
>> 
>>                  ath10k@0,0 {
>>                          reg = <0 0 0 0 0>;
>>                          device_type = "pci";
>>                          qcom,ath10k-calibration-data = [ 01 02 03 ... 
>> ];
>>                  };
>>          };
>> };
>> "
>> 
>> If wifi@0,0 is preferable, then I will add new entry "wifi@0,0" and 
>> add
>> ext-fem-name into it.
> 
> Node names are supposed to reflect the class of device, not the model.
> See the DT spec for a list.

Will change accordingly.

> 
>> 
>> >
>> >> +                    reg = <0 0 0 0 0>;
>> >> +                    device_type = "pci";
> 
> Also, this is wrong. Only PCI bridges should have this property. dtc
> will give you a warning on this.

Will remove it in v2 patch.

> 
>> >> +                    ext-fem-name = "microsemi-lx5586";
>> >> +            };
>> >> +    };
>> >> +};
>> >> --
>> >> 1.9.1
>> >>
>> 
>> Thanks, Sorry for the delayed response,
>> Bhagavathi Perumal S.