[PATCH 00/10] brcmfmac: device support and fixes

[PATCH 00/10] brcmfmac: device support and fixes

[Arend van Spriel]

This series include some fixes related to SDIO suspend and resume,
wiphy band info and changes in regulatory settings. Furthermore,
support for BCM4324 SDIO and BCM4358 PCIe is added. Finally,
some patches from Hante that should enable support of PCIe devices
on router platforms.

This series is intended for v4.2 and applies to the master branch
of the wireless-drivers-next repository.

Arend van Spriel (8):
  brcmfmac: use static superset of channels for wiphy bands
  brcmfmac: update wiphy band information upon updating regulatory
    domain
  brcmfmac: add description for feature flags
  brcmfmac: make scheduled scan support conditional
  brcmfmac: add support for BCM4324 rev B5 chipset
  brcmfmac: process interrupt regardless sdiod state
  brcmfmac: fix sdio suspend and resume
  brcmfmac: add support for BCM4358 PCIe device

Hante Meuleman (2):
  brcmfmac: add additional 43602 pcie device id.
  brcmfmac: Add support for multiple PCIE devices in nvram.

 drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c   |  18 +-
 drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c | 202 ++++++++++++---------
 drivers/net/wireless/brcm80211/brcmfmac/chip.c     |   1 +
 drivers/net/wireless/brcm80211/brcmfmac/feature.c  |   1 +
 drivers/net/wireless/brcm80211/brcmfmac/feature.h  |   4 +
 drivers/net/wireless/brcm80211/brcmfmac/firmware.c | 189 ++++++++++++++++++-
 drivers/net/wireless/brcm80211/brcmfmac/firmware.h |   6 +
 drivers/net/wireless/brcm80211/brcmfmac/pcie.c     |  25 ++-
 drivers/net/wireless/brcm80211/brcmfmac/sdio.c     |  11 +-
 .../net/wireless/brcm80211/include/brcm_hw_ids.h   |   3 +
 10 files changed, 348 insertions(+), 112 deletions(-)

-- 
1.9.1

[PATCH 01/10] brcmfmac: use static superset of channels for wiphy bands

[Arend van Spriel]

The driver was constructing a list of channels per wiphy band
by querying the device. This list is not what the hardware is
able to do as it is already filtered by the country setting in
the device. As user-space may change the country this would
require updating the channel list which is not recommended [1].
This patch introduces a superset of channels. The individual
channels are disabled appropriately by querying the device.

[1] http://mid.gmane.org/1426706320.3001.21.camel@sipsolutions.net

Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
Reviewed-by: Daniel (Deognyoun) Kim <dekim@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c | 193 +++++++++++----------
 1 file changed, 106 insertions(+), 87 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
index 8a15ebb..76dff0e 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
@@ -129,13 +129,47 @@ static struct ieee80211_rate __wl_rates[] = {
 	RATETAB_ENT(BRCM_RATE_54M, 0),
 };
 
-#define wl_a_rates		(__wl_rates + 4)
-#define wl_a_rates_size	8
 #define wl_g_rates		(__wl_rates + 0)
-#define wl_g_rates_size	12
+#define wl_g_rates_size		ARRAY_SIZE(__wl_rates)
+#define wl_a_rates		(__wl_rates + 4)
+#define wl_a_rates_size		(wl_g_rates_size - 4)
+
+#define CHAN2G(_channel, _freq) {				\
+	.band			= IEEE80211_BAND_2GHZ,		\
+	.center_freq		= (_freq),			\
+	.hw_value		= (_channel),			\
+	.flags			= IEEE80211_CHAN_DISABLED,	\
+	.max_antenna_gain	= 0,				\
+	.max_power		= 30,				\
+}
+
+#define CHAN5G(_channel) {					\
+	.band			= IEEE80211_BAND_5GHZ,		\
+	.center_freq		= 5000 + (5 * (_channel)),	\
+	.hw_value		= (_channel),			\
+	.flags			= IEEE80211_CHAN_DISABLED,	\
+	.max_antenna_gain	= 0,				\
+	.max_power		= 30,				\
+}
+
+static struct ieee80211_channel __wl_2ghz_channels[] = {
+	CHAN2G(1, 2412), CHAN2G(2, 2417), CHAN2G(3, 2422), CHAN2G(4, 2427),
+	CHAN2G(5, 2432), CHAN2G(6, 2437), CHAN2G(7, 2442), CHAN2G(8, 2447),
+	CHAN2G(9, 2452), CHAN2G(10, 2457), CHAN2G(11, 2462), CHAN2G(12, 2467),
+	CHAN2G(13, 2472), CHAN2G(14, 2484)
+};
+
+static struct ieee80211_channel __wl_5ghz_channels[] = {
+	CHAN5G(34), CHAN5G(36), CHAN5G(38), CHAN5G(40), CHAN5G(42),
+	CHAN5G(44), CHAN5G(46), CHAN5G(48), CHAN5G(52), CHAN5G(56),
+	CHAN5G(60), CHAN5G(64), CHAN5G(100), CHAN5G(104), CHAN5G(108),
+	CHAN5G(112), CHAN5G(116), CHAN5G(120), CHAN5G(124), CHAN5G(128),
+	CHAN5G(132), CHAN5G(136), CHAN5G(140), CHAN5G(144), CHAN5G(149),
+	CHAN5G(153), CHAN5G(157), CHAN5G(161), CHAN5G(165)
+};
 
 /* Band templates duplicated per wiphy. The channel info
- * is filled in after querying the device.
+ * above is added to the band during setup.
  */
 static const struct ieee80211_supported_band __wl_band_2ghz = {
 	.band = IEEE80211_BAND_2GHZ,
@@ -143,7 +177,7 @@ static const struct ieee80211_supported_band __wl_band_2ghz = {
 	.n_bitrates = wl_g_rates_size,
 };
 
-static const struct ieee80211_supported_band __wl_band_5ghz_a = {
+static const struct ieee80211_supported_band __wl_band_5ghz = {
 	.band = IEEE80211_BAND_5GHZ,
 	.bitrates = wl_a_rates,
 	.n_bitrates = wl_a_rates_size,
@@ -5253,40 +5287,6 @@ dongle_scantime_out:
 	return err;
 }
 
-/* Filter the list of channels received from firmware counting only
- * the 20MHz channels. The wiphy band data only needs those which get
- * flagged to indicate if they can take part in higher bandwidth.
- */
-static void brcmf_count_20mhz_channels(struct brcmf_cfg80211_info *cfg,
-				       struct brcmf_chanspec_list *chlist,
-				       u32 chcnt[])
-{
-	u32 total = le32_to_cpu(chlist->count);
-	struct brcmu_chan ch;
-	int i;
-
-	for (i = 0; i < total; i++) {
-		ch.chspec = (u16)le32_to_cpu(chlist->element[i]);
-		cfg->d11inf.decchspec(&ch);
-
-		/* Firmware gives a ordered list. We skip non-20MHz
-		 * channels is 2G. For 5G we can abort upon reaching
-		 * a non-20MHz channel in the list.
-		 */
-		if (ch.bw != BRCMU_CHAN_BW_20) {
-			if (ch.band == BRCMU_CHAN_BAND_5G)
-				break;
-			else
-				continue;
-		}
-
-		if (ch.band == BRCMU_CHAN_BAND_2G)
-			chcnt[0] += 1;
-		else if (ch.band == BRCMU_CHAN_BAND_5G)
-			chcnt[1] += 1;
-	}
-}
-
 static void brcmf_update_bw40_channel_flag(struct ieee80211_channel *channel,
 					   struct brcmu_chan *ch)
 {
@@ -5322,7 +5322,6 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
 	u32 i, j;
 	u32 total;
 	u32 chaninfo;
-	u32 chcnt[2] = { 0, 0 };
 	u32 index;
 
 	pbuf = kzalloc(BRCMF_DCMD_MEDLEN, GFP_KERNEL);
@@ -5339,42 +5338,15 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
 		goto fail_pbuf;
 	}
 
-	brcmf_count_20mhz_channels(cfg, list, chcnt);
 	wiphy = cfg_to_wiphy(cfg);
-	if (chcnt[0]) {
-		band = kmemdup(&__wl_band_2ghz, sizeof(__wl_band_2ghz),
-			       GFP_KERNEL);
-		if (band == NULL) {
-			err = -ENOMEM;
-			goto fail_pbuf;
-		}
-		band->channels = kcalloc(chcnt[0], sizeof(*channel),
-					 GFP_KERNEL);
-		if (band->channels == NULL) {
-			kfree(band);
-			err = -ENOMEM;
-			goto fail_pbuf;
-		}
-		band->n_channels = 0;
-		wiphy->bands[IEEE80211_BAND_2GHZ] = band;
-	}
-	if (chcnt[1]) {
-		band = kmemdup(&__wl_band_5ghz_a, sizeof(__wl_band_5ghz_a),
-			       GFP_KERNEL);
-		if (band == NULL) {
-			err = -ENOMEM;
-			goto fail_band2g;
-		}
-		band->channels = kcalloc(chcnt[1], sizeof(*channel),
-					 GFP_KERNEL);
-		if (band->channels == NULL) {
-			kfree(band);
-			err = -ENOMEM;
-			goto fail_band2g;
-		}
-		band->n_channels = 0;
-		wiphy->bands[IEEE80211_BAND_5GHZ] = band;
-	}
+	band = wiphy->bands[IEEE80211_BAND_2GHZ];
+	if (band)
+		for (i = 0; i < band->n_channels; i++)
+			band->channels[i].flags = IEEE80211_CHAN_DISABLED;
+	band = wiphy->bands[IEEE80211_BAND_5GHZ];
+	if (band)
+		for (i = 0; i < band->n_channels; i++)
+			band->channels[i].flags = IEEE80211_CHAN_DISABLED;
 
 	total = le32_to_cpu(list->count);
 	for (i = 0; i < total; i++) {
@@ -5389,6 +5361,8 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
 			brcmf_err("Invalid channel Spec. 0x%x.\n", ch.chspec);
 			continue;
 		}
+		if (!band)
+			continue;
 		if (!(bw_cap[band->band] & WLC_BW_40MHZ_BIT) &&
 		    ch.bw == BRCMU_CHAN_BW_40)
 			continue;
@@ -5416,9 +5390,9 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
 		} else if (ch.bw == BRCMU_CHAN_BW_40) {
 			brcmf_update_bw40_channel_flag(&channel[index], &ch);
 		} else {
-			/* disable other bandwidths for now as mentioned
-			 * order assure they are enabled for subsequent
-			 * chanspecs.
+			/* enable the channel and disable other bandwidths
+			 * for now as mentioned order assure they are enabled
+			 * for subsequent chanspecs.
 			 */
 			channel[index].flags = IEEE80211_CHAN_NO_HT40 |
 					       IEEE80211_CHAN_NO_80MHZ;
@@ -5437,16 +5411,8 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
 						IEEE80211_CHAN_NO_IR;
 			}
 		}
-		if (index == band->n_channels)
-			band->n_channels++;
 	}
-	kfree(pbuf);
-	return 0;
 
-fail_band2g:
-	kfree(wiphy->bands[IEEE80211_BAND_2GHZ]->channels);
-	kfree(wiphy->bands[IEEE80211_BAND_2GHZ]);
-	wiphy->bands[IEEE80211_BAND_2GHZ] = NULL;
 fail_pbuf:
 	kfree(pbuf);
 	return err;
@@ -5779,7 +5745,12 @@ static void brcmf_wiphy_wowl_params(struct wiphy *wiphy)
 
 static int brcmf_setup_wiphy(struct wiphy *wiphy, struct brcmf_if *ifp)
 {
+	struct ieee80211_supported_band *band;
 	struct ieee80211_iface_combination ifc_combo;
+	__le32 bandlist[3];
+	u32 n_bands;
+	int err, i;
+
 	wiphy->max_scan_ssids = WL_NUM_SCAN_MAX;
 	wiphy->max_scan_ie_len = BRCMF_SCAN_IE_LEN_MAX;
 	wiphy->max_num_pmkids = WL_NUM_PMKIDS_MAX;
@@ -5821,7 +5792,52 @@ static int brcmf_setup_wiphy(struct wiphy *wiphy, struct brcmf_if *ifp)
 	if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_WOWL))
 		brcmf_wiphy_wowl_params(wiphy);
 
-	return brcmf_setup_wiphybands(wiphy);
+	err = brcmf_fil_cmd_data_get(ifp, BRCMF_C_GET_BANDLIST, &bandlist,
+				     sizeof(bandlist));
+	if (err) {
+		brcmf_err("could not obtain band info: err=%d\n", err);
+		return err;
+	}
+	/* first entry in bandlist is number of bands */
+	n_bands = le32_to_cpu(bandlist[0]);
+	for (i = 1; i <= n_bands && i < ARRAY_SIZE(bandlist); i++) {
+		if (bandlist[i] == cpu_to_le32(WLC_BAND_2G)) {
+			band = kmemdup(&__wl_band_2ghz, sizeof(__wl_band_2ghz),
+				       GFP_KERNEL);
+			if (!band)
+				return -ENOMEM;
+
+			band->channels = kmemdup(&__wl_2ghz_channels,
+						 sizeof(__wl_2ghz_channels),
+						 GFP_KERNEL);
+			if (!band->channels) {
+				kfree(band);
+				return -ENOMEM;
+			}
+
+			band->n_channels = ARRAY_SIZE(__wl_2ghz_channels);
+			wiphy->bands[IEEE80211_BAND_2GHZ] = band;
+		}
+		if (bandlist[i] == cpu_to_le32(WLC_BAND_5G)) {
+			band = kmemdup(&__wl_band_5ghz, sizeof(__wl_band_5ghz),
+				       GFP_KERNEL);
+			if (!band)
+				return -ENOMEM;
+
+			band->channels = kmemdup(&__wl_5ghz_channels,
+						 sizeof(__wl_5ghz_channels),
+						 GFP_KERNEL);
+			if (!band->channels) {
+				kfree(band);
+				return -ENOMEM;
+			}
+
+			band->n_channels = ARRAY_SIZE(__wl_5ghz_channels);
+			wiphy->bands[IEEE80211_BAND_5GHZ] = band;
+		}
+	}
+	err = brcmf_setup_wiphybands(wiphy);
+	return err;
 }
 
 static s32 brcmf_config_dongle(struct brcmf_cfg80211_info *cfg)
