Re: [RFC v2 11/11] ath10k: Added sdio support

[Erik Stromdahl]

On 12/16/2016 12:21 PM, Valo, Kalle wrote:
> Erik Stromdahl  writes:
> 
>> Initial HIF sdio/mailbox implementation.
>>
>> Signed-off-by: Erik Stromdahl 
> 
> 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 = _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 = >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 *)[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 = >endpoint[id];
>> +
>> +if (ep->service_id == 0) {
>> +ath10k_err(ar, "ep %d is not connected !\n", id);
>> +status = -ENOMEM;
>> +goto out;
>> +}
>> +
>> +pkt = _sdio->rx_pkts[i];
>> +
>> +if (pkt->part_of_bundle && !pkt->last_in_bundle) {
>> +/* Only read lookahead's from RX trailers
>> + * for 

Re: [RFC v2 11/11] ath10k: Added sdio support

[Valo, Kalle]

Erik Stromdahl  writes:

> Initial HIF sdio/mailbox implementation.
>
> Signed-off-by: Erik Stromdahl 

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 :)

> +#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 = _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 = >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 *)[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 = >endpoint[id];
> +
> + if (ep->service_id == 0) {
> + ath10k_err(ar, "ep %d is not connected !\n", id);
> + status = -ENOMEM;
> + goto out;
> + }
> +
> + pkt = _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++)
> 

Re: [RFC v2 11/11] ath10k: Added sdio support

[Erik Stromdahl]

On 12/15/2016 05:40 PM, Valo, Kalle wrote:
> Erik Stromdahl  writes:
> 
>> Initial HIF sdio/mailbox implementation.
>>
>> Signed-off-by: Erik Stromdahl 
>> ---
>>  drivers/net/wireless/ath/ath10k/Kconfig  |6 +
>>  drivers/net/wireless/ath/ath10k/Makefile |3 +
>>  drivers/net/wireless/ath/ath10k/sdio.c   | 1860 
>> ++
>>  drivers/net/wireless/ath/ath10k/sdio.h   |  276 +
>>  4 files changed, 2145 insertions(+)
>>  create mode 100644 drivers/net/wireless/ath/ath10k/sdio.c
>>  create mode 100644 drivers/net/wireless/ath/ath10k/sdio.h
> 
> AFAIK the sdio firmware binary is different from pci and usb. And as all
> the firmware images need to coexist in the same repository I suspect we
> can't continue to use firmware-N.bin for both pcie and sdio (and in
> future usb). So should we somehow rename the sdio firmware filename to
> something else, like firmware-sdio-N.bin?
> 
I suppose you are right. I would be surprised if the firmware images
were the same for sdio, usb and pci. Hopefully all sdio chipsets sharing
the same device id and BMI version will use the same firmware image.
Currently, these two params are the only two that are used when
selecting firmware.

struct bmi_target_info also has a type parameter, but I can't see it
being used anywhere in the code.

If it turns out that several sdio chipsets uses the same BMI version and
device id, this parameter might be used (together with the other two) to
select hw params.

Re: [RFC v2 11/11] ath10k: Added sdio support

[Valo, Kalle]

Erik Stromdahl  writes:

> Initial HIF sdio/mailbox implementation.
>
> Signed-off-by: Erik Stromdahl 
> ---
>  drivers/net/wireless/ath/ath10k/Kconfig  |6 +
>  drivers/net/wireless/ath/ath10k/Makefile |3 +
>  drivers/net/wireless/ath/ath10k/sdio.c   | 1860 
> ++
>  drivers/net/wireless/ath/ath10k/sdio.h   |  276 +
>  4 files changed, 2145 insertions(+)
>  create mode 100644 drivers/net/wireless/ath/ath10k/sdio.c
>  create mode 100644 drivers/net/wireless/ath/ath10k/sdio.h

AFAIK the sdio firmware binary is different from pci and usb. And as all
the firmware images need to coexist in the same repository I suspect we
can't continue to use firmware-N.bin for both pcie and sdio (and in
future usb). So should we somehow rename the sdio firmware filename to
something else, like firmware-sdio-N.bin?

-- 
Kalle Valo

Re: [RFC v2 11/11] ath10k: Added sdio support

[Valo, Kalle]

Erik Stromdahl  writes:

> Initial HIF sdio/mailbox implementation.
>
> Signed-off-by: Erik Stromdahl 

While testing this I noticed few new warnings:

drivers/net/wireless/ath/ath10k/sdio.c: In function ath10k_sdio_probe:
drivers/net/wireless/ath/ath10k/sdio.c:1723:6: warning: 'ret' may be used 
uninitialized in this function [-Wuninitialized]
drivers/net/wireless/ath/ath10k/sdio.c:375:5: warning: symbol 
'ath10k_sdio_mbox_rxmsg_pending_handler' was not declared. Should it be static?
drivers/net/wireless/ath/ath10k/sdio.c:1018:5: warning: symbol 
'ath10k_sdio_hif_tx_sg' was not declared. Should it be static?
drivers/net/wireless/ath/ath10k/sdio.c:1415:5: warning: symbol 
'ath10k_sdio_hif_exchange_bmi_msg' was not declared. Should it be static?
drivers/net/wireless/ath/ath10k/sdio.c:1555:5: warning: symbol 
'ath10k_sdio_hif_map_service_to_pipe' was not declared. Should it be static?
drivers/net/wireless/ath/ath10k/sdio.c:1635:6: warning: symbol 
'ath10k_sdio_hif_get_default_pipe' was not declared. Should it be static?
drivers/net/wireless/ath/ath10k/htc.c:265: line over 90 characters
drivers/net/wireless/ath/ath10k/htc.c:355: line over 90 characters

-- 
Kalle Valo