Date: Wed, 31 Aug 2022 10:11:47 +0800 From: Ganbold Tsagaankhuu <ganbold@gmail.com> To: Emmanuel Vadot <manu@bidouilliste.com> Cc: Peter Jeremy <peterj@freebsd.org>, Ganbold Tsagaankhuu <ganbold@freebsd.org>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: 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. Message-ID: <CAGtf9xOQ7hfzsW=PBqs-%2B8iQwkTpZngrHBFN5dRgZugKh1-W0w@mail.gmail.com> In-Reply-To: <20220821093952.08d2a29ea9c2c47aae1e0ea5@bidouilliste.com> References: <202208191322.27JDMv0b007432@gitrepo.freebsd.org> <YwGu8dSqAcVoMNt2@server.rulingia.com> <20220821093952.08d2a29ea9c2c47aae1e0ea5@bidouilliste.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--00000000000095c53005e78005ad Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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=3D901df07a47684dca7b06f60d838a564= 56d751a23 > > > > > >commit 901df07a47684dca7b06f60d838a56456d751a23 > > >Author: S=C3=B8ren 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 =3D 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 (=3D0x78) 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> > --00000000000095c53005e78005ad Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">= <div dir=3D"ltr" class=3D"gmail_attr">On Sun, Aug 21, 2022 at 3:40 PM Emman= uel Vadot <<a href=3D"mailto:manu@bidouilliste.com">manu@bidouilliste.co= m</a>> wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin= :0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"= >On Sun, 21 Aug 2022 14:05:05 +1000<br> Peter Jeremy <<a href=3D"mailto:peterj@freebsd.org" target=3D"_blank">pe= terj@freebsd.org</a>> wrote:<br> <br> > On 2022-Aug-19 13:22:57 +0000, Ganbold Tsagaankhuu <ganbold@FreeBSD= .org> wrote:<br> > >The branch main has been updated by ganbold:<br> > ><br> > >URL: <a href=3D"https://cgit.FreeBSD.org/src/commit/?id=3D901df07a= 47684dca7b06f60d838a56456d751a23" rel=3D"noreferrer" target=3D"_blank">http= s://cgit.FreeBSD.org/src/commit/?id=3D901df07a47684dca7b06f60d838a56456d751= a23</a><br> > ><br> > >commit 901df07a47684dca7b06f60d838a56456d751a23<br> > >Author:=C2=A0 =C2=A0 =C2=A0S=C3=B8ren Schmidt <sos@FreeBSD.org&= gt;<br> > >AuthorDate: 2022-08-19 13:22:01 +0000<br> > >Commit:=C2=A0 =C2=A0 =C2=A0Ganbold Tsagaankhuu <ganbold@FreeBSD= .org><br> > >CommitDate: 2022-08-19 13:22:01 +0000<br> > ><br> > >=C2=A0 =C2=A0 Code refactoring for existing rk_gpio driver.<br> > >=C2=A0 =C2=A0 It supports gpio type checking. Depending on gpio ty= pe some<br> > >=C2=A0 =C2=A0 register addresses are different.<br> > >=C2=A0 =C2=A0 <br> > >=C2=A0 =C2=A0 Reviewed by:=C2=A0 =C2=A0 manu<br> > >=C2=A0 =C2=A0 Differential Revision:=C2=A0 <a href=3D"https://revi= ews.freebsd.org/D36262" rel=3D"noreferrer" target=3D"_blank">https://review= s.freebsd.org/D36262</a><br> > <br> > My Rock64 is now hanging on boot as follows:<br> > rk_pinctrl0: <RockChip Pinctrl controller> on ofwbus0<br> > gpio0: <RockChip GPIO Bank controller> mem 0xff210000-0xff2100ff= irq 53 on rk_pinctrl0<br> > gpio0: Unknown gpio version 48000000<br> > <<hang>><br> > <br> > >@@ -170,6 +221,43 @@ rk_gpio_attach(device_t dev)<br> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0rk_gpio_detach(dev= );<br> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return (ENXIO);<br= > > >=C2=A0 =C2=A0 =C2=A0}=C2=A0<br> > >+=C2=A0 =C2=A0 RK_GPIO_LOCK(sc);<br> > >+=C2=A0 =C2=A0 sc->version =3D rk_gpio_read_4(sc, RK_GPIO_VERSI= ON);<br> > >+=C2=A0 =C2=A0 RK_GPIO_UNLOCK(sc);<br> > <br> > This call to rk_gpio_read_4() looks wrong:<br> > a) rk_gpio_read_4() tests sc->version which this call is setting.<b= r> > b) The second argument to rk_gpio_read_4() is a "enum gpio_regs&q= uot;, not<br> >=C2=A0 =C2=A0 an actual offset - RK_GPIO_VERSION (=3D0x78) is way outsi= de the<br> >=C2=A0 =C2=A0 sc->regs array.<br> > c) sc->regs is also uninitialised at this point.<br> <br> =C2=A0Yes indeed, sorry to not have spotted that during review.<br> <br> > Maybe this should call RK_GPIO_READ() instead, but neither my RK3328<b= r> > TRM (revision 1.2 from July 2017) nor my RK3399 TRM (revision 1.4 from= <br> > April 2017) document a GPIO register at offset 0x78 - both only go to<= br> > 0x60.=C2=A0 (If you have a later TRM for either chip, I would be inter= ested<br> > in a copy).<br></blockquote><div><br></div><div>Sorry for the break.</= div><div>I double checked those TRMs for RK3328/RK3399 and there seems no<b= r></div><div>GPIO_VER_ID register as RK3568 does.</div><div>=C2=A0</div><bl= ockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-lef= t:1px solid rgb(204,204,204);padding-left:1ex"> <br> =C2=A0We should match the version based on the compat string of the parent.= <br> =C2=A0In a perfect world the RK356x gpio controller shouldn't have the = same<br> compatible as the other ones as the register map isn't the same but ...= <br></blockquote><div><br></div><div>As manu@ said, compat string match is = maybe better.</div><div><div>I will look into it.=C2=A0</div><br class=3D"g= mail-Apple-interchange-newline"></div><div>Ganbold</div><div>=C2=A0</div><d= iv>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0p= x 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> -- <br> Emmanuel Vadot <<a href=3D"mailto:manu@bidouilliste.com" target=3D"_blan= k">manu@bidouilliste.com</a>> <<a href=3D"mailto:manu@freebsd.org" ta= rget=3D"_blank">manu@freebsd.org</a>><br> </blockquote></div></div> --00000000000095c53005e78005ad--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGtf9xOQ7hfzsW=PBqs-%2B8iQwkTpZngrHBFN5dRgZugKh1-W0w>