@@ -6012,6 +6028,9 @@ static void brcmf_cfg80211_reg_notifier(struct wiphy *wiphy,
 
 static void brcmf_free_wiphy(struct wiphy *wiphy)
 {
+	if (!wiphy)
+		return;
+
 	kfree(wiphy->iface_combinations);
 	if (wiphy->bands[IEEE80211_BAND_2GHZ]) {
 		kfree(wiphy->bands[IEEE80211_BAND_2GHZ]->channels);
-- 
1.9.1

Re: [01/10] brcmfmac: use static superset of channels for wiphy bands

[Kalle Valo]

> The driver was constructing a list of channels per wiphy band
> by querying the device. This list is not what the hardware is
> able to do as it is already filtered by the country setting in
> the device. As user-space may change the country this would
> require updating the channel list which is not recommended [1].
> This patch introduces a superset of channels. The individual
> channels are disabled appropriately by querying the device.
> 
> [1] http://mid.gmane.org/1426706320.3001.21.camel@sipsolutions.net
> 
> Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
> Reviewed-by: Daniel (Deognyoun) Kim <dekim@broadcom.com>
> Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
> Signed-off-by: Arend van Spriel <arend@broadcom.com>

Thanks, 10 patches applied to wireless-drivers-next.git:

58de92d2f95e brcmfmac: use static superset of channels for wiphy bands
8afe0ece7bfd brcmfmac: update wiphy band information upon updating regulatory domain
28d96e5ee513 brcmfmac: add description for feature flags
7a7a87dc947c brcmfmac: make scheduled scan support conditional
ba5e8665b805 brcmfmac: add support for BCM4324 rev B5 chipset
8b36e98895e1 brcmfmac: process interrupt regardless sdiod state
bdf1340cf20f brcmfmac: fix sdio suspend and resume
67f3b6a3d1ca brcmfmac: add support for BCM4358 PCIe device
27aace2dcc9a brcmfmac: add additional 43602 pcie device id.
c43655345234 brcmfmac: Add support for multiple PCIE devices in nvram.

Kalle Valo

[PATCH 02/10] brcmfmac: update wiphy band information upon updating regulatory domain

[Arend van Spriel]

When change the country code the available channels may change. So
the wiphy bands should be updated accordingly.

Reviewed-by: Daniel (Deognyoun) Kim <dekim@broadcom.com>
Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
index 76dff0e..84d9ab1 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
@@ -6023,7 +6023,11 @@ static void brcmf_cfg80211_reg_notifier(struct wiphy *wiphy,
 	memset(&ccreq, 0, sizeof(ccreq));
 	ccreq.rev = cpu_to_le32(-1);
 	memcpy(ccreq.ccode, req->alpha2, sizeof(req->alpha2));
-	brcmf_fil_iovar_data_set(ifp, "country", &ccreq, sizeof(ccreq));
+	if (brcmf_fil_iovar_data_set(ifp, "country", &ccreq, sizeof(ccreq))) {
+		brcmf_err("firmware rejected country setting\n");
+		return;
+	}
+	brcmf_setup_wiphybands(wiphy);
 }
 
 static void brcmf_free_wiphy(struct wiphy *wiphy)
-- 
1.9.1

[PATCH 03/10] brcmfmac: add description for feature flags

[Arend van Spriel]

Some feature flags were not described in the header file. Adding
the description.

Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/feature.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/feature.h b/drivers/net/wireless/brcm80211/brcmfmac/feature.h
index f5832e0..4bf40ff 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/feature.h
+++ b/drivers/net/wireless/brcm80211/brcmfmac/feature.h
@@ -19,7 +19,9 @@
 /*
  * Features:
  *
+ * MBSS: multiple BSSID support (eg. guest network in AP mode).
  * MCHAN: multi-channel for concurrent P2P.
+ * WOWL: Wake-On-WLAN.
  */
 #define BRCMF_FEAT_LIST \
 	BRCMF_FEAT_DEF(MBSS) \
-- 
1.9.1

[PATCH 04/10] brcmfmac: make scheduled scan support conditional

[Arend van Spriel]

The scheduled scan support depends on firmware supporting the PNO
feature. This feature is optional so add a feature flag for this
in the driver and announce scheduled scan support accordingly.

Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c | 3 ++-
 drivers/net/wireless/brcm80211/brcmfmac/feature.c  | 1 +
 drivers/net/wireless/brcm80211/brcmfmac/feature.h  | 2 ++
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
index 84d9ab1..6fe2b75 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
@@ -5783,7 +5783,8 @@ static int brcmf_setup_wiphy(struct wiphy *wiphy, struct brcmf_if *ifp)
 		wiphy->flags |= WIPHY_FLAG_SUPPORTS_FW_ROAM;
 	wiphy->mgmt_stypes = brcmf_txrx_stypes;
 	wiphy->max_remain_on_channel_duration = 5000;
-	brcmf_wiphy_pno_params(wiphy);
+	if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_PNO))
+		brcmf_wiphy_pno_params(wiphy);
 
 	/* vendor commands/events support */
 	wiphy->vendor_commands = brcmf_vendor_cmds;
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/feature.c b/drivers/net/wireless/brcm80211/brcmfmac/feature.c
index 7748a1c..2c5fad3 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/feature.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/feature.c
@@ -124,6 +124,7 @@ void brcmf_feat_attach(struct brcmf_pub *drvr)
 	struct brcmf_if *ifp = drvr->iflist[0];
 
 	brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_MCHAN, "mchan");
+	brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_PNO, "pfn");
 	if (drvr->bus_if->wowl_supported)
 		brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_WOWL, "wowl");
 	if (drvr->bus_if->chip != BRCM_CC_43362_CHIP_ID)
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/feature.h b/drivers/net/wireless/brcm80211/brcmfmac/feature.h
index 4bf40ff..5469625 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/feature.h
+++ b/drivers/net/wireless/brcm80211/brcmfmac/feature.h
@@ -21,11 +21,13 @@
  *
  * MBSS: multiple BSSID support (eg. guest network in AP mode).
  * MCHAN: multi-channel for concurrent P2P.
+ * PNO: preferred network offload.
  * WOWL: Wake-On-WLAN.
  */
 #define BRCMF_FEAT_LIST \
 	BRCMF_FEAT_DEF(MBSS) \
 	BRCMF_FEAT_DEF(MCHAN) \
+	BRCMF_FEAT_DEF(PNO) \
 	BRCMF_FEAT_DEF(WOWL)
 /*
  * Quirks:
-- 
1.9.1

[PATCH 05/10] brcmfmac: add support for BCM4324 rev B5 chipset

[Arend van Spriel]

This patch adds support for the BCM4324 B5 revision. This device
is similar to BCM43241 from driver and firmware perspective. It
is known to be used in Lenovo Thinkpad Tablet devices.

Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/sdio.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
index ab0c898..cbdda54 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
@@ -601,6 +601,8 @@ static const struct sdiod_drive_str sdiod_drvstr_tab2_3v3[] = {
 #define BCM43241B0_NVRAM_NAME		"brcm/brcmfmac43241b0-sdio.txt"
 #define BCM43241B4_FIRMWARE_NAME	"brcm/brcmfmac43241b4-sdio.bin"
 #define BCM43241B4_NVRAM_NAME		"brcm/brcmfmac43241b4-sdio.txt"
+#define BCM43241B5_FIRMWARE_NAME	"brcm/brcmfmac43241b5-sdio.bin"
+#define BCM43241B5_NVRAM_NAME		"brcm/brcmfmac43241b5-sdio.txt"
 #define BCM4329_FIRMWARE_NAME		"brcm/brcmfmac4329-sdio.bin"
 #define BCM4329_NVRAM_NAME		"brcm/brcmfmac4329-sdio.txt"
 #define BCM4330_FIRMWARE_NAME		"brcm/brcmfmac4330-sdio.bin"
@@ -628,6 +630,8 @@ MODULE_FIRMWARE(BCM43241B0_FIRMWARE_NAME);
 MODULE_FIRMWARE(BCM43241B0_NVRAM_NAME);
 MODULE_FIRMWARE(BCM43241B4_FIRMWARE_NAME);
 MODULE_FIRMWARE(BCM43241B4_NVRAM_NAME);
+MODULE_FIRMWARE(BCM43241B5_FIRMWARE_NAME);
+MODULE_FIRMWARE(BCM43241B5_NVRAM_NAME);
 MODULE_FIRMWARE(BCM4329_FIRMWARE_NAME);
 MODULE_FIRMWARE(BCM4329_NVRAM_NAME);
 MODULE_FIRMWARE(BCM4330_FIRMWARE_NAME);
@@ -667,7 +671,8 @@ enum brcmf_firmware_type {
 static const struct brcmf_firmware_names brcmf_fwname_data[] = {
 	{ BRCM_CC_43143_CHIP_ID, 0xFFFFFFFF, BRCMF_FIRMWARE_NVRAM(BCM43143) },
 	{ BRCM_CC_43241_CHIP_ID, 0x0000001F, BRCMF_FIRMWARE_NVRAM(BCM43241B0) },
-	{ BRCM_CC_43241_CHIP_ID, 0xFFFFFFE0, BRCMF_FIRMWARE_NVRAM(BCM43241B4) },
+	{ BRCM_CC_43241_CHIP_ID, 0x00000020, BRCMF_FIRMWARE_NVRAM(BCM43241B4) },
+	{ BRCM_CC_43241_CHIP_ID, 0xFFFFFFC0, BRCMF_FIRMWARE_NVRAM(BCM43241B5) },
 	{ BRCM_CC_4329_CHIP_ID, 0xFFFFFFFF, BRCMF_FIRMWARE_NVRAM(BCM4329) },
 	{ BRCM_CC_4330_CHIP_ID, 0xFFFFFFFF, BRCMF_FIRMWARE_NVRAM(BCM4330) },
 	{ BRCM_CC_4334_CHIP_ID, 0xFFFFFFFF, BRCMF_FIRMWARE_NVRAM(BCM4334) },
-- 
1.9.1

[PATCH 06/10] brcmfmac: process interrupt regardless sdiod state

[Arend van Spriel]

When the sdio bus state is not ready to process we abort the
interrupt service routine. This is not wanted as it keeps the
interrupt source active. Better clear the interrupt source.

Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Reviewed-by: Franky (Zhenhui) Lin <frankyl@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/sdio.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
index cbdda54..bf7a8b1 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
@@ -3555,10 +3555,6 @@ void brcmf_sdio_isr(struct brcmf_sdio *bus)
 		return;
 	}
 
-	if (bus->sdiodev->state != BRCMF_SDIOD_DATA) {
-		brcmf_err("bus is down. we have nothing to do\n");
-		return;
-	}
 	/* Count the interrupt call */
 	bus->sdcnt.intrcount++;
 	if (in_interrupt())
-- 
1.9.1

[PATCH 07/10] brcmfmac: fix sdio suspend and resume

[Arend van Spriel]

commit 330b4e4be937 ("brcmfmac: Add wowl support for SDIO devices.")
changed the behaviour by removing the MMC_PM_KEEP_POWER flag for
non-wowl scenario, which needs to be restored. Another necessary
change is to mark the card as being non-removable. With this in place
the suspend resume test passes successfully doing:

 # echo devices > /sys/power/pm_test
 # echo mem > /sys/power/state

Note that power may still be switched off when system is going
in S3 state.

Reported-by: Fu, Zhonghui <<zhonghui.fu@linux.intel.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Reviewed-by: Franky (Zhenhui) Lin <frankyl@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
index 9b508bd..8a69544 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
@@ -1011,6 +1011,14 @@ static int brcmf_sdiod_remove(struct brcmf_sdio_dev *sdiodev)
 	return 0;
 }
 
+static void brcmf_sdiod_host_fixup(struct mmc_host *host)
+{
+	/* runtime-pm powers off the device */
+	pm_runtime_forbid(host->parent);
+	/* avoid removal detection upon resume */
+	host->caps |= MMC_CAP_NONREMOVABLE;
+}
+
 static int brcmf_sdiod_probe(struct brcmf_sdio_dev *sdiodev)
 {
 	struct sdio_func *func;
@@ -1076,7 +1084,7 @@ static int brcmf_sdiod_probe(struct brcmf_sdio_dev *sdiodev)
 		ret = -ENODEV;
 		goto out;
 	}
-	pm_runtime_forbid(host->parent);
+	brcmf_sdiod_host_fixup(host);
 out:
 	if (ret)
 		brcmf_sdiod_remove(sdiodev);
@@ -1246,15 +1254,15 @@ static int brcmf_ops_sdio_suspend(struct device *dev)
 	brcmf_sdiod_freezer_on(sdiodev);
 	brcmf_sdio_wd_timer(sdiodev->bus, 0);
 
