| Message ID | 20240818201533.89669-1-marex@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/2] wifi: brcmfmac: add support for TRX firmware download | expand |
Marek Vasut <marex@denx.de> > From: Chung-Hsien Hsu <stanley.hsu@cypress.com> > > Add support to download TRX firmware for PCIe and SDIO. > > Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com> The email address of From field (author) is different from first s-o-b.
On 8/19/24 5:08 AM, Ping-Ke Shih wrote: > Marek Vasut <marex@denx.de> >> From: Chung-Hsien Hsu <stanley.hsu@cypress.com> >> >> Add support to download TRX firmware for PCIe and SDIO. >> >> Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com> > > The email address of From field (author) is different from first s-o-b. It seems this is a result of cypress being assimilated into infineon, it seems like it is still the same person though.
Marek Vasut <marex@denx.de> writes: > From: Chung-Hsien Hsu <stanley.hsu@cypress.com> > > Add support to download TRX firmware for PCIe and SDIO. > > Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com> > Signed-off-by: Marek Vasut <marex@denx.de> # Upport to current linux-next The commit message should answer to the question 'Why?'. What's TRX firmware and why do we need it?
On August 19, 2024 4:36:05 PM Kalle Valo <kvalo@kernel.org> wrote: > Marek Vasut <marex@denx.de> writes: > >> From: Chung-Hsien Hsu <stanley.hsu@cypress.com> >> >> Add support to download TRX firmware for PCIe and SDIO. >> >> Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com> >> Signed-off-by: Marek Vasut <marex@denx.de> # Upport to current linux-next > > The commit message should answer to the question 'Why?'. What's TRX > firmware and why do we need it? I looked over the patches, but did not sit down to comment on this. The TRX firmware format allows multiple images and possibly compression. Not sure if Infineon is using all this functionality. This is probably needed for 55572 device support (patch 2/2). Turns out this device has a bootloader that the driver has to talk with and that is probably where this TRX support comes from. Not something I considered to happen for SDIO and PCIe devices. There is always a puzzle to solve ;-) Also noticed a random seed is provided to firmware. This already there for apple chips so it should be looked at whether that code can be shared. I will follow up. Regards, Arend > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 8/19/24 5:52 PM, Arend Van Spriel wrote: Hi, >>> Add support to download TRX firmware for PCIe and SDIO. >>> >>> Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com> >>> Signed-off-by: Marek Vasut <marex@denx.de> # Upport to current >>> linux-next >> >> The commit message should answer to the question 'Why?'. What's TRX >> firmware and why do we need it? > > I looked over the patches, but did not sit down to comment on this. Thanks, it is real nice that someone who is actually familiar with the hardware jumped in. I only upported the patches from infineon downstream. > The > TRX firmware format allows multiple images and possibly compression. Not > sure if Infineon is using all this functionality. This is probably > needed for 55572 device support (patch 2/2). Turns out this device has a > bootloader that the driver has to talk with and that is probably where > this TRX support comes from. Not something I considered to happen for > SDIO and PCIe devices. There is always a puzzle to solve ;-) > > Also noticed a random seed is provided to firmware. This already there > for apple chips so it should be looked at whether that code can be > shared. I will follow up. I did notice just now there is some TRX support for USB broadcom devices ?
On 8/19/24 09:17, Marek Vasut wrote: > On 8/19/24 5:52 PM, Arend Van Spriel wrote: > > Hi, > >>>> Add support to download TRX firmware for PCIe and SDIO. >>>> >>>> Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com> >>>> Signed-off-by: Marek Vasut <marex@denx.de> # Upport to current >>>> linux-next >>> >>> The commit message should answer to the question 'Why?'. What's TRX >>> firmware and why do we need it? >> >> I looked over the patches, but did not sit down to comment on this. > > Thanks, it is real nice that someone who is actually familiar with the > hardware jumped in. I only upported the patches from infineon downstream. > >> The TRX firmware format allows multiple images and possibly >> compression. Not sure if Infineon is using all this functionality. >> This is probably needed for 55572 device support (patch 2/2). Turns >> out this device has a bootloader that the driver has to talk with and >> that is probably where this TRX support comes from. Not something I >> considered to happen for SDIO and PCIe devices. There is always a >> puzzle to solve ;-) >> >> Also noticed a random seed is provided to firmware. This already there >> for apple chips so it should be looked at whether that code can be >> shared. I will follow up. > > I did notice just now there is some TRX support for USB broadcom devices ? > TRX is a format that is also used for partitioning flash devices, so there is probably some room for making a common header: ./Documentation/devicetree/bindings/mtd/partitions/brcm,trx.txt/drivers/mtd/parsers/parser_trx.c
On 8/19/24 8:09 PM, Florian Fainelli wrote: Hi, >> I did notice just now there is some TRX support for USB broadcom >> devices ? >> > > TRX is a format that is also used for partitioning flash devices, so > there is probably some room for making a common header: > > ./Documentation/devicetree/bindings/mtd/partitions/brcm,trx.txt/drivers/mtd/parsers/parser_trx.c Is that format somehow standardized ? I recall it was used on some OpenWRT routers ? It seems the TRXSE (whatever it is) is used more like a carrier for the firmware blob (chunks?) which are pushed into SRAM (?) of the CYW55572 device.
Marek Vasut <marex@denx.de> wrote: > From: Chung-Hsien Hsu <stanley.hsu@cypress.com> > > Add support to download TRX firmware for PCIe and SDIO. > > Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com> > Signed-off-by: Marek Vasut <marex@denx.de> # Upport to current linux-next Please fix the review comments, also Ping's comment that from and s-o-b needs to match. 2 patches set to Changes Requested. 13767592 [1/2] wifi: brcmfmac: add support for TRX firmware download 13767593 [2/2] wifi: brcmfmac: add support for CYW55572 PCIe chipset
On 8/22/24 10:37 AM, Kalle Valo wrote: > Marek Vasut <marex@denx.de> wrote: > >> From: Chung-Hsien Hsu <stanley.hsu@cypress.com> >> >> Add support to download TRX firmware for PCIe and SDIO. >> >> Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com> >> Signed-off-by: Marek Vasut <marex@denx.de> # Upport to current linux-next > > Please fix the review comments, also Ping's comment that from and s-o-b > needs to match. I have most of the changes addressed locally already. Regarding the SoB line, do I update the commit Author email (because that would make more sense, cypress got assimilated into infineon) or the SoB line email to the "old" cypress email address ? I am still hoping to get a bit more input on the TRX firmware handling from Arend ... or maybe there is no further feedback ?
+ Double Lo On August 22, 2024 5:24:09 PM Marek Vasut <marex@denx.de> wrote: > On 8/22/24 10:37 AM, Kalle Valo wrote: >> Marek Vasut <marex@denx.de> wrote: >> >>> From: Chung-Hsien Hsu <stanley.hsu@cypress.com> >>> >>> Add support to download TRX firmware for PCIe and SDIO. >>> >>> Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com> >>> Signed-off-by: Marek Vasut <marex@denx.de> # Upport to current linux-next >> >> Please fix the review comments, also Ping's comment that from and s-o-b >> needs to match. > > I have most of the changes addressed locally already. > > Regarding the SoB line, do I update the commit Author email (because > that would make more sense, cypress got assimilated into infineon) or > the SoB line email to the "old" cypress email address ? I would say the SoB line is leading as developer's certificate and that has the Infineon address so that seems more appropriate. > I am still hoping to get a bit more input on the TRX firmware handling > from Arend ... or maybe there is no further feedback ? Took me a while before I looked into this. Can not offer much info though. Within Broadcom code bases we only have a v1 and v2 defined for the TRX format. This patch specifies v4. The only difference seem to be the number of offsets and their meaning. Looking at the index definitions I suspect this version accommodates signed firmware images which is verified by the bootloader on the device. Hopefully Infineon can/will confirm my suspicion and explain what SE in .TRXSE stands for (security extension?). Anyways, this seem pretty specific to (some) Infineon devices. Despite its history I would keep this separate and specifically use trxse instead of trx. Regards, Arend
On 9/1/24 8:55 AM, Arend Van Spriel wrote: Hi, >> On 8/22/24 10:37 AM, Kalle Valo wrote: >>> Marek Vasut <marex@denx.de> wrote: >>> >>>> From: Chung-Hsien Hsu <stanley.hsu@cypress.com> >>>> >>>> Add support to download TRX firmware for PCIe and SDIO. >>>> >>>> Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com> >>>> Signed-off-by: Marek Vasut <marex@denx.de> # Upport to current >>>> linux-next >>> >>> Please fix the review comments, also Ping's comment that from and s-o-b >>> needs to match. >> >> I have most of the changes addressed locally already. >> >> Regarding the SoB line, do I update the commit Author email (because >> that would make more sense, cypress got assimilated into infineon) or >> the SoB line email to the "old" cypress email address ? > > I would say the SoB line is leading as developer's certificate and that > has the Infineon address so that seems more appropriate. Let's add both to be on the safe side. >> I am still hoping to get a bit more input on the TRX firmware handling >> from Arend ... or maybe there is no further feedback ? > > Took me a while before I looked into this. Can not offer much info > though. Within Broadcom code bases we only have a v1 and v2 defined for > the TRX format. This patch specifies v4. The only difference seem to be > the number of offsets and their meaning. Looking at the index > definitions I suspect this version accommodates signed firmware images > which is verified by the bootloader on the device. > > Hopefully Infineon can/will confirm my suspicion and explain what SE > in .TRXSE stands for (security extension?). Anyways, this seem pretty > specific to (some) Infineon devices. Despite its history I would keep > this separate and specifically use trxse instead of trx. OK
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c index ce482a3877e90..058a742d17eda 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c @@ -42,6 +42,7 @@ #include "chip.h" #include "core.h" #include "common.h" +#include "trxhdr.h" enum brcmf_pcie_state { @@ -1684,6 +1685,8 @@ static int brcmf_pcie_download_fw_nvram(struct brcmf_pciedev_info *devinfo, u32 nvram_len) { struct brcmf_bus *bus = dev_get_drvdata(&devinfo->pdev->dev); + struct trx_header_le *trx = (struct trx_header_le *)fw->data; + u32 fw_size; u32 sharedram_addr; u32 sharedram_addr_written; u32 loop_counter; @@ -1697,8 +1700,13 @@ static int brcmf_pcie_download_fw_nvram(struct brcmf_pciedev_info *devinfo, return err; brcmf_dbg(PCIE, "Download FW %s\n", devinfo->fw_name); - memcpy_toio(devinfo->tcm + devinfo->ci->rambase, - (void *)fw->data, fw->size); + address = devinfo->ci->rambase; + fw_size = fw->size; + if (trx->magic == cpu_to_le32(TRX_MAGIC)) { + address -= sizeof(struct trx_header_le); + fw_size = le32_to_cpu(trx->len); + } + memcpy_toio(devinfo->tcm + address, (void *)fw->data, fw_size); resetintr = get_unaligned_le32(fw->data); release_firmware(fw); diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c index 1461dc453ac22..08881e366cae2 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c @@ -35,6 +35,7 @@ #include "core.h" #include "common.h" #include "bcdc.h" +#include "trxhdr.h" #define DCMD_RESP_TIMEOUT msecs_to_jiffies(2500) #define CTL_DONE_TIMEOUT msecs_to_jiffies(2500) @@ -3346,17 +3347,26 @@ brcmf_sdio_verifymemory(struct brcmf_sdio_dev *sdiodev, u32 ram_addr, static int brcmf_sdio_download_code_file(struct brcmf_sdio *bus, const struct firmware *fw) { + struct trx_header_le *trx = (struct trx_header_le *)fw->data; + u32 fw_size; + u32 address; int err; brcmf_dbg(TRACE, "Enter\n"); - err = brcmf_sdiod_ramrw(bus->sdiodev, true, bus->ci->rambase, - (u8 *)fw->data, fw->size); + address = bus->ci->rambase; + fw_size = fw->size; + if (trx->magic == cpu_to_le32(TRX_MAGIC)) { + address -= sizeof(struct trx_header_le); + fw_size = le32_to_cpu(trx->len); + } + err = brcmf_sdiod_ramrw(bus->sdiodev, true, address, + (u8 *)fw->data, fw_size); if (err) brcmf_err("error %d on writing %d membytes at 0x%08x\n", - err, (int)fw->size, bus->ci->rambase); - else if (!brcmf_sdio_verifymemory(bus->sdiodev, bus->ci->rambase, - (u8 *)fw->data, fw->size)) + err, (int)fw_size, address); + else if (!brcmf_sdio_verifymemory(bus->sdiodev, address, + (u8 *)fw->data, fw_size)) err = -EIO; return err; diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/trxhdr.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/trxhdr.h new file mode 100644 index 0000000000000..0411c7c7ffb99 --- /dev/null +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/trxhdr.h @@ -0,0 +1,38 @@ +/* SPDX-License-Identifier: ISC */ +/* Copyright (c) 2020 Cypress Semiconductor Corporation */ + +#ifndef BRCMFMAC_TRXHDR_H +#define BRCMFMAC_TRXHDR_H + +/* Bootloader makes special use of trx header "offsets" array */ +enum { + TRX_OFFSET_SIGN_INFO_IDX = 0, + TRX_OFFSET_DATA_FOR_SIGN1_IDX = 1, + TRX_OFFSET_DATA_FOR_SIGN2_IDX = 2, + TRX_OFFSET_ROOT_MODULUS_IDX = 3, + TRX_OFFSET_ROOT_EXPONENT_IDX = 67, + TRX_OFFSET_CONT_MODULUS_IDX = 68, + TRX_OFFSET_CONT_EXPONENT_IDX = 132, + TRX_OFFSET_HASH_FW_IDX = 133, + TRX_OFFSET_FW_LEN_IDX = 149, + TRX_OFFSET_TR_RST_IDX = 150, + TRX_OFFSET_FW_VER_FOR_ANTIROOLBACK_IDX = 151, + TRX_OFFSET_IV_IDX = 152, + TRX_OFFSET_NONCE_IDX = 160, + TRX_OFFSET_SIGN_INFO2_IDX = 168, + TRX_OFFSET_MAX_IDX +}; + +#define TRX_MAGIC 0x30524448 /* "HDR0" */ +#define TRX_VERSION 4 /* Version 4 */ +#define TRX_MAX_OFFSET TRX_OFFSET_MAX_IDX /* Max number of file offsets */ + +struct trx_header_le { + __le32 magic; /* "HDR0" */ + __le32 len; /* Length of file including header */ + __le32 crc32; /* CRC from flag_version to end of file */ + __le32 flag_version; /* 0:15 flags, 16:31 version */ + __le32 offsets[TRX_MAX_OFFSET]; /* Offsets of partitions */ +}; + +#endif /* BRCMFMAC_TRXHDR_H */