Re: [PATCH V2 0/5] mmc: add stm32 sdmmc controller

[Ludovic BARRE]

On 03/01/2018 11:44 AM, Ulf Hansson wrote:
> On 1 March 2018 at 10:57, Ludovic BARRE <ludovic.barre@xxxxxx> wrote:
> > Hi Ulf
> >
> > On 03/01/2018 10:06 AM, Ulf Hansson wrote:
> > > Hi Ludovic,
> > >
> > > On 28 February 2018 at 16:47, Ludovic Barre <ludovic.Barre@xxxxxx> wrote:
> > > > From: Ludovic Barre <ludovic.barre@xxxxxx>
> > > >
> > > > This patch serie adds support of stm32 SDMMC controller.
> > > > stm32h7 is the first SoC to use stm32 SDMMC controller
> > > > (previous SoC had pl180 controller).
> > >
> > >
> > > I am a not convinced this isn't a new improved variant of the pl180.
> > > According to register layout and the code you submitted in patch2,
> > > there are great similarities to pl180 and the mmci driver.
> >
> >
> > In fact, ST designers which created stm32-sdmmc hardware block from scratch
> > are the same which have done the modifications on pl180 variant (stm32
> > legacy f4, f7).
> > So some registers or bits name seem identical (or strongly inspirited) but
> > the engine and features are different.
>
> Well, in that case, I assume the driver would also need work
> differently, but when looking at the code in patch2 this doesn't seem
> to be the case.

I understand why you push to avoid drivers multiplication. But I'm
scared to squash 2 different hardware block which are their own roadmap
in the same drivers. I fear that it complicates the features management 
and maintenance of this driver.

there are some difference:
-the stm32-sdmmc use an internal dma (stm32-sdmmc is master on the bus), 
-idma alignment constraint.
-idma transfer mode single buffer, double buffer... (future evolution)
-need a command to stop transmission for data state machine
-same bit naming could have offset, mask width or set in different register.
=> I will try to synthesis register difference in a document
-voltage switch sequence
-delay block: calibration, tunning (sdr104)
-reset line required
-the same feature have not the same impact, example ddr mode
stm32:no bypass clk, activated in clk register
pl180: clkreg_neg_edge_enable, activated in datactrl register,
...

stm32-sdmmc is just at begining of its life cycle. Each revision of this 
block increases the difference with pl180. Today, I've just push the 
minimal driver to start a request, but I've not yet send all features of 
this revision like sdio, sdr104 support. After this revision there 
already are 2 other revisions in the pipe (at short term).

I am Out of Office with limited access to my e-mail till 2018 march 12th
I'll try to think about it on ski slope. euh, in fact no just ski and 
enjoy ;-)

BR
Ludo

> > You could find the datasheet of STM32H7x3 on:
> > http://www.st.com/content/ccc/resource/technical/document/reference_manual/group0/c9/a3/76/fa/55/46/45/fa/DM00314099/files/DM00314099.pdf/jcr:content/translations/en.DM00314099.pdf
> >
> > Chapters: 55 Secure digital input/output MultiMediaCard interface
> > (SDMMC)
>
> Thanks for sharing this. However this confirms my view, it looks
> exactly as a new improved mmci variant. :-)
>
> > This hardware block has own roadmap and some features are already in the
> > pipe for next SoC.
>
> That's fine. I don't have a problem extending the mmci driver, even
> several times, as to cope with new revisions.
>
> > For code design: like I also worked on pl180 in the past :-)
> > my code is inspirited of this driver.
>
> Right, that may explain things a bit.
>
> However, besides a re-name of the registers, I really think that the
> code execution, dealing with IRQs etc, is very similar to the mmci
> driver. Isn't it?
>
> So, I think it's at least worth to give it a go with the mmci driver
> first, to see if we can get it to work.
>
> I guess you understand why I am pushing!? This is about maintenance -
> and I really want to avoid having a yet another driver to maintain,
> unless we can extend an existing one.
>
> [...]
>
> Kind regards
> Uffe