Hello,
On Mon, Feb 1, 2010 at 7:32 PM, Hennerich, Michael
<[email protected]> wrote:
>>From: Dmitry Eremin-Solenikov [mailto:[email protected]]
>>Hello,
>>
>>Thank you for your work!
>>
>>Hennerich, Michael wrote:
>>> This patch adds support for MAC Hardware Accelerator functions such as
>>Automated Acknowledgement (AACK) and CSMA-CA with Retransmission found on a
>>few 802.15.4 Radio Transceivers.
>>> In order to support such functionality the radio's address filtering needs
>>to know it's Short, IEEE Address and PAN ID.
>>> This patch makes sure that this information is pushed to the radio in case
>>it provides a new ieee802154_ops callback function (ext_mac_ctrl).
>>> All this is optional, and shouldn't influence the current implementation.
>>
>>Most of devices should support HW-ACK, otherwise they will be too
>>constrained. See below.
>>
>>>
>>> Current kernel-zigbee doesn't implement ACK transmissions, nor does it
>>implement any kind of Retry.
>>> (BTW it also doesn't do anything useful with the xmit return value)
>>> I understand that this is going to be added in future, as part of the
>>softmac implementation.
>>> I also understand that this softmac allows for greater flexibility such
>>indirect addressing or utilizing different sets of addresses on the same
>>radio.
>>
>>There was once software ACK implementation. It was removed once, as it
>>didn't provide any additional value due to time constraints in standard.
>>
>>>
>>> From: Michael Hennerich <[email protected]>
>>>
>>> include/net/mac802154.h | 6 +++
>>> include/net/wpan-phy.h | 8 ++++
>>> net/mac802154/dev.c | 1 +
>>> net/mac802154/mac_cmd.c | 3 ++
>>> net/mac802154/mib.c | 89
>>+++++++++++++++++++++++++++++++++++++++++++++++
>>> net/mac802154/mib.h | 2 +
>>> 6 files changed, 109 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/include/net/mac802154.h b/include/net/mac802154.h
>>> index 3463996..081aa45 100644
>>> --- a/include/net/mac802154.h
>>> +++ b/include/net/mac802154.h
>>> @@ -21,6 +21,10 @@
>>> #ifndef NET_MAC802154_H
>>> #define NET_MAC802154_H
>>>
>>> +#define IEEE802154_PHY_CMD_IEEE_ADDR 1
>>> +#define IEEE802154_PHY_CMD_SHORT_ADDR 2
>>> +#define IEEE802154_PHY_CMD_PAN_ID 3
>>> +#define IEEE802154_PHY_CMD_PAN_COORD 4
>>> struct ieee802154_dev {
>>> /* filled by the driver */
>>> int extra_tx_headroom; /* headroom to reserve for tx skb */
>>> @@ -96,6 +100,8 @@ struct ieee802154_ops {
>>> int (*ed)(struct ieee802154_dev *dev, u8 *level);
>>> int (*set_channel)(struct ieee802154_dev *dev,
>>> int channel);
>>> + int (*ext_mac_ctrl)(struct ieee802154_dev *dev,
>>> + int cmd, unsigned long
>>val);
>>> };
>>>
>>
>>Hmmm. This sounds like an IOCTL. I'd prefer to have an optional function
>>specially dedicated to hw-ack/filtering addressing fields. This function
>>can take as an argument a pointer to a structure containing all
>>addressing fields.
>
> This is how the radio driver side could look like.
>
> static int trx_ext_mac_ctrl(struct ieee802154_dev *dev,
> int cmd, unsigned long val)
> {
> struct trx_local *lp = dev->priv;
> int ret;
> u8 reg;
>
> DBG(2, "%s :Enter\n", __func__);
> DBG(1, "%s :CMD=0x%X VAL=0x%X\n", __func__, cmd, (u32) val);
>
> might_sleep();
>
> switch(cmd) {
> case IEEE802154_PHY_CMD_IEEE_ADDR:
> trx_write_reg(lp, REG_IEEE_ADDR_0,
> dev->phy->current_ieee_addr[7]);
> trx_write_reg(lp, REG_IEEE_ADDR_1,
> dev->phy->current_ieee_addr[6]);
> trx_write_reg(lp, REG_IEEE_ADDR_2,
> dev->phy->current_ieee_addr[5]);
> trx_write_reg(lp, REG_IEEE_ADDR_3,
> dev->phy->current_ieee_addr[4]);
> trx_write_reg(lp, REG_IEEE_ADDR_4,
> dev->phy->current_ieee_addr[3]);
> trx_write_reg(lp, REG_IEEE_ADDR_5,
> dev->phy->current_ieee_addr[2]);
> trx_write_reg(lp, REG_IEEE_ADDR_6,
> dev->phy->current_ieee_addr[1]);
> trx_write_reg(lp, REG_IEEE_ADDR_7,
> dev->phy->current_ieee_addr[0]);
> break;
> case IEEE802154_PHY_CMD_SHORT_ADDR:
> trx_write_reg(lp, REG_SHORT_ADDR_0, val);
> trx_write_reg(lp, REG_SHORT_ADDR_1, val >> 8);
> dev->phy->current_short_addr = val;
> break;
> case IEEE802154_PHY_CMD_PAN_ID:
> trx_write_reg(lp, REG_PAN_ID0, val);
> trx_write_reg(lp, REG_PAN_ID1, val >> 8);
> dev->phy->current_pan_id = val;
> break;
> case IEEE802154_PHY_CMD_PAN_COORD:
> trx_read_reg(lp, REG_AUTO_CFG, ®);
> if (val)
> reg |= IS_PANCOORD;
> else
> reg &= ~IS_PANCOORD;
> trx_write_reg(lp, REG_AUTO_CFG, reg);
> dev->phy->current_pan_coord = val;
> break;
> default:
> ret = -EFAULT;
> }
>
> DBG(2, "%s :Exit\n", __func__);
> return 0;
> }
>
> However - you are right - this looks pretty much like an ioctl.
Yeah. And also the name is too generic for the task. I'd really
prefer not to have 'do-it-all-here' interfaces.
>>
>>> struct ieee802154_dev *ieee802154_alloc_device(size_t priv_size,
>>> diff --git a/include/net/wpan-phy.h b/include/net/wpan-phy.h
>>> index 8592623..802c555 100644
>>> --- a/include/net/wpan-phy.h
>>> +++ b/include/net/wpan-phy.h
>>> @@ -23,6 +23,7 @@
>>>
>>> #include <linux/netdevice.h>
>>> #include <linux/mutex.h>
>>> +#include <net/af_ieee802154.h>
>>>
>>> struct wpan_phy {
>>> struct mutex pib_lock;
>>> @@ -38,6 +39,13 @@ struct wpan_phy {
>>> u8 transmit_power;
>>> u8 cca_mode;
>>>
>>> + /*
>>> + * Extended MAC Control
>>> + */
>>> + u8 current_pan_coord;
>>> + u16 current_pan_id;
>>> + u16 current_short_addr;
>>> + u8 current_ieee_addr[IEEE802154_ADDR_LEN];
>>
>>I'm not sure that these fields should be present in MAC. I think it may
>>be better to move them directly to per-interface/per-device private
>>data. We have (long-term) plans for one of our soft devices to support
>>multiple-mac filtering and I'd really prefer to keep wpan-phy a phy-only
>>instance (at least for now).
>
> The reason I added those was to only queue work into the drivers 'ioctl' when
> values need to be changed.
> See above ext_mac_ctrl() example.
> So if we decide to better go with a pointer to a structure containing all
> addressing fields, I would prefer to branch off the work queue in the driver
> only in case fields differ.
This is suitable. Or one can add 'changed' bitmask that will be used
by the driver to
push changes to the HW (see mac80211 for some examples of it).
BTW: do you know if engenering samples/engenering evaluation boards
for your tranceiver
are available? We'd most probably consider getting one or two?
--
With best wishes
Dmitry
------------------------------------------------------------------------------
The Planet: dedicated and managed hosting, cloud storage, colocation
Stay online with enterprise data centers and the best network in the business
Choose flexible plans and management services without long-term contracts
Personal 24x7 support from experience hosting pros just a phone call away.
http://p.sf.net/sfu/theplanet-com
_______________________________________________
Linux-zigbee-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
