Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Wolfram Sang <wsa@the-dreams.de>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
	Wolfram Sang <wsa+renesas@sang-engineering.com>,
	Linux I2C <linux-i2c@vger.kernel.org>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
Subject: Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3
Date: Wed, 27 Jun 2018 10:51:10 +0200	[thread overview]
Message-ID: <CAMuHMdX6ON5o8TAap18BiKXdxMtTzpkJGU+Z1yAEhT587V05aA@mail.gmail.com> (raw)
In-Reply-To: <20180627084421.zrtpsms5eyftqcfb@ninjato>

Hi Wolfram,

On Wed, Jun 27, 2018 at 10:44 AM Wolfram Sang <wsa@the-dreams.de> wrote:
> > > The hardware team said:
> > >  - In CPG point of view:
> > >    - such polling doesn't need (because the reset pulse is generated correctly).
> > >    - About the interval after deassert the reset, this is depend on each hardware module.
> > >      (In other words, if each hardware module has a special handling about after the deassert interval,
> > >       we should follow the special handling.)
> > >  - The I2C controller needs an interval of reading SRCR at least (this is a special handling).
> > >
> > > So, I think adding this code is not good fit in CPG point of view.
> >
> > Calling reset_control_status() from i2c-rcar is fine for me.
> >
>
> Doesn't make this writing device drivers a bit harder, I wonder? If we
> follow the above, we need to know per driver (like i2c-rcar.c) if
> reset_control_reset() is enough or if we need to call
> reset_control_status() additionally. For a driver author, it would be
> much less error prone IMHO, if reset_control_reset() just does the right
> thing. It has also the advantage that we can handle special handling on
> different SoCs differently (if that is ever needed) because MSSR driver
> is per SoC.

True.

But this seems to be a bug in the R-Car Gen3 I2C controller implementation.

> > Note that reset controller support is optional, so we want to add
> >
> >         select RESET_CONTROLLER if ARCH_RENESAS && ARM64
> >
> > to the I2C_RCAR section drivers/i2c/busses/Kconfig. Else reset will fail
> > silently.
>
> Technically, it should also be "&& HAS_DMA". But this is maybe a tad too
> defensive?

s/HAS_DMA/RCAR_DMAC/ :-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds