| Message ID | 20171120185445.146061-1-venture@google.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [linux,dev-4.10] ARM: dts: aspeed: gpio controller register range | expand |
Can I get a status on this patch? The device datasheet has 4K reserved for the range, but only the first 512 are registers controlled by this driver. The rest will be handled by a separate driver. Or at least that's how it's planned. On Mon, Nov 20, 2017 at 10:54 AM, Patrick Venture <venture@google.com> wrote: > Instead of 4k, it should be 512 because the latter > part of the memory region are registers for the serial > gpio driver. > > Signed-off-by: Patrick Venture <venture@google.com> > --- > arch/arm/boot/dts/aspeed-g4.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi > index 6505062ac6c7..424d843ec1da 100644 > --- a/arch/arm/boot/dts/aspeed-g4.dtsi > +++ b/arch/arm/boot/dts/aspeed-g4.dtsi > @@ -231,7 +231,7 @@ > #gpio-cells = <2>; > gpio-controller; > compatible = "aspeed,ast2400-gpio"; > - reg = <0x1e780000 0x1000>; > + reg = <0x1e780000 0x200>; > interrupts = <20>; > interrupt-controller; > gpio-ranges = <&pinctrl 0 0 220>; > -- > 2.15.0.448.gf294e3d99a-goog >
On Thu, Nov 30, 2017 at 5:03 AM, Patrick Venture <venture@google.com> wrote: > Can I get a status on this patch? The device datasheet has 4K > reserved for the range, but only the first 512 are registers > controlled by this driver. The rest will be handled by a separate > driver. Or at least that's how it's planned. +cc the people who have worked on this. I think that's okay, but lets see what the others have to say. Do you have a driver for the rest of the GPIOs you can submit? I'd like to see that code so we can decide if there is good reason to make that completely separate, and not part of the existing driver. This work is best done upstream, as you will need to make the same arguments there. Cheers, Joel > > On Mon, Nov 20, 2017 at 10:54 AM, Patrick Venture <venture@google.com> wrote: >> Instead of 4k, it should be 512 because the latter >> part of the memory region are registers for the serial >> gpio driver. >> >> Signed-off-by: Patrick Venture <venture@google.com> >> --- >> arch/arm/boot/dts/aspeed-g4.dtsi | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi >> index 6505062ac6c7..424d843ec1da 100644 >> --- a/arch/arm/boot/dts/aspeed-g4.dtsi >> +++ b/arch/arm/boot/dts/aspeed-g4.dtsi >> @@ -231,7 +231,7 @@ >> #gpio-cells = <2>; >> gpio-controller; >> compatible = "aspeed,ast2400-gpio"; >> - reg = <0x1e780000 0x1000>; >> + reg = <0x1e780000 0x200>; >> interrupts = <20>; >> interrupt-controller; >> gpio-ranges = <&pinctrl 0 0 220>; >> -- >> 2.15.0.448.gf294e3d99a-goog >>
On Wed, Nov 29, 2017 at 7:53 PM, Joel Stanley <joel@jms.id.au> wrote: > On Thu, Nov 30, 2017 at 5:03 AM, Patrick Venture <venture@google.com> wrote: >> Can I get a status on this patch? The device datasheet has 4K >> reserved for the range, but only the first 512 are registers >> controlled by this driver. The rest will be handled by a separate >> driver. Or at least that's how it's planned. > > +cc the people who have worked on this. > > I think that's okay, but lets see what the others have to say. > > Do you have a driver for the rest of the GPIOs you can submit? I'd > like to see that code so we can decide if there is good reason to > make that completely separate, and not part of the existing driver. > > This work is best done upstream, as you will need to make the same > arguments there. I should have a driver ready sometime late next week that I'll be sending upstream first. I still need to finish up a few loose ends on it. It wouldn't be unreasonable to have them in the same driver, you'd just end up with two groups of gpios that are handled differently, and extra soft-irqs. That said, if you have two large groups of things that need to be treated differently, then that's somewhat of an argument for having them separate. :) I'd actually appreciate hearing some thoughts at this point on which way to go, since merging the serial handling into the main driver will require re-working a few things -- I'd rather take the extra time to handle that merge if it's going to head in that direction anyway. I have read through the gpio-aspeed driver before, as it was a bit useful as a guide, and I could see it getting a bit cluttered with also handling the serial gpios. Thoughts? > > Cheers, > > Joel > >> >> On Mon, Nov 20, 2017 at 10:54 AM, Patrick Venture <venture@google.com> wrote: >>> Instead of 4k, it should be 512 because the latter >>> part of the memory region are registers for the serial >>> gpio driver. >>> >>> Signed-off-by: Patrick Venture <venture@google.com> >>> --- >>> arch/arm/boot/dts/aspeed-g4.dtsi | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi >>> index 6505062ac6c7..424d843ec1da 100644 >>> --- a/arch/arm/boot/dts/aspeed-g4.dtsi >>> +++ b/arch/arm/boot/dts/aspeed-g4.dtsi >>> @@ -231,7 +231,7 @@ >>> #gpio-cells = <2>; >>> gpio-controller; >>> compatible = "aspeed,ast2400-gpio"; >>> - reg = <0x1e780000 0x1000>; >>> + reg = <0x1e780000 0x200>; >>> interrupts = <20>; >>> interrupt-controller; >>> gpio-ranges = <&pinctrl 0 0 220>; >>> -- >>> 2.15.0.448.gf294e3d99a-goog >>>
On Fri, Dec 1, 2017 at 2:05 AM, Patrick Venture <venture@google.com> wrote: > On Wed, Nov 29, 2017 at 7:53 PM, Joel Stanley <joel@jms.id.au> wrote: >> On Thu, Nov 30, 2017 at 5:03 AM, Patrick Venture <venture@google.com> wrote: >>> Can I get a status on this patch? The device datasheet has 4K >>> reserved for the range, but only the first 512 are registers >>> controlled by this driver. The rest will be handled by a separate >>> driver. Or at least that's how it's planned. >> >> +cc the people who have worked on this. >> >> I think that's okay, but lets see what the others have to say. >> >> Do you have a driver for the rest of the GPIOs you can submit? I'd >> like to see that code so we can decide if there is good reason to >> make that completely separate, and not part of the existing driver. >> >> This work is best done upstream, as you will need to make the same >> arguments there. > > I should have a driver ready sometime late next week that I'll be > sending upstream first. I still need to finish up a few loose ends on > it. It wouldn't be unreasonable to have them in the same driver, > you'd just end up with two groups of gpios that are handled > differently, and extra soft-irqs. That said, if you have two large > groups of things that need to be treated differently, then that's > somewhat of an argument for having them separate. :) > > I'd actually appreciate hearing some thoughts at this point on which > way to go, since merging the serial handling into the main driver will > require re-working a few things -- I'd rather take the extra time to > handle that merge if it's going to head in that direction anyway. > > I have read through the gpio-aspeed driver before, as it was a bit > useful as a guide, and I could see it getting a bit cluttered with > also handling the serial gpios. It sounds like a call that could go either way. If you think it would be easier to maintain two separate drivers then we can go that direction. I'd suggest discussing it upstream. The best way to discuss it would be in the form of a patch. Even if it's not complete, post it as a RFC. Cheers, Joel
Thanks for the feedback. I'm still working on getting a few kinks worked out in the driver, for instance, verifying that waiting on the rising edge for the J1 gpio (on the ast2400) is required for stable writing -- which has been true. We were actually seeing the serial gpios wedge to a specific value on the quanta-q71l board, but having it wait on that prevents this case... so more work is going into verifying behaviors and caveats. I'm without a motherboard that has serial gpios and the ast2500 for testing... Thanks, Patrick On Thu, Dec 7, 2017 at 10:30 PM, Joel Stanley <joel@jms.id.au> wrote: > On Fri, Dec 1, 2017 at 2:05 AM, Patrick Venture <venture@google.com> wrote: >> On Wed, Nov 29, 2017 at 7:53 PM, Joel Stanley <joel@jms.id.au> wrote: >>> On Thu, Nov 30, 2017 at 5:03 AM, Patrick Venture <venture@google.com> wrote: >>>> Can I get a status on this patch? The device datasheet has 4K >>>> reserved for the range, but only the first 512 are registers >>>> controlled by this driver. The rest will be handled by a separate >>>> driver. Or at least that's how it's planned. >>> >>> +cc the people who have worked on this. >>> >>> I think that's okay, but lets see what the others have to say. >>> >>> Do you have a driver for the rest of the GPIOs you can submit? I'd >>> like to see that code so we can decide if there is good reason to >>> make that completely separate, and not part of the existing driver. >>> >>> This work is best done upstream, as you will need to make the same >>> arguments there. >> >> I should have a driver ready sometime late next week that I'll be >> sending upstream first. I still need to finish up a few loose ends on >> it. It wouldn't be unreasonable to have them in the same driver, >> you'd just end up with two groups of gpios that are handled >> differently, and extra soft-irqs. That said, if you have two large >> groups of things that need to be treated differently, then that's >> somewhat of an argument for having them separate. :) >> >> I'd actually appreciate hearing some thoughts at this point on which >> way to go, since merging the serial handling into the main driver will >> require re-working a few things -- I'd rather take the extra time to >> handle that merge if it's going to head in that direction anyway. >> >> I have read through the gpio-aspeed driver before, as it was a bit >> useful as a guide, and I could see it getting a bit cluttered with >> also handling the serial gpios. > > It sounds like a call that could go either way. If you think it would > be easier to maintain two separate drivers then we can go that > direction. > > I'd suggest discussing it upstream. > > The best way to discuss it would be in the form of a patch. Even if > it's not complete, post it as a RFC. > > Cheers, > > Joel
On Sat, Dec 9, 2017 at 3:40 AM, Patrick Venture <venture@google.com> wrote: > > Thanks, > Patrick > > On Thu, Dec 7, 2017 at 10:30 PM, Joel Stanley <joel@jms.id.au> wrote: >> On Fri, Dec 1, 2017 at 2:05 AM, Patrick Venture <venture@google.com> wrote: >>> On Wed, Nov 29, 2017 at 7:53 PM, Joel Stanley <joel@jms.id.au> wrote: >>> I'd actually appreciate hearing some thoughts at this point on which >>> way to go, since merging the serial handling into the main driver will >>> require re-working a few things -- I'd rather take the extra time to >>> handle that merge if it's going to head in that direction anyway. >>> >>> I have read through the gpio-aspeed driver before, as it was a bit >>> useful as a guide, and I could see it getting a bit cluttered with >>> also handling the serial gpios. >> >> It sounds like a call that could go either way. If you think it would >> be easier to maintain two separate drivers then we can go that >> direction. >> >> I'd suggest discussing it upstream. >> >> The best way to discuss it would be in the form of a patch. Even if >> it's not complete, post it as a RFC. > > Thanks for the feedback. I'm still working on getting a few kinks > worked out in the driver, for instance, verifying that waiting on the > rising edge for the J1 gpio (on the ast2400) is required for stable > writing -- which has been true. We were actually seeing the serial > gpios wedge to a specific value on the quanta-q71l board, but having > it wait on that prevents this case... so more work is going into > verifying behaviors and caveats. I'm without a motherboard that has > serial gpios and the ast2500 for testing... This is a good example of where sending a RFC is the right thing to do - it doesn't matter that the code isn't functioning, but it will allow review of the structure. I encourage you to send it out as you have it now. Cheers, Joel
diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi index 6505062ac6c7..424d843ec1da 100644 --- a/arch/arm/boot/dts/aspeed-g4.dtsi +++ b/arch/arm/boot/dts/aspeed-g4.dtsi @@ -231,7 +231,7 @@ #gpio-cells = <2>; gpio-controller; compatible = "aspeed,ast2400-gpio"; - reg = <0x1e780000 0x1000>; + reg = <0x1e780000 0x200>; interrupts = <20>; interrupt-controller; gpio-ranges = <&pinctrl 0 0 220>;
Instead of 4k, it should be 512 because the latter part of the memory region are registers for the serial gpio driver. Signed-off-by: Patrick Venture <venture@google.com> --- arch/arm/boot/dts/aspeed-g4.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)