| Message ID | 1357911185-11048-3-git-send-email-ldewangan@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/11/2013 06:33 AM, Laxman Dewangan wrote: > Enable tegra based keyboard controller and populate the key matrix for > seaboard. The key matrix was originally on driver code which is removed > to have clean driver. The key mapping is now passed through dts file. > > Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> > --- > Requesting for testing on seaboard. > I generated this patch as Stephen suggested to have one patch for dt file > entry for keys on seaboard. To some extent this works; at least this patch series is probably almost OK, but there are a bunch of issues in the KBC driver obviously: This series won't work on its own with the current Tegra for-next content, because: a) The clock driver doesn't provide any KBC clock for Tegra20, either in the old Tegra clock driver, or the first conversion to the common clock framework, nor the most recent complete conversion to the common clock framework. Was the driver tested at all on the upstream kernel? I suppose it's just possible it was tested on Tegra30, although see below... This brings up the point that the clk_get() call in the KBC driver is probably wrong. At least with Prashant's latest common clock framework patches applied, clk_get() returns NULL, not an error pointer for a non-existent clock. As such, the following driver code succeeds: > kbc->clk = clk_get(&pdev->dev, NULL); > if (IS_ERR(kbc->clk)) { > dev_err(&pdev->dev, "failed to get keyboard clock\n"); > err = PTR_ERR(kbc->clk); > goto err_iounmap; > } Should that check be if (!kbc-clk) instead? Or does the common clock framework require if (IS_ERR_OR_NULL(kbc->clk)); hopefully not since IS_ERR_OR_NULL shouldn't be used any more. This then causes the later call to tegra_periph_reset_{de,}assert() to crash since it generates a bogus pointer from the NULL pointer and dereferences it. I assume similar things happen before Prashant's latest clock patches. Oh, and the KBC clock is marked TEGRA_PERIPH_NO_RESET for Tegra30, and also for Tegra20 in the ChromeOS kernel; why is the driver trying to assert reset on a clock that doesn't support it? I hacked around this by adding the following to clk-tegra20.c on top of Prashant's latest patches: > /* kbc */ > clk = tegra_clk_periph_gate("kbc", "clk_32k", TEGRA_PERIPH_NO_RESET | > TEGRA_PERIPH_ON_APB, clk_base, 0, > 36, &periph_h_regs, periph_clk_enb_refcnt); > clk_register_clkdev(clk, NULL, "tegra-kbc"); > clks[kbc] = clk; > b) There is no AUXDATA in the board file, nor clocks properties in the .dts files, in this series to actually hook the KBC driver up to be able to get the correct clock. This leads me to suspect the driver was never tested upstream on either Tegra20 /or/ Tegra30. I hacked around this by applying the following: > diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi > index 8e97825..9ca5e94 100644 > --- a/arch/arm/boot/dts/tegra20.dtsi > +++ b/arch/arm/boot/dts/tegra20.dtsi > @@ -400,6 +400,7 @@ > reg = <0x7000e200 0x100>; > interrupts = <0 85 0x04>; > status = "disabled"; > + clocks = <&tegra_car 36>; > }; > > pmc { > diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi > index 9dca3e4..e580045 100644 > --- a/arch/arm/boot/dts/tegra30.dtsi > +++ b/arch/arm/boot/dts/tegra30.dtsi > @@ -417,6 +417,7 @@ > reg = <0x7000e200 0x100>; > interrupts = <0 85 0x04>; > status = "disabled"; > + clocks = <&tegra_car 36>; > }; > > pmc { I guess I can fold that fix into this series when I apply it, after Prashant's clock patches are merged. Once I hacked around the above issues, the driver doesn't work very well at all. There is a *long* delay between when I press a key and when it shows up (e.g. echo'd to the HDMI console). When I release the key the same keypress is generated twice. Or perhaps it's a repeat, but since it's *always* 2 extra keypresses and I doubt I always hold my finger on the key the exact same amount of time, I think it's key release that does this not repeat. I assume this would repro on any board, so you won't need Seaboard to debug it. Harmony is able to read the keyboard using the KBC. Even with the above, I was able to validate that the keymap in the Seaboard .dts file looks reasonable. However, please fix the KBC driver...
On Fri, Jan 11, 2013 at 04:09:59PM -0700, Stephen Warren wrote: > On 01/11/2013 06:33 AM, Laxman Dewangan wrote: > > kbc->clk = clk_get(&pdev->dev, NULL); > > if (IS_ERR(kbc->clk)) { > > dev_err(&pdev->dev, "failed to get keyboard clock\n"); > > err = PTR_ERR(kbc->clk); > > goto err_iounmap; > > } > > Should that check be if (!kbc-clk) instead? Or does the common clock > framework require if (IS_ERR_OR_NULL(kbc->clk)); hopefully not since > IS_ERR_OR_NULL shouldn't be used any more. /** * clk_get - lookup and obtain a reference to a clock producer. * @dev: device for clock "consumer" * @id: clock consumer ID * * Returns a struct clk corresponding to the clock producer, or ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ * valid IS_ERR() condition containing errno. ... ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Or, put another way: If (!IS_ERR(clk)) The_Clock_Is_Valid(); Else The_Clock_Is_Invalid(); The_Error = PTR_ERR(clk);
On 01/11/2013 04:24 PM, Russell King - ARM Linux wrote: > On Fri, Jan 11, 2013 at 04:09:59PM -0700, Stephen Warren wrote: >> On 01/11/2013 06:33 AM, Laxman Dewangan wrote: >>> kbc->clk = clk_get(&pdev->dev, NULL); >>> if (IS_ERR(kbc->clk)) { >>> dev_err(&pdev->dev, "failed to get keyboard clock\n"); >>> err = PTR_ERR(kbc->clk); >>> goto err_iounmap; >>> } >> >> Should that check be if (!kbc-clk) instead? Or does the common clock >> framework require if (IS_ERR_OR_NULL(kbc->clk)); hopefully not since >> IS_ERR_OR_NULL shouldn't be used any more. > > /** > * clk_get - lookup and obtain a reference to a clock producer. > * @dev: device for clock "consumer" > * @id: clock consumer ID > * > * Returns a struct clk corresponding to the clock producer, or > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > * valid IS_ERR() condition containing errno. ... > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Or, put another way: > > If (!IS_ERR(clk)) > The_Clock_Is_Valid(); > Else > The_Clock_Is_Invalid(); > The_Error = PTR_ERR(clk); OK, but that doesn't appear to be what happened in practice.
On Fri, Jan 11, 2013 at 04:27:07PM -0700, Stephen Warren wrote: > On 01/11/2013 04:24 PM, Russell King - ARM Linux wrote: > > On Fri, Jan 11, 2013 at 04:09:59PM -0700, Stephen Warren wrote: > >> On 01/11/2013 06:33 AM, Laxman Dewangan wrote: > >>> kbc->clk = clk_get(&pdev->dev, NULL); > >>> if (IS_ERR(kbc->clk)) { > >>> dev_err(&pdev->dev, "failed to get keyboard clock\n"); > >>> err = PTR_ERR(kbc->clk); > >>> goto err_iounmap; > >>> } > >> > >> Should that check be if (!kbc-clk) instead? Or does the common clock > >> framework require if (IS_ERR_OR_NULL(kbc->clk)); hopefully not since > >> IS_ERR_OR_NULL shouldn't be used any more. > > > > /** > > * clk_get - lookup and obtain a reference to a clock producer. > > * @dev: device for clock "consumer" > > * @id: clock consumer ID > > * > > * Returns a struct clk corresponding to the clock producer, or > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > * valid IS_ERR() condition containing errno. ... > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > Or, put another way: > > > > If (!IS_ERR(clk)) > > The_Clock_Is_Valid(); > > Else > > The_Clock_Is_Invalid(); > > The_Error = PTR_ERR(clk); > > OK, but that doesn't appear to be what happened in practice. It's what I've been saying each time I see an abuse, the problem is people don't care to read the documentation provided let alone understand the interfaces. That's precisely why IS_ERR_OR_NULL() is to be removed. One less thing for someone to throw a dart at as a selection method to use. Or maybe roll a dice. Or whatever way they do seem to choose. (Whatever that is, it's not based on any sound engineering practice I can make out.)
On 01/11/2013 04:36 PM, Russell King - ARM Linux wrote: > On Fri, Jan 11, 2013 at 04:27:07PM -0700, Stephen Warren wrote: >> On 01/11/2013 04:24 PM, Russell King - ARM Linux wrote: >>> On Fri, Jan 11, 2013 at 04:09:59PM -0700, Stephen Warren wrote: >>>> On 01/11/2013 06:33 AM, Laxman Dewangan wrote: >>>>> kbc->clk = clk_get(&pdev->dev, NULL); >>>>> if (IS_ERR(kbc->clk)) { >>>>> dev_err(&pdev->dev, "failed to get keyboard clock\n"); >>>>> err = PTR_ERR(kbc->clk); >>>>> goto err_iounmap; >>>>> } >>>> >>>> Should that check be if (!kbc-clk) instead? Or does the common clock >>>> framework require if (IS_ERR_OR_NULL(kbc->clk)); hopefully not since >>>> IS_ERR_OR_NULL shouldn't be used any more. >>> >>> /** >>> * clk_get - lookup and obtain a reference to a clock producer. >>> * @dev: device for clock "consumer" >>> * @id: clock consumer ID >>> * >>> * Returns a struct clk corresponding to the clock producer, or >>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>> * valid IS_ERR() condition containing errno. ... >>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>> >>> Or, put another way: >>> >>> If (!IS_ERR(clk)) >>> The_Clock_Is_Valid(); >>> Else >>> The_Clock_Is_Invalid(); >>> The_Error = PTR_ERR(clk); >> >> OK, but that doesn't appear to be what happened in practice. > > It's what I've been saying each time I see an abuse, the problem is > people don't care to read the documentation provided let alone > understand the interfaces. > > That's precisely why IS_ERR_OR_NULL() is to be removed. One less > thing for someone to throw a dart at as a selection method to use. > Or maybe roll a dice. Or whatever way they do seem to choose. > (Whatever that is, it's not based on any sound engineering practice > I can make out.) For the record, I did mention that IS_ERR_OR_NULL() should not be the solution here. And the point of my email to Laxman was that he should go figure out the answer to my question, which would entail reading the documentation/code/...
On Saturday 12 January 2013 04:39 AM, Stephen Warren wrote: > On 01/11/2013 06:33 AM, Laxman Dewangan wrote: >> Enable tegra based keyboard controller and populate the key matrix for >> seaboard. The key matrix was originally on driver code which is removed >> to have clean driver. The key mapping is now passed through dts file. >> >> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> >> --- >> Requesting for testing on seaboard. >> I generated this patch as Stephen suggested to have one patch for dt file >> entry for keys on seaboard. Thanks for testing. > To some extent this works; at least this patch series is probably almost > OK, but there are a bunch of issues in the KBC driver obviously: > > This series won't work on its own with the current Tegra for-next > content, because: > > a) The clock driver doesn't provide any KBC clock for Tegra20, either in > the old Tegra clock driver, or the first conversion to the common clock > framework, nor the most recent complete conversion to the common clock > framework. Was the driver tested at all on the upstream kernel? I > suppose it's just possible it was tested on Tegra30, although see below... I have cardhu and Ventana and unfortunately both does not support the key pad matrix. Both are on gpio keys. I tested this patch series (along with recent kbc patches) on t114 platform Dalmore, it is 3x3 matrix. Yes I made a hack on the board-dt-* file for getting proper clock. I did not send similar change as this will not be very much used when we will we in CCF. > This brings up the point that the clk_get() call in the KBC driver is > probably wrong. At least with Prashant's latest common clock framework > patches applied, clk_get() returns NULL, not an error pointer for a > non-existent clock. As such, the following driver code succeeds: > >> kbc->clk = clk_get(&pdev->dev, NULL); >> if (IS_ERR(kbc->clk)) { >> dev_err(&pdev->dev, "failed to get keyboard clock\n"); >> err = PTR_ERR(kbc->clk); >> goto err_iounmap; >> } > Should that check be if (!kbc-clk) instead? Or does the common clock > framework require if (IS_ERR_OR_NULL(kbc->clk)); hopefully not since > IS_ERR_OR_NULL shouldn't be used any more. If clk_get() failed then it should be return ptr_err. if not then it need to change almost all driver i.e. i2c/spi. If clk_get(0 returns NULL then should be part of the clock CCF changes series to change here also. > This then causes the later call to tegra_periph_reset_{de,}assert() to > crash since it generates a bogus pointer from the NULL pointer and > dereferences it. I assume similar things happen before Prashant's latest > clock patches. No, Currently clk_get() returns the PTR_ERR and probe fail. This is well tested till now. Something changed with CCF. > > Oh, and the KBC clock is marked TEGRA_PERIPH_NO_RESET for Tegra30, and > also for Tegra20 in the ChromeOS kernel; why is the driver trying to > assert reset on a clock that doesn't support it? This is something our kbc controller reset and clock design. KBC controller is on Always Power ON domain so that it can work even when system is in sleep. The KBC clock is enabled through two places, PMC control register and CAR register set. KBC controller is only reset through PMC control register. This is not well implemented either on our downstream or in mainline. Sometime back I tried to implement it in downstream but was having lots of comment and not able to complete this. Possibly we will talk internally that how can we implement this. > > I hacked around this by adding the following to clk-tegra20.c on top of > Prashant's latest patches: > >> /* kbc */ >> clk = tegra_clk_periph_gate("kbc", "clk_32k", TEGRA_PERIPH_NO_RESET | >> TEGRA_PERIPH_ON_APB, clk_base, 0, >> 36, &periph_h_regs, periph_clk_enb_refcnt); >> clk_register_clkdev(clk, NULL, "tegra-kbc"); >> clks[kbc] = clk; >> > b) There is no AUXDATA in the board file, nor clocks properties in the > .dts files, in this series to actually hook the KBC driver up to be able > to get the correct clock. This leads me to suspect the driver was never > tested upstream on either Tegra20 /or/ Tegra30. I have my local change for the AUXDATA board file which I did not sent. I think CCF does not require and this is the reason. I can sent this if it needed. > I hacked around this by applying the following: Yes, hack is required here. Either hack in the board file for auxdata or in dts file for clock entry when you have ccf ready. > I guess I can fold that fix into this series when I apply it, after > Prashant's clock patches are merged. > > Once I hacked around the above issues, the driver doesn't work very well > at all. There is a *long* delay between when I press a key and when it > shows up (e.g. echo'd to the HDMI console). When I release the key the > same keypress is generated twice. Or perhaps it's a repeat, but since > it's *always* 2 extra keypresses and I doubt I always hold my finger on > the key the exact same amount of time, I think it's key release that > does this not repeat. I assume this would repro on any board, so you > won't need Seaboard to debug it. Harmony is able to read the keyboard > using the KBC. > > Even with the above, I was able to validate that the keymap in the > Seaboard .dts file looks reasonable. > > However, please fix the KBC driver... I did not see any issue with the 3x3 matrix. I think all these about is to tuning debaunce and other parameter also. Another thing is that to make sure that all pinmuxes are properly configured. However, again I will try to enable kbc on harmony and run it.
On Saturday 12 January 2013 05:09 AM, Stephen Warren wrote: > On 01/11/2013 04:36 PM, Russell King - ARM Linux wrote: >> On Fri, Jan 11, 2013 at 04:27:07PM -0700, Stephen Warren wrote: >>> On 01/11/2013 04:24 PM, Russell King - ARM Linux wrote: >>>> On Fri, Jan 11, 2013 at 04:09:59PM -0700, Stephen Warren wrote: >>>>> On 01/11/2013 06:33 AM, Laxman Dewangan wrote: >>>>>> kbc->clk = clk_get(&pdev->dev, NULL); >>>>>> if (IS_ERR(kbc->clk)) { >>>>>> dev_err(&pdev->dev, "failed to get keyboard clock\n"); >>>>>> err = PTR_ERR(kbc->clk); >>>>>> goto err_iounmap; >>>>>> } >>>>> Should that check be if (!kbc-clk) instead? Or does the common clock >>>>> framework require if (IS_ERR_OR_NULL(kbc->clk)); hopefully not since >>>>> IS_ERR_OR_NULL shouldn't be used any more. >>>> /** >>>> * clk_get - lookup and obtain a reference to a clock producer. >>>> * @dev: device for clock "consumer" >>>> * @id: clock consumer ID >>>> * >>>> * Returns a struct clk corresponding to the clock producer, or >>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>> * valid IS_ERR() condition containing errno. ... >>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>> >>>> Or, put another way: >>>> >>>> If (!IS_ERR(clk)) >>>> The_Clock_Is_Valid(); >>>> Else >>>> The_Clock_Is_Invalid(); >>>> The_Error = PTR_ERR(clk); >>> OK, but that doesn't appear to be what happened in practice. >> It's what I've been saying each time I see an abuse, the problem is >> people don't care to read the documentation provided let alone >> understand the interfaces. >> >> That's precisely why IS_ERR_OR_NULL() is to be removed. One less >> thing for someone to throw a dart at as a selection method to use. >> Or maybe roll a dice. Or whatever way they do seem to choose. >> (Whatever that is, it's not based on any sound engineering practice >> I can make out.) > For the record, I did mention that IS_ERR_OR_NULL() should not be the > solution here. And the point of my email to Laxman was that he should go > figure out the answer to my question, which would entail reading the > documentation/code/... Going through clock driver, the clk_get() should return error pointer or valid pointer, atleast not NULL. Probably our clock driver is returning to NULL and hence issue is becasue whole world, it check for IS_ERR or !IS_ERR() and null clk pointer treated as !IS_ERR(). Stephen, Which is the tree on which Prashant changes are applied. I can debug from that tree.
On Sat, Jan 12, 2013 at 04:04:28PM +0530, Laxman Dewangan wrote: > Going through clock driver, the clk_get() should return error pointer or > valid pointer, atleast not NULL. No. clk_get() will return whatever value the 'clk' pointer found in the clk_lookup structure contains. Or it will return an error pointer. This topic has had enough discussion in the past that I'll say: go and look up in the archives emails from me on this subject. I've been very clear over the years about this. > Probably our clock driver is returning to NULL and hence issue is > becasue whole world, it check for IS_ERR or !IS_ERR() and null clk > pointer treated as !IS_ERR(). Probably not. As far as _drivers_ go, either the clk cookie is an error (IS_ERR() is true) _OR_ it is valid to be passed back into the clock API. You will find statements from me in the past to that: whatever clk_get() returns which is _not_ an error must be accepted by the rest of the clk API. Drivers never dereference the returned clk, so why should they care about anything other than "is it an error" given by IS_ERR(). (notice the lack of question mark, that's not a question to be answered.)
On Saturday 12 January 2013 04:17 PM, Russell King - ARM Linux wrote: > On Sat, Jan 12, 2013 at 04:04:28PM +0530, Laxman Dewangan wrote: >> Going through clock driver, the clk_get() should return error pointer or >> valid pointer, atleast not NULL. > No. clk_get() will return whatever value the 'clk' pointer found in > the clk_lookup structure contains. Or it will return an error pointer. > > This topic has had enough discussion in the past that I'll say: go and > look up in the archives emails from me on this subject. I've been very > clear over the years about this. > >> Probably our clock driver is returning to NULL and hence issue is >> becasue whole world, it check for IS_ERR or !IS_ERR() and null clk >> pointer treated as !IS_ERR(). > Probably not. As far as _drivers_ go, either the clk cookie is an error > (IS_ERR() is true) _OR_ it is valid to be passed back into the clock API. > You will find statements from me in the past to that: whatever clk_get() > returns which is _not_ an error must be accepted by the rest of the clk > API. Ok, Should we document that after clk_get(), we should check for IS_ERR() only for validation. The way you explain earlier. Documentation does not have this. /** struct clk *clk; clk = clk_get(NULL, "my_gateable_clk"); clk_prepare(clk); clk_enable(clk); **/ The clk.h explain this: * Returns a struct clk corresponding to the clock producer, or * valid IS_ERR() condition containing errno. The IS_ERR() return false for the null pointer. So as per you, it should be valid if the IS_ERR() false for returned pointer and hence NULL pointer should be valid for the clock driver. Now we need to change the tegra_periph_reset_*() to accept the null pointer. > Drivers never dereference the returned clk, so why should they care about > anything other than "is it an error" given by IS_ERR(). (notice the lack > of question mark, that's not a question to be answered.) The issue is that information is getting passed to other part fo driver where it is dereferencing the pointer. Probably we need to work on the reset part also to move as part of clock driver to avoid this crash.
On 01/12/2013 03:34 AM, Laxman Dewangan wrote: ... > Which is the tree on which Prashant changes are applied. I can debug > from that tree. git://nv-tegra.nvidia.com/user/swarren/linux-2.6 test-ccf-rework-v4 But as I said in some other email in response to Prashant, I'll go and find out why the clk driver is returning NULL. (Russell, while the clk API definition may allow NULL to be a legal clk value, the actual implementation on Tegra at least doesn't consider NULL to be a legal clk value, so it shouldn't be returned in the first place). There's no issue here in the comprehension of the clk API.
On Mon, Jan 14, 2013 at 09:45:52AM -0700, Stephen Warren wrote: > On 01/12/2013 03:34 AM, Laxman Dewangan wrote: > ... > > Which is the tree on which Prashant changes are applied. I can debug > > from that tree. > > git://nv-tegra.nvidia.com/user/swarren/linux-2.6 test-ccf-rework-v4 > > But as I said in some other email in response to Prashant, I'll go and > find out why the clk driver is returning NULL. Exactly - the rule is, the rest of the clk API should accept as valid anything which clk_get() produces _for that implementation_ which IS_ERR() is not equal to 1. What that means is: - if a platform's clk_get() never returns NULL, then the rest of the clk API need not test for that value. (If a NULL pointer is passed to one of the clk API functions, crashing _is_ _okay_. Consider what memset(NULL, 0, 4096) would do from the kernel - it doesn't test for a NULL pointer just because it can - it crashes so you can debug why it was passed a NULL pointer.) - if a platform's clk_get() does return NULL, then the rest of the clk API is expected to deal with a NULL pointer in a way that does _not_ result in the system crashing. Or, to put it another way: clk = clk_get(...); if (IS_ERR(clk)) return PTR_ERR(clk); clk_foo(clk); for all possible return values of clk_get() that an implementation _actually_ _intentionally_ returns[*] should never cause an oops. [*] - bugs excluded. No bugs were squashed in the creation of this email.
On 01/12/2013 03:02 AM, Laxman Dewangan wrote: > On Saturday 12 January 2013 04:39 AM, Stephen Warren wrote: >> On 01/11/2013 06:33 AM, Laxman Dewangan wrote: >>> Enable tegra based keyboard controller and populate the key matrix for >>> seaboard. The key matrix was originally on driver code which is removed >>> to have clean driver. The key mapping is now passed through dts file. >>> >>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> >>> --- >>> Requesting for testing on seaboard. >>> I generated this patch as Stephen suggested to have one patch for dt >>> file >>> entry for keys on seaboard. > > Thanks for testing. >> Oh, and the KBC clock is marked TEGRA_PERIPH_NO_RESET for Tegra30, and >> also for Tegra20 in the ChromeOS kernel; why is the driver trying to >> assert reset on a clock that doesn't support it? > > This is something our kbc controller reset and clock design. > KBC controller is on Always Power ON domain so that it can work even > when system is in sleep. > The KBC clock is enabled through two places, PMC control register and > CAR register set. > KBC controller is only reset through PMC control register. > This is not well implemented either on our downstream or in mainline. > Sometime back I tried to implement it in downstream but was having lots > of comment and not able to complete this. Possibly we will talk > internally that how can we implement this. I guess there's little point in the KBC driver calling the Tegra clock reset APIs then. Still, since the code is already there and it's not doing any harm, I guess it's fine to leave it there; if we ever enhance the clock driver to implement reset for those clocks via the PMC, it will all just magically work. >> Once I hacked around the above issues, the driver doesn't work very well >> at all. There is a *long* delay between when I press a key and when it >> shows up (e.g. echo'd to the HDMI console). When I release the key the >> same keypress is generated twice. Or perhaps it's a repeat, but since >> it's *always* 2 extra keypresses and I doubt I always hold my finger on >> the key the exact same amount of time, I think it's key release that >> does this not repeat. I assume this would repro on any board, so you >> won't need Seaboard to debug it. Harmony is able to read the keyboard >> using the KBC. >> >> Even with the above, I was able to validate that the keymap in the >> Seaboard .dts file looks reasonable. >> >> However, please fix the KBC driver... > > I did not see any issue with the 3x3 matrix. I think all these about is > to tuning debaunce and other parameter also. Another thing is that to > make sure that all pinmuxes are properly configured. OK, I'll go look at some downstream kernels for Seaboard and see if the debounce etc. parameters all match up. > However, again I will try to enable kbc on harmony and run it. That would be extremely useful; I worry that if you tested it on Tegra114, that means using a downstream kernel only, and also whether there are any HW differences between SoC versions. That's quite a lot of variables that could explain the issues I'm seeing.
diff --git a/arch/arm/boot/dts/tegra20-seaboard.dts b/arch/arm/boot/dts/tegra20-seaboard.dts index f9e3ad4..b22522d 100644 --- a/arch/arm/boot/dts/tegra20-seaboard.dts +++ b/arch/arm/boot/dts/tegra20-seaboard.dts @@ -612,6 +612,144 @@ }; }; + kbc { + status = "okay"; + nvidia,debounce-delay-ms = <640>; + nvidia,repeat-delay-ms = <1>; + nvidia,kbc-row-pins = <0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15>; + nvidia,kbc-col-pins = <16 17 18 19 20 21 22 23>; + linux,keymap = <0x00020011 /* KEY_W */ + 0x0003001F /* KEY_S */ + 0x0004001E /* KEY_A */ + 0x0005002C /* KEY_Z */ + 0x000701d0 /* KEY_FN */ + + 0x0107007D /* KEY_LEFTMETA */ + 0x02060064 /* KEY_RIGHTALT */ + 0x02070038 /* KEY_LEFTALT */ + + 0x03000006 /* KEY_5 */ + 0x03010005 /* KEY_4 */ + 0x03020013 /* KEY_R */ + 0x03030012 /* KEY_E */ + 0x03040021 /* KEY_F */ + 0x03050020 /* KEY_D */ + 0x0306002D /* KEY_X */ + + 0x04000008 /* KEY_7 */ + 0x04010007 /* KEY_6 */ + 0x04020014 /* KEY_T */ + 0x04030023 /* KEY_H */ + 0x04040022 /* KEY_G */ + 0x0405002F /* KEY_V */ + 0x0406002E /* KEY_C */ + 0x04070039 /* KEY_SPACE */ + + 0x0500000A /* KEY_9 */ + 0x05010009 /* KEY_8 */ + 0x05020016 /* KEY_U */ + 0x05030015 /* KEY_Y */ + 0x05040024 /* KEY_J */ + 0x05050031 /* KEY_N */ + 0x05060030 /* KEY_B */ + 0x0507002B /* KEY_BACKSLASH */ + + 0x0600000C /* KEY_MINUS */ + 0x0601000B /* KEY_0 */ + 0x06020018 /* KEY_O */ + 0x06030017 /* KEY_I */ + 0x06040026 /* KEY_L */ + 0x06050025 /* KEY_K */ + 0x06060033 /* KEY_COMMA */ + 0x06070032 /* KEY_M */ + + 0x0701000D /* KEY_EQUAL */ + 0x0702001B /* KEY_RIGHTBRACE */ + 0x0703001C /* KEY_ENTER */ + 0x0707008B /* KEY_MENU */ + + 0x08040036 /* KEY_RIGHTSHIFT */ + 0x0805002A /* KEY_LEFTSHIFT */ + + 0x09050061 /* KEY_RIGHTCTRL */ + 0x0907001B /* KEY_LEFTCTRL */ + + 0x0B00001A /* KEY_LEFTBRACE */ + 0x0B010019 /* KEY_P */ + 0x0B020028 /* KEY_APOSTROPHE */ + 0x0B030027 /* KEY_SEMICOLON */ + 0x0B040035 /* KEY_SLASH */ + 0x0B050034 /* KEY_DOT */ + + 0x0C000044 /* KEY_F10 */ + 0x0C010043 /* KEY_F9 */ + 0x0C02000E /* KEY_BACKSPACE */ + 0x0C030004 /* KEY_3 */ + 0x0C040003 /* KEY_2 */ + 0x0C050067 /* KEY_UP */ + 0x0C0600D2 /* KEY_PRINT */ + 0x0C070077 /* KEY_PAUSE */ + + 0x0D00006E /* KEY_INSERT */ + 0x0D01006F /* KEY_DELETE */ + 0x0D030068 /* KEY_PAGEUP */ + 0x0D04006D /* KEY_PAGEDOWN */ + 0x0D05006A /* KEY_RIGHT */ + 0x0D06006C /* KEY_DOWN */ + 0x0D070069 /* KEY_LEFT */ + + 0x0E000057 /* KEY_F11 */ + 0x0E010058 /* KEY_F12 */ + 0x0E020042 /* KEY_F8 */ + 0x0E030010 /* KEY_Q */ + 0x0E04003E /* KEY_F4 */ + 0x0E05003D /* KEY_F3 */ + 0x0E060002 /* KEY_1 */1 + 0x0E070041 /* KEY_F7 */ + + 0x0F000001 /* KEY_ESC */ + 0x0F010029 /* KEY_GRAVE */ + 0x0F02003F /* KEY_F5 */ + 0x0F03000F /* KEY_TAB */ + 0x0F04003B /* KEY_F1 */ + 0x0F05003C /* KEY_F2 */ + 0x0F06003A /* KEY_CAPSLOCK */ + 0x0F070040 /* KEY_F6 */ + + /* Software Handled Function Keys */ + 0x14000047 /* KEY_KP7 */ + + 0x15000049 /* KEY_KP9 */ + 0x15010048 /* KEY_KP8 */ + 0x1502004B /* KEY_KP4 */ + 0x1504004F /* KEY_KP1 */ + + 0x1601004E /* KEY_KPSLASH */ + 0x1602004D /* KEY_KP6 */ + 0x1603004C /* KEY_KP5 */ + 0x16040051 /* KEY_KP3 */ + 0x16050050 /* KEY_KP2 */ + 0x16070052 /* KEY_KP0 */ + + 0x1B010037 /* KEY_KPASTERISK */ + 0x1B03004A /* KEY_KPMINUS */ + 0x1B04004E /* KEY_KPPLUS */ + 0x1B050053 /* KEY_KPDOT */ + + 0x1C050073 /* KEY_VOLUMEUP */ + + 0x1D030066 /* KEY_HOME */ + 0x1D04006B /* KEY_END */ + 0x1D0500E0 /* KEY_BRIGHTNESSDOWN */ + 0x1D060072 /* KEY_VOLUMEDOWN */ + 0x1D0700E1 /* KEY_BRIGHTNESSUP */ + + 0x1E000045 /* KEY_NUMLOCK */ + 0x1E010046 /* KEY_SCROLLLOCK */ + 0x1E020071 /* KEY_MUTE */ + + 0x1F04008A>; /* KEY_HELP */ + }; regulators { compatible = "simple-bus"; #address-cells = <1>;
Enable tegra based keyboard controller and populate the key matrix for seaboard. The key matrix was originally on driver code which is removed to have clean driver. The key mapping is now passed through dts file. Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> --- Requesting for testing on seaboard. I generated this patch as Stephen suggested to have one patch for dt file entry for keys on seaboard. arch/arm/boot/dts/tegra20-seaboard.dts | 138 ++++++++++++++++++++++++++++++++ 1 files changed, 138 insertions(+), 0 deletions(-)