+	sdio_flags = MMC_PM_KEEP_POWER;
 	if (sdiodev->wowl_enabled) {
-		sdio_flags = MMC_PM_KEEP_POWER;
 		if (sdiodev->pdata->oob_irq_supported)
 			enable_irq_wake(sdiodev->pdata->oob_irq_nr);
 		else
-			sdio_flags = MMC_PM_WAKE_SDIO_IRQ;
-		if (sdio_set_host_pm_flags(sdiodev->func[1], sdio_flags))
-			brcmf_err("Failed to set pm_flags %x\n", sdio_flags);
+			sdio_flags |= MMC_PM_WAKE_SDIO_IRQ;
 	}
+	if (sdio_set_host_pm_flags(sdiodev->func[1], sdio_flags))
+		brcmf_err("Failed to set pm_flags %x\n", sdio_flags);
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH 07/10] brcmfmac: fix sdio suspend and resume

[Ulf Hansson]

On 14 April 2015 at 20:10, Arend van Spriel <arend@broadcom.com> wrote:
> commit 330b4e4be937 ("brcmfmac: Add wowl support for SDIO devices.")
> changed the behaviour by removing the MMC_PM_KEEP_POWER flag for
> non-wowl scenario, which needs to be restored. Another necessary
> change is to mark the card as being non-removable. With this in place
> the suspend resume test passes successfully doing:
>
>  # echo devices > /sys/power/pm_test
>  # echo mem > /sys/power/state
>
> Note that power may still be switched off when system is going
> in S3 state.
>
> Reported-by: Fu, Zhonghui <<zhonghui.fu@linux.intel.com>
> Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
> Reviewed-by: Franky (Zhenhui) Lin <frankyl@broadcom.com>
> Signed-off-by: Arend van Spriel <arend@broadcom.com>
> ---
>  drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
> index 9b508bd..8a69544 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
> @@ -1011,6 +1011,14 @@ static int brcmf_sdiod_remove(struct brcmf_sdio_dev *sdiodev)
>         return 0;
>  }
>
> +static void brcmf_sdiod_host_fixup(struct mmc_host *host)
> +{
> +       /* runtime-pm powers off the device */
> +       pm_runtime_forbid(host->parent);

That you need this, clearly shows that something is broken in the mmc
core/host layer.

Could you elaborate a bit on what configuration you are using. Like
what mmc host, which SDIO bus speed mode.

And have you tested different configurations? Like what happens if you
use a different SDIO bus speed mode?

> +       /* avoid removal detection upon resume */
> +       host->caps |= MMC_CAP_NONREMOVABLE;
> +}
> +
>  static int brcmf_sdiod_probe(struct brcmf_sdio_dev *sdiodev)
>  {
>         struct sdio_func *func;
> @@ -1076,7 +1084,7 @@ static int brcmf_sdiod_probe(struct brcmf_sdio_dev *sdiodev)
>                 ret = -ENODEV;
>                 goto out;
>         }
> -       pm_runtime_forbid(host->parent);
> +       brcmf_sdiod_host_fixup(host);
>  out:
>         if (ret)
>                 brcmf_sdiod_remove(sdiodev);
> @@ -1246,15 +1254,15 @@ static int brcmf_ops_sdio_suspend(struct device *dev)
>         brcmf_sdiod_freezer_on(sdiodev);
>         brcmf_sdio_wd_timer(sdiodev->bus, 0);
>
> +       sdio_flags = MMC_PM_KEEP_POWER;
>         if (sdiodev->wowl_enabled) {
> -               sdio_flags = MMC_PM_KEEP_POWER;
>                 if (sdiodev->pdata->oob_irq_supported)
>                         enable_irq_wake(sdiodev->pdata->oob_irq_nr);
>                 else
> -                       sdio_flags = MMC_PM_WAKE_SDIO_IRQ;
> -               if (sdio_set_host_pm_flags(sdiodev->func[1], sdio_flags))
> -                       brcmf_err("Failed to set pm_flags %x\n", sdio_flags);
> +                       sdio_flags |= MMC_PM_WAKE_SDIO_IRQ;
>         }
> +       if (sdio_set_host_pm_flags(sdiodev->func[1], sdio_flags))
> +               brcmf_err("Failed to set pm_flags %x\n", sdio_flags);
>         return 0;
>  }
>
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kind regards
Uffe

Re: [PATCH 07/10] brcmfmac: fix sdio suspend and resume

[Arend van Spriel]

- wireless list/maintainer

On 04/22/15 09:32, Ulf Hansson wrote:
> On 14 April 2015 at 20:10, Arend van Spriel<arend@broadcom.com>  wrote:
>> commit 330b4e4be937 ("brcmfmac: Add wowl support for SDIO devices.")
>> changed the behaviour by removing the MMC_PM_KEEP_POWER flag for
>> non-wowl scenario, which needs to be restored. Another necessary
>> change is to mark the card as being non-removable. With this in place
>> the suspend resume test passes successfully doing:
>>
>>   # echo devices>  /sys/power/pm_test
>>   # echo mem>  /sys/power/state
>>
>> Note that power may still be switched off when system is going
>> in S3 state.
>>
>> Reported-by: Fu, Zhonghui<<zhonghui.fu@linux.intel.com>
>> Reviewed-by: Pieter-Paul Giesberts<pieterpg@broadcom.com>
>> Reviewed-by: Franky (Zhenhui) Lin<frankyl@broadcom.com>
>> Signed-off-by: Arend van Spriel<arend@broadcom.com>
>> ---
>>   drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 18 +++++++++++++-----
>>   1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>> index 9b508bd..8a69544 100644
>> --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>> @@ -1011,6 +1011,14 @@ static int brcmf_sdiod_remove(struct brcmf_sdio_dev *sdiodev)
>>          return 0;
>>   }
>>
>> +static void brcmf_sdiod_host_fixup(struct mmc_host *host)
>> +{
>> +       /* runtime-pm powers off the device */
>> +       pm_runtime_forbid(host->parent);
>
> That you need this, clearly shows that something is broken in the mmc
> core/host layer.

This patch only moved this. The patch introducing this is here [1].

> Could you elaborate a bit on what configuration you are using. Like
> what mmc host, which SDIO bus speed mode.

Not just one. My test setup is a dev board hooked up to a card reader 
slot using sdhci-pci driver. Another setup I have is a chromebook with 
our device integrated with dw_mmc-rockchip driver. It is an arm platform 
with dt entry:

&sdio0 {
         broken-cd;
         bus-width = <4>;
         cap-sd-highspeed;
         sd-uhs-sdr12;
         sd-uhs-sdr25;
         sd-uhs-sdr50;
         sd-uhs-sdr104;
         cap-sdio-irq;
         card-external-vcc-supply = <&wifi_regulator>;
         clocks = <&cru HCLK_SDIO0>, <&cru SCLK_SDIO0>, <&cru 
SCLK_SDIO0_DRV>,
                  <&cru SCLK_SDIO0_SAMPLE>, <&rk808 RK808_CLKOUT1>;
         clock-names = "biu", "ciu", "ciu_drv", "ciu_sample", 
"card_ext_clock";
         keep-power-in-suspend;
         non-removable;
         num-slots = <1>;
         default-sample-phase = <90>;
         pinctrl-names = "default";
         pinctrl-0 = <&sdio0_clk &sdio0_cmd &sdio0_bus4>;
         status = "okay";
         vmmc-supply = <&vcc33_sys>;
         vqmmc-supply = <&vcc18_wl>;
};

I think card-external-vcc-supply is property that chromeos kernel 
handles to power the device.

> And have you tested different configurations? Like what happens if you
> use a different SDIO bus speed mode?

Regards,
Arend

[1] https://patchwork.kernel.org/patch/6038511/

>> +       /* avoid removal detection upon resume */
>> +       host->caps |= MMC_CAP_NONREMOVABLE;
>> +}
>> +
>>   static int brcmf_sdiod_probe(struct brcmf_sdio_dev *sdiodev)
>>   {
>>          struct sdio_func *func;
>> @@ -1076,7 +1084,7 @@ static int brcmf_sdiod_probe(struct brcmf_sdio_dev *sdiodev)
>>                  ret = -ENODEV;
>>                  goto out;
>>          }
>> -       pm_runtime_forbid(host->parent);
>> +       brcmf_sdiod_host_fixup(host);
>>   out:
>>          if (ret)
>>                  brcmf_sdiod_remove(sdiodev);
>> @@ -1246,15 +1254,15 @@ static int brcmf_ops_sdio_suspend(struct device *dev)
>>          brcmf_sdiod_freezer_on(sdiodev);
>>          brcmf_sdio_wd_timer(sdiodev->bus, 0);
>>
>> +       sdio_flags = MMC_PM_KEEP_POWER;
>>          if (sdiodev->wowl_enabled) {
>> -               sdio_flags = MMC_PM_KEEP_POWER;
>>                  if (sdiodev->pdata->oob_irq_supported)
>>                          enable_irq_wake(sdiodev->pdata->oob_irq_nr);
>>                  else
>> -                       sdio_flags = MMC_PM_WAKE_SDIO_IRQ;
>> -               if (sdio_set_host_pm_flags(sdiodev->func[1], sdio_flags))
>> -                       brcmf_err("Failed to set pm_flags %x\n", sdio_flags);
>> +                       sdio_flags |= MMC_PM_WAKE_SDIO_IRQ;
>>          }
>> +       if (sdio_set_host_pm_flags(sdiodev->func[1], sdio_flags))
>> +               brcmf_err("Failed to set pm_flags %x\n", sdio_flags);
>>          return 0;
>>   }
>>
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> Kind regards
> Uffe

Re: [PATCH 07/10] brcmfmac: fix sdio suspend and resume

[Ulf Hansson]

On 22 April 2015 at 10:52, Arend van Spriel <arend@broadcom.com> wrote:
> - wireless list/maintainer
>
>
> On 04/22/15 09:32, Ulf Hansson wrote:
>>
>> On 14 April 2015 at 20:10, Arend van Spriel<arend@broadcom.com>  wrote:
>>>
>>> commit 330b4e4be937 ("brcmfmac: Add wowl support for SDIO devices.")
>>> changed the behaviour by removing the MMC_PM_KEEP_POWER flag for
>>> non-wowl scenario, which needs to be restored. Another necessary
>>> change is to mark the card as being non-removable. With this in place
>>> the suspend resume test passes successfully doing:
>>>
>>>   # echo devices>  /sys/power/pm_test
>>>   # echo mem>  /sys/power/state
>>>
>>> Note that power may still be switched off when system is going
>>> in S3 state.
>>>
>>> Reported-by: Fu, Zhonghui<<zhonghui.fu@linux.intel.com>
>>> Reviewed-by: Pieter-Paul Giesberts<pieterpg@broadcom.com>
>>> Reviewed-by: Franky (Zhenhui) Lin<frankyl@broadcom.com>
>>> Signed-off-by: Arend van Spriel<arend@broadcom.com>
>>> ---
>>>   drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 18
>>> +++++++++++++-----
>>>   1 file changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>> b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>> index 9b508bd..8a69544 100644
>>> --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>> @@ -1011,6 +1011,14 @@ static int brcmf_sdiod_remove(struct
>>> brcmf_sdio_dev *sdiodev)
>>>          return 0;
>>>   }
>>>
>>> +static void brcmf_sdiod_host_fixup(struct mmc_host *host)
>>> +{
>>> +       /* runtime-pm powers off the device */
>>> +       pm_runtime_forbid(host->parent);
>>
>>
>> That you need this, clearly shows that something is broken in the mmc
>> core/host layer.
>
>
> This patch only moved this. The patch introducing this is here [1].

OK, thanks for sharing the information!

>
>> Could you elaborate a bit on what configuration you are using. Like
>> what mmc host, which SDIO bus speed mode.
>
>
> Not just one. My test setup is a dev board hooked up to a card reader slot
> using sdhci-pci driver. Another setup I have is a chromebook with our device
> integrated with dw_mmc-rockchip driver. It is an arm platform with dt entry:
>
> &sdio0 {
>         broken-cd;
>         bus-width = <4>;
>         cap-sd-highspeed;
>         sd-uhs-sdr12;
>         sd-uhs-sdr25;
>         sd-uhs-sdr50;
>         sd-uhs-sdr104;
>         cap-sdio-irq;
>         card-external-vcc-supply = <&wifi_regulator>;
>         clocks = <&cru HCLK_SDIO0>, <&cru SCLK_SDIO0>, <&cru
> SCLK_SDIO0_DRV>,
>                  <&cru SCLK_SDIO0_SAMPLE>, <&rk808 RK808_CLKOUT1>;
>         clock-names = "biu", "ciu", "ciu_drv", "ciu_sample",
> "card_ext_clock";
>         keep-power-in-suspend;
>         non-removable;
>         num-slots = <1>;
>         default-sample-phase = <90>;
>         pinctrl-names = "default";
>         pinctrl-0 = <&sdio0_clk &sdio0_cmd &sdio0_bus4>;
>         status = "okay";
>         vmmc-supply = <&vcc33_sys>;
>         vqmmc-supply = <&vcc18_wl>;
> };
>
> I think card-external-vcc-supply is property that chromeos kernel handles to
> power the device.

Should then likely be moved in a mmc pwrseq node and handled by the
mmc core, right?

The same might apply to "card_ext_clock"!?

>
>> And have you tested different configurations? Like what happens if you
>> use a different SDIO bus speed mode?

In the dw_mmc case, the host controller driver doesn't implement
runtime PM - only system PM (unless you are carrying some additional
out of tree patch :-) ).

