Hello, Sorry for the long delay,
On Mon, Feb 22, 2010 at 5:56 PM, Hennerich, Michael <[email protected]> wrote: > Hi Dmitry, > > Any objections adding two members of struct ieee802154_hw_addr_filt to struct > ieee802154_dev? > One is filled by the mib, the other is updated by the driver. > In case fields differ we schedule work and call > ieee802154_ops:set_hw_addr_filt() with a changed mask. I'm still not sure, that this is the best way to do this. I'd prefer to keep the mib-related instance in the struct ieee802154_sub_if_data, as it's really subinterface-related. And the 'updated by driver' instance most probably should go to the driver private data. I'm trying to make ieee802154_dev device (and data) agnostic (e.g. we are currently trying to write a fw. that will fully support multiple logical devices (with filtering) on one antenna). OTOH, if it looks really hard for you, do it the way you've suggested (I'd still move the second struct instance to driver's priv data though). We can refactor things later, when there will be need to do this. > > struct ieee802154_hw_addr_filt { > u16 pan_id; > u16 short_addr; > u8 ieee_addr[IEEE802154_ADDR_LEN]; > u8 pan_coord; > } > > struct ieee802154_dev { > /* filled by the driver */ > int extra_tx_headroom; /* headroom to reserve for tx skb */ > u32 flags; /* Flags for device to set */ > struct device *parent; > struct ieee802154_hw_addr_filt curr_hw_filt; > > /* filled by mac802154 core */ > struct ieee802154_hw_addr_filt hw_filt; > void *priv; /* driver-specific data */ > struct wpan_phy *phy; > }; > > Regards, > Michael > > ------------------------------------------------------------------ > ********* Analog Devices GmbH Open Platform Solutions > ** ***** > ** ** Wilhelm-Wagenfeld-Strasse 6 > ** ***** D-80807 Munich > ********* Germany > Registergericht München HRB 40368, Geschäftsführer: Thomas Wessel, William > A. Martin, Margaret K. Seif > > > >>-----Original Message----- >>From: Hennerich, Michael [mailto:[email protected]] >>Sent: Montag, 1. Februar 2010 17:32 >>To: Dmitry Eremin-Solenikov >>Cc: linux-zigbee-devel >>Subject: Re: [Linux-zigbee-devel] [PATCH] Support for MAC Hardware >>Accelerator functions >> >>>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. >> >>> >>>> 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. >> >>-Michael >> >>> >>>> struct device dev; >>>> int idx; >>>> >>> >>>[rest of the patch skipped] >>> >>>-- >>>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 > -- With best wishes Dmitry ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev _______________________________________________ Linux-zigbee-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
