[PATCH bluetooth-next 1/3] ieee802154: add set transmit power support

[PATCH bluetooth-next 1/3] ieee802154: add set transmit power support

[Varka Bhadram]

This patch adds transmission power setting support to nl802154.

Signed-off-by: Varka Bhadram <varkab@cdac.in>
---
 include/net/cfg802154.h   |    1 +
 net/ieee802154/nl802154.c |   20 ++++++++++++++++++++
 net/ieee802154/rdev-ops.h |    7 +++++++
 net/mac802154/cfg.c       |   19 +++++++++++++++++++
 4 files changed, 47 insertions(+)

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index eeda676..b163d4e 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -57,6 +57,7 @@ struct cfg802154_ops {
 					 s8 max_frame_retries);
 	int	(*set_lbt_mode)(struct wpan_phy *wpan_phy,
 				struct wpan_dev *wpan_dev, bool mode);
+	int	(*set_tx_power)(struct wpan_phy *wpan_phy, s8 power);
 };
 
 struct wpan_phy_cca {
diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
index a4daf91..8288fcb 100644
--- a/net/ieee802154/nl802154.c
+++ b/net/ieee802154/nl802154.c
@@ -794,6 +794,18 @@ static int nl802154_set_lbt_mode(struct sk_buff *skb, struct genl_info *info)
 	return rdev_set_lbt_mode(rdev, wpan_dev, mode);
 }
 
+static int nl802154_set_tx_power(struct sk_buff *skb, struct genl_info *info)
+{
+	struct cfg802154_registered_device *rdev = info->user_ptr[0];
+	s8 power;
+
+	if (!info->attrs[NL802154_ATTR_TX_POWER])
+		return -EINVAL;
+
+	power = nla_get_s8(info->attrs[NL802154_ATTR_TX_POWER]);
+	return rdev_set_tx_power(rdev, power);
+}
+
 #define NL802154_FLAG_NEED_WPAN_PHY	0x01
 #define NL802154_FLAG_NEED_NETDEV	0x02
 #define NL802154_FLAG_NEED_RTNL		0x04
@@ -984,6 +996,14 @@ static const struct genl_ops nl802154_ops[] = {
 		.internal_flags = NL802154_FLAG_NEED_NETDEV |
 				  NL802154_FLAG_NEED_RTNL,
 	},
+	{
+		.cmd = NL802154_CMD_SET_TX_POWER,
+		.doit = nl802154_set_tx_power,
+		.policy = nl802154_policy,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = NL802154_FLAG_NEED_WPAN_PHY |
+				  NL802154_FLAG_NEED_RTNL,
+	},
 };
 
 /* initialisation/exit functions */
diff --git a/net/ieee802154/rdev-ops.h b/net/ieee802154/rdev-ops.h
index 7c46732..3de05a8 100644
--- a/net/ieee802154/rdev-ops.h
+++ b/net/ieee802154/rdev-ops.h
@@ -91,6 +91,13 @@ rdev_set_lbt_mode(struct cfg802154_registered_device *rdev,
 		  struct wpan_dev *wpan_dev, bool mode)
 {
 	return rdev->ops->set_lbt_mode(&rdev->wpan_phy, wpan_dev, mode);
+
 }
 
+static inline int
+rdev_set_tx_power(struct cfg802154_registered_device *rdev,
+		  u8 power)
+{
+	return rdev->ops->set_tx_power(&rdev->wpan_phy, power);
+}
 #endif /* __CFG802154_RDEV_OPS */
diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
index 5d9f68c..dde26f1 100644
--- a/net/mac802154/cfg.c
+++ b/net/mac802154/cfg.c
@@ -212,6 +212,24 @@ ieee802154_set_lbt_mode(struct wpan_phy *wpan_phy, struct wpan_dev *wpan_dev,
 	return 0;
 }
 
