[v3,0/3] mmc: tmio_mmc: Add support for RZ/A2

Basically the same HW block that was used in R-Car Gen 3 is used in
RZ/A2 (with only a couple small differences).

Not sure if you're going to like the Kconfig change or not.

Chris Brandt (3):
  clk: renesas: r7s9210: Add SDHI clocks
  mmc: renesas_sdhi_internal_dmac: Add R7S9210 support
  dt-bindings: mmc: tmio_mmc: Document Renesas R7S9210

 Documentation/devicetree/bindings/mmc/tmio_mmc.txt |  3 ++-
 drivers/clk/renesas/r7s9210-cpg-mssr.c             |  5 ++++
 drivers/mmc/host/Kconfig                           |  5 ++--
 drivers/mmc/host/renesas_sdhi_internal_dmac.c      | 28 ++++++++++++++++++++--
 4 files changed, 36 insertions(+), 5 deletions(-)

Hi Chris,

> +/* RZ/A2 does not have the ADRR_MODE bit */
> +#define SDHI_INTERNAL_DMAC_ADRR_MODE_FIXED 2

First, there is a typo: s/ADRR/ADDR/g

Also, I think it would make the code much more comprehensible if this
macro was named SDHI_INTERNAL_DMAC_ADDR_MODE_BROKEN. Or maybe
SDHI_INTERNAL_DMAC_FIXED_ADDR_MODE_BROKEN. Currently, on first read I
thought this mode was fixed on this SoC and broken on all the others and
was confused.

What do you think?

Rest looks okay to me!

Regards,

   Wolfram

Hi Wolfram,

On Friday, October 12, 2018, Wolfram Sang wrote:
> > +/* RZ/A2 does not have the ADRR_MODE bit */
> > +#define SDHI_INTERNAL_DMAC_ADRR_MODE_FIXED 2
> 
> First, there is a typo: s/ADRR/ADDR/g

Thanks!

Even after you pointed that out...I had to stare real hard to see it. I 
guess the brain corrects what you see.


> Also, I think it would make the code much more comprehensible if this
> macro was named SDHI_INTERNAL_DMAC_ADDR_MODE_BROKEN. Or maybe
> SDHI_INTERNAL_DMAC_FIXED_ADDR_MODE_BROKEN. Currently, on first read I
> thought this mode was fixed on this SoC and broken on all the others and
> was confused.

To be honest, I was not able to fully understand the issue in detail. I was getting information through someone that was not in the design team. So to be fair to the chip design guys, I want to avoid the word "broken".
They took the bit out of the hardware manual, and since the HW was designed to work either way (and the default register setting is for fixed), I consider it an 'unsupported feature'.

So, how about a compromise of:

#define SDHI_INTERNAL_DMAC_ADDR_MODE_FIXED_ONLY 2


>From your other email:
> > As you can imagine, it does have this bit. And it worked fine from me.
> > But the chip guys said they found something not right with it, so they
> > removed it from the v1.0 Hardware Manual.
> 
> Do you happen to know if this applies for Gen3 SDHI as well?

I don't think this applies to Gen3. This is just a RZ/A2 thing.

With that said, there are some Gen3 HW bugs that are in RZ/A2 HW, like the QSPI read bug. Since the RZ/A2 has the exact same HW as Gen3, it has the same HW bug. Now that one is a "bug", and I don't mind calling that one broken.


Chris

> Even after you pointed that out...I had to stare real hard to see it. I 
> guess the brain corrects what you see.

Yes. c57d3e7a9391 ("i2c-dev: Fix typo in ioctl name reference") fixed
something in 2015 which was around forever. I had to look twice at this
patch as well.

> I was getting information through someone that was not in the design
> team. So to be fair to the chip design guys, I want to avoid the word
> "broken". They took the bit out of the hardware manual, and since the
> HW was designed to work either way (and the default register setting
> is for fixed), I consider it an 'unsupported feature'.
> 
> So, how about a compromise of:
> 
> #define SDHI_INTERNAL_DMAC_ADDR_MODE_FIXED_ONLY 2

Perfect. Now we nailed it, I think.

> I don't think this applies to Gen3. This is just a RZ/A2 thing.

OK. Thanks for the heads up!