mmc: tmio: why enable/disable SDIO irq on every transaction with IOMOD?

mmc: tmio: why enable/disable SDIO irq on every transaction with IOMOD?

[Yasushi SHOJI]

Hello,

We've been using tmio_mmc_pio.c and seeing the following message:

	"sh_mobile_sdhi sh_mobile_sdhi.1: timeout waiting for SD bus idle"

While searching for a fix, we found some code we don't understand.
I'm sending this question with a hope that someone would enlighten us a
bit.  So, here goes.

The SDIO spec. seems to allow sending SDIO IRQ between the blocks of
multi-block transfer.  The hardware, SCLKDIVEN bit of SD_INFO02, is
set to zero (0) and prohibits to access IOMOD bit of SDIO_MODE
register while multi-block transfer is going.

The current code, however, tries to disable SDIO IRQ by setting IOMOD
to zero in tmio_mmc_enable_sdio_irq() all the time, even during
multi-block transfer.  This prints the above waring.  What we don't
understand is that the code does both masking and disabling the IRQ.

So my question is that "What is the reason behind to disable IRQ with
SDIO_MODE?  Is there any situation which masking with SDIO_INFO1_MASK
is not enough?

Thanks you,
-- 
           yashi

Re: mmc: tmio: why enable/disable SDIO irq on every transaction with IOMOD?

[Ulf Hansson]

+Wolfram, Simon

Looping maintainers of TMIO/SDHI.

On 29 November 2016 at 07:23, Yasushi SHOJI <yashi@atmark-techno.com> wrote:
> Hello,
>
> We've been using tmio_mmc_pio.c and seeing the following message:
>
>         "sh_mobile_sdhi sh_mobile_sdhi.1: timeout waiting for SD bus idle"
>
> While searching for a fix, we found some code we don't understand.
> I'm sending this question with a hope that someone would enlighten us a
> bit.  So, here goes.
>
> The SDIO spec. seems to allow sending SDIO IRQ between the blocks of
> multi-block transfer.  The hardware, SCLKDIVEN bit of SD_INFO02, is
> set to zero (0) and prohibits to access IOMOD bit of SDIO_MODE
> register while multi-block transfer is going.
>
> The current code, however, tries to disable SDIO IRQ by setting IOMOD
> to zero in tmio_mmc_enable_sdio_irq() all the time, even during
> multi-block transfer.  This prints the above waring.  What we don't
> understand is that the code does both masking and disabling the IRQ.
>
> So my question is that "What is the reason behind to disable IRQ with
> SDIO_MODE?  Is there any situation which masking with SDIO_INFO1_MASK
> is not enough?
>
> Thanks you,
> --
>            yashi
> --

Kind regards
Uffe

Re: mmc: tmio: why enable/disable SDIO irq on every transaction with IOMOD?

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

Hi,

thanks for the report and thanks to Ulf for forwarding. Adding
renesas-soc list, too.

> > So my question is that "What is the reason behind to disable IRQ with
> > SDIO_MODE?  Is there any situation which masking with SDIO_INFO1_MASK
> > is not enough?

This code predates the time since I took over maintainership of the
driver, so I had to dig in git history.

The code was introduced with 845ecd20239c28 ("mmc: tmio_mmc: implement
SDIO IRQ support") which was in 2010. I don't have that old datasheets
to check if the SCLKDIVEN restriction was already present in the SDHI
cores which were available back then.

My assumption is that it was not, or it was overlooked. So, it might be
just for completeness that not only the individual IRQs have been
disabled but also the big master switch (IOMOD) was turned off.

My further assumption is that it is very likely good enough to disable
the individual IRQs. If we can't guarantee the conditions to set IOMOD,
it seems okay to me to just leave it.

Do you have a patch which works for you?

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: mmc: tmio: why enable/disable SDIO irq on every transaction with IOMOD?

[Yasushi SHOJI]

Hi Wolfram,

On Thu, 01 Dec 2016 19:46:22 +0900,
Wolfram Sang wrote:
> 
> > > So my question is that "What is the reason behind to disable IRQ with
> > > SDIO_MODE?  Is there any situation which masking with SDIO_INFO1_MASK
> > > is not enough?
> 
> The code was introduced with 845ecd20239c28 ("mmc: tmio_mmc: implement
> SDIO IRQ support") which was in 2010. I don't have that old datasheets
> to check if the SCLKDIVEN restriction was already present in the SDHI
> cores which were available back then.

Is Arnd Hannemann, the author, around these days by any chance?

Or does anyone on the list have a shareable old datasheet?  The one we
have is the one for R-Mobile A1 and has CONFIDENTIAL on it.

> My assumption is that it was not, or it was overlooked. So, it might be
> just for completeness that not only the individual IRQs have been
> disabled but also the big master switch (IOMOD) was turned off.
> 
> My further assumption is that it is very likely good enough to disable
> the individual IRQs. If we can't guarantee the conditions to set IOMOD,
> it seems okay to me to just leave it.

Yes that's what we think and what we see with our test code, which is
just the original without IOMOD removed.

> Do you have a patch which works for you?

We don't have it yet.  A question we have is that where should we
enable / disable the IOMOD?

Do we have to keep it disabled while the controller is in non-SDIO?
The spec says that we don't get SDIO irq while in non-SDIO mode.  But
should we trust?

Or, should we disable it when we switch back to non-SDIO mode and
enable when we detect SDIO?

Please enlighten us this area.  We'll submit the proper fix once we
know how we should do it.

Thanks,
-- 
            yashi

Re: mmc: tmio: why enable/disable SDIO irq on every transaction with IOMOD?

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

Hi,

> Is Arnd Hannemann, the author, around these days by any chance?

According to git history, he is sending a patch occasionally, but all
his SH involvement was from 2010. I don't know him.

> We don't have it yet.  A question we have is that where should we
> enable / disable the IOMOD?

I think unconditionally in probe. I will send an RFC with some
explanations in a minute.

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]