+static int
+ieee802154_set_tx_power(struct wpan_phy *wpan_phy, s8 power)
+{
+	struct ieee802154_local *local = wpan_phy_priv(wpan_phy);
+	int ret;
+
+	ASSERT_RTNL();
+
+	if (!(local->hw.flags & IEEE802154_HW_TXPOWER))
+		return -EOPNOTSUPP;
+
+	ret = drv_set_tx_power(local, power);
+	if (!ret)
+		wpan_phy->transmit_power = power;
+
+	return ret;
+}
+
 const struct cfg802154_ops mac802154_config_ops = {
 	.add_virtual_intf_deprecated = ieee802154_add_iface_deprecated,
 	.del_virtual_intf_deprecated = ieee802154_del_iface_deprecated,
@@ -225,4 +243,5 @@ const struct cfg802154_ops mac802154_config_ops = {
 	.set_max_csma_backoffs = ieee802154_set_max_csma_backoffs,
 	.set_max_frame_retries = ieee802154_set_max_frame_retries,
 	.set_lbt_mode = ieee802154_set_lbt_mode,
+	.set_tx_power = ieee802154_set_tx_power,
 };
-- 
1.7.9.5

[PATCH bluetooth-next 2/3] cc2520: add set transmit power support

[Varka Bhadram]

Signed-off-by: Varka Bhadram <varkab@cdac.in>
---
 drivers/net/ieee802154/cc2520.c |   56 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
index f833b8b..f96cc50 100644
--- a/drivers/net/ieee802154/cc2520.c
+++ b/drivers/net/ieee802154/cc2520.c
@@ -53,6 +53,17 @@
 #define	CC2520_MAXCHANNEL		26
 #define	CC2520_CHANNEL_SPACING		5
 
+/* Tx power values */
+#define CC2520_TXPOWER_0		0x03 /* -18dbm */
+#define CC2520_TXPOWER_1		0x2c /* -7dbm */
+#define CC2520_TXPOWER_2		0x88 /* -4dbm */
+#define CC2520_TXPOWER_3		0x81 /* -2dbm */
+#define CC2520_TXPOWER_4		0x32 /* 0dbm */
+#define CC2520_TXPOWER_5		0x13 /* 1dbm */
+#define CC2520_TXPOWER_6		0xab /* 2dbm */
+#define CC2520_TXPOWER_7		0xf2 /* 3dbm */
+#define CC2520_TXPOWER_8		0xf7 /* 5dbm */
+
 /* command strobes */
 #define	CC2520_CMD_SNOP			0x00
 #define	CC2520_CMD_IBUFLD		0x02
@@ -628,6 +639,48 @@ cc2520_filter(struct ieee802154_hw *hw,
 	return 0;
 }
 
+static int
+cc2520_set_txpower(struct ieee802154_hw *hw, int db)
+{
+	struct cc2520_private *priv = hw->priv;
+	u8 power;
+
+	switch (db) {
+	case 5:
+		power = CC2520_TXPOWER_8;
+		break;
+	case 3:
+		power = CC2520_TXPOWER_7;
+		break;
+	case 2:
+		power = CC2520_TXPOWER_6;
+		break;
+	case 1:
+		power = CC2520_TXPOWER_5;
+		break;
+	case 0:
+		power = CC2520_TXPOWER_4;
+		break;
+	case -2:
+		power = CC2520_TXPOWER_3;
+		break;
+	case -4:
+		power = CC2520_TXPOWER_2;
+		break;
+	case -7:
+		power = CC2520_TXPOWER_1;
+		break;
+	case -18:
+		power = CC2520_TXPOWER_0;
+		break;
+	default:
+		dev_err(&priv->spi->dev, "invalid tx power setting\n");
+		return -EINVAL;
+	}
+
+	return cc2520_write_register(priv, CC2520_TXPOWER, power);
+}
+
 static const struct ieee802154_ops cc2520_ops = {
 	.owner = THIS_MODULE,
 	.start = cc2520_start,
@@ -636,6 +689,7 @@ static const struct ieee802154_ops cc2520_ops = {
 	.ed = cc2520_ed,
 	.set_channel = cc2520_set_channel,
 	.set_hw_addr_filt = cc2520_filter,
+	.set_txpower = cc2520_set_txpower,
 };
 
 static int cc2520_register(struct cc2520_private *priv)
@@ -655,7 +709,7 @@ static int cc2520_register(struct cc2520_private *priv)
 	/* We do support only 2.4 Ghz */
 	priv->hw->phy->channels_supported[0] = 0x7FFF800;
 	priv->hw->flags = IEEE802154_HW_OMIT_CKSUM | IEEE802154_HW_AACK |
-			  IEEE802154_HW_AFILT;
+			  IEEE802154_HW_AFILT | IEEE802154_HW_TXPOWER;
 
 	dev_vdbg(&priv->spi->dev, "registered cc2520\n");
 	ret = ieee802154_register_hw(priv->hw);
-- 
1.7.9.5

[PATCH bluetooth-next 3/3] cc2520: fix in updated register settings

[Varka Bhadram]

This patch fix the updated register settings for transmit power.

As per the datasheet [section 28.1 Register Settings Update]
the transmit power register value has to be updated to 0x32 (0 dBm)
from the default value.

CC2520_TXPOWER_4 indicates the value 0x32 (0 dBm).

Signed-off-by: Varka Bhadram <varkab@cdac.in>
---
 drivers/net/ieee802154/cc2520.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
index f96cc50..d9135ec 100644
--- a/drivers/net/ieee802154/cc2520.c
+++ b/drivers/net/ieee802154/cc2520.c
@@ -860,7 +860,8 @@ static int cc2520_hw_init(struct cc2520_private *priv)
 		if (ret)
 			goto err_ret;
 	} else {
-		ret = cc2520_write_register(priv, CC2520_TXPOWER, 0xF7);
+		ret = cc2520_write_register(priv, CC2520_TXPOWER,
+					    CC2520_TXPOWER_4);
 		if (ret)
 			goto err_ret;
 
-- 
1.7.9.5

Re: [PATCH bluetooth-next 2/3] cc2520: add set transmit power support

[Stefan Schmidt]

Hello.

On 20/03/15 08:22, Varka Bhadram wrote:
> Signed-off-by: Varka Bhadram <varkab@cdac.in>
> ---
>   drivers/net/ieee802154/cc2520.c |   56
> ++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ieee802154/cc2520.c
> b/drivers/net/ieee802154/cc2520.c
> index f833b8b..f96cc50 100644
> --- a/drivers/net/ieee802154/cc2520.c
> +++ b/drivers/net/ieee802154/cc2520.c
> @@ -53,6 +53,17 @@
>   #define	CC2520_MAXCHANNEL		26
>   #define	CC2520_CHANNEL_SPACING		5
>
> +/* Tx power values */
> +#define CC2520_TXPOWER_0		0x03 /* -18dbm */
> +#define CC2520_TXPOWER_1		0x2c /* -7dbm */
> +#define CC2520_TXPOWER_2		0x88 /* -4dbm */
> +#define CC2520_TXPOWER_3		0x81 /* -2dbm */
> +#define CC2520_TXPOWER_4		0x32 /* 0dbm */
> +#define CC2520_TXPOWER_5		0x13 /* 1dbm */
> +#define CC2520_TXPOWER_6		0xab /* 2dbm */
> +#define CC2520_TXPOWER_7		0xf2 /* 3dbm */
> +#define CC2520_TXPOWER_8		0xf7 /* 5dbm */
> +
>   /* command strobes */
>   #define	CC2520_CMD_SNOP			0x00
>   #define	CC2520_CMD_IBUFLD		0x02
> @@ -628,6 +639,48 @@ cc2520_filter(struct ieee802154_hw *hw,
>   	return 0;
>   }
>
> +static int
> +cc2520_set_txpower(struct ieee802154_hw *hw, int db)
> +{
> +	struct cc2520_private *priv = hw->priv;
> +	u8 power;
> +
> +	switch (db) {
> +	case 5:
> +		power = CC2520_TXPOWER_8;
> +		break;
> +	case 3:
> +		power = CC2520_TXPOWER_7;
> +		break;
> +	case 2:
> +		power = CC2520_TXPOWER_6;
> +		break;
> +	case 1:
> +		power = CC2520_TXPOWER_5;
> +		break;
> +	case 0:
> +		power = CC2520_TXPOWER_4;
> +		break;
> +	case -2:
> +		power = CC2520_TXPOWER_3;
> +		break;
> +	case -4:
> +		power = CC2520_TXPOWER_2;
> +		break;
> +	case -7:
> +		power = CC2520_TXPOWER_1;
> +		break;
> +	case -18:
> +		power = CC2520_TXPOWER_0;
> +		break;
> +	default:
> +		dev_err(&priv->spi->dev, "invalid tx power setting\n");
> +		return -EINVAL;
> +	}
> +
> +	return cc2520_write_register(priv, CC2520_TXPOWER, power);
> +}
> +

Thanks for working on the power setting API.

One thing I find problematic here is that the user has to know the 
values it can set for the db level beforehand and these values can 
change for different transceivers. Which makes these nl call hardware 
depended.

We should at least have a way to query what db levels are available or 
better see if we can harmonize the setting over different drivers and 
transceivers.

regards
Stefan Schmidt

Re: [PATCH bluetooth-next 1/3] ieee802154: add set transmit power support

[Alexander Aring]

Hi,

there exist _maybe_ some general issues with the current interface and the
tx_power handling:

1.
   The at86rf2xx has also settings for a more fine granularity tx power
   setting. For example the at86rf233 has these values:
     - 4 dBm
     - 3.7 dBm
     - 3.4 dBm
     - 3 dBm
     - 2.5 dBm
     - 2 dBm
     ...
   I don't know if we should simple "round-down" and use the nearest
   value in this case. For the moment I am fine to handle the nearest value.

2.
   It seems that, for example the at86rf212 [1] which operates in
   700, 800 or 900 Mhz, the tx_power setting depends on the current setting of
   page and channel. I believe we can completely handle this inside the
   driver layer, but I am not 100% sure.

btw:
   What I am know is that the set_txpower driver callback in at86rf230
   need to decide if at86rf233, at86rf231 or at86rf212 and making some
   special handling. Especially the at86rf212 needs to check the current
   page and channel setting, because the register values differs here.
   Current behaviour is broken.


On Fri, Mar 20, 2015 at 12:52:18PM +0530, Varka Bhadram wrote:
> This patch adds transmission power setting support to nl802154.
> 
> Signed-off-by: Varka Bhadram <varkab@cdac.in>
> ---
>  include/net/cfg802154.h   |    1 +
>  net/ieee802154/nl802154.c |   20 ++++++++++++++++++++
>  net/ieee802154/rdev-ops.h |    7 +++++++
>  net/mac802154/cfg.c       |   19 +++++++++++++++++++
>  4 files changed, 47 insertions(+)
> 
> diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> index eeda676..b163d4e 100644
> --- a/include/net/cfg802154.h
> +++ b/include/net/cfg802154.h
> @@ -57,6 +57,7 @@ struct cfg802154_ops {
>  					 s8 max_frame_retries);
>  	int	(*set_lbt_mode)(struct wpan_phy *wpan_phy,
>  				struct wpan_dev *wpan_dev, bool mode);
> +	int	(*set_tx_power)(struct wpan_phy *wpan_phy, s8 power);

put this into the sequence of the others phy settings like channel,
page. cca. etc...

>  };
>  
>  struct wpan_phy_cca {
> diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
> index a4daf91..8288fcb 100644
> --- a/net/ieee802154/nl802154.c
> +++ b/net/ieee802154/nl802154.c
> @@ -794,6 +794,18 @@ static int nl802154_set_lbt_mode(struct sk_buff *skb, struct genl_info *info)
>  	return rdev_set_lbt_mode(rdev, wpan_dev, mode);
>  }
>  
> +static int nl802154_set_tx_power(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct cfg802154_registered_device *rdev = info->user_ptr[0];
> +	s8 power;
> +
> +	if (!info->attrs[NL802154_ATTR_TX_POWER])
> +		return -EINVAL;
> +
> +	power = nla_get_s8(info->attrs[NL802154_ATTR_TX_POWER]);
> +	return rdev_set_tx_power(rdev, power);
> +}
> +
>  #define NL802154_FLAG_NEED_WPAN_PHY	0x01
>  #define NL802154_FLAG_NEED_NETDEV	0x02
>  #define NL802154_FLAG_NEED_RTNL		0x04
> @@ -984,6 +996,14 @@ static const struct genl_ops nl802154_ops[] = {
>  		.internal_flags = NL802154_FLAG_NEED_NETDEV |
>  				  NL802154_FLAG_NEED_RTNL,
>  	},
> +	{
> +		.cmd = NL802154_CMD_SET_TX_POWER,
> +		.doit = nl802154_set_tx_power,
> +		.policy = nl802154_policy,
> +		.flags = GENL_ADMIN_PERM,
> +		.internal_flags = NL802154_FLAG_NEED_WPAN_PHY |
> +				  NL802154_FLAG_NEED_RTNL,
> +	},
>  };
>  

