All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCHv2 bluetooth-next 1/3] at86rf230: fix phy settings while sleeping
@ 2015-06-16  9:07 Alexander Aring
  2015-06-16  9:07 ` [PATCHv2 bluetooth-next 2/3] at86rf230: add recommended csma backoffs settings Alexander Aring
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Alexander Aring @ 2015-06-16  9:07 UTC (permalink / raw)
  To: linux-wpan; +Cc: kernel, Alexander Aring

While in sleep state then we can't access the at86rf2xx registers. This
patch checks if the transceiver is in sleep state before sending spi
messages via regmap. Regmap is used on every driver ops callback except
for receive and xmit handling, but while receive and xmit handling the
phy should not be inside the sleep state.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
change since v2:
 - I detect an issue with cca ed level setting in v1, there was a return in the
   middle of awake and sleep transceiver. This results that the transceiver
   wasn't again turn into sleep state afterwards.
   To avoid such behaviour I moved the awake and sleep into lowlevel register io
   functions. These functions "could" be called only when the phy is in sleep
   state.


 drivers/net/ieee802154/at86rf230.c | 88 +++++++++++++++++++++++++++++---------
 1 file changed, 68 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 6b31f47..b839bbd 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -90,6 +90,7 @@ struct at86rf230_local {
 	struct at86rf2xx_chip_data *data;
 	struct regmap *regmap;
 	int slp_tr;
+	bool sleep;
 
 	struct completion state_complete;
 	struct at86rf230_state_change state;
@@ -114,18 +115,66 @@ at86rf230_async_state_change(struct at86rf230_local *lp,
 			     const u8 state, void (*complete)(void *context),
 			     const bool irq_enable);
 
+static inline void
+at86rf230_sleep(struct at86rf230_local *lp)
+{
+	if (gpio_is_valid(lp->slp_tr)) {
+		gpio_set_value(lp->slp_tr, 1);
+		usleep_range(lp->data->t_off_to_sleep,
+			     lp->data->t_off_to_sleep + 10);
+		lp->sleep = true;
+	}
+}
+
+static inline void
+at86rf230_awake(struct at86rf230_local *lp)
+{
+	if (gpio_is_valid(lp->slp_tr)) {
+		gpio_set_value(lp->slp_tr, 0);
+		usleep_range(lp->data->t_sleep_to_off,
+			     lp->data->t_sleep_to_off + 100);
+		lp->sleep = false;
+	}
+}
+
 static inline int
 __at86rf230_write(struct at86rf230_local *lp,
 		  unsigned int addr, unsigned int data)
 {
-	return regmap_write(lp->regmap, addr, data);
+	bool sleep = lp->sleep;
+	int ret;
+
+	/* awake for register setting if sleep */
+	if (sleep)
+		at86rf230_awake(lp);
+
+	ret = regmap_write(lp->regmap, addr, data);
+
+	/* sleep again if was sleeping */
+	if (sleep)
+		at86rf230_sleep(lp);
+
+	return ret;
 }
 
 static inline int
 __at86rf230_read(struct at86rf230_local *lp,
 		 unsigned int addr, unsigned int *data)
 {
-	return regmap_read(lp->regmap, addr, data);
+	bool sleep = lp->sleep;
+	int ret;
+
+	/* awake for register setting if sleep */
+	if (sleep)
+		at86rf230_awake(lp);
+
+	ret = regmap_read(lp->regmap, addr, data);
+
+	/* sleep again if was sleeping */
+	if (sleep)
+		at86rf230_sleep(lp);
+
+	return ret;
 }
 
 static inline int
@@ -147,7 +196,20 @@ at86rf230_write_subreg(struct at86rf230_local *lp,
 		       unsigned int addr, unsigned int mask,
 		       unsigned int shift, unsigned int data)
 {
-	return regmap_update_bits(lp->regmap, addr, mask, data << shift);
+	bool sleep = lp->sleep;
+	int ret;
+
+	/* awake for register setting if sleep */
+	if (sleep)
+		at86rf230_awake(lp);
+
+	ret = regmap_update_bits(lp->regmap, addr, mask, data << shift);
+
+	/* sleep again if was sleeping */
+	if (sleep)
+		at86rf230_sleep(lp);
+
+	return ret;
 }
 
 static inline void
@@ -873,12 +935,7 @@ at86rf230_start(struct ieee802154_hw *hw)
 {
 	struct at86rf230_local *lp = hw->priv;
 
-	if (gpio_is_valid(lp->slp_tr)) {
-		gpio_set_value(lp->slp_tr, 0);
-		usleep_range(lp->data->t_sleep_to_off,
-			     lp->data->t_sleep_to_off + 100);
-	}
-
+	at86rf230_awake(lp);
 	enable_irq(lp->spi->irq);
 
 	return at86rf230_sync_state_change(hw->priv, STATE_RX_AACK_ON);
@@ -892,12 +949,7 @@ at86rf230_stop(struct ieee802154_hw *hw)
 	at86rf230_sync_state_change(hw->priv, STATE_FORCE_TRX_OFF);
 
 	disable_irq(lp->spi->irq);
-
-	if (gpio_is_valid(lp->slp_tr)) {
-		gpio_set_value(lp->slp_tr, 1);
-		usleep_range(lp->data->t_off_to_sleep,
-			     lp->data->t_off_to_sleep + 10);
-	}
+	at86rf230_sleep(lp);
 }
 
 static int
@@ -1672,11 +1724,7 @@ static int at86rf230_probe(struct spi_device *spi)
 	disable_irq(spi->irq);
 
 	/* going into sleep by default */
-	if (gpio_is_valid(slp_tr)) {
-		gpio_set_value(slp_tr, 1);
-		usleep_range(lp->data->t_off_to_sleep,
-			     lp->data->t_off_to_sleep + 10);
-	}
+	at86rf230_sleep(lp);
 
 	rc = ieee802154_register_hw(lp->hw);
 	if (rc)
-- 
2.4.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCHv2 bluetooth-next 2/3] at86rf230: add recommended csma backoffs settings
  2015-06-16  9:07 [PATCHv2 bluetooth-next 1/3] at86rf230: fix phy settings while sleeping Alexander Aring
@ 2015-06-16  9:07 ` Alexander Aring
  2015-06-16 16:56   ` Marcel Holtmann
  2015-06-16  9:07 ` [PATCHv2 bluetooth-next 3/3] at86rf230: cleanup start and stop callbacks Alexander Aring
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Alexander Aring @ 2015-06-16  9:07 UTC (permalink / raw)
  To: linux-wpan; +Cc: kernel, Alexander Aring

