>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, &reg);
                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

Reply via email to