So, are you sure the bug exists in this configuration at all?

Kind regards
Uffe

Re: [PATCH 07/10] brcmfmac: fix sdio suspend and resume

[Arend van Spriel]

On 04/22/15 11:18, Ulf Hansson wrote:
> On 22 April 2015 at 10:52, Arend van Spriel<arend@broadcom.com>  wrote:
>> - wireless list/maintainer
>>
>>
>> On 04/22/15 09:32, Ulf Hansson wrote:
>>>
>>> On 14 April 2015 at 20:10, Arend van Spriel<arend@broadcom.com>   wrote:
>>>>
>>>> commit 330b4e4be937 ("brcmfmac: Add wowl support for SDIO devices.")
>>>> changed the behaviour by removing the MMC_PM_KEEP_POWER flag for
>>>> non-wowl scenario, which needs to be restored. Another necessary
>>>> change is to mark the card as being non-removable. With this in place
>>>> the suspend resume test passes successfully doing:
>>>>
>>>>    # echo devices>   /sys/power/pm_test
>>>>    # echo mem>   /sys/power/state
>>>>
>>>> Note that power may still be switched off when system is going
>>>> in S3 state.
>>>>
>>>> Reported-by: Fu, Zhonghui<<zhonghui.fu@linux.intel.com>
>>>> Reviewed-by: Pieter-Paul Giesberts<pieterpg@broadcom.com>
>>>> Reviewed-by: Franky (Zhenhui) Lin<frankyl@broadcom.com>
>>>> Signed-off-by: Arend van Spriel<arend@broadcom.com>
>>>> ---
>>>>    drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 18
>>>> +++++++++++++-----
>>>>    1 file changed, 13 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>>> b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>>> index 9b508bd..8a69544 100644
>>>> --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>>> @@ -1011,6 +1011,14 @@ static int brcmf_sdiod_remove(struct
>>>> brcmf_sdio_dev *sdiodev)
>>>>           return 0;
>>>>    }
>>>>
>>>> +static void brcmf_sdiod_host_fixup(struct mmc_host *host)
>>>> +{
>>>> +       /* runtime-pm powers off the device */
>>>> +       pm_runtime_forbid(host->parent);
>>>
>>>
>>> That you need this, clearly shows that something is broken in the mmc
>>> core/host layer.
>>
>>
>> This patch only moved this. The patch introducing this is here [1].
>
> OK, thanks for sharing the information!
>
>>
>>> Could you elaborate a bit on what configuration you are using. Like
>>> what mmc host, which SDIO bus speed mode.
>>
>>
>> Not just one. My test setup is a dev board hooked up to a card reader slot
>> using sdhci-pci driver. Another setup I have is a chromebook with our device
>> integrated with dw_mmc-rockchip driver. It is an arm platform with dt entry:
>>
>> &sdio0 {
>>          broken-cd;
>>          bus-width =<4>;
>>          cap-sd-highspeed;
>>          sd-uhs-sdr12;
>>          sd-uhs-sdr25;
>>          sd-uhs-sdr50;
>>          sd-uhs-sdr104;
>>          cap-sdio-irq;
>>          card-external-vcc-supply =<&wifi_regulator>;
>>          clocks =<&cru HCLK_SDIO0>,<&cru SCLK_SDIO0>,<&cru
>> SCLK_SDIO0_DRV>,
>>                   <&cru SCLK_SDIO0_SAMPLE>,<&rk808 RK808_CLKOUT1>;
>>          clock-names = "biu", "ciu", "ciu_drv", "ciu_sample",
>> "card_ext_clock";
>>          keep-power-in-suspend;
>>          non-removable;
>>          num-slots =<1>;
>>          default-sample-phase =<90>;
>>          pinctrl-names = "default";
>>          pinctrl-0 =<&sdio0_clk&sdio0_cmd&sdio0_bus4>;
>>          status = "okay";
>>          vmmc-supply =<&vcc33_sys>;
>>          vqmmc-supply =<&vcc18_wl>;
>> };
>>
>> I think card-external-vcc-supply is property that chromeos kernel handles to
>> power the device.
>
> Should then likely be moved in a mmc pwrseq node and handled by the
> mmc core, right?
>
> The same might apply to "card_ext_clock"!?

Guess so. I was not involved in these DT changes and my focus is more on 
upstream.

>>
>>> And have you tested different configurations? Like what happens if you
>>> use a different SDIO bus speed mode?
>
> In the dw_mmc case, the host controller driver doesn't implement
> runtime PM - only system PM (unless you are carrying some additional
> out of tree patch :-) ).
>
> So, are you sure the bug exists in this configuration at all?

Forgot to mention the most applicable setup. I also have a Sony Vaio Duo 
13 with wifi sdio chip integrated through sdhci-acpi driver, which is 
doing runtime-pm. I had a number of people contacting me and I had them 
disable runtime-pm through sysfs to get working wifi. That was the 
reason for adding pm_runtime_forbid() to brcmfmac driver.

Regards,
Arend

Re: [PATCH 07/10] brcmfmac: fix sdio suspend and resume

[Ulf Hansson]

On 22 April 2015 at 11:38, Arend van Spriel <arend@broadcom.com> wrote:
> On 04/22/15 11:18, Ulf Hansson wrote:
>>
>> On 22 April 2015 at 10:52, Arend van Spriel<arend@broadcom.com>  wrote:
>>>
>>> - wireless list/maintainer
>>>
>>>
>>> On 04/22/15 09:32, Ulf Hansson wrote:
>>>>
>>>>
>>>> On 14 April 2015 at 20:10, Arend van Spriel<arend@broadcom.com>   wrote:
>>>>>
>>>>>
>>>>> commit 330b4e4be937 ("brcmfmac: Add wowl support for SDIO devices.")
>>>>> changed the behaviour by removing the MMC_PM_KEEP_POWER flag for
>>>>> non-wowl scenario, which needs to be restored. Another necessary
>>>>> change is to mark the card as being non-removable. With this in place
>>>>> the suspend resume test passes successfully doing:
>>>>>
>>>>>    # echo devices>   /sys/power/pm_test
>>>>>    # echo mem>   /sys/power/state
>>>>>
>>>>> Note that power may still be switched off when system is going
>>>>> in S3 state.
>>>>>
>>>>> Reported-by: Fu, Zhonghui<<zhonghui.fu@linux.intel.com>
>>>>> Reviewed-by: Pieter-Paul Giesberts<pieterpg@broadcom.com>
>>>>> Reviewed-by: Franky (Zhenhui) Lin<frankyl@broadcom.com>
>>>>> Signed-off-by: Arend van Spriel<arend@broadcom.com>
>>>>> ---
>>>>>    drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 18
>>>>> +++++++++++++-----
>>>>>    1 file changed, 13 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>>>> b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>>>> index 9b508bd..8a69544 100644
>>>>> --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>>>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>>>> @@ -1011,6 +1011,14 @@ static int brcmf_sdiod_remove(struct
>>>>> brcmf_sdio_dev *sdiodev)
>>>>>           return 0;
>>>>>    }
>>>>>
>>>>> +static void brcmf_sdiod_host_fixup(struct mmc_host *host)
>>>>> +{
>>>>> +       /* runtime-pm powers off the device */
>>>>> +       pm_runtime_forbid(host->parent);
>>>>
>>>>
>>>>
>>>> That you need this, clearly shows that something is broken in the mmc
>>>> core/host layer.
>>>
>>>
>>>
>>> This patch only moved this. The patch introducing this is here [1].
>>
>>
>> OK, thanks for sharing the information!
>>
>>>
>>>> Could you elaborate a bit on what configuration you are using. Like
>>>> what mmc host, which SDIO bus speed mode.
>>>
>>>
>>>
>>> Not just one. My test setup is a dev board hooked up to a card reader
>>> slot
>>> using sdhci-pci driver. Another setup I have is a chromebook with our
>>> device
>>> integrated with dw_mmc-rockchip driver. It is an arm platform with dt
>>> entry:
>>>
>>> &sdio0 {
>>>          broken-cd;
>>>          bus-width =<4>;
>>>          cap-sd-highspeed;
>>>          sd-uhs-sdr12;
>>>          sd-uhs-sdr25;
>>>          sd-uhs-sdr50;
>>>          sd-uhs-sdr104;
>>>          cap-sdio-irq;
>>>          card-external-vcc-supply =<&wifi_regulator>;
>>>          clocks =<&cru HCLK_SDIO0>,<&cru SCLK_SDIO0>,<&cru
>>> SCLK_SDIO0_DRV>,
>>>                   <&cru SCLK_SDIO0_SAMPLE>,<&rk808 RK808_CLKOUT1>;
>>>          clock-names = "biu", "ciu", "ciu_drv", "ciu_sample",
>>> "card_ext_clock";
>>>          keep-power-in-suspend;
>>>          non-removable;
>>>          num-slots =<1>;
>>>          default-sample-phase =<90>;
>>>          pinctrl-names = "default";
>>>          pinctrl-0 =<&sdio0_clk&sdio0_cmd&sdio0_bus4>;
>>>          status = "okay";
>>>          vmmc-supply =<&vcc33_sys>;
>>>          vqmmc-supply =<&vcc18_wl>;
>>> };
>>>
>>> I think card-external-vcc-supply is property that chromeos kernel handles
>>> to
>>> power the device.
>>
>>
>> Should then likely be moved in a mmc pwrseq node and handled by the
>> mmc core, right?
>>
>> The same might apply to "card_ext_clock"!?
>
>
> Guess so. I was not involved in these DT changes and my focus is more on
> upstream.
>
>>>
>>>> And have you tested different configurations? Like what happens if you
>>>> use a different SDIO bus speed mode?
>>
>>
>> In the dw_mmc case, the host controller driver doesn't implement
>> runtime PM - only system PM (unless you are carrying some additional
>> out of tree patch :-) ).
>>
>> So, are you sure the bug exists in this configuration at all?
>
>
> Forgot to mention the most applicable setup. I also have a Sony Vaio Duo 13
> with wifi sdio chip integrated through sdhci-acpi driver, which is doing
> runtime-pm. I had a number of people contacting me and I had them disable
> runtime-pm through sysfs to get working wifi. That was the reason for adding
> pm_runtime_forbid() to brcmfmac driver.

Okay.

Anyway, if you find time to try the other configurations and then
change to "allow" runtime PM, it would be nice to know the results.

Kind regards
Uffe

Re: [PATCH 07/10] brcmfmac: fix sdio suspend and resume

[Kalle Valo]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> On 14 April 2015 at 20:10, Arend van Spriel <arend@broadcom.com> wrote:
>> commit 330b4e4be937 ("brcmfmac: Add wowl support for SDIO devices.")
>> changed the behaviour by removing the MMC_PM_KEEP_POWER flag for
>> non-wowl scenario, which needs to be restored. Another necessary
>> change is to mark the card as being non-removable. With this in place
>> the suspend resume test passes successfully doing:
>>
>>  # echo devices > /sys/power/pm_test
>>  # echo mem > /sys/power/state
>>
>> Note that power may still be switched off when system is going
>> in S3 state.
>>
>> Reported-by: Fu, Zhonghui <<zhonghui.fu@linux.intel.com>
>> Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
>> Reviewed-by: Franky (Zhenhui) Lin <frankyl@broadcom.com>
>> Signed-off-by: Arend van Spriel <arend@broadcom.com>
>> ---
>>  drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 18 +++++++++++++-----
>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>> index 9b508bd..8a69544 100644
>> --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>> @@ -1011,6 +1011,14 @@ static int brcmf_sdiod_remove(struct brcmf_sdio_dev *sdiodev)
>>         return 0;
>>  }
>>
>> +static void brcmf_sdiod_host_fixup(struct mmc_host *host)
>> +{
>> +       /* runtime-pm powers off the device */
>> +       pm_runtime_forbid(host->parent);
>
> That you need this, clearly shows that something is broken in the mmc
> core/host layer.
>
> Could you elaborate a bit on what configuration you are using. Like
> what mmc host, which SDIO bus speed mode.
>
> And have you tested different configurations? Like what happens if you
> use a different SDIO bus speed mode?

So what should I do with this patch? Good to commit still?

-- 
Kalle Valo

Re: [PATCH 07/10] brcmfmac: fix sdio suspend and resume

[Arend van Spriel]

On 04/28/15 18:14, Kalle Valo wrote:
> Ulf Hansson<ulf.hansson@linaro.org>  writes:
>
>> On 14 April 2015 at 20:10, Arend van Spriel<arend@broadcom.com>  wrote:
>>> commit 330b4e4be937 ("brcmfmac: Add wowl support for SDIO devices.")
>>> changed the behaviour by removing the MMC_PM_KEEP_POWER flag for
>>> non-wowl scenario, which needs to be restored. Another necessary
>>> change is to mark the card as being non-removable. With this in place
>>> the suspend resume test passes successfully doing:
>>>
>>>   # echo devices>  /sys/power/pm_test
>>>   # echo mem>  /sys/power/state
>>>
>>> Note that power may still be switched off when system is going
>>> in S3 state.
>>>
>>> Reported-by: Fu, Zhonghui<<zhonghui.fu@linux.intel.com>
>>> Reviewed-by: Pieter-Paul Giesberts<pieterpg@broadcom.com>
>>> Reviewed-by: Franky (Zhenhui) Lin<frankyl@broadcom.com>
>>> Signed-off-by: Arend van Spriel<arend@broadcom.com>
>>> ---
>>>   drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 18 +++++++++++++-----
>>>   1 file changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>> index 9b508bd..8a69544 100644
>>> --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>> @@ -1011,6 +1011,14 @@ static int brcmf_sdiod_remove(struct brcmf_sdio_dev *sdiodev)
>>>          return 0;
>>>   }
>>>
>>> +static void brcmf_sdiod_host_fixup(struct mmc_host *host)
>>> +{
>>> +       /* runtime-pm powers off the device */
>>> +       pm_runtime_forbid(host->parent);
>>
>> That you need this, clearly shows that something is broken in the mmc
>> core/host layer.
>>
>> Could you elaborate a bit on what configuration you are using. Like
>> what mmc host, which SDIO bus speed mode.
>>
>> And have you tested different configurations? Like what happens if you
>> use a different SDIO bus speed mode?
>
> So what should I do with this patch? Good to commit still?

I think so, but I am biased ;-) Seriously, this enables people that have 
brcmfmac devices hooked up through runtime-pm capable host controllers 
to use wifi. So while we need to work to proper runtime-pm support for 
SDIO function drivers with proper interaction with the MMC stack this 
patch provides a short term solution/workaround at the cost of being a 
bit less power efficient.