This patch adds support for a new random csma backoffs settings when
going into sleep state. This is recommended according at86rf2xx
datasheets.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index b839bbd..7e30a45 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -945,10 +945,21 @@ static void
 at86rf230_stop(struct ieee802154_hw *hw)
 {
 	struct at86rf230_local *lp = hw->priv;
+	u8 csma_seed[2];
 
 	at86rf230_sync_state_change(hw->priv, STATE_FORCE_TRX_OFF);
 
 	disable_irq(lp->spi->irq);
+
+	/* It's recommended to set random new csma_seeds before sleep state.
+	 * Makes only sense in the stop callback, not doing this inside of
+	 * at86rf230_sleep, this is also used when we don't transmit afterwards
+	 * when calling start callback again.
+	 */
+	get_random_bytes(csma_seed, ARRAY_SIZE(csma_seed));
+	at86rf230_write_subreg(lp, SR_CSMA_SEED_0, csma_seed[0]);
+	at86rf230_write_subreg(lp, SR_CSMA_SEED_1, csma_seed[1]);
+
 	at86rf230_sleep(lp);
 }
 
-- 
2.4.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCHv2 bluetooth-next 3/3] at86rf230: cleanup start and stop callbacks
  2015-06-16  9:07 [PATCHv2 bluetooth-next 1/3] at86rf230: fix phy settings while sleeping Alexander Aring
  2015-06-16  9:07 ` [PATCHv2 bluetooth-next 2/3] at86rf230: add recommended csma backoffs settings Alexander Aring
@ 2015-06-16  9:07 ` Alexander Aring
  2015-06-16 16:56   ` Marcel Holtmann
  2015-06-16  9:19 ` [PATCHv2 bluetooth-next 1/3] at86rf230: fix phy settings while sleeping Lucas Stach
  2015-06-16 16:56 ` Marcel Holtmann
  3 siblings, 1 reply; 9+ messages in thread
From: Alexander Aring @ 2015-06-16  9:07 UTC (permalink / raw)
  To: linux-wpan; +Cc: kernel, Alexander Aring

This code cleanups the start and stop callbacks by removing hw->priv and
using the already dereferenced variable lp which is the same.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 7e30a45..f7bd9f3 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -938,7 +938,7 @@ at86rf230_start(struct ieee802154_hw *hw)
 	at86rf230_awake(lp);
 	enable_irq(lp->spi->irq);
 
-	return at86rf230_sync_state_change(hw->priv, STATE_RX_AACK_ON);
+	return at86rf230_sync_state_change(lp, STATE_RX_AACK_ON);
 }
 
 static void
@@ -947,7 +947,7 @@ at86rf230_stop(struct ieee802154_hw *hw)
 	struct at86rf230_local *lp = hw->priv;
 	u8 csma_seed[2];
 
-	at86rf230_sync_state_change(hw->priv, STATE_FORCE_TRX_OFF);
+	at86rf230_sync_state_change(lp, STATE_FORCE_TRX_OFF);
 
 	disable_irq(lp->spi->irq);
 
-- 
2.4.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCHv2 bluetooth-next 1/3] at86rf230: fix phy settings while sleeping
  2015-06-16  9:07 [PATCHv2 bluetooth-next 1/3] at86rf230: fix phy settings while sleeping Alexander Aring
  2015-06-16  9:07 ` [PATCHv2 bluetooth-next 2/3] at86rf230: add recommended csma backoffs settings Alexander Aring
  2015-06-16  9:07 ` [PATCHv2 bluetooth-next 3/3] at86rf230: cleanup start and stop callbacks Alexander Aring
@ 2015-06-16  9:19 ` Lucas Stach
  2015-06-16 10:08   ` Alexander Aring
  2015-06-16 16:56 ` Marcel Holtmann
  3 siblings, 1 reply; 9+ messages in thread