I posted ~4 days ago [0] a series which makes this kind of set cmd
obsolete. We should not introduce new commands if we decide to set phy
settings with only one cmd only. I will create a new thread about which
are the advantages and disadvantage about the new setting commands, then
we can decide there if we still use the old interface or switch to the
new behaviour. If we decide to use the new interface then you can rebase
your work on it.

>  /* initialisation/exit functions */
> diff --git a/net/ieee802154/rdev-ops.h b/net/ieee802154/rdev-ops.h
> index 7c46732..3de05a8 100644
> --- a/net/ieee802154/rdev-ops.h
> +++ b/net/ieee802154/rdev-ops.h
> @@ -91,6 +91,13 @@ rdev_set_lbt_mode(struct cfg802154_registered_device *rdev,
>  		  struct wpan_dev *wpan_dev, bool mode)
>  {
>  	return rdev->ops->set_lbt_mode(&rdev->wpan_phy, wpan_dev, mode);
> +

remove this whitespace.

>  }
>  
> +static inline int
> +rdev_set_tx_power(struct cfg802154_registered_device *rdev,
> +		  u8 power)

s/u8/s8/

> +{
> +	return rdev->ops->set_tx_power(&rdev->wpan_phy, power);
> +}

also move this to the other phy rdev-ops wrappers.

