Re: [PATCH v4] brcmfmac: add CLM download support
On 28-08-17 16:52, Steve deRosier wrote: On Mon, Aug 28, 2017 at 2:25 AM, Wright Feng wrote: From: Chung-Hsien Hsu The firmware for brcmfmac devices includes information regarding regulatory constraints. For certain devices this information is kept separately in a binary form that needs to be downloaded to the device. This patch adds support to download this so-called CLM blob file. It uses the same naming scheme as the other firmware files with extension of .clm_blob. Not to bikeshed this, but why not just ".clm"? ".clm_blob" seems unnecessary and could feel "unprofessional" to users. To me simple seems better. Hi Steve, Glad you ask ;-) It was one of my comments pre-reviewing this "internally"*. The problem with calling it .clm is that it means something else within Broadcom and Cypress for that matter. The .clm_blob is the output of tool compiling the regulatory data according an input specification which uses the .clm extension. It is similar to regdb.txt and regulatory.bin. Regards, Arend * internally does not quite cover Cypress+Broadcom collaboration.
Re: [PATCH v4] brcmfmac: add CLM download support
On Mon, Aug 28, 2017 at 2:25 AM, Wright Feng wrote: > From: Chung-Hsien Hsu > > The firmware for brcmfmac devices includes information regarding > regulatory constraints. For certain devices this information is kept > separately in a binary form that needs to be downloaded to the device. > This patch adds support to download this so-called CLM blob file. It > uses the same naming scheme as the other firmware files with extension > of .clm_blob. > Not to bikeshed this, but why not just ".clm"? ".clm_blob" seems unnecessary and could feel "unprofessional" to users. To me simple seems better. - Steve
Re: [PATCH v4] brcmfmac: add CLM download support
On Mon, Aug 28, 2017 at 12:27:38PM +0200, Arend van Spriel wrote:
> On 8/28/2017 11:25 AM, Wright Feng wrote:
> >From: Chung-Hsien Hsu
> >
> >The firmware for brcmfmac devices includes information regarding
> >regulatory constraints. For certain devices this information is kept
> >separately in a binary form that needs to be downloaded to the device.
> >This patch adds support to download this so-called CLM blob file. It
> >uses the same naming scheme as the other firmware files with extension
> >of .clm_blob.
> >
> >The CLM blob file is optional. If the file does not exist, the download
> >process will be bypassed. It will not affect the driver loading.
> >
> >Signed-off-by: Chung-Hsien Hsu
> >---
> >v2: Revise commit message to describe in more detail
> >v3: Add error handling in brcmf_c_get_clm_name function
> >v4: Correct the length of dload_buf in brcmf_c_download function
> >---
> > .../net/wireless/broadcom/brcm80211/brcmfmac/bus.h | 10 ++
> > .../wireless/broadcom/brcm80211/brcmfmac/common.c | 161
> > +
> > .../wireless/broadcom/brcm80211/brcmfmac/core.c| 2 +
> > .../wireless/broadcom/brcm80211/brcmfmac/core.h| 2 +
> > .../broadcom/brcm80211/brcmfmac/fwil_types.h | 31
> > .../wireless/broadcom/brcm80211/brcmfmac/pcie.c| 19 +++
> > .../wireless/broadcom/brcm80211/brcmfmac/sdio.c| 19 +++
> > .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 18 +++
> > 8 files changed, 262 insertions(+)
> >
>
> [...]
>
> >diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> >b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> >index 7a2b495..d09922b 100644
> >--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> >+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>
> [...]
>
> >@@ -104,12 +106,139 @@ void brcmf_c_set_joinpref_default(struct brcmf_if
> >*ifp)
> > brcmf_err("Set join_pref error (%d)\n", err);
> > }
> >
> >+static int brcmf_c_download(struct brcmf_if *ifp, u16 flag,
> >+struct brcmf_dload_data_le *dload_buf,
> >+u32 len)
> >+{
> >+s32 err;
> >+
> >+flag |= (DLOAD_HANDLER_VER << DLOAD_FLAG_VER_SHIFT);
> >+dload_buf->flag = cpu_to_le16(flag);
> >+dload_buf->dload_type = cpu_to_le16(DL_TYPE_CLM);
> >+dload_buf->len = cpu_to_le32(len);
> >+dload_buf->crc = cpu_to_le32(0);
> >+len = sizeof(*dload_buf) + len - 1;
> >+len = len + 8 - (len % 8);
>
> In case len is 8 this will result in 16. So better use roundup()
> macro from include/linux/kernel.h [1]. However, this means
> brcmf_fil_iovar_data_set() will read after the allocated buffer
> space.
>
> By the way, where does the alignment requirement come from?
>
Thanks for pointing this out. I've tried the download process without
this alignment. There's no error/failure showed up. Looks like the
alignment is not necessary here so I will remove it.
> >+
> >+err = brcmf_fil_iovar_data_set(ifp, "clmload", (void *)dload_buf, len);
>
> The cast is not necessary so please remove.
Will do it.
Regards,
Chung-Hsien
>
> Regards,
> Arend
>
> [1]
> http://elixir.free-electrons.com/linux/latest/source/include/linux/kernel.h#L90
Re: [PATCH v4] brcmfmac: add CLM download support
On 8/28/2017 11:25 AM, Wright Feng wrote:
From: Chung-Hsien Hsu
The firmware for brcmfmac devices includes information regarding
regulatory constraints. For certain devices this information is kept
separately in a binary form that needs to be downloaded to the device.
This patch adds support to download this so-called CLM blob file. It
uses the same naming scheme as the other firmware files with extension
of .clm_blob.
The CLM blob file is optional. If the file does not exist, the download
process will be bypassed. It will not affect the driver loading.
Signed-off-by: Chung-Hsien Hsu
---
v2: Revise commit message to describe in more detail
v3: Add error handling in brcmf_c_get_clm_name function
v4: Correct the length of dload_buf in brcmf_c_download function
---
.../net/wireless/broadcom/brcm80211/brcmfmac/bus.h | 10 ++
.../wireless/broadcom/brcm80211/brcmfmac/common.c | 161 +
.../wireless/broadcom/brcm80211/brcmfmac/core.c| 2 +
.../wireless/broadcom/brcm80211/brcmfmac/core.h| 2 +
.../broadcom/brcm80211/brcmfmac/fwil_types.h | 31
.../wireless/broadcom/brcm80211/brcmfmac/pcie.c| 19 +++
.../wireless/broadcom/brcm80211/brcmfmac/sdio.c| 19 +++
.../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 18 +++
8 files changed, 262 insertions(+)
[...]
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index 7a2b495..d09922b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
[...]
@@ -104,12 +106,139 @@ void brcmf_c_set_joinpref_default(struct brcmf_if *ifp)
brcmf_err("Set join_pref error (%d)\n", err);
}
+static int brcmf_c_download(struct brcmf_if *ifp, u16 flag,
+ struct brcmf_dload_data_le *dload_buf,
+ u32 len)
+{
+ s32 err;
+
+ flag |= (DLOAD_HANDLER_VER << DLOAD_FLAG_VER_SHIFT);
+ dload_buf->flag = cpu_to_le16(flag);
+ dload_buf->dload_type = cpu_to_le16(DL_TYPE_CLM);
+ dload_buf->len = cpu_to_le32(len);
+ dload_buf->crc = cpu_to_le32(0);
+ len = sizeof(*dload_buf) + len - 1;
+ len = len + 8 - (len % 8);
In case len is 8 this will result in 16. So better use roundup() macro
from include/linux/kernel.h [1]. However, this means
brcmf_fil_iovar_data_set() will read after the allocated buffer space.
By the way, where does the alignment requirement come from?
+
+ err = brcmf_fil_iovar_data_set(ifp, "clmload", (void *)dload_buf, len);
The cast is not necessary so please remove.
Regards,
Arend
[1]
http://elixir.free-electrons.com/linux/latest/source/include/linux/kernel.h#L90
