Re: [PATCH 1/2] rsi: New firware loading method for RSI 91X devices

[Kalle Valo]

Prameela Rani Garnepudi  writes:

> RSI deprecated the old firmware loading method and introduced
> new method using soft boot loader for 9113 chipsets.
> Current driver only supports 9113 device model hence firmware
> loading method has been changed.
>
> In the new method, complete RAM image and flash image are present
> in the flash. Two firmwares present in the device, Boot loader firmware
> and functional firmware. Boot loader firmware is fixed but functional
> firmware can be changed. Before loading the functional firmware, host
> issues commands to check whether existing firmware in the chip and the
> firmware file content to load are same or not. If not, host issues
> commands to load the RAM image and then boot loaded switches to the
> functioanl firmware.
>
> Signed-off-by: Prameela Rani Garnepudi 

Forgot this:


[...]

> +#define FIRMWARE_RSI9113 "rsi_91x.fw"

I see that you add the define here but I don't see you anywhere removing
the old one. I assume you were planning just to move it.

-- 
Kalle Valo

Re: [PATCH 1/2] rsi: New firware loading method for RSI 91X devices

[Kalle Valo]

Prameela Rani Garnepudi  writes:

> RSI deprecated the old firmware loading method and introduced
> new method using soft boot loader for 9113 chipsets.
> Current driver only supports 9113 device model hence firmware
> loading method has been changed.
>
> In the new method, complete RAM image and flash image are present
> in the flash. Two firmwares present in the device, Boot loader firmware
> and functional firmware. Boot loader firmware is fixed but functional
> firmware can be changed. Before loading the functional firmware, host
> issues commands to check whether existing firmware in the chip and the
> firmware file content to load are same or not. If not, host issues
> commands to load the RAM image and then boot loaded switches to the
> functioanl firmware.
>
> Signed-off-by: Prameela Rani Garnepudi 

A general rule is that existing firmware support should not be broken.
Instead there should be a transition period for some time end then
support for old firmware method is removed. But as I don't know if that
upstream driver is not that widely used I guess it might be ok to break
it as it won't cause that much problems to people.

Typo in the title: "firware"

>  drivers/net/wireless/rsi/Makefile   |2 +-
>  drivers/net/wireless/rsi/rsi_91x_hal.c  | 1049 
> +++
>  drivers/net/wireless/rsi/rsi_91x_mgmt.c |   49 ++
>  drivers/net/wireless/rsi/rsi_91x_pkt.c  |  215 --
>  drivers/net/wireless/rsi/rsi_91x_sdio.c |  231 +-
>  drivers/net/wireless/rsi/rsi_91x_sdio_ops.c |  192 +
>  drivers/net/wireless/rsi/rsi_91x_usb.c  |  176 -
>  drivers/net/wireless/rsi/rsi_91x_usb_ops.c  |  130 +---
>  drivers/net/wireless/rsi/rsi_common.h   |4 +
>  drivers/net/wireless/rsi/rsi_hal.h  |  150 
>  drivers/net/wireless/rsi/rsi_main.h |   68 +-
>  drivers/net/wireless/rsi/rsi_mgmt.h |2 +
>  drivers/net/wireless/rsi/rsi_sdio.h |   18 +-
>  drivers/net/wireless/rsi/rsi_usb.h  |   14 +-
>  14 files changed, 1720 insertions(+), 580 deletions(-)
>  create mode 100644 drivers/net/wireless/rsi/rsi_91x_hal.c
>  delete mode 100644 drivers/net/wireless/rsi/rsi_91x_pkt.c
>  create mode 100644 drivers/net/wireless/rsi/rsi_hal.h

The patch is quite big which makes review hard. If you had split this
into, for example, three patches it would be a lot faster to review. You
seem to be doing multiple logical changes in one path.

But remember that neither the build nor runtime functionality should
break between the patches.

> +/* FLASH Firmware */
> +struct ta_metadata metadata_flash_content[] = {
> + {"flash_content", 0x0001},
> + {"RS9113_WLAN_QSPI.rps", 0x0001},
> + {"RS9113_WLAN_BT_DUAL_MODE.rps", 0x0001},
> + {"RS9113_WLAN_ZIGBEE.rps", 0x0001},
> + {"RS9113_AP_BT_DUAL_MODE.rps", 0x0001},
> + {"RS9113_WLAN_QSPI.rps", 0x0001}
> +};

Are the strings here the names of the firmware images on host
filesystem? The preference is that they are lower case.

Also will you be posting the firmware images to linux-firmware.git?