>  #endif /* __CFG802154_RDEV_OPS */
> diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
> index 5d9f68c..dde26f1 100644
> --- a/net/mac802154/cfg.c
> +++ b/net/mac802154/cfg.c
> @@ -212,6 +212,24 @@ ieee802154_set_lbt_mode(struct wpan_phy *wpan_phy, struct wpan_dev *wpan_dev,
>  	return 0;
>  }
>  
> +static int
> +ieee802154_set_tx_power(struct wpan_phy *wpan_phy, s8 power)
> +{
> +	struct ieee802154_local *local = wpan_phy_priv(wpan_phy);
> +	int ret;
> +
> +	ASSERT_RTNL();
> +
> +	if (!(local->hw.flags & IEEE802154_HW_TXPOWER))
> +		return -EOPNOTSUPP;
> +
> +	ret = drv_set_tx_power(local, power);
> +	if (!ret)
> +		wpan_phy->transmit_power = power;
> +
> +	return ret;
> +}

also move this to the other phy handlers.

> +
>  const struct cfg802154_ops mac802154_config_ops = {
>  	.add_virtual_intf_deprecated = ieee802154_add_iface_deprecated,
>  	.del_virtual_intf_deprecated = ieee802154_del_iface_deprecated,
> @@ -225,4 +243,5 @@ const struct cfg802154_ops mac802154_config_ops = {
>  	.set_max_csma_backoffs = ieee802154_set_max_csma_backoffs,
>  	.set_max_frame_retries = ieee802154_set_max_frame_retries,
>  	.set_lbt_mode = ieee802154_set_lbt_mode,
> +	.set_tx_power = ieee802154_set_tx_power,

same here.

- Alex

[0] http://www.spinics.net/lists/linux-wpan/msg01550.html
[1] http://www.atmel.com/images/doc8168.pdf

Re: [PATCH bluetooth-next 2/3] cc2520: add set transmit power support

[Alexander Aring]

Hi,

On Fri, Mar 20, 2015 at 12:52:19PM +0530, Varka Bhadram wrote:
> Signed-off-by: Varka Bhadram <varkab@cdac.in>
> ---
>  drivers/net/ieee802154/cc2520.c |   56 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
> index f833b8b..f96cc50 100644
> --- a/drivers/net/ieee802154/cc2520.c
> +++ b/drivers/net/ieee802154/cc2520.c
> @@ -53,6 +53,17 @@
>  #define	CC2520_MAXCHANNEL		26
>  #define	CC2520_CHANNEL_SPACING		5
>  
> +/* Tx power values */
> +#define CC2520_TXPOWER_0		0x03 /* -18dbm */
> +#define CC2520_TXPOWER_1		0x2c /* -7dbm */
> +#define CC2520_TXPOWER_2		0x88 /* -4dbm */
> +#define CC2520_TXPOWER_3		0x81 /* -2dbm */
> +#define CC2520_TXPOWER_4		0x32 /* 0dbm */
> +#define CC2520_TXPOWER_5		0x13 /* 1dbm */
> +#define CC2520_TXPOWER_6		0xab /* 2dbm */
> +#define CC2520_TXPOWER_7		0xf2 /* 3dbm */
> +#define CC2520_TXPOWER_8		0xf7 /* 5dbm */
> +
>  /* command strobes */
>  #define	CC2520_CMD_SNOP			0x00
>  #define	CC2520_CMD_IBUFLD		0x02
> @@ -628,6 +639,48 @@ cc2520_filter(struct ieee802154_hw *hw,
>  	return 0;
>  }
>  
> +static int
> +cc2520_set_txpower(struct ieee802154_hw *hw, int db)
> +{

our netlink attribute is s8, maybe we should fix the mac layer at first
and change the int to s8.

- Alex

Re: [PATCH bluetooth-next 3/3] cc2520: fix in updated register settings

[Alexander Aring]

Hi,

On Fri, Mar 20, 2015 at 12:52:20PM +0530, Varka Bhadram wrote:
> This patch fix the updated register settings for transmit power.
> 
> As per the datasheet [section 28.1 Register Settings Update]
> the transmit power register value has to be updated to 0x32 (0 dBm)
> from the default value.
> 
> CC2520_TXPOWER_4 indicates the value 0x32 (0 dBm).
> 
> Signed-off-by: Varka Bhadram <varkab@cdac.in>
> ---
>  drivers/net/ieee802154/cc2520.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
> index f96cc50..d9135ec 100644
> --- a/drivers/net/ieee802154/cc2520.c
> +++ b/drivers/net/ieee802154/cc2520.c
> @@ -860,7 +860,8 @@ static int cc2520_hw_init(struct cc2520_private *priv)
>  		if (ret)
>  			goto err_ret;
>  	} else {
> -		ret = cc2520_write_register(priv, CC2520_TXPOWER, 0xF7);
> +		ret = cc2520_write_register(priv, CC2520_TXPOWER,
> +					    CC2520_TXPOWER_4);
>  		if (ret)
>  			goto err_ret;

you also need to set _always_ the default phy value after reset in the
wpan_phy struct.

Example:

/* sets default transmit_power */
foobar->hw->phy->transmit_power = 4;


otherwise the phy layer doesn't know the initial txpower. In cc2520
there is also a missing initial value for the CCA mode. There must be
running some CCA mode which also stands in the datasheet (I doesn't
looked into that now).

Example:

foobar->hw->phy->cca.mode = NL802154_CCA_ENERGY_CARRIER;


btw:
We have also there a lack of support because this describes energy
above threshold. Currently we have not implemented to get this threshold
from the phy. (We also need support for that, in case of at86rf230 the
default value is used).

- Alex

Re: [PATCH bluetooth-next 2/3] cc2520: add set transmit power support

[Alexander Aring]

On Fri, Mar 20, 2015 at 09:19:56AM +0000, Stefan Schmidt wrote:
> Hello.
> 
> On 20/03/15 08:22, Varka Bhadram wrote:
> >Signed-off-by: Varka Bhadram <varkab@cdac.in>
> >---
> >  drivers/net/ieee802154/cc2520.c |   56
> >++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 55 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/net/ieee802154/cc2520.c
> >b/drivers/net/ieee802154/cc2520.c
> >index f833b8b..f96cc50 100644
> >--- a/drivers/net/ieee802154/cc2520.c
> >+++ b/drivers/net/ieee802154/cc2520.c
> >@@ -53,6 +53,17 @@
> >  #define	CC2520_MAXCHANNEL		26
> >  #define	CC2520_CHANNEL_SPACING		5
> >

...

> >+}
> >+
> 
> Thanks for working on the power setting API.
> 

agree.

> One thing I find problematic here is that the user has to know the values it
> can set for the db level beforehand and these values can change for
> different transceivers. Which makes these nl call hardware depended.
> 
> We should at least have a way to query what db levels are available or
> better see if we can harmonize the setting over different drivers and
> transceivers.
> 

agree, here too. But we also need that for the current others phy
settings like cca modes. There exist also MAC settings like
MIN_BE/MAX_BE (csma backoffs) which cannot support every phy. (I also
saw transceivers like mrf24j40 [0] which can set MIN_BE only. I suppose
that MAX_BE is always be 802.15.4 default).

Back to the txpower setting, what I previous mention in another mail is
that for transceivers which supports mutliple bands/modulations the supported
txpower dbm values differs. For example the at86rf212 [1]. This means
that we need some mechanism to handle that.

I imagine the following scenario.

1. Somebody wants to know all txpower settings for all bands.

   Then we need some big array which is filled at probe time.



2. Somebody wants to know all txpower settings of the current
   page/channel setting.

   This would be much easier, I found two solutions:

     1. add some supported_txpower array, which is updated while channel
        /page setting. (Also for reset values while probing).

     2. have a driver callback which will fill some dbm values and the driver
        decide of the current channel/page settings how this array is filled.

   I would prefer the 1. solution because that's similar like the PHY
   PIB handles the channels_supported array, then we have something like
   txpower_supported.


The point is we can support such feature later, currently the only one
way is to do try and error and look if -EINVAL is returned. :D

btw:
All phy settings are direct calls to the driver layer, but for mac
settings this is more problematic, because we only can decide if the phy
supports the setting on an interface up. This sucks! This means you set
some value which is 802.15.4 complaint and is successful on interface
down. Then an interface up will return -EINVAL and you don't know why.
:-/

That's why we need also something like "csma_backoffs_supported" thing
(array/flag/I don't know, better flags) and check this while setting
the backoffs while interface down.

The good thing is, we don't have such transceiver currently. The
at86rf230 supports all 802.15.4 values and (sometimes) beyond.

All MAC settings which is handled by phy need such checking and
supported array.

- Alex

[0] http://ww1.microchip.com/downloads/en/DeviceDoc/39776C.pdf
[1] http://www.atmel.com/images/doc8168.pdf

Re: [PATCH bluetooth-next 1/3] ieee802154: add set transmit power support

[Varka Bhadram]

Hi Alex,

On 03/20/2015 08:58 PM, Alexander Aring wrote:

> Hi,
>
> there exist _maybe_ some general issues with the current interface and the
> tx_power handling:
>
> 1.
>     The at86rf2xx has also settings for a more fine granularity tx power
>     setting. For example the at86rf233 has these values:
>       - 4 dBm
>       - 3.7 dBm
>       - 3.4 dBm
>       - 3 dBm
>       - 2.5 dBm
>       - 2 dBm
>       ...
>     I don't know if we should simple "round-down" and use the nearest
>     value in this case. For the moment I am fine to handle the nearest value.
>
> 2.
>     It seems that, for example the at86rf212 [1] which operates in
>     700, 800 or 900 Mhz, the tx_power setting depends on the current setting of
>     page and channel. I believe we can completely handle this inside the
>     driver layer, but I am not 100% sure.
>
> btw:
>     What I am know is that the set_txpower driver callback in at86rf230
>     need to decide if at86rf233, at86rf231 or at86rf212 and making some
>     special handling. Especially the at86rf212 needs to check the current
>     page and channel setting, because the register values differs here.
>     Current behaviour is broken.
>
>
> On Fri, Mar 20, 2015 at 12:52:18PM +0530, Varka Bhadram wrote:
>> This patch adds transmission power setting support to nl802154.
>>
>> Signed-off-by: Varka Bhadram <varkab@cdac.in>
>> ---
>>   include/net/cfg802154.h   |    1 +
>>   net/ieee802154/nl802154.c |   20 ++++++++++++++++++++
>>   net/ieee802154/rdev-ops.h |    7 +++++++
>>   net/mac802154/cfg.c       |   19 +++++++++++++++++++
>>   4 files changed, 47 insertions(+)
>>
>> diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
>> index eeda676..b163d4e 100644
>> --- a/include/net/cfg802154.h
>> +++ b/include/net/cfg802154.h
>> @@ -57,6 +57,7 @@ struct cfg802154_ops {
>>   					 s8 max_frame_retries);
>>   	int	(*set_lbt_mode)(struct wpan_phy *wpan_phy,
>>   				struct wpan_dev *wpan_dev, bool mode);
>> +	int	(*set_tx_power)(struct wpan_phy *wpan_phy, s8 power);
> put this into the sequence of the others phy settings like channel,
> page. cca. etc...
>
Sure I will do..

>>   };
>>   
>>   struct wpan_phy_cca {
>> diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
>> index a4daf91..8288fcb 100644
>> --- a/net/ieee802154/nl802154.c
>> +++ b/net/ieee802154/nl802154.c
>> @@ -794,6 +794,18 @@ static int nl802154_set_lbt_mode(struct sk_buff *skb, struct genl_info *info)
>>   	return rdev_set_lbt_mode(rdev, wpan_dev, mode);
>>   }
>>   
>> +static int nl802154_set_tx_power(struct sk_buff *skb, struct genl_info *info)
>> +{
>> +	struct cfg802154_registered_device *rdev = info->user_ptr[0];
>> +	s8 power;
>> +
>> +	if (!info->attrs[NL802154_ATTR_TX_POWER])
>> +		return -EINVAL;
>> +
>> +	power = nla_get_s8(info->attrs[NL802154_ATTR_TX_POWER]);
>> +	return rdev_set_tx_power(rdev, power);
>> +}
>> +
>>   #define NL802154_FLAG_NEED_WPAN_PHY	0x01
>>   #define NL802154_FLAG_NEED_NETDEV	0x02
>>   #define NL802154_FLAG_NEED_RTNL		0x04
>> @@ -984,6 +996,14 @@ static const struct genl_ops nl802154_ops[] = {
>>   		.internal_flags = NL802154_FLAG_NEED_NETDEV |
>>   				  NL802154_FLAG_NEED_RTNL,
>>   	},
>> +	{
>> +		.cmd = NL802154_CMD_SET_TX_POWER,
>> +		.doit = nl802154_set_tx_power,
>> +		.policy = nl802154_policy,
>> +		.flags = GENL_ADMIN_PERM,
>> +		.internal_flags = NL802154_FLAG_NEED_WPAN_PHY |
>> +				  NL802154_FLAG_NEED_RTNL,
>> +	},
>>   };
>>   
> I posted ~4 days ago [0] a series which makes this kind of set cmd
> obsolete. We should not introduce new commands if we decide to set phy
> settings with only one cmd only. I will create a new thread about which
> are the advantages and disadvantage about the new setting commands, then
> we can decide there if we still use the old interface or switch to the
> new behaviour. If we decide to use the new interface then you can rebase
> your work on it.

First i will send the series, after review if the series is fine i will rebase it.

>>   /* initialisation/exit functions */
>> diff --git a/net/ieee802154/rdev-ops.h b/net/ieee802154/rdev-ops.h
>> index 7c46732..3de05a8 100644
>> --- a/net/ieee802154/rdev-ops.h
>> +++ b/net/ieee802154/rdev-ops.h
>> @@ -91,6 +91,13 @@ rdev_set_lbt_mode(struct cfg802154_registered_device *rdev,
>>   		  struct wpan_dev *wpan_dev, bool mode)
>>   {
>>   	return rdev->ops->set_lbt_mode(&rdev->wpan_phy, wpan_dev, mode);
>> +
> remove this whitespace.
>
I don't how it came. Sorry for that. Will remove ie.

>>   }
>>   
>> +static inline int
>> +rdev_set_tx_power(struct cfg802154_registered_device *rdev,
>> +		  u8 power)
> s/u8/s8/

Sure

>
>> +{
>> +	return rdev->ops->set_tx_power(&rdev->wpan_phy, power);
>> +}
> also move this to the other phy rdev-ops wrappers.

Sure

>>   #endif /* __CFG802154_RDEV_OPS */
>> diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
>> index 5d9f68c..dde26f1 100644
>> --- a/net/mac802154/cfg.c
>> +++ b/net/mac802154/cfg.c
>> @@ -212,6 +212,24 @@ ieee802154_set_lbt_mode(struct wpan_phy *wpan_phy, struct wpan_dev *wpan_dev,
>>   	return 0;
>>   }
>>   
>> +static int
>> +ieee802154_set_tx_power(struct wpan_phy *wpan_phy, s8 power)
>> +{
>> +	struct ieee802154_local *local = wpan_phy_priv(wpan_phy);
>> +	int ret;
>> +
>> +	ASSERT_RTNL();
>> +
>> +	if (!(local->hw.flags & IEEE802154_HW_TXPOWER))
>> +		return -EOPNOTSUPP;
>> +
>> +	ret = drv_set_tx_power(local, power);
>> +	if (!ret)
>> +		wpan_phy->transmit_power = power;
>> +
>> +	return ret;
>> +}
> also move this to the other phy handlers.

Ok

>> +
>>   const struct cfg802154_ops mac802154_config_ops = {
>>   	.add_virtual_intf_deprecated = ieee802154_add_iface_deprecated,
>>   	.del_virtual_intf_deprecated = ieee802154_del_iface_deprecated,
>> @@ -225,4 +243,5 @@ const struct cfg802154_ops mac802154_config_ops = {
>>   	.set_max_csma_backoffs = ieee802154_set_max_csma_backoffs,
>>   	.set_max_frame_retries = ieee802154_set_max_frame_retries,
>>   	.set_lbt_mode = ieee802154_set_lbt_mode,
>> +	.set_tx_power = ieee802154_set_tx_power,
> same here.

Ok

> - Alex
>
> [0] http://www.spinics.net/lists/linux-wpan/msg01550.html
> [1] http://www.atmel.com/images/doc8168.pdf

Thanks.


-- 
Varka Bhadram

Re: [PATCH bluetooth-next 2/3] cc2520: add set transmit power support

[Varka Bhadram]

On 03/20/2015 09:00 PM, Alexander Aring wrote:
> Hi,
>
> On Fri, Mar 20, 2015 at 12:52:19PM +0530, Varka Bhadram wrote:
>> Signed-off-by: Varka Bhadram <varkab@cdac.in>
>> ---
>>   drivers/net/ieee802154/cc2520.c |   56 ++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
>> index f833b8b..f96cc50 100644
>> --- a/drivers/net/ieee802154/cc2520.c
>> +++ b/drivers/net/ieee802154/cc2520.c
>> @@ -53,6 +53,17 @@
>>   #define	CC2520_MAXCHANNEL		26
>>   #define	CC2520_CHANNEL_SPACING		5
>>   
>> +/* Tx power values */
>> +#define CC2520_TXPOWER_0		0x03 /* -18dbm */
>> +#define CC2520_TXPOWER_1		0x2c /* -7dbm */
>> +#define CC2520_TXPOWER_2		0x88 /* -4dbm */
>> +#define CC2520_TXPOWER_3		0x81 /* -2dbm */
>> +#define CC2520_TXPOWER_4		0x32 /* 0dbm */
>> +#define CC2520_TXPOWER_5		0x13 /* 1dbm */
>> +#define CC2520_TXPOWER_6		0xab /* 2dbm */
>> +#define CC2520_TXPOWER_7		0xf2 /* 3dbm */
>> +#define CC2520_TXPOWER_8		0xf7 /* 5dbm */
>> +
>>   /* command strobes */
>>   #define	CC2520_CMD_SNOP			0x00
>>   #define	CC2520_CMD_IBUFLD		0x02
>> @@ -628,6 +639,48 @@ cc2520_filter(struct ieee802154_hw *hw,
>>   	return 0;
>>   }
>>   
>> +static int
>> +cc2520_set_txpower(struct ieee802154_hw *hw, int db)
>> +{
> our netlink attribute is s8, maybe we should fix the mac layer at first
> and change the int to s8.

Ok. I will look into that.

> - Alex


-- 
Varka Bhadram