From: Lucas Stach @ 2015-06-16  9:19 UTC (permalink / raw)
  To: Alexander Aring; +Cc: linux-wpan, kernel

Am Dienstag, den 16.06.2015, 11:07 +0200 schrieb Alexander Aring:
> While in sleep state then we can't access the at86rf2xx registers. This
> patch checks if the transceiver is in sleep state before sending spi
> messages via regmap. Regmap is used on every driver ops callback except
> for receive and xmit handling, but while receive and xmit handling the
> phy should not be inside the sleep state.
> 
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
> change since v2:
>  - I detect an issue with cca ed level setting in v1, there was a return in the
>    middle of awake and sleep transceiver. This results that the transceiver
>    wasn't again turn into sleep state afterwards.
>    To avoid such behaviour I moved the awake and sleep into lowlevel register io
>    functions. These functions "could" be called only when the phy is in sleep
>    state.
> 
> 
>  drivers/net/ieee802154/at86rf230.c | 88 +++++++++++++++++++++++++++++---------
>  1 file changed, 68 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> index 6b31f47..b839bbd 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -90,6 +90,7 @@ struct at86rf230_local {
>  	struct at86rf2xx_chip_data *data;
>  	struct regmap *regmap;
>  	int slp_tr;
> +	bool sleep;

This should probably be a reference count instead of a simple bool. I
suppose there are more locations in the driver where you want to keep
the PHY awake.

Also this would allow you to throw away all those "if (sleep)" checks at
the callers. Just call at86rf230_phy_wake_get() before you do something
that needs the phy to be awake and increment the refcount there and have
a similar ..._put() function to decrement the refcount. Then toggle the
GPIO when the refcount is going 0->1 and the other way around.

Regards,
Lucas

>  
>  	struct completion state_complete;
>  	struct at86rf230_state_change state;
> @@ -114,18 +115,66 @@ at86rf230_async_state_change(struct at86rf230_local *lp,
>  			     const u8 state, void (*complete)(void *context),
>  			     const bool irq_enable);
>  
> +static inline void
> +at86rf230_sleep(struct at86rf230_local *lp)
> +{
> +	if (gpio_is_valid(lp->slp_tr)) {
> +		gpio_set_value(lp->slp_tr, 1);
> +		usleep_range(lp->data->t_off_to_sleep,
> +			     lp->data->t_off_to_sleep + 10);
> +		lp->sleep = true;
> +	}
> +}
> +
> +static inline void
> +at86rf230_awake(struct at86rf230_local *lp)
> +{
> +	if (gpio_is_valid(lp->slp_tr)) {
> +		gpio_set_value(lp->slp_tr, 0);
> +		usleep_range(lp->data->t_sleep_to_off,
> +			     lp->data->t_sleep_to_off + 100);
> +		lp->sleep = false;
> +	}
> +}
> +
>  static inline int
>  __at86rf230_write(struct at86rf230_local *lp,
>  		  unsigned int addr, unsigned int data)
>  {
> -	return regmap_write(lp->regmap, addr, data);
> +	bool sleep = lp->sleep;
> +	int ret;
> +
> +	/* awake for register setting if sleep */
> +	if (sleep)
> +		at86rf230_awake(lp);
> +
> +	ret = regmap_write(lp->regmap, addr, data);
> +
> +	/* sleep again if was sleeping */
> +	if (sleep)
> +		at86rf230_sleep(lp);
> +
> +	return ret;
>  }
>  
>  static inline int
>  __at86rf230_read(struct at86rf230_local *lp,
>  		 unsigned int addr, unsigned int *data)
>  {
> -	return regmap_read(lp->regmap, addr, data);
> +	bool sleep = lp->sleep;
> +	int ret;
> +
> +	/* awake for register setting if sleep */
> +	if (sleep)
> +		at86rf230_awake(lp);
> +
> +	ret = regmap_read(lp->regmap, addr, data);
> +
> +	/* sleep again if was sleeping */
> +	if (sleep)
> +		at86rf230_sleep(lp);
> +
> +	return ret;
>  }
>  
>  static inline int
> @@ -147,7 +196,20 @@ at86rf230_write_subreg(struct at86rf230_local *lp,
>  		       unsigned int addr, unsigned int mask,
>  		       unsigned int shift, unsigned int data)
>  {
> -	return regmap_update_bits(lp->regmap, addr, mask, data << shift);
> +	bool sleep = lp->sleep;
> +	int ret;
> +
> +	/* awake for register setting if sleep */
> +	if (sleep)
> +		at86rf230_awake(lp);
> +
> +	ret = regmap_update_bits(lp->regmap, addr, mask, data << shift);
> +
> +	/* sleep again if was sleeping */
> +	if (sleep)
> +		at86rf230_sleep(lp);
> +
> +	return ret;
>  }
>  
>  static inline void
> @@ -873,12 +935,7 @@ at86rf230_start(struct ieee802154_hw *hw)
>  {
>  	struct at86rf230_local *lp = hw->priv;
>  
> -	if (gpio_is_valid(lp->slp_tr)) {
> -		gpio_set_value(lp->slp_tr, 0);
> -		usleep_range(lp->data->t_sleep_to_off,
> -			     lp->data->t_sleep_to_off + 100);
> -	}
> -
> +	at86rf230_awake(lp);
>  	enable_irq(lp->spi->irq);
>  
>  	return at86rf230_sync_state_change(hw->priv, STATE_RX_AACK_ON);
> @@ -892,12 +949,7 @@ at86rf230_stop(struct ieee802154_hw *hw)
>  	at86rf230_sync_state_change(hw->priv, STATE_FORCE_TRX_OFF);
>  
>  	disable_irq(lp->spi->irq);
> -
> -	if (gpio_is_valid(lp->slp_tr)) {
> -		gpio_set_value(lp->slp_tr, 1);
> -		usleep_range(lp->data->t_off_to_sleep,
> -			     lp->data->t_off_to_sleep + 10);
> -	}
> +	at86rf230_sleep(lp);
>  }
>  
>  static int
> @@ -1672,11 +1724,7 @@ static int at86rf230_probe(struct spi_device *spi)
>  	disable_irq(spi->irq);
>  
>  	/* going into sleep by default */
> -	if (gpio_is_valid(slp_tr)) {
> -		gpio_set_value(slp_tr, 1);
> -		usleep_range(lp->data->t_off_to_sleep,
> -			     lp->data->t_off_to_sleep + 10);
> -	}
> +	at86rf230_sleep(lp);
>  
>  	rc = ieee802154_register_hw(lp->hw);
>  	if (rc)

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCHv2 bluetooth-next 1/3] at86rf230: fix phy settings while sleeping
  2015-06-16  9:19 ` [PATCHv2 bluetooth-next 1/3] at86rf230: fix phy settings while sleeping Lucas Stach
@ 2015-06-16 10:08   ` Alexander Aring
  2015-06-16 18:31     ` Alexander Aring
  0 siblings, 1 reply; 9+ messages in thread
From: Alexander Aring @ 2015-06-16 10:08 UTC (permalink / raw)
  To: Lucas Stach; +Cc: linux-wpan, kernel

Hi,

On Tue, Jun 16, 2015 at 11:19:54AM +0200, Lucas Stach wrote:
> Am Dienstag, den 16.06.2015, 11:07 +0200 schrieb Alexander Aring:
> > While in sleep state then we can't access the at86rf2xx registers. This
> > patch checks if the transceiver is in sleep state before sending spi
> > messages via regmap. Regmap is used on every driver ops callback except
> > for receive and xmit handling, but while receive and xmit handling the
> > phy should not be inside the sleep state.
> > 
> > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > ---
> > change since v2:
> >  - I detect an issue with cca ed level setting in v1, there was a return in the
> >    middle of awake and sleep transceiver. This results that the transceiver
> >    wasn't again turn into sleep state afterwards.
> >    To avoid such behaviour I moved the awake and sleep into lowlevel register io
> >    functions. These functions "could" be called only when the phy is in sleep
> >    state.
> > 
> > 
> >  drivers/net/ieee802154/at86rf230.c | 88 +++++++++++++++++++++++++++++---------
> >  1 file changed, 68 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> > index 6b31f47..b839bbd 100644
> > --- a/drivers/net/ieee802154/at86rf230.c
> > +++ b/drivers/net/ieee802154/at86rf230.c
> > @@ -90,6 +90,7 @@ struct at86rf230_local {
> >  	struct at86rf2xx_chip_data *data;
> >  	struct regmap *regmap;
> >  	int slp_tr;
> > +	bool sleep;
> 
> This should probably be a reference count instead of a simple bool. I
> suppose there are more locations in the driver where you want to keep
> the PHY awake.
> 

Yes, I want that e.g. for this case:

For example the regmap register dump is on sleep state invalid. I can't
dump the registers, because they returning zero's (In case of volatile).

Doing handling this by refcount does not solve this issue. What I need
is to overwrite then the regmap lowlevel spi io callbacks to do that,
but I simple leave the state that it's invalid to do a register dump on
it. (It's also only for debugfs and then people should know what they
doing).

Normally the driver should also read the registers which are registered
in the 802.15.4 subsystem. Regmap simple doesn't know the state of the
transceiver.

> Also this would allow you to throw away all those "if (sleep)" checks at
> the callers. Just call at86rf230_phy_wake_get() before you do something
> that needs the phy to be awake and increment the refcount there and have
> a similar ..._put() function to decrement the refcount. Then toggle the
> GPIO when the refcount is going 0->1 and the other way around.
> 

I agree this is the common kernel soltution. I think is what we need is
to put this logic then into 802.15.4 subsystem.

In the wpan_phy we put a refcounter and let them increment when the phy
is "used by susbsytem" -> if was 0 then awake and decrement
"when it's not used" -> if reach 0 then subsystem.

Currently all ops (except the xmit op, but this should not called when
sleeping) are protected by rtnl mutex so the refcount can't be greater
than one.

I will put it to my ToDo List, then we have some driver_ops
resume/suspsend when the susbsystem will use the wpan_phy when it's
sleeping and the driver needs to do something.

Nevertheless this patch looks for _now_ valid and introducing a refcount
in 802.15.4 for the wpan_phy is better. At the moment I broke the
behaviour when introduced the sleep state some days ago.

At Wednesday Marcel will send the last push request to net-next and I
need to decide now if we do this fix OR reverting the introducing of
sleep state.

I would leave this patch and when the times comes we introduce a better
802.15.4 wpan behaviour with refcounts later. The logic is just
completely in driver layer right now.

- Alex

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCHv2 bluetooth-next 1/3] at86rf230: fix phy settings while sleeping
  2015-06-16  9:07 [PATCHv2 bluetooth-next 1/3] at86rf230: fix phy settings while sleeping Alexander Aring
                   ` (2 preceding siblings ...)
  2015-06-16  9:19 ` [PATCHv2 bluetooth-next 1/3] at86rf230: fix phy settings while sleeping Lucas Stach
@ 2015-06-16 16:56 ` Marcel Holtmann
  3 siblings, 0 replies; 9+ messages in thread
