Re: [PATCH] libertas: call into generic suspend code before turning off power

[Ulf Hansson]

On 9 October 2018 at 15:36, Kalle Valo  wrote:
> Ulf Hansson  writes:
>
>> On 8 October 2018 at 22:03, Daniel Mack  wrote:
>>> When powering down a SDIO connected card during suspend, make sure to call
>>> into the generic lbs_suspend() function before pulling the plug. This will
>>> make sure the card is successfully deregistered from the system to avoid
>>> communication to the card starving out.
>>>
>>> Fixes: 7444a8092906 ("libertas: fix suspend and resume for SDIO connected 
>>> cards")
>>> Signed-off-by: Daniel Mack 
>>
>> Reviewed-by: Ulf Hansson 
>>
>> BTW, if you need this to reach 4.19 I have already queued some other
>> mmc fixes so I can take this as well, if it helps. I need and ack of
>> course.
>
> I'm not planning to send anything to 4.19 anymore and this is so simple
> that hopefully it cause any conflicts with -next patches, so feel free
> to do that:
>
> Acked-by: Kalle Valo 

Applied for fixes, thanks!

Kind regards
Uffe

Re: [PATCH] libertas: call into generic suspend code before turning off power

[Ulf Hansson]

On 8 October 2018 at 22:03, Daniel Mack  wrote:
> When powering down a SDIO connected card during suspend, make sure to call
> into the generic lbs_suspend() function before pulling the plug. This will
> make sure the card is successfully deregistered from the system to avoid
> communication to the card starving out.
>
> Fixes: 7444a8092906 ("libertas: fix suspend and resume for SDIO connected 
> cards")
> Signed-off-by: Daniel Mack 

Reviewed-by: Ulf Hansson 

BTW, if you need this to reach 4.19 I have already queued some other
mmc fixes so I can take this as well, if it helps. I need and ack of
course.

Kind regards
Uffe

> ---
> If possible, this should go in for 4.19.
>
>  drivers/net/wireless/marvell/libertas/if_sdio.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/wireless/marvell/libertas/if_sdio.c 
> b/drivers/net/wireless/marvell/libertas/if_sdio.c
> index 43743c26c071..39bf85d0ade0 100644
> --- a/drivers/net/wireless/marvell/libertas/if_sdio.c
> +++ b/drivers/net/wireless/marvell/libertas/if_sdio.c
> @@ -1317,6 +1317,10 @@ static int if_sdio_suspend(struct device *dev)
> if (priv->wol_criteria == EHS_REMOVE_WAKEUP) {
> dev_info(dev, "Suspend without wake params -- powering down 
> card\n");
> if (priv->fw_ready) {
> +   ret = lbs_suspend(priv);
> +   if (ret)
> +   return ret;
> +
> priv->power_up_on_resume = true;
> if_sdio_power_off(card);
> }
> --
> 2.17.1
>

Re: [PATCH v2] libertas: fix suspend and resume for SDIO connected cards

[Ulf Hansson]

On 2 July 2018 at 16:57, Kalle Valo  wrote:
> Daniel Mack  writes:
>
>> On Friday, June 29, 2018 05:39 PM, Ulf Hansson wrote:
>>> On 27 June 2018 at 20:58, Daniel Mack  wrote:
>>>> Prior to commit 573185cc7e64 ("mmc: core: Invoke sdio func driver's PM
>>>> callbacks from the sdio bus"), the MMC core used to call into the power
>>>> management functions of SDIO clients itself and removed the card if the
>>>> return code was non-zero. IOW, the mmc handled errors gracefully and didn't
>>>> upchain them to the pm core.
>>>>
>>>> Since this change, the mmc core relies on generic power management
>>>> functions which treat all errors as a reason to cancel the suspend
>>>> immediately. This causes suspend attempts to fail when the libertas
>>>> driver is loaded.
>>>>
>>>> To fix this, power down the card explicitly in if_sdio_suspend() when we
>>>> know we're about to lose power and return success. Also set a flag in these
>>>> cases, and power up the card again in if_sdio_resume().
>>>>
>>>> Signed-off-by: Daniel Mack 
>>>> Cc: Ulf Hansson 
>>>> Cc: Chris Ball 
>>>
>>> Looks good to me! Should be a candidate for stable as well!?
>>
>> Thanks!
>>
>> Yeah, it should probably get a
>>
>> Fixes: 573185cc7e64 ("mmc: core: Invoke sdio func driver's PM
>> callbacks from the sdio bus")
>>
>> as well.
>
> So I'll queue this for wireless-drivers-next and add:
>
> Fixes: 573185cc7e64 ("mmc: core: Invoke sdio func driver's PM callbacks from 
> the sdio bus")
> Cc: 
> Ok?

That's fine by me.

Kind regards
Uffe

Re: [PATCH v2] libertas: fix suspend and resume for SDIO connected cards

[Ulf Hansson]

On 27 June 2018 at 20:58, Daniel Mack  wrote:
> Prior to commit 573185cc7e64 ("mmc: core: Invoke sdio func driver's PM
> callbacks from the sdio bus"), the MMC core used to call into the power
> management functions of SDIO clients itself and removed the card if the
> return code was non-zero. IOW, the mmc handled errors gracefully and didn't
> upchain them to the pm core.
>
> Since this change, the mmc core relies on generic power management
> functions which treat all errors as a reason to cancel the suspend
> immediately. This causes suspend attempts to fail when the libertas
> driver is loaded.
>
> To fix this, power down the card explicitly in if_sdio_suspend() when we
> know we're about to lose power and return success. Also set a flag in these
> cases, and power up the card again in if_sdio_resume().
>
> Signed-off-by: Daniel Mack 
> Cc: Ulf Hansson 
> Cc: Chris Ball 

Looks good to me! Should be a candidate for stable as well!?

Reviewed-by: Ulf Hansson 

I have some additional related changes in mind for the libertas SDIO
driver, however let me post patches for us to discuss around instead.

Kind regards
Uffe

> ---
> v1 → v2:
> * Reworded patch subject
> * Added wait_event(card->pwron_waitq, card->priv->fw_ready)
>
>  drivers/net/wireless/marvell/libertas/dev.h   |  1 +
>  .../net/wireless/marvell/libertas/if_sdio.c   | 30 +++
>  2 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/libertas/dev.h 
> b/drivers/net/wireless/marvell/libertas/dev.h
> index dd1ee1f0af48..469134930026 100644
> --- a/drivers/net/wireless/marvell/libertas/dev.h
> +++ b/drivers/net/wireless/marvell/libertas/dev.h
> @@ -104,6 +104,7 @@ struct lbs_private {
> u8 fw_ready;
> u8 surpriseremoved;
> u8 setup_fw_on_resume;
> +   u8 power_up_on_resume;
> int (*hw_host_to_card) (struct lbs_private *priv, u8 type, u8 
> *payload, u16 nb);
> void (*reset_card) (struct lbs_private *priv);
> int (*power_save) (struct lbs_private *priv);
> diff --git a/drivers/net/wireless/marvell/libertas/if_sdio.c 
> b/drivers/net/wireless/marvell/libertas/if_sdio.c
> index 2300e796c6ab..43743c26c071 100644
> --- a/drivers/net/wireless/marvell/libertas/if_sdio.c
> +++ b/drivers/net/wireless/marvell/libertas/if_sdio.c
> @@ -1290,15 +1290,23 @@ static void if_sdio_remove(struct sdio_func *func)
>  static int if_sdio_suspend(struct device *dev)
>  {
> struct sdio_func *func = dev_to_sdio_func(dev);
> -   int ret;
> struct if_sdio_card *card = sdio_get_drvdata(func);
> +   struct lbs_private *priv = card->priv;
> +   int ret;
>
> mmc_pm_flag_t flags = sdio_get_host_pm_caps(func);
> +   priv->power_up_on_resume = false;
>
> /* If we're powered off anyway, just let the mmc layer remove the
>  * card. */
> -   if (!lbs_iface_active(card->priv))
> -   return -ENOSYS;
> +   if (!lbs_iface_active(priv)) {
> +   if (priv->fw_ready) {
> +   priv->power_up_on_resume = true;
> +   if_sdio_power_off(card);
> +   }
> +
> +   return 0;
> +   }
>
> dev_info(dev, "%s: suspend: PM flags = 0x%x\n",
>  sdio_func_id(func), flags);
> @@ -1306,9 +1314,14 @@ static int if_sdio_suspend(struct device *dev)
> /* If we aren't being asked to wake on anything, we should bail out
>  * and let the SD stack power down the card.
>  */
> -   if (card->priv->wol_criteria == EHS_REMOVE_WAKEUP) {
> +   if (priv->wol_criteria == EHS_REMOVE_WAKEUP) {
> dev_info(dev, "Suspend without wake params -- powering down 
> card\n");
> -   return -ENOSYS;
> +   if (priv->fw_ready) {
> +   priv->power_up_on_resume = true;
> +   if_sdio_power_off(card);
> +   }
> +
> +   return 0;
> }
>
> if (!(flags & MMC_PM_KEEP_POWER)) {
> @@ -1321,7 +1334,7 @@ static int if_sdio_suspend(struct device *dev)
> if (ret)
> return ret;
>
> -   ret = lbs_suspend(card->priv);
> +   ret = lbs_suspend(priv);
> if (ret)
> return ret;
>
> @@ -1336,6 +1349,11 @@ static int if_sdio_resume(struct device *dev)
>
> dev_info(dev, "%s: resume: we're back\n", sdio_func_id(func));
>
> +   if (card->priv->power_up_on_resume) {
> +   if_sdio_power_on(card);
> +   wait_event(card->pwron_waitq, card->priv->fw_ready);
> +   }
> +
> ret = lbs_resume(card->priv);
>
> return ret;
> --
> 2.17.1
>

Re: [PATCH] libertas: fix return codes in if_sdio_suspend()

[Ulf Hansson]

On 27 June 2018 at 10:31, Daniel Mack  wrote:
> On Wednesday, June 27, 2018 09:54 AM, Ulf Hansson wrote:
>>
>> On 26 June 2018 at 22:50, Daniel Mack  wrote:
>>>
>>> On Tuesday, June 26, 2018 10:48 PM, Daniel Mack wrote:
>
>
>>>> @@ -1321,7 +1334,7 @@ static int if_sdio_suspend(struct device *dev)
>>>>  if (ret)
>>>>  return ret;
>>>>- ret = lbs_suspend(card->priv);
>>>> +   ret = lbs_suspend(priv);
>>>>  if (ret)
>>>>  return ret;
>>>>@@ -1336,6 +1349,9 @@ static int if_sdio_resume(struct device *dev)
>>>>  dev_info(dev, "%s: resume: we're back\n", sdio_func_id(func));
>>>>+ if (card->priv->power_up_on_resume)
>>>> +   if_sdio_power_on(card);
>>>> +
>>
>>
>> To guarantee firmware is loaded, don't you need the below as well?
>>
>> wait_event(card->pwron_waitq, priv->fw_ready);
>
>
> Hmm, yes. I should probably just be calling into if_sdio_power_restore().

You don't want to mess up the runtime PM counter though.

>
> I'll test that and resend the patch.

Great!

Kind regards
Uffe

Re: [PATCH] libertas: fix return codes in if_sdio_suspend()

[Ulf Hansson]

On 26 June 2018 at 22:50, Daniel Mack  wrote:
> Ah, the subject line of this patch could actually be improved. Let me know
> if you're happy with the contents of this patch, so I can resend.
>
> Thanks,
> Daniel
>
>
>
> On Tuesday, June 26, 2018 10:48 PM, Daniel Mack wrote:
>>
>> Prior to commit 573185cc7e64 ("mmc: core: Invoke sdio func driver's PM
>> callbacks from the sdio bus"), the MMC core used to call into the power
>> management functions of SDIO clients itself and removed the card if the
>> return code was non-zero. IOW, the mmc handled errors gracefully and
>> didn't
>> upchain them to the pm core.
>>
>> Since this change, the mmc core relies on generic power management
>> functions which treat all errors as a reason to cancel the suspend
>> immediately. This causes suspend attempts to fail when the libertas
>> driver is loaded.
>>
>> To fix this, power down the card explicitly in if_sdio_suspend() when we
>> know we're about to lose power and return success. Also set a flag in
>> these
>> cases, and power up the card again in if_sdio_resume().
>>
>> Signed-off-by: Daniel Mack 
>> Cc: Ulf Hansson 
>> Cc: Chris Ball 
>> ---
>>   drivers/net/wireless/marvell/libertas/dev.h   |  1 +
>>   .../net/wireless/marvell/libertas/if_sdio.c   | 28 +++
>>   2 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/wireless/marvell/libertas/dev.h
>> b/drivers/net/wireless/marvell/libertas/dev.h
>> index dd1ee1f0af48..469134930026 100644
>> --- a/drivers/net/wireless/marvell/libertas/dev.h
>> +++ b/drivers/net/wireless/marvell/libertas/dev.h
>> @@ -104,6 +104,7 @@ struct lbs_private {
>> u8 fw_ready;
>> u8 surpriseremoved;
>> u8 setup_fw_on_resume;
>> +   u8 power_up_on_resume;
>> int (*hw_host_to_card) (struct lbs_private *priv, u8 type, u8
>> *payload, u16 nb);
>> void (*reset_card) (struct lbs_private *priv);
>> int (*power_save) (struct lbs_private *priv);
>> diff --git a/drivers/net/wireless/marvell/libertas/if_sdio.c
>> b/drivers/net/wireless/marvell/libertas/if_sdio.c
>> index 2300e796c6ab..f43807663b1b 100644
>> --- a/drivers/net/wireless/marvell/libertas/if_sdio.c
>> +++ b/drivers/net/wireless/marvell/libertas/if_sdio.c
>> @@ -1290,15 +1290,23 @@ static void if_sdio_remove(struct sdio_func *func)
>>   static int if_sdio_suspend(struct device *dev)
>>   {
>> struct sdio_func *func = dev_to_sdio_func(dev);
>> -   int ret;
>> struct if_sdio_card *card = sdio_get_drvdata(func);
>> +   struct lbs_private *priv = card->priv;
>> +   int ret;
>> mmc_pm_flag_t flags = sdio_get_host_pm_caps(func);
>> +   priv->power_up_on_resume = false;
>> /* If we're powered off anyway, just let the mmc layer remove the
>>  * card. */
>> -   if (!lbs_iface_active(card->priv))
>> -   return -ENOSYS;
>> +   if (!lbs_iface_active(priv)) {
>> +   if (priv->fw_ready) {
>> +   priv->power_up_on_resume = true;
>> +   if_sdio_power_off(card);
>> +   }
>> +
>> +   return 0;
>> +   }
>> dev_info(dev, "%s: suspend: PM flags = 0x%x\n",
>>  sdio_func_id(func), flags);
>> @@ -1306,9 +1314,14 @@ static int if_sdio_suspend(struct device *dev)
>> /* If we aren't being asked to wake on anything, we should bail
>> out
>>  * and let the SD stack power down the card.
>>  */
>> -   if (card->priv->wol_criteria == EHS_REMOVE_WAKEUP) {
>> +   if (priv->wol_criteria == EHS_REMOVE_WAKEUP) {
>> dev_info(dev, "Suspend without wake params -- powering
>> down card\n");
>> -   return -ENOSYS;
>> +   if (priv->fw_ready) {
>> +   priv->power_up_on_resume = true;
>> +   if_sdio_power_off(card);
>> +   }
>> +
>> +   return 0;
>> }
>> if (!(flags & MMC_PM_KEEP_POWER)) {
>> @@ -1321,7 +1334,7 @@ static int if_sdio_suspend(struct device *dev)
>> if (ret)
>> return ret;
>>   - ret = lbs_suspend(card->priv);
>> +   ret = lbs_suspend(priv);
>> if (ret)
>> return ret;
>>   @@ -1336,6 +1349,9 @@ static int if_sdio_resume(struct device *dev)
>> dev_info(dev, "%s: resume: we're back\n", sdio_func_id(func));
>>   + if (card->priv->power_up_on_resume)
>> +   if_sdio_power_on(card);
>> +

To guarantee firmware is loaded, don't you need the below as well?

wait_event(card->pwron_waitq, priv->fw_ready);

>> ret = lbs_resume(card->priv);
>> return ret;
>>
>

Kind regards
Uffe

Re: [PATCH] brcmfmac: Add support for bcm43364 wireless chipset

[Ulf Hansson]

On 4 May 2018 at 08:48, Sean Lanigan <s...@lano.id.au> wrote:
> Add support for the BCM43364 chipset via an SDIO interface, as used in
> e.g. the Murata 1FX module.
>
> The BCM43364 uses the same firmware as the BCM43430 (which is already
> included), the only difference is the omission of Bluetooth.
>
> However, the SDIO_ID for the BCM43364 is 02D0:A9A4, giving it a MODALIAS
> of sdio:c00v02D0dA9A4, which doesn't get recognised and hence doesn't
> load the brcmfmac module. Adding the 'A9A4' ID in the appropriate place
> triggers the brcmfmac driver to load, and then correctly use the
> firmware file 'brcmfmac43430-sdio.bin'.
>
>
> Signed-off-by: Sean Lanigan <s...@lano.id.au>

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

Arend, I assume you want to pick this up? If not, just tell and I can do it.

Kind regards
Uffe

> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 1 +
>  include/linux/mmc/sdio_ids.h  | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index 0b68240..4648a3d 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -963,6 +963,7 @@ static const struct sdio_device_id brcmf_sdmmc_ids[] = {
> BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_43340),
> BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_43341),
> BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_43362),
> +   BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_43364),
> BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_4335_4339),
> BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_4339),
> BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_43430),
> diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h
> index cdd66a5..0a7abe8 100644
> --- a/include/linux/mmc/sdio_ids.h
> +++ b/include/linux/mmc/sdio_ids.h
> @@ -35,6 +35,7 @@
>  #define SDIO_DEVICE_ID_BROADCOM_4335_4339  0x4335
>  #define SDIO_DEVICE_ID_BROADCOM_4339   0x4339
>  #define SDIO_DEVICE_ID_BROADCOM_43362  0xa962
> +#define SDIO_DEVICE_ID_BROADCOM_43364  0xa9a4
>  #define SDIO_DEVICE_ID_BROADCOM_43430  0xa9a6
>  #define SDIO_DEVICE_ID_BROADCOM_4345   0x4345
>  #define SDIO_DEVICE_ID_BROADCOM_43455  0xa9bf
> --
> 2.7.4
>

Re: [PATCH net-next v2 2/2] dt: bindings: add new dt entries for brcmfmac

[Ulf Hansson]

On 5 April 2018 at 15:10, Kalle Valo <kv...@codeaurora.org> wrote:
> Ulf Hansson <ulf.hans...@linaro.org> writes:
>
>> On 20 March 2018 at 10:55, Kalle Valo <kv...@codeaurora.org> wrote:
>>> Arend van Spriel <arend.vanspr...@broadcom.com> writes:
>>>
>>>>>> If I get it right, you mean something like this:
>>>>>>
>>>>>> mmc3: mmc@1c12000 {
>>>>>> ...
>>>>>>  broken-sg-support;
>>>>>>  sd-head-align = 4;
>>>>>>  sd-sgentry-align = 512;
>>>>>>
>>>>>>  brcmf: wifi@1 {
>>>>>>  ...
>>>>>>  };
>>>>>> };
>>>>>>
>>>>>> Where dt: bindings documentation for these entries should reside?
>>>>>> In generic MMC bindings? Well, this is the very special case and
>>>>>> mmc-linux maintainer will unlikely to accept these changes.
>>>>>> Also, extra kernel code modification might be required. It could make
>>>>>> quite trivial change much more complex.
>>>>>
>>>>> If the MMC maintainers are not copied on this patch series, it will
>>>>> likely be hard for them to identify this patch series and chime in...
>>>>
>>>> The main question is whether this is indeed a "very special case" as
>>>> Alexey claims it to be or that it is likely to be applicable to other
>>>> device and host combinations as you are suggesting.
>>>>
>>>> If these properties are imposed by the host or host controller it
>>>> would make sense to have these in the mmc bindings.
>>>
>>> BTW, last year we were discussing something similar (I mean related to
>>> alignment requirements) with ath10k SDIO patches and at the time the
>>> patch submitter was proposing to have a bounce buffer in ath10k to
>>> workaround that. I don't remember the details anymore, they are on the
>>> ath10k mailing list archive if anyone is curious to know, but I would
>>> not be surprised if they are similar as here. So there might be a need
>>> to solve this in a generic way (but not sure of course as I haven't
>>> checked the details).
>>
>> I re-call something about these as well, here are the patches. Perhaps
>> I should pick some of them up...
>>
>> https://patchwork.kernel.org/patch/10123137/
>> https://patchwork.kernel.org/patch/10123139/
>> https://patchwork.kernel.org/patch/10123141/
>> https://patchwork.kernel.org/patch/10123143/
>
> Actually I was talking about a different patch, found it now:
>
> ath10k_sdio: DMA bounce buffers for read write
>
> https://patchwork.kernel.org/patch/9979543/

Ah, yes. This is about buffer alignment, particularly when using DMA.

Normally there should be no constraint on the alignment, if the
mmc/sdio controller driver would implement a fallback mechanism from
DMA to PIO mode, in case the buffer can't be used for DMA.

However, I know about cases where simply PIO doesn't work because of
broken HW and in many cases the mmc drivers don't implement the
fallback to PIO even if they could.

Moreover, it seems reasonable to anyway have a way for mmc driver to
inform upper layers about DMA buffer alignment constraints, as to be
able to use DMA as long as possible.

Thoughts?

Kind regards
Uffe

Re: [PATCH net-next v2 2/2] dt: bindings: add new dt entries for brcmfmac

[Ulf Hansson]

On 20 March 2018 at 10:55, Kalle Valo  wrote:
> Arend van Spriel  writes:
>
 If I get it right, you mean something like this:

 mmc3: mmc@1c12000 {
 ...
  broken-sg-support;
  sd-head-align = 4;
  sd-sgentry-align = 512;

  brcmf: wifi@1 {
  ...
  };
 };

 Where dt: bindings documentation for these entries should reside?
 In generic MMC bindings? Well, this is the very special case and
 mmc-linux maintainer will unlikely to accept these changes.
 Also, extra kernel code modification might be required. It could make
 quite trivial change much more complex.
>>>
>>> If the MMC maintainers are not copied on this patch series, it will
>>> likely be hard for them to identify this patch series and chime in...
>>
>> The main question is whether this is indeed a "very special case" as
>> Alexey claims it to be or that it is likely to be applicable to other
>> device and host combinations as you are suggesting.
>>
>> If these properties are imposed by the host or host controller it
>> would make sense to have these in the mmc bindings.
>
> BTW, last year we were discussing something similar (I mean related to
> alignment requirements) with ath10k SDIO patches and at the time the
> patch submitter was proposing to have a bounce buffer in ath10k to
> workaround that. I don't remember the details anymore, they are on the
> ath10k mailing list archive if anyone is curious to know, but I would
> not be surprised if they are similar as here. So there might be a need
> to solve this in a generic way (but not sure of course as I haven't
> checked the details).

I re-call something about these as well, here are the patches. Perhaps
I should pick some of them up...

https://patchwork.kernel.org/patch/10123137/
https://patchwork.kernel.org/patch/10123139/
https://patchwork.kernel.org/patch/10123141/
https://patchwork.kernel.org/patch/10123143/

Kind regards
Uffe

Re: [PATCH] brcmfmac: add support for BCM43455 with modalias sdio:c00v02D0dA9BF

[Ulf Hansson]

On 16 January 2017 at 11:17, Martin Blumenstingl
<martin.blumensti...@googlemail.com> wrote:
> BCM43455 is a more recent revision of the BCM4345. Some of the BCM43455
> got a dedicated SDIO device ID which is currently not supported by
> brcmfmac.
> Adding the new sdio_device_id to brcmfmac is enough to get the BCM43455
> supported because the chip itself is already supported (due to BCM4345
> support in the driver).
>
> Signed-off-by: Martin Blumenstingl <martin.blumensti...@googlemail.com>

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

Kind regards
Uffe

> ---
> This is the proper patch following the (short) discussion from [0]
>
> [0] https://marc.info/?l=linux-wireless=148455981002310=2
>
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 1 +
>  include/linux/mmc/sdio_ids.h  | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index 72139b579b18..5bc2ba214735 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -1104,6 +1104,7 @@ static const struct sdio_device_id brcmf_sdmmc_ids[] = {
> BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_4339),
> BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_43430),
> BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_4345),
> +   BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_43455),
> BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_4354),
> BRCMF_SDIO_DEVICE(SDIO_DEVICE_ID_BROADCOM_4356),
> { /* end: all zeroes */ }
> diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h
> index d43ef96bf075..71b113e1223f 100644
> --- a/include/linux/mmc/sdio_ids.h
> +++ b/include/linux/mmc/sdio_ids.h
> @@ -36,6 +36,7 @@
>  #define SDIO_DEVICE_ID_BROADCOM_43362  0xa962
>  #define SDIO_DEVICE_ID_BROADCOM_43430  0xa9a6
>  #define SDIO_DEVICE_ID_BROADCOM_4345   0x4345
> +#define SDIO_DEVICE_ID_BROADCOM_43455  0xa9bf
>  #define SDIO_DEVICE_ID_BROADCOM_4354   0x4354
>  #define SDIO_DEVICE_ID_BROADCOM_4356   0x4356
>
> --
> 2.11.0
>

Re: [PATCH v3 2/2] mmc: pwrseq: add support for Marvell SD8787 chip

[Ulf Hansson]

On 20 January 2017 at 03:42, Shawn Lin <shawn@rock-chips.com> wrote:
> On 2017/1/19 22:13, Ulf Hansson wrote:
>>
>> +Shawn
>>
>> On 13 January 2017 at 06:29, Matt Ranostay <matt@ranostay.consulting>
>> wrote:
>>>
>>> Allow power sequencing for the Marvell SD8787 Wifi/BT chip.
>>> This can be abstracted to other chipsets if needed in the future.
>>>
>>> Cc: Tony Lindgren <t...@atomide.com>
>>> Cc: Ulf Hansson <ulf.hans...@linaro.org>
>>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>>> ---
>>>  drivers/mmc/core/Kconfig |  10 
>>>  drivers/mmc/core/Makefile|   1 +
>>>  drivers/mmc/core/pwrseq_sd8787.c | 117
>>> +++
>>>  3 files changed, 128 insertions(+)
>>>  create mode 100644 drivers/mmc/core/pwrseq_sd8787.c
>>>
>>> diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
>>> index cdfa8520a4b1..fc1ecdaaa9ca 100644
>>> --- a/drivers/mmc/core/Kconfig
>>> +++ b/drivers/mmc/core/Kconfig
>>> @@ -12,6 +12,16 @@ config PWRSEQ_EMMC
>>>   This driver can also be built as a module. If so, the module
>>>   will be called pwrseq_emmc.
>>>
>>> +config PWRSEQ_SD8787
>>> +   tristate "HW reset support for SD8787 BT + Wifi module"
>>> +   depends on OF && (MWIFIEX || BT_MRVL_SDIO)
>>> +   help
>>> + This selects hardware reset support for the SD8787 BT + Wifi
>>> + module. By default this option is set to n.
>>> +
>>> + This driver can also be built as a module. If so, the module
>>> + will be called pwrseq_sd8787.
>>> +
>>>  config PWRSEQ_SIMPLE
>>> tristate "Simple HW reset support for MMC"
>>> default y
>>> diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
>>> index b2a257dc644f..0f81464fa824 100644
>>> --- a/drivers/mmc/core/Makefile
>>> +++ b/drivers/mmc/core/Makefile
>>> @@ -10,6 +10,7 @@ mmc_core-y:= core.o bus.o host.o \
>>>quirks.o slot-gpio.o
>>>  mmc_core-$(CONFIG_OF)  += pwrseq.o
>>>  obj-$(CONFIG_PWRSEQ_SIMPLE)+= pwrseq_simple.o
>>> +obj-$(CONFIG_PWRSEQ_SD8787)+= pwrseq_sd8787.o
>>>  obj-$(CONFIG_PWRSEQ_EMMC)  += pwrseq_emmc.o
>>>  mmc_core-$(CONFIG_DEBUG_FS)+= debugfs.o
>>>  obj-$(CONFIG_MMC_BLOCK)+= mmc_block.o
>>> diff --git a/drivers/mmc/core/pwrseq_sd8787.c
>>> b/drivers/mmc/core/pwrseq_sd8787.c
>>> new file mode 100644
>>> index ..f4080fe6439e
>>> --- /dev/null
>>> +++ b/drivers/mmc/core/pwrseq_sd8787.c
>>> @@ -0,0 +1,117 @@
>>> +/*
>>> + * pwrseq_sd8787.c - power sequence support for Marvell SD8787 BT + Wifi
>>> chip
>>> + *
>>> + * Copyright (C) 2016 Matt Ranostay <matt@ranostay.consulting>
>>> + *
>>> + * Based on the original work pwrseq_simple.c
>>> + *  Copyright (C) 2014 Linaro Ltd
>>> + *  Author: Ulf Hansson <ulf.hans...@linaro.org>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + */
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#include 
>>> +
>>> +#include "pwrseq.h"
>>> +
>>> +struct mmc_pwrseq_sd8787 {
>>> +   struct mmc_pwrseq pwrseq;
>>> +   struct gpio_desc *reset_gpio;
>>> +   struct gpio_desc *pwrdn_gpio;
>>> +};
>>> +
>>> +#define to_pwrseq_sd8787(p) container_of(p, struct mmc_pwrseq_sd8787,
>>> pwrseq)
>>> +
>>> +static void mmc_pwrseq_sd8787_pre_power_on(struct mmc_host *host)
>

Re: [PATCH v3 2/2] mmc: pwrseq: add support for Marvell SD8787 chip

[Ulf Hansson]

On 19 January 2017 at 21:10, Kalle Valo <kv...@codeaurora.org> wrote:
> Ulf Hansson <ulf.hans...@linaro.org> writes:
>
>> Twisting my head around how this could be integrated smoothly into
>> pwrseq simple. No, I just can find a good way forward without messing
>> up pwrseq simple itself.
>>
>> So, for now I decided (once more :-), that let's keep this as separate 
>> driver!
>>
>> Perhaps, following device specific mmc pwrseq drivers will needs
>> something similar, but in such case we can look into that then.
>> Thinking about cw1200 for example.
>>
>> Let's get Rob's ack for the DT bindings, seems almost there, then I
>> will queue this.
>
> Just to confirm, you will take the whole set (including the bindings
> patch)?

Yes, correct.

Kind regards
Uffe

Re: [PATCH v3 2/2] mmc: pwrseq: add support for Marvell SD8787 chip

[Ulf Hansson]

+Shawn

On 13 January 2017 at 06:29, Matt Ranostay <matt@ranostay.consulting> wrote:
> Allow power sequencing for the Marvell SD8787 Wifi/BT chip.
> This can be abstracted to other chipsets if needed in the future.
>
> Cc: Tony Lindgren <t...@atomide.com>
> Cc: Ulf Hansson <ulf.hans...@linaro.org>
> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> ---
>  drivers/mmc/core/Kconfig |  10 
>  drivers/mmc/core/Makefile|   1 +
>  drivers/mmc/core/pwrseq_sd8787.c | 117 
> +++
>  3 files changed, 128 insertions(+)
>  create mode 100644 drivers/mmc/core/pwrseq_sd8787.c
>
> diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
> index cdfa8520a4b1..fc1ecdaaa9ca 100644
> --- a/drivers/mmc/core/Kconfig
> +++ b/drivers/mmc/core/Kconfig
> @@ -12,6 +12,16 @@ config PWRSEQ_EMMC
>   This driver can also be built as a module. If so, the module
>   will be called pwrseq_emmc.
>
> +config PWRSEQ_SD8787
> +   tristate "HW reset support for SD8787 BT + Wifi module"
> +   depends on OF && (MWIFIEX || BT_MRVL_SDIO)
> +   help
> + This selects hardware reset support for the SD8787 BT + Wifi
> + module. By default this option is set to n.
> +
> + This driver can also be built as a module. If so, the module
> + will be called pwrseq_sd8787.
> +
>  config PWRSEQ_SIMPLE
> tristate "Simple HW reset support for MMC"
> default y
> diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
> index b2a257dc644f..0f81464fa824 100644
> --- a/drivers/mmc/core/Makefile
> +++ b/drivers/mmc/core/Makefile
> @@ -10,6 +10,7 @@ mmc_core-y:= core.o bus.o host.o \
>quirks.o slot-gpio.o
>  mmc_core-$(CONFIG_OF)  += pwrseq.o
>  obj-$(CONFIG_PWRSEQ_SIMPLE)+= pwrseq_simple.o
> +obj-$(CONFIG_PWRSEQ_SD8787)+= pwrseq_sd8787.o
>  obj-$(CONFIG_PWRSEQ_EMMC)  += pwrseq_emmc.o
>  mmc_core-$(CONFIG_DEBUG_FS)+= debugfs.o
>  obj-$(CONFIG_MMC_BLOCK)+= mmc_block.o
> diff --git a/drivers/mmc/core/pwrseq_sd8787.c 
> b/drivers/mmc/core/pwrseq_sd8787.c
> new file mode 100644
> index ..f4080fe6439e
> --- /dev/null
> +++ b/drivers/mmc/core/pwrseq_sd8787.c
> @@ -0,0 +1,117 @@
> +/*
> + * pwrseq_sd8787.c - power sequence support for Marvell SD8787 BT + Wifi chip
> + *
> + * Copyright (C) 2016 Matt Ranostay <matt@ranostay.consulting>
> + *
> + * Based on the original work pwrseq_simple.c
> + *  Copyright (C) 2014 Linaro Ltd
> + *  Author: Ulf Hansson <ulf.hans...@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include "pwrseq.h"
> +
> +struct mmc_pwrseq_sd8787 {
> +   struct mmc_pwrseq pwrseq;
> +   struct gpio_desc *reset_gpio;
> +   struct gpio_desc *pwrdn_gpio;
> +};
> +
> +#define to_pwrseq_sd8787(p) container_of(p, struct mmc_pwrseq_sd8787, pwrseq)
> +
> +static void mmc_pwrseq_sd8787_pre_power_on(struct mmc_host *host)
> +{
> +   struct mmc_pwrseq_sd8787 *pwrseq = to_pwrseq_sd8787(host->pwrseq);
> +
> +   gpiod_set_value_cansleep(pwrseq->reset_gpio, 1);
> +
> +   msleep(300);
> +   gpiod_set_value_cansleep(pwrseq->pwrdn_gpio, 1);
> +}
> +
> +static void mmc_pwrseq_sd8787_power_off(struct mmc_host *host)
> +{
> +   struct mmc_pwrseq_sd8787 *pwrseq = to_pwrseq_sd8787(host->pwrseq);
> +
> +   gpiod_set_value_cansleep(pwrseq->pwrdn_gpio, 0);
> +   gpiod_set_value_cansleep(pwrseq->reset_gpio, 0);
> +}
> +
> +static const struct mmc_pwrseq_ops mmc_pwrseq_sd8787_ops = {
> +   .pre_power_on = mmc_pwrseq_sd8787_pre_power_on,
> +   .power_off = mmc_pwrseq_sd8787_power_off,
> +};
> +
> +static const struct of_device_id mmc_pwrseq_sd8787_of_match[] = {
> +   { .compatible = "mmc-pwrseq-sd8787",},
> +   {/* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of,

Re: [PATCH v3 2/2] mmc: pwrseq: add support for Marvell SD8787 chip

[Ulf Hansson]

On 18 January 2017 at 08:50, Matt Ranostay <matt@ranostay.consulting> wrote:
> On Sun, Jan 15, 2017 at 6:35 PM, Shawn Lin <shawn@rock-chips.com> wrote:
>> On 2017/1/16 5:41, Matt Ranostay wrote:
>>>
>>> On Thu, Jan 12, 2017 at 11:16 PM, Shawn Lin <shawn@rock-chips.com>
>>> wrote:
>>>>
>>>> On 2017/1/13 13:29, Matt Ranostay wrote:
>>>>>
>>>>>
>>>>> Allow power sequencing for the Marvell SD8787 Wifi/BT chip.
>>>>> This can be abstracted to other chipsets if needed in the future.
>>>>>
>>>>> Cc: Tony Lindgren <t...@atomide.com>
>>>>> Cc: Ulf Hansson <ulf.hans...@linaro.org>
>>>>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>>>>> ---
>>>>>  drivers/mmc/core/Kconfig |  10 
>>>>>  drivers/mmc/core/Makefile|   1 +
>>>>>  drivers/mmc/core/pwrseq_sd8787.c | 117
>>>>> +++
>>>>>  3 files changed, 128 insertions(+)
>>>>>  create mode 100644 drivers/mmc/core/pwrseq_sd8787.c
>>>>>
>>>>> diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
>>>>> index cdfa8520a4b1..fc1ecdaaa9ca 100644
>>>>> --- a/drivers/mmc/core/Kconfig
>>>>> +++ b/drivers/mmc/core/Kconfig
>>>>> @@ -12,6 +12,16 @@ config PWRSEQ_EMMC
>>>>>   This driver can also be built as a module. If so, the module
>>>>>   will be called pwrseq_emmc.
>>>>>
>>>>> +config PWRSEQ_SD8787
>>>>> +   tristate "HW reset support for SD8787 BT + Wifi module"
>>>>> +   depends on OF && (MWIFIEX || BT_MRVL_SDIO)
>>>>> +   help
>>>>> + This selects hardware reset support for the SD8787 BT + Wifi
>>>>> + module. By default this option is set to n.
>>>>> +
>>>>> + This driver can also be built as a module. If so, the module
>>>>> + will be called pwrseq_sd8787.
>>>>> +
>>>>
>>>>
>>>>
>>>> I don't like this way, as we have a chance to list lots
>>>> configure options here. wifi A,B,C,D...Z, all of them need a
>>>> new section here if needed?
>>>>
>>>> Instead, could you just extent pwrseq_simple.c and add you
>>>> .compatible = "mmc-pwrseq-sd8787", "mmc-pwrseq-simple"?
>>>
>>>
>>> You mean all the chipset pwrseqs linked into the pwrseq-simple module?
>>
>>
>> What I mean was if you just extent the pwrseq-simple a bit, you could
>> just add your chipset pwrseqs linked into the pwrseq-simple. But if you
>> need a different *pattern* of pwrseqs, you should need a new name, for
>> instance, pwrseq-sdio.c etc... But please don't use the name of
>> sd8787? So if I use a wifi named ABC but using the same pwrseq pattenr,
>> should I include your "mmc-pwrseq- sd8787" for that or I need a new
>> mmc-pwrseq-ABC.c?
>
> Ah so pwrseq-sdio.c seems reasonable and having chipsets functions
> defined in a structure. That could be abstracted out for other
> chipsets that could needed in the future.

I think get the idea and it seems reasonable. With that in mind, I
have looked at the code once more and I got some new ideas on how to
adopt pwrseq-simple for your case.

I post the comments separately.

Kind regards
Uffe

Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip

[Ulf Hansson]

On 30 November 2016 at 14:11, Javier Martinez Canillas
 wrote:
> Hello Matt,
>
> On Tue, Nov 29, 2016 at 10:20 PM, Matt Ranostay
>  wrote:
>> On Tue, Nov 29, 2016 at 9:13 AM, Javier Martinez Canillas
>
> [snip]
>
>>
>>
 +- pwndn-gpio: contains a power down GPIO specifier.
 +- reset-gpio: contains a reset GPIO specifier.
 +
>>>
>>> I wonder if we really need a custom power sequence provider for just
>>> this SDIO WiFI chip though. AFAICT the only missing piece in
>>> mmc-pwrseq-simple is the power down GPIO property, so maybe
>>> mmc-pwrseq-simple could be extended instead to have an optional
>>> powerdown-gpios property and instead in the Marvell SD8787 DT binding
>>> can be mentioned which mmc-pwrseq-simple properties are required for
>>> the device.
>>>
>>
>> The reason we didn't do that is we need delay between the two
>> assertions/desertions of GPIOs. It wouldn't seems good practice to
>> hack the pwrseq-simple for this...
>>
>
> Yes, I noticed that. I wouldn't say that it would be a hack for the
> pwrseq-simple since it already has a "post-power-on-delay-ms" DT
> property, so AFAICT it would just be adding a "pre-power-on-delay-ms"
> property for your use case.
>
> It would also be more consistent since it would support a delay for
> pre and post power callbacks. It would also make you avoid hardcoding
> the 300 msec wait, in case other device has a similar need but with a
> different wait time.
>
> In summary, I think that devices having a power (or power down) and
> enable GPIO, and needing to wait between the GPIO toggling are common.
> So I would prefer to make pwrseq-simple usable for these instead of
> adding device specific power sequence providers. But it's just my
> opinion and not my call :-)

This is a good idea. Please try out this approach.

[...]

Kind regards
Uffe

Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip

[Ulf Hansson]

On 29 November 2016 at 15:51, Rob Herring <r...@kernel.org> wrote:
> On Mon, Nov 28, 2016 at 9:54 AM, Ulf Hansson <ulf.hans...@linaro.org> wrote:
>> [...]
>>
>>>> +
>>>> +Example:
>>>> +
>>>> + wifi_pwrseq: wifi_pwrseq {
>>>> + compatible = "mmc-pwrseq-sd8787";
>>>> + pwrdn-gpio = <_gpio 0 GPIO_ACTIVE_LOW>;
>>>> + reset-gpio = <_gpio 1 GPIO_ACTIVE_LOW>;
>>>> + }
>>>> diff --git 
>>>> a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt 
>>>> b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>>>> index c421aba0a5bc..08fd65d35725 100644
>>>> --- a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>>>> +++ b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>>>> @@ -32,6 +32,9 @@ Optional properties:
>>>>so that the wifi chip can wakeup host platform under 
>>>> certain condition.
>>>>during system resume, the irq will be disabled to make sure
>>>>unnecessary interrupt is not received.
>>>> +  - vmmc-supply: a phandle of a regulator, supplying VCC to the card
>>>
>>> This is why pwrseq is wrong. You have some properties in the card node
>>> and some in pwrseq node. Everything should be in the card node.
>>
>> Put "all" in the card node, just doesn't work for MMC. Particular in
>> cases when we have removable cards, as then it would be wrong to have
>> a card node.
>
> When is there a problem with removable cards? The connector is
> standard and everything needed (CD, VMMC, VDDIO, etc.) is defined in
> the host controller node. If that isn't sufficient, then we should
> start defining a connector node.

I probably didn't get your point. Anyway, let's try again.

I don't see any problems with removable cards and neither with
non-removable cards.

Normally, we don't need a connector node, but instead we put the
related properties/phandles in the host controller node. This works
fine!

For SDIO func devices, sometimes we need a child node of the host
controller node, as to be able to describe certain characteristics of
the SDIO func device. So this case is kind of a special type of card
node (because one can have several SDIO func devices attached, even if
this isn't used).

Now, to follow the current bindings, we must not put connector related
bindings in the card node, like the vmmc-supply, perhaps that is what
causes confusion here?

>
>> The mmc pwrseq DT bindings just follows the legacy approach for MMC
>> and that's why the pwrseq handle is at the controller node. Yes, would
>> could have done it differently, but this is the case now, so we will
>> have to accept that.
>
> We're stuck with supporting the existing cases. That doesn't mean
> we're stuck with the same thing for new cases.

Agree!

But I think there is nothing to improve in this case, or am I wrong?

Kind regards
Uffe

Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip

[Ulf Hansson]

[...]

>> +
>> +Example:
>> +
>> + wifi_pwrseq: wifi_pwrseq {
>> + compatible = "mmc-pwrseq-sd8787";
>> + pwrdn-gpio = <_gpio 0 GPIO_ACTIVE_LOW>;
>> + reset-gpio = <_gpio 1 GPIO_ACTIVE_LOW>;
>> + }
>> diff --git 
>> a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt 
>> b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>> index c421aba0a5bc..08fd65d35725 100644
>> --- a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>> +++ b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>> @@ -32,6 +32,9 @@ Optional properties:
>>so that the wifi chip can wakeup host platform under certain 
>> condition.
>>during system resume, the irq will be disabled to make sure
>>unnecessary interrupt is not received.
>> +  - vmmc-supply: a phandle of a regulator, supplying VCC to the card
>
> This is why pwrseq is wrong. You have some properties in the card node
> and some in pwrseq node. Everything should be in the card node.

Put "all" in the card node, just doesn't work for MMC. Particular in
cases when we have removable cards, as then it would be wrong to have
a card node.

The mmc pwrseq DT bindings just follows the legacy approach for MMC
and that's why the pwrseq handle is at the controller node. Yes, would
could have done it differently, but this is the case now, so we will
have to accept that.

[...]

Kind regards
Uffe

Re: [PATCH] mmc: add API for data write using scatter gather DMA

[Ulf Hansson]

On 29 March 2016 at 14:57, Amitkumar Karwar  wrote:
> From: Bing Zhao 
>
> This patch adds new API for SDIO scatter gather.
>
> Existing mmc_io_rw_extended() API expects caller to pass single contiguous
> buffer. It will be split if it exceeds segment size, SG table is prepared
> and used it for reading/writing the data.
>
> Our intention for defining new API here is to facilitate caller to provide
> ready SG table(scattered buffer list). It will be useful for the drivers
> which intend to handle SDIO SG internally.
>
> Signed-off-by: Bing Zhao 
> Signed-off-by: Xinming Hu 
> Signed-off-by: Amitkumar Karwar 
> ---

So this has been posted and discussed earlier. In upcoming revisions,
please attach that history so people know when reviewing.

Moreover, if you have been using this API to improve throughput,
perhaps you can provide us with some numbers as well!?

>  drivers/mmc/core/sdio_ops.c   | 64 
> +++
>  include/linux/mmc/sdio_func.h |  5 
>  2 files changed, 69 insertions(+)
>
> diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
> index 62508b4..6875980 100644
> --- a/drivers/mmc/core/sdio_ops.c
> +++ b/drivers/mmc/core/sdio_ops.c
> @@ -204,6 +204,70 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, 
> unsigned fn,
> return 0;
>  }
>
> +int mmc_io_rw_extended_sg(struct mmc_card *card, int write, unsigned fn,
> + unsigned addr, int incr_addr,
> + struct sg_table *sgt, unsigned blksz)
> +{
> +   struct mmc_request mrq = {NULL};
> +   struct mmc_command cmd = {0};
> +   struct mmc_data data = {0};
> +   struct scatterlist *sg_ptr;
> +   unsigned blocks = 0;
> +   int i;
> +
> +   WARN_ON(!card || !card->host);
> +   WARN_ON(fn > 7);
> +   WARN_ON(blksz == 0);

WARN_ON is not correct here. You should return proper error codes instead.

> +   for_each_sg(sgt->sgl, sg_ptr, sgt->nents, i) {
> +   WARN_ON(sg_ptr->length > card->host->max_seg_size);

This raises a concern. Somehow the callers of this new API needs to be
able to fetch the max_seg_size, as otherwise how would they know what
size to use when allocating the buffers.

Of course they can just pick the value from card->host->max_seg_size,
but that's not the correct way. Can we add a separate SDIO API for
this!?

Moreover, I think you should return a proper error code instead of WARN_ON.

> +   blocks += DIV_ROUND_UP(sg_ptr->length, blksz);
> +   }
> +
> +   /* sanity check */
> +   if (addr & ~0x1)
> +   return -EINVAL;
> +
> +   mrq.cmd = 
> +   mrq.data = 
> +
> +   cmd.opcode = SD_IO_RW_EXTENDED;
> +   cmd.arg = write ? 0x8000 : 0x;
> +   cmd.arg |= fn << 28;
> +   cmd.arg |= incr_addr ? 0x0400 : 0x;
> +   cmd.arg |= addr << 9;
> +   cmd.arg |= 0x0800 | blocks;
> +   cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 | MMC_CMD_ADTC;
> +
> +   data.blksz = blksz;
> +   data.blocks = blocks;
> +   data.flags = write ? MMC_DATA_WRITE : MMC_DATA_READ;
> +
> +   data.sg = sgt->sgl;
> +   data.sg_len = sgt->nents;
> +
> +   mmc_set_data_timeout(, card);
> +   mmc_wait_for_req(card->host, );
> +
> +   if (cmd.error)
> +   return cmd.error;
> +   if (data.error)
> +   return data.error;
> +
> +   if (mmc_host_is_spi(card->host)) {
> +   /* host driver already reported errors */
> +   } else {
> +   if (cmd.resp[0] & R5_ERROR)
> +   return -EIO;
> +   if (cmd.resp[0] & R5_FUNCTION_NUMBER)
> +   return -EINVAL;
> +   if (cmd.resp[0] & R5_OUT_OF_RANGE)
> +   return -ERANGE;
> +   }
> +
> +   return 0;
> +}
> +EXPORT_SYMBOL_GPL(mmc_io_rw_extended_sg);

Besides these comment, clearly you have duplicated code from
mmc_io_rw_extended() in the above function.

Can you try to re-factor mmc_io_rw_extended(), so we can avoid too
much code duplications.

> +
>  int sdio_reset(struct mmc_host *host)
>  {
> int ret;
> diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
> index aab032a..2107e91 100644
> --- a/include/linux/mmc/sdio_func.h
> +++ b/include/linux/mmc/sdio_func.h
> @@ -14,6 +14,7 @@
>
>  #include 
>  #include 
> +#include 
>
>  #include 
>
> @@ -151,6 +152,10 @@ extern int sdio_memcpy_toio(struct sdio_func *func, 
> unsigned int addr,
>  extern int sdio_writesb(struct sdio_func *func, unsigned int addr,
> void *src, int count);
>
> +int mmc_io_rw_extended_sg(struct mmc_card *card, int write, unsigned fn,
> + unsigned addr, int incr_addr,
> + struct sg_table *sgt, unsigned blksz);
> +
>  extern unsigned char 

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

[Ulf Hansson]

On 14 April 2015 at 20:10, Arend van Spriel ar...@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...@linux.intel.com
 Reviewed-by: Pieter-Paul Giesberts piete...@broadcom.com
 Reviewed-by: Franky (Zhenhui) Lin fran...@broadcom.com
 Signed-off-by: Arend van Spriel ar...@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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kind regards
Uffe
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: new API for data write using scatter gather DMA

[Ulf Hansson]

On 5 December 2014 at 10:24, Avinash Patil pat...@marvell.com wrote:
 Hi Ulf,

 We have decided to drop this patch.

Why?

Kind regards
Uffe


 Thanks,
 Avinash
 
 From: Avinash Patil
 Sent: Tuesday, December 02, 2014 2:55 PM
 To: Ulf Hansson
 Cc: linux-mmc; linux-wireless@vger.kernel.org; Amitkumar Karwar; Cathy Luo; 
 Marc Yang; Xinming Hu; bin9z...@gmail.com; Bing Zhao
 Subject: RE: [PATCH] mmc: new API for data write using scatter gather DMA

 Hi Ulf,

 Purpose of patch is to offload scatter gather list population to host driver.
 Patch provides MMC API using which host driver can directly use to program SG 
 DMA.


 Thanks,
 Avinash
 
 From: Ulf Hansson [ulf.hans...@linaro.org]
 Sent: Wednesday, November 26, 2014 7:13 PM
 To: Avinash Patil
 Cc: linux-mmc; linux-wireless@vger.kernel.org; Amitkumar Karwar; Cathy Luo; 
 Marc Yang; Xinming Hu; bin9z...@gmail.com; Bing Zhao
 Subject: Re: [PATCH] mmc: new API for data write using scatter gather DMA

 On 26 November 2014 at 12:07, Avinash Patil pat...@marvell.com wrote:
 From: Bing Zhao bz...@marvell.com

 This patch adds new API to handle scatter gather aggregation.

 Why is this needed?


 Signed-off-by: Bing Zhao bz...@marvell.com
 Signed-off-by: Avinash Patil pat...@marvell.com
 Signed-off-by: Xinming Hu h...@marvell.com
 Signed-off-by: Cathy Luo c...@marvell.com
 ---
  drivers/mmc/core/sdio_ops.c   | 65 
 +++
  include/linux/mmc/sdio_func.h |  5 
  2 files changed, 70 insertions(+)

 diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
 index 62508b4..4c5e362 100644
 --- a/drivers/mmc/core/sdio_ops.c
 +++ b/drivers/mmc/core/sdio_ops.c
 @@ -15,6 +15,7 @@
  #include linux/mmc/card.h
  #include linux/mmc/mmc.h
  #include linux/mmc/sdio.h
 +#include linux/export.h

  #include core.h
  #include sdio_ops.h
 @@ -204,6 +205,70 @@ int mmc_io_rw_extended(struct mmc_card *card, int 
 write, unsigned fn,
 return 0;
  }

 +int mmc_io_rw_extended_sg(struct mmc_card *card, int write, unsigned fn,
 + unsigned addr, int incr_addr,
 + struct sg_table *sgt, unsigned blksz)
 +{
 +   struct mmc_request mrq = {NULL};
 +   struct mmc_command cmd = {0};
 +   struct mmc_data data = {0};
 +   struct scatterlist *sg_ptr;
 +   unsigned blocks = 0;
 +   int i;
 +
 +   BUG_ON(!card || !card-host);
 +   BUG_ON(fn  7);
 +   BUG_ON(!blksz);

 No BUG_ON() here please. Better to return an error.

 +   for_each_sg(sgt-sgl, sg_ptr, sgt-nents, i) {
 +   WARN_ON(sg_ptr-length  card-host-max_seg_size);
 +   blocks += DIV_ROUND_UP(sg_ptr-length, blksz);
 +   }
 +
 +   /* sanity check */
 +   if (addr  ~0x1)
 +   return -EINVAL;
 +
 +   mrq.cmd = cmd;
 +   mrq.data = data;
 +
 +   cmd.opcode = SD_IO_RW_EXTENDED;
 +   cmd.arg = write ? 0x8000 : 0x;
 +   cmd.arg |= fn  28;
 +   cmd.arg |= incr_addr ? 0x0400 : 0x;
 +   cmd.arg |= addr  9;
 +   cmd.arg |= 0x0800 | blocks;
 +   cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 | MMC_CMD_ADTC;
 +
 +   data.blksz = blksz;
 +   data.blocks = blocks;
 +   data.flags = write ? MMC_DATA_WRITE : MMC_DATA_READ;
 +
 +   data.sg = sgt-sgl;
 +   data.sg_len = sgt-nents;
 +
 +   mmc_set_data_timeout(data, card);
 +   mmc_wait_for_req(card-host, mrq);
 +
 +   if (cmd.error)
 +   return cmd.error;
 +   if (data.error)
 +   return data.error;
 +
 +   if (mmc_host_is_spi(card-host)) {
 +   /* host driver already reported errors */
 +   } else {
 +   if (cmd.resp[0]  R5_ERROR)
 +   return -EIO;
 +   if (cmd.resp[0]  R5_FUNCTION_NUMBER)
 +   return -EINVAL;
 +   if (cmd.resp[0]  R5_OUT_OF_RANGE)
 +   return -ERANGE;
 +   }
 +
 +   return 0;
 +}
 +EXPORT_SYMBOL_GPL(mmc_io_rw_extended_sg);
 +
  int sdio_reset(struct mmc_host *host)
  {
 int ret;
 diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
 index 50f0bc9..927ea8f 100644
 --- a/include/linux/mmc/sdio_func.h
 +++ b/include/linux/mmc/sdio_func.h
 @@ -14,6 +14,7 @@

  #include linux/device.h
  #include linux/mod_devicetable.h
 +#include linux/scatterlist.h

  #include linux/mmc/pm.h

 @@ -153,6 +154,10 @@ extern int sdio_memcpy_toio(struct sdio_func *func, 
 unsigned int addr,
  extern int sdio_writesb(struct sdio_func *func, unsigned int addr,
 void *src, int count);

 +int mmc_io_rw_extended_sg(struct mmc_card *card, int write, unsigned fn,
 + unsigned addr, int incr_addr,
 + struct sg_table *sgt, unsigned blksz);
 +
  extern unsigned char sdio_f0_readb(struct sdio_func *func,
 unsigned int addr, int

Re: [PATCH] mmc: new API for data write using scatter gather DMA

[Ulf Hansson]

On 26 November 2014 at 12:07, Avinash Patil pat...@marvell.com wrote:
 From: Bing Zhao bz...@marvell.com

 This patch adds new API to handle scatter gather aggregation.

Why is this needed?


 Signed-off-by: Bing Zhao bz...@marvell.com
 Signed-off-by: Avinash Patil pat...@marvell.com
 Signed-off-by: Xinming Hu h...@marvell.com
 Signed-off-by: Cathy Luo c...@marvell.com
 ---
  drivers/mmc/core/sdio_ops.c   | 65 
 +++
  include/linux/mmc/sdio_func.h |  5 
  2 files changed, 70 insertions(+)

 diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
 index 62508b4..4c5e362 100644
 --- a/drivers/mmc/core/sdio_ops.c
 +++ b/drivers/mmc/core/sdio_ops.c
 @@ -15,6 +15,7 @@
  #include linux/mmc/card.h
  #include linux/mmc/mmc.h
  #include linux/mmc/sdio.h
 +#include linux/export.h

  #include core.h
  #include sdio_ops.h
 @@ -204,6 +205,70 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, 
 unsigned fn,
 return 0;
  }

 +int mmc_io_rw_extended_sg(struct mmc_card *card, int write, unsigned fn,
 + unsigned addr, int incr_addr,
 + struct sg_table *sgt, unsigned blksz)
 +{
 +   struct mmc_request mrq = {NULL};
 +   struct mmc_command cmd = {0};
 +   struct mmc_data data = {0};
 +   struct scatterlist *sg_ptr;
 +   unsigned blocks = 0;
 +   int i;
 +
 +   BUG_ON(!card || !card-host);
 +   BUG_ON(fn  7);
 +   BUG_ON(!blksz);

No BUG_ON() here please. Better to return an error.

 +   for_each_sg(sgt-sgl, sg_ptr, sgt-nents, i) {
 +   WARN_ON(sg_ptr-length  card-host-max_seg_size);
 +   blocks += DIV_ROUND_UP(sg_ptr-length, blksz);
 +   }
 +
 +   /* sanity check */
 +   if (addr  ~0x1)
 +   return -EINVAL;
 +
 +   mrq.cmd = cmd;
 +   mrq.data = data;
 +
 +   cmd.opcode = SD_IO_RW_EXTENDED;
 +   cmd.arg = write ? 0x8000 : 0x;
 +   cmd.arg |= fn  28;
 +   cmd.arg |= incr_addr ? 0x0400 : 0x;
 +   cmd.arg |= addr  9;
 +   cmd.arg |= 0x0800 | blocks;
 +   cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 | MMC_CMD_ADTC;
 +
 +   data.blksz = blksz;
 +   data.blocks = blocks;
 +   data.flags = write ? MMC_DATA_WRITE : MMC_DATA_READ;
 +
 +   data.sg = sgt-sgl;
 +   data.sg_len = sgt-nents;
 +
 +   mmc_set_data_timeout(data, card);
 +   mmc_wait_for_req(card-host, mrq);
 +
 +   if (cmd.error)
 +   return cmd.error;
 +   if (data.error)
 +   return data.error;
 +
 +   if (mmc_host_is_spi(card-host)) {
 +   /* host driver already reported errors */
 +   } else {
 +   if (cmd.resp[0]  R5_ERROR)
 +   return -EIO;
 +   if (cmd.resp[0]  R5_FUNCTION_NUMBER)
 +   return -EINVAL;
 +   if (cmd.resp[0]  R5_OUT_OF_RANGE)
 +   return -ERANGE;
 +   }
 +
 +   return 0;
 +}
 +EXPORT_SYMBOL_GPL(mmc_io_rw_extended_sg);
 +
  int sdio_reset(struct mmc_host *host)
  {
 int ret;
 diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
 index 50f0bc9..927ea8f 100644
 --- a/include/linux/mmc/sdio_func.h
 +++ b/include/linux/mmc/sdio_func.h
 @@ -14,6 +14,7 @@

  #include linux/device.h
  #include linux/mod_devicetable.h
 +#include linux/scatterlist.h

  #include linux/mmc/pm.h

 @@ -153,6 +154,10 @@ extern int sdio_memcpy_toio(struct sdio_func *func, 
 unsigned int addr,
  extern int sdio_writesb(struct sdio_func *func, unsigned int addr,
 void *src, int count);

 +int mmc_io_rw_extended_sg(struct mmc_card *card, int write, unsigned fn,
 + unsigned addr, int incr_addr,
 + struct sg_table *sgt, unsigned blksz);
 +
  extern unsigned char sdio_f0_readb(struct sdio_func *func,
 unsigned int addr, int *err_ret);
  extern void sdio_f0_writeb(struct sdio_func *func, unsigned char b,
 --
 1.8.1.4


Kind regards
Uffe
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html