| Message ID | 20220117115440.60296-8-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | IEEE 802.15.4 scan support | expand |
Hi, On Mon, 17 Jan 2022 at 06:54, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > These periods are expressed in time units (microseconds) while 40 and 12 > are the number of symbol durations these periods will last. We need to > multiply them both with phy->symbol_duration in order to get these > values in microseconds. > > Fixes: 8c6ad9cc5157 ("ieee802154: Add NXP MCR20A IEEE 802.15.4 transceiver driver") > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/net/ieee802154/mcr20a.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c > index f0eb2d3b1c4e..e2c249aef430 100644 > --- a/drivers/net/ieee802154/mcr20a.c > +++ b/drivers/net/ieee802154/mcr20a.c > @@ -976,8 +976,8 @@ static void mcr20a_hw_setup(struct mcr20a_local *lp) > dev_dbg(printdev(lp), "%s\n", __func__); > > phy->symbol_duration = 16; > - phy->lifs_period = 40; > - phy->sifs_period = 12; > + phy->lifs_period = 40 * phy->symbol_duration; > + phy->sifs_period = 12 * phy->symbol_duration; I thought we do that now in register_hw(). Why does this patch exist? - Alex
Hi Alexander, alex.aring@gmail.com wrote on Mon, 17 Jan 2022 17:52:10 -0500: > Hi, > > On Mon, 17 Jan 2022 at 06:54, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > These periods are expressed in time units (microseconds) while 40 and 12 > > are the number of symbol durations these periods will last. We need to > > multiply them both with phy->symbol_duration in order to get these > > values in microseconds. > > > > Fixes: 8c6ad9cc5157 ("ieee802154: Add NXP MCR20A IEEE 802.15.4 transceiver driver") > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/net/ieee802154/mcr20a.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c > > index f0eb2d3b1c4e..e2c249aef430 100644 > > --- a/drivers/net/ieee802154/mcr20a.c > > +++ b/drivers/net/ieee802154/mcr20a.c > > @@ -976,8 +976,8 @@ static void mcr20a_hw_setup(struct mcr20a_local *lp) > > dev_dbg(printdev(lp), "%s\n", __func__); > > > > phy->symbol_duration = 16; > > - phy->lifs_period = 40; > > - phy->sifs_period = 12; > > + phy->lifs_period = 40 * phy->symbol_duration; > > + phy->sifs_period = 12 * phy->symbol_duration; > > I thought we do that now in register_hw(). Why does this patch exist? The lifs and sifs period are wrong. Fixing this silently by generalizing the calculation is simply wrong. I feel we need to do this in order: 1- Fix the period because it is wrong. 2- Now that the period is set to a valid value and the core is able to do the same operation and set the variables to an identical content, we can drop these lines from the driver. #2 being a mechanical change, doing it without #1 means that something that appears harmless actually changes the behavior of the driver. We generally try to avoid that, no? Thanks, Miquèl
Hi, On Tue, 18 Jan 2022 at 13:20, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > alex.aring@gmail.com wrote on Mon, 17 Jan 2022 17:52:10 -0500: > > > Hi, > > > > On Mon, 17 Jan 2022 at 06:54, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > These periods are expressed in time units (microseconds) while 40 and 12 > > > are the number of symbol durations these periods will last. We need to > > > multiply them both with phy->symbol_duration in order to get these > > > values in microseconds. > > > > > > Fixes: 8c6ad9cc5157 ("ieee802154: Add NXP MCR20A IEEE 802.15.4 transceiver driver") > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/net/ieee802154/mcr20a.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c > > > index f0eb2d3b1c4e..e2c249aef430 100644 > > > --- a/drivers/net/ieee802154/mcr20a.c > > > +++ b/drivers/net/ieee802154/mcr20a.c > > > @@ -976,8 +976,8 @@ static void mcr20a_hw_setup(struct mcr20a_local *lp) > > > dev_dbg(printdev(lp), "%s\n", __func__); > > > > > > phy->symbol_duration = 16; > > > - phy->lifs_period = 40; > > > - phy->sifs_period = 12; > > > + phy->lifs_period = 40 * phy->symbol_duration; > > > + phy->sifs_period = 12 * phy->symbol_duration; > > > > I thought we do that now in register_hw(). Why does this patch exist? > > The lifs and sifs period are wrong. > > Fixing this silently by generalizing the calculation is simply wrong. I > feel we need to do this in order: > 1- Fix the period because it is wrong. > 2- Now that the period is set to a valid value and the core is able to > do the same operation and set the variables to an identical content, > we can drop these lines from the driver. > > #2 being a mechanical change, doing it without #1 means that something > that appears harmless actually changes the behavior of the driver. We > generally try to avoid that, no? yes, maybe Stefan can get this patch then somehow to wpan and queue it for stable. Thanks for clarification. - Alex
diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c index f0eb2d3b1c4e..e2c249aef430 100644 --- a/drivers/net/ieee802154/mcr20a.c +++ b/drivers/net/ieee802154/mcr20a.c @@ -976,8 +976,8 @@ static void mcr20a_hw_setup(struct mcr20a_local *lp) dev_dbg(printdev(lp), "%s\n", __func__); phy->symbol_duration = 16; - phy->lifs_period = 40; - phy->sifs_period = 12; + phy->lifs_period = 40 * phy->symbol_duration; + phy->sifs_period = 12 * phy->symbol_duration; hw->flags = IEEE802154_HW_TX_OMIT_CKSUM | IEEE802154_HW_AFILT |
These periods are expressed in time units (microseconds) while 40 and 12 are the number of symbol durations these periods will last. We need to multiply them both with phy->symbol_duration in order to get these values in microseconds. Fixes: 8c6ad9cc5157 ("ieee802154: Add NXP MCR20A IEEE 802.15.4 transceiver driver") Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/net/ieee802154/mcr20a.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)