Date: Wed, 31 Aug 2022 11:15:18 +0800 From: Ganbold Tsagaankhuu <ganbold@gmail.com> To: Peter Jeremy <peterj@freebsd.org> Cc: 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: <CAGtf9xOox5aPozG9qEw3d1APaE9LpbP=RZqLnd369MRk20BuDA@mail.gmail.com> In-Reply-To: <YwGu8dSqAcVoMNt2@server.rulingia.com> References: <202208191322.27JDMv0b007432@gitrepo.freebsd.org> <YwGu8dSqAcVoMNt2@server.rulingia.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--000000000000c4b41005e780e836 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Peter, On Sun, Aug 21, 2022 at 12:05 PM 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 o= n > 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. > > 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). > Does the following patch work for you? diff --git a/sys/arm64/rockchip/rk_gpio.c b/sys/arm64/rockchip/rk_gpio.c index c3b1044df2f7..d41a9077d0cc 100644 --- a/sys/arm64/rockchip/rk_gpio.c +++ b/sys/arm64/rockchip/rk_gpio.c @@ -259,12 +259,13 @@ static int rk_gpio_attach(device_t dev) { struct rk_gpio_softc *sc; - phandle_t node; + phandle_t parent_node, node; int err, i; sc =3D device_get_softc(dev); sc->sc_dev =3D dev; sc->pinctrl =3D device_get_parent(dev); + parent_node =3D ofw_bus_get_node(sc->pinctrl); node =3D ofw_bus_get_node(sc->sc_dev); if (!OF_hasprop(node, "gpio-controller")) @@ -303,9 +304,11 @@ rk_gpio_attach(device_t dev) return (ENXIO); } - RK_GPIO_LOCK(sc); - sc->version =3D rk_gpio_read_4(sc, RK_GPIO_VERSION); - RK_GPIO_UNLOCK(sc); + /* RK3568 has GPIO_VER_ID register */ + if (ofw_bus_node_is_compatible(parent_node, "rockchip,rk3568-pinctrl")) + sc->version =3D RK_GPIO_TYPE_V2; + else + sc->version =3D RK_GPIO_TYPE_V1; switch (sc->version) { case RK_GPIO_TYPE_V1: Ganbold > > -- > Peter Jeremy > --000000000000c4b41005e780e836 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div dir=3D"ltr">Peter,</div><br><div class=3D"gmail_quote= "><div dir=3D"ltr" class=3D"gmail_attr">On Sun, Aug 21, 2022 at 12:05 PM Pe= ter Jeremy <<a href=3D"mailto:peterj@freebsd.org">peterj@freebsd.org</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 2= 022-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=3D901df07a47684= dca7b06f60d838a56456d751a23" rel=3D"noreferrer" target=3D"_blank">https://c= git.FreeBSD.org/src/commit/?id=3D901df07a47684dca7b06f60d838a56456d751a23</= a><br> ><br> >commit 901df07a47684dca7b06f60d838a56456d751a23<br> >Author:=C2=A0 =C2=A0 =C2=A0S=C3=B8ren Schmidt <sos@FreeBSD.org><b= r> >AuthorDate: 2022-08-19 13:22:01 +0000<br> >Commit:=C2=A0 =C2=A0 =C2=A0Ganbold Tsagaankhuu <ganbold@FreeBSD.org&= gt;<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 type so= me<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://reviews.f= reebsd.org/D36262" rel=3D"noreferrer" target=3D"_blank">https://reviews.fre= ebsd.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=A0 =C2=A0rk_gpio_detach(d= ev);<br> >=C2=A0 =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 =C2=A0 RK_GPIO_LOCK(sc);<br> >+=C2=A0 =C2=A0 =C2=A0 sc->version =3D rk_gpio_read_4(sc, RK_GPIO_VER= SION);<br> >+=C2=A0 =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.<br> b) The second argument to rk_gpio_read_4() is a "enum gpio_regs",= not<br> =C2=A0 =C2=A0an actual offset - RK_GPIO_VERSION (=3D0x78) is way outside th= e<br> =C2=A0 =C2=A0sc->regs array.<br> c) sc->regs is also uninitialised at this point.<br> <br> Maybe this should call RK_GPIO_READ() instead, but neither my RK3328<br> 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 interested= <br> in a copy).<br></blockquote><div><br></div><div>Does the following patch wo= rk for you?</div><div><br></div><div>diff --git a/sys/arm64/rockchip/rk_gpi= o.c b/sys/arm64/rockchip/rk_gpio.c<br>index c3b1044df2f7..d41a9077d0cc 1006= 44<br>--- a/sys/arm64/rockchip/rk_gpio.c<br>+++ b/sys/arm64/rockchip/rk_gpi= o.c<br>@@ -259,12 +259,13 @@ static int<br>=C2=A0rk_gpio_attach(device_t de= v)<br>=C2=A0{<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 struct rk_gpio_softc *sc;<br>-= =C2=A0 =C2=A0 =C2=A0 phandle_t node;<br>+ =C2=A0 =C2=A0 =C2=A0 phandle_t p= arent_node, node;<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 int err, i;<br><br>=C2=A0 = =C2=A0 =C2=A0 =C2=A0 sc =3D device_get_softc(dev);<br>=C2=A0 =C2=A0 =C2=A0 = =C2=A0 sc->sc_dev =3D dev;<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 sc->pinctrl= =3D device_get_parent(dev);<br>+ =C2=A0 =C2=A0 =C2=A0 parent_node =3D ofw_= bus_get_node(sc->pinctrl);<br><br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 node =3D o= fw_bus_get_node(sc->sc_dev);<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!OF_hasp= rop(node, "gpio-controller"))<br>@@ -303,9 +304,11 @@ rk_gpio_att= ach(device_t dev)<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 return (ENXIO);<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 }<br><br>- =C2=A0 =C2=A0= =C2=A0 RK_GPIO_LOCK(sc);<br>- =C2=A0 =C2=A0 =C2=A0 sc->version =3D rk_g= pio_read_4(sc, RK_GPIO_VERSION);<br>- =C2=A0 =C2=A0 =C2=A0 RK_GPIO_UNLOCK(s= c);<br>+ =C2=A0 =C2=A0 =C2=A0 /* RK3568 has GPIO_VER_ID register */<br>+ = =C2=A0 =C2=A0 =C2=A0 if (ofw_bus_node_is_compatible(parent_node, "rock= chip,rk3568-pinctrl"))<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 sc->version =3D RK_GPIO_TYPE_V2;<br>+ =C2=A0 =C2=A0 =C2=A0 else<b= r>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sc->version =3D RK_= GPIO_TYPE_V1;<br><br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 switch (sc->version) {<= br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 case RK_GPIO_TYPE_V1:<br></div><div><br></di= v><div><br></div><div>Ganbold</div><div><br></div><div>=C2=A0</div><blockqu= ote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px= solid rgb(204,204,204);padding-left:1ex"> <br> -- <br> Peter Jeremy<br> </blockquote></div></div> --000000000000c4b41005e780e836--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGtf9xOox5aPozG9qEw3d1APaE9LpbP=RZqLnd369MRk20BuDA>