> +
> +/**
> + * rsi_send_data_pkt() - This function sends the received data packet from
> + *driver to device.
> + * @common: Pointer to the driver private structure.
> + * @skb: Pointer to the socket buffer structure.
> + *
> + * Return: status: 0 on success, -1 on failure.
> + */
> +int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
> +{
> + struct rsi_hw *adapter = common->priv;
> + struct ieee80211_hdr *wh = NULL;
> + struct ieee80211_tx_info *info;
> + struct skb_info *tx_params;
> + struct ieee80211_bss_conf *bss = NULL;
> + int status = -EINVAL;

Documentation and code don't match again.

> + u8 ieee80211_hdr_size = MIN_802_11_HDR_LEN;
> + u8 dword_align_bytes = 0;
> + u8 header_size = 0;
> + __le16 *frame_desc;
> + struct xtended_desc *xtend_desc;
> + u16 seq_num = 0;
> +
> + info = IEEE80211_SKB_CB(skb);
> + bss = >control.vif->bss_conf;
> + tx_params = (struct skb_info *)info->driver_data;
> +
> + if (!bss->assoc)
> + goto err;
> +
> + dword_align_bytes = ((uintptr_t)skb->data & 0x3f);

ALIGN()?

> + header_size = dword_align_bytes + FRAME_DESC_SZ +
> + sizeof(struct xtended_desc);
> + if (header_size > skb_headroom(skb)) {
> + rsi_dbg(ERR_ZONE, "%s: Not enough headroom\n", __func__);
> + status = -ENOSPC;
> + goto err;
> + }
> +
> + skb_push(skb, header_size);
> + frame_desc = (__le16 *)>data[0];
> + xtend_desc = (struct xtended_desc *)>data[FRAME_DESC_SZ];
> + memset((u8 *)frame_desc, 0, header_size);
> +
> + wh = (struct ieee80211_hdr *)>data[header_size];
> + seq_num = (le16_to_cpu(wh->seq_ctrl) >> 4);

I would hope include/linux/ieee80211.h already provides a 

Re: [1/2] rsi: New firware loading method for RSI 91X devices

[Kalle Valo]

Prameela Rani Garnepudi <prameela.garnep...@redpinesignals.com> writes:

>  
> On 11/09/2016 06:45 AM, Kalle Valo wrote: 
>> Prameela Rani Garnepudi <prameela.j0...@gmail.com> wrote: 
>>> RSI deprecated the old firmware loading method and introduced 
>>> new method using soft boot loader for 9113 chipsets. 
>>> Current driver only supports 9113 device model hence firmware 
>>> loading method has been changed. 
>>> 
>>> In the new method, complete RAM image and flash image are present 
>>> in the flash. Two firmwares present in the device, Boot loader firmware 
>>> and functional firmware. Boot loader firmware is fixed but functional 
>>> firmware can be changed. Before loading the functional firmware, host 
>>> issues commands to check whether existing firmware in the chip and the 
>>> firmware file content to load are same or not. If not, host issues 
>>> commands to load the RAM image and then boot loaded switches to the 
>>> functioanl firmware. 
>>> 
>>> Signed-off-by: Prameela Rani Garnepudi <prameela.j0...@gmail.com> 
>> These two patches are quite big, difficult to review. Smaller changes 
>> would help with that. Will review later. 
>> 
>> 2 patches set to Deferred. 
>> 
>> 9388629 [1/2] rsi: New firware loading method for RSI 91X devices 
>> 9388627 [2/2] rsi: Device initialization sequence is changed 
>> 
> Hi, 
>  
> Can you please let me know when will you consider to review these two  
> patches. Because these are the mandatory and important patches to go  
> into the mail line kernel. If it gets too late can I send each one as  
> patch set? Please let me know urgently. 

I'm still catching up with everything after my travels, sorry for the
delay. I'm hoping to get to your patches in next few days and give
better answers.

>From the last review I noticed that you both moved functions and made
functionality changes in the same patch. These are two logically
different changes and should be in different patches to make the review
easier. That's why I put them to deferred in the first place.

-- 
Kalle Valo

Re: [1/2] rsi: New firware loading method for RSI 91X devices

[Kalle Valo]

Prameela Rani Garnepudi <prameela.j0...@gmail.com> wrote:
> RSI deprecated the old firmware loading method and introduced
> new method using soft boot loader for 9113 chipsets.
> Current driver only supports 9113 device model hence firmware
> loading method has been changed.
> 
> In the new method, complete RAM image and flash image are present
> in the flash. Two firmwares present in the device, Boot loader firmware
> and functional firmware. Boot loader firmware is fixed but functional
> firmware can be changed. Before loading the functional firmware, host
> issues commands to check whether existing firmware in the chip and the
> firmware file content to load are same or not. If not, host issues
> commands to load the RAM image and then boot loaded switches to the
> functioanl firmware.
> 
> Signed-off-by: Prameela Rani Garnepudi <prameela.j0...@gmail.com>

These two patches are quite big, difficult to review. Smaller changes
would help with that. Will review later.

2 patches set to Deferred.

9388629 [1/2] rsi: New firware loading method for RSI 91X devices
9388627 [2/2] rsi: Device initialization sequence is changed

-- 
https://patchwork.kernel.org/patch/9388629/

Documentation about submitting wireless patches and checking status
from patchwork:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH 1/2] rsi: New firware loading method for RSI 91X devices

[Prameela Rani Garnepudi]

RSI deprecated the old firmware loading method and introduced
new method using soft boot loader for 9113 chipsets.
Current driver only supports 9113 device model hence firmware
loading method has been changed.

