[PATCH v5 0/3] ieee802154: Add support for Cascoda CA8210

[PATCH v5 0/3] ieee802154: Add support for Cascoda CA8210

[harrymorris12]

From: Harry Morris <h.morris@cascoda.com>

This patchset adds a new device driver, documentation and build support for
Cascoda's CA8210 IEEE 802.15.4 radio transceiver:
http://www.cascoda.com/products/ca-821x/

v5:
+Cc: Marcel Holtmann

v4:
Added allowable tx_powers and cca_ed_levels
Changed power units to mbm (tx_power & cca_ed_level)

Harry Morris (3):
  ieee802154: Add CA8210 IEEE 802.15.4 device driver
  ieee802154: Add device tree documentation for CA8210
  ieee802154: Add entry in MAINTAINTERS for CA8210 driver

 .../devicetree/bindings/net/ieee802154/ca8210.txt  |   28 +
 .../devicetree/bindings/vendor-prefixes.txt        |    1 +
 MAINTAINERS                                        |    9 +
 drivers/net/ieee802154/Kconfig                     |   18 +
 drivers/net/ieee802154/Makefile                    |    1 +
 drivers/net/ieee802154/ca8210.c                    | 3665 ++++++++++++++++++++
 6 files changed, 3722 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ieee802154/ca8210.txt
 create mode 100644 drivers/net/ieee802154/ca8210.c

--
2.9.3.windows.1

[PATCH v5 2/3] ieee802154: Add device tree documentation for CA8210

[harrymorris12]

From: Harry Morris <h.morris@cascoda.com>

Signed-off-by: Harry Morris <h.morris@cascoda.com>
Acked-by: Stefan Schmidt <stefan@osg.samsung.com>
---
 .../devicetree/bindings/net/ieee802154/ca8210.txt  | 28 ++++++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.txt        |  1 +
 2 files changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ieee802154/ca8210.txt

diff --git a/Documentation/devicetree/bindings/net/ieee802154/ca8210.txt b/Documentation/devicetree/bindings/net/ieee802154/ca8210.txt
new file mode 100644
index 0000000..0297ce8
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ieee802154/ca8210.txt
@@ -0,0 +1,28 @@
+* CA8210 IEEE 802.15.4 *
+
+Required properties:
+	- compatible:           Should be "cascoda,ca8210"
+	- reg:                  Controlling chip select
+	- spi-max-frequency:    Maximum clock speed, should be *less than*
+	                        4000000
+	- spi-cpol:             Requires inverted clock polarity
+	- reset-gpio:           GPIO attached to reset
+	- irq-gpio:             GPIO attached to IRQ
+Optional properties:
+	- extclock-enable:      Include for the ca8210 to route its 16MHz clock
+	                        to an output
+	- extclock-freq:        Frequency in Hz of the external clock
+	- extclock-gpio:        GPIO of the ca8210 to output the clock on
+
+Example:
+	ca8210@0 {
+		compatible = "cascoda,ca8210";
+		reg = <0>;
+		spi-max-frequency = <3000000>;
+		spi-cpol;
+		reset-gpio = <&gpio1 1 GPIO_ACTIVE_HIGH>;
+		irq-gpio = <&gpio1 2 GPIO_ACTIVE_HIGH>;
+		extclock-enable;
+		extclock-freq = 16000000;
+		extclock-gpio = 2;
+	};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 24c6f65..6c5609f 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -47,6 +47,7 @@ brcm	Broadcom Corporation
 buffalo	Buffalo, Inc.
 calxeda	Calxeda
 capella	Capella Microsystems, Inc
+cascoda	Cascoda, Ltd.
 cavium	Cavium, Inc.
 cdns	Cadence Design Systems Inc.
 ceva	Ceva, Inc.
--
2.9.3.windows.1

[PATCH v5 3/3] ieee802154: Add entry in MAINTAINTERS for CA8210 driver

[harrymorris12]

From: Harry Morris <h.morris@cascoda.com>

Signed-off-by: Harry Morris <h.morris@cascoda.com>
Acked-by: Stefan Schmidt <stefan@osg.samsung.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5e925a2..cb3428f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2829,6 +2829,15 @@ W:	http://www.linux-c6x.org/wiki/index.php/Main_Page
 S:	Maintained
 F:	arch/c6x/