From: Marcel Holtmann @ 2015-06-16 16:56 UTC (permalink / raw)
  To: Alexander Aring; +Cc: linux-wpan, kernel

Hi Alex,

> While in sleep state then we can't access the at86rf2xx registers. This
> patch checks if the transceiver is in sleep state before sending spi
> messages via regmap. Regmap is used on every driver ops callback except
> for receive and xmit handling, but while receive and xmit handling the
> phy should not be inside the sleep state.
> 
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
> change since v2:
> - I detect an issue with cca ed level setting in v1, there was a return in the
>   middle of awake and sleep transceiver. This results that the transceiver
>   wasn't again turn into sleep state afterwards.
>   To avoid such behaviour I moved the awake and sleep into lowlevel register io
>   functions. These functions "could" be called only when the phy is in sleep
>   state.
> 
> 
> drivers/net/ieee802154/at86rf230.c | 88 +++++++++++++++++++++++++++++---------
> 1 file changed, 68 insertions(+), 20 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCHv2 bluetooth-next 2/3] at86rf230: add recommended csma backoffs settings
  2015-06-16  9:07 ` [PATCHv2 bluetooth-next 2/3] at86rf230: add recommended csma backoffs settings Alexander Aring
@ 2015-06-16 16:56   ` Marcel Holtmann
  0 siblings, 0 replies; 9+ messages in thread
