* Re: [PATCH 1/2] mmc: tmio: Allow SDHI MMC to use multiple IRQ vectors
2011-04-27 7:43 [PATCH 1/2] mmc: tmio: Allow SDHI MMC to use multiple IRQ vectors Simon Horman
@ 2011-05-02 8:55 ` Guennadi Liakhovetski
2011-05-02 11:16 ` [PATCH 1/2] mmc: tmio: Allow SDHI MMC to use multiple IRQ Simon Horman
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Guennadi Liakhovetski @ 2011-05-02 8:55 UTC (permalink / raw)
To: linux-sh
Hi Simon
On Wed, 27 Apr 2011, Simon Horman wrote:
> SDHI blocks may have up to 4 IRQ vectors.
> The first three are of use to us, the 4th one
> relates to DRM features that I don't have documentation
> for and are almost certainly tainted by licensing issues.
>
> This patch allows multiple vectors to be used if supplied
> in the platform data. Which will allow IRQ multiplexing hacks
> to be removed.
>
> I plan to do further work to split tmio_mmc_irq() into per-vector
> handlers. But this patch should be useful by itself.
I have one nitpick: using a constant like "3" at multiple locations makes
code less future-proof. I would use a macro like TMIO_MMC_MAX_IRQS, at
some locations you can also do ARRAY_SIZE(host->irq). Also, are we sure,
that only "1 IRQ" or "max IRQs" (currently 3) are valid? Can there not be
a valid configuration with 2 IRQs? Don't we have such controllers on some
SoCs? For example, ap4 SDHI0 and SDHI2 have 4 IRQ vectors, but SDHI1 only
3... But ok, no, I don't see any controllers with < 3 IRQs, so, should
work for now.
Thanks
Guennadi
>
> Cc: Chris Ball <cjb@laptop.org>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Paul Mundt <lethal@linux-sh.org>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Signed-off-by: Simon Horman <horms@verge.net.au>
> ---
> drivers/mmc/host/sh_mobile_sdhi.c | 7 ++++---
> drivers/mmc/host/tmio_mmc.h | 2 +-
> drivers/mmc/host/tmio_mmc_pio.c | 32 ++++++++++++++++++++++----------
> 3 files changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> index cc70123..2216210 100644
> --- a/drivers/mmc/host/sh_mobile_sdhi.c
> +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> @@ -62,7 +62,7 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
> struct sh_mobile_sdhi_info *p = pdev->dev.platform_data;
> struct tmio_mmc_host *host;
> char clk_name[8];
> - int ret;
> + int i, ret;
>
> priv = kzalloc(sizeof(struct sh_mobile_sdhi), GFP_KERNEL);
> if (priv = NULL) {
> @@ -116,8 +116,9 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
> if (ret < 0)
> goto eprobe;
>
> - pr_info("%s at 0x%08lx irq %d\n", mmc_hostname(host->mmc),
> - (unsigned long)host->ctl, host->irq);
> + for (i = 0; host->irq[i] >= 0 && i < 3; i++)
> + pr_info("%s at 0x%08lx irq %d\n", mmc_hostname(host->mmc),
> + (unsigned long)host->ctl, host->irq[i]);
>
> return ret;
>
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index 099ed49..6b240f5 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -44,7 +44,7 @@ struct tmio_mmc_host {
> struct mmc_request *mrq;
> struct mmc_data *data;
> struct mmc_host *mmc;
> - int irq;
> + int irq[3];
> unsigned int sdio_irq_enabled;
>
> /* Callbacks for clock / power control */
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index 62d37de..9d84d90 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -790,7 +790,7 @@ int __devinit tmio_mmc_host_probe(struct tmio_mmc_host **host,
> struct tmio_mmc_host *_host;
> struct mmc_host *mmc;
> struct resource *res_ctl;
> - int ret;
> + int i, ret;
> u32 irq_mask = TMIO_MASK_CMD;
>
> res_ctl = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -837,20 +837,29 @@ int __devinit tmio_mmc_host_probe(struct tmio_mmc_host **host,
> tmio_mmc_clk_stop(_host);
> tmio_mmc_reset(_host);
>
> - ret = platform_get_irq(pdev, 0);
> - if (ret < 0)
> - goto unmap_ctl;
> -
> - _host->irq = ret;
> + for (i = 0; i < 3; i++) {
> + _host->irq[i] = ret = platform_get_irq(pdev, i);
> + if (ret < 0) {
> + if (i = 1)
> + break;
> + else
> + goto unmap_ctl;
> + }
> + }
>
> tmio_mmc_disable_mmc_irqs(_host, TMIO_MASK_ALL);
> if (pdata->flags & TMIO_MMC_SDIO_IRQ)
> tmio_mmc_enable_sdio_irq(mmc, 0);
>
> - ret = request_irq(_host->irq, tmio_mmc_irq, IRQF_DISABLED |
> + for (i = 0; _host->irq[i] >= 0 && i < 3; i++) {
> + ret = request_irq(_host->irq[i], tmio_mmc_irq, IRQF_DISABLED |
> IRQF_TRIGGER_FALLING, dev_name(&pdev->dev), _host);
> - if (ret)
> - goto unmap_ctl;
> + if (ret) {
> + while (--i > 0)
> + free_irq(_host->irq[i], host);
> + goto unmap_ctl;
> + }
> + }
>
> spin_lock_init(&_host->lock);
>
> @@ -885,10 +894,13 @@ EXPORT_SYMBOL(tmio_mmc_host_probe);
>
> void tmio_mmc_host_remove(struct tmio_mmc_host *host)
> {
> + int i;
> +
> mmc_remove_host(host->mmc);
> cancel_delayed_work_sync(&host->delayed_reset_work);
> tmio_mmc_release_dma(host);
> - free_irq(host->irq, host);
> + for (i = 0; host->irq[i] >= 0 && i < 3; i++)
> + free_irq(host->irq[i], host);
> iounmap(host->ctl);
> mmc_free_host(host->mmc);
> }
> --
> 1.7.4.1
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] mmc: tmio: Allow SDHI MMC to use multiple IRQ
2011-04-27 7:43 [PATCH 1/2] mmc: tmio: Allow SDHI MMC to use multiple IRQ vectors Simon Horman
2011-05-02 8:55 ` Guennadi Liakhovetski
@ 2011-05-02 11:16 ` Simon Horman
2011-05-02 12:21 ` [PATCH 1/2] mmc: tmio: Allow SDHI MMC to use multiple IRQ vectors Magnus Damm
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2011-05-02 11:16 UTC (permalink / raw)
To: linux-sh
On Mon, May 02, 2011 at 10:55:22AM +0200, Guennadi Liakhovetski wrote:
> Hi Simon
>
> On Wed, 27 Apr 2011, Simon Horman wrote:
>
> > SDHI blocks may have up to 4 IRQ vectors.
> > The first three are of use to us, the 4th one
> > relates to DRM features that I don't have documentation
> > for and are almost certainly tainted by licensing issues.
> >
> > This patch allows multiple vectors to be used if supplied
> > in the platform data. Which will allow IRQ multiplexing hacks
> > to be removed.
> >
> > I plan to do further work to split tmio_mmc_irq() into per-vector
> > handlers. But this patch should be useful by itself.
>
> I have one nitpick: using a constant like "3" at multiple locations makes
> code less future-proof. I would use a macro like TMIO_MMC_MAX_IRQS, at
> some locations you can also do ARRAY_SIZE(host->irq).
Good thinking, I'll fix that up.
> Also, are we sure,
> that only "1 IRQ" or "max IRQs" (currently 3) are valid? Can there not be
> a valid configuration with 2 IRQs? Don't we have such controllers on some
> SoCs? For example, ap4 SDHI0 and SDHI2 have 4 IRQ vectors, but SDHI1 only
> 3... But ok, no, I don't see any controllers with < 3 IRQs, so, should
> work for now.
As far as I know there are currently controllers with 1, 3 and 4 IRQs.
But in the case of controllers with 4 IRQs we can treat this as 3,
as the 4th IRQ has some secret IP sauce that I believe we are best to
say away from.
So handling 1 and 3 IRQs is sufficient to deal with all controllers that
I am aware of. If there are other combinations that need to be handled
the obviously my code will need to be extended accordingly.
I think the easiest way to support other numbers of IRQs would be to probe
up to TMIO_MMC_MAX_IRQS, And then either record how many we found in a new
element of tmio_mmc_host, or make the irq[] element terminated by a
negative value.
I can change the code around to do that, though I think that
I prefer the current approach given the hardware that I am aware of.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] mmc: tmio: Allow SDHI MMC to use multiple IRQ vectors
2011-04-27 7:43 [PATCH 1/2] mmc: tmio: Allow SDHI MMC to use multiple IRQ vectors Simon Horman
2011-05-02 8:55 ` Guennadi Liakhovetski
2011-05-02 11:16 ` [PATCH 1/2] mmc: tmio: Allow SDHI MMC to use multiple IRQ Simon Horman
@ 2011-05-02 12:21 ` Magnus Damm
2011-05-02 22:05 ` [PATCH 1/2] mmc: tmio: Allow SDHI MMC to use multiple IRQ Simon Horman
2011-05-02 22:39 ` Simon Horman
4 siblings, 0 replies; 6+ messages in thread
From: Magnus Damm @ 2011-05-02 12:21 UTC (permalink / raw)
To: linux-sh
Hey Simon,
On Mon, May 2, 2011 at 8:16 PM, Simon Horman <horms@verge.net.au> wrote:
> On Mon, May 02, 2011 at 10:55:22AM +0200, Guennadi Liakhovetski wrote:
>> On Wed, 27 Apr 2011, Simon Horman wrote:
>>
>> > SDHI blocks may have up to 4 IRQ vectors.
>> > The first three are of use to us, the 4th one
>> > relates to DRM features that I don't have documentation
>> > for and are almost certainly tainted by licensing issues.
>> >
>> > This patch allows multiple vectors to be used if supplied
>> > in the platform data. Which will allow IRQ multiplexing hacks
>> > to be removed.
>> >
>> > I plan to do further work to split tmio_mmc_irq() into per-vector
>> > handlers. But this patch should be useful by itself.
>>
>> I have one nitpick: using a constant like "3" at multiple locations makes
>> code less future-proof. I would use a macro like TMIO_MMC_MAX_IRQS, at
>> some locations you can also do ARRAY_SIZE(host->irq).
>
> Good thinking, I'll fix that up.
>
>> Also, are we sure,
>> that only "1 IRQ" or "max IRQs" (currently 3) are valid? Can there not be
>> a valid configuration with 2 IRQs? Don't we have such controllers on some
>> SoCs? For example, ap4 SDHI0 and SDHI2 have 4 IRQ vectors, but SDHI1 only
>> 3... But ok, no, I don't see any controllers with < 3 IRQs, so, should
>> work for now.
>
> As far as I know there are currently controllers with 1, 3 and 4 IRQs.
> But in the case of controllers with 4 IRQs we can treat this as 3,
> as the 4th IRQ has some secret IP sauce that I believe we are best to
> say away from.
>
> So handling 1 and 3 IRQs is sufficient to deal with all controllers that
> I am aware of. If there are other combinations that need to be handled
> the obviously my code will need to be extended accordingly.
>
> I think the easiest way to support other numbers of IRQs would be to probe
> up to TMIO_MMC_MAX_IRQS, And then either record how many we found in a new
> element of tmio_mmc_host, or make the irq[] element terminated by a
> negative value.
>
> I can change the code around to do that, though I think that
> I prefer the current approach given the hardware that I am aware of.
In the future I imagine we may want to associate different interrupt
handler functions to each interrupt source, so from that point of view
it may make sense to move the request_irq() out from
tmio_mmc_host_probe() into sh_mobile_sdhi_probe() and
tmio_mmc_probe(). Same thing with the free_irq() implementation in
xxx_remove(). I guess tmio_mmc_irq() needs to be global and use
EXPORT_SYMBOL() for such a break out.
If we break out the IRQ part of the code we can also easily modify the
SDHI code to get rid of the IRQF_TRIGGER_FALLING flag that isn't
working on GIC-enabled platforms like AG5. Such a break out would also
remove the need for any array of interrupts and TMIO_MMC_MAX_IRQS.
There may be some side effects of the suggestion above though, but anyway...
Thanks,
/ magnus
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] mmc: tmio: Allow SDHI MMC to use multiple IRQ
2011-04-27 7:43 [PATCH 1/2] mmc: tmio: Allow SDHI MMC to use multiple IRQ vectors Simon Horman
` (2 preceding siblings ...)
2011-05-02 12:21 ` [PATCH 1/2] mmc: tmio: Allow SDHI MMC to use multiple IRQ vectors Magnus Damm
@ 2011-05-02 22:05 ` Simon Horman
2011-05-02 22:39 ` Simon Horman
4 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2011-05-02 22:05 UTC (permalink / raw)
To: linux-sh
On Mon, May 02, 2011 at 09:21:37PM +0900, Magnus Damm wrote:
> Hey Simon,
>
> On Mon, May 2, 2011 at 8:16 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Mon, May 02, 2011 at 10:55:22AM +0200, Guennadi Liakhovetski wrote:
> >> On Wed, 27 Apr 2011, Simon Horman wrote:
> >>
> >> > SDHI blocks may have up to 4 IRQ vectors.
> >> > The first three are of use to us, the 4th one
> >> > relates to DRM features that I don't have documentation
> >> > for and are almost certainly tainted by licensing issues.
> >> >
> >> > This patch allows multiple vectors to be used if supplied
> >> > in the platform data. Which will allow IRQ multiplexing hacks
> >> > to be removed.
> >> >
> >> > I plan to do further work to split tmio_mmc_irq() into per-vector
> >> > handlers. But this patch should be useful by itself.
> >>
> >> I have one nitpick: using a constant like "3" at multiple locations makes
> >> code less future-proof. I would use a macro like TMIO_MMC_MAX_IRQS, at
> >> some locations you can also do ARRAY_SIZE(host->irq).
> >
> > Good thinking, I'll fix that up.
> >
> >> Also, are we sure,
> >> that only "1 IRQ" or "max IRQs" (currently 3) are valid? Can there not be
> >> a valid configuration with 2 IRQs? Don't we have such controllers on some
> >> SoCs? For example, ap4 SDHI0 and SDHI2 have 4 IRQ vectors, but SDHI1 only
> >> 3... But ok, no, I don't see any controllers with < 3 IRQs, so, should
> >> work for now.
> >
> > As far as I know there are currently controllers with 1, 3 and 4 IRQs.
> > But in the case of controllers with 4 IRQs we can treat this as 3,
> > as the 4th IRQ has some secret IP sauce that I believe we are best to
> > say away from.
> >
> > So handling 1 and 3 IRQs is sufficient to deal with all controllers that
> > I am aware of. If there are other combinations that need to be handled
> > the obviously my code will need to be extended accordingly.
> >
> > I think the easiest way to support other numbers of IRQs would be to probe
> > up to TMIO_MMC_MAX_IRQS, And then either record how many we found in a new
> > element of tmio_mmc_host, or make the irq[] element terminated by a
> > negative value.
> >
> > I can change the code around to do that, though I think that
> > I prefer the current approach given the hardware that I am aware of.
>
> In the future I imagine we may want to associate different interrupt
> handler functions to each interrupt source, so from that point of view
> it may make sense to move the request_irq() out from
> tmio_mmc_host_probe() into sh_mobile_sdhi_probe() and
> tmio_mmc_probe(). Same thing with the free_irq() implementation in
> xxx_remove(). I guess tmio_mmc_irq() needs to be global and use
> EXPORT_SYMBOL() for such a break out.
That sounds like a lot of work to go to in order to remove
IRQF_TRIGGER_FALLING for some platforms. But breaking out
the code may be worth it for the case of multiple IRQ handlers.
Is there ever a case where sh_mobile_sdhi_probe() would
need to handle a single IRQ handler?
> If we break out the IRQ part of the code we can also easily modify the
> SDHI code to get rid of the IRQF_TRIGGER_FALLING flag that isn't
> working on GIC-enabled platforms like AG5. Such a break out would also
> remove the need for any array of interrupts and TMIO_MMC_MAX_IRQS.
I noticed that your original implementation didn't have an array of
interrupts. Which seems to basically mean that free_irq() can't be called
in tmio_mmc_host_remove() (or anywhere else on removal). What am I missing?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] mmc: tmio: Allow SDHI MMC to use multiple IRQ
2011-04-27 7:43 [PATCH 1/2] mmc: tmio: Allow SDHI MMC to use multiple IRQ vectors Simon Horman
` (3 preceding siblings ...)
2011-05-02 22:05 ` [PATCH 1/2] mmc: tmio: Allow SDHI MMC to use multiple IRQ Simon Horman
@ 2011-05-02 22:39 ` Simon Horman
4 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2011-05-02 22:39 UTC (permalink / raw)
To: linux-sh
On Tue, May 03, 2011 at 07:05:56AM +0900, Simon Horman wrote:
> On Mon, May 02, 2011 at 09:21:37PM +0900, Magnus Damm wrote:
[ snip ]
> > If we break out the IRQ part of the code we can also easily modify the
> > SDHI code to get rid of the IRQF_TRIGGER_FALLING flag that isn't
> > working on GIC-enabled platforms like AG5. Such a break out would also
> > remove the need for any array of interrupts and TMIO_MMC_MAX_IRQS.
>
> I noticed that your original implementation didn't have an array of
> interrupts. Which seems to basically mean that free_irq() can't be called
> in tmio_mmc_host_remove() (or anywhere else on removal). What am I missing?
I see that you answered that witha patch.
^ permalink raw reply [flat|nested] 6+ messages in thread