+CA8210 IEEE-802.15.4 RADIO DRIVER
+M:	Harry Morris <h.morris@cascoda.com>
+M:	linuxdev@cascoda.com
+L:	linux-wpan@vger.kernel.org
+W:	https://github.com/Cascoda/ca8210-linux.git
+S:	Maintained
+F:	drivers/net/ieee802154/ca8210.c
+F:	Documentation/devicetree/bindings/net/ieee802154/ca8210.txt
+
 CACHEFILES: FS-CACHE BACKEND FOR CACHING ON MOUNTED FILESYSTEMS
 M:	David Howells <dhowells@redhat.com>
 L:	linux-cachefs@redhat.com (moderated for non-subscribers)
--
2.9.3.windows.1

Re: [PATCH v5 1/3] ieee802154: Add CA8210 IEEE 802.15.4 device driver

[Stefan Schmidt]

Hello.

I cutted them pieces of the patch out so we do not have to carry the 
3600 lines around for each message from now on.

On 30/11/16 15:10, Harry Morris wrote:
> Hi Stefan,
>
> On 25/11/2016 14:44, Stefan Schmidt wrote:
>
>> When running this patch through scripts/checkpatch.pl I get the
>> following:
>>
>> total: 52 errors, 60 warnings, 189 checks, 3690 lines checked
>>
>> That is quite a bunch or warnings and errors. Please have a look at
>> them. Many of them are coding style related and it saves us both a lot
>> of time and re-post cycles if you go with the checkscript on your side
>> to find and fix most of them. I will drop my coding style comments
>> until this is fixed. Easier for both of us. :)
>
> Sorry about that, I ran through the script previously but I guess some
> issues have been introduced in more recent changes. I'll run through it
> again.

Thanks :)

[Snip]

>>> +
>>> +/**
>>> + * struct fulladdr - full MAC addressing information structure
>>> + * @mode:    address mode (none, short, extended)
>>> + * @pan_id:  16-bit LE pan id
>>> + * @address: LE address, variable length as specified by mode
>>> + *
>>> + */
>>
>>
>> Out of interest because I deal with something like this myself for
>> production. Does the chip has some way to store a unique extended
>> permanent address somewhere? I ask this because I know the device is
>> not a simple transceiver but also runs some firmware which could be
>> used to handling storing the address, etc. From what I have seen used
>> on the Ci40 it relies on getting the address set from userspace in
>> early network bringup.
>
> No I'm afraid the ca8210 has no non-volatile memory so cannot store the
> extended address set in the PIB. A user can read the address and store
> it somewhere off-chip of course and then write it back but the chip
> doesn't do anything like that itself.

OK, so not different to all the other transceivers at all. I just had 
hope due to the MAC firmware you might have such a feature. :)

[Snip]

>>> +
>>> +/**
>>> + * ca8210_reset_send() - Hard resets the ca8210 for a given time
>>> + * @spi:  Pointer to target ca8210 spi device
>>> + * @ms:   Milliseconds to hold the reset line low for
>>> + */
>>> +static void ca8210_reset_send(struct spi_device *spi, unsigned int ms)
>>> +{
>>> +    struct ca8210_platform_data *pdata = spi->dev.platform_data;
>>> +    struct ca8210_priv *priv = spi_get_drvdata(spi);
>>> +    unsigned long startjiffies;
>>> +
>>> +    #define RESET_OFF 0
>>> +    #define RESET_ON 1
>>
>>
>> I#m pretty sure I commented on this before. Having this define and
>> undef in function scope just for having a 0 and a 1 used once is to
>> much. Either have globale defines like #define OFF 0 and #define ON 1
>> to be used over the driver or just simply use 0 or 1. What 0 and 1
>> means is pretty clear, the magic number we do not want are things like
>> reg = 0x73 where we have no idea what they mean.
>>
>> This is true for other instances of this define undef pattern all over
>> the code.
>
> My original thinking was that to someone who didn't know that the reset
> was active-low there was a possibility for some misunderstanding. After
> looking again though I think you're right and it's fine to just have a 1
> and 0.

Good, thanks.

> Regarding the other instances I wanted some way to make things a bit
> more readable (|DATAREQ.msdu_length| vs
> |command.pdata.data_req.msdulength|) but also wanted the macro
> definition visible when looking at the function so it's clear what
> DATAREQ actually means.


Matter of taste surely. I find the long form pretty readable without 
having to refer to the macro again what it read stands for.

  Would just declaring a pointer locally be a
> better way of doing this? I just thought it's a waste of memory since
> it's constant but I guess it's totally negligible.

Having the local pointer should really not be a problem here and would 
make things a lot more comfortable. :)