Regards,
Arend

Re: [PATCH 07/10] brcmfmac: fix sdio suspend and resume

[Ulf Hansson]

On 28 April 2015 at 18:50, Arend van Spriel <arend@broadcom.com> wrote:
> On 04/28/15 18:14, Kalle Valo wrote:
>>
>> Ulf Hansson<ulf.hansson@linaro.org>  writes:
>>
>>> On 14 April 2015 at 20:10, Arend van Spriel<arend@broadcom.com>  wrote:
>>>>
>>>> commit 330b4e4be937 ("brcmfmac: Add wowl support for SDIO devices.")
>>>> changed the behaviour by removing the MMC_PM_KEEP_POWER flag for
>>>> non-wowl scenario, which needs to be restored. Another necessary
>>>> change is to mark the card as being non-removable. With this in place
>>>> the suspend resume test passes successfully doing:
>>>>
>>>>   # echo devices>  /sys/power/pm_test
>>>>   # echo mem>  /sys/power/state
>>>>
>>>> Note that power may still be switched off when system is going
>>>> in S3 state.
>>>>
>>>> Reported-by: Fu, Zhonghui<<zhonghui.fu@linux.intel.com>
>>>> Reviewed-by: Pieter-Paul Giesberts<pieterpg@broadcom.com>
>>>> Reviewed-by: Franky (Zhenhui) Lin<frankyl@broadcom.com>
>>>> Signed-off-by: Arend van Spriel<arend@broadcom.com>
>>>> ---
>>>>   drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 18
>>>> +++++++++++++-----
>>>>   1 file changed, 13 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>>> b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>>> index 9b508bd..8a69544 100644
>>>> --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>>>> @@ -1011,6 +1011,14 @@ static int brcmf_sdiod_remove(struct
>>>> brcmf_sdio_dev *sdiodev)
>>>>          return 0;
>>>>   }
>>>>
>>>> +static void brcmf_sdiod_host_fixup(struct mmc_host *host)
>>>> +{
>>>> +       /* runtime-pm powers off the device */
>>>> +       pm_runtime_forbid(host->parent);
>>>
>>>
>>> That you need this, clearly shows that something is broken in the mmc
>>> core/host layer.
>>>
>>> Could you elaborate a bit on what configuration you are using. Like
>>> what mmc host, which SDIO bus speed mode.
>>>
>>> And have you tested different configurations? Like what happens if you
>>> use a different SDIO bus speed mode?
>>
>>
>> So what should I do with this patch? Good to commit still?
>
>
> I think so, but I am biased ;-) Seriously, this enables people that have
> brcmfmac devices hooked up through runtime-pm capable host controllers to
> use wifi. So while we need to work to proper runtime-pm support for SDIO
> function drivers with proper interaction with the MMC stack this patch
> provides a short term solution/workaround at the cost of being a bit less
> power efficient.

I agree that this seems to be the only viable short-term solution.
The only concern I have, is that we might drop the focus to find a
proper long-term solution. But, let's try to avoid that.

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

[PATCH 08/10] brcmfmac: add support for BCM4358 PCIe device

[Arend van Spriel]

This patch adds support for the BCM4358 2x2 11ac device.

Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/chip.c       | 1 +
 drivers/net/wireless/brcm80211/brcmfmac/pcie.c       | 9 +++++++++
 drivers/net/wireless/brcm80211/include/brcm_hw_ids.h | 2 ++
 3 files changed, 12 insertions(+)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/chip.c b/drivers/net/wireless/brcm80211/brcmfmac/chip.c
index ab2fac8..288f831 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/chip.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/chip.c
@@ -649,6 +649,7 @@ static u32 brcmf_chip_tcm_rambase(struct brcmf_chip_priv *ci)
 	case BRCM_CC_43567_CHIP_ID:
 	case BRCM_CC_43569_CHIP_ID:
 	case BRCM_CC_43570_CHIP_ID:
