On Sun, 13 Mar 2011, Magnus Damm wrote:
> On Sun, Mar 13, 2011 at 7:57 AM, Guennadi Liakhovetski
> <g.liakhovetski@xxxxxx> wrote:
> > On Sun, 13 Mar 2011, Magnus Damm wrote:
> >
> >> On Fri, Mar 11, 2011 at 5:09 PM, Guennadi Liakhovetski
> >> <g.liakhovetski@xxxxxx> wrote:
> >> > Not all mn57xx / tmio implementations have registers above oxff.
> >> > Accessing them on thise platforms is dangerous. In some cases it leads
> >> > to address wrapping to addresses below 0x100, which corrupts random
> >> > unrelated registers.
> >> >
> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> >> > ---
> >> >
> >> > v2:
> >>
> >> > --- a/drivers/mmc/host/tmio_mmc_pio.c
> >> > +++ b/drivers/mmc/host/tmio_mmc_pio.c
> >>
> >> Nice, it looks like this patch should apply on your earlier broken out
> >> patch set...
> >>
> >> > + if (resource_size(res) > 0x100) {
> >> > + sd_ctrl_write16(host, CTL_CLK_AND_WAIT_CTL, 0x0000);
> >> > + msleep(10);
> >> > + }
> >>
> >> ... but now since you've separated the tmio specific code from the
> >> SDHI implementation, why don't you just make all the offsets above
> >> 0x100 stay in the tmio code, and let the SDHI only access below 0xff?
> >
> > The current split is:
> >
> > tmio_mmc_pio.c: _all_ driver functionality in PIO mode, including MMC API
> > tmio_mmc_dma.c: DMA
> >
> > tmio_mmc.c: _only_ MFD glue - no IO at all
> >
> > sh_mobile_sdhi.c: _only_ platform glue for SDHI - no IO at all
> >
> > This split seems logical to me - every file performs one function. But I
> > do agree, that special-casing addresses > 0xff in the generic core is not
> > very nice... We could otherwise modify the actual read / write accessors
> > to check, whether _each_ register address is within the resource, but that
> > would be too much of a performance hit. But pulling those couple of > 0xff
> > accesses out of the driver proper and into the MFD glue didn't look like a
> > very logical thing to me. I thought this version provided both - no
> > performance hit and preserved clean function separation - at the cost of
> > "slight ugliness." Opinions?
>
> I don't really see why you want to have "no IO at all" in the
> tmio/sdhi specific files.
>
> Actually, I think it makes sense to abstract based on the hardware
> register layout difference.
>
> Since some parts of the code only applies to tmio (offset >= 0x100)
> and some only to SDHI (TMIO_MMC_SDIO_IRQ), why don't you do the
> following:
>
> In tmio_mmc_pio.c:
>
> tmio_mmc_core_clk_start()
> tmio_mmc_core_clk_stop()
> tmio_mmc_core_reset()
...and then you'd have to add pointers to these functions to the tmio
struct to be able to call them from the core (*_pio) and check it for NULL
and-and-and... And this instead of just one test for resource size >
0x100. Do you really think it's worth it? Basically, when your code has to
act differently in different situations you can either check some
parameters, set by the environment, or, in more complicated cases, call
some environment callback. I think this case is still simple enough - you
even don't have to add any new parameters - they are already there - the
resource size. You just have to check it.
We might well need some callbacks in the future, when we implement
multiple interrupts or some even more significant differences, but this
looks like a really simple thing to me, and your approach makes it a bit
overly complicated, don't you think?
Thanks
Guennadi
>
> Above functions only implement offset < 0x100 and no TMIO_MMC_ stuff
> in there if possible
>
> In tmio_mmc.c
>
> tmio_mmc_clk_start()
> tmio_mmc_clk_stop()
> tmio_mmc_reset()
>
> Above 3 functions call tmio_mmc_core_xxx() and also implement >= 0x100
> offset access.
>
> In sh_mobile_sdhi.c
>
> sh_mobile_sdhi_clk_start()
> sh_mobile_sdhi_clk_stop()
> sh_mobile_sdhi_reset()
>
> Above 3 functions call tmio_mmc_core_xxx() and perhaps also implement SDIO IRQ.
>
> Ideally we want to share the SDIO IRQ code with the TMIO MFD
> implementation, but we're not really receiving any test feedback from
> anyone, so from my POV to avoid breakage we may as well keep things in
> the SDHI portion and move over to the common place if needed in the
> future.
>
> / magnus
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
