Date:   Wed, 3 Nov 2021 17:28:13 +0900
From:   Hector Martin <marcan@...can.st>
To:     Aditya Garg <gargaditya08@...e.com>,
        "aspriel@...il.com" <aspriel@...il.com>,
        "franky.lin@...adcom.com" <franky.lin@...adcom.com>,
        "hante.meuleman@...adcom.com" <hante.meuleman@...adcom.com>,
        "chi-hsien.lin@...ineon.com" <chi-hsien.lin@...ineon.com>,
        "wright.feng@...ineon.com" <wright.feng@...ineon.com>,
        "chung-hsien.hsu@...ineon.com" <chung-hsien.hsu@...ineon.com>
Cc:     "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        "brcm80211-dev-list.pdl@...adcom.com" 
        <brcm80211-dev-list.pdl@...adcom.com>,
        "SHA-cyfmac-dev-list@...ineon.com" <SHA-cyfmac-dev-list@...ineon.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Stan Skowronek <stan@...ellium.com>,
        Sven Peter <sven@...npeter.dev>,
        Alyssa Rosenzweig <alyssa@...enzweig.io>,
        Joey Gouly <joey.gouly@....com>
Subject: Re: [PATCH v3 1/3] brcmfmac: Add support for BCM4378 on Apple
 hardware (with their special OTP).

Hi,

I'd been meaning to rewrite this patch because it's such a mess, but 
since we're here... (CCing some relevant folks)

Overall: this patch combines a ton of unrelated random changes, many of 
which without explanation, with some completely crazy approaches. Stan 
(CC'ed) has so far refused to interact with the kernel community in any 
way whatsoever, and I do not feel comfortable using his patches without 
thorough review, including reverse engineering the changes to figure out 
what they actually do and why. We've already gone through this with some 
of his other patches, which ended up being largely rewritten or entirely 
dropped in the end.

The firmware situation with this patch is completely unacceptable. It 
seems the original intent here is to have users load the driver, have it 
print the required firmware version, and then expect users to copy 
specifically that firmware file set from macOS, and reload again. This 
is obviously not the right way to do this. We need to statically copy 
all firmware from macOS/recovery mode with a naming scheme that this 
driver can use, at initial install time, and it needs to dynamically 
select the right firmware for any given platform it is booted on.

The main issue with these machines is that there is a large set of 
required firmware variants; a few core firmware files plus many nvram 
variants for different hardware modules and device revisions. A lot of 
them are identical and can be symlinked, but we need to work out a 
naming scheme for these variations. There are several more dimensions of 
nvram selection than what we're used to on Linux.

For example, nvram is currently named in this kind of fashion:

brcmfmac43455-sdio.raspberrypi,3-model-a-plus.txt

While Apple indexes firmware using a combination of platform module, 
vendor, module version, and antenna config. The first three come from an 
OTP inside the WiFi module, while the antenna config comes from the 
Apple Device Tree and needs to be forwarded to Linux at boot time by the 
bootloader. In addition, there is also chip ID and revision.

Apple names their NVRAM using the following scheme:

C-4378__s-B1/P-honshu-X3_M-RASP_V-u__m-6.5.txt

Where 4378 is the chip, B1 is the revision, "honshu" is the platform, 
"X3" is the antenna config, "RASP" is the module, "u" is the vendor, and 
"6.5" is the module version.

Trying to translate this to something following the Linux conventions, 
we might end up with something like this:

brcmfmac4378b1-pcie.apple,honshu-RASP-u-6.5-X3.txt

However, on macOS, many of these files are copies or symlinks. For 
example, on honshu, the module version and antenna config do not matter. 
So this can simplify to:

brcmfmac4378b1-pcie.apple,honshu-RASP-u.txt

What I've been thinking is we can have the installation process detect 
those duplicates/links, and install only firmware files with the most 
generic name. Then the driver could attempt to load firmwares starting 
with more specific naming and going towards less specific. That would 
avoid having to have a giant pile of symlinks in /lib/firmware/brcm 
(there are 286 discrete files/links in Apple's firmware directory just 
for 4378...)

I would like to have a thorough discussion about how to handle the 
firmware situation, as this affects the software stack from the 
installer to the bootloader to the kernel. We also need a new devicetree 
binding for the antenna type parameter, as that needs to be set via the 
bootloader from the Apple Device Tree info.

On 03/11/2021 13.30, Aditya Garg wrote:
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index 0ee421f30aa24..8f7db434de111 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -5136,8 +5136,13 @@ brcmf_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
>   		ie_offset =  DOT11_MGMT_HDR_LEN +
>   			     DOT11_BCN_PRB_FIXED_LEN;
>   		ie_len = len - ie_offset;
> -		if (vif == cfg->p2p.bss_idx[P2PAPI_BSSCFG_PRIMARY].vif)
> +		if (vif == cfg->p2p.bss_idx[P2PAPI_BSSCFG_PRIMARY].vif) {
>   			vif = cfg->p2p.bss_idx[P2PAPI_BSSCFG_DEVICE].vif;
> +			if (vif == NULL) {
> +				bphy_err(drvr, "No p2p device available for probe response\n");
> +				return -ENODEV;
> +			}
> +		}

Why is this necessary?

> -static char brcmf_firmware_path[BRCMF_FW_ALTPATH_LEN];
> +char brcmf_firmware_path[BRCMF_FW_ALTPATH_LEN];
>   module_param_string(alternative_fw_path, brcmf_firmware_path,
> -		    BRCMF_FW_ALTPATH_LEN, 0400);
> +		    BRCMF_FW_ALTPATH_LEN, 0600);
>   MODULE_PARM_DESC(alternative_fw_path, "Alternative firmware path");
>   
> +char brcmf_mac_addr[18];
> +module_param_string(nvram_mac_addr, brcmf_mac_addr,
> +			18, 0600);
> +MODULE_PARM_DESC(nvram_mac_addr, "Set MAC address in NVRAM");

