Re: 回信:Re: [PATCH linux] Add debounce for power button, pull down gpioQ7 to unlock identify LED
Joel Stanley
joel at jms.id.au
Tue May 17 17:47:41 AEST 2016
Hello Ken and Adriana,
It seems you both missed the email sent by Andrew on Friday:
https://lists.ozlabs.org/pipermail/openbmc/2016-May/003078.html
He also had some feedback for your patch.
I have some suggestions below that you can follow to help us proceed.
>> > > + writel(0x00001080, AST_IO(AST_BASE_GPIO | 0x84));
>> > We have a GPIO driver that handles setting this register.
>
> Agreed - superficially it looks like we should be doing this in e.g.
> bin/Barreleye.py in the skeleton repo.
>
> So looking there, R4 was previously described:
>
> GPIO_CONFIG['HEART_BEAT'] = { 'gpio_pin': 'R4', 'direction':
> 'out' }
>
> But was removed by Adriana in skeleton's "9c75104b Implement new LED
> driver" commit. Presumably this should've taken care of the
> configuration of R4.
>
Are you aware that the identify LED is controlled by the gpio-leds
driver in the kernel? Was this working?
.
You can see the configuration of this in the device tree
arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts:
leds {
compatible = "gpio-leds";
heartbeat {
# GPIO R4, BMC_HEARTBRAT_LED_N
gpios = <&gpio 140 GPIO_ACTIVE_HIGH>;
};
identify {
# GPIO H2, BMC_SYS_PWROK_IDLED_N
gpios = <&gpio 58 GPIO_ACTIVE_LOW>;
};
beep {
# GPIO N7, BMC_BEEP
gpios = <&gpio 111 GPIO_ACTIVE_HIGH>;
};
};
> The register write is also configuring GPIOQ7 as output, but I can't
> find mention of it in the skeleton history.
>> > > - writel(0x0031FFAF, AST_IO(AST_BASE_GPIO | 0x80));
>> > > + writel(0x0031FF2F, AST_IO(AST_BASE_GPIO | 0x80));
>
> So here we're bringing Q7 low. Can we deal with it as mentioned above?
As Andrew observed you are setting Q7 as an otuput and bringing it
low. It is BMC_READY_N, which makes sense.
However, this can be and should be done in skeleton/bin/Barreleye.py.
Norm, as a policy, should we do this when the BMC userspace is up and ready?
>> > > + writel(0x00000001, AST_IO(AST_BASE_GPIO | 0x48));
>> > > + writel(0x00000001, AST_IO(AST_BASE_GPIO | 0x4C));
>> > > + writel(0x00075300, AST_IO(AST_BASE_GPIO | 0x58));
>
> Right, so these registers configure debounce. I'm not sure why we
> chose timer 3, but regardless, my understanding is that debounce cannot
> be configured from userspace via sysfs.
Andrew points out that we need to do denounce configuration where you
have, for now.
Please re-submit this part of the patch with a commit message about
what GPIOs are being configured and why.
You could write something like:
"This configures GPIOE0, which is BMC_PWBTN_IN_N, to be debounced. It
uses timer 3 for 480000 cycles, which with a pclk of 24MHz is 20
milliseconds."
Cheers,
Joel
On Tue, May 17, 2016 at 12:35 PM, <ken.th.liu at foxconn.com> wrote:
>
> Hi Adri
>
> Response as below.
> Hi Ken,
>
> Could you reply to Joel if you have any questions about his comments or make a new pull request so that the change can be merged? Specifically is provide more info regarding the changes, I'm copying the comments here. Thanks.
>
>
> >> From: Ken <ken.sk.lai at mail.foxconn.com>
> > We need to include a description of why this change is being made.
> > What does it fix? Why does the previous behaviour need to be modified?
>
> It's fix when power on, the identify led can not be controlled.
>
> And power button issue.
>
>
>
> >> + writel(0x00001080, AST_IO(AST_BASE_GPIO | 0x84));
> > We have a GPIO driver that handles setting this register.
>
> OK. We can move the code to skeleton.
>
>
>
> The GPIO is for unlock Identify LED by GPIOQ7 when power on.
>
>
> >> + writel(0x010FFFFF, AST_IO(AST_BASE_SCU | 0xA8));
> > Can you explain each of these you're setting each of these bits?
>
> It's a long story, please check the attachment mail history.
>
>
>
> To conclude, to avoid the debounce pulse for power button interrupt.
>
>
>
>
>
> From: Joel Stanley <joel at jms.id.au>
> To: OpenBMC Patches <openbmc-patches at stwcx.xyz>, ken.th.liu at foxconn.com
> Cc: OpenBMC Maillist <openbmc at lists.ozlabs.org>
> Date: 05/12/2016 02:11 AM
> Subject: Re: [PATCH linux] Add debounce for power button, pull down gpioQ7 to unlock identify LED
> Sent by: "openbmc" <openbmc-bounces+anoo=us.ibm.com at lists.ozlabs.org>
> ________________________________
>
>
>
> ping
>
> On Mon, May 2, 2016 at 11:12 AM, Joel Stanley <joel at jms.id.au> wrote:
> > On Fri, Apr 29, 2016 at 3:10 PM, OpenBMC Patches
> > <openbmc-patches at stwcx.xyz> wrote:
> >> From: Ken <ken.sk.lai at mail.foxconn.com>
> >>
> >
> > We need to include a description of why this change is being made.
> > What does it fix? Why does the previous behaviour need to be modified?
> >
> >> ---
> >> arch/arm/mach-aspeed/aspeed.c | 8 ++++++--
> >> 1 file changed, 6 insertions(+), 2 deletions(-)
> >> mode change 100644 => 100755 arch/arm/mach-aspeed/aspeed.c
> >>
> >> diff --git a/arch/arm/mach-aspeed/aspeed.c b/arch/arm/mach-aspeed/aspeed.c
> >> old mode 100644
> >> new mode 100755
> >> index 594a781..fa5d467
> >> --- a/arch/arm/mach-aspeed/aspeed.c
> >> +++ b/arch/arm/mach-aspeed/aspeed.c
> >> @@ -138,9 +138,10 @@ static void __init do_barreleye_setup(void)
> >> /* GPIO setup */
> >> writel(0x9E82FCE7, AST_IO(AST_BASE_GPIO | 0x00));
> >> writel(0x0370E677, AST_IO(AST_BASE_GPIO | 0x04));
> >> -
> >> + writel(0x00001080, AST_IO(AST_BASE_GPIO | 0x84));
> >
> > We have a GPIO driver that handles setting this register.
> >
> >> /* SCU setup */
> >> writel(0x01C00000, AST_IO(AST_BASE_SCU | 0x88));
> >> + writel(0x010FFFFF, AST_IO(AST_BASE_SCU | 0xA8));
> >
> > Can you explain each of these you're setting each of these bits?
> >
> >> /*
> >> * Do read/modify/write on power gpio to prevent resetting power on
> >> @@ -150,7 +151,10 @@ static void __init do_barreleye_setup(void)
> >> reg |= 0xCFC8F7FD;
> >> writel(reg, AST_IO(AST_BASE_GPIO | 0x20));
> >> writel(0xC738F20A, AST_IO(AST_BASE_GPIO | 0x24));
> >> - writel(0x0031FFAF, AST_IO(AST_BASE_GPIO | 0x80));
> >> + writel(0x0031FF2F, AST_IO(AST_BASE_GPIO | 0x80));
> >> + writel(0x00000001, AST_IO(AST_BASE_GPIO | 0x48));
> >> + writel(0x00000001, AST_IO(AST_BASE_GPIO | 0x4C));
> >> + writel(0x00075300, AST_IO(AST_BASE_GPIO | 0x58));
> >
> > As above; we have a GPIO driver that handles setting this register.
> > Changing these states needs to be done from userspace. See
> > https://github.com/openbmc/skeleton/blob/master/bin/Barreleye.py#L531
More information about the openbmc
mailing list