+	case BRCM_CC_4358_CHIP_ID:
 	case BRCM_CC_43602_CHIP_ID:
 		return 0x180000;
 	default:
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/brcm80211/brcmfmac/pcie.c
index 1831ecd..8815de7 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/pcie.c
@@ -51,6 +51,8 @@ enum brcmf_pcie_state {
 #define BRCMF_PCIE_4356_NVRAM_NAME		"brcm/brcmfmac4356-pcie.txt"
 #define BRCMF_PCIE_43570_FW_NAME		"brcm/brcmfmac43570-pcie.bin"
 #define BRCMF_PCIE_43570_NVRAM_NAME		"brcm/brcmfmac43570-pcie.txt"
+#define BRCMF_PCIE_4358_FW_NAME			"brcm/brcmfmac4358-pcie.bin"
+#define BRCMF_PCIE_4358_NVRAM_NAME		"brcm/brcmfmac4358-pcie.txt"
 
 #define BRCMF_PCIE_FW_UP_TIMEOUT		2000 /* msec */
 
@@ -189,6 +191,8 @@ MODULE_FIRMWARE(BRCMF_PCIE_4356_FW_NAME);
 MODULE_FIRMWARE(BRCMF_PCIE_4356_NVRAM_NAME);
 MODULE_FIRMWARE(BRCMF_PCIE_43570_FW_NAME);
 MODULE_FIRMWARE(BRCMF_PCIE_43570_NVRAM_NAME);
+MODULE_FIRMWARE(BRCMF_PCIE_4358_FW_NAME);
+MODULE_FIRMWARE(BRCMF_PCIE_4358_NVRAM_NAME);
 
 
 struct brcmf_pcie_console {
@@ -1333,6 +1337,10 @@ static int brcmf_pcie_get_fwnames(struct brcmf_pciedev_info *devinfo)
 		fw_name = BRCMF_PCIE_43570_FW_NAME;
 		nvram_name = BRCMF_PCIE_43570_NVRAM_NAME;
 		break;
+	case BRCM_CC_4358_CHIP_ID:
+		fw_name = BRCMF_PCIE_4358_FW_NAME;
+		nvram_name = BRCMF_PCIE_4358_NVRAM_NAME;
+		break;
 	default:
 		brcmf_err("Unsupported chip 0x%04x\n", devinfo->ci->chip);
 		return -ENODEV;
@@ -1850,6 +1858,7 @@ static struct pci_device_id brcmf_pcie_devid_table[] = {
 	BRCMF_PCIE_DEVICE(BRCM_PCIE_4356_DEVICE_ID),
 	BRCMF_PCIE_DEVICE(BRCM_PCIE_43567_DEVICE_ID),
 	BRCMF_PCIE_DEVICE(BRCM_PCIE_43570_DEVICE_ID),
+	BRCMF_PCIE_DEVICE(BRCM_PCIE_4358_DEVICE_ID),
 	BRCMF_PCIE_DEVICE(BRCM_PCIE_43602_DEVICE_ID),
 	BRCMF_PCIE_DEVICE(BRCM_PCIE_43602_2G_DEVICE_ID),
 	BRCMF_PCIE_DEVICE(BRCM_PCIE_43602_5G_DEVICE_ID),
diff --git a/drivers/net/wireless/brcm80211/include/brcm_hw_ids.h b/drivers/net/wireless/brcm80211/include/brcm_hw_ids.h
index 4efdd51..2543282 100644
--- a/drivers/net/wireless/brcm80211/include/brcm_hw_ids.h
+++ b/drivers/net/wireless/brcm80211/include/brcm_hw_ids.h
@@ -45,6 +45,7 @@
 #define BRCM_CC_43567_CHIP_ID		43567
 #define BRCM_CC_43569_CHIP_ID		43569
 #define BRCM_CC_43570_CHIP_ID		43570
+#define BRCM_CC_4358_CHIP_ID		0x4358
 #define BRCM_CC_43602_CHIP_ID		43602
 
 /* USB Device IDs */
@@ -59,6 +60,7 @@
 #define BRCM_PCIE_4356_DEVICE_ID	0x43ec
 #define BRCM_PCIE_43567_DEVICE_ID	0x43d3
 #define BRCM_PCIE_43570_DEVICE_ID	0x43d9
+#define BRCM_PCIE_4358_DEVICE_ID	0x43e9
 #define BRCM_PCIE_43602_DEVICE_ID	0x43ba
 #define BRCM_PCIE_43602_2G_DEVICE_ID	0x43bb
 #define BRCM_PCIE_43602_5G_DEVICE_ID	0x43bc
-- 
1.9.1

[PATCH 09/10] brcmfmac: add additional 43602 pcie device id.

[Arend van Spriel]

From: Hante Meuleman <meuleman@broadcom.com>

Reviewed-by: Arend Van Spriel <arend@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Signed-off-by: Hante Meuleman <meuleman@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/pcie.c       | 1 +
 drivers/net/wireless/brcm80211/include/brcm_hw_ids.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/brcm80211/brcmfmac/pcie.c
index 8815de7..5e97a97 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/pcie.c
@@ -1862,6 +1862,7 @@ static struct pci_device_id brcmf_pcie_devid_table[] = {
 	BRCMF_PCIE_DEVICE(BRCM_PCIE_43602_DEVICE_ID),
 	BRCMF_PCIE_DEVICE(BRCM_PCIE_43602_2G_DEVICE_ID),
 	BRCMF_PCIE_DEVICE(BRCM_PCIE_43602_5G_DEVICE_ID),
+	BRCMF_PCIE_DEVICE(BRCM_PCIE_43602_RAW_DEVICE_ID),
 	{ /* end: all zeroes */ }
 };
 
diff --git a/drivers/net/wireless/brcm80211/include/brcm_hw_ids.h b/drivers/net/wireless/brcm80211/include/brcm_hw_ids.h
index 2543282..7a6daa3 100644
--- a/drivers/net/wireless/brcm80211/include/brcm_hw_ids.h
+++ b/drivers/net/wireless/brcm80211/include/brcm_hw_ids.h
@@ -64,6 +64,7 @@
 #define BRCM_PCIE_43602_DEVICE_ID	0x43ba
 #define BRCM_PCIE_43602_2G_DEVICE_ID	0x43bb
 #define BRCM_PCIE_43602_5G_DEVICE_ID	0x43bc
+#define BRCM_PCIE_43602_RAW_DEVICE_ID	43602
 
 /* brcmsmac IDs */
 #define BCM4313_D11N2G_ID	0x4727	/* 4313 802.11n 2.4G device */
-- 
1.9.1

[PATCH 10/10] brcmfmac: Add support for multiple PCIE devices in nvram.

[Arend van Spriel]

From: Hante Meuleman <meuleman@broadcom.com>

With PCIE it is possible to support multiple devices with the
same device type. They all load the same nvram file. In order to
support this the nvram can specify which part of the nvram is
for which pcie device. This patch adds support for these new
types of nvram files.

Reviewed-by: Arend Van Spriel <arend@broadcom.com>
Reviewed-by: Franky (Zhenhui) Lin <frankyl@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Reviewed-by: Daniel (Deognyoun) Kim <dekim@broadcom.com>
Signed-off-by: Hante Meuleman <meuleman@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/firmware.c | 189 ++++++++++++++++++++-
 drivers/net/wireless/brcm80211/brcmfmac/firmware.h |   6 +
 drivers/net/wireless/brcm80211/brcmfmac/pcie.c     |  15 +-
 3 files changed, 197 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
index 9cb9915..8ff31ff 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
@@ -23,6 +23,10 @@
 #include "debug.h"
 #include "firmware.h"
 
+#define BRCMF_FW_MAX_NVRAM_SIZE			64000
+#define BRCMF_FW_NVRAM_DEVPATH_LEN		19	/* devpath0=pcie/1/4/ */
+#define BRCMF_FW_NVRAM_PCIEDEV_LEN		9	/* pcie/1/4/ */
+
 char brcmf_firmware_path[BRCMF_FW_PATH_LEN];
 module_param_string(firmware_path, brcmf_firmware_path,
 		    BRCMF_FW_PATH_LEN, 0440);
@@ -46,6 +50,8 @@ enum nvram_parser_state {
  * @column: current column in line.
  * @pos: byte offset in input buffer.
  * @entry: start position of key,value entry.
+ * @multi_dev_v1: detect pcie multi device v1 (compressed).
+ * @multi_dev_v2: detect pcie multi device v2.
  */
 struct nvram_parser {
 	enum nvram_parser_state state;
@@ -56,6 +62,8 @@ struct nvram_parser {
 	u32 column;
 	u32 pos;
 	u32 entry;
+	bool multi_dev_v1;
+	bool multi_dev_v2;
 };
 
 static bool is_nvram_char(char c)
@@ -108,6 +116,10 @@ static enum nvram_parser_state brcmf_nvram_handle_key(struct nvram_parser *nvp)
 			st = COMMENT;
 		else
 			st = VALUE;
+		if (strncmp(&nvp->fwnv->data[nvp->entry], "devpath", 7) == 0)
+			nvp->multi_dev_v1 = true;
+		if (strncmp(&nvp->fwnv->data[nvp->entry], "pcie/", 5) == 0)
+			nvp->multi_dev_v2 = true;
 	} else if (!is_nvram_char(c)) {
 		brcmf_dbg(INFO, "warning: ln=%d:col=%d: '=' expected, skip invalid key entry\n",
 			  nvp->line, nvp->column);
@@ -133,6 +145,8 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp)
 		ekv = (u8 *)&nvp->fwnv->data[nvp->pos];
 		skv = (u8 *)&nvp->fwnv->data[nvp->entry];
 		cplen = ekv - skv;
+		if (nvp->nvram_len + cplen + 1 >= BRCMF_FW_MAX_NVRAM_SIZE)
+			return END;
 		/* copy to output buffer */
 		memcpy(&nvp->nvram[nvp->nvram_len], skv, cplen);
 		nvp->nvram_len += cplen;
@@ -180,10 +194,18 @@ static enum nvram_parser_state
 static int brcmf_init_nvram_parser(struct nvram_parser *nvp,
 				   const struct firmware *nv)
 {
+	size_t size;
+
 	memset(nvp, 0, sizeof(*nvp));
 	nvp->fwnv = nv;
+	/* Limit size to MAX_NVRAM_SIZE, some files contain lot of comment */
+	if (nv->size > BRCMF_FW_MAX_NVRAM_SIZE)
+		size = BRCMF_FW_MAX_NVRAM_SIZE;
+	else
+		size = nv->size;
 	/* Alloc for extra 0 byte + roundup by 4 + length field */
-	nvp->nvram = kzalloc(nv->size + 1 + 3 + sizeof(u32), GFP_KERNEL);
+	size += 1 + 3 + sizeof(u32);
+	nvp->nvram = kzalloc(size, GFP_KERNEL);
 	if (!nvp->nvram)
 		return -ENOMEM;
 
@@ -192,12 +214,136 @@ static int brcmf_init_nvram_parser(struct nvram_parser *nvp,
 	return 0;
 }
 
+/* brcmf_fw_strip_multi_v1 :Some nvram files contain settings for multiple
+ * devices. Strip it down for one device, use domain_nr/bus_nr to determine
+ * which data is to be returned. v1 is the version where nvram is stored
+ * compressed and "devpath" maps to index for valid entries.
+ */
+static void brcmf_fw_strip_multi_v1(struct nvram_parser *nvp, u16 domain_nr,
+				    u16 bus_nr)
+{
+	u32 i, j;
+	bool found;
+	u8 *nvram;
+	u8 id;
+
+	nvram = kzalloc(nvp->nvram_len + 1 + 3 + sizeof(u32), GFP_KERNEL);
+	if (!nvram)
+		goto fail;
+
+	/* min length: devpath0=pcie/1/4/ + 0:x=y */
+	if (nvp->nvram_len < BRCMF_FW_NVRAM_DEVPATH_LEN + 6)
+		goto fail;
+
+	/* First search for the devpathX and see if it is the configuration
+	 * for domain_nr/bus_nr. Search complete nvp
+	 */
+	found = false;
+	i = 0;
+	while (i < nvp->nvram_len - BRCMF_FW_NVRAM_DEVPATH_LEN) {
+		/* Format: devpathX=pcie/Y/Z/
+		 * Y = domain_nr, Z = bus_nr, X = virtual ID
+		 */
+		if ((strncmp(&nvp->nvram[i], "devpath", 7) == 0) &&
+		    (strncmp(&nvp->nvram[i + 8], "=pcie/", 6) == 0)) {
+			if (((nvp->nvram[i + 14] - '0') == domain_nr) &&
+			    ((nvp->nvram[i + 16] - '0') == bus_nr)) {
+				id = nvp->nvram[i + 7] - '0';
+				found = true;
+				break;
+			}
+		}
+		while (nvp->nvram[i] != 0)
+			i++;
+		i++;
+	}
+	if (!found)
+		goto fail;
+
+	/* Now copy all valid entries, release old nvram and assign new one */
+	i = 0;
+	j = 0;
+	while (i < nvp->nvram_len) {
+		if ((nvp->nvram[i] - '0' == id) && (nvp->nvram[i + 1] == ':')) {
+			i += 2;
+			while (nvp->nvram[i] != 0) {
+				nvram[j] = nvp->nvram[i];
+				i++;
+				j++;
+			}
+			nvram[j] = 0;
+			j++;
+		}
+		while (nvp->nvram[i] != 0)
+			i++;
+		i++;
+	}
+	kfree(nvp->nvram);
+	nvp->nvram = nvram;
+	nvp->nvram_len = j;
+	return;
+
+fail:
+	kfree(nvram);
+	nvp->nvram_len = 0;
+}
+
+/* brcmf_fw_strip_multi_v2 :Some nvram files contain settings for multiple
+ * devices. Strip it down for one device, use domain_nr/bus_nr to determine
+ * which data is to be returned. v2 is the version where nvram is stored
+ * uncompressed, all relevant valid entries are identified by
+ * pcie/domain_nr/bus_nr:
+ */
+static void brcmf_fw_strip_multi_v2(struct nvram_parser *nvp, u16 domain_nr,
+				    u16 bus_nr)
+{
+	u32 i, j;
+	u8 *nvram;
+
+	nvram = kzalloc(nvp->nvram_len + 1 + 3 + sizeof(u32), GFP_KERNEL);
+	if (!nvram)
+		goto fail;
+
+	/* Copy all valid entries, release old nvram and assign new one.
+	 * Valid entries are of type pcie/X/Y/ where X = domain_nr and
+	 * Y = bus_nr.
+	 */
+	i = 0;
+	j = 0;
+	while (i < nvp->nvram_len - BRCMF_FW_NVRAM_PCIEDEV_LEN) {
+		if ((strncmp(&nvp->nvram[i], "pcie/", 5) == 0) &&
+		    (nvp->nvram[i + 6] == '/') && (nvp->nvram[i + 8] == '/') &&
+		    ((nvp->nvram[i + 5] - '0') == domain_nr) &&
+		    ((nvp->nvram[i + 7] - '0') == bus_nr)) {
+			i += BRCMF_FW_NVRAM_PCIEDEV_LEN;
+			while (nvp->nvram[i] != 0) {
+				nvram[j] = nvp->nvram[i];
+				i++;
+				j++;
+			}
+			nvram[j] = 0;
+			j++;
+		}
+		while (nvp->nvram[i] != 0)
+			i++;
+		i++;
+	}
+	kfree(nvp->nvram);
+	nvp->nvram = nvram;
+	nvp->nvram_len = j;
+	return;
+fail:
+	kfree(nvram);
+	nvp->nvram_len = 0;
+}
+
 /* brcmf_nvram_strip :Takes a buffer of "<var>=<value>\n" lines read from a fil
  * and ending in a NUL. Removes carriage returns, empty lines, comment lines,
  * and converts newlines to NULs. Shortens buffer as needed and pads with NULs.
  * End of buffer is completed with token identifying length of buffer.
  */
-static void *brcmf_fw_nvram_strip(const struct firmware *nv, u32 *new_length)
+static void *brcmf_fw_nvram_strip(const struct firmware *nv, u32 *new_length,
+				  u16 domain_nr, u16 bus_nr)
 {
 	struct nvram_parser nvp;
 	u32 pad;
@@ -212,6 +358,16 @@ static void *brcmf_fw_nvram_strip(const struct firmware *nv, u32 *new_length)
 		if (nvp.state == END)
 			break;
 	}
+	if (nvp.multi_dev_v1)
+		brcmf_fw_strip_multi_v1(&nvp, domain_nr, bus_nr);
+	else if (nvp.multi_dev_v2)
+		brcmf_fw_strip_multi_v2(&nvp, domain_nr, bus_nr);
+
+	if (nvp.nvram_len == 0) {
+		kfree(nvp.nvram);
+		return NULL;
+	}
+
 	pad = nvp.nvram_len;
 	*new_length = roundup(nvp.nvram_len + 1, 4);
 	while (pad != *new_length) {
@@ -239,6 +395,8 @@ struct brcmf_fw {
 	u16 flags;
 	const struct firmware *code;
 	const char *nvram_name;
+	u16 domain_nr;
+	u16 bus_nr;
 	void (*done)(struct device *dev, const struct firmware *fw,
 		     void *nvram_image, u32 nvram_len);
 };
@@ -254,7 +412,8 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
 		goto fail;
 
 	if (fw) {
-		nvram = brcmf_fw_nvram_strip(fw, &nvram_length);
+		nvram = brcmf_fw_nvram_strip(fw, &nvram_length,
+					     fwctx->domain_nr, fwctx->bus_nr);
 		release_firmware(fw);
 		if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
 			goto fail;
@@ -309,11 +468,12 @@ fail:
 	kfree(fwctx);
 }
 
-int brcmf_fw_get_firmwares(struct device *dev, u16 flags,
-			   const char *code, const char *nvram,
-			   void (*fw_cb)(struct device *dev,
-					 const struct firmware *fw,
-					 void *nvram_image, u32 nvram_len))
+int brcmf_fw_get_firmwares_pcie(struct device *dev, u16 flags,
+				const char *code, const char *nvram,
+				void (*fw_cb)(struct device *dev,
+					      const struct firmware *fw,
+					      void *nvram_image, u32 nvram_len),
+				u16 domain_nr, u16 bus_nr)
 {
 	struct brcmf_fw *fwctx;
 
@@ -333,8 +493,21 @@ int brcmf_fw_get_firmwares(struct device *dev, u16 flags,
 	fwctx->done = fw_cb;
 	if (flags & BRCMF_FW_REQUEST_NVRAM)
 		fwctx->nvram_name = nvram;
+	fwctx->domain_nr = domain_nr;
+	fwctx->bus_nr = bus_nr;
 
 	return request_firmware_nowait(THIS_MODULE, true, code, dev,
 				       GFP_KERNEL, fwctx,
 				       brcmf_fw_request_code_done);
 }
+
+int brcmf_fw_get_firmwares(struct device *dev, u16 flags,
+			   const char *code, const char *nvram,
+			   void (*fw_cb)(struct device *dev,
+					 const struct firmware *fw,
+					 void *nvram_image, u32 nvram_len))
+{
+	return brcmf_fw_get_firmwares_pcie(dev, flags, code, nvram, fw_cb, 0,
+					   0);
+}
+
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/firmware.h b/drivers/net/wireless/brcm80211/brcmfmac/firmware.h
index 4d34823..604dd48 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/firmware.h
+++ b/drivers/net/wireless/brcm80211/brcmfmac/firmware.h
@@ -32,6 +32,12 @@ void brcmf_fw_nvram_free(void *nvram);
  * fails it will not use the callback, but call device_release_driver()
  * instead which will call the driver .remove() callback.
  */
+int brcmf_fw_get_firmwares_pcie(struct device *dev, u16 flags,
+				const char *code, const char *nvram,
+				void (*fw_cb)(struct device *dev,
+					      const struct firmware *fw,
+					      void *nvram_image, u32 nvram_len),
+				u16 domain_nr, u16 bus_nr);
 int brcmf_fw_get_firmwares(struct device *dev, u16 flags,
 			   const char *code, const char *nvram,
 			   void (*fw_cb)(struct device *dev,
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/brcm80211/brcmfmac/pcie.c
index 5e97a97..0453d10 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/pcie.c
@@ -1649,8 +1649,13 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	struct brcmf_pciedev_info *devinfo;
 	struct brcmf_pciedev *pcie_bus_dev;
 	struct brcmf_bus *bus;
+	u16 domain_nr;
+	u16 bus_nr;
 
-	brcmf_dbg(PCIE, "Enter %x:%x\n", pdev->vendor, pdev->device);
+	domain_nr = pci_domain_nr(pdev->bus) + 1;
+	bus_nr = pdev->bus->number;
+	brcmf_dbg(PCIE, "Enter %x:%x (%d/%d)\n", pdev->vendor, pdev->device,
+		  domain_nr, bus_nr);
 
 	ret = -ENOMEM;
 	devinfo = kzalloc(sizeof(*devinfo), GFP_KERNEL);
@@ -1699,10 +1704,10 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (ret)
 		goto fail_bus;
 
-	ret = brcmf_fw_get_firmwares(bus->dev, BRCMF_FW_REQUEST_NVRAM |
-					       BRCMF_FW_REQ_NV_OPTIONAL,
-				     devinfo->fw_name, devinfo->nvram_name,
-				     brcmf_pcie_setup);
+	ret = brcmf_fw_get_firmwares_pcie(bus->dev, BRCMF_FW_REQUEST_NVRAM |
+						    BRCMF_FW_REQ_NV_OPTIONAL,
+					  devinfo->fw_name, devinfo->nvram_name,
+					  brcmf_pcie_setup, domain_nr, bus_nr);
 	if (ret == 0)
 		return 0;
 fail_bus:
-- 
1.9.1

Re: [PATCH 10/10] brcmfmac: Add support for multiple PCIE devices in nvram.

[Rafał Miłecki]

On 14 April 2015 at 20:10, Arend van Spriel <arend@broadcom.com> wrote:
> From: Hante Meuleman <meuleman@broadcom.com>
>
> With PCIE it is possible to support multiple devices with the
> same device type. They all load the same nvram file. In order to
> support this the nvram can specify which part of the nvram is
> for which pcie device. This patch adds support for these new
> types of nvram files.

I'm a bit unsure about this patch. Does it support NVRAM files that
are already in use by any platform? Or are you trying to solve problem
we hit with ARM routers like Netgear R8000?

If you developed this patch to support e.g. Netgear R8000 I'm afraid
it won't be really helpful. You won't be able to provide a single
NVRAM file for all devices because of many differences between
devices. I don't only mean different models, but even units. I believe
these PCIe devices have to receive NVRAM with e.g. MAC address so you
don't want to provide one file for all users.
It means we'll need to develop some extra layer that will fetch
*system* (SoC) NVRAM and translate it into a NVRAM file that will be
stored in user space. Then this file will be loaded back into the
kernel.

So if this is about adding support for embedded devices, I'd prefer
much more to simply use Linux's NVRAM driver to get all wanted
entries.

Other than that I'm a bit confused by the "pcie/" prefix you decided
to use. Every Broadcom NVRAM I've seen was using pci/ prefix.
Shouldn't you stick to this standard format? Switching to "pcie/"
would require another translation when reading entries from system
NVRAM.

Re: [PATCH 10/10] brcmfmac: Add support for multiple PCIE devices in nvram.

[Rafał Miłecki]

Huh, why dropping linux-wireless (and top posting btw)? Please let
everyone follow the discussion :)

On 15 April 2015 at 21:20, Hante Meuleman <meuleman@broadcom.com> wrote:
> As I wrote to you in a mail and on the openwrt forum, this patch is indeed an attempt to support more complex nvram files. I also wrote, that in order to be able to use it, the nvram contents of the device (r8000) needs to be put a specific file. Now for your concerns, we can perhaps add something which will read the nvram contents directly from an nvram store. But that is irrelevant to this patch. The parsing is still needed, and all we would need to add is something which is reading the nvram contents from some other place

So it makes me wonder if we need this patch in its current form. I
think getting NVRAM directly from the platform is much user friendly.
It doesn't require user to install some extra tools for dumping NVRAM
and putting it in a specific file. One extra layer less.
With that said I think it's hard to review your code for parsing
NVRAM. We don't know how it's going to be fetched in the first place.


> though it would have to be put under some kernel config flag as this would not be supported in non-router systems. The contents of the nvram would however still need to be parsed in exactly the same way as the nvram files we read from disk.

Again, it's hard to say for me. Are you going to use
bcm47xx_nvram_getenv? Are you going to use MTD subsystem? Are you
going to develop different solution? When using e.g.
bcm47xx_nvram_getenv you won't want all this parsing stuff at all.

It seems this patch provides some end-support for NVRAM parsing while
we still miss something between. Something for getting NVRAM from
platform and providing it to the brcmfmac somehow.


> As to your concern regarding pci/ versus pcie/: pci/ is old type and will never be used/supported by brcmfmac. All new routers will use either the compressed format like the r8000 does or the pcie/ (uncompressed) format depending on the size of the nvram store.

Oh, I didn't notice that. OK, thanks for pointing this.

Re: [PATCH 10/10] brcmfmac: Add support for multiple PCIE devices in nvram.

[Arend van Spriel]

+ openwrt-devel list

On 04/17/15 09:45, Rafał Miłecki wrote:
> Huh, why dropping linux-wireless (and top posting btw)? Please let
> everyone follow the discussion :)
>
> On 15 April 2015 at 21:20, Hante Meuleman<meuleman@broadcom.com>  wrote:
>> As I wrote to you in a mail and on the openwrt forum, this patch is indeed an attempt to support more complex nvram files. I also wrote, that in order to be able to use it, the nvram contents of the device (r8000) needs to be put a specific file. Now for your concerns, we can perhaps add something which will read the nvram contents directly from an nvram store. But that is irrelevant to this patch. The parsing is still needed, and all we would need to add is something which is reading the nvram contents from some other place
>
> So it makes me wonder if we need this patch in its current form. I
> think getting NVRAM directly from the platform is much user friendly.
> It doesn't require user to install some extra tools for dumping NVRAM
> and putting it in a specific file. One extra layer less.
> With that said I think it's hard to review your code for parsing
> NVRAM. We don't know how it's going to be fetched in the first place.

You already made that point and we agreed to look for a solution.

>> though it would have to be put under some kernel config flag as this would not be supported in non-router systems. The contents of the nvram would however still need to be parsed in exactly the same way as the nvram files we read from disk.
>
> Again, it's hard to say for me. Are you going to use
> bcm47xx_nvram_getenv? Are you going to use MTD subsystem? Are you
> going to develop different solution? When using e.g.
> bcm47xx_nvram_getenv you won't want all this parsing stuff at all.

Please look at the usage scenario here. The brcmfmac driver is not 
needing a few key,value pairs. It needs a portion of NVRAM to download 
to the device. The patch provides the functionality to do just that. Get 
the appropriate portion, strip comments and whitespace, and send it to 
the device. Using bcm47xx_nvram_getenv is not a useful api as it would 
mean we need brcmfmac to know which key ids to ask for, reassemble it to 
key=value string and send it to the fullmac device.

In bcm47xx_nvram_getenv() it does have the whole nvram content available 
in nvram_buf which is filled by nvram_init(). So if something similar is 
made available on r8000 (or ARM routers in general) build target all we 
need is an api to get the nvram_buf.

Another option is to add the parsing stuff in that nvram code and have 
an api to get the appropriate portion based on pcie domain and bus 
number as used in brcmf_fw_get_firmwares_pcie(). However, I would prefer 
to have this in the driver and not in arch specific code as there may be 
other platforms like our set-top boxes needing this.

Regards,
Arend

Re: [PATCH 10/10] brcmfmac: Add support for multiple PCIE devices in nvram.

[Rafał Miłecki]

On 17 April 2015 at 10:50, Arend van Spriel <arend@broadcom.com> wrote:
> On 04/17/15 09:45, Rafał Miłecki wrote:
>>
>> Huh, why dropping linux-wireless (and top posting btw)? Please let
>> everyone follow the discussion :)
>>
>> On 15 April 2015 at 21:20, Hante Meuleman<meuleman@broadcom.com>  wrote:
>>>
>>> As I wrote to you in a mail and on the openwrt forum, this patch is
>>> indeed an attempt to support more complex nvram files. I also wrote, that in
>>> order to be able to use it, the nvram contents of the device (r8000) needs
>>> to be put a specific file. Now for your concerns, we can perhaps add
>>> something which will read the nvram contents directly from an nvram store.
>>> But that is irrelevant to this patch. The parsing is still needed, and all
>>> we would need to add is something which is reading the nvram contents from
>>> some other place
>>
>>
>> So it makes me wonder if we need this patch in its current form. I
>> think getting NVRAM directly from the platform is much user friendly.
>> It doesn't require user to install some extra tools for dumping NVRAM
>> and putting it in a specific file. One extra layer less.
>> With that said I think it's hard to review your code for parsing
>> NVRAM. We don't know how it's going to be fetched in the first place.
>
> You already made that point and we agreed to look for a solution.

OK, it wasn't supposed to be rude or anything :) Thanks.


>>> though it would have to be put under some kernel config flag as this
>>> would not be supported in non-router systems. The contents of the nvram
>>> would however still need to be parsed in exactly the same way as the nvram
>>> files we read from disk.
>>
>>
>> Again, it's hard to say for me. Are you going to use
>> bcm47xx_nvram_getenv? Are you going to use MTD subsystem? Are you
>> going to develop different solution? When using e.g.
>> bcm47xx_nvram_getenv you won't want all this parsing stuff at all.
>
>
> Please look at the usage scenario here. The brcmfmac driver is not needing a
> few key,value pairs. It needs a portion of NVRAM to download to the device.
> The patch provides the functionality to do just that. Get the appropriate
> portion, strip comments and whitespace, and send it to the device. Using
> bcm47xx_nvram_getenv is not a useful api as it would mean we need brcmfmac
> to know which key ids to ask for, reassemble it to key=value string and send
> it to the fullmac device.
>
> In bcm47xx_nvram_getenv() it does have the whole nvram content available in
> nvram_buf which is filled by nvram_init(). So if something similar is made
> available on r8000 (or ARM routers in general) build target all we need is
> an api to get the nvram_buf.
>
> Another option is to add the parsing stuff in that nvram code and have an
> api to get the appropriate portion based on pcie domain and bus number as
> used in brcmf_fw_get_firmwares_pcie(). However, I would prefer to have this
> in the driver and not in arch specific code as there may be other platforms
> like our set-top boxes needing this.

This is some plan for the future I was lacking from the beginning. It
makes things more clear now, thanks.

-- 
Rafał

Re: [PATCH 10/10] brcmfmac: Add support for multiple PCIE devices in nvram.

[Arend van Spriel]

On 04/20/15 13:26, Rafał Miłecki wrote:
> On 17 April 2015 at 10:50, Arend van Spriel<arend@broadcom.com>  wrote:
>> On 04/17/15 09:45, Rafał Miłecki wrote:
>>>
>>> Huh, why dropping linux-wireless (and top posting btw)? Please let
>>> everyone follow the discussion :)
>>>
>>> On 15 April 2015 at 21:20, Hante Meuleman<meuleman@broadcom.com>   wrote:
>>>>
>>>> As I wrote to you in a mail and on the openwrt forum, this patch is
>>>> indeed an attempt to support more complex nvram files. I also wrote, that in
>>>> order to be able to use it, the nvram contents of the device (r8000) needs
>>>> to be put a specific file. Now for your concerns, we can perhaps add
>>>> something which will read the nvram contents directly from an nvram store.
>>>> But that is irrelevant to this patch. The parsing is still needed, and all
>>>> we would need to add is something which is reading the nvram contents from
>>>> some other place
>>>
>>>
>>> So it makes me wonder if we need this patch in its current form. I
>>> think getting NVRAM directly from the platform is much user friendly.
>>> It doesn't require user to install some extra tools for dumping NVRAM
>>> and putting it in a specific file. One extra layer less.
>>> With that said I think it's hard to review your code for parsing
>>> NVRAM. We don't know how it's going to be fetched in the first place.
>>
>> You already made that point and we agreed to look for a solution.
>
> OK, it wasn't supposed to be rude or anything :) Thanks.

No problem. Just wanted to make sure we are moving forward.

>>>> though it would have to be put under some kernel config flag as this
>>>> would not be supported in non-router systems. The contents of the nvram
>>>> would however still need to be parsed in exactly the same way as the nvram
>>>> files we read from disk.
>>>
>>>
>>> Again, it's hard to say for me. Are you going to use
>>> bcm47xx_nvram_getenv? Are you going to use MTD subsystem? Are you
>>> going to develop different solution? When using e.g.
>>> bcm47xx_nvram_getenv you won't want all this parsing stuff at all.
>>
>>
>> Please look at the usage scenario here. The brcmfmac driver is not needing a
>> few key,value pairs. It needs a portion of NVRAM to download to the device.
>> The patch provides the functionality to do just that. Get the appropriate
>> portion, strip comments and whitespace, and send it to the device. Using
>> bcm47xx_nvram_getenv is not a useful api as it would mean we need brcmfmac
>> to know which key ids to ask for, reassemble it to key=value string and send
>> it to the fullmac device.
>>
>> In bcm47xx_nvram_getenv() it does have the whole nvram content available in
>> nvram_buf which is filled by nvram_init(). So if something similar is made
>> available on r8000 (or ARM routers in general) build target all we need is
>> an api to get the nvram_buf.
>>
>> Another option is to add the parsing stuff in that nvram code and have an
>> api to get the appropriate portion based on pcie domain and bus number as
>> used in brcmf_fw_get_firmwares_pcie(). However, I would prefer to have this
>> in the driver and not in arch specific code as there may be other platforms
>> like our set-top boxes needing this.
>
> This is some plan for the future I was lacking from the beginning. It
> makes things more clear now, thanks.

You're welcome. Do you want to see this clarification in the commit message?

Regards,
Arend

Re: [PATCH 10/10] brcmfmac: Add support for multiple PCIE devices in nvram.

[Rafał Miłecki]

On 20 April 2015 at 19:12, Arend van Spriel <arend@broadcom.com> wrote:
> On 04/20/15 13:26, Rafał Miłecki wrote:
>>
>> On 17 April 2015 at 10:50, Arend van Spriel<arend@broadcom.com>  wrote:
>>> Another option is to add the parsing stuff in that nvram code and have an
>>> api to get the appropriate portion based on pcie domain and bus number as
>>> used in brcmf_fw_get_firmwares_pcie(). However, I would prefer to have
>>> this
>>> in the driver and not in arch specific code as there may be other
>>> platforms
>>> like our set-top boxes needing this.
>>
>>
>> This is some plan for the future I was lacking from the beginning. It
>> makes things more clear now, thanks.
>
> You're welcome. Do you want to see this clarification in the commit message?

I don't really need that, I'm leaving it up to you :)

The last remaining question from me is if  about this NVRAM validation
function (if it exists).

-- 
Rafał

Re: [PATCH 10/10] brcmfmac: Add support for multiple PCIE devices in nvram.

[Arend van Spriel]

On 04/20/15 20:49, Rafał Miłecki wrote:
> On 20 April 2015 at 19:12, Arend van Spriel<arend@broadcom.com>  wrote:
>> On 04/20/15 13:26, Rafał Miłecki wrote:
>>>
>>> On 17 April 2015 at 10:50, Arend van Spriel<arend@broadcom.com>   wrote:
>>>> Another option is to add the parsing stuff in that nvram code and have an
>>>> api to get the appropriate portion based on pcie domain and bus number as
>>>> used in brcmf_fw_get_firmwares_pcie(). However, I would prefer to have
>>>> this
>>>> in the driver and not in arch specific code as there may be other
>>>> platforms
>>>> like our set-top boxes needing this.
>>>
>>>
>>> This is some plan for the future I was lacking from the beginning. It
>>> makes things more clear now, thanks.
>>
>> You're welcome. Do you want to see this clarification in the commit message?
>
> I don't really need that, I'm leaving it up to you :)
>
> The last remaining question from me is if  about this NVRAM validation
> function (if it exists).

Ok. I can try to answer that one. The nvram parsing code in firmware.c 
intends to do just that. So apart from stripping comments and whitespace 
it validates each line and gives off a warning if a wrongly formatted 
entry is found.

Regards,
Arend

Re: [PATCH 10/10] brcmfmac: Add support for multiple PCIE devices in nvram.

[Rafał Miłecki]

On 20 April 2015 at 22:16, Arend van Spriel <arend@broadcom.com> wrote:
> On 04/20/15 20:49, Rafał Miłecki wrote:
>>
>> On 20 April 2015 at 19:12, Arend van Spriel<arend@broadcom.com>  wrote:
>>>
>>> On 04/20/15 13:26, Rafał Miłecki wrote:
>>>>
>>>>
>>>> On 17 April 2015 at 10:50, Arend van Spriel<arend@broadcom.com>   wrote:
>>>>>
>>>>> Another option is to add the parsing stuff in that nvram code and have
>>>>> an
>>>>> api to get the appropriate portion based on pcie domain and bus number
>>>>> as
>>>>> used in brcmf_fw_get_firmwares_pcie(). However, I would prefer to have
>>>>> this
>>>>> in the driver and not in arch specific code as there may be other
>>>>> platforms
>>>>> like our set-top boxes needing this.
>>>>
>>>>
>>>>
>>>> This is some plan for the future I was lacking from the beginning. It
>>>> makes things more clear now, thanks.
>>>
>>>
>>> You're welcome. Do you want to see this clarification in the commit
>>> message?
>>
>>
>> I don't really need that, I'm leaving it up to you :)
>>
>> The last remaining question from me is if  about this NVRAM validation
>> function (if it exists).
>
>
> Ok. I can try to answer that one. The nvram parsing code in firmware.c
> intends to do just that. So apart from stripping comments and whitespace it
> validates each line and gives off a warning if a wrongly formatted entry is
> found.

OK, thanks. So I guess this patch is OK after all?

-- 
Rafał

Re: [PATCH 10/10] brcmfmac: Add support for multiple PCIE devices in nvram.

[Arend van Spriel]

On 04/21/15 12:16, Rafał Miłecki wrote:
> On 20 April 2015 at 22:16, Arend van Spriel<arend@broadcom.com>  wrote:
>> On 04/20/15 20:49, Rafał Miłecki wrote:
>>>
>>> On 20 April 2015 at 19:12, Arend van Spriel<arend@broadcom.com>   wrote:
>>>>
>>>> On 04/20/15 13:26, Rafał Miłecki wrote:
>>>>>
>>>>>
>>>>> On 17 April 2015 at 10:50, Arend van Spriel<arend@broadcom.com>    wrote:
>>>>>>
>>>>>> Another option is to add the parsing stuff in that nvram code and have
>>>>>> an
>>>>>> api to get the appropriate portion based on pcie domain and bus number
>>>>>> as
>>>>>> used in brcmf_fw_get_firmwares_pcie(). However, I would prefer to have
>>>>>> this
>>>>>> in the driver and not in arch specific code as there may be other
>>>>>> platforms
>>>>>> like our set-top boxes needing this.
>>>>>
>>>>>
>>>>>
>>>>> This is some plan for the future I was lacking from the beginning. It
>>>>> makes things more clear now, thanks.
>>>>
>>>>
>>>> You're welcome. Do you want to see this clarification in the commit
>>>> message?
>>>
>>>
>>> I don't really need that, I'm leaving it up to you :)
>>>
>>> The last remaining question from me is if  about this NVRAM validation
>>> function (if it exists).
>>
>>
>> Ok. I can try to answer that one. The nvram parsing code in firmware.c
>> intends to do just that. So apart from stripping comments and whitespace it
>> validates each line and gives off a warning if a wrongly formatted entry is
>> found.
>
> OK, thanks. So I guess this patch is OK after all?

Until proven otherwise. I tested it with a number of format violations, 
but there could be more creative people out there coming up with a 
pattern uncovering some issue.

Regards,
Arend

Re: [PATCH 10/10] brcmfmac: Add support for multiple PCIE devices in nvram.

[Rafał Miłecki]

On 14 April 2015 at 20:10, Arend van Spriel <arend@broadcom.com> wrote:
> @@ -23,6 +23,10 @@
>  #include "debug.h"
>  #include "firmware.h"
>
> +#define BRCMF_FW_MAX_NVRAM_SIZE                        64000
> +#define BRCMF_FW_NVRAM_DEVPATH_LEN             19      /* devpath0=pcie/1/4/ */
> +#define BRCMF_FW_NVRAM_PCIEDEV_LEN             9       /* pcie/1/4/ */

You most likely know what you're doing, but just for sure: are there
any devices using "pcie/" prefix *without* a devpath? I got an
impression that Broadcom first started using devpath-s and then
(later) started using "pcie/" prefix (instead of "pci/").


> @@ -192,12 +214,136 @@ static int brcmf_init_nvram_parser(struct nvram_parser *nvp,
>         return 0;
>  }
>
> +/* brcmf_fw_strip_multi_v1 :Some nvram files contain settings for multiple
> + * devices. Strip it down for one device, use domain_nr/bus_nr to determine
> + * which data is to be returned. v1 is the version where nvram is stored
> + * compressed and "devpath" maps to index for valid entries.
> + */
> +static void brcmf_fw_strip_multi_v1(struct nvram_parser *nvp, u16 domain_nr,
> +                                   u16 bus_nr)
> +{
> +       u32 i, j;
> +       bool found;
> +       u8 *nvram;
> +       u8 id;
> +
> +       nvram = kzalloc(nvp->nvram_len + 1 + 3 + sizeof(u32), GFP_KERNEL);
> +       if (!nvram)
> +               goto fail;
> +
> +       /* min length: devpath0=pcie/1/4/ + 0:x=y */
> +       if (nvp->nvram_len < BRCMF_FW_NVRAM_DEVPATH_LEN + 6)
> +               goto fail;
> +
> +       /* First search for the devpathX and see if it is the configuration
> +        * for domain_nr/bus_nr. Search complete nvp
> +        */
> +       found = false;
> +       i = 0;
> +       while (i < nvp->nvram_len - BRCMF_FW_NVRAM_DEVPATH_LEN) {
> +               /* Format: devpathX=pcie/Y/Z/
> +                * Y = domain_nr, Z = bus_nr, X = virtual ID
> +                */
> +               if ((strncmp(&nvp->nvram[i], "devpath", 7) == 0) &&
> +                   (strncmp(&nvp->nvram[i + 8], "=pcie/", 6) == 0)) {
> +                       if (((nvp->nvram[i + 14] - '0') == domain_nr) &&
> +                           ((nvp->nvram[i + 16] - '0') == bus_nr)) {
> +                               id = nvp->nvram[i + 7] - '0';
> +                               found = true;
> +                               break;
> +                       }
> +               }
> +               while (nvp->nvram[i] != 0)
> +                       i++;

I guess while should also check nvp->nvram_len to avoid crashing with
malformed NVRAM content. Same in _v2 function.


> +               i++;
> +       }
> +       if (!found)
> +               goto fail;
> +
> +       /* Now copy all valid entries, release old nvram and assign new one */
> +       i = 0;
> +       j = 0;
> +       while (i < nvp->nvram_len) {
> +               if ((nvp->nvram[i] - '0' == id) && (nvp->nvram[i + 1] == ':')) {
> +                       i += 2;
> +                       while (nvp->nvram[i] != 0) {
> +                               nvram[j] = nvp->nvram[i];
> +                               i++;
> +                               j++;
> +                       }
> +                       nvram[j] = 0;
> +                       j++;
> +               }
> +               while (nvp->nvram[i] != 0)
> +                       i++;

Same here.


> +/* brcmf_fw_strip_multi_v2 :Some nvram files contain settings for multiple
> + * devices. Strip it down for one device, use domain_nr/bus_nr to determine
> + * which data is to be returned. v2 is the version where nvram is stored
> + * uncompressed, all relevant valid entries are identified by
> + * pcie/domain_nr/bus_nr:
> + */
> +static void brcmf_fw_strip_multi_v2(struct nvram_parser *nvp, u16 domain_nr,
> +                                   u16 bus_nr)
> +{
> +       u32 i, j;
> +       u8 *nvram;
> +
> +       nvram = kzalloc(nvp->nvram_len + 1 + 3 + sizeof(u32), GFP_KERNEL);
> +       if (!nvram)
> +               goto fail;
> +
> +       /* Copy all valid entries, release old nvram and assign new one.
> +        * Valid entries are of type pcie/X/Y/ where X = domain_nr and
> +        * Y = bus_nr.
> +        */
> +       i = 0;
> +       j = 0;
> +       while (i < nvp->nvram_len - BRCMF_FW_NVRAM_PCIEDEV_LEN) {
> +               if ((strncmp(&nvp->nvram[i], "pcie/", 5) == 0) &&
> +                   (nvp->nvram[i + 6] == '/') && (nvp->nvram[i + 8] == '/') &&
> +                   ((nvp->nvram[i + 5] - '0') == domain_nr) &&
> +                   ((nvp->nvram[i + 7] - '0') == bus_nr)) {
> +                       i += BRCMF_FW_NVRAM_PCIEDEV_LEN;
> +                       while (nvp->nvram[i] != 0) {
> +                               nvram[j] = nvp->nvram[i];
> +                               i++;
> +                               j++;
> +                       }
> +                       nvram[j] = 0;
> +                       j++;
> +               }
> +               while (nvp->nvram[i] != 0)
> +                       i++;
> +               i++;
> +       }
> +       kfree(nvp->nvram);
> +       nvp->nvram = nvram;
> +       nvp->nvram_len = j;
> +       return;
> +fail:
> +       kfree(nvram);
> +       nvp->nvram_len = 0;
> +}

I guess you could consider implementing a one generic function that
will accept sth like a "char *prefix" argument. First check for a
devpathX=$prefix entry and use X: instead of prefix: or not.

Re: [PATCH 10/10] brcmfmac: Add support for multiple PCIE devices in nvram.

[Rafał Miłecki]

On 14 April 2015 at 20:10, Arend van Spriel <arend@broadcom.com> wrote:
> +int brcmf_fw_get_firmwares(struct device *dev, u16 flags,
> +                          const char *code, const char *nvram,
> +                          void (*fw_cb)(struct device *dev,
> +                                        const struct firmware *fw,
> +                                        void *nvram_image, u32 nvram_len))
> +{
> +       return brcmf_fw_get_firmwares_pcie(dev, flags, code, nvram, fw_cb, 0,
> +                                          0);
> +}
> +

git complains about adding an extra (2nd) empty line at EOF

Re: [PATCH 00/10] brcmfmac: device support and fixes

[Kalle Valo]

Hi Arend,

Arend van Spriel <arend@broadcom.com> writes:

> This series include some fixes related to SDIO suspend and resume,
> wiphy band info and changes in regulatory settings. Furthermore,
> support for BCM4324 SDIO and BCM4358 PCIe is added. Finally,
> some patches from Hante that should enable support of PCIe devices
> on router platforms.

Your patch series are always top notch but I still would like to ask for
one small formatting change :) It would make my life easier if you could
list the major changes like this:

* SDIO suspend and resume fixes
* wiphy band info and changes in regulatory settings
* add support for BCM4324 SDIO and BCM4358 PCIe
* enable support of PCIe devices on router platforms (Hante)

That way I could directly copy your list to the pull request I send to
Dave.

-- 
Kalle Valo

Re: [PATCH 00/10] brcmfmac: device support and fixes

[Arend van Spriel]

On 05/09/15 16:21, Kalle Valo wrote:
> Hi Arend,
>
> Arend van Spriel<arend@broadcom.com>  writes:
>
>> This series include some fixes related to SDIO suspend and resume,
>> wiphy band info and changes in regulatory settings. Furthermore,
>> support for BCM4324 SDIO and BCM4358 PCIe is added. Finally,
>> some patches from Hante that should enable support of PCIe devices
>> on router platforms.
>
> Your patch series are always top notch but I still would like to ask for
> one small formatting change :) It would make my life easier if you could
> list the major changes like this:
>
> * SDIO suspend and resume fixes
> * wiphy band info and changes in regulatory settings
> * add support for BCM4324 SDIO and BCM4358 PCIe
> * enable support of PCIe devices on router platforms (Hante)
>
> That way I could directly copy your list to the pull request I send to
> Dave.

No problem. We want our lives to be as easy as possible ;-)

Regards,
Arend