Re: git: 901df07a4768 - main - Code refactoring for existing rk_gpio driver. It supports gpio type checking. Depending on gpio type some register addresses are different.

On Sun, Aug 21, 2022 at 3:40 PM Emmanuel Vadot <manu@bidouilliste.com>
wrote:

> On Sun, 21 Aug 2022 14:05:05 +1000
> Peter Jeremy <peterj@freebsd.org> wrote:
>
> > On 2022-Aug-19 13:22:57 +0000, Ganbold Tsagaankhuu <ganbold@FreeBSD.org>
> wrote:
> > >The branch main has been updated by ganbold:
> > >
> > >URL:
> https://cgit.FreeBSD.org/src/commit/?id=901df07a47684dca7b06f60d838a56456d751a23
> > >
> > >commit 901df07a47684dca7b06f60d838a56456d751a23
> > >Author:     Søren Schmidt <sos@FreeBSD.org>
> > >AuthorDate: 2022-08-19 13:22:01 +0000
> > >Commit:     Ganbold Tsagaankhuu <ganbold@FreeBSD.org>
> > >CommitDate: 2022-08-19 13:22:01 +0000
> > >
> > >    Code refactoring for existing rk_gpio driver.
> > >    It supports gpio type checking. Depending on gpio type some
> > >    register addresses are different.
> > >
> > >    Reviewed by:    manu
> > >    Differential Revision:  https://reviews.freebsd.org/D36262
> >
> > My Rock64 is now hanging on boot as follows:
> > rk_pinctrl0: <RockChip Pinctrl controller> on ofwbus0
> > gpio0: <RockChip GPIO Bank controller> mem 0xff210000-0xff2100ff irq 53
> on rk_pinctrl0
> > gpio0: Unknown gpio version 48000000
> > <<hang>>
> >
> > >@@ -170,6 +221,43 @@ rk_gpio_attach(device_t dev)
> > >             rk_gpio_detach(dev);
> > >             return (ENXIO);
> > >     }
> > >+    RK_GPIO_LOCK(sc);
> > >+    sc->version = rk_gpio_read_4(sc, RK_GPIO_VERSION);
> > >+    RK_GPIO_UNLOCK(sc);
> >
> > This call to rk_gpio_read_4() looks wrong:
> > a) rk_gpio_read_4() tests sc->version which this call is setting.
> > b) The second argument to rk_gpio_read_4() is a "enum gpio_regs", not
> >    an actual offset - RK_GPIO_VERSION (=0x78) is way outside the
> >    sc->regs array.
> > c) sc->regs is also uninitialised at this point.
>
>  Yes indeed, sorry to not have spotted that during review.
>
> > Maybe this should call RK_GPIO_READ() instead, but neither my RK3328
> > TRM (revision 1.2 from July 2017) nor my RK3399 TRM (revision 1.4 from
> > April 2017) document a GPIO register at offset 0x78 - both only go to
> > 0x60.  (If you have a later TRM for either chip, I would be interested
> > in a copy).
>

Sorry for the break.
I double checked those TRMs for RK3328/RK3399 and there seems no
GPIO_VER_ID register as RK3568 does.


>
>  We should match the version based on the compat string of the parent.
>  In a perfect world the RK356x gpio controller shouldn't have the same
> compatible as the other ones as the register map isn't the same but ...
>

As manu@ said, compat string match is maybe better.
I will look into it.

Ganbold



>
> --
> Emmanuel Vadot <manu@bidouilliste.com> <manu@freebsd.org>
>