The MAC address should come from the device tree, just as it works for 
every other ethernet device on OF platforms. This already works fine for 
e.g. the wired ethernet; it should be done the same way here.

> +
> +char brcmf_otp_chip_id[BRCMF_OTP_VERSION_MAX];
> +module_param_string(otp_chip_id, brcmf_otp_chip_id, BRCMF_OTP_VERSION_MAX, 0400);
> +MODULE_PARM_DESC(otp_chip_id, "Chip ID and revision from OTP");
> +
> +char brcmf_otp_nvram_id[BRCMF_OTP_VERSION_MAX];
> +module_param_string(otp_nvram_id, brcmf_otp_nvram_id, BRCMF_OTP_VERSION_MAX, 0400);
> +MODULE_PARM_DESC(otp_chip_id, "NVRAM variant from OTP");
> +

This is crazy; the driver should read OTP and figure out what firmware 
it needs. The only piece of external information required is the antenna 
config, which should come from the device tree on ARM platforms (not 
sure how this is passed through on x86/T2 Macs, does anyone have any idea?)

>   static int brcmf_fcmode;
>   module_param_named(fcmode, brcmf_fcmode, int, 0);
>   MODULE_PARM_DESC(fcmode, "Mode of firmware signalled flow control");
> @@ -75,7 +88,6 @@ MODULE_PARM_DESC(ignore_probe_fail, "always succeed probe for debugging");
>   #endif
>   
>   static struct brcmfmac_platform_data *brcmfmac_pdata;
> -struct brcmf_mp_global_t brcmf_mp_global;
>   
>   void brcmf_c_set_joinpref_default(struct brcmf_if *ifp)
>   {
> @@ -376,22 +388,6 @@ void __brcmf_dbg(u32 level, const char *func, const char *fmt, ...)
>   }
>   #endif
>   
> -static void brcmf_mp_attach(void)
> -{
> -	/* If module param firmware path is set then this will always be used,
> -	 * if not set then if available use the platform data version. To make
> -	 * sure it gets initialized at all, always copy the module param version
> -	 */
> -	strlcpy(brcmf_mp_global.firmware_path, brcmf_firmware_path,
> -		BRCMF_FW_ALTPATH_LEN);
> -	if ((brcmfmac_pdata) && (brcmfmac_pdata->fw_alternative_path) &&
> -	    (brcmf_mp_global.firmware_path[0] == '\0')) {
> -		strlcpy(brcmf_mp_global.firmware_path,
> -			brcmfmac_pdata->fw_alternative_path,
> -			BRCMF_FW_ALTPATH_LEN);
> -	}
> -}

Why is this being removed?

