* Re: [PATCH 1/3] NFC: trf7970a: add device tree option for 27MHz clock
From: Geoff Lansberry @ 2016-12-20 18:29 UTC (permalink / raw)
To: Mark Greer
Cc: linux-wireless, Lauro Ramos Venancio, Aloisio Almeida Jr,
Samuel Ortiz, robh+dt, mark.rutland, netdev, devicetree,
linux-kernel, Justin Bronder
In-Reply-To: <20161220181157.GB30273@animalcreek.com>
On Tue, Dec 20, 2016 at 1:11 PM, Mark Greer <mgreer@animalcreek.com> wrote:
> Hi Geoff.
>
> Please put the version in your subjects when submitting anything but the
> initial version of a patch (e.g., [PATCH v2 1/3]).
>
> Which series do you want reviewed?
>
> Mark
> --
Sorry about the double posting, I had forgotten to erase the patches I
generated while rebasing and checking, and I'll have to figure out how
to add that v2 line to the automatically generated subject line if I
end up submitting another round.
Please review the three most recent patches, which have the send time
of 17:16.
Best Regards,
Geoff
^ permalink raw reply
* Re: [PATCH 1/3] NFC: trf7970a: add device tree option for 27MHz clock
From: Mark Greer @ 2016-12-20 18:35 UTC (permalink / raw)
To: Geoff Lansberry
Cc: linux-wireless, Lauro Ramos Venancio, Aloisio Almeida Jr,
Samuel Ortiz, robh+dt, mark.rutland, netdev, devicetree,
linux-kernel, Justin Bronder
In-Reply-To: <CAO7Z3WLYMZ3rRGKpksYnRksbC=A1WNog913UJFiPsAT7ymOnaw@mail.gmail.com>
On Tue, Dec 20, 2016 at 01:29:13PM -0500, Geoff Lansberry wrote:
> On Tue, Dec 20, 2016 at 1:11 PM, Mark Greer <mgreer@animalcreek.com> wrote:
> > Hi Geoff.
> >
> > Please put the version in your subjects when submitting anything but the
> > initial version of a patch (e.g., [PATCH v2 1/3]).
> >
> > Which series do you want reviewed?
> >
> > Mark
> > --
> Sorry about the double posting, I had forgotten to erase the patches I
> generated while rebasing and checking, and I'll have to figure out how
> to add that v2 line to the automatically generated subject line if I
> end up submitting another round.
Hint: -v <n> option of 'git format-patch'
> Please review the three most recent patches, which have the send time
> of 17:16.
Okay, thank.
Mark
--
^ permalink raw reply
* Re: [RFC v2 11/11] ath10k: Added sdio support
From: Erik Stromdahl @ 2016-12-20 18:14 UTC (permalink / raw)
To: Valo, Kalle; +Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org
In-Reply-To: <871sx8unjn.fsf@kamboji.qca.qualcomm.com>
On 12/16/2016 12:21 PM, Valo, Kalle wrote:
> Erik Stromdahl <erik.stromdahl@gmail.com> writes:
>
>> Initial HIF sdio/mailbox implementation.
>>
>> Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com>
>
> I know most of this coming from ath6kl but I think we should still
> improve the code. Lots of comments will follow, don't get scared :)
>
I have started to fix most of the review comments below and added a new
branch on my git repo (https://github.com/erstrom/linux-ath) for this:
ath-201612150919-ath10k-sdio-kvalo-review
The changes are quite massive and I am not entirely finished yet.
I will of course squash these commits before submitting a new RFC.
You can have a look to see where I am heading.
The dma_buffer removal was a little bit tricky since there are a lot of
places in the code where the sdio functions are called with stack
allocated buffers. This does not seem to be a problem on the hardware I
am running (NXP/Freescale iMX6ul) but I am afraid that there could be
problems on other architectures.
Therefore I have introduced some temporary dynamically allocated buffers
on a few places.
>> +#define CALC_TXRX_PADDED_LEN(ar_sdio, len) \
>> + (__ALIGN_MASK((len), (ar_sdio)->mbox_info.block_mask))
>
> I think this could be a proper static inline function. Andrew Morton
> once said: "Write in C, not CPP" (or something like that), I think
> that's a really good point.
>
>> +static int ath10k_sdio_read_write_sync(struct ath10k *ar, u32 addr, u8 *buf,
>> + u32 len, u32 request);
>> +static int ath10k_sdio_hif_diag_read(struct ath10k *ar, u32 address, void *buf,
>> + size_t buf_len);
>> +static int ath10k_sdio_hif_diag_read32(struct ath10k *ar, u32 address,
>> + u32 *value);
>
> We prefer to avoid forward declarations if at all possible. I didn't
> check but if there's a clean way to avoid these please remove them.
>
>> +/* HIF mbox interrupt handling */
>> +
>> +static int ath10k_sdio_mbox_rx_process_packet(struct ath10k_sdio *ar_sdio,
>> + struct ath10k_sdio_rx_data *pkt,
>> + u32 *lookaheads,
>> + int *n_lookaheads)
>> +{
>
> So the style in ath10k is that all functions (of course this doesn't
> apply to callbacks etc) have "struct ath10k *ar" as the first parameter.
> This way there's no need to check if a function takes ar or ar_sdio.
>
>> + int status = 0;
>
> In ath10k we prefer to use ret. And avoid initialising it, please.
>
>> + struct ath10k_htc *htc = &ar_sdio->ar->htc;
>> + struct sk_buff *skb = pkt->skb;
>> + struct ath10k_htc_hdr *htc_hdr = (struct ath10k_htc_hdr *)skb->data;
>> + bool trailer_present = htc_hdr->flags & ATH10K_HTC_FLAG_TRAILER_PRESENT;
>> + u16 payload_len;
>> +
>> + payload_len = le16_to_cpu(htc_hdr->len);
>> +
>> + if (trailer_present) {
>> + u8 *trailer;
>> + enum ath10k_htc_ep_id eid;
>> +
>> + trailer = skb->data + sizeof(struct ath10k_htc_hdr) +
>> + payload_len - htc_hdr->trailer_len;
>> +
>> + eid = (enum ath10k_htc_ep_id)htc_hdr->eid;
>
> A some kind of mapping function for ep id would be nice, it makes it
> more visible how it works.
>
>> +static int ath10k_sdio_mbox_rx_process_packets(struct ath10k_sdio *ar_sdio,
>> + u32 lookaheads[],
>> + int *n_lookahead)
>> +{
>> + struct ath10k *ar = ar_sdio->ar;
>> + struct ath10k_htc *htc = &ar->htc;
>> + struct ath10k_sdio_rx_data *pkt;
>> + int status = 0, i;
>> +
>> + for (i = 0; i < ar_sdio->n_rx_pkts; i++) {
>> + struct ath10k_htc_ep *ep;
>> + enum ath10k_htc_ep_id id;
>> + u32 *lookaheads_local = lookaheads;
>> + int *n_lookahead_local = n_lookahead;
>> +
>> + id = ((struct ath10k_htc_hdr *)&lookaheads[i])->eid;
>> +
>> + if (id >= ATH10K_HTC_EP_COUNT) {
>> + ath10k_err(ar, "Invalid endpoint in look-ahead: %d\n",
>> + id);
>
> In ath10k we use ath10k_err() for errors from which can't survive and
> ath10k_warn() for errors where we try to continue. So ath10k_warn()
> would be more approriate here and most of other cases in sdio.c
>
>> + status = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + ep = &htc->endpoint[id];
>> +
>> + if (ep->service_id == 0) {
>> + ath10k_err(ar, "ep %d is not connected !\n", id);
>> + status = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + pkt = &ar_sdio->rx_pkts[i];
>> +
>> + if (pkt->part_of_bundle && !pkt->last_in_bundle) {
>> + /* Only read lookahead's from RX trailers
>> + * for the last packet in a bundle.
>> + */
>> + lookaheads_local = NULL;
>> + n_lookahead_local = NULL;
>> + }
>> +
>> + status = ath10k_sdio_mbox_rx_process_packet(ar_sdio,
>> + pkt,
>> + lookaheads_local,
>> + n_lookahead_local);
>> + if (status)
>> + goto out;
>> +
>> + ep->ep_ops.ep_rx_complete(ar_sdio->ar, pkt->skb);
>> + /* The RX complete handler now owns the skb...*/
>> + pkt->skb = NULL;
>> + pkt->alloc_len = 0;
>> + }
>> +
>> +out:
>> + /* Free all packets that was not passed on to the RX completion
>> + * handler...
>> + */
>> + for (; i < ar_sdio->n_rx_pkts; i++)
>> + ath10k_sdio_mbox_free_rx_pkt(&ar_sdio->rx_pkts[i]);
>
> I got a bit fooled by not initialising i here and only then realised
> why. I guess it's ok but a bit of so and so
>
>> +
>> + return status;
>> +}
>> +
>> +static int alloc_pkt_bundle(struct ath10k *ar,
>> + struct ath10k_sdio_rx_data *rx_pkts,
>> + struct ath10k_htc_hdr *htc_hdr,
>> + size_t full_len, size_t act_len, size_t *bndl_cnt)
>> +{
>> + int i, status = 0;
>> +
>> + *bndl_cnt = (htc_hdr->flags & ATH10K_HTC_FLAG_BUNDLE_MASK) >>
>> + ATH10K_HTC_FLAG_BUNDLE_LSB;
>
> We recently got FIELD_GET() and FIELD_PREP() to kernel for handling
> bitmasks. ath10k is not yet converted (patches welcome!) but it would be
> good to use those already in sdio.c. Also SM() could be replaced with
> those.
>
>> +int ath10k_sdio_mbox_rxmsg_pending_handler(struct ath10k_sdio *ar_sdio,
>> + u32 msg_lookahead, bool *done)
>> +{
>> + struct ath10k *ar = ar_sdio->ar;
>> + int status = 0;
>> + u32 lookaheads[ATH10K_SDIO_MAX_RX_MSGS];
>> + int n_lookaheads = 1;
>> +
>> + *done = true;
>> +
>> + /* Copy the lookahead obtained from the HTC register table into our
>> + * temp array as a start value.
>> + */
>> + lookaheads[0] = msg_lookahead;
>> +
>> + for (;;) {
>
> Iternal loops in kernel can be dangerous. Better to add some sort of
> timeout check with a warning message, something like:
>
> while ((time_before(jiffies, timeout)) {
> }
>
> if (timed out)
> ath10k_warn("timeout in foo");
>
>> + /* Try to allocate as many HTC RX packets indicated by
>> + * n_lookaheads.
>> + */
>> + status = ath10k_sdio_mbox_rx_alloc(ar_sdio, lookaheads,
>> + n_lookaheads);
>> + if (status)
>> + break;
>> +
>> + if (ar_sdio->n_rx_pkts >= 2)
>> + /* A recv bundle was detected, force IRQ status
>> + * re-check again.
>> + */
>> + *done = false;
>> +
>> + status = ath10k_sdio_mbox_rx_fetch(ar_sdio);
>> +
>> + /* Process fetched packets. This will potentially update
>> + * n_lookaheads depending on if the packets contain lookahead
>> + * reports.
>> + */
>> + n_lookaheads = 0;
>> + status = ath10k_sdio_mbox_rx_process_packets(ar_sdio,
>> + lookaheads,
>> + &n_lookaheads);
>> +
>> + if (!n_lookaheads || status)
>> + break;
>> +
>> + /* For SYNCH processing, if we get here, we are running
>> + * through the loop again due to updated lookaheads. Set
>> + * flag that we should re-check IRQ status registers again
>> + * before leaving IRQ processing, this can net better
>> + * performance in high throughput situations.
>> + */
>> + *done = false;
>> + }
>> +
>> + if (status && (status != -ECANCELED))
>> + ath10k_err(ar, "failed to get pending recv messages: %d\n",
>> + status);
>> +
>> + if (atomic_read(&ar_sdio->stopping)) {
>> + ath10k_warn(ar, "host is going to stop. Turning of RX\n");
>> + ath10k_sdio_hif_rx_control(ar_sdio, false);
>> + }
>
> I'm always skeptic when I use atomic variables used like this, I doubt
> it's really correct.
>
>> +
>> + return status;
>> +}
>> +
>> +static int ath10k_sdio_mbox_proc_dbg_intr(struct ath10k_sdio *ar_sdio)
>> +{
>> + int ret;
>> + u32 dummy;
>> + struct ath10k *ar = ar_sdio->ar;
>> +
>> + ath10k_warn(ar, "firmware crashed\n");
>
> We have firmware crash dump support in ath10k. You could add a "TODO:"
> comment about implementing that later.
>
>> +static int ath10k_sdio_mbox_proc_err_intr(struct ath10k_sdio *ar_sdio)
>> +{
>> + int status;
>> + u8 error_int_status;
>> + u8 reg_buf[4];
>> + struct ath10k_sdio_irq_data *irq_data = &ar_sdio->irq_data;
>> + struct ath10k *ar = ar_sdio->ar;
>> +
>> + ath10k_dbg(ar, ATH10K_DBG_SDIO, "error interrupt\n");
>> +
>> + error_int_status = irq_data->irq_proc_reg.error_int_status & 0x0F;
>> + if (!error_int_status) {
>> + WARN_ON(1);
>> + return -EIO;
>> + }
>> +
>> + ath10k_dbg(ar, ATH10K_DBG_SDIO,
>> + "valid interrupt source(s) in ERROR_INT_STATUS: 0x%x\n",
>> + error_int_status);
>> +
>> + if (MS(error_int_status, MBOX_ERROR_INT_STATUS_WAKEUP))
>> + ath10k_dbg(ar, ATH10K_DBG_SDIO, "error : wakeup\n");
>> +
>> + if (MS(error_int_status, MBOX_ERROR_INT_STATUS_RX_UNDERFLOW))
>> + ath10k_err(ar, "rx underflow\n");
>> +
>> + if (MS(error_int_status, MBOX_ERROR_INT_STATUS_TX_OVERFLOW))
>> + ath10k_err(ar, "tx overflow\n");
>> +
>> + /* Clear the interrupt */
>> + irq_data->irq_proc_reg.error_int_status &= ~error_int_status;
>> +
>> + /* set W1C value to clear the interrupt, this hits the register first */
>> + reg_buf[0] = error_int_status;
>> + reg_buf[1] = 0;
>> + reg_buf[2] = 0;
>> + reg_buf[3] = 0;
>> +
>> + status = ath10k_sdio_read_write_sync(ar,
>> + MBOX_ERROR_INT_STATUS_ADDRESS,
>> + reg_buf, 4, HIF_WR_SYNC_BYTE_FIX);
>> +
>> + WARN_ON(status);
>
> This is a bit dangerous, in worst case it can spam the kernel log and
> force a host reboot due watchdog timing out etc.
>
> Better to replace with standard warning message:
>
> ret = ath10k_sdio_read_write_sync(ar,
> MBOX_ERROR_INT_STATUS_ADDRESS,
> reg_buf, 4, HIF_WR_SYNC_BYTE_FIX);
> if (ret) {
> ath10k_warn("failed to read interrupr status: %d", ret);
> return ret;
> }
>
>> +static int ath10k_sdio_mbox_proc_cpu_intr(struct ath10k_sdio *ar_sdio)
>> +{
>> + int status;
>> + struct ath10k_sdio_irq_data *irq_data = &ar_sdio->irq_data;
>> + struct ath10k *ar = ar_sdio->ar;
>> + u8 cpu_int_status, reg_buf[4];
>> +
>> + cpu_int_status = irq_data->irq_proc_reg.cpu_int_status &
>> + irq_data->irq_en_reg.cpu_int_status_en;
>> + if (!cpu_int_status) {
>> + WARN_ON(1);
>> + return -EIO;
>> + }
>
> Ditto about WARN_ON(), ath10k_warn() is much better.
>
>> +/* process pending interrupts synchronously */
>> +static int ath10k_sdio_mbox_proc_pending_irqs(struct ath10k_sdio *ar_sdio,
>> + bool *done)
>> +{
>> + struct ath10k_sdio_irq_data *irq_data = &ar_sdio->irq_data;
>> + struct ath10k *ar = ar_sdio->ar;
>> + struct ath10k_sdio_irq_proc_registers *rg;
>> + int status = 0;
>> + u8 host_int_status = 0;
>> + u32 lookahead = 0;
>> + u8 htc_mbox = 1 << ATH10K_HTC_MAILBOX;
>> +
>> + /* NOTE: HIF implementation guarantees that the context of this
>> + * call allows us to perform SYNCHRONOUS I/O, that is we can block,
>> + * sleep or call any API that can block or switch thread/task
>> + * contexts. This is a fully schedulable context.
>> + */
>> +
>> + /* Process pending intr only when int_status_en is clear, it may
>> + * result in unnecessary bus transaction otherwise. Target may be
>> + * unresponsive at the time.
>> + */
>> + if (irq_data->irq_en_reg.int_status_en) {
>> + /* Read the first sizeof(struct ath10k_irq_proc_registers)
>> + * bytes of the HTC register table. This
>> + * will yield us the value of different int status
>> + * registers and the lookahead registers.
>> + */
>> + status = ath10k_sdio_read_write_sync(
>> + ar,
>> + MBOX_HOST_INT_STATUS_ADDRESS,
>> + (u8 *)&irq_data->irq_proc_reg,
>> + sizeof(irq_data->irq_proc_reg),
>> + HIF_RD_SYNC_BYTE_INC);
>> + if (status)
>> + goto out;
>> +
>> + /* Update only those registers that are enabled */
>> + host_int_status = irq_data->irq_proc_reg.host_int_status &
>> + irq_data->irq_en_reg.int_status_en;
>> +
>> + /* Look at mbox status */
>> + if (host_int_status & htc_mbox) {
>> + /* Mask out pending mbox value, we use look ahead as
>> + * the real flag for mbox processing.
>> + */
>> + host_int_status &= ~htc_mbox;
>> + if (irq_data->irq_proc_reg.rx_lookahead_valid &
>> + htc_mbox) {
>> + rg = &irq_data->irq_proc_reg;
>> + lookahead = le32_to_cpu(
>> + rg->rx_lookahead[ATH10K_HTC_MAILBOX]);
>> + if (!lookahead)
>> + ath10k_err(ar, "lookahead is zero!\n");
>> + }
>> + }
>> + }
>> +
>> + if (!host_int_status && !lookahead) {
>> + *done = true;
>> + goto out;
>> + }
>> +
>> + if (lookahead) {
>> + ath10k_dbg(ar, ATH10K_DBG_SDIO,
>> + "pending mailbox msg, lookahead: 0x%08X\n",
>> + lookahead);
>> +
>> + status = ath10k_sdio_mbox_rxmsg_pending_handler(ar_sdio,
>> + lookahead,
>> + done);
>> + if (status)
>> + goto out;
>> + }
>> +
>> + /* now, handle the rest of the interrupts */
>> + ath10k_dbg(ar, ATH10K_DBG_SDIO,
>> + "valid interrupt source(s) for other interrupts: 0x%x\n",
>> + host_int_status);
>> +
>> + if (MS(host_int_status, MBOX_HOST_INT_STATUS_CPU)) {
>> + /* CPU Interrupt */
>> + status = ath10k_sdio_mbox_proc_cpu_intr(ar_sdio);
>> + if (status)
>> + goto out;
>> + }
>> +
>> + if (MS(host_int_status, MBOX_HOST_INT_STATUS_ERROR)) {
>> + /* Error Interrupt */
>> + status = ath10k_sdio_mbox_proc_err_intr(ar_sdio);
>> + if (status)
>> + goto out;
>> + }
>> +
>> + if (MS(host_int_status, MBOX_HOST_INT_STATUS_COUNTER))
>> + /* Counter Interrupt */
>> + status = ath10k_sdio_mbox_proc_counter_intr(ar_sdio);
>> +
>> +out:
>> + /* An optimization to bypass reading the IRQ status registers
>> + * unecessarily which can re-wake the target, if upper layers
>> + * determine that we are in a low-throughput mode, we can rely on
>> + * taking another interrupt rather than re-checking the status
>> + * registers which can re-wake the target.
>> + *
>> + * NOTE : for host interfaces that makes use of detecting pending
>> + * mbox messages at hif can not use this optimization due to
>> + * possible side effects, SPI requires the host to drain all
>> + * messages from the mailbox before exiting the ISR routine.
>> + */
>> +
>> + ath10k_dbg(ar, ATH10K_DBG_SDIO,
>> + "%s: (done:%d, status=%d)\n", __func__, *done, status);
>
> We try to follow this kind of format for debug messages:
>
> "sdio pending irqs done %d status %d"
>
> So start with the debug level name followed by the debug separated with spaces.
>
> And IIRC no need for "\n", the macro adds that automatically.
>
>> +
>> + return status;
>> +}
>> +
>> +/* Macro to check if DMA buffer is WORD-aligned and DMA-able.
>> + * Most host controllers assume the buffer is DMA'able and will
>> + * bug-check otherwise (i.e. buffers on the stack). virt_addr_valid
>> + * check fails on stack memory.
>> + */
>> +static inline bool buf_needs_bounce(u8 *buf)
>> +{
>> + return ((unsigned long)buf & 0x3) || !virt_addr_valid(buf);
>> +}
>
> IS_ALIGNED()? And this is super ugly, do we really need this? I would
> much prefer that we would directly use struct sk_buff, more of that
> later.
>
>> +static inline enum ath10k_htc_ep_id pipe_id_to_eid(u8 pipe_id)
>> +{
>> + return (enum ath10k_htc_ep_id)pipe_id;
>> +}
>
> Oh, we already have a this kind of mapping function? Can't this be used
> earlier?
>
>> +static void ath10k_sdio_set_mbox_info(struct ath10k_sdio *ar_sdio)
>> +{
>> + struct ath10k_mbox_info *mbox_info = &ar_sdio->mbox_info;
>> + u16 device = ar_sdio->func->device;
>> +
>> + mbox_info->htc_addr = ATH10K_HIF_MBOX_BASE_ADDR;
>> + mbox_info->block_size = ATH10K_HIF_MBOX_BLOCK_SIZE;
>> + mbox_info->block_mask = ATH10K_HIF_MBOX_BLOCK_SIZE - 1;
>> + mbox_info->gmbox_addr = ATH10K_HIF_GMBOX_BASE_ADDR;
>> + mbox_info->gmbox_sz = ATH10K_HIF_GMBOX_WIDTH;
>> +
>> + mbox_info->ext_info[0].htc_ext_addr = ATH10K_HIF_MBOX0_EXT_BASE_ADDR;
>> +
>> + if ((device & ATH10K_MANUFACTURER_ID_REV_MASK) < 4)
>> + mbox_info->ext_info[0].htc_ext_sz = ATH10K_HIF_MBOX0_EXT_WIDTH;
>> + else
>> + /* from rome 2.0(0x504), the width has been extended
>> + * to 56K
>> + */
>> + mbox_info->ext_info[0].htc_ext_sz =
>> + ATH10K_HIF_MBOX0_EXT_WIDTH_ROME_2_0;
>> +
>> + mbox_info->ext_info[1].htc_ext_addr =
>> + mbox_info->ext_info[0].htc_ext_addr +
>> + mbox_info->ext_info[0].htc_ext_sz +
>> + ATH10K_HIF_MBOX_DUMMY_SPACE_SIZE;
>> + mbox_info->ext_info[1].htc_ext_sz = ATH10K_HIF_MBOX1_EXT_WIDTH;
>> +}
>> +
>> +static inline void ath10k_sdio_set_cmd52_arg(u32 *arg, u8 write, u8 raw,
>> + unsigned int address,
>> + unsigned char val)
>> +{
>> + const u8 func = 0;
>> +
>> + *arg = ((write & 1) << 31) |
>> + ((func & 0x7) << 28) |
>> + ((raw & 1) << 27) |
>> + (1 << 26) |
>> + ((address & 0x1FFFF) << 9) |
>> + (1 << 8) |
>> + (val & 0xFF);
>> +}
>
> Quite ugly. FIELD_PREP() & co?
>
>> +
>> +static int ath10k_sdio_func0_cmd52_wr_byte(struct mmc_card *card,
>> + unsigned int address,
>> + unsigned char byte)
>> +{
>> + struct mmc_command io_cmd;
>> +
>> + memset(&io_cmd, 0, sizeof(io_cmd));
>> + ath10k_sdio_set_cmd52_arg(&io_cmd.arg, 1, 0, address, byte);
>> + io_cmd.opcode = SD_IO_RW_DIRECT;
>> + io_cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
>> +
>> + return mmc_wait_for_cmd(card->host, &io_cmd, 0);
>> +}
>> +
>> +static int ath10k_sdio_func0_cmd52_rd_byte(struct mmc_card *card,
>> + unsigned int address,
>> + unsigned char *byte)
>> +{
>> + int ret;
>> + struct mmc_command io_cmd;
>> +
>> + memset(&io_cmd, 0, sizeof(io_cmd));
>> + ath10k_sdio_set_cmd52_arg(&io_cmd.arg, 0, 0, address, 0);
>> + io_cmd.opcode = SD_IO_RW_DIRECT;
>> + io_cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
>> +
>> + ret = mmc_wait_for_cmd(card->host, &io_cmd, 0);
>> + if (!ret)
>> + *byte = io_cmd.resp[0];
>> +
>> + return ret;
>> +}
>> +
>> +static int ath10k_sdio_io(struct ath10k_sdio *ar_sdio, u32 request, u32 addr,
>> + u8 *buf, u32 len)
>> +{
>> + int ret = 0;
>
> Avoid these kind of unnecessary initialisations.
>
>> + struct sdio_func *func = ar_sdio->func;
>> + struct ath10k *ar = ar_sdio->ar;
>> +
>> + sdio_claim_host(func);
>> +
>> + if (request & HIF_WRITE) {
>> + if (request & HIF_FIXED_ADDRESS)
>> + ret = sdio_writesb(func, addr, buf, len);
>> + else
>> + ret = sdio_memcpy_toio(func, addr, buf, len);
>> + } else {
>> + if (request & HIF_FIXED_ADDRESS)
>> + ret = sdio_readsb(func, buf, addr, len);
>> + else
>> + ret = sdio_memcpy_fromio(func, buf, addr, len);
>> + }
>> +
>> + sdio_release_host(func);
>> +
>> + ath10k_dbg(ar, ATH10K_DBG_SDIO, "%s addr 0x%x%s buf 0x%p len %d\n",
>> + request & HIF_WRITE ? "wr" : "rd", addr,
>> + request & HIF_FIXED_ADDRESS ? " (fixed)" : "", buf, len);
>> + ath10k_dbg_dump(ar, ATH10K_DBG_SDIO_DUMP, NULL,
>> + request & HIF_WRITE ? "sdio wr " : "sdio rd ",
>> + buf, len);
>> +
>> + return ret;
>> +}
>> +
>> +static struct ath10k_sdio_bus_request
>> +*ath10k_sdio_alloc_busreq(struct ath10k_sdio *ar_sdio)
>> +{
>> + struct ath10k_sdio_bus_request *bus_req;
>> +
>> + spin_lock_bh(&ar_sdio->lock);
>> +
>> + if (list_empty(&ar_sdio->bus_req_freeq)) {
>> + spin_unlock_bh(&ar_sdio->lock);
>> + return NULL;
>> + }
>> +
>> + bus_req = list_first_entry(&ar_sdio->bus_req_freeq,
>> + struct ath10k_sdio_bus_request, list);
>> + list_del(&bus_req->list);
>> +
>> + spin_unlock_bh(&ar_sdio->lock);
>> +
>> + return bus_req;
>> +}
>> +
>> +static void ath10k_sdio_free_bus_req(struct ath10k_sdio *ar_sdio,
>> + struct ath10k_sdio_bus_request *bus_req)
>> +{
>> + spin_lock_bh(&ar_sdio->lock);
>> + list_add_tail(&bus_req->list, &ar_sdio->bus_req_freeq);
>> + spin_unlock_bh(&ar_sdio->lock);
>> +}
>> +
>> +static int ath10k_sdio_read_write_sync(struct ath10k *ar, u32 addr, u8 *buf,
>> + u32 len, u32 request)
>> +{
>> + struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
>> + u8 *tbuf = NULL;
>
> Extra space after u8?
>
>> + int ret;
>> + bool bounced = false;
>> +
>> + if (request & HIF_BLOCK_BASIS)
>> + len = round_down(len, ar_sdio->mbox_info.block_size);
>> +
>> + if (buf_needs_bounce(buf)) {
>> + if (!ar_sdio->dma_buffer)
>> + return -ENOMEM;
>> + /* FIXME: I am not sure if it is always correct to assume
>> + * that the SDIO irq is a "fake" irq and sleep is possible.
>> + * (this function will get called from
>> + * ath10k_sdio_irq_handler
>> + */
>> + mutex_lock(&ar_sdio->dma_buffer_mutex);
>> + tbuf = ar_sdio->dma_buffer;
>> +
>> + if (request & HIF_WRITE)
>> + memcpy(tbuf, buf, len);
>> +
>> + bounced = true;
>> + } else {
>> + tbuf = buf;
>> + }
>
> So I really hate that ar_sdio->dma_buffer, do we really need it? What if
> we just get rid of it and allocate struct sk_buff and pass that around?
> No need to do extra copying then, I hope :)
>
>> +
>> + ret = ath10k_sdio_io(ar_sdio, request, addr, tbuf, len);
>> + if ((request & HIF_READ) && bounced)
>> + memcpy(buf, tbuf, len);
>> +
>> + if (bounced)
>> + mutex_unlock(&ar_sdio->dma_buffer_mutex);
>> +
>> + return ret;
>> +}
>> +
>> +static void __ath10k_sdio_write_async(struct ath10k_sdio *ar_sdio,
>> + struct ath10k_sdio_bus_request *req)
>> +{
>> + int status;
>> + struct ath10k_htc_ep *ep;
>> + struct sk_buff *skb;
>> +
>> + skb = req->skb;
>> + status = ath10k_sdio_read_write_sync(ar_sdio->ar, req->address,
>> + skb->data, req->len,
>> + req->request);
>> + ep = &ar_sdio->ar->htc.endpoint[req->eid];
>> + ath10k_htc_notify_tx_completion(ep, skb);
>> + ath10k_sdio_free_bus_req(ar_sdio, req);
>> +}
>> +
>> +static void ath10k_sdio_write_async_work(struct work_struct *work)
>> +{
>> + struct ath10k_sdio *ar_sdio;
>> + struct ath10k_sdio_bus_request *req, *tmp_req;
>> +
>> + ar_sdio = container_of(work, struct ath10k_sdio, wr_async_work);
>> +
>> + spin_lock_bh(&ar_sdio->wr_async_lock);
>> + list_for_each_entry_safe(req, tmp_req, &ar_sdio->wr_asyncq, list) {
>> + list_del(&req->list);
>> + spin_unlock_bh(&ar_sdio->wr_async_lock);
>> + __ath10k_sdio_write_async(ar_sdio, req);
>> + spin_lock_bh(&ar_sdio->wr_async_lock);
>> + }
>> + spin_unlock_bh(&ar_sdio->wr_async_lock);
>> +}
>> +
>> +static void ath10k_sdio_irq_handler(struct sdio_func *func)
>> +{
>> + int status = 0;
>> + unsigned long timeout;
>> + struct ath10k_sdio *ar_sdio;
>> + bool done = false;
>> +
>> + ar_sdio = sdio_get_drvdata(func);
>> + atomic_set(&ar_sdio->irq_handling, 1);
>> +
>> + /* Release the host during interrupts so we can pick it back up when
>> + * we process commands.
>> + */
>> + sdio_release_host(ar_sdio->func);
>> +
>> + timeout = jiffies + ATH10K_SDIO_HIF_COMMUNICATION_TIMEOUT_HZ;
>> + while (time_before(jiffies, timeout) && !done) {
>> + status = ath10k_sdio_mbox_proc_pending_irqs(ar_sdio, &done);
>> + if (status)
>> + break;
>> + }
>> +
>> + sdio_claim_host(ar_sdio->func);
>> +
>> + atomic_set(&ar_sdio->irq_handling, 0);
>> + wake_up(&ar_sdio->irq_wq);
>> +
>> + WARN_ON(status && status != -ECANCELED);
>> +}
>
> Questionable use of an atomic variable again, looks like badly implement
> poor man's locking to me. And instead of wake_up() we should workqueues
> or threaded irqs (if sdio supports that).
>
>> +
>> +static int ath10k_sdio_hif_disable_intrs(struct ath10k_sdio *ar_sdio)
>> +{
>> + int ret;
>> + struct ath10k_sdio_irq_enable_reg regs;
>> + struct ath10k_sdio_irq_data *irq_data = &ar_sdio->irq_data;
>> +
>> + memset(®s, 0, sizeof(regs));
>> +
>> + ret = ath10k_sdio_read_write_sync(ar_sdio->ar,
>> + MBOX_INT_STATUS_ENABLE_ADDRESS,
>> + ®s.int_status_en, sizeof(regs),
>> + HIF_WR_SYNC_BYTE_INC);
>> + if (ret) {
>> + ath10k_err(ar_sdio->ar, "Unable to disable sdio interrupts\n");
>> + return ret;
>> + }
>> +
>> + spin_lock_bh(&irq_data->lock);
>> + irq_data->irq_en_reg = regs;
>> + spin_unlock_bh(&irq_data->lock);
>> +
>> + return 0;
>> +}
>> +
>> +static int ath10k_sdio_hif_power_up(struct ath10k *ar)
>> +{
>> + struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
>> + struct sdio_func *func = ar_sdio->func;
>> + int ret = 0;
>> +
>> + if (!ar_sdio->is_disabled)
>> + return 0;
>> +
>> + ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio power on\n");
>> +
>> + sdio_claim_host(func);
>> +
>> + ret = sdio_enable_func(func);
>> + if (ret) {
>> + ath10k_err(ar, "Unable to enable sdio func: %d)\n", ret);
>> + sdio_release_host(func);
>> + return ret;
>> + }
>> +
>> + sdio_release_host(func);
>> +
>> + /* Wait for hardware to initialise. It should take a lot less than
>> + * 20 ms but let's be conservative here.
>> + */
>> + msleep(20);
>> +
>> + ar_sdio->is_disabled = false;
>> +
>> + ret = ath10k_sdio_hif_disable_intrs(ar_sdio);
>> +
>> + return ret;
>> +}
>> +
>> +static void ath10k_sdio_hif_power_down(struct ath10k *ar)
>> +{
>> + struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
>> + int ret;
>> +
>> + if (ar_sdio->is_disabled)
>> + return;
>> +
>> + ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio power off\n");
>> +
>> + /* Disable the card */
>> + sdio_claim_host(ar_sdio->func);
>> + ret = sdio_disable_func(ar_sdio->func);
>> + sdio_release_host(ar_sdio->func);
>> +
>> + if (ret)
>> + ath10k_dbg(ar, ATH10K_DBG_BOOT,
>> + "Unable to disable sdio: %d\n", ret);
>
> Shouldn't this be ath10k_warn()?
>
>> +
>> + ar_sdio->is_disabled = true;
>> +}
>> +
>> +int ath10k_sdio_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
>> + struct ath10k_hif_sg_item *items, int n_items)
>> +{
>> + int i;
>> + u32 address;
>> + struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
>> + struct ath10k_sdio_bus_request *bus_req;
>> +
>> + bus_req = ath10k_sdio_alloc_busreq(ar_sdio);
>> +
>> + if (WARN_ON_ONCE(!bus_req))
>> + return -ENOMEM;
>
> Is ath10k_warn() more approriate?
>
>> + for (i = 0; i < n_items; i++) {
>> + bus_req->skb = items[i].transfer_context;
>> + bus_req->request = HIF_WRITE;
>> + bus_req->eid = pipe_id_to_eid(pipe_id);
>> + /* Write TX data to the end of the mbox address space */
>> + address = ar_sdio->mbox_addr[bus_req->eid] +
>> + ar_sdio->mbox_size[bus_req->eid] - bus_req->skb->len;
>> + bus_req->address = address;
>> + bus_req->len = CALC_TXRX_PADDED_LEN(ar_sdio, bus_req->skb->len);
>> +
>> + spin_lock_bh(&ar_sdio->wr_async_lock);
>> + list_add_tail(&bus_req->list, &ar_sdio->wr_asyncq);
>> + spin_unlock_bh(&ar_sdio->wr_async_lock);
>> + }
>> +
>> + queue_work(ar_sdio->workqueue, &ar_sdio->wr_async_work);
>> +
>> + return 0;
>> +}
>> +
>> +static int ath10k_sdio_hif_enable_intrs(struct ath10k_sdio *ar_sdio)
>> +{
>> + struct ath10k_sdio_irq_enable_reg regs;
>> + int status;
>> + struct ath10k_sdio_irq_data *irq_data = &ar_sdio->irq_data;
>> +
>> + memset(®s, 0, sizeof(regs));
>> +
>> + /* Enable all but CPU interrupts */
>> + regs.int_status_en = SM(0x01, MBOX_INT_STATUS_ENABLE_ERROR) |
>> + SM(0x01, MBOX_INT_STATUS_ENABLE_CPU) |
>> + SM(0x01, MBOX_INT_STATUS_ENABLE_COUNTER);
>> +
>> + /* NOTE: There are some cases where HIF can do detection of
>> + * pending mbox messages which is disabled now.
>> + */
>> + regs.int_status_en |= SM(0x01, MBOX_INT_STATUS_ENABLE_MBOX_DATA);
>> +
>> + /* Set up the CPU Interrupt status Register */
>> + regs.cpu_int_status_en = 0;
>> +
>> + /* Set up the Error Interrupt status Register */
>> + regs.err_int_status_en =
>> + SM(0x01, MBOX_ERROR_STATUS_ENABLE_RX_UNDERFLOW) |
>> + SM(0x01, MBOX_ERROR_STATUS_ENABLE_TX_OVERFLOW);
>> +
>> + /* Enable Counter interrupt status register to get fatal errors for
>> + * debugging.
>> + */
>> + regs.cntr_int_status_en = SM(ATH10K_SDIO_TARGET_DEBUG_INTR_MASK,
>> + MBOX_COUNTER_INT_STATUS_ENABLE_BIT);
>> +
>> + status = ath10k_sdio_read_write_sync(ar_sdio->ar,
>> + MBOX_INT_STATUS_ENABLE_ADDRESS,
>> + ®s.int_status_en, sizeof(regs),
>> + HIF_WR_SYNC_BYTE_INC);
>> + if (status) {
>> + ath10k_err(ar_sdio->ar,
>> + "failed to update interrupt ctl reg err: %d\n",
>> + status);
>> + return status;
>> + }
>> +
>> + spin_lock_bh(&irq_data->lock);
>> + irq_data->irq_en_reg = regs;
>> + spin_unlock_bh(&irq_data->lock);
>> +
>> + return 0;
>> +}
>> +
>> +static int ath10k_sdio_hif_start(struct ath10k *ar)
>> +{
>> + int ret;
>> + struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
>> + u32 addr, val;
>> +
>> + addr = host_interest_item_address(HI_ITEM(hi_acs_flags));
>> +
>> + ret = ath10k_sdio_hif_diag_read32(ar, addr, &val);
>> + if (ret) {
>> + ath10k_err(ar, "Unable to read diag mem: %d\n", ret);
>> + goto out;
>> + }
>> +
>> + if (val & HI_ACS_FLAGS_SDIO_SWAP_MAILBOX_FW_ACK) {
>> + ath10k_dbg(ar, ATH10K_DBG_SDIO,
>> + "Mailbox SWAP Service is enabled\n");
>> + ar_sdio->swap_mbox = true;
>> + }
>> +
>> + /* eid 0 always uses the lower part of the extended mailbox address
>> + * space (ext_info[0].htc_ext_addr).
>> + */
>> + ar_sdio->mbox_addr[0] = ar_sdio->mbox_info.ext_info[0].htc_ext_addr;
>> + ar_sdio->mbox_size[0] = ar_sdio->mbox_info.ext_info[0].htc_ext_sz;
>> +
>> + sdio_claim_host(ar_sdio->func);
>> +
>> + /* Register the isr */
>> + ret = sdio_claim_irq(ar_sdio->func, ath10k_sdio_irq_handler);
>> + if (ret) {
>> + ath10k_err(ar, "Failed to claim sdio irq: %d\n", ret);
>> + sdio_release_host(ar_sdio->func);
>> + goto out;
>> + }
>> +
>> + sdio_release_host(ar_sdio->func);
>> +
>> + ret = ath10k_sdio_hif_enable_intrs(ar_sdio);
>> + if (ret)
>> + ath10k_err(ar, "Failed to enable sdio interrupts: %d\n", ret);
>> +
>> +out:
>> + return ret;
>> +}
>> +
>> +static bool ath10k_sdio_is_on_irq(struct ath10k *ar)
>> +{
>> + struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
>> +
>> + return !atomic_read(&ar_sdio->irq_handling);
>> +}
>
> Yikes.
>
>> +
>> +static void ath10k_sdio_irq_disable(struct ath10k *ar)
>> +{
>> + struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
>> + int ret;
>> +
>> + sdio_claim_host(ar_sdio->func);
>> +
>> + atomic_set(&ar_sdio->stopping, 1);
>> +
>> + if (atomic_read(&ar_sdio->irq_handling)) {
>> + sdio_release_host(ar_sdio->func);
>> +
>> + ret = wait_event_interruptible(ar_sdio->irq_wq,
>> + ath10k_sdio_is_on_irq(ar));
>> + if (ret)
>> + return;
>> +
>> + sdio_claim_host(ar_sdio->func);
>> + }
>
> There has to be a better way to implement this, now it feels that it's
> just reimplementing the wheel. We should have proper way to wait for
> interrupt handlers and workqueues to end etc.
>
>> + ret = sdio_release_irq(ar_sdio->func);
>> + if (ret)
>> + ath10k_err(ar, "Failed to release sdio irq: %d\n", ret);
>> +
>> + sdio_release_host(ar_sdio->func);
>> +}
>> +
>> +static int ath10k_sdio_config(struct ath10k_sdio *ar_sdio)
>> +{
>> + struct ath10k *ar = ar_sdio->ar;
>> + struct sdio_func *func = ar_sdio->func;
>> + unsigned char byte, asyncintdelay = 2;
>> + int ret;
>> +
>> + ath10k_dbg(ar, ATH10K_DBG_BOOT, "SDIO configuration\n");
>> +
>> + sdio_claim_host(func);
>> +
>> + byte = 0;
>> + ret = ath10k_sdio_func0_cmd52_rd_byte(func->card,
>> + SDIO_CCCR_DRIVE_STRENGTH,
>> + &byte);
>> +
>> + byte = (byte & (~(SDIO_DRIVE_DTSx_MASK << SDIO_DRIVE_DTSx_SHIFT))) |
>> + SDIO_DTSx_SET_TYPE_D;
>
> FIELD_PREP(). There are lots of places where that an be used.
>
>> +static void ath10k_sdio_write32(struct ath10k *ar, u32 offset, u32 value)
>> +{
>> +}
>> +
>> +static u32 ath10k_sdio_read32(struct ath10k *ar, u32 offset)
>> +{
>> + return 0;
>> +}
>
> Somekind of FIXME/TODO comments for write32() and read32()? What
> functionality are we going to miss when we don't implement these?
>
>> +static void ath10k_sdio_hif_send_complete_check(struct ath10k *ar,
>> + u8 pipe, int force)
>> +{
>> +}
>> +
>> +static u16 ath10k_sdio_hif_get_free_queue_number(struct ath10k *ar, u8 pipe)
>> +{
>> + return 0;
>> +}
>
> Similar comments here also.
>
>> +
>> +static const struct ath10k_hif_ops ath10k_sdio_hif_ops = {
>> + .tx_sg = ath10k_sdio_hif_tx_sg,
>> + .diag_read = ath10k_sdio_hif_diag_read,
>> + .diag_write = ath10k_sdio_diag_write_mem,
>> + .exchange_bmi_msg = ath10k_sdio_hif_exchange_bmi_msg,
>> + .start = ath10k_sdio_hif_start,
>> + .stop = ath10k_sdio_hif_stop,
>> + .map_service_to_pipe = ath10k_sdio_hif_map_service_to_pipe,
>> + .get_default_pipe = ath10k_sdio_hif_get_default_pipe,
>> + .send_complete_check = ath10k_sdio_hif_send_complete_check,
>> + .get_free_queue_number = ath10k_sdio_hif_get_free_queue_number,
>> + .power_up = ath10k_sdio_hif_power_up,
>> + .power_down = ath10k_sdio_hif_power_down,
>> + .read32 = ath10k_sdio_read32,
>> + .write32 = ath10k_sdio_write32,
>> +#ifdef CONFIG_PM
>> + .suspend = ath10k_sdio_hif_suspend,
>> + .resume = ath10k_sdio_hif_resume,
>> +#endif
>> + .fetch_cal_eeprom = ath10k_sdio_hif_fetch_cal_eeprom,
>> +};
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +
>> +/* Empty handlers so that mmc subsystem doesn't remove us entirely during
>> + * suspend. We instead follow cfg80211 suspend/resume handlers.
>> + */
>> +static int ath10k_sdio_pm_suspend(struct device *device)
>> +{
>> + return 0;
>> +}
>> +
>> +static int ath10k_sdio_pm_resume(struct device *device)
>> +{
>> + return 0;
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(ath10k_sdio_pm_ops, ath10k_sdio_pm_suspend,
>> + ath10k_sdio_pm_resume);
>> +
>> +#define ATH10K_SDIO_PM_OPS (&ath10k_sdio_pm_ops)
>> +
>> +#else
>> +
>> +#define ATH10K_SDIO_PM_OPS NULL
>> +
>> +#endif /* CONFIG_PM_SLEEP */
>> +
>> +static int ath10k_sdio_probe(struct sdio_func *func,
>> + const struct sdio_device_id *id)
>> +{
>> + int ret;
>> + struct ath10k_sdio *ar_sdio;
>> + struct ath10k *ar;
>> + int i;
>> + enum ath10k_hw_rev hw_rev;
>> +
>> + hw_rev = ATH10K_HW_QCA6174;
>
> This needs a comment, at least to remind if ever add something else than
> QCA6174 based SDIO design.
>
>> +
>> + ar = ath10k_core_create(sizeof(*ar_sdio), &func->dev, ATH10K_BUS_SDIO,
>> + hw_rev, &ath10k_sdio_hif_ops);
>> + if (!ar) {
>> + dev_err(&func->dev, "failed to allocate core\n");
>> + return -ENOMEM;
>> + }
>> +
>> + ath10k_dbg(ar, ATH10K_DBG_BOOT,
>> + "sdio new func %d vendor 0x%x device 0x%x block 0x%x/0x%x\n",
>> + func->num, func->vendor, func->device,
>> + func->max_blksize, func->cur_blksize);
>> +
>> + ar_sdio = ath10k_sdio_priv(ar);
>> +
>> + ar_sdio->dma_buffer = kzalloc(ATH10K_HIF_DMA_BUFFER_SIZE, GFP_KERNEL);
>> + if (!ar_sdio->dma_buffer) {
>> + ret = -ENOMEM;
>> + goto err_core_destroy;
>> + }
>> +
>> + ar_sdio->func = func;
>> + sdio_set_drvdata(func, ar_sdio);
>> +
>> + ar_sdio->is_disabled = true;
>> + ar_sdio->ar = ar;
>> +
>> + spin_lock_init(&ar_sdio->lock);
>> + spin_lock_init(&ar_sdio->wr_async_lock);
>> + spin_lock_init(&ar_sdio->irq_data.lock);
>> + mutex_init(&ar_sdio->dma_buffer_mutex);
>> +
>> + INIT_LIST_HEAD(&ar_sdio->bus_req_freeq);
>> + INIT_LIST_HEAD(&ar_sdio->wr_asyncq);
>> +
>> + INIT_WORK(&ar_sdio->wr_async_work, ath10k_sdio_write_async_work);
>> + ar_sdio->workqueue = create_singlethread_workqueue("ath10k_sdio_wq");
>> + if (!ar_sdio->workqueue)
>> + goto err;
>> +
>> + init_waitqueue_head(&ar_sdio->irq_wq);
>> +
>> + for (i = 0; i < ATH10K_SDIO_BUS_REQUEST_MAX_NUM; i++)
>> + ath10k_sdio_free_bus_req(ar_sdio, &ar_sdio->bus_req[i]);
>> +
>> + ar->dev_id = id->device;
>> + ar->id.vendor = id->vendor;
>> + ar->id.device = id->device;
>> +
>> + ath10k_sdio_set_mbox_info(ar_sdio);
>> +
>> + ret = ath10k_sdio_config(ar_sdio);
>> + if (ret) {
>> + ath10k_err(ar, "Failed to config sdio: %d\n", ret);
>> + goto err;
>> + }
>> +
>> + ret = ath10k_core_register(ar, 0/*chip_id is not applicaple for SDIO*/);
>> + if (ret) {
>> + ath10k_err(ar, "failed to register driver core: %d\n", ret);
>> + goto err;
>> + }
>
> I would assume that chip_id is applicaple also with SDIO, there has to
> be a register where to get it. Also this kind of comment style is
> preferred:
>
> /* TODO: don't know yet how to get chip_id with SDIO */
> chip_id = 0;
>
> ret = ath10k_core_register(ar, chip_id);
>
>> +
>> + return ret;
>> +
>> +err:
>> + kfree(ar_sdio->dma_buffer);
>> +err_core_destroy:
>> + ath10k_core_destroy(ar);
>> +
>> + return ret;
>> +}
>> +
>> +static void ath10k_sdio_remove(struct sdio_func *func)
>> +{
>> + struct ath10k_sdio *ar_sdio;
>> + struct ath10k *ar;
>> +
>> + ar_sdio = sdio_get_drvdata(func);
>> + ar = ar_sdio->ar;
>> +
>> + ath10k_dbg(ar, ATH10K_DBG_BOOT,
>> + "sdio removed func %d vendor 0x%x device 0x%x\n",
>> + func->num, func->vendor, func->device);
>> +
>> + (void)ath10k_sdio_hif_disable_intrs(ar_sdio);
>> + cancel_work_sync(&ar_sdio->wr_async_work);
>> + ath10k_core_unregister(ar);
>> + ath10k_core_destroy(ar);
>> +
>> + kfree(ar_sdio->dma_buffer);
>> +}
>> +
>> +static const struct sdio_device_id ath10k_sdio_devices[] = {
>> + {SDIO_DEVICE(ATH10K_MANUFACTURER_CODE,
>> + (ATH10K_MANUFACTURER_ID_AR6005_BASE | 0xA))},
>> + {},
>> +};
>
> I suspect there's a more sensible way to create the device table than
> this, just no time to check it now. Anyone know?
>
> The naming "ath10k manufacturer" is also wrong, it should be QCA or
> Qualcomm.
>
^ permalink raw reply
* Re: [PATCH 1/3] NFC: trf7970a: add device tree option for 27MHz clock
From: Mark Greer @ 2016-12-20 18:11 UTC (permalink / raw)
To: Geoff Lansberry
Cc: linux-wireless, lauro.venancio, aloisio.almeida, sameo, robh+dt,
mark.rutland, netdev, devicetree, linux-kernel, justin
In-Reply-To: <1482250592-4268-1-git-send-email-glansberry@gmail.com>
Hi Geoff.
Please put the version in your subjects when submitting anything but the
initial version of a patch (e.g., [PATCH v2 1/3]).
Which series do you want reviewed?
Mark
--
^ permalink raw reply
* Re: [PATCH 1/3] NFC: trf7970a: add device tree option for 27MHz clock
From: Jones Desougi @ 2016-12-20 17:58 UTC (permalink / raw)
To: Geoff Lansberry, linux-wireless
Cc: lauro.venancio, aloisio.almeida, sameo, robh+dt, mark.rutland,
netdev, devicetree, linux-kernel, mgreer, justin
In-Reply-To: <1482250592-4268-1-git-send-email-glansberry@gmail.com>
On 2016-12-20 17:16, Geoff Lansberry wrote:
> From: Geoff Lansberry <geoff@kuvee.com>
>
> The TRF7970A has configuration options to support hardware designs
> which use a 27.12MHz clock. This commit adds a device tree option
> 'clock-frequency' to support configuring the this chip for default
> 13.56MHz clock or the optional 27.12MHz clock.
> ---
> .../devicetree/bindings/net/nfc/trf7970a.txt | 4 ++
> drivers/nfc/trf7970a.c | 50 +++++++++++++++++-----
> 2 files changed, 43 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/nfc/trf7970a.txt b/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
> index 32b35a0..e262ac1 100644
> --- a/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
> +++ b/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
> @@ -21,6 +21,8 @@ Optional SoC Specific Properties:
> - t5t-rmb-extra-byte-quirk: Specify that the trf7970a has the erratum
> where an extra byte is returned by Read Multiple Block commands issued
> to Type 5 tags.
> +- clock-frequency: Set to specify that the input frequency to the trf7970a is 13560000Hz or 27120000Hz
> +
You're adding an empty line here that is removed in the next patch.
>
> Example (for ARM-based BeagleBone with TRF7970A on SPI1):
>
> @@ -43,6 +45,8 @@ Example (for ARM-based BeagleBone with TRF7970A on SPI1):
> irq-status-read-quirk;
> en2-rf-quirk;
> t5t-rmb-extra-byte-quirk;
> + vdd_io_1v8;
This does not belong here, and so no need to remove in the next patch.
> + clock-frequency = <27120000>;
> status = "okay";
> };
> };
> diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
> index 26c9dbb..4e051e9 100644
> --- a/drivers/nfc/trf7970a.c
> +++ b/drivers/nfc/trf7970a.c
> @@ -124,6 +124,9 @@
> NFC_PROTO_ISO15693_MASK | NFC_PROTO_NFC_DEP_MASK)
>
> #define TRF7970A_AUTOSUSPEND_DELAY 30000 /* 30 seconds */
> +#define TRF7970A_13MHZ_CLOCK_FREQUENCY 13560000
> +#define TRF7970A_27MHZ_CLOCK_FREQUENCY 27120000
> +
>
> #define TRF7970A_RX_SKB_ALLOC_SIZE 256
>
> @@ -1056,12 +1059,11 @@ static int trf7970a_init(struct trf7970a *trf)
>
> trf->chip_status_ctrl &= ~TRF7970A_CHIP_STATUS_RF_ON;
>
> - ret = trf7970a_write(trf, TRF7970A_MODULATOR_SYS_CLK_CTRL, 0);
> + ret = trf7970a_write(trf, TRF7970A_MODULATOR_SYS_CLK_CTRL,
> + trf->modulator_sys_clk_ctrl);
> if (ret)
> goto err_out;
>
> - trf->modulator_sys_clk_ctrl = 0;
> -
> ret = trf7970a_write(trf, TRF7970A_ADJUTABLE_FIFO_IRQ_LEVELS,
> TRF7970A_ADJUTABLE_FIFO_IRQ_LEVELS_WLH_96 |
> TRF7970A_ADJUTABLE_FIFO_IRQ_LEVELS_WLL_32);
> @@ -1181,27 +1183,37 @@ static int trf7970a_in_config_rf_tech(struct trf7970a *trf, int tech)
> switch (tech) {
> case NFC_DIGITAL_RF_TECH_106A:
> trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_14443A_106;
> - trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_OOK;
> + trf->modulator_sys_clk_ctrl =
> + (trf->modulator_sys_clk_ctrl & 0xF8) |
> + TRF7970A_MODULATOR_DEPTH_OOK;
> trf->guard_time = TRF7970A_GUARD_TIME_NFCA;
> break;
> case NFC_DIGITAL_RF_TECH_106B:
> trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_14443B_106;
> - trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_ASK10;
> + trf->modulator_sys_clk_ctrl =
> + (trf->modulator_sys_clk_ctrl & 0xF8) |
> + TRF7970A_MODULATOR_DEPTH_ASK10;
> trf->guard_time = TRF7970A_GUARD_TIME_NFCB;
> break;
> case NFC_DIGITAL_RF_TECH_212F:
> trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_FELICA_212;
> - trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_ASK10;
> + trf->modulator_sys_clk_ctrl =
> + (trf->modulator_sys_clk_ctrl & 0xF8) |
> + TRF7970A_MODULATOR_DEPTH_ASK10;
> trf->guard_time = TRF7970A_GUARD_TIME_NFCF;
> break;
> case NFC_DIGITAL_RF_TECH_424F:
> trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_FELICA_424;
> - trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_ASK10;
> + trf->modulator_sys_clk_ctrl =
> + (trf->modulator_sys_clk_ctrl & 0xF8) |
> + TRF7970A_MODULATOR_DEPTH_ASK10;
> trf->guard_time = TRF7970A_GUARD_TIME_NFCF;
> break;
> case NFC_DIGITAL_RF_TECH_ISO15693:
> trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_15693_SGL_1OF4_2648;
> - trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_OOK;
> + trf->modulator_sys_clk_ctrl =
> + (trf->modulator_sys_clk_ctrl & 0xF8) |
> + TRF7970A_MODULATOR_DEPTH_OOK;
> trf->guard_time = TRF7970A_GUARD_TIME_15693;
> break;
> default:
> @@ -1571,17 +1583,23 @@ static int trf7970a_tg_config_rf_tech(struct trf7970a *trf, int tech)
> trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_NFC_NFC_CE_MODE |
> TRF7970A_ISO_CTRL_NFC_CE |
> TRF7970A_ISO_CTRL_NFC_CE_14443A;
> - trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_OOK;
> + trf->modulator_sys_clk_ctrl =
> + (trf->modulator_sys_clk_ctrl & 0xF8) |
> + TRF7970A_MODULATOR_DEPTH_OOK;
> break;
> case NFC_DIGITAL_RF_TECH_212F:
> trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_NFC_NFC_CE_MODE |
> TRF7970A_ISO_CTRL_NFC_NFCF_212;
> - trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_ASK10;
> + trf->modulator_sys_clk_ctrl =
> + (trf->modulator_sys_clk_ctrl & 0xF8) |
> + TRF7970A_MODULATOR_DEPTH_ASK10;
> break;
> case NFC_DIGITAL_RF_TECH_424F:
> trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_NFC_NFC_CE_MODE |
> TRF7970A_ISO_CTRL_NFC_NFCF_424;
> - trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_ASK10;
> + trf->modulator_sys_clk_ctrl =
> + (trf->modulator_sys_clk_ctrl & 0xF8) |
> + TRF7970A_MODULATOR_DEPTH_ASK10;
> break;
> default:
> dev_dbg(trf->dev, "Unsupported rf technology: %d\n", tech);
> @@ -1987,6 +2005,7 @@ static int trf7970a_probe(struct spi_device *spi)
> struct device_node *np = spi->dev.of_node;
> struct trf7970a *trf;
> int uvolts, autosuspend_delay, ret;
> + u32 clk_freq = 13560000;
Use TRF7970A_13MHZ_CLOCK_FREQUENCY here?
>
> if (!np) {
> dev_err(&spi->dev, "No Device Tree entry\n");
> @@ -2043,6 +2062,15 @@ static int trf7970a_probe(struct spi_device *spi)
> return ret;
> }
>
> + of_property_read_u32(np, "clock-frequency", &clk_freq);
> + if ((clk_freq != TRF7970A_27MHZ_CLOCK_FREQUENCY) ||
> + (clk_freq != TRF7970A_27MHZ_CLOCK_FREQUENCY)) {
Two comparisons with 27MHz, missing 13MHz.
> + dev_err(trf->dev,
> + "clock-frequency (%u Hz) unsupported\n",
> + clk_freq);
> + return -EINVAL;
> + }
> +
> if (of_property_read_bool(np, "en2-rf-quirk"))
> trf->quirks |= TRF7970A_QUIRK_EN2_MUST_STAY_LOW;
>
>
^ permalink raw reply
* Re: wl1251 & mac address & calibration data
From: Tony Lindgren @ 2016-12-20 17:21 UTC (permalink / raw)
To: Kalle Valo
Cc: Arend Van Spriel, Pali Rohár, Daniel Wagner,
Luis R. Rodriguez, Tom Gundersen, Johannes Berg, Ming Lei,
Mimi Zohar, Bjorn Andersson, Rafał Miłecki,
Sebastian Reichel, Pavel Machek, Michal Kazior, Ivaylo Dimitrov,
Aaro Koskinen, linux-wireless, Network Development,
linux-kernel@vger.kernel.org, David Woodhouse, Takashi Iwai,
Josh Boyer, Dmitry Torokhov
In-Reply-To: <87bmw6ttim.fsf@kamboji.qca.qualcomm.com>
* Kalle Valo <kvalo@codeaurora.org> [161220 09:12]:
> Tony Lindgren <tony@atomide.com> writes:
>
> > * Kalle Valo <kvalo@codeaurora.org> [161220 03:47]:
> >> Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
> >>
> >> > On 18-12-2016 13:09, Pali Rohár wrote:
> >> >
> >> >> File wl1251-nvs.bin is provided by linux-firmware package and contains
> >> >> default data which should be overriden by model specific calibrated
> >> >> data.
> >> >
> >> > Ah. Someone thought it was a good idea to provide the "one ring to rule
> >> > them all". Nice.
> >>
> >> Yes, that was a bad idea. wl1251-nvs.bin in linux-firmware.git should be
> >> renamed to wl1251-nvs.bin.example, or something like that, as it should
> >> be only installed to a real system only if there's no real calibration
> >> data available (only for developers to use, not real users).
> >
> > Makes sense to me. Note that with the recent changes to wlcore, we can
> > now easily provide board specific calibration firmware simply by adding a
> > new compatible value. So for n900, we could have something like
> > compatible = "ti,wl1251-n900" and have it point to n900 specific calibration
> > file wl1251-nvs-n900.bin. Of course this won't help with the mac address,
> > or any of the device specific data..
> >
> > That is assuming the calibration values are the same for each similar
> > device and don't have to be generated for each device. And naturally wl1251
> > needs simlar changes done to make use of devices specific calibration files.
>
> No, these are unique per each sold device. Every N900 was calibrated at
> the factory and they all have different calibration data which is stored
> to the flash. So when N900 boots (and in _every_ boot) it has to load
> the calibration data from the flash and provide it to the wl1251 driver
> somehow.
Urgh, OK. So much for that idea then.
Thanks,
Tony
^ permalink raw reply
* Re: iwl3945 randomly crashes
From: Dan Williams @ 2016-12-20 17:19 UTC (permalink / raw)
To: Łukasz Walewski; +Cc: linux-wireless, ilw, sgruszka
In-Reply-To: <20161220181145.f92ef40322426f7911300885@icm.edu.pl>
On Tue, 2016-12-20 at 18:11 +0100, Łukasz Walewski wrote:
> On Tue, 20 Dec 2016 10:49:52 -0600
> Dan Williams <dcbw@redhat.com> wrote:
>
> >
> > On Tue, 2016-12-20 at 17:24 +0100, Łukasz Walewski wrote:
> > >
> > > Hi,
> > >
> > > I am using the latest Lubuntu 16.10 with fairly recent kernel
> > >
> > > ljw@hideo:~$ uname -a
> > > Linux hideo 4.8.0-30-generic #32-Ubuntu SMP Fri Dec 2 03:43:33
> > > UTC
> > > 2016 i686 i686 i686 GNU/Linux
> > >
> > > on an old Toshiba laptop (Satellite Pro U200) equipped with an
> > > Intel
> > > PRO/Wireless 3945ABG adapter
> > >
> > > ljw@hideo:~$ lspci | grep ireless
> > > 02:00.0 Network controller: Intel Corporation PRO/Wireless
> > > 3945ABG
> > > [Golan] Network Connection (rev 02)
> > >
> > > The wireless network crashes once in a while with no apparent
> > > reason
> > > with the following log entries:
> > >
> > > Dec 18 17:23:42 hideo kernel: [94900.863133] iwl3945
> > > 0000:02:00.0:
> > > loaded firmware version 15.32.2.9
> > > Dec 18 17:23:42 hideo kernel: [94900.913626] iwl3945
> > > 0000:02:00.0:
> > > BSM uCode verification failed at addr 0x00003800+0 (of 900), is
> > > 0xa5a5a5a2, s/b 0xf802020
> > > Dec 18 17:23:42 hideo kernel: [94900.913633] iwl3945
> > > 0000:02:00.0:
> > > Unable to set up bootstrap uCode: -5
> > >
> > > This problem has been reported several times on different mailing
> > > lists and community portals all over the Internet, nevertheless I
> > > was
> > > not able to locate the right fix for it.
> > >
> > > The workaround suggested elswhere (restarting the network-manager
> > > when the problem occurs) works with my setup.
> >
> > Instead of restarting NM, you can "pkill -TERM wpa_supplicant" or
> > something, which will do the same thing but just bounce the wifi
> > rather
> > than everything. Or 'rmmod iwl3945; modprobe iwl3945'.
>
> Thanks, but that is just another workaround. I am interested in
> fixing the problem, i.e. getting the driver not to crash at all,
> rather than playing around with it crashing every now and then.
Yeah, I know. Just pointing out that people often use a huge hammer,
when a much smaller hammer will do.
Dan
^ permalink raw reply
* Re: wl1251 & mac address & calibration data
From: Kalle Valo @ 2016-12-20 17:11 UTC (permalink / raw)
To: Tony Lindgren
Cc: Arend Van Spriel, Pali Rohár, Daniel Wagner,
Luis R. Rodriguez, Tom Gundersen, Johannes Berg, Ming Lei,
Mimi Zohar, Bjorn Andersson, Rafał Miłecki,
Sebastian Reichel, Pavel Machek, Michal Kazior, Ivaylo Dimitrov,
Aaro Koskinen, linux-wireless, Network Development,
linux-kernel@vger.kernel.org, David Woodhouse, Takashi Iwai,
Josh Boyer, Dmitry Torokhov
In-Reply-To: <20161220165658.GI4920@atomide.com>
Tony Lindgren <tony@atomide.com> writes:
> * Kalle Valo <kvalo@codeaurora.org> [161220 03:47]:
>> Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
>>=20
>> > On 18-12-2016 13:09, Pali Roh=C3=A1r wrote:
>> >
>> >> File wl1251-nvs.bin is provided by linux-firmware package and contain=
s=20
>> >> default data which should be overriden by model specific calibrated=20
>> >> data.
>> >
>> > Ah. Someone thought it was a good idea to provide the "one ring to rule
>> > them all". Nice.
>>=20
>> Yes, that was a bad idea. wl1251-nvs.bin in linux-firmware.git should be
>> renamed to wl1251-nvs.bin.example, or something like that, as it should
>> be only installed to a real system only if there's no real calibration
>> data available (only for developers to use, not real users).
>
> Makes sense to me. Note that with the recent changes to wlcore, we can
> now easily provide board specific calibration firmware simply by adding a
> new compatible value. So for n900, we could have something like
> compatible =3D "ti,wl1251-n900" and have it point to n900 specific calibr=
ation
> file wl1251-nvs-n900.bin. Of course this won't help with the mac address,
> or any of the device specific data..
>
> That is assuming the calibration values are the same for each similar
> device and don't have to be generated for each device. And naturally wl12=
51
> needs simlar changes done to make use of devices specific calibration fil=
es.
No, these are unique per each sold device. Every N900 was calibrated at
the factory and they all have different calibration data which is stored
to the flash. So when N900 boots (and in _every_ boot) it has to load
the calibration data from the flash and provide it to the wl1251 driver
somehow.
--=20
Kalle Valo
^ permalink raw reply
* Re: iwl3945 randomly crashes
From: Łukasz Walewski @ 2016-12-20 17:11 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-wireless, ilw, sgruszka
In-Reply-To: <1482252592.30160.8.camel@redhat.com>
On Tue, 20 Dec 2016 10:49:52 -0600
Dan Williams <dcbw@redhat.com> wrote:
> On Tue, 2016-12-20 at 17:24 +0100, Łukasz Walewski wrote:
> > Hi,
> >
> > I am using the latest Lubuntu 16.10 with fairly recent kernel
> >
> > ljw@hideo:~$ uname -a
> > Linux hideo 4.8.0-30-generic #32-Ubuntu SMP Fri Dec 2 03:43:33 UTC
> > 2016 i686 i686 i686 GNU/Linux
> >
> > on an old Toshiba laptop (Satellite Pro U200) equipped with an Intel
> > PRO/Wireless 3945ABG adapter
> >
> > ljw@hideo:~$ lspci | grep ireless
> > 02:00.0 Network controller: Intel Corporation PRO/Wireless 3945ABG
> > [Golan] Network Connection (rev 02)
> >
> > The wireless network crashes once in a while with no apparent reason
> > with the following log entries:
> >
> > Dec 18 17:23:42 hideo kernel: [94900.863133] iwl3945 0000:02:00.0:
> > loaded firmware version 15.32.2.9
> > Dec 18 17:23:42 hideo kernel: [94900.913626] iwl3945 0000:02:00.0:
> > BSM uCode verification failed at addr 0x00003800+0 (of 900), is
> > 0xa5a5a5a2, s/b 0xf802020
> > Dec 18 17:23:42 hideo kernel: [94900.913633] iwl3945 0000:02:00.0:
> > Unable to set up bootstrap uCode: -5
> >
> > This problem has been reported several times on different mailing
> > lists and community portals all over the Internet, nevertheless I was
> > not able to locate the right fix for it.
> >
> > The workaround suggested elswhere (restarting the network-manager
> > when the problem occurs) works with my setup.
>
> Instead of restarting NM, you can "pkill -TERM wpa_supplicant" or
> something, which will do the same thing but just bounce the wifi rather
> than everything. Or 'rmmod iwl3945; modprobe iwl3945'.
Thanks, but that is just another workaround. I am interested in fixing the problem, i.e. getting the driver not to crash at all, rather than playing around with it crashing every now and then.
Lukasz
^ permalink raw reply
* Re: wl1251 & mac address & calibration data
From: Pali Rohár @ 2016-12-20 17:06 UTC (permalink / raw)
To: Tony Lindgren
Cc: Kalle Valo, Arend Van Spriel, Daniel Wagner, Luis R. Rodriguez,
Tom Gundersen, Johannes Berg, Ming Lei, Mimi Zohar,
Bjorn Andersson, Rafał Miłecki, Sebastian Reichel,
Pavel Machek, Michal Kazior, Ivaylo Dimitrov, Aaro Koskinen,
linux-wireless, Network Development, linux-kernel@vger.kernel.org,
David Woodhouse, Takashi Iwai, Josh Boyer, Dmitry Torokhov
In-Reply-To: <20161220165658.GI4920@atomide.com>
[-- Attachment #1: Type: Text/Plain, Size: 2331 bytes --]
On Tuesday 20 December 2016 17:56:58 Tony Lindgren wrote:
> * Kalle Valo <kvalo@codeaurora.org> [161220 03:47]:
> > Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
> > > On 18-12-2016 13:09, Pali Rohár wrote:
> > >> File wl1251-nvs.bin is provided by linux-firmware package and
> > >> contains default data which should be overriden by model
> > >> specific calibrated data.
> > >
> > > Ah. Someone thought it was a good idea to provide the "one ring
> > > to rule them all". Nice.
> >
> > Yes, that was a bad idea. wl1251-nvs.bin in linux-firmware.git
> > should be renamed to wl1251-nvs.bin.example, or something like
> > that, as it should be only installed to a real system only if
> > there's no real calibration data available (only for developers to
> > use, not real users).
>
> Makes sense to me. Note that with the recent changes to wlcore, we
> can now easily provide board specific calibration firmware simply by
> adding a new compatible value. So for n900, we could have something
> like compatible = "ti,wl1251-n900" and have it point to n900
> specific calibration file wl1251-nvs-n900.bin. Of course this won't
> help with the mac address, or any of the device specific data..
>
> That is assuming the calibration values are the same for each similar
> device and don't have to be generated for each device. And naturally
> wl1251 needs simlar changes done to make use of devices specific
> calibration files.
>
> Regards,
>
> Tony
As wrote in another thread "wl1251 NVS calibration data format"
calibration data for wl1251 (wl1251-nvs.bin) contains also MAC address,
which kernel sends to wl1251 chip. Kernel just do not use it.
So... my idea now is:
1) extend request_firmware function family with ability to use userspace
helper first and fallback to VFS
2) teach wl1251.ko to parse MAC address from wl1251-nvs.bin and use it
(in case it is not empty or 00:00:20:07:03:09 which is in that example
linux-firmware package)
3) write Nokia n900 specific userspace helper for providing data when
kernel requests wl1251-nvs.bin. So userspace helper reads MAC address
and calibration data from CAL, place MAC address into calibration data
and send put it into kernel.
Are you OK with this idea?
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH v2] cfg80211: add set/get link loss profile
From: Dan Williams @ 2016-12-20 17:00 UTC (permalink / raw)
To: Lazar, Alexei Avshalom, Kalle Valo; +Cc: johannes, linux-wireless, wil6210
In-Reply-To: <9bdf0a3a-1283-21c5-0739-e0b48dd9ddcc@codeaurora.org>
On Mon, 2016-12-19 at 12:58 +0200, Lazar, Alexei Avshalom wrote:
> From 7e41d45179a8762a7d2e1653db35751c19c8c747 Mon Sep 17 00:00:00
> 2001
> From: Alexei Avshalom Lazar <ailizaro@codeaurora.org>
> Date: Sun, 18 Dec 2016 15:06:38 +0200
> Subject: [PATCH] cfg80211: add set/get link loss profile
>
> Introduce NL80211_CMD_SET_LINK_LOSS_PROFILE and
> NL80211_CMD_GET_LINK_LOSS_PROFILE needed by user space to configure
> the
> link loss profile.
> The link loss profile represents the priority of maintaining link up
> (connection to AP) in different link quality environments.
> This is intended for full mac80211 drivers. Typically link loss
> detection
> is offloaded to firmware, since it requires close monitoring of the
> RF.
> The firmware will disconnect when link quality drops.
> There are scenarios that require to lose connection faster once
> reduced
> link quality is detected in order to maintain better user experience.
> This can be used in AP and station mode.
> Example use case is FST (Fast Session Transfer), where we have a fast
> 11ad connection but close-range and a backup 11ac connection which is
> "always on". In such cases if we detect poor link quality in the
> active
> 11ad band we prefer to disconnect and switch to the backup 11ac
> connection
> with the good link quality. This will be reached by setting
> "aggressive"
> profile that will cause more rapid disconnect in such cases.
> Three types of behavior for link loss are defined:
> LINK_LOSS_PROFILE_RELAXED: prefer maintaining link up even in a poor
> link quality environment.
> LINK_LOSS_PROFILE_DEFAULT: The default behavior for maintaining link
> up vs link quality.
> LINK_LOSS_PROFILE_AGGRESSIVE: prefer losing link up in a poor link
> quality environment.
> New cfg80211 API also been added, set/get_link_loss_profile.
Still need to address Kalle's comments requesting more detailed
explanation of these. What do each of these actually mean? What does
"poor link quality" mean? Is it based on dBm, noise floor, SNR, etc?
Is it a windowed average or something? There's way too many details
left out here for userspace to reliably use this API, especially with
different drivers.
Also need to address Kalle's comment about what modes these are
relevant in, and what differences there are here between modes.
Dan
> Signed-off-by: Alexei Avshalom Lazar <ailizaro@codeaurora.org>
> ---
> drivers/net/wireless/ath/wil6210/cfg80211.c | 18 ++++++++
> include/net/cfg80211.h | 13 ++++++
> include/uapi/linux/nl80211.h | 32 +++++++++++++
> net/wireless/nl80211.c | 70
> +++++++++++++++++++++++++++++
> net/wireless/rdev-ops.h | 28 ++++++++++++
> net/wireless/trace.h | 39 ++++++++++++++++
> 6 files changed, 200 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/wil6210/cfg80211.c
> b/drivers/net/wireless/ath/wil6210/cfg80211.c
> index 6aa3ff4..681b792 100644
> --- a/drivers/net/wireless/ath/wil6210/cfg80211.c
> +++ b/drivers/net/wireless/ath/wil6210/cfg80211.c
> @@ -1499,6 +1499,22 @@ static int wil_cfg80211_set_power_mgmt(struct
> wiphy *wiphy,
> return rc;
> }
>
> +static int
> +wil_cfg80211_set_link_loss_profile(struct wiphy *wiphy,
> + struct wireless_dev *wdev,
> + enum nl80211_link_loss_profile
> profile,
> + const u8 *addr)
> +{
> + return -ENOTSUPP;
> +}
> +
> +static enum nl80211_link_loss_profile
> +wil_cfg80211_get_link_loss_profile(struct wiphy *wiphy,
> + struct wireless_dev *wdev, const
> u8 *addr)
> +{
> + return -ENOTSUPP;
> +}
> +
> static struct cfg80211_ops wil_cfg80211_ops = {
> .add_virtual_intf = wil_cfg80211_add_iface,
> .del_virtual_intf = wil_cfg80211_del_iface,
> @@ -1528,6 +1544,8 @@ static struct cfg80211_ops wil_cfg80211_ops = {
> .start_p2p_device = wil_cfg80211_start_p2p_device,
> .stop_p2p_device = wil_cfg80211_stop_p2p_device,
> .set_power_mgmt = wil_cfg80211_set_power_mgmt,
> + .set_link_loss_profile = wil_cfg80211_set_link_loss_profile,
> + .get_link_loss_profile = wil_cfg80211_get_link_loss_profile,
> };
>
> static void wil_wiphy_init(struct wiphy *wiphy)
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index ca2ac1c..7c2e420 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -2764,6 +2764,10 @@ struct cfg80211_nan_func {
> * All other parameters must be ignored.
> *
> * @set_multicast_to_unicast: configure multicast to unicast
> conversion for BSS
> + *
> + * @set_link_loss_profile: Set link loss profile for specific
> connection.
> + * @get_link_loss_profile: Get the current link loss profile of
> specific
> + * connection.
> */
> struct cfg80211_ops {
> int (*suspend)(struct wiphy *wiphy, struct
> cfg80211_wowlan *wow);
> @@ -3048,6 +3052,15 @@ struct cfg80211_ops {
> int (*set_multicast_to_unicast)(struct wiphy *wiphy,
> struct net_device *dev,
> const bool enabled);
> +
> + int (*set_link_loss_profile)(struct wiphy *wiphy,
> + struct wireless_dev *wdev,
> + enum
> nl80211_link_loss_profile profile,
> + const u8 *addr);
> + enum nl80211_link_loss_profile (*get_link_loss_profil
> e)(
> + struct wiphy *wiphy,
> + struct wireless_dev *wdev,
> + const u8 *addr);
> };
>
> /*
> diff --git a/include/uapi/linux/nl80211.h
> b/include/uapi/linux/nl80211.h
> index d74e10b..7723294 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -894,6 +894,11 @@
> * does not result in a change for the current association.
> Currently,
> * only the %NL80211_ATTR_IE data is used and updated with
> this command.
> *
> + * @NL80211_CMD_SET_LINK_LOSS_PROFILE: Set link loss profile
> (behavior) for
> + * specific connection.
> + * @NL80211_CMD_GET_LINK_LOSS_PROFILE: Get current link loss profile
> of specific
> + * connection.
> + *
> * @NL80211_CMD_MAX: highest used command number
> * @__NL80211_CMD_AFTER_LAST: internal use
> */
> @@ -1093,6 +1098,9 @@ enum nl80211_commands {
>
> NL80211_CMD_UPDATE_CONNECT_PARAMS,
>
> + NL80211_CMD_SET_LINK_LOSS_PROFILE,
> + NL80211_CMD_GET_LINK_LOSS_PROFILE,
> +
> /* add new commands above here */
>
> /* used to define NL80211_CMD_MAX below */
> @@ -1980,6 +1988,9 @@ enum nl80211_commands {
> * @NL80211_ATTR_BSSID: The BSSID of the AP. Note that
> %NL80211_ATTR_MAC is also
> * used in various commands/events for specifying the BSSID.
> *
> + * @NL80211_ATTR_LINK_LOSS_PROFILE: attribute that indicate the link
> loss
> + * behavior using &enum nl80211_link_loss_profile values.
> + *
> * @NUM_NL80211_ATTR: total number of nl80211_attrs available
> * @NL80211_ATTR_MAX: highest attribute number currently defined
> * @__NL80211_ATTR_AFTER_LAST: internal use
> @@ -2386,6 +2397,8 @@ enum nl80211_attrs {
>
> NL80211_ATTR_BSSID,
>
> + NL80211_ATTR_LINK_LOSS_PROFILE,
> +
> /* add attributes here, update the policy in nl80211.c */
>
> __NL80211_ATTR_AFTER_LAST,
> @@ -5188,4 +5201,23 @@ enum nl80211_nan_match_attributes {
> NL80211_NAN_MATCH_ATTR_MAX = NUM_NL80211_NAN_MATCH_ATTR - 1
> };
>
> +/**
> + * enum nl80211_link_loss_profile.
> + *
> + * Used by set_link_loss_profile() and get_link_loss_profile()
> + * to set/get link loss behavior.
> + *
> + * @NL80211_LINK_LOSS_PROFILE_RELAXED: prefer maintaining link
> + * up even in poor link quality environment
> + * @NL80211_LINK_LOSS_PROFILE_DEFAULT: The default behavior for
> + * maintaining link up vs link quality.
> + * @NL80211_LINK_LOSS_PROFILE_AGGRESSIVE: prefer losing link
> + * up in poor link quality environment
> + */
> +enum nl80211_link_loss_profile {
> + NL80211_LINK_LOSS_PROFILE_RELAXED,
> + NL80211_LINK_LOSS_PROFILE_DEFAULT,
> + NL80211_LINK_LOSS_PROFILE_AGGRESSIVE
> +};
> +
> #endif /* __LINUX_NL80211_H */
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 2369265..e160630 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -405,6 +405,7 @@ static const struct nla_policy
> nl80211_policy[NUM_NL80211_ATTR] = {
> [NL80211_ATTR_FILS_NONCES] = { .len = 2 * FILS_NONCE_LEN },
> [NL80211_ATTR_MULTICAST_TO_UNICAST_ENABLED] = { .type =
> NLA_FLAG, },
> [NL80211_ATTR_BSSID] = { .len = ETH_ALEN },
> + [NL80211_ATTR_LINK_LOSS_PROFILE] = { .type = NLA_U8 },
> };
>
> /* policy for the key attributes */
> @@ -11817,6 +11818,60 @@ static int
> nl80211_set_multicast_to_unicast(struct sk_buff *skb,
> return rdev_set_multicast_to_unicast(rdev, dev, enabled);
> }
>
> +static int nl80211_set_link_loss_profile(struct sk_buff *skb,
> + struct genl_info *info)
> +{
> + struct cfg80211_registered_device *rdev = info->user_ptr[0];
> + struct wireless_dev *wdev = info->user_ptr[1];
> + enum nl80211_link_loss_profile profile;
> + const u8 *addr;
> + int ret;
> +
> + if (!info->attrs[NL80211_ATTR_LINK_LOSS_PROFILE] ||
> + !info->attrs[NL80211_ATTR_MAC])
> + return -EINVAL;
> +
> + profile = nla_get_u8(info-
> >attrs[NL80211_ATTR_LINK_LOSS_PROFILE]);
> + addr = nla_data(info->attrs[NL80211_ATTR_MAC]);
> +
> + if (profile != NL80211_LINK_LOSS_PROFILE_RELAXED &&
> + profile != NL80211_LINK_LOSS_PROFILE_DEFAULT &&
> + profile != NL80211_LINK_LOSS_PROFILE_AGGRESSIVE)
> + return -EINVAL;
> +
> + if (!rdev->ops->set_link_loss_profile)
> + return -EOPNOTSUPP;
> +
> + wdev_lock(wdev);
> + ret = rdev_set_link_loss_profile(rdev, wdev, profile, addr);
> + wdev_unlock(wdev);
> +
> + return ret;
> +}
> +
> +static int nl80211_get_link_loss_profile(struct sk_buff *skb,
> + struct genl_info *info)
> +{
> + struct cfg80211_registered_device *rdev = info->user_ptr[0];
> + struct wireless_dev *wdev = info->user_ptr[1];
> + const u8 *addr;
> + enum nl80211_link_loss_profile profile;
> +
> + if (!info->attrs[NL80211_ATTR_MAC])
> + return -EINVAL;
> +
> + addr = nla_data(info->attrs[NL80211_ATTR_MAC]);
> +
> + if (!rdev->ops->get_link_loss_profile)
> + return -EOPNOTSUPP;
> +
> + wdev_lock(wdev);
> + profile = rdev_get_link_loss_profile(rdev, wdev, addr);
> + wdev_unlock(wdev);
> +
> + return profile;
> +}
> +
> #define NL80211_FLAG_NEED_WIPHY 0x01
> #define NL80211_FLAG_NEED_NETDEV 0x02
> #define NL80211_FLAG_NEED_RTNL 0x04
> @@ -12692,6 +12747,21 @@ static const struct genl_ops nl80211_ops[] =
> {
> .internal_flags = NL80211_FLAG_NEED_NETDEV |
> NL80211_FLAG_NEED_RTNL,
> },
> + {
> + .cmd = NL80211_CMD_SET_LINK_LOSS_PROFILE,
> + .doit = nl80211_set_link_loss_profile,
> + .policy = nl80211_policy,
> + .flags = GENL_ADMIN_PERM,
> + .internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> + NL80211_FLAG_NEED_RTNL,
> + },
> + {
> + .cmd = NL80211_CMD_GET_LINK_LOSS_PROFILE,
> + .doit = nl80211_get_link_loss_profile,
> + .policy = nl80211_policy,
> + .internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> + NL80211_FLAG_NEED_RTNL,
> + },
> };
>
> static struct genl_family nl80211_fam __ro_after_init = {
> diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
> index 2f42507..caf0127 100644
> --- a/net/wireless/rdev-ops.h
> +++ b/net/wireless/rdev-ops.h
> @@ -1153,4 +1153,32 @@ rdev_set_coalesce(struct
> cfg80211_registered_device *rdev,
> trace_rdev_return_int(&rdev->wiphy, ret);
> return ret;
> }
> +
> +static inline int
> +rdev_set_link_loss_profile(struct cfg80211_registered_device *rdev,
> + struct wireless_dev *wdev,
> + enum nl80211_link_loss_profile profile,
> + const u8 *addr)
> +{
> + int ret;
> +
> + trace_rdev_set_link_loss_profile(&rdev->wiphy, wdev,
> profile, addr);
> + ret = rdev->ops->set_link_loss_profile(&rdev->wiphy, wdev,
> profile,
> + addr);
> + trace_rdev_return_int(&rdev->wiphy, ret);
> + return ret;
> +}
> +
> +static inline enum nl80211_link_loss_profile
> +rdev_get_link_loss_profile(struct cfg80211_registered_device *rdev,
> + struct wireless_dev *wdev,
> + const u8 *addr)
> +{
> + enum nl80211_link_loss_profile profile;
> +
> + trace_rdev_get_link_loss_profile(&rdev->wiphy, wdev, addr);
> + profile = rdev->ops->get_link_loss_profile(&rdev->wiphy,
> wdev, addr);
> + trace_rdev_return_int(&rdev->wiphy, profile);
> + return profile;
> +}
> #endif /* __CFG80211_RDEV_OPS */
> diff --git a/net/wireless/trace.h b/net/wireless/trace.h
> index ea1b47e..3af813b 100644
> --- a/net/wireless/trace.h
> +++ b/net/wireless/trace.h
> @@ -2230,6 +2230,45 @@ TRACE_EVENT(rdev_tdls_cancel_channel_switch,
> WIPHY_PR_ARG, NETDEV_PR_ARG, MAC_PR_ARG(addr))
> );
>
> +TRACE_EVENT(rdev_set_link_loss_profile,
> + TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev,
> + enum nl80211_link_loss_profile profile, const u8
> *addr),
> + TP_ARGS(wiphy, wdev, profile, addr),
> + TP_STRUCT__entry(
> + WIPHY_ENTRY
> + WDEV_ENTRY
> + __field(enum nl80211_link_loss_profile, profile)
> + MAC_ENTRY(addr)
> + ),
> + TP_fast_assign(
> + WIPHY_ASSIGN;
> + WDEV_ASSIGN;
> + __entry->profile = profile;
> + MAC_ASSIGN(addr, addr);
> + ),
> + TP_printk(WIPHY_PR_FMT ", " WDEV_PR_FMT ", " MAC_PR_FMT ",
> PROFILE %d",
> + WIPHY_PR_ARG, WDEV_PR_ARG, MAC_PR_ARG(addr),
> + __entry->profile)
> +);
> +
> +TRACE_EVENT(rdev_get_link_loss_profile,
> + TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev,
> + const u8 *addr),
> + TP_ARGS(wiphy, wdev, addr),
> + TP_STRUCT__entry(
> + WIPHY_ENTRY
> + WDEV_ENTRY
> + MAC_ENTRY(addr)
> + ),
> + TP_fast_assign(
> + WIPHY_ASSIGN;
> + WDEV_ASSIGN;
> + MAC_ASSIGN(addr, addr);
> + ),
> + TP_printk(WIPHY_PR_FMT ", " WDEV_PR_FMT ", " MAC_PR_FMT,
> WIPHY_PR_ARG,
> + WDEV_PR_ARG, MAC_PR_ARG(addr))
> +);
> +
> /*************************************************************
> * cfg80211 exported functions traces
> *
> *************************************************************/
^ permalink raw reply
* Re: wl1251 & mac address & calibration data
From: Tony Lindgren @ 2016-12-20 16:56 UTC (permalink / raw)
To: Kalle Valo
Cc: Arend Van Spriel, Pali Rohár, Daniel Wagner,
Luis R. Rodriguez, Tom Gundersen, Johannes Berg, Ming Lei,
Mimi Zohar, Bjorn Andersson, Rafał Miłecki,
Sebastian Reichel, Pavel Machek, Michal Kazior, Ivaylo Dimitrov,
Aaro Koskinen, linux-wireless, Network Development,
linux-kernel@vger.kernel.org, David Woodhouse, Takashi Iwai,
Josh Boyer, Dmitry Torokhov
In-Reply-To: <87shpiu8j8.fsf@kamboji.qca.qualcomm.com>
* Kalle Valo <kvalo@codeaurora.org> [161220 03:47]:
> Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
>
> > On 18-12-2016 13:09, Pali Rohár wrote:
> >
> >> File wl1251-nvs.bin is provided by linux-firmware package and contains
> >> default data which should be overriden by model specific calibrated
> >> data.
> >
> > Ah. Someone thought it was a good idea to provide the "one ring to rule
> > them all". Nice.
>
> Yes, that was a bad idea. wl1251-nvs.bin in linux-firmware.git should be
> renamed to wl1251-nvs.bin.example, or something like that, as it should
> be only installed to a real system only if there's no real calibration
> data available (only for developers to use, not real users).
Makes sense to me. Note that with the recent changes to wlcore, we can
now easily provide board specific calibration firmware simply by adding a
new compatible value. So for n900, we could have something like
compatible = "ti,wl1251-n900" and have it point to n900 specific calibration
file wl1251-nvs-n900.bin. Of course this won't help with the mac address,
or any of the device specific data..
That is assuming the calibration values are the same for each similar
device and don't have to be generated for each device. And naturally wl1251
needs simlar changes done to make use of devices specific calibration files.
Regards,
Tony
^ permalink raw reply
* Re: iwl3945 randomly crashes
From: Dan Williams @ 2016-12-20 16:49 UTC (permalink / raw)
To: Łukasz Walewski, linux-wireless; +Cc: ilw, sgruszka
In-Reply-To: <20161220172429.300dc743cad7b840857142a1@icm.edu.pl>
On Tue, 2016-12-20 at 17:24 +0100, Łukasz Walewski wrote:
> Hi,
>
> I am using the latest Lubuntu 16.10 with fairly recent kernel
>
> ljw@hideo:~$ uname -a
> Linux hideo 4.8.0-30-generic #32-Ubuntu SMP Fri Dec 2 03:43:33 UTC
> 2016 i686 i686 i686 GNU/Linux
>
> on an old Toshiba laptop (Satellite Pro U200) equipped with an Intel
> PRO/Wireless 3945ABG adapter
>
> ljw@hideo:~$ lspci | grep ireless
> 02:00.0 Network controller: Intel Corporation PRO/Wireless 3945ABG
> [Golan] Network Connection (rev 02)
>
> The wireless network crashes once in a while with no apparent reason
> with the following log entries:
>
> Dec 18 17:23:42 hideo kernel: [94900.863133] iwl3945 0000:02:00.0:
> loaded firmware version 15.32.2.9
> Dec 18 17:23:42 hideo kernel: [94900.913626] iwl3945 0000:02:00.0:
> BSM uCode verification failed at addr 0x00003800+0 (of 900), is
> 0xa5a5a5a2, s/b 0xf802020
> Dec 18 17:23:42 hideo kernel: [94900.913633] iwl3945 0000:02:00.0:
> Unable to set up bootstrap uCode: -5
>
> This problem has been reported several times on different mailing
> lists and community portals all over the Internet, nevertheless I was
> not able to locate the right fix for it.
>
> The workaround suggested elswhere (restarting the network-manager
> when the problem occurs) works with my setup.
Instead of restarting NM, you can "pkill -TERM wpa_supplicant" or
something, which will do the same thing but just bounce the wifi rather
than everything. Or 'rmmod iwl3945; modprobe iwl3945'.
Dan
> Could you please suggest the solution to the problem? Alternatively,
> where should I file the bug to get it fixed?
>
> Thank you very much in advance,
> Lukasz Walewski
>
^ permalink raw reply
* iwl3945 randomly crashes
From: Łukasz Walewski @ 2016-12-20 16:24 UTC (permalink / raw)
To: linux-wireless; +Cc: ilw, sgruszka
Hi,
I am using the latest Lubuntu 16.10 with fairly recent kernel
ljw@hideo:~$ uname -a
Linux hideo 4.8.0-30-generic #32-Ubuntu SMP Fri Dec 2 03:43:33 UTC 2016 i686 i686 i686 GNU/Linux
on an old Toshiba laptop (Satellite Pro U200) equipped with an Intel PRO/Wireless 3945ABG adapter
ljw@hideo:~$ lspci | grep ireless
02:00.0 Network controller: Intel Corporation PRO/Wireless 3945ABG [Golan] Network Connection (rev 02)
The wireless network crashes once in a while with no apparent reason with the following log entries:
Dec 18 17:23:42 hideo kernel: [94900.863133] iwl3945 0000:02:00.0: loaded firmware version 15.32.2.9
Dec 18 17:23:42 hideo kernel: [94900.913626] iwl3945 0000:02:00.0: BSM uCode verification failed at addr 0x00003800+0 (of 900), is 0xa5a5a5a2, s/b 0xf802020
Dec 18 17:23:42 hideo kernel: [94900.913633] iwl3945 0000:02:00.0: Unable to set up bootstrap uCode: -5
This problem has been reported several times on different mailing lists and community portals all over the Internet, nevertheless I was not able to locate the right fix for it.
The workaround suggested elswhere (restarting the network-manager when the problem occurs) works with my setup.
Could you please suggest the solution to the problem? Alternatively, where should I file the bug to get it fixed?
Thank you very much in advance,
Lukasz Walewski
--
Łukasz Walewski <ljw@icm.edu.pl>
^ permalink raw reply
* [PATCH 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel
From: Geoff Lansberry @ 2016-12-20 16:16 UTC (permalink / raw)
To: linux-wireless
Cc: lauro.venancio, aloisio.almeida, sameo, robh+dt, mark.rutland,
netdev, devicetree, linux-kernel, mgreer, justin, Jaret Cantu,
Geoff Lansberry
In-Reply-To: <1482250592-4268-1-git-send-email-glansberry@gmail.com>
From: Jaret Cantu <jaret.cantu@timesys.com>
Repeated polling attempts cause a NULL dereference error to occur.
This is because the state of the trf7970a is currently reading but
another request has been made to send a command before it has finished.
The solution is to properly kill the waiting reading (workqueue)
before failing on the send.
---
drivers/nfc/trf7970a.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
index 8a88195..5916737 100644
--- a/drivers/nfc/trf7970a.c
+++ b/drivers/nfc/trf7970a.c
@@ -1496,6 +1496,10 @@ static int trf7970a_send_cmd(struct nfc_digital_dev *ddev,
(trf->state != TRF7970A_ST_IDLE_RX_BLOCKED)) {
dev_err(trf->dev, "%s - Bogus state: %d\n", __func__,
trf->state);
+ if (trf->state == TRF7970A_ST_WAIT_FOR_RX_DATA ||
+ trf->state == TRF7970A_ST_WAIT_FOR_RX_DATA_CONT)
+ trf->ignore_timeout =
+ !cancel_delayed_work(&trf->timeout_work);
ret = -EIO;
goto out_err;
}
--
Signed-off-by: Geoff Lansberry <geoff@kuvee.com>
^ permalink raw reply related
* [PATCH 2/3] NFC: trf7970a: Add device tree option of 1.8 Volt IO voltage
From: Geoff Lansberry @ 2016-12-20 16:16 UTC (permalink / raw)
To: linux-wireless
Cc: lauro.venancio, aloisio.almeida, sameo, robh+dt, mark.rutland,
netdev, devicetree, linux-kernel, mgreer, justin, Geoff Lansberry
In-Reply-To: <1482250592-4268-1-git-send-email-glansberry@gmail.com>
From: Geoff Lansberry <geoff@kuvee.com>
The TRF7970A has configuration options for supporting hardware designs
with 1.8 Volt or 3.3 Volt IO. This commit adds a device tree option,
using a fixed regulator binding, for setting the io voltage to match
the hardware configuration. If no option is supplied it defaults to
3.3 volt configuration.
---
.../devicetree/bindings/net/nfc/trf7970a.txt | 4 ++--
drivers/nfc/trf7970a.c | 28 +++++++++++++++++++++-
2 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/nfc/trf7970a.txt b/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
index e262ac1..b5777d8 100644
--- a/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
+++ b/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
@@ -21,9 +21,9 @@ Optional SoC Specific Properties:
- t5t-rmb-extra-byte-quirk: Specify that the trf7970a has the erratum
where an extra byte is returned by Read Multiple Block commands issued
to Type 5 tags.
+- vdd-io-supply: Regulator specifying voltage for vdd-io
- clock-frequency: Set to specify that the input frequency to the trf7970a is 13560000Hz or 27120000Hz
-
Example (for ARM-based BeagleBone with TRF7970A on SPI1):
&spi1 {
@@ -41,11 +41,11 @@ Example (for ARM-based BeagleBone with TRF7970A on SPI1):
<&gpio2 5 GPIO_ACTIVE_LOW>;
vin-supply = <&ldo3_reg>;
vin-voltage-override = <5000000>;
+ vdd-io-supply = <&ldo2_reg>;
autosuspend-delay = <30000>;
irq-status-read-quirk;
en2-rf-quirk;
t5t-rmb-extra-byte-quirk;
- vdd_io_1v8;
clock-frequency = <27120000>;
status = "okay";
};
diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
index 4e051e9..8a88195 100644
--- a/drivers/nfc/trf7970a.c
+++ b/drivers/nfc/trf7970a.c
@@ -444,6 +444,7 @@ struct trf7970a {
u8 iso_ctrl_tech;
u8 modulator_sys_clk_ctrl;
u8 special_fcn_reg1;
+ u8 io_ctrl;
unsigned int guard_time;
int technology;
int framing;
@@ -1051,6 +1052,11 @@ static int trf7970a_init(struct trf7970a *trf)
if (ret)
goto err_out;
+ ret = trf7970a_write(trf, TRF7970A_REG_IO_CTRL,
+ trf->io_ctrl | TRF7970A_REG_IO_CTRL_VRS(0x1));
+ if (ret)
+ goto err_out;
+
ret = trf7970a_write(trf, TRF7970A_NFC_TARGET_LEVEL, 0);
if (ret)
goto err_out;
@@ -1767,7 +1773,7 @@ static int _trf7970a_tg_listen(struct nfc_digital_dev *ddev, u16 timeout,
goto out_err;
ret = trf7970a_write(trf, TRF7970A_REG_IO_CTRL,
- TRF7970A_REG_IO_CTRL_VRS(0x1));
+ trf->io_ctrl | TRF7970A_REG_IO_CTRL_VRS(0x1));
if (ret)
goto out_err;
@@ -2062,6 +2068,7 @@ static int trf7970a_probe(struct spi_device *spi)
return ret;
}
+
of_property_read_u32(np, "clock-frequency", &clk_freq);
if ((clk_freq != TRF7970A_27MHZ_CLOCK_FREQUENCY) ||
(clk_freq != TRF7970A_27MHZ_CLOCK_FREQUENCY)) {
@@ -2105,6 +2112,25 @@ static int trf7970a_probe(struct spi_device *spi)
if (uvolts > 4000000)
trf->chip_status_ctrl = TRF7970A_CHIP_STATUS_VRS5_3;
+ trf->regulator = devm_regulator_get(&spi->dev, "vdd-io");
+ if (IS_ERR(trf->regulator)) {
+ ret = PTR_ERR(trf->regulator);
+ dev_err(trf->dev, "Can't get VDD_IO regulator: %d\n", ret);
+ goto err_destroy_lock;
+ }
+
+ ret = regulator_enable(trf->regulator);
+ if (ret) {
+ dev_err(trf->dev, "Can't enable VDD_IO: %d\n", ret);
+ goto err_destroy_lock;
+ }
+
+
+ if (regulator_get_voltage(trf->regulator) == 1800000) {
+ trf->io_ctrl = TRF7970A_REG_IO_CTRL_IO_LOW;
+ dev_dbg(trf->dev, "trf7970a config vdd_io to 1.8V\n");
+ }
+
trf->ddev = nfc_digital_allocate_device(&trf7970a_nfc_ops,
TRF7970A_SUPPORTED_PROTOCOLS,
NFC_DIGITAL_DRV_CAPS_IN_CRC |
--
Signed-off-by: Geoff Lansberry <geoff@kuvee.com>
^ permalink raw reply related
* [PATCH 1/3] NFC: trf7970a: add device tree option for 27MHz clock
From: Geoff Lansberry @ 2016-12-20 16:16 UTC (permalink / raw)
To: linux-wireless
Cc: lauro.venancio, aloisio.almeida, sameo, robh+dt, mark.rutland,
netdev, devicetree, linux-kernel, mgreer, justin, Geoff Lansberry
From: Geoff Lansberry <geoff@kuvee.com>
The TRF7970A has configuration options to support hardware designs
which use a 27.12MHz clock. This commit adds a device tree option
'clock-frequency' to support configuring the this chip for default
13.56MHz clock or the optional 27.12MHz clock.
---
.../devicetree/bindings/net/nfc/trf7970a.txt | 4 ++
drivers/nfc/trf7970a.c | 50 +++++++++++++++++-----
2 files changed, 43 insertions(+), 11 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/nfc/trf7970a.txt b/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
index 32b35a0..e262ac1 100644
--- a/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
+++ b/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
@@ -21,6 +21,8 @@ Optional SoC Specific Properties:
- t5t-rmb-extra-byte-quirk: Specify that the trf7970a has the erratum
where an extra byte is returned by Read Multiple Block commands issued
to Type 5 tags.
+- clock-frequency: Set to specify that the input frequency to the trf7970a is 13560000Hz or 27120000Hz
+
Example (for ARM-based BeagleBone with TRF7970A on SPI1):
@@ -43,6 +45,8 @@ Example (for ARM-based BeagleBone with TRF7970A on SPI1):
irq-status-read-quirk;
en2-rf-quirk;
t5t-rmb-extra-byte-quirk;
+ vdd_io_1v8;
+ clock-frequency = <27120000>;
status = "okay";
};
};
diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
index 26c9dbb..4e051e9 100644
--- a/drivers/nfc/trf7970a.c
+++ b/drivers/nfc/trf7970a.c
@@ -124,6 +124,9 @@
NFC_PROTO_ISO15693_MASK | NFC_PROTO_NFC_DEP_MASK)
#define TRF7970A_AUTOSUSPEND_DELAY 30000 /* 30 seconds */
+#define TRF7970A_13MHZ_CLOCK_FREQUENCY 13560000
+#define TRF7970A_27MHZ_CLOCK_FREQUENCY 27120000
+
#define TRF7970A_RX_SKB_ALLOC_SIZE 256
@@ -1056,12 +1059,11 @@ static int trf7970a_init(struct trf7970a *trf)
trf->chip_status_ctrl &= ~TRF7970A_CHIP_STATUS_RF_ON;
- ret = trf7970a_write(trf, TRF7970A_MODULATOR_SYS_CLK_CTRL, 0);
+ ret = trf7970a_write(trf, TRF7970A_MODULATOR_SYS_CLK_CTRL,
+ trf->modulator_sys_clk_ctrl);
if (ret)
goto err_out;
- trf->modulator_sys_clk_ctrl = 0;
-
ret = trf7970a_write(trf, TRF7970A_ADJUTABLE_FIFO_IRQ_LEVELS,
TRF7970A_ADJUTABLE_FIFO_IRQ_LEVELS_WLH_96 |
TRF7970A_ADJUTABLE_FIFO_IRQ_LEVELS_WLL_32);
@@ -1181,27 +1183,37 @@ static int trf7970a_in_config_rf_tech(struct trf7970a *trf, int tech)
switch (tech) {
case NFC_DIGITAL_RF_TECH_106A:
trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_14443A_106;
- trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_OOK;
+ trf->modulator_sys_clk_ctrl =
+ (trf->modulator_sys_clk_ctrl & 0xF8) |
+ TRF7970A_MODULATOR_DEPTH_OOK;
trf->guard_time = TRF7970A_GUARD_TIME_NFCA;
break;
case NFC_DIGITAL_RF_TECH_106B:
trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_14443B_106;
- trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_ASK10;
+ trf->modulator_sys_clk_ctrl =
+ (trf->modulator_sys_clk_ctrl & 0xF8) |
+ TRF7970A_MODULATOR_DEPTH_ASK10;
trf->guard_time = TRF7970A_GUARD_TIME_NFCB;
break;
case NFC_DIGITAL_RF_TECH_212F:
trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_FELICA_212;
- trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_ASK10;
+ trf->modulator_sys_clk_ctrl =
+ (trf->modulator_sys_clk_ctrl & 0xF8) |
+ TRF7970A_MODULATOR_DEPTH_ASK10;
trf->guard_time = TRF7970A_GUARD_TIME_NFCF;
break;
case NFC_DIGITAL_RF_TECH_424F:
trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_FELICA_424;
- trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_ASK10;
+ trf->modulator_sys_clk_ctrl =
+ (trf->modulator_sys_clk_ctrl & 0xF8) |
+ TRF7970A_MODULATOR_DEPTH_ASK10;
trf->guard_time = TRF7970A_GUARD_TIME_NFCF;
break;
case NFC_DIGITAL_RF_TECH_ISO15693:
trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_15693_SGL_1OF4_2648;
- trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_OOK;
+ trf->modulator_sys_clk_ctrl =
+ (trf->modulator_sys_clk_ctrl & 0xF8) |
+ TRF7970A_MODULATOR_DEPTH_OOK;
trf->guard_time = TRF7970A_GUARD_TIME_15693;
break;
default:
@@ -1571,17 +1583,23 @@ static int trf7970a_tg_config_rf_tech(struct trf7970a *trf, int tech)
trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_NFC_NFC_CE_MODE |
TRF7970A_ISO_CTRL_NFC_CE |
TRF7970A_ISO_CTRL_NFC_CE_14443A;
- trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_OOK;
+ trf->modulator_sys_clk_ctrl =
+ (trf->modulator_sys_clk_ctrl & 0xF8) |
+ TRF7970A_MODULATOR_DEPTH_OOK;
break;
case NFC_DIGITAL_RF_TECH_212F:
trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_NFC_NFC_CE_MODE |
TRF7970A_ISO_CTRL_NFC_NFCF_212;
- trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_ASK10;
+ trf->modulator_sys_clk_ctrl =
+ (trf->modulator_sys_clk_ctrl & 0xF8) |
+ TRF7970A_MODULATOR_DEPTH_ASK10;
break;
case NFC_DIGITAL_RF_TECH_424F:
trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_NFC_NFC_CE_MODE |
TRF7970A_ISO_CTRL_NFC_NFCF_424;
- trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_ASK10;
+ trf->modulator_sys_clk_ctrl =
+ (trf->modulator_sys_clk_ctrl & 0xF8) |
+ TRF7970A_MODULATOR_DEPTH_ASK10;
break;
default:
dev_dbg(trf->dev, "Unsupported rf technology: %d\n", tech);
@@ -1987,6 +2005,7 @@ static int trf7970a_probe(struct spi_device *spi)
struct device_node *np = spi->dev.of_node;
struct trf7970a *trf;
int uvolts, autosuspend_delay, ret;
+ u32 clk_freq = 13560000;
if (!np) {
dev_err(&spi->dev, "No Device Tree entry\n");
@@ -2043,6 +2062,15 @@ static int trf7970a_probe(struct spi_device *spi)
return ret;
}
+ of_property_read_u32(np, "clock-frequency", &clk_freq);
+ if ((clk_freq != TRF7970A_27MHZ_CLOCK_FREQUENCY) ||
+ (clk_freq != TRF7970A_27MHZ_CLOCK_FREQUENCY)) {
+ dev_err(trf->dev,
+ "clock-frequency (%u Hz) unsupported\n",
+ clk_freq);
+ return -EINVAL;
+ }
+
if (of_property_read_bool(np, "en2-rf-quirk"))
trf->quirks |= TRF7970A_QUIRK_EN2_MUST_STAY_LOW;
--
Signed-off-by: Geoff Lansberry <geoff@kuvee.com>
^ permalink raw reply related
* Re: [PATCH 2/3] NFC: trf7970a: Add device tree option of 1.8 Volt IO voltage
From: Geoff Lansberry @ 2016-12-20 16:13 UTC (permalink / raw)
To: Rob Herring
Cc: linux-wireless, Lauro Ramos Venancio, Aloisio Almeida Jr,
Samuel Ortiz, mark.rutland, netdev, devicetree, linux-kernel,
Mark Greer, Justin Bronder
In-Reply-To: <20161219223504.ttwayzpfvdumgouu@rob-hp-laptop>
On Mon, Dec 19, 2016 at 5:35 PM, Rob Herring <robh@kernel.org> wrote:
> On Thu, Dec 15, 2016 at 05:30:43PM -0500, Geoff Lansberry wrote:
>> From: Geoff Lansberry <geoff@kuvee.com>
>>
>> ---
>> Documentation/devicetree/bindings/net/nfc/trf7970a.txt | 2 ++
>> drivers/nfc/trf7970a.c | 13 ++++++++++++-
>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/nfc/trf7970a.txt b/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
>> index 9dda879..208f045 100644
>> --- a/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
>> +++ b/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
>> @@ -21,6 +21,7 @@ Optional SoC Specific Properties:
>> - t5t-rmb-extra-byte-quirk: Specify that the trf7970a has the erratum
>> where an extra byte is returned by Read Multiple Block commands issued
>> to Type 5 tags.
>> +- vdd_io_1v8: Set to specify that the trf7970a io voltage should be set to 1.8V
>
> Use the regulator binding and provide a fixed 1.8V supply.
>
>> - crystal_27mhz: Set to specify that the input frequency to the trf7970a is 27.12MHz
>>
>>
>> @@ -45,6 +46,7 @@ Example (for ARM-based BeagleBone with TRF7970A on SPI1):
>> irq-status-read-quirk;
>> en2-rf-quirk;
>> t5t-rmb-extra-byte-quirk;
>> + vdd_io_1v8;
>> crystal_27mhz;
>> status = "okay";
>> };
Rob - using the regulator binding is new to me, but I've given it a
shot and just sent you another set of patches for your inspection.
Please let me know if this is what you had in mind.
Geoff
^ permalink raw reply
* [PATCH 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel
From: Geoff Lansberry @ 2016-12-20 16:10 UTC (permalink / raw)
To: linux-wireless
Cc: lauro.venancio, aloisio.almeida, sameo, robh+dt, mark.rutland,
netdev, devicetree, linux-kernel, mgreer, justin, Jaret Cantu,
Geoff Lansberry
In-Reply-To: <1482250250-4192-1-git-send-email-glansberry@gmail.com>
From: Jaret Cantu <jaret.cantu@timesys.com>
Repeated polling attempts cause a NULL dereference error to occur.
This is because the state of the trf7970a is currently reading but
another request has been made to send a command before it has finished.
The solution is to properly kill the waiting reading (workqueue)
before failing on the send.
---
drivers/nfc/trf7970a.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
index 8a88195..5916737 100644
--- a/drivers/nfc/trf7970a.c
+++ b/drivers/nfc/trf7970a.c
@@ -1496,6 +1496,10 @@ static int trf7970a_send_cmd(struct nfc_digital_dev *ddev,
(trf->state != TRF7970A_ST_IDLE_RX_BLOCKED)) {
dev_err(trf->dev, "%s - Bogus state: %d\n", __func__,
trf->state);
+ if (trf->state == TRF7970A_ST_WAIT_FOR_RX_DATA ||
+ trf->state == TRF7970A_ST_WAIT_FOR_RX_DATA_CONT)
+ trf->ignore_timeout =
+ !cancel_delayed_work(&trf->timeout_work);
ret = -EIO;
goto out_err;
}
--
Signed-off-by: Geoff Lansberry <geoff@kuvee.com>
^ permalink raw reply related
* [PATCH 4/4] mod of frequency
From: Geoff Lansberry @ 2016-12-20 16:10 UTC (permalink / raw)
To: linux-wireless
Cc: lauro.venancio, aloisio.almeida, sameo, robh+dt, mark.rutland,
netdev, devicetree, linux-kernel, mgreer, justin, Geoff Lansberry,
Geoff Lansberry
In-Reply-To: <1482250250-4192-1-git-send-email-glansberry@gmail.com>
---
drivers/nfc/trf7970a.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
index e9e93ea..5916737 100644
--- a/drivers/nfc/trf7970a.c
+++ b/drivers/nfc/trf7970a.c
@@ -2076,10 +2076,10 @@ static int trf7970a_probe(struct spi_device *spi)
of_property_read_u32(np, "clock-frequency", &clk_freq);
if ((clk_freq != TRF7970A_27MHZ_CLOCK_FREQUENCY) ||
(clk_freq != TRF7970A_27MHZ_CLOCK_FREQUENCY)) {
- dev_err(trf->dev,
- "clock-frequency (%u Hz) unsupported\n",
- clk_freq);
- return -EINVAL;
+ dev_err(trf->dev,
+ "clock-frequency (%u Hz) unsupported\n",
+ clk_freq);
+ return -EINVAL;
}
if (of_property_read_bool(np, "en2-rf-quirk"))
--
Signed-off-by: Geoff Lansberry <geoff@kuvee.com>
^ permalink raw reply related
* [PATCH 2/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel
From: Geoff Lansberry @ 2016-12-20 16:10 UTC (permalink / raw)
To: linux-wireless
Cc: lauro.venancio, aloisio.almeida, sameo, robh+dt, mark.rutland,
netdev, devicetree, linux-kernel, mgreer, justin, Jaret Cantu,
Geoff Lansberry
In-Reply-To: <1482250250-4192-1-git-send-email-glansberry@gmail.com>
From: Jaret Cantu <jaret.cantu@timesys.com>
Repeated polling attempts cause a NULL dereference error to occur.
This is because the state of the trf7970a is currently reading but
another request has been made to send a command before it has finished.
The solution is to properly kill the waiting reading (workqueue)
before failing on the send.
---
drivers/nfc/trf7970a.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
index 94c31f8..e9e93ea 100644
--- a/drivers/nfc/trf7970a.c
+++ b/drivers/nfc/trf7970a.c
@@ -1496,6 +1496,10 @@ static int trf7970a_send_cmd(struct nfc_digital_dev *ddev,
(trf->state != TRF7970A_ST_IDLE_RX_BLOCKED)) {
dev_err(trf->dev, "%s - Bogus state: %d\n", __func__,
trf->state);
+ if (trf->state == TRF7970A_ST_WAIT_FOR_RX_DATA ||
+ trf->state == TRF7970A_ST_WAIT_FOR_RX_DATA_CONT)
+ trf->ignore_timeout =
+ !cancel_delayed_work(&trf->timeout_work);
ret = -EIO;
goto out_err;
}
--
Signed-off-by: Geoff Lansberry <geoff@kuvee.com>
^ permalink raw reply related
* [PATCH 3/3] mod of frequency
From: Geoff Lansberry @ 2016-12-20 16:10 UTC (permalink / raw)
To: linux-wireless
Cc: lauro.venancio, aloisio.almeida, sameo, robh+dt, mark.rutland,
netdev, devicetree, linux-kernel, mgreer, justin, Geoff Lansberry,
Geoff Lansberry
In-Reply-To: <1482250250-4192-1-git-send-email-glansberry@gmail.com>
---
drivers/nfc/trf7970a.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
index e9e93ea..5916737 100644
--- a/drivers/nfc/trf7970a.c
+++ b/drivers/nfc/trf7970a.c
@@ -2076,10 +2076,10 @@ static int trf7970a_probe(struct spi_device *spi)
of_property_read_u32(np, "clock-frequency", &clk_freq);
if ((clk_freq != TRF7970A_27MHZ_CLOCK_FREQUENCY) ||
(clk_freq != TRF7970A_27MHZ_CLOCK_FREQUENCY)) {
- dev_err(trf->dev,
- "clock-frequency (%u Hz) unsupported\n",
- clk_freq);
- return -EINVAL;
+ dev_err(trf->dev,
+ "clock-frequency (%u Hz) unsupported\n",
+ clk_freq);
+ return -EINVAL;
}
if (of_property_read_bool(np, "en2-rf-quirk"))
--
Signed-off-by: Geoff Lansberry <geoff@kuvee.com>
^ permalink raw reply related
* [PATCH 2/3] NFC: trf7970a: Add device tree option of 1.8 Volt IO voltage
From: Geoff Lansberry @ 2016-12-20 16:10 UTC (permalink / raw)
To: linux-wireless
Cc: lauro.venancio, aloisio.almeida, sameo, robh+dt, mark.rutland,
netdev, devicetree, linux-kernel, mgreer, justin, Geoff Lansberry
In-Reply-To: <1482250250-4192-1-git-send-email-glansberry@gmail.com>
From: Geoff Lansberry <geoff@kuvee.com>
The TRF7970A has configuration options for supporting hardware designs
with 1.8 Volt or 3.3 Volt IO. This commit adds a device tree option,
using a fixed regulator binding, for setting the io voltage to match
the hardware configuration. If no option is supplied it defaults to
3.3 volt configuration.
---
.../devicetree/bindings/net/nfc/trf7970a.txt | 4 ++--
drivers/nfc/trf7970a.c | 28 +++++++++++++++++++++-
2 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/nfc/trf7970a.txt b/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
index e262ac1..b5777d8 100644
--- a/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
+++ b/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
@@ -21,9 +21,9 @@ Optional SoC Specific Properties:
- t5t-rmb-extra-byte-quirk: Specify that the trf7970a has the erratum
where an extra byte is returned by Read Multiple Block commands issued
to Type 5 tags.
+- vdd-io-supply: Regulator specifying voltage for vdd-io
- clock-frequency: Set to specify that the input frequency to the trf7970a is 13560000Hz or 27120000Hz
-
Example (for ARM-based BeagleBone with TRF7970A on SPI1):
&spi1 {
@@ -41,11 +41,11 @@ Example (for ARM-based BeagleBone with TRF7970A on SPI1):
<&gpio2 5 GPIO_ACTIVE_LOW>;
vin-supply = <&ldo3_reg>;
vin-voltage-override = <5000000>;
+ vdd-io-supply = <&ldo2_reg>;
autosuspend-delay = <30000>;
irq-status-read-quirk;
en2-rf-quirk;
t5t-rmb-extra-byte-quirk;
- vdd_io_1v8;
clock-frequency = <27120000>;
status = "okay";
};
diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
index 4e051e9..8a88195 100644
--- a/drivers/nfc/trf7970a.c
+++ b/drivers/nfc/trf7970a.c
@@ -444,6 +444,7 @@ struct trf7970a {
u8 iso_ctrl_tech;
u8 modulator_sys_clk_ctrl;
u8 special_fcn_reg1;
+ u8 io_ctrl;
unsigned int guard_time;
int technology;
int framing;
@@ -1051,6 +1052,11 @@ static int trf7970a_init(struct trf7970a *trf)
if (ret)
goto err_out;
+ ret = trf7970a_write(trf, TRF7970A_REG_IO_CTRL,
+ trf->io_ctrl | TRF7970A_REG_IO_CTRL_VRS(0x1));
+ if (ret)
+ goto err_out;
+
ret = trf7970a_write(trf, TRF7970A_NFC_TARGET_LEVEL, 0);
if (ret)
goto err_out;
@@ -1767,7 +1773,7 @@ static int _trf7970a_tg_listen(struct nfc_digital_dev *ddev, u16 timeout,
goto out_err;
ret = trf7970a_write(trf, TRF7970A_REG_IO_CTRL,
- TRF7970A_REG_IO_CTRL_VRS(0x1));
+ trf->io_ctrl | TRF7970A_REG_IO_CTRL_VRS(0x1));
if (ret)
goto out_err;
@@ -2062,6 +2068,7 @@ static int trf7970a_probe(struct spi_device *spi)
return ret;
}
+
of_property_read_u32(np, "clock-frequency", &clk_freq);
if ((clk_freq != TRF7970A_27MHZ_CLOCK_FREQUENCY) ||
(clk_freq != TRF7970A_27MHZ_CLOCK_FREQUENCY)) {
@@ -2105,6 +2112,25 @@ static int trf7970a_probe(struct spi_device *spi)
if (uvolts > 4000000)
trf->chip_status_ctrl = TRF7970A_CHIP_STATUS_VRS5_3;
+ trf->regulator = devm_regulator_get(&spi->dev, "vdd-io");
+ if (IS_ERR(trf->regulator)) {
+ ret = PTR_ERR(trf->regulator);
+ dev_err(trf->dev, "Can't get VDD_IO regulator: %d\n", ret);
+ goto err_destroy_lock;
+ }
+
+ ret = regulator_enable(trf->regulator);
+ if (ret) {
+ dev_err(trf->dev, "Can't enable VDD_IO: %d\n", ret);
+ goto err_destroy_lock;
+ }
+
+
+ if (regulator_get_voltage(trf->regulator) == 1800000) {
+ trf->io_ctrl = TRF7970A_REG_IO_CTRL_IO_LOW;
+ dev_dbg(trf->dev, "trf7970a config vdd_io to 1.8V\n");
+ }
+
trf->ddev = nfc_digital_allocate_device(&trf7970a_nfc_ops,
TRF7970A_SUPPORTED_PROTOCOLS,
NFC_DIGITAL_DRV_CAPS_IN_CRC |
--
Signed-off-by: Geoff Lansberry <geoff@kuvee.com>
^ permalink raw reply related
* [PATCH 1/3] NFC: trf7970a: Add device tree option of 1.8 Volt IO voltage
From: Geoff Lansberry @ 2016-12-20 16:10 UTC (permalink / raw)
To: linux-wireless
Cc: lauro.venancio, aloisio.almeida, sameo, robh+dt, mark.rutland,
netdev, devicetree, linux-kernel, mgreer, justin, Geoff Lansberry
In-Reply-To: <1482250250-4192-1-git-send-email-glansberry@gmail.com>
From: Geoff Lansberry <geoff@kuvee.com>
The TRF7970A has configuration options for supporting hardware designs
with 1.8 Volt or 3.3 Volt IO. This commit adds a device tree option,
using a fixed regulator binding, for setting the io voltage to match
the hardware configuration. If no option is supplied it defaults to
3.3 volt configuration.
---
.../devicetree/bindings/net/nfc/trf7970a.txt | 4 ++--
drivers/nfc/trf7970a.c | 28 +++++++++++++++++++++-
2 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/nfc/trf7970a.txt b/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
index e262ac1..b5777d8 100644
--- a/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
+++ b/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
@@ -21,9 +21,9 @@ Optional SoC Specific Properties:
- t5t-rmb-extra-byte-quirk: Specify that the trf7970a has the erratum
where an extra byte is returned by Read Multiple Block commands issued
to Type 5 tags.
+- vdd-io-supply: Regulator specifying voltage for vdd-io
- clock-frequency: Set to specify that the input frequency to the trf7970a is 13560000Hz or 27120000Hz
-
Example (for ARM-based BeagleBone with TRF7970A on SPI1):
&spi1 {
@@ -41,11 +41,11 @@ Example (for ARM-based BeagleBone with TRF7970A on SPI1):
<&gpio2 5 GPIO_ACTIVE_LOW>;
vin-supply = <&ldo3_reg>;
vin-voltage-override = <5000000>;
+ vdd-io-supply = <&ldo2_reg>;
autosuspend-delay = <30000>;
irq-status-read-quirk;
en2-rf-quirk;
t5t-rmb-extra-byte-quirk;
- vdd_io_1v8;
clock-frequency = <27120000>;
status = "okay";
};
diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
index c9cb278..94c31f8 100644
--- a/drivers/nfc/trf7970a.c
+++ b/drivers/nfc/trf7970a.c
@@ -444,6 +444,7 @@ struct trf7970a {
u8 iso_ctrl_tech;
u8 modulator_sys_clk_ctrl;
u8 special_fcn_reg1;
+ u8 io_ctrl;
unsigned int guard_time;
int technology;
int framing;
@@ -1051,6 +1052,11 @@ static int trf7970a_init(struct trf7970a *trf)
if (ret)
goto err_out;
+ ret = trf7970a_write(trf, TRF7970A_REG_IO_CTRL,
+ trf->io_ctrl | TRF7970A_REG_IO_CTRL_VRS(0x1));
+ if (ret)
+ goto err_out;
+
ret = trf7970a_write(trf, TRF7970A_NFC_TARGET_LEVEL, 0);
if (ret)
goto err_out;
@@ -1767,7 +1773,7 @@ static int _trf7970a_tg_listen(struct nfc_digital_dev *ddev, u16 timeout,
goto out_err;
ret = trf7970a_write(trf, TRF7970A_REG_IO_CTRL,
- TRF7970A_REG_IO_CTRL_VRS(0x1));
+ trf->io_ctrl | TRF7970A_REG_IO_CTRL_VRS(0x1));
if (ret)
goto out_err;
@@ -2062,6 +2068,7 @@ static int trf7970a_probe(struct spi_device *spi)
return ret;
}
+
of_property_read_u32(np, "clock-frequency", &clk_freq);
if ((clk_freq != TRF7970A_27MHZ_CLOCK_FREQUENCY) ||
(clk_freq != TRF7970A_27MHZ_CLOCK_FREQUENCY)) {
@@ -2105,6 +2112,25 @@ static int trf7970a_probe(struct spi_device *spi)
if (uvolts > 4000000)
trf->chip_status_ctrl = TRF7970A_CHIP_STATUS_VRS5_3;
+ trf->regulator = devm_regulator_get(&spi->dev, "vdd-io");
+ if (IS_ERR(trf->regulator)) {
+ ret = PTR_ERR(trf->regulator);
+ dev_err(trf->dev, "Can't get VDD_IO regulator: %d\n", ret);
+ goto err_destroy_lock;
+ }
+
+ ret = regulator_enable(trf->regulator);
+ if (ret) {
+ dev_err(trf->dev, "Can't enable VDD_IO: %d\n", ret);
+ goto err_destroy_lock;
+ }
+
+
+ if (regulator_get_voltage(trf->regulator) == 1800000) {
+ trf->io_ctrl = TRF7970A_REG_IO_CTRL_IO_LOW;
+ dev_dbg(trf->dev, "trf7970a config vdd_io to 1.8V\n");
+ }
+
trf->ddev = nfc_digital_allocate_device(&trf7970a_nfc_ops,
TRF7970A_SUPPORTED_PROTOCOLS,
NFC_DIGITAL_DRV_CAPS_IN_CRC |
--
Signed-off-by: Geoff Lansberry <geoff@kuvee.com>
^ permalink raw reply related
* [PATCH 1/3] NFC: trf7970a: add device tree option for 27MHz clock
From: Geoff Lansberry @ 2016-12-20 16:10 UTC (permalink / raw)
To: linux-wireless
Cc: lauro.venancio, aloisio.almeida, sameo, robh+dt, mark.rutland,
netdev, devicetree, linux-kernel, mgreer, justin, Geoff Lansberry
From: Geoff Lansberry <geoff@kuvee.com>
The TRF7970A has configuration options to support hardware designs
which use a 27.12MHz clock. This commit adds a device tree option
'clock-frequency' to support configuring the this chip for default
13.56MHz clock or the optional 27.12MHz clock.
---
.../devicetree/bindings/net/nfc/trf7970a.txt | 4 ++
drivers/nfc/trf7970a.c | 50 +++++++++++++++++-----
2 files changed, 43 insertions(+), 11 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/nfc/trf7970a.txt b/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
index 32b35a0..e262ac1 100644
--- a/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
+++ b/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
@@ -21,6 +21,8 @@ Optional SoC Specific Properties:
- t5t-rmb-extra-byte-quirk: Specify that the trf7970a has the erratum
where an extra byte is returned by Read Multiple Block commands issued
to Type 5 tags.
+- clock-frequency: Set to specify that the input frequency to the trf7970a is 13560000Hz or 27120000Hz
+
Example (for ARM-based BeagleBone with TRF7970A on SPI1):
@@ -43,6 +45,8 @@ Example (for ARM-based BeagleBone with TRF7970A on SPI1):
irq-status-read-quirk;
en2-rf-quirk;
t5t-rmb-extra-byte-quirk;
+ vdd_io_1v8;
+ clock-frequency = <27120000>;
status = "okay";
};
};
diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
index 26c9dbb..4e051e9 100644
--- a/drivers/nfc/trf7970a.c
+++ b/drivers/nfc/trf7970a.c
@@ -124,6 +124,9 @@
NFC_PROTO_ISO15693_MASK | NFC_PROTO_NFC_DEP_MASK)
#define TRF7970A_AUTOSUSPEND_DELAY 30000 /* 30 seconds */
+#define TRF7970A_13MHZ_CLOCK_FREQUENCY 13560000
+#define TRF7970A_27MHZ_CLOCK_FREQUENCY 27120000
+
#define TRF7970A_RX_SKB_ALLOC_SIZE 256
@@ -1056,12 +1059,11 @@ static int trf7970a_init(struct trf7970a *trf)
trf->chip_status_ctrl &= ~TRF7970A_CHIP_STATUS_RF_ON;
- ret = trf7970a_write(trf, TRF7970A_MODULATOR_SYS_CLK_CTRL, 0);
+ ret = trf7970a_write(trf, TRF7970A_MODULATOR_SYS_CLK_CTRL,
+ trf->modulator_sys_clk_ctrl);
if (ret)
goto err_out;
- trf->modulator_sys_clk_ctrl = 0;
-
ret = trf7970a_write(trf, TRF7970A_ADJUTABLE_FIFO_IRQ_LEVELS,
TRF7970A_ADJUTABLE_FIFO_IRQ_LEVELS_WLH_96 |
TRF7970A_ADJUTABLE_FIFO_IRQ_LEVELS_WLL_32);
@@ -1181,27 +1183,37 @@ static int trf7970a_in_config_rf_tech(struct trf7970a *trf, int tech)
switch (tech) {
case NFC_DIGITAL_RF_TECH_106A:
trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_14443A_106;
- trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_OOK;
+ trf->modulator_sys_clk_ctrl =
+ (trf->modulator_sys_clk_ctrl & 0xF8) |
+ TRF7970A_MODULATOR_DEPTH_OOK;
trf->guard_time = TRF7970A_GUARD_TIME_NFCA;
break;
case NFC_DIGITAL_RF_TECH_106B:
trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_14443B_106;
- trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_ASK10;
+ trf->modulator_sys_clk_ctrl =
+ (trf->modulator_sys_clk_ctrl & 0xF8) |
+ TRF7970A_MODULATOR_DEPTH_ASK10;
trf->guard_time = TRF7970A_GUARD_TIME_NFCB;
break;
case NFC_DIGITAL_RF_TECH_212F:
trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_FELICA_212;
- trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_ASK10;
+ trf->modulator_sys_clk_ctrl =
+ (trf->modulator_sys_clk_ctrl & 0xF8) |
+ TRF7970A_MODULATOR_DEPTH_ASK10;
trf->guard_time = TRF7970A_GUARD_TIME_NFCF;
break;
case NFC_DIGITAL_RF_TECH_424F:
trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_FELICA_424;
- trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_ASK10;
+ trf->modulator_sys_clk_ctrl =
+ (trf->modulator_sys_clk_ctrl & 0xF8) |
+ TRF7970A_MODULATOR_DEPTH_ASK10;
trf->guard_time = TRF7970A_GUARD_TIME_NFCF;
break;
case NFC_DIGITAL_RF_TECH_ISO15693:
trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_15693_SGL_1OF4_2648;
- trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_OOK;
+ trf->modulator_sys_clk_ctrl =
+ (trf->modulator_sys_clk_ctrl & 0xF8) |
+ TRF7970A_MODULATOR_DEPTH_OOK;
trf->guard_time = TRF7970A_GUARD_TIME_15693;
break;
default:
@@ -1571,17 +1583,23 @@ static int trf7970a_tg_config_rf_tech(struct trf7970a *trf, int tech)
trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_NFC_NFC_CE_MODE |
TRF7970A_ISO_CTRL_NFC_CE |
TRF7970A_ISO_CTRL_NFC_CE_14443A;
- trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_OOK;
+ trf->modulator_sys_clk_ctrl =
+ (trf->modulator_sys_clk_ctrl & 0xF8) |
+ TRF7970A_MODULATOR_DEPTH_OOK;
break;
case NFC_DIGITAL_RF_TECH_212F:
trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_NFC_NFC_CE_MODE |
TRF7970A_ISO_CTRL_NFC_NFCF_212;
- trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_ASK10;
+ trf->modulator_sys_clk_ctrl =
+ (trf->modulator_sys_clk_ctrl & 0xF8) |
+ TRF7970A_MODULATOR_DEPTH_ASK10;
break;
case NFC_DIGITAL_RF_TECH_424F:
trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_NFC_NFC_CE_MODE |
TRF7970A_ISO_CTRL_NFC_NFCF_424;
- trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_ASK10;
+ trf->modulator_sys_clk_ctrl =
+ (trf->modulator_sys_clk_ctrl & 0xF8) |
+ TRF7970A_MODULATOR_DEPTH_ASK10;
break;
default:
dev_dbg(trf->dev, "Unsupported rf technology: %d\n", tech);
@@ -1987,6 +2005,7 @@ static int trf7970a_probe(struct spi_device *spi)
struct device_node *np = spi->dev.of_node;
struct trf7970a *trf;
int uvolts, autosuspend_delay, ret;
+ u32 clk_freq = 13560000;
if (!np) {
dev_err(&spi->dev, "No Device Tree entry\n");
@@ -2043,6 +2062,15 @@ static int trf7970a_probe(struct spi_device *spi)
return ret;
}
+ of_property_read_u32(np, "clock-frequency", &clk_freq);
+ if ((clk_freq != TRF7970A_27MHZ_CLOCK_FREQUENCY) ||
+ (clk_freq != TRF7970A_27MHZ_CLOCK_FREQUENCY)) {
+ dev_err(trf->dev,
+ "clock-frequency (%u Hz) unsupported\n",
+ clk_freq);
+ return -EINVAL;
+ }
+
if (of_property_read_bool(np, "en2-rf-quirk"))
trf->quirks |= TRF7970A_QUIRK_EN2_MUST_STAY_LOW;
--
Signed-off-by: Geoff Lansberry <geoff@kuvee.com>
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox