by Andreas Fenkart

[permalink] [raw]

Subject: Re: [PATCH v3 2/4] mmc: omap_hsmmc: Enable SDIO IRQ.

Hi

2013/10/8 Felipe Balbi <[email protected]>:
> Hi,
>
> On Sat, Oct 05, 2013 at 01:17:08PM +0200, Andreas Fenkart wrote:
>> For now, only support SDIO interrupt if we are booted with
>> DT. This is because some platforms need special quirks. And
>> we don't want to add new legacy mux platform init code
>> callbacks any longer as we are moving to DT based booting
>> anyways.
>>
>> Broken hardware, missing the swakueup line, should fallback
>> to polling, by setting 'ti,quirk-swakup-missing' in the
>> device tree. Otherwise pending SDIO IRQ are not detected
>> while in suspend. This affects am33xx processors.
>>
>> Signed-off-by: Andreas Fenkart <[email protected]>
>
> I still think this patch needs to be splitted, see below.
>
>> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>> index 94d6dc8..53beac4 100644
>> --- a/drivers/mmc/host/omap_hsmmc.c
>> +++ b/drivers/mmc/host/omap_hsmmc.c
>> @@ -130,6 +130,7 @@ static void apply_clk_hack(struct device *dev)
>> #define TC_EN (1 << 1)
>> #define BWR_EN (1 << 4)
>> #define BRR_EN (1 << 5)
>> +#define CIRQ_EN (1 << 8)
>> #define ERR_EN (1 << 15)
>> #define CTO_EN (1 << 16)
>> #define CCRC_EN (1 << 17)
>> @@ -210,6 +211,10 @@ struct omap_hsmmc_host {
>> int reqs_blocked;
>> int use_reg;
>> int req_in_progress;
>> + int flags;
>> +#define HSMMC_RUNTIME_SUSPENDED (1 << 0) /* Runtime suspended */
>> +#define HSMMC_SDIO_IRQ_ENABLED (1 << 1) /* SDIO irq enabled */
>> +
>> struct omap_hsmmc_next next_data;
>> struct omap_mmc_platform_data *pdata;
>> };
>> @@ -490,27 +495,40 @@ static void omap_hsmmc_stop_clock(struct omap_hsmmc_host *host)
>> static void omap_hsmmc_enable_irq(struct omap_hsmmc_host *host,
>> struct mmc_command *cmd)
>> {
>> - unsigned int irq_mask;
>> + u32 irq_mask = INT_EN_MASK;
>> + unsigned long flags;
>>
>> if (host->use_dma)
>> - irq_mask = INT_EN_MASK & ~(BRR_EN | BWR_EN);
>> - else
>> - irq_mask = INT_EN_MASK;
>> + irq_mask &= ~(BRR_EN | BWR_EN);
>
> is this a bugfix ? should it be sent as a separate patch ?

maybe the u32, but otherwise it's just shorter... shall I drop it.

>>
>> /* Disable timeout for erases */
>> if (cmd->opcode == MMC_ERASE)
>> irq_mask &= ~DTO_EN;
>>
>> + spin_lock_irqsave(&host->irq_lock, flags);
>
> why do you need this new locking here ? register acesses are atomic,
> right ? If you really do need the locking, should it be a separate patch
> as well ?

because host->flags can be accessed from irq context as well

>> OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR);
>> OMAP_HSMMC_WRITE(host->base, ISE, irq_mask);
>> +
>> + /* latch pending CIRQ, but don't signal */
>> + if (host->flags & HSMMC_SDIO_IRQ_ENABLED)
>> + irq_mask |= CIRQ_EN;

<- imagine sdio irq here ->
with the next write we would reenable sdio irq, despite the irq handler
having them disabled

>
> why do you need this ? Looks like it should be yet another patch.

>>
>> static void omap_hsmmc_disable_irq(struct omap_hsmmc_host *host)
>> {
>> - OMAP_HSMMC_WRITE(host->base, ISE, 0);
>> - OMAP_HSMMC_WRITE(host->base, IE, 0);
>> + u32 irq_mask = 0;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&host->irq_lock, flags);
>> + /* no transfer running, need to signal cirq if */
>
> if... ?
>
>> + if (host->flags & HSMMC_SDIO_IRQ_ENABLED)
>> + irq_mask |= CIRQ_EN;
>> + OMAP_HSMMC_WRITE(host->base, ISE, irq_mask);
>
> the whole fiddling with STAT and ISE registers look very bogus to me
> (even on current state before this patch). We shouldn't be clearing
> pending IRQ events here, right ? We could miss IRQs due to that.