> +static void brcmf_fw_set_macaddr(struct nvram_parser *nvp, const char *mac_addr)
> +{
> +	uint8_t mac[6] = { 0 };
> +	size_t len = strlen("macaddr=") + 17 + 1; /* 17 = "aa:bb:cc:dd:ee:ff" */
> +	unsigned i = 0;
> +	unsigned long res = 0;
> +	char tmp[3];
> +
> +	while(mac_addr[0] && mac_addr[1] && i < 6) {
> +		tmp[0] = mac_addr[0];
> +		tmp[1] = mac_addr[1];
> +		tmp[2] = 0;
> +		if(kstrtoul(tmp, 16, &res))
> +			break;
> +		mac[i] = res;
> +		mac_addr += 2;
> +		i ++;
> +		while(*mac_addr == ':' || *mac_addr == ' ' || *mac_addr == '-')
> +			mac_addr ++;
> +	}
> +	if(i < 6)
> +		pr_warn("invalid MAC address supplied for brcmfmac!\n");
> +
> +	memmove(&nvp->nvram[len], &nvp->nvram[0], nvp->nvram_len);
> +	nvp->nvram_len += len;
> +	sprintf(&nvp->nvram[0], "macaddr=%02x:%02x:%02x:%02x:%02x:%02x",
> +		mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
> +}

The driver already has the ability to set the MAC address; why do we 
need to set it in nvram? Just call the MAC address change function at 
driver initialization (instead of the MAC address get function, for 
these chips). The MAC address should come from the device tree, e.g. via 
eth_platform_get_mac_address(). See tg3 for a driver that already does 
this properly (and works on the M1 Mac Mini with zero changes).

> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
> index ec6fc7a150a6a..1c1d5131c3f36 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
> @@ -565,7 +565,8 @@ static s32 brcmf_p2p_deinit_discovery(struct brcmf_p2p_info *p2p)
>   
>   	/* Set the discovery state to SCAN */
>   	vif = p2p->bss_idx[P2PAPI_BSSCFG_DEVICE].vif;
> -	(void)brcmf_p2p_set_discover_state(vif->ifp, WL_P2P_DISC_ST_SCAN, 0, 0);
> +	if (vif != NULL)
> +		(void)brcmf_p2p_set_discover_state(vif->ifp, WL_P2P_DISC_ST_SCAN, 0, 0);
>   
>   	/* Disable P2P discovery in the firmware */
>   	vif = p2p->bss_idx[P2PAPI_BSSCFG_PRIMARY].vif;
> @@ -1351,6 +1352,8 @@ brcmf_p2p_gon_req_collision(struct brcmf_p2p_info *p2p, u8 *mac)
>   	 * if not (sa addr > da addr),
>   	 * this device will process gon request and drop gon req of peer.
>   	 */
> +	if(p2p->bss_idx[P2PAPI_BSSCFG_DEVICE].vif == NULL)
> +		return false;
>   	ifp = p2p->bss_idx[P2PAPI_BSSCFG_DEVICE].vif->ifp;
>   	if (memcmp(mac, ifp->mac_addr, ETH_ALEN) < 0) {
>   		brcmf_dbg(INFO, "Block transmit gon req !!!\n");
> @@ -1559,6 +1562,10 @@ static s32 brcmf_p2p_tx_action_frame(struct brcmf_p2p_info *p2p,
>   	else
>   		vif = p2p->bss_idx[P2PAPI_BSSCFG_DEVICE].vif;
>   
> +	if (vif == NULL) {
> +		bphy_err(drvr, " no P2P interface available\n");
> +		goto exit;
> +	}
>   	err = brcmf_fil_bsscfg_data_set(vif->ifp, "actframe", af_params,
>   					sizeof(*af_params));
>   	if (err) {

Are these really the only brcmfmac chips without a P2P interface? What 
does this mean for end users? What features are lost?

> +static u32
> +brcmf_pcie_reg_map(struct brcmf_pciedev_info *devinfo, u32 reg)
> +{
> +	switch(reg) {
> +	case BRCMF_PCIE_PCIE2REG_INTMASK:
> +		if(devinfo->ci->chip == BRCM_CC_4378_CHIP_ID)
> +			return BRCMF_PCIE_64_PCIE2REG_INTMASK;
> +		return reg;
> +	case BRCMF_PCIE_PCIE2REG_MAILBOXINT:
> +		if(devinfo->ci->chip == BRCM_CC_4378_CHIP_ID)
> +			return BRCMF_PCIE_64_PCIE2REG_MAILBOXINT;
> +		return reg;
> +	case BRCMF_PCIE_PCIE2REG_MAILBOXMASK:
> +		if(devinfo->ci->chip == BRCM_CC_4378_CHIP_ID)
> +			return BRCMF_PCIE_64_PCIE2REG_MAILBOXMASK;
> +		return reg;
> +	case BRCMF_PCIE_PCIE2REG_H2D_MAILBOX_0:
> +		if(devinfo->ci->chip == BRCM_CC_4378_CHIP_ID)
> +			return BRCMF_PCIE_64_PCIE2REG_H2D_MAILBOX_0;
> +		return reg;
> +	case BRCMF_PCIE_PCIE2REG_H2D_MAILBOX_1:
> +		if(devinfo->ci->chip == BRCM_CC_4378_CHIP_ID)
> +			return BRCMF_PCIE_64_PCIE2REG_H2D_MAILBOX_1;
> +		return reg;
> +	default:
> +		return reg;
> +	}
> +}

This kind of register mapping is ugly. A much better way to do this is 
to have static structures containing the list of registers for each 
given chip/variant. Then you just set it at load time and the functions 
that need those registers use those structures to find them, instead of 
constants.

> +
> +
> +

Too many blank lines

>   #define WRITECC32(devinfo, reg, value) brcmf_pcie_write_reg32(devinfo, \
>   		CHIPCREGOFFS(reg), value)
>   
> @@ -543,6 +626,7 @@ brcmf_pcie_select_core(struct brcmf_pciedev_info *devinfo, u16 coreid)
>   	core = brcmf_chip_get_core(devinfo->ci, coreid);
>   	if (core) {
>   		bar0_win = core->base;
> +
>   		pci_write_config_dword(pdev, BRCMF_PCIE_BAR0_WINDOW, bar0_win);
>   		if (pci_read_config_dword(pdev, BRCMF_PCIE_BAR0_WINDOW,
>   					  &bar0_win) == 0) {

This seems like a leftover.

> @@ -615,11 +699,129 @@ static void brcmf_pcie_reset_device(struct brcmf_pciedev_info *devinfo)
>   	}
>   }
>   
> +#define OTP_SIZE		64
> +#define OTP_CORE_ID		BCMA_CORE_GCI
> +#define OTP_CC_ADDR_4378	0x1120
> +
> +static void brcmf_pcie_process_otp_tuple(struct brcmf_pciedev_info *devinfo, u8 type, u8 size, u8 *data)
> +{
> +	char tmp[OTP_SIZE], t_chiprev[8] = "", t_module[8] = "", t_modrev[8] = "", t_vendor[8] = "", t_chip[8] = "";
> +	unsigned i, len;
> +
> +	switch(type) {
> +	case 0x15: /* system vendor OTP */
> +		if(size < 4)
> +			return;
> +		if(*(u32 *)data != 8)
> +			dev_warn(&devinfo->pdev->dev, "system vendor OTP header unexpected: %d\n", *(u32 *)data);
> +		size -= 4;
> +		data += 4;
> +		while(size) {
> +			if(data[0] == 0xFF)
> +				break;
> +			for(len=0; len<size; len++)
> +				if(data[len] == 0x00 || data[len] == ' ' || data[len] == 0xFF)
> +					break;
> +			memcpy(tmp, data, len);
> +			tmp[len] = 0;
> +			data += len;
> +			size -= len;
> +			if(size) {
> +				data ++;
> +				size --;
> +			}
> +			brcmf_dbg(PCIE, "system vendor OTP element '%s'\n", tmp);
> +
> +			if(len < 2)
> +				continue;
> +			if(tmp[1] == '=' && len < 8)
> +				switch(tmp[0]) {
> +				case 's':
> +					strcpy(t_chiprev, tmp + 2);
> +					break;
> +				case 'M':
> +					strcpy(t_module, tmp + 2);
> +					break;
> +				case 'm':
> +					strcpy(t_modrev, tmp + 2);
> +					break;
> +				case 'V':
> +					strcpy(t_vendor, tmp + 2);
> +					break;
> +				}
> +		}
> +
> +		sprintf(t_chip, (devinfo->ci->chip > 40000) ? "%05d" : "%04x", devinfo->ci->chip);
> +		dev_info(&devinfo->pdev->dev, "module revision data: chip %s, chip rev %s, module %s, module rev %s, vendor %s\n", t_chip, t_chiprev, t_module, t_modrev, t_vendor);
> +
> +		if(t_chiprev[0])
> +			sprintf(brcmf_otp_chip_id, "C-%s__s-%s", t_chip, t_chiprev);
> +		else
> +			sprintf(brcmf_otp_chip_id, "C-%s", t_chip);
> +		sprintf(brcmf_otp_nvram_id, "M-%s_V-%s__m-%s", t_module, t_vendor, t_modrev);
> +
> +		break;
> +	case 0x80: /* Broadcom CIS */
> +		if(size < 1)
> +			return;
> +		switch(data[0]) {
> +		case 0x83: /* serial number */
> +			for(i=0; i<16 && i<size-1; i++)
> +				sprintf(tmp + 2 * i, "%02x", data[i+1]);
> +			dev_info(&devinfo->pdev->dev, "module serial number: %s\n", tmp);
> +			break;
> +		}
> +		break;
> +	}
> +}

This seems to be building the Apple-formwat firmware name for users to 
use to find the right firmware. As I said, this is entirely the wrong 
approach to do it. The driver needs to use this information to find the 
right firmware itself, and the firmware copying/installation process 
needs to copy *all* of them.

> +
> +static u32 brcmf_pcie_buscore_prep_addr(const struct pci_dev *pdev, u32 addr);
> +
> +static void brcmf_pcie_read_otp(struct brcmf_pciedev_info *devinfo)
> +{
> +	u8 otp[OTP_SIZE], type, size;
> +	unsigned i;
> +	struct brcmf_core *core;
> +	u32 base;
> +
> +	if (devinfo->ci->chip == BRCM_CC_4378_CHIP_ID) {
> +		/* for whatever reason, reading OTP works only once after reset */
> +		if(brcmf_otp_chip_id[0])
> +			return;

Why? Has this been debugged to figure out why it fails and after what point?

> +
> +		core = brcmf_chip_get_core(devinfo->ci, OTP_CORE_ID);
> +		if(!core) {
> +			dev_err(&devinfo->pdev->dev, "can't find core for OTP\n");
> +			return;
> +		}
> +		base = brcmf_pcie_buscore_prep_addr(devinfo->pdev, core->base + OTP_CC_ADDR_4378);
> +
> +		for(i=0; i<OTP_SIZE; i+=2)
> +			((u16 *)otp)[i/2] = brcmf_pcie_read_reg16(devinfo, base + i);
> +
> +		i = 0;
> +		while(i < OTP_SIZE - 1) {
> +			type = otp[i];
> +			if(!type) { /* null tuple */
> +				i ++;
> +				continue;
> +			}
> +			size = otp[i + 1];
> +			i += 2;
> +			if(i + size <= OTP_SIZE)
> +				brcmf_pcie_process_otp_tuple(devinfo, type, size, otp + i);
> +			i += size;
> +		}
> +	}
> +}
> +
>   
>   static void brcmf_pcie_attach(struct brcmf_pciedev_info *devinfo)
>   {
>   	u32 config;
>   
> +	brcmf_pcie_read_otp(devinfo);
> +
>   	/* BAR1 window may not be sized properly */
>   	brcmf_pcie_select_core(devinfo, BCMA_CORE_PCIE2);
>   	brcmf_pcie_write_reg32(devinfo, BRCMF_PCIE_PCIE2REG_CONFIGADDR, 0x4e0);
> @@ -809,30 +1011,34 @@ static void brcmf_pcie_bus_console_read(struct brcmf_pciedev_info *devinfo,
>   
>   static void brcmf_pcie_intr_disable(struct brcmf_pciedev_info *devinfo)
>   {
> -	brcmf_pcie_write_reg32(devinfo, BRCMF_PCIE_PCIE2REG_MAILBOXMASK, 0);
> +	brcmf_pcie_write_reg32(devinfo, brcmf_pcie_reg_map(devinfo, BRCMF_PCIE_PCIE2REG_MAILBOXMASK), 0);
>   }

See above for why this is the wrong approach (also the other instances).
> @@ -1543,6 +1754,8 @@ brcmf_pcie_init_share_ram_info(struct brcmf_pciedev_info *devinfo,
>   	return 0;
>   }
>   
> +#define RANDOMBYTES_SIZE        264
> +#define CLEAR_SIZE              256
>   
>   static int brcmf_pcie_download_fw_nvram(struct brcmf_pciedev_info *devinfo,
>   					const struct firmware *fw, void *nvram,
> @@ -1553,15 +1766,16 @@ static int brcmf_pcie_download_fw_nvram(struct brcmf_pciedev_info *devinfo,
>   	u32 sharedram_addr_written;
>   	u32 loop_counter;
>   	int err;
> -	u32 address;
> +	u32 address, clraddr;
>   	u32 resetintr;
> +	uint8_t randb[RANDOMBYTES_SIZE];
>   
>   	brcmf_dbg(PCIE, "Halt ARM.\n");
>   	err = brcmf_pcie_enter_download_state(devinfo);
>   	if (err)
>   		return err;
>   
> -	brcmf_dbg(PCIE, "Download FW %s\n", devinfo->fw_name);
> +	brcmf_dbg(PCIE, "Download FW %s 0x%x 0x%x\n", devinfo->fw_name, (unsigned)devinfo->ci->rambase, (unsigned)fw->size);

This seems like a leftover debug change.

>   	brcmf_pcie_copy_mem_todev(devinfo, devinfo->ci->rambase,
>   				  (void *)fw->data, fw->size);
>   
> @@ -1574,20 +1788,38 @@ static int brcmf_pcie_download_fw_nvram(struct brcmf_pciedev_info *devinfo,
>   	brcmf_pcie_write_ram32(devinfo, devinfo->ci->ramsize - 4, 0);
>   
>   	if (nvram) {
> -		brcmf_dbg(PCIE, "Download NVRAM %s\n", devinfo->nvram_name);
>   		address = devinfo->ci->rambase + devinfo->ci->ramsize -
>   			  nvram_len;
> +		brcmf_dbg(PCIE, "Download NVRAM %s 0x%x 0x%x\n", devinfo->nvram_name, address, nvram_len);

Ditto

>   		brcmf_pcie_copy_mem_todev(devinfo, address, nvram, nvram_len);
>   		brcmf_fw_nvram_free(nvram);
> +
> +		address -= RANDOMBYTES_SIZE;
> +		get_random_bytes(randb, RANDOMBYTES_SIZE - 8);
> +		memcpy(randb + RANDOMBYTES_SIZE - 8, "\x00\x01\x00\x00\xde\xc0\xed\xfe", 8);
> +		brcmf_pcie_copy_mem_todev(devinfo, address, randb, RANDOMBYTES_SIZE);

Do Apple chips require some random seed placed before the nvram? And if 
so, why is this being done unconditionally for all chips? Do the other 
chips not care if we do this? Can it cause issues?

>   	} else {
>   		brcmf_dbg(PCIE, "No matching NVRAM file found %s\n",
>   			  devinfo->nvram_name);
> +		address = devinfo->ci->rambase + devinfo->ci->ramsize;
> +	}
> +
> +	memset(randb, 0, CLEAR_SIZE);
> +	clraddr = devinfo->ci->rambase + fw->size;
> +	while(clraddr < address) {
> +		u32 block = address - clraddr;
> +		if(block > CLEAR_SIZE)
> +			block = CLEAR_SIZE;
> +		if(((clraddr + block - 1) ^ clraddr) & -CLEAR_SIZE)
> +			block = (CLEAR_SIZE - clraddr) & (CLEAR_SIZE - 1);
> +		brcmf_pcie_copy_mem_todev(devinfo, clraddr, randb, block);
> +		clraddr += block;
>   	}

Looks like this is clearing memory from the end of firmware to the 
nvram/random area. Why is this necessary?

>   
>   	sharedram_addr_written = brcmf_pcie_read_ram32(devinfo,
>   						       devinfo->ci->ramsize -
>   						       4);
> -	brcmf_dbg(PCIE, "Bring ARM in running state\n");
> +	brcmf_dbg(PCIE, "Bring ARM in running state (RAM sign: 0x%08x)\n", sharedram_addr_written);
>   	err = brcmf_pcie_exit_download_state(devinfo, resetintr);
>   	if (err)
>   		return err;

Leftover debug?

> +	if(devinfo->ci->chip == BRCM_CC_4378_CHIP_ID) {
> +		brcmf_pcie_read_otp(devinfo);
> +
> +		if(!brcmf_mac_addr[0]) {
> +			dev_info(&pdev->dev, "hardware discovery complete, not starting driver\n");
> +			ret = -ENODEV;
> +			goto exit;
> +		}
> +	}

Yeah, this is crazy. The whole "load the driver once, tell the user what 
info they need, have them copy the firmware and set things up and load 
it again" dance is ridiculous. Hard NAK on doing things this way.

-- 
Hector Martin (marcan@...can.st)
Public Key: https://mrcn.st/pub

Powered by blists - more mailing lists