From: Marcel Holtmann @ 2015-06-16 16:56 UTC (permalink / raw)
  To: Alexander Aring; +Cc: linux-wpan, kernel

Hi Alex,

> This patch adds support for a new random csma backoffs settings when
> going into sleep state. This is recommended according at86rf2xx
> datasheets.
> 
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
> drivers/net/ieee802154/at86rf230.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)

patch has been applied to bluetooth-next tree.

Regards

Marcel


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCHv2 bluetooth-next 3/3] at86rf230: cleanup start and stop callbacks
  2015-06-16  9:07 ` [PATCHv2 bluetooth-next 3/3] at86rf230: cleanup start and stop callbacks Alexander Aring
@ 2015-06-16 16:56   ` Marcel Holtmann
  0 siblings, 0 replies; 9+ messages in thread
From: Marcel Holtmann @ 2015-06-16 16:56 UTC (permalink / raw)
  To: Alexander Aring; +Cc: linux-wpan, kernel

Hi Alex,

> This code cleanups the start and stop callbacks by removing hw->priv and
> using the already dereferenced variable lp which is the same.
> 
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
> drivers/net/ieee802154/at86rf230.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCHv2 bluetooth-next 1/3] at86rf230: fix phy settings while sleeping
  2015-06-16 10:08   ` Alexander Aring
@ 2015-06-16 18:31     ` Alexander Aring
  0 siblings, 0 replies; 9+ messages in thread