In the new method, complete RAM image and flash image are present
in the flash. Two firmwares present in the device, Boot loader firmware
and functional firmware. Boot loader firmware is fixed but functional
firmware can be changed. Before loading the functional firmware, host
issues commands to check whether existing firmware in the chip and the
firmware file content to load are same or not. If not, host issues
commands to load the RAM image and then boot loaded switches to the
functioanl firmware.

Signed-off-by: Prameela Rani Garnepudi 
---
 drivers/net/wireless/rsi/Makefile   |2 +-
 drivers/net/wireless/rsi/rsi_91x_hal.c  | 1049 +++
 drivers/net/wireless/rsi/rsi_91x_mgmt.c |   49 ++
 drivers/net/wireless/rsi/rsi_91x_pkt.c  |  215 --
 drivers/net/wireless/rsi/rsi_91x_sdio.c |  231 +-
 drivers/net/wireless/rsi/rsi_91x_sdio_ops.c |  192 +
 drivers/net/wireless/rsi/rsi_91x_usb.c  |  176 -
 drivers/net/wireless/rsi/rsi_91x_usb_ops.c  |  130 +---
 drivers/net/wireless/rsi/rsi_common.h   |4 +
 drivers/net/wireless/rsi/rsi_hal.h  |  150 
 drivers/net/wireless/rsi/rsi_main.h |   68 +-
 drivers/net/wireless/rsi/rsi_mgmt.h |2 +
 drivers/net/wireless/rsi/rsi_sdio.h |   18 +-
 drivers/net/wireless/rsi/rsi_usb.h  |   14 +-
 14 files changed, 1720 insertions(+), 580 deletions(-)
 create mode 100644 drivers/net/wireless/rsi/rsi_91x_hal.c
 delete mode 100644 drivers/net/wireless/rsi/rsi_91x_pkt.c
 create mode 100644 drivers/net/wireless/rsi/rsi_hal.h

diff --git a/drivers/net/wireless/rsi/Makefile 
b/drivers/net/wireless/rsi/Makefile
index 25828b6..a475c81 100644
--- a/drivers/net/wireless/rsi/Makefile
+++ b/drivers/net/wireless/rsi/Makefile
@@ -2,7 +2,7 @@ rsi_91x-y   += rsi_91x_main.o
 rsi_91x-y  += rsi_91x_core.o
 rsi_91x-y  += rsi_91x_mac80211.o
 rsi_91x-y  += rsi_91x_mgmt.o
-rsi_91x-y  += rsi_91x_pkt.o
+rsi_91x-y  += rsi_91x_hal.o
 rsi_91x-$(CONFIG_RSI_DEBUGFS)  += rsi_91x_debugfs.o
 
 rsi_usb-y  += rsi_91x_usb.o rsi_91x_usb_ops.o
diff --git a/drivers/net/wireless/rsi/rsi_91x_hal.c 
b/drivers/net/wireless/rsi/rsi_91x_hal.c
new file mode 100644
index 000..178d411
--- /dev/null
+++ b/drivers/net/wireless/rsi/rsi_91x_hal.c
@@ -0,0 +1,1049 @@
+/**
+ * Copyright (c) 2014 Redpine Signals Inc.
+ *
+ * Developers
+ * Prameela Rani Garnepudi 2016 
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include 
+#include 
+#include 
+#include "rsi_mgmt.h"
+#include "rsi_hal.h"
+#include "rsi_sdio.h"
+#include "rsi_common.h"
+
+/* FLASH Firmware */
+struct ta_metadata metadata_flash_content[] = {
+   {"flash_content", 0x0001},
+   {"RS9113_WLAN_QSPI.rps", 0x0001},
+   {"RS9113_WLAN_BT_DUAL_MODE.rps", 0x0001},
+   {"RS9113_WLAN_ZIGBEE.rps", 0x0001},
+   {"RS9113_AP_BT_DUAL_MODE.rps", 0x0001},
+   {"RS9113_WLAN_QSPI.rps", 0x0001}
+};
+
+/**
+ * rsi_send_data_pkt() - This function sends the received data packet from
+ *  driver to device.
+ * @common: Pointer to the driver private structure.
+ * @skb: Pointer to the socket buffer structure.
+ *
+ * Return: status: 0 on success, -1 on failure.
+ */
+int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
+{
+   struct rsi_hw *adapter = common->priv;
+   struct ieee80211_hdr *wh = NULL;
+   struct ieee80211_tx_info *info;
+   struct skb_info *tx_params;
+   struct ieee80211_bss_conf *bss = NULL;
+   int status = -EINVAL;
+   u8 ieee80211_hdr_size = MIN_802_11_HDR_LEN;
+   u8 dword_align_bytes = 0;
+   u8 header_size = 0;
+   __le16 *frame_desc;
+   struct xtended_desc *xtend_desc;
+   u16 seq_num = 0;
+
+   info = IEEE80211_SKB_CB(skb);
+   bss = >control.vif->bss_conf;
+   tx_params = (struct skb_info