[Snip]

>>> +
>>> +/**
>>> + * tdme_chipinit() - TDME Chip Register Default Initialisation Macro
>>> + * @device_ref: Nondescript pointer to target device
>>> + *
>>> + * Return: 802.15.4 status code of API calls
>>> + */
>>> +static uint8_t tdme_chipinit(void *device_ref)
>>> +{
>>> +    uint8_t status = MAC_SUCCESS;
>>> +    uint8_t sfr_address;
>>> +    struct spi_device *spi = device_ref;
>>> +
>>> +    if ((status = tdme_setsfr_request_sync(
>>> +        1, (sfr_address = CA8210_SFR_LNAGX40), 0x29, device_ref))
>>
>>
>> here, and all the examples below, we now have the kind of magic
>> numbers that tell other developers nothing when looking at the driver.
>> Having defines with a meaningful name would be much better.
>>
>
> The following are different LNA gain values in 1/4dB steps so they are
> simple values which have just been calibrated for the hardware, is it
> enough just to give them names like LNAGX41_DEFAULT_GAIN? Or maybe
> LNAGX41_21DB_GAIN? feel like any more explanation of how those values
> are derived is out of the scope of the driver and one should really be
> consulting the extended hardware documentation. I agree the numbers are
> pretty meaningless to readers but I'm not sure where to draw the line
> with firmware-related documentation.


Such names are already a step forward and enough from my view. These 
values are pretty clear to you, but you have to keep in mind that you 
have been working with this specific hardware a long time already. 
Writing drivers for different systems for it already. For newcomers 
looking at this driver even this simple names have a good value over the 
plain magic numbers.

[Snip]

>>> +static int ca8210_skb_rx(
>>> +    struct ieee802154_hw  *hw,
>>> +    size_t                 len,
>>> +    uint8_t               *data_ind
>>> +)
>>> +{
>>> +    struct ieee802154_hdr hdr;
>>> +    int msdulen;
>>> +    int hlen;
>>> +    struct sk_buff *skb;
>>> +    struct ca8210_priv *priv = hw->priv;
>>> +
>>> +    /* Allocate mtu size buffer for every rx packet */
>>> +    skb = dev_alloc_skb(IEEE802154_MTU + sizeof(hdr));
>>> +    if (skb == NULL) {
>>> +        dev_crit(&priv->spi->dev, "dev_alloc_skb failed\n");
>>> +        return -ENOMEM;
>>> +    }
>>> +    skb_reserve(skb, sizeof(hdr));
>>> +
>>> +    msdulen = data_ind[22]; /* msdu_length */
>>> +    if (msdulen > 127) {
>>> +        dev_err(
>>> +            &priv->spi->dev,
>>> +            "received erroneously large msdu length!\n"
>>> +        );
>>> +        kfree_skb(skb);
>>> +        return -EMSGSIZE;
>>> +    }
>>> +    dev_dbg(&priv->spi->dev, "skb buffer length = %d\n", msdulen);
>>> +
>>> +    /* Populate hdr */
>>> +    hdr.sec.level = data_ind[29 + msdulen];
>>> +    dev_dbg(&priv->spi->dev, "security level: %#03x\n", hdr.sec.level);
>>> +    if (hdr.sec.level > 0) {
>>> +        hdr.sec.key_id_mode = data_ind[30 + msdulen];
>>> +        memcpy(&hdr.sec.extended_src, &data_ind[31 + msdulen], 8);
>>> +        hdr.sec.key_id = data_ind[39 + msdulen];
>>> +    }
>>> +    hdr.source.mode = data_ind[0];
>>> +    dev_dbg(&priv->spi->dev, "srcAddrMode: %#03x\n", hdr.source.mode);
>>> +    hdr.source.pan_id = *(uint16_t *)&data_ind[1];
>>> +    dev_dbg(&priv->spi->dev, "srcPanId: %#06x\n", hdr.source.pan_id);
>>> +    memcpy(&hdr.source.extended_addr, &data_ind[3], 8);
>>> +    hdr.dest.mode = data_ind[11];
>>> +    dev_dbg(&priv->spi->dev, "dstAddrMode: %#03x\n", hdr.dest.mode);
>>> +    hdr.dest.pan_id = *(uint16_t *)&data_ind[12];
>>> +    dev_dbg(&priv->spi->dev, "dstPanId: %#06x\n", hdr.dest.pan_id);
>>> +    memcpy(&hdr.dest.extended_addr, &data_ind[14], 8);
>>> +
>>> +    /* Fill in FC implicitly */
>>> +    hdr.fc.type = 1; /* Data frame */
>>
>> Pretty hard core hard coding of the frame type :)
>>
>> Right now that works as we do not support beacon or mac command frames
>> and ack frames are handled on hardware level but when that changes
>> this will just break apart.
>>
>
> Yep, the whole hardmac device/softmac driver situation is not ideal, a
> conversion to a hardmac driver is what's really needed going forward. I
> mean as it stands this function is only called on receipt of an
> MCPS-DATA.indication so it /will/ always be the receipt of a data frame
> being reported but yeah it's a bit... painful

:D

As you have been running into all these problems it would be great if 
you could scribble them down as some notes. Based on that we can start 
discussion what needs to be done in the stack to support hardMAC devices 
better. Having the notes of someone who already worked around things 
would be really useful. Nothing we need to discuss here and now but 
after the driver is in that would become interesting.


>>
>>> +    if (hdr.sec.level)
>>> +        hdr.fc.security_enabled = 1;
>>> +    else
>>> +        hdr.fc.security_enabled = 0;
>>> +    if (data_ind[1] != data_ind[12] || data_ind[2] != data_ind[13])
>>> +        hdr.fc.intra_pan = 1;
>>> +    else
>>> +        hdr.fc.intra_pan = 0;
>>> +    hdr.fc.dest_addr_mode = hdr.dest.mode;
>>> +    hdr.fc.source_addr_mode = hdr.source.mode;
>>> +
>>> +    /* Add hdr to front of buffer */
>>> +    hlen = ieee802154_hdr_push(skb, &hdr);
>>> +
>>> +    if (hlen < 0) {
>>> +        dev_crit(&priv->spi->dev, "failed to push mac hdr onto
>>> skb!\n");
>>> +        kfree_skb(skb);
>>> +        return hlen;
>>> +    }
>>> +
>>> +    skb_reset_mac_header(skb);
>>> +    skb->mac_len = hlen;
>>> +
>>> +    /* Add <msdulen> bytes of space to the back of the buffer */
>>> +    /* Copy msdu to skb */
>>> +    memcpy(skb_put(skb, msdulen), &data_ind[29], msdulen);
>>> +
>>> +    ieee802154_rx_irqsafe(hw, skb, data_ind[23]/*LQI*/);
>>> +    /* TODO: Set protocol & checksum? */
>>> +    /* TODO: update statistics */
>>
>> I hope that you will come to this TODOs at some point and they are not
>> going to stay in the driver forever after initial submission. No bad
>> feelings but I have seen this very often thus I give a bit more
>> pressure on the initial set of patches. :)
>
> Sorry I think it's actually more the case that those TODOs shouldn't be
> there. They were really a reminder for myself to check out whether or
> not they're needed but the IEEE802154_HW_OMIT_CHECKSUM flag removes the
> need for that and my understanding is that the statistics are handled
> further up the stack. I'll get rid of them.

Even better :)

[Snip]

>>> +static int ca8210_set_channel(
>>> +    struct ieee802154_hw  *hw,
>>> +    uint8_t                page,
>>> +    uint8_t                channel
>>> +)
>>> +{
>>> +    uint8_t status;
>>> +    struct ca8210_priv *priv = hw->priv;
>>> +
>>> +    status = mlme_set_request_sync(
>>> +        PHY_CURRENT_CHANNEL,
>>> +        0,
>>> +        1,
>>> +        &channel,
>>> +        priv->spi
>>> +    );
>>
>>
>> Thmlme commands figures out the page the channel is on on the channel
>> number alone?
>
> The channel page is stored in a separate PIB attribute phyCurrentPage
> which since the CA8210 supports only a 2.4GHz O-QPSK PHY is read-only as
> 0. I guess there should be some check of the channel page and the
> function should fail if !=0? I'm not sure if that makes sense in the
> spec actually.

Nah, sorry, I forgot that the hardware only supports channels which are 
implicitly on page 0. Since we added support for newer specs or and more 
frequencies and pages within the stack. In you case its fine to keep it 
like this. Maybe adding a comment why the page is always 0 in your case, 
not sure if this is really worth it though.

[Snip]

>>> +static const struct ieee802154_ops ca8210_phy_ops = {
>>> +    .start = ca8210_start,
>>> +    .stop = ca8210_stop,
>>> +    .xmit_sync = ca8210_xmit_sync,
>>> +    .xmit_async = ca8210_xmit_async,
>>> +    .ed = ca8210_get_ed,
>>> +    .set_channel = ca8210_set_channel,
>>> +    .set_hw_addr_filt = ca8210_set_hw_addr_filt,
>>> +    .set_txpower = ca8210_set_tx_power,
>>> +    .set_cca_mode = ca8210_set_cca_mode,
>>> +    .set_cca_ed_level = ca8210_set_cca_ed_level,
>>> +    .set_csma_params = ca8210_set_csma_params,
>>> +    .set_frame_retries = ca8210_set_frame_retries
>>
>>
>> I miss the .set_promiscuous_mode function here. Any plans to support
>> this?
>
> The problem with promiscuous mode is again related to the fact that the
> ca8210 actually has a hardmac and the fact that the spec doesn't
> actually define a way of passing promiscuous frames out of the MAC. So a
> received PSDU skips frame-filtering, okay, but how does that frame then
> get communicated above the MAC? The frame was not anticipated so there's
> no direct translation to a SAP command. In the ca8210 we just put the
> contents of the PSDU into the MSDU parameter of an MCPS-DATA.indication
> with the other parameters set to 0. This works just fine but then the
> way the frame is extracted/constructed for the stack changes. I decided
> that it wasn't worth supporting promiscuous mode for this
> implementation, at least not for the time being.

OK, I see. This is not needed for the initial submission. We can work 
out a way to get this supported after the driver was merged .

[Snip]

>>> +static int ca8210_remove(struct spi_device *spi_device)
>>> +{
>>> +    struct ca8210_priv *priv;
>>> +    struct ca8210_platform_data *pdata;
>>> +
>>> +    dev_info(&spi_device->dev, "Removing ca8210\n");
>>> +
>>> +    pdata = spi_device->dev.platform_data;
>>> +    if (pdata) {
>>> +        if (pdata->extclockenable) {
>>> +            ca8210_unregister_ext_clock(spi_device);
>>> +            ca8210_config_extern_clk(pdata, spi_device, 0);
>>> +        }
>>> +        free_irq(pdata->irq_id, spi_device->dev.driver_data);
>>> +        kfree(pdata);
>>> +        spi_device->dev.platform_data = NULL;
>>> +    }
>>> +    /* get spi_device private data */
>>> +    priv = spi_get_drvdata(spi_device);
>>> +    if (priv) {
>>> +        dev_info(
>>> +            &spi_device->dev,
>>> +            "sync_down = %d, sync_up = %d\n",
>>> +            priv->sync_down,
>>> +            priv->sync_up
>>> +        );
>>> +        ca8210_dev_com_clear(spi_device->dev.driver_data);
>>> +        if (priv->hw) {
>>> +            if (priv->hw_registered) {
>>> +                ieee802154_unregister_hw(priv->hw);
>>> +            }
>>> +            ieee802154_free_hw(priv->hw);
>>> +            priv->hw = NULL;
>>> +            dev_info(
>>> +                &spi_device->dev,
>>> +                "Unregistered & freed ieee802154_hw.\n"
>>> +            );
>>> +        }
>>> +        ca8210_test_interface_clear(priv);
>>
>>
>> Are you sure that it is save to remove the test interface even if the
>> DEBUG_FS option is not set. It should not have been created in that
>> case. You do not check for the KCONFIG option here while you do when
>> creating.
>
> Nope you're absolutely right, just an oversight on my part, thanks for
> spotting that.


No problem, thats what reviews are for.

[Snip]

>> regards
>> Stefan Schmidt
>>
>
> Thanks for picking up on all this, I'll start making changes.

Great. With the comments from above and the warnings and errors for 
CheckPatch fixed I should be satisfied and we should finally be really 
close in getting this merged. Thanks for keeping up the work on this. 
The initial submission of such a large drivers is always a bit lengthy. 
It will get better for upcoming enhancements and fixes. :)

regards
Stefan Schmidt