From: Alexander Aring @ 2015-06-16 18:31 UTC (permalink / raw)
  To: Lucas Stach; +Cc: linux-wpan, kernel

On Tue, Jun 16, 2015 at 12:08:06PM +0200, Alexander Aring wrote:
...
> 
> I agree this is the common kernel soltution. I think is what we need is
> to put this logic then into 802.15.4 subsystem.
> 
> In the wpan_phy we put a refcounter and let them increment when the phy
> is "used by susbsytem" -> if was 0 then awake and decrement
> "when it's not used" -> if reach 0 then subsystem.
> 
> Currently all ops (except the xmit op, but this should not called when
> sleeping) are protected by rtnl mutex so the refcount can't be greater
> than one.
> 
> I will put it to my ToDo List, then we have some driver_ops
> resume/suspsend when the susbsystem will use the wpan_phy when it's
> sleeping and the driver needs to do something.
> 
> Nevertheless this patch looks for _now_ valid and introducing a refcount
> in 802.15.4 for the wpan_phy is better. At the moment I broke the
> behaviour when introduced the sleep state some days ago.
> 
> At Wednesday Marcel will send the last push request to net-next and I
> need to decide now if we do this fix OR reverting the introducing of
> sleep state.
> 
> I would leave this patch and when the times comes we introduce a better
> 802.15.4 wpan behaviour with refcounts later. The logic is just
> completely in driver layer right now.

