From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Rivshin Subject: Re: [PATCH 2/2] gpio: omap: compute debounce-time from actual debounce-clock rate Date: Thu, 20 Apr 2017 09:53:41 -0400 Message-ID: <20170420095341.6c919e40.drivshin@awxrd.com> References: <20170317005704.11971-1-drivshin@awxrd.com> <20170317005704.11971-3-drivshin@awxrd.com> <20170317191419.021bdd84.drivshin@awxrd.com> <5d0fab0d-bc9d-5148-063c-8bfe9e26f9a5@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5d0fab0d-bc9d-5148-063c-8bfe9e26f9a5@ti.com> Sender: linux-kernel-owner@vger.kernel.org To: Grygorii Strashko , Tero Kristo Cc: linux-gpio@vger.kernel.org, linux-omap@vger.kernel.org, Santosh Shilimkar , Kevin Hilman , Linus Walleij , Alexandre Courbot , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org List-Id: linux-gpio@vger.kernel.org On Fri, 17 Mar 2017 19:06:47 -0500 Grygorii Strashko wrote: > CC: Tero Gentle ping. Any thoughts on this? Is there anything I should investigate, or other information you'd like me to provide? > On 03/17/2017 06:14 PM, David Rivshin wrote: > > On Fri, 17 Mar 2017 14:43:02 -0500 > > Grygorii Strashko wrote: > > > >> On 03/16/2017 07:57 PM, David Rivshin wrote: > >>> From: David Rivshin > >>> > >>> omap2_set_gpio_debounce() assumes the debounce clock runs at 32768Hz, > >>> leading to 31us granularity. In reality the debounce clock (which > >>> is provided by other modules) could be at different rate, leading to > >>> an incorrect computation of the number of debounce clock cycles for > >>> GPIO_DEBOUNCINGTIME[DEBOUNCETIME]. > >>> > >>> Also, even with a standard 32768Hz input clock, the actual granularity > >>> is ~30.5us. This leads to the actual debounce time being ~1.5% too > >>> short. > >>> > >>> Fix both issues by simply querying the dbck rate, rather than > >>> hardcoding. > >> > >> Pls, hold on with this - I'm trying to check it, as it doesn't follow TRMs. > > > > Yes, the TRMs are somewhat cryptic about the details, perhaps I can give > > some more background on how I came to this. I think the chapters for the > > GPIO block are written with a strong assumption that the input debounce > > clock is 32768Hz, and further round 1/32768Hz to the nearest whole > > microsecond (~30.5us->31us). > > > > However, at least on AM335x the debounce clock (i.e. the CLK_32KHZ) can > > be made to run at other rates via the Control Module. > > > > Here is an example from clk_summary: > > clk_24mhz 1 1 24000000 0 0 > > clkdiv32k_ck 1 1 65536 0 0 > > clkdiv32k_ick 2 6 65536 0 0 > > timer7_fck 1 1 65536 0 0 > > wdt1_fck 0 1 65536 0 0 > > gpio3_dbclk 0 2 65536 0 0 > > gpio2_dbclk 0 2 65536 0 0 > > gpio1_dbclk 0 2 65536 0 0 > > > > This is accomplished by setting clk32kdivratio_ctl[clkdivopp50_en] to 1 > > even while in OPP100, and adjusting the devicetree to match. Timer7 is > > known to truly have a 65536Hz fck by configuring it as a 50% duty cycle > > PWM and measuring with an oscilloscope. > > > > Here are some relevant sections from the AM335x TRM (spruh73o): > > 25.2.2 GPIO Clock and Reset Management > > Debounce Functional clock (GPIO[123]) comes from CLK_32KHZ. > > 8.1.6.8 Peripheral PLL Description > > CLK_32KHZ is CLK_24 divided by either 732.4219 or 366.2109. > > In OPP100 this results in either 32768Hz or 65536Hz. > > In OPP50 this results in either 16384Hz or 32768Hz. > > 9.3.1.8 clk32kdivratio_ctrl Register > > Names of the register and field differ from 8.1.6.8, but it's > > clear they are the same. > > > > I'm not sure if other devices can be similarly cajoled into running > > their equivalent 32KHZ clocks at a different rate, but it certainly > > works for AM335x. Checking OMAP4430 TRM as an example makes it look > > like OMAP44xx has a fixed 32768Hz clock with no adjustment possible > > (backed up by omap44xx-clocks.dtsi having it just be a fixed-clock). > > > > > >>> > >>> Fixes: e85ec6c3047b ("gpio: omap: fix omap2_set_gpio_debounce") > >>> Cc: # 4.3+ > >>> Signed-off-by: David Rivshin > >>> --- > >>> > >>> This logical bug existed before e85ec6c3047b, but if backporting > >>> further it's probably best to just cherry-pick/backport e85ec6c3047b > >>> first. > >>> > >>> drivers/gpio/gpio-omap.c | 8 +++++--- > >>> 1 file changed, 5 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > >>> index 33ec02d..865a831 100644 > >>> --- a/drivers/gpio/gpio-omap.c > >>> +++ b/drivers/gpio/gpio-omap.c > >>> @@ -205,8 +205,8 @@ static inline void omap_gpio_dbck_disable(struct gpio_bank *bank) > >>> * @offset: the gpio number on this @bank > >>> * @debounce: debounce time to use > >>> * > >>> - * OMAP's debounce time is in 31us steps > >>> - * = (GPIO_DEBOUNCINGTIME[7:0].DEBOUNCETIME + 1) x 31 > >>> + * OMAP's debounce time is in 1/DBCK steps > >>> + * = (GPIO_DEBOUNCINGTIME[7:0].DEBOUNCETIME + 1) / DBCK > >>> * so we need to convert and round up to the closest unit. > >>> * > >>> * Return: 0 on success, negative error otherwise. > >>> @@ -223,7 +223,9 @@ static int omap2_set_gpio_debounce(struct gpio_bank *bank, unsigned offset, > >>> return -ENOTSUPP; > >>> > >>> if (enable) { > >>> - debounce = DIV_ROUND_UP(debounce, 31) - 1; > >>> + u64 tmp = (u64)debounce * clk_get_rate(bank->dbck); > >>> + > >>> + debounce = DIV_ROUND_UP_ULL(tmp, 1000000) - 1; > >>> if ((debounce & OMAP4_GPIO_DEBOUNCINGTIME_MASK) != debounce) > >>> return -EINVAL; > >>> } > >>> > >> > > > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031683AbdDTNxq (ORCPT ); Thu, 20 Apr 2017 09:53:46 -0400 Received: from out1-smtp.messagingengine.com ([66.111.4.25]:34063 "EHLO out1-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1031579AbdDTNxn (ORCPT ); Thu, 20 Apr 2017 09:53:43 -0400 X-ME-Sender: X-Sasl-enc: bc2rEC9ANfLSwXSHlylT+IDFFmGGWtxln2AvLAdl3jkx 1492696422 Date: Thu, 20 Apr 2017 09:53:41 -0400 From: David Rivshin To: Grygorii Strashko , Tero Kristo Cc: , , Santosh Shilimkar , Kevin Hilman , Linus Walleij , Alexandre Courbot , , Subject: Re: [PATCH 2/2] gpio: omap: compute debounce-time from actual debounce-clock rate Message-ID: <20170420095341.6c919e40.drivshin@awxrd.com> In-Reply-To: <5d0fab0d-bc9d-5148-063c-8bfe9e26f9a5@ti.com> References: <20170317005704.11971-1-drivshin@awxrd.com> <20170317005704.11971-3-drivshin@awxrd.com> <20170317191419.021bdd84.drivshin@awxrd.com> <5d0fab0d-bc9d-5148-063c-8bfe9e26f9a5@ti.com> Organization: Allworx X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 17 Mar 2017 19:06:47 -0500 Grygorii Strashko wrote: > CC: Tero Gentle ping. Any thoughts on this? Is there anything I should investigate, or other information you'd like me to provide? > On 03/17/2017 06:14 PM, David Rivshin wrote: > > On Fri, 17 Mar 2017 14:43:02 -0500 > > Grygorii Strashko wrote: > > > >> On 03/16/2017 07:57 PM, David Rivshin wrote: > >>> From: David Rivshin > >>> > >>> omap2_set_gpio_debounce() assumes the debounce clock runs at 32768Hz, > >>> leading to 31us granularity. In reality the debounce clock (which > >>> is provided by other modules) could be at different rate, leading to > >>> an incorrect computation of the number of debounce clock cycles for > >>> GPIO_DEBOUNCINGTIME[DEBOUNCETIME]. > >>> > >>> Also, even with a standard 32768Hz input clock, the actual granularity > >>> is ~30.5us. This leads to the actual debounce time being ~1.5% too > >>> short. > >>> > >>> Fix both issues by simply querying the dbck rate, rather than > >>> hardcoding. > >> > >> Pls, hold on with this - I'm trying to check it, as it doesn't follow TRMs. > > > > Yes, the TRMs are somewhat cryptic about the details, perhaps I can give > > some more background on how I came to this. I think the chapters for the > > GPIO block are written with a strong assumption that the input debounce > > clock is 32768Hz, and further round 1/32768Hz to the nearest whole > > microsecond (~30.5us->31us). > > > > However, at least on AM335x the debounce clock (i.e. the CLK_32KHZ) can > > be made to run at other rates via the Control Module. > > > > Here is an example from clk_summary: > > clk_24mhz 1 1 24000000 0 0 > > clkdiv32k_ck 1 1 65536 0 0 > > clkdiv32k_ick 2 6 65536 0 0 > > timer7_fck 1 1 65536 0 0 > > wdt1_fck 0 1 65536 0 0 > > gpio3_dbclk 0 2 65536 0 0 > > gpio2_dbclk 0 2 65536 0 0 > > gpio1_dbclk 0 2 65536 0 0 > > > > This is accomplished by setting clk32kdivratio_ctl[clkdivopp50_en] to 1 > > even while in OPP100, and adjusting the devicetree to match. Timer7 is > > known to truly have a 65536Hz fck by configuring it as a 50% duty cycle > > PWM and measuring with an oscilloscope. > > > > Here are some relevant sections from the AM335x TRM (spruh73o): > > 25.2.2 GPIO Clock and Reset Management > > Debounce Functional clock (GPIO[123]) comes from CLK_32KHZ. > > 8.1.6.8 Peripheral PLL Description > > CLK_32KHZ is CLK_24 divided by either 732.4219 or 366.2109. > > In OPP100 this results in either 32768Hz or 65536Hz. > > In OPP50 this results in either 16384Hz or 32768Hz. > > 9.3.1.8 clk32kdivratio_ctrl Register > > Names of the register and field differ from 8.1.6.8, but it's > > clear they are the same. > > > > I'm not sure if other devices can be similarly cajoled into running > > their equivalent 32KHZ clocks at a different rate, but it certainly > > works for AM335x. Checking OMAP4430 TRM as an example makes it look > > like OMAP44xx has a fixed 32768Hz clock with no adjustment possible > > (backed up by omap44xx-clocks.dtsi having it just be a fixed-clock). > > > > > >>> > >>> Fixes: e85ec6c3047b ("gpio: omap: fix omap2_set_gpio_debounce") > >>> Cc: # 4.3+ > >>> Signed-off-by: David Rivshin > >>> --- > >>> > >>> This logical bug existed before e85ec6c3047b, but if backporting > >>> further it's probably best to just cherry-pick/backport e85ec6c3047b > >>> first. > >>> > >>> drivers/gpio/gpio-omap.c | 8 +++++--- > >>> 1 file changed, 5 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > >>> index 33ec02d..865a831 100644 > >>> --- a/drivers/gpio/gpio-omap.c > >>> +++ b/drivers/gpio/gpio-omap.c > >>> @@ -205,8 +205,8 @@ static inline void omap_gpio_dbck_disable(struct gpio_bank *bank) > >>> * @offset: the gpio number on this @bank > >>> * @debounce: debounce time to use > >>> * > >>> - * OMAP's debounce time is in 31us steps > >>> - * = (GPIO_DEBOUNCINGTIME[7:0].DEBOUNCETIME + 1) x 31 > >>> + * OMAP's debounce time is in 1/DBCK steps > >>> + * = (GPIO_DEBOUNCINGTIME[7:0].DEBOUNCETIME + 1) / DBCK > >>> * so we need to convert and round up to the closest unit. > >>> * > >>> * Return: 0 on success, negative error otherwise. > >>> @@ -223,7 +223,9 @@ static int omap2_set_gpio_debounce(struct gpio_bank *bank, unsigned offset, > >>> return -ENOTSUPP; > >>> > >>> if (enable) { > >>> - debounce = DIV_ROUND_UP(debounce, 31) - 1; > >>> + u64 tmp = (u64)debounce * clk_get_rate(bank->dbck); > >>> + > >>> + debounce = DIV_ROUND_UP_ULL(tmp, 1000000) - 1; > >>> if ((debounce & OMAP4_GPIO_DEBOUNCINGTIME_MASK) != debounce) > >>> return -EINVAL; > >>> } > >>> > >> > > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: drivshin@awxrd.com (David Rivshin) Date: Thu, 20 Apr 2017 09:53:41 -0400 Subject: [PATCH 2/2] gpio: omap: compute debounce-time from actual debounce-clock rate In-Reply-To: <5d0fab0d-bc9d-5148-063c-8bfe9e26f9a5@ti.com> References: <20170317005704.11971-1-drivshin@awxrd.com> <20170317005704.11971-3-drivshin@awxrd.com> <20170317191419.021bdd84.drivshin@awxrd.com> <5d0fab0d-bc9d-5148-063c-8bfe9e26f9a5@ti.com> Message-ID: <20170420095341.6c919e40.drivshin@awxrd.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Fri, 17 Mar 2017 19:06:47 -0500 Grygorii Strashko wrote: > CC: Tero Gentle ping. Any thoughts on this? Is there anything I should investigate, or other information you'd like me to provide? > On 03/17/2017 06:14 PM, David Rivshin wrote: > > On Fri, 17 Mar 2017 14:43:02 -0500 > > Grygorii Strashko wrote: > > > >> On 03/16/2017 07:57 PM, David Rivshin wrote: > >>> From: David Rivshin > >>> > >>> omap2_set_gpio_debounce() assumes the debounce clock runs at 32768Hz, > >>> leading to 31us granularity. In reality the debounce clock (which > >>> is provided by other modules) could be at different rate, leading to > >>> an incorrect computation of the number of debounce clock cycles for > >>> GPIO_DEBOUNCINGTIME[DEBOUNCETIME]. > >>> > >>> Also, even with a standard 32768Hz input clock, the actual granularity > >>> is ~30.5us. This leads to the actual debounce time being ~1.5% too > >>> short. > >>> > >>> Fix both issues by simply querying the dbck rate, rather than > >>> hardcoding. > >> > >> Pls, hold on with this - I'm trying to check it, as it doesn't follow TRMs. > > > > Yes, the TRMs are somewhat cryptic about the details, perhaps I can give > > some more background on how I came to this. I think the chapters for the > > GPIO block are written with a strong assumption that the input debounce > > clock is 32768Hz, and further round 1/32768Hz to the nearest whole > > microsecond (~30.5us->31us). > > > > However, at least on AM335x the debounce clock (i.e. the CLK_32KHZ) can > > be made to run at other rates via the Control Module. > > > > Here is an example from clk_summary: > > clk_24mhz 1 1 24000000 0 0 > > clkdiv32k_ck 1 1 65536 0 0 > > clkdiv32k_ick 2 6 65536 0 0 > > timer7_fck 1 1 65536 0 0 > > wdt1_fck 0 1 65536 0 0 > > gpio3_dbclk 0 2 65536 0 0 > > gpio2_dbclk 0 2 65536 0 0 > > gpio1_dbclk 0 2 65536 0 0 > > > > This is accomplished by setting clk32kdivratio_ctl[clkdivopp50_en] to 1 > > even while in OPP100, and adjusting the devicetree to match. Timer7 is > > known to truly have a 65536Hz fck by configuring it as a 50% duty cycle > > PWM and measuring with an oscilloscope. > > > > Here are some relevant sections from the AM335x TRM (spruh73o): > > 25.2.2 GPIO Clock and Reset Management > > Debounce Functional clock (GPIO[123]) comes from CLK_32KHZ. > > 8.1.6.8 Peripheral PLL Description > > CLK_32KHZ is CLK_24 divided by either 732.4219 or 366.2109. > > In OPP100 this results in either 32768Hz or 65536Hz. > > In OPP50 this results in either 16384Hz or 32768Hz. > > 9.3.1.8 clk32kdivratio_ctrl Register > > Names of the register and field differ from 8.1.6.8, but it's > > clear they are the same. > > > > I'm not sure if other devices can be similarly cajoled into running > > their equivalent 32KHZ clocks at a different rate, but it certainly > > works for AM335x. Checking OMAP4430 TRM as an example makes it look > > like OMAP44xx has a fixed 32768Hz clock with no adjustment possible > > (backed up by omap44xx-clocks.dtsi having it just be a fixed-clock). > > > > > >>> > >>> Fixes: e85ec6c3047b ("gpio: omap: fix omap2_set_gpio_debounce") > >>> Cc: # 4.3+ > >>> Signed-off-by: David Rivshin > >>> --- > >>> > >>> This logical bug existed before e85ec6c3047b, but if backporting > >>> further it's probably best to just cherry-pick/backport e85ec6c3047b > >>> first. > >>> > >>> drivers/gpio/gpio-omap.c | 8 +++++--- > >>> 1 file changed, 5 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > >>> index 33ec02d..865a831 100644 > >>> --- a/drivers/gpio/gpio-omap.c > >>> +++ b/drivers/gpio/gpio-omap.c > >>> @@ -205,8 +205,8 @@ static inline void omap_gpio_dbck_disable(struct gpio_bank *bank) > >>> * @offset: the gpio number on this @bank > >>> * @debounce: debounce time to use > >>> * > >>> - * OMAP's debounce time is in 31us steps > >>> - * = (GPIO_DEBOUNCINGTIME[7:0].DEBOUNCETIME + 1) x 31 > >>> + * OMAP's debounce time is in 1/DBCK steps > >>> + * = (GPIO_DEBOUNCINGTIME[7:0].DEBOUNCETIME + 1) / DBCK > >>> * so we need to convert and round up to the closest unit. > >>> * > >>> * Return: 0 on success, negative error otherwise. > >>> @@ -223,7 +223,9 @@ static int omap2_set_gpio_debounce(struct gpio_bank *bank, unsigned offset, > >>> return -ENOTSUPP; > >>> > >>> if (enable) { > >>> - debounce = DIV_ROUND_UP(debounce, 31) - 1; > >>> + u64 tmp = (u64)debounce * clk_get_rate(bank->dbck); > >>> + > >>> + debounce = DIV_ROUND_UP_ULL(tmp, 1000000) - 1; > >>> if ((debounce & OMAP4_GPIO_DEBOUNCINGTIME_MASK) != debounce) > >>> return -EINVAL; > >>> } > >>> > >> > > >