sdio_claim_host excludes multiple users and typical users are using synchronous
communication, issue a transfer, wait till it's done, then release the host.
Hence when we come here, the content of ISE/STAT is a leftover from
the previous transfer.
Probably the intent here is to reset the registers in defined state,
maybe it wasn't needed
but it doesn't hurt either.

It's conservative... I don't like to change it, along the side of my
sdio irq patches.
If I did lots of such changes, surely I would create a regression.

On the other side If I was up to optimize the driver, then I would do this.

>
>> @@ -1067,8 +1085,12 @@ static irqreturn_t omap_hsmmc_irq(int irq, void *dev_id)
>> int status;
>>
>> status = OMAP_HSMMC_READ(host->base, STAT);
>> - while (status & INT_EN_MASK && host->req_in_progress) {
>> - omap_hsmmc_do_irq(host, status);
>> + while (status & (INT_EN_MASK | CIRQ_EN)) {
>
> why don't enable CIRQ_EN in INT_EN_MASK directly ?

INT_EN_MASK is also used when bootstrapping the card in
send_init_stream, but there
you don't want to enable sdio irqs. So I would have to mask it there,
so this is the smaller
change.

>
>> + if (host->req_in_progress)
>> + omap_hsmmc_do_irq(host, status);
>> +
>> + if (status & CIRQ_EN)
>> + mmc_signal_sdio_irq(host->mmc);
>>
>> /* Flush posted write */
>> status = OMAP_HSMMC_READ(host->base, STAT);
>> @@ -1583,6 +1605,42 @@ static void omap_hsmmc_init_card(struct mmc_host *mmc, struct mmc_card *card)
>> mmc_slot(host).init_card(card);
>> }
>>
>> +static void omap_hsmmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
>> +{
>> + struct omap_hsmmc_host *host = mmc_priv(mmc);
>> + u32 irq_mask;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&host->irq_lock, flags);
>> +
>> + if (enable)
>> + host->flags |= HSMMC_SDIO_IRQ_ENABLED;
>> + else
>> + host->flags &= ~HSMMC_SDIO_IRQ_ENABLED;
>> +
>> + /* if statement here with followup patch */
>> + {
>> + irq_mask = OMAP_HSMMC_READ(host->base, ISE);
>> + if (enable)
>> + irq_mask |= CIRQ_EN;
>> + else
>> + irq_mask &= ~CIRQ_EN;
>
> perhaps combine this branch with previous one ?
>
>> + OMAP_HSMMC_WRITE(host->base, IE, irq_mask);
>> +
>> + /*
>> + * if enable, piggy back on current request
>> + * but always disable
>> + */
>> + if (!host->req_in_progress || !enable)
>> + OMAP_HSMMC_WRITE(host->base, ISE, irq_mask);
>> +
>> + /* flush posted write */
>> + OMAP_HSMMC_READ(host->base, IE);
>> + }
>> +
>> + spin_unlock_irqrestore(&host->irq_lock, flags);
>> +}
>> +
>> static void omap_hsmmc_conf_bus_power(struct omap_hsmmc_host *host)
>> {
>> u32 hctl, capa, value;
>> @@ -1635,7 +1693,7 @@ static const struct mmc_host_ops omap_hsmmc_ops = {
>> .get_cd = omap_hsmmc_get_cd,
>> .get_ro = omap_hsmmc_get_ro,
>> .init_card = omap_hsmmc_init_card,
>> - /* NYET -- enable_sdio_irq */
>> + .enable_sdio_irq = omap_hsmmc_enable_sdio_irq,
>> };
>>
>> #ifdef CONFIG_DEBUG_FS
>> @@ -1833,6 +1891,7 @@ static int omap_hsmmc_probe(struct platform_device *pdev)
>> host->dma_ch = -1;
>> host->irq = irq;
>> host->slot_id = 0;
>> + host->flags = 0;
>
> why do you need to zero-initialize flags here ? another bug ?
>
>> @@ -2023,6 +2082,22 @@ static int omap_hsmmc_probe(struct platform_device *pdev)
>> dev_warn(&pdev->dev,
>> "pins are not configured from the driver\n");
>>
>> + /*
>> + * For now, only support SDIO interrupt if we are booted with
>> + * DT. This is because some platforms need special quirks. And
>> + * we don't want to add new legacy mux platform init code
>> + * callbacks any longer as we are moving to DT based booting
>> + * anyways.
>> + */
>> + if (match) {
>
> isn't if (dev->of.node) a better check here ?
>
>> + mmc->caps |= MMC_CAP_SDIO_IRQ;
>> + if (of_find_property(host->dev->of_node,
>> + "ti,quirk-swakeup-missing", NULL)) {
>
> looks like a SW configuration to me. Can't you figure this out by
> reading the revision register ?

It's not about the IP block, but about the SOC, so I guess that would
be brittle.
There could be an SOC using the same IP block revision but having the
wakeup line.

I was thinking about triggering on the device trees compatible section.

compatible = "ti,am33xx" -> use some fallback
otherwise -> enable irq.

Thanks for the review, appreciate it
Andreas

>
> --
> balbi

2013-10-29 17:16:49

by Kumar Gala

[permalink] [raw]

Subject: Re: [PATCH v3 2/4] mmc: omap_hsmmc: Enable SDIO IRQ.

On Oct 5, 2013, at 6:17 AM, Andreas Fenkart wrote:

> For now, only support SDIO interrupt if we are booted with
> DT. This is because some platforms need special quirks. And
> we don't want to add new legacy mux platform init code
> callbacks any longer as we are moving to DT based booting
> anyways.
>
> Broken hardware, missing the swakueup line, should fallback
> to polling, by setting 'ti,quirk-swakup-missing' in the
> device tree. Otherwise pending SDIO IRQ are not detected
> while in suspend. This affects am33xx processors.
>
> Signed-off-by: Andreas Fenkart <[email protected]>
>
> diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> index ed271fc..1136e6b 100644
> --- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> @@ -20,6 +20,24 @@ ti,dual-volt: boolean, supports dual voltage cards
> ti,non-removable: non-removable slot (like eMMC)
> ti,needs-special-reset: Requires a special softreset sequence
> ti,needs-special-hs-handling: HSMMC IP needs special setting for handling High Speed
> +ti,quirk-swakup-missing: SOC missing the swakeup line, will not detect
> +SDIO irq while in suspend. Fallback to polling. Affected chips are
> +am335x,
> +
> + ------
> + | PRCM |
> + ------
> + ^ |
> + swakeup | | fclk
> + | v
> + ------ ------- -----
> + | card | -- CIRQ --> | hsmmc | -- IRQ --> | CPU |
> + ------ ------- -----
> +
> +In suspend the fclk is off and the module is disfunctional. Even
> +register reads will fail. A small logic in the host will request fclk
> +restore, when an external event is detected. Once the clock is
> +restored, the host detects the event normally.
>
> Example:
> mmc1: mmc@0x4809c000 {

For the DT-Binding portion:

Acked-by: Kumar Gala <[email protected]>

- k

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

2013-10-29 17:22:31

by Felipe Balbi

[permalink] [raw]

Subject: Re: [PATCH v3 2/4] mmc: omap_hsmmc: Enable SDIO IRQ.

Hi,

On Tue, Oct 29, 2013 at 04:06:58PM +0100, Andreas Fenkart wrote:
> >> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> >> index 94d6dc8..53beac4 100644
> >> --- a/drivers/mmc/host/omap_hsmmc.c
> >> +++ b/drivers/mmc/host/omap_hsmmc.c
> >> @@ -130,6 +130,7 @@ static void apply_clk_hack(struct device *dev)
> >> #define TC_EN (1 << 1)
> >> #define BWR_EN (1 << 4)
> >> #define BRR_EN (1 << 5)
> >> +#define CIRQ_EN (1 << 8)
> >> #define ERR_EN (1 << 15)
> >> #define CTO_EN (1 << 16)
> >> #define CCRC_EN (1 << 17)
> >> @@ -210,6 +211,10 @@ struct omap_hsmmc_host {
> >> int reqs_blocked;
> >> int use_reg;
> >> int req_in_progress;
> >> + int flags;
> >> +#define HSMMC_RUNTIME_SUSPENDED (1 << 0) /* Runtime suspended */
> >> +#define HSMMC_SDIO_IRQ_ENABLED (1 << 1) /* SDIO irq enabled */
> >> +
> >> struct omap_hsmmc_next next_data;
> >> struct omap_mmc_platform_data *pdata;
> >> };
> >> @@ -490,27 +495,40 @@ static void omap_hsmmc_stop_clock(struct omap_hsmmc_host *host)
> >> static void omap_hsmmc_enable_irq(struct omap_hsmmc_host *host,
> >> struct mmc_command *cmd)
> >> {
> >> - unsigned int irq_mask;
> >> + u32 irq_mask = INT_EN_MASK;
> >> + unsigned long flags;
> >>
> >> if (host->use_dma)
> >> - irq_mask = INT_EN_MASK & ~(BRR_EN | BWR_EN);
> >> - else
> >> - irq_mask = INT_EN_MASK;
> >> + irq_mask &= ~(BRR_EN | BWR_EN);
> >
> > is this a bugfix ? should it be sent as a separate patch ?
>
> maybe the u32, but otherwise it's just shorter... shall I drop it.

no, now I saw what you did ;-)

> >>
> >> /* Disable timeout for erases */
> >> if (cmd->opcode == MMC_ERASE)
> >> irq_mask &= ~DTO_EN;
> >>
> >> + spin_lock_irqsave(&host->irq_lock, flags);
> >
> > why do you need this new locking here ? register acesses are atomic,
> > right ? If you really do need the locking, should it be a separate patch
> > as well ?
>
> because host->flags can be accessed from irq context as well

fair point :-)

> >> static void omap_hsmmc_disable_irq(struct omap_hsmmc_host *host)
> >> {
> >> - OMAP_HSMMC_WRITE(host->base, ISE, 0);
> >> - OMAP_HSMMC_WRITE(host->base, IE, 0);
> >> + u32 irq_mask = 0;
> >> + unsigned long flags;
> >> +
> >> + spin_lock_irqsave(&host->irq_lock, flags);
> >> + /* no transfer running, need to signal cirq if */
> >
> > if... ?
> >
> >> + if (host->flags & HSMMC_SDIO_IRQ_ENABLED)
> >> + irq_mask |= CIRQ_EN;
> >> + OMAP_HSMMC_WRITE(host->base, ISE, irq_mask);
> >
> > the whole fiddling with STAT and ISE registers look very bogus to me
> > (even on current state before this patch). We shouldn't be clearing
> > pending IRQ events here, right ? We could miss IRQs due to that.
>
> sdio_claim_host excludes multiple users and typical users are using synchronous
> communication, issue a transfer, wait till it's done, then release the host.
> Hence when we come here, the content of ISE/STAT is a leftover from
> the previous transfer.
> Probably the intent here is to reset the registers in defined state,
> maybe it wasn't needed
> but it doesn't hurt either.
>
> It's conservative... I don't like to change it, along the side of my
> sdio irq patches.
> If I did lots of such changes, surely I would create a regression.
>
> On the other side If I was up to optimize the driver, then I would do this.

sure, that was an unrelated comment, just exposing some possible problem
with that approach :-)

> >> @@ -1067,8 +1085,12 @@ static irqreturn_t omap_hsmmc_irq(int irq, void *dev_id)
> >> int status;
> >>
> >> status = OMAP_HSMMC_READ(host->base, STAT);
> >> - while (status & INT_EN_MASK && host->req_in_progress) {
> >> - omap_hsmmc_do_irq(host, status);
> >> + while (status & (INT_EN_MASK | CIRQ_EN)) {
> >
> > why don't enable CIRQ_EN in INT_EN_MASK directly ?
>
> INT_EN_MASK is also used when bootstrapping the card in
> send_init_stream, but there
> you don't want to enable sdio irqs. So I would have to mask it there,
> so this is the smaller
> change.

true, fair enough.

--
balbi

Attachments:

(No filename) (4.38 kB)
signature.asc (836.00 B)
Digital signature Download all attachments