I tried to implement such feature but I gave up.

What we have is currently a very transceiver specific issue which is
that we can't access the register when the transceiver is in sleep mode.

I find out that e.g. the mrf24j40 [0] can access the registers while in
sleep mode. See "3.15 Sleep": "... registers and the MRF24J40 is accessible
via the SPI port".

So there are transceivers which have the possibility to access the
register inside sleep state and there are transceivers which have no the
possibility.

Well, our at86rf2xx has it not and need to be wake-up before access the
spi registers.


Now inside the 802.15.4 subsystem the transceiver _should_ in sleep mode
in the following states:

1. In probing after hardware init. -> transceiver hasn't been started is
   like starting stop callback, because the transceiver hasn't been started.
2. After calling stop driver callback. -> should be called by suspend
   callback of pm_ops and last interface down only.

On a start driver callback the transceiver isn't in sleep mode anymore,
which means xmit callback don't need to check on "if we are in sleep
state" we can be sure, that we are not in a sleep state.


Now if the transceiver isn't started (this could be the case when we
have no interfaces up) then we can't access the registers, BUT there are
netlink commands to change e.g. channel, tx_power etc. which can also be
accessible while interfaces are down (means transceiver is in sleep state).

In this case we need to awake the transceiver (in case of at86rf2xx
transceivers, there are some transceivers which offers the register
accessible while sleep).


What I said before was that all driver_ops callbacks are protected by a
mutex, so we can't run twice a driver_ops operation (except xmit
callback but this is a special case, it's only called between start <->
stop state and then the transceiver should _not_ sleeping).

What I did now in this patch was to check if we sleeping, if yes then
make the register access possible, otherwise do nothing. If we do
nothing then we are in the start <-> stop state when the transceiver
should always be not in the sleeping state.

I think this is a very specific transceiver issue and should be handled
by the driver now. The only thing which I could do is to introduce a new
hardware flag "IEEE802154_HW_NO_SLEEP_ACCESS" and the layer above checks
on each driver_ops on this and awakes the transceiver before and set the
transceiver sleeping after driver_op. On more than one driver_ops in the
above layer we could save some time. But I think we should not do that and
this time is so small - it doesn't matter.

To introduce some refcnt would also not give any benefits, when all
driver_ops are protected by a mutex and can't run twice. So I would
simple say that we leave the state of handling the "register accessible
when transceiver is in sleep state" as it is.

- Alex

[0] http://ww1.microchip.com/downloads/en/DeviceDoc/39776C.pdf

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2015-06-16 18:31 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-16  9:07 [PATCHv2 bluetooth-next 1/3] at86rf230: fix phy settings while sleeping Alexander Aring
2015-06-16  9:07 ` [PATCHv2 bluetooth-next 2/3] at86rf230: add recommended csma backoffs settings Alexander Aring
2015-06-16 16:56   ` Marcel Holtmann
2015-06-16  9:07 ` [PATCHv2 bluetooth-next 3/3] at86rf230: cleanup start and stop callbacks Alexander Aring
2015-06-16 16:56   ` Marcel Holtmann
2015-06-16  9:19 ` [PATCHv2 bluetooth-next 1/3] at86rf230: fix phy settings while sleeping Lucas Stach
2015-06-16 10:08   ` Alexander Aring
2015-06-16 18:31     ` Alexander Aring
2015-06-16 16:56 ` Marcel Holtmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.