* Re: [PATCH 2/2] clk: tegra: Always program PLL_E when enabled
From: LABBE Corentin @ 2020-06-03 13:13 UTC (permalink / raw)
To: Thierry Reding
Cc: Michael Turquette, Stephen Boyd, Jon Hunter, Dmitry Osipenko,
linux-clk, linux-tegra
In-Reply-To: <20200603111923.3545261-2-thierry.reding@gmail.com>
On Wed, Jun 03, 2020 at 01:19:23PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> Commit bff1cef5f23a ("clk: tegra: Don't enable already enabled PLLs")
> added checks to avoid enabling PLLs that have already been enabled by
> the bootloader. However, the PLL_E configuration inherited from the
> bootloader isn't necessarily the one that is needed for the kernel.
>
> This can cause SATA to fail like this:
>
> [ 5.310270] phy phy-sata.6: phy poweron failed --> -110
> [ 5.315604] tegra-ahci 70027000.sata: failed to power on AHCI controller: -110
> [ 5.323022] tegra-ahci: probe of 70027000.sata failed with error -110
>
> Fix this by always programming the PLL_E. This ensures that any mis-
> configuration by the bootloader will be overwritten by the kernel.
>
> Fixes: bff1cef5f23a ("clk: tegra: Don't enable already enabled PLLs")
> Reported-by: LABBE Corentin <clabbe@baylibre.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> drivers/clk/tegra/clk-pll.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> index 583d2ac61e9e..b2d39a66f0fa 100644
> --- a/drivers/clk/tegra/clk-pll.c
> +++ b/drivers/clk/tegra/clk-pll.c
> @@ -1601,9 +1601,6 @@ static int clk_plle_tegra114_enable(struct clk_hw *hw)
> unsigned long flags = 0;
> unsigned long input_rate;
>
> - if (clk_pll_is_enabled(hw))
> - return 0;
> -
> input_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
>
> if (_get_table_rate(hw, &sel, pll->params->fixed_rate, input_rate))
> --
> 2.24.1
>
Tested-by: Corentin Labbe <clabbe@baylibre.com>
Thanks
^ permalink raw reply
* HELLO: I AM MRS SUSAN JONES
From: Mrs.Susan Jones @ 2020-06-03 13:48 UTC (permalink / raw)
--
OUR GOLDEN OPPORTUNITY
Hello Dear Friend,
Complement of the day, i hope you are doing great today. However, I am
Mrs.Susan Jones, an auditor with one of the new generation banks here
in Burkina Faso.
I am writing you this letter based on the latest development at my
Department. i discovered some abandoned huge amount of money, Ten
Million, Five hundred thousand United States Dollars.($10.500.000).
Now I am only contacting you as a foreigner because this money cannot
be approved to a local bank account here, but can only be approved to
any foreign account and foreign beneficiary because the money is in US
dollars
This will be a legitimate transaction once you accept to build trust
with me and follow simple instruction doing the transfer process,
until the total sum transfer out of the bank here to your own bank
account any where in the world, and I agreed to share the total money
50/50 with you once you successful confirmed it in your bank account.
But any expenses doing the transfer process will be deduct from the
amount before sharing, If you are interested to work with me and
provide a good receiving bank account, get back to me as soon as
possible with the following details below.
Your full name
Your Profession
Your direct mobile phone number
Your Scanned International passport or any of your identity
NOTE: PLEASE IT YOU ARE NOT INTERESTED DON'T BORDER TO RESPOND BACK TO
AVOID TIME WASTED.
As soon as I receive these data's, I will forward to you the
application form which you will send to the bank for the claim and
transfer of the fund into your bank account as the new beneficial.
I am waiting to hear from you soon
Yours
Mrs.Susan Jones
^ permalink raw reply
* Re: [PATCH 2/2] clk: tegra: Always program PLL_E when enabled
From: Dmitry Osipenko @ 2020-06-03 14:04 UTC (permalink / raw)
To: Thierry Reding
Cc: Michael Turquette, Stephen Boyd, Jon Hunter, LABBE Corentin,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20200603111923.3545261-2-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
03.06.2020 14:19, Thierry Reding пишет:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> Commit bff1cef5f23a ("clk: tegra: Don't enable already enabled PLLs")
> added checks to avoid enabling PLLs that have already been enabled by
> the bootloader. However, the PLL_E configuration inherited from the
> bootloader isn't necessarily the one that is needed for the kernel.
>
> This can cause SATA to fail like this:
>
> [ 5.310270] phy phy-sata.6: phy poweron failed --> -110
> [ 5.315604] tegra-ahci 70027000.sata: failed to power on AHCI controller: -110
> [ 5.323022] tegra-ahci: probe of 70027000.sata failed with error -110
>
> Fix this by always programming the PLL_E. This ensures that any mis-
> configuration by the bootloader will be overwritten by the kernel.
>
> Fixes: bff1cef5f23a ("clk: tegra: Don't enable already enabled PLLs")
> Reported-by: LABBE Corentin <clabbe-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> drivers/clk/tegra/clk-pll.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> index 583d2ac61e9e..b2d39a66f0fa 100644
> --- a/drivers/clk/tegra/clk-pll.c
> +++ b/drivers/clk/tegra/clk-pll.c
> @@ -1601,9 +1601,6 @@ static int clk_plle_tegra114_enable(struct clk_hw *hw)
> unsigned long flags = 0;
> unsigned long input_rate;
>
> - if (clk_pll_is_enabled(hw))
> - return 0;
> -
> input_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
>
> if (_get_table_rate(hw, &sel, pll->params->fixed_rate, input_rate))
>
Reviewed-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
^ permalink raw reply
* [PATCH] drm/nouveau: gr/gk20a: Use firmware version 0
From: Thierry Reding @ 2020-06-03 14:20 UTC (permalink / raw)
To: Ben Skeggs
Cc: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Tegra firmware doesn't actually use any version numbers and passing -1
causes the existing firmware binaries not to be found. Use version 0 to
find the correct files.
Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
drivers/gpu/drm/nouveau/nvkm/engine/gr/gk20a.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/gr/gk20a.c b/drivers/gpu/drm/nouveau/nvkm/engine/gr/gk20a.c
index ec330d791d15..e56880f3e3bd 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/gr/gk20a.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/gr/gk20a.c
@@ -352,7 +352,7 @@ gk20a_gr_load(struct gf100_gr *gr, int ver, const struct gf100_gr_fwif *fwif)
static const struct gf100_gr_fwif
gk20a_gr_fwif[] = {
- { -1, gk20a_gr_load, &gk20a_gr },
+ { 0, gk20a_gr_load, &gk20a_gr },
{}
};
--
2.24.1
^ permalink raw reply related
* Re: [PATCH 4.9 00/55] 4.9.226-rc3 review
From: Jon Hunter @ 2020-06-03 15:05 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-0h96xk9xTtrk1uMJSBkQmQ, shuah-DgEjT+Ai2ygdnm+yROfE0A,
patches-ssFOTAMYnuFg9hUCZPvPmw,
ben.hutchings-4yDnlxn2s6sWdaTGBSpHTA,
lkft-triage-cunTk1MwBs8s++Sfvej+rw, stable-u79uwXL29TY76Z2rM5mHXA,
linux-tegra
In-Reply-To: <4eaf333a-e497-d39e-338e-a790b116dc62-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On 03/06/2020 11:42, Jon Hunter wrote:
>
> On 02/06/2020 19:13, Greg Kroah-Hartman wrote:
>> This is the start of the stable review cycle for the 4.9.226 release.
>> There are 55 patches in this series, all will be posted as a response
>> to this one. If anyone has any issues with these being applied, please
>> let me know.
>>
>> Responses should be made by Thu, 04 Jun 2020 18:12:28 +0000.
>> Anything received after that time might be too late.
>>
>> The whole patch series can be found in one patch at:
>> https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.9.226-rc3.gz
>> or in the git tree and branch at:
>> git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.9.y
>> and the diffstat can be found below.
>>
>> thanks,
>>
>> greg k-h
>
>
> All tests are passing for Tegra. Seems that our test gremlins are still
> at large and I cannot pull the report yet. However, I can see that
> everything is passing fine.
Just for completeness ...
Test results for stable-v4.9:
8 builds: 8 pass, 0 fail
16 boots: 16 pass, 0 fail
26 tests: 26 pass, 0 fail
Linux version: 4.9.226-rc3-ga836fd8c024d
Boards tested: tegra124-jetson-tk1, tegra20-ventana,
tegra210-p2371-2180, tegra30-cardhu-a04
Jon
--
nvpublic
^ permalink raw reply
* Re: [Nouveau] [PATCH] drm/nouveau: gr/gk20a: Use firmware version 0
From: Emil Velikov @ 2020-06-03 15:39 UTC (permalink / raw)
To: Thierry Reding
Cc: Ben Skeggs, linux-tegra-u79uwXL29TY76Z2rM5mHXA, ML nouveau,
ML dri-devel
In-Reply-To: <20200603142002.3776672-1-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Wed, 3 Jun 2020 at 15:20, Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> Tegra firmware doesn't actually use any version numbers and passing -1
> causes the existing firmware binaries not to be found. Use version 0 to
> find the correct files.
>
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Fixes: ef16dc278ec2 ("drm/nouveau/gr/gf100-: select implementation
based on available FW")
Reviewed-by: Emil Velikov <emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
-Emil
^ permalink raw reply
* Re: [PATCH V4] pwm: tegra: dynamic clk freq configuration by PWM driver
From: Uwe Kleine-König @ 2020-06-03 16:29 UTC (permalink / raw)
To: Sandipan Patra
Cc: treding, jonathanh, bbasu, ldewangan, kyarlagadda, linux-pwm,
linux-tegra, linux-kernel
In-Reply-To: <1590988836-11308-1-git-send-email-spatra@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 5826 bytes --]
Hello,
On Mon, Jun 01, 2020 at 10:50:36AM +0530, Sandipan Patra wrote:
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index d26ed8f..1daf591 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -4,8 +4,36 @@
> *
> * Tegra pulse-width-modulation controller driver
> *
> - * Copyright (c) 2010, NVIDIA Corporation.
> + * Copyright (c) 2010-2020, NVIDIA Corporation.
> * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer <s.hauer@pengutronix.de>
> + *
> + * Overview of Tegra Pulse Width Modulator Register:
> + * 1. 13-bit: Frequency division (SCALE)
> + * 2. 8-bit : Pulse division (DUTY)
> + * 3. 1-bit : Enable bit
> + *
> + * The PWM clock frequency is divided by 256 before subdividing it based
> + * on the programmable frequency division value to generate the required
> + * frequency for PWM output. The maximum output frequency that can be
> + * achieved is (max rate of source clock) / 256.
> + * e.g. if source clock rate is 408 MHz, maximum output frequency can be:
> + * 408 MHz/256 = 1.6 MHz.
> + * This 1.6 MHz frequency can further be divided using SCALE value in PWM.
> + *
> + * PWM pulse width: 8 bits are usable [23:16] for varying pulse width.
> + * To achieve 100% duty cycle, program Bit [24] of this register to
> + * 1’b1. In which case the other bits [23:16] are set to don't care.
> + *
> + * Limitations:
> + * - When PWM is disabled, the output is driven to inactive.
> + * - It does not allow the current PWM period to complete and
> + * stops abruptly.
> + *
I'd prefer to have no empty lines in the in Limitations paragraph to be
able to get all infos using something like:
sed -rn '/\* Limitations:/,/^ \*\/?$/p' drivers/pwm/pwm-tegra.c
> + * - If the register is reconfigured while PWM is running,
> + * it does not complete the currently running period.
> + *
> + * - If the user input duty is beyond acceptible limits,
> + * -EINVAL is returned.
s/acceptible/acceptable/ (but in fact this isn't a limitation, so I'd
drop this here, as pointed out in v2).
In v2 I mentioned a few things to add here.
> */
>
> #include <linux/clk.h>
> @@ -41,6 +69,7 @@ struct tegra_pwm_chip {
> struct reset_control*rst;
>
> unsigned long clk_rate;
> + unsigned long min_period_ns;
>
> void __iomem *regs;
>
> @@ -68,7 +97,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> {
> struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> unsigned long long c = duty_ns, hz;
> - unsigned long rate;
> + unsigned long rate, required_clk_rate;
In v2 I requested to move this into the if block below. You replied to
want to move it accordingly.
> u32 val = 0;
> int err;
>
> @@ -83,9 +112,47 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> val = (u32)c << PWM_DUTY_SHIFT;
>
> /*
> + * min period = max clock limit >> PWM_DUTY_WIDTH
> + */
> + if (period_ns < pc->min_period_ns)
> + return -EINVAL;
> +
> + /*
> * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
> * cycles at the PWM clock rate will take period_ns nanoseconds.
> + *
> + * num_channels: If single instance of PWM controller has multiple
> + * channels (e.g. Tegra210 or older) then it is not possible to
> + * configure separate clock rates to each of the channels, in such
> + * case the value stored during probe will be referred.
> + *
> + * If every PWM controller instance has one channel respectively, i.e.
> + * nums_channels == 1 then only the clock rate can be modified
> + * dynamically (e.g. Tegra186 or Tegra194).
> */
> + if (pc->soc->num_channels == 1) {
> + /*
> + * Rate is multiplied with 2^PWM_DUTY_WIDTH so that it matches
> + * with the maximum possible rate that the controller can
> + * provide. Any further lower value can be derived by setting
> + * PFM bits[0:12].
It looks a bit strange that the algorithm to calculate the clock
settings depends on the number of channels. Looks like a wrong
abstraction.
> + *
> + * required_clk_rate is a reference rate for source clock and
> + * it is derived based on user requested period. By setting the
> + * source clock rate as required_clk_rate, PWM controller will
> + * be able to configure the requested period.
> + */
> + required_clk_rate =
> + (NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
> +
> + err = clk_set_rate(pc->clk, required_clk_rate);
> + if (err < 0)
> + return -EINVAL;
> +
> + /* Store the new rate for further references */
> + pc->clk_rate = clk_get_rate(pc->clk);
> + }
> +
> rate = pc->clk_rate >> PWM_DUTY_WIDTH;
>
> /* Consider precision in PWM_SCALE_WIDTH rate calculation */
> @@ -94,7 +161,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>
> /*
> * Since the actual PWM divider is the register's frequency divider
> - * field minus 1, we need to decrement to get the correct value to
> + * field plus 1, we need to decrement to get the correct value to
I would have put this in a separate change.
> * write to the register.
> */
> if (rate > 0)
> @@ -205,6 +272,10 @@ static int tegra_pwm_probe(struct platform_device *pdev)
> */
> pwm->clk_rate = clk_get_rate(pwm->clk);
>
> + /* Set minimum limit of PWM period for the IP */
> + pwm->min_period_ns =
> + (NSEC_PER_SEC / (pwm->soc->max_frequency >> PWM_DUTY_WIDTH)) + 1;
To ensure that required_clk_rate in tegra_pwm_config doesn't get bigger
than pwm->soc->max_frequency this isn't the right formula I think. I'd
use
pwm->min_period_ns = DIV_ROUNDUP(NSEC_PER_SEC, pwm->soc->max_frequency >> PWM_DUTY_WIDTH);
. Can you confirm?
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
From: Bjorn Andersson @ 2020-06-03 17:17 UTC (permalink / raw)
To: Thierry Reding
Cc: Will Deacon, John Stultz, Robin Murphy, Laurentiu Tudor,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, linux-arm-msm, Joerg Roedel,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-arm-kernel
In-Reply-To: <20200603102446.GA3478467@ulmo>
On Wed 03 Jun 03:24 PDT 2020, Thierry Reding wrote:
> On Tue, Jun 02, 2020 at 12:32:49PM -0700, Bjorn Andersson wrote:
> > On Tue 02 Jun 04:02 PDT 2020, Thierry Reding wrote:
> >
> > > On Wed, May 27, 2020 at 12:03:44PM +0100, Will Deacon wrote:
> > > > Hi John, Bjorn,
> > > >
> > > > On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote:
> > > > > On Thu, May 14, 2020 at 12:34 PM <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > > > > >
> > > > > > On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote:
> > > > > >
> > > > > > Rob, Will, we're reaching the point where upstream has enough
> > > > > > functionality that this is becoming a critical issue for us.
> > > > > >
> > > > > > E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot
> > > > > > mainline with display, GPU, WiFi and audio working and the story is
> > > > > > similar on several devboards.
> > > > > >
> > > > > > As previously described, the only thing I want is the stream mapping
> > > > > > related to the display controller in place, either with the CB with
> > > > > > translation disabled or possibly with a way to specify the framebuffer
> > > > > > region (although this turns out to mess things up in the display
> > > > > > driver...)
> > > > > >
> > > > > > I did pick this up again recently and concluded that by omitting the
> > > > > > streams for the USB controllers causes an instability issue seen on one
> > > > > > of the controller to disappear. So I would prefer if we somehow could
> > > > > > have a mechanism to only pick the display streams and the context
> > > > > > allocation for this.
> > > > > >
> > > > > >
> > > > > > Can you please share some pointers/insights/wishes for how we can
> > > > > > conclude on this subject?
> > > > >
> > > > > Ping? I just wanted to follow up on this discussion as this small
> > > > > series is crucial for booting mainline on the Dragonboard 845c
> > > > > devboard. It would be really valuable to be able to get some solution
> > > > > upstream so we can test mainline w/o adding additional patches.
> > > >
> > > > Sorry, it's been insanely busy recently and I haven't had a chance to think
> > > > about this on top of everything else. We're also carrying a hack in Android
> > > > for you :)
> > > >
> > > > > The rest of the db845c series has been moving forward smoothly, but
> > > > > this set seems to be very stuck with no visible progress since Dec.
> > > > >
> > > > > Are there any pointers for what folks would prefer to see?
> > > >
> > > > I've had a chat with Robin about this. Originally, I was hoping that
> > > > people would all work together towards an idyllic future where firmware
> > > > would be able to describe arbitrary pre-existing mappings for devices,
> > > > irrespective of the IOMMU through which they master and Linux could
> > > > inherit this configuration. However, that hasn't materialised (there was
> > > > supposed to be an IORT update, but I don't know what happened to that)
> > > > and, in actual fact, the problem that you have on db845 is /far/ more
> > > > restricted than the general problem.
> > >
> > > It doesn't sound to me like implementing platform-specific workarounds
> > > is a good long-term solution (especially since, according to Bjorn, they
> > > aren't as trivial to implement as it sounds). And we already have all
> > > the infrastructure in place to implement what you describe, so I don't
> > > see why we shouldn't do that. This patchset uses standard device tree
> > > bindings that were designed for exactly this kind of use-case.
> > >
> >
> > I think my results would imply that we would have to end up with (at
> > least) some special case of your proposal (i.e. we need a context bank
> > allocated).
>
> I wasn't talking about implementation details, but rather about the
> surrounding infrastructure. It seemed like Will was suggesting that
> there's no way of conveying what memory regions to direct-map from
> the firmware to the kernel. But that really isn't the problem here,
> is it? What we're really looking for is how to take what we have in
> device tree and use it in the ARM SMMU driver to create an early
> mapping that will stay in place until a device has been properly
> attached to the IOMMU domain.
>
I agree.
We do have the iommu properties from our display node and we typically
do have a reserved-memory region for the existing framebuffer that we
want to associate with those properties.
The one exception is on devices targeted to run Windows, where the
memory region is passed to the kernel using UEFI GOP.
But as you say, the information is available to the kernel already.
> > > So at least for device-tree based boot firmware can already describe
> > > these pre-existing mappings. If something standard materializes for ACPI
> > > eventually I'm sure we can find ways to integrate that into whatever we
> > > come up with now for DT.
> > >
> > > I think between Bjorn, John, Laurentiu and myself there's pretty broad
> > > consensus (correct me if I'm wrong, guys) that solving this via reserved
> > > memory regions is a good solution that works. So I think what's really
> > > missing is feedback on whether the changes proposed here or Laurentiu's
> > > updated proposal[0] are acceptable, and if not, what the preference is
> > > for getting something equivalent upstream.
> > >
> >
> > As described in my reply to your proposal, the one problem I ran into
> > was that I haven't figured out how to reliably "move" my display streams
> > from one mapping entry to another.
> >
> > With the current scheme I see that their will either be gaps in time
> > with no mapping for my display, or multiple mappings.
>
> I think you would inevitably end up with two mappings for a transitional
> period while you prepare the final mapping that you want to switch to.
>
I think that's fine, I saw that there's a bit to configure if multiple
SMR matches is considered a fault or not, so I will verify if this is
what causes the faults I saw on one of my platforms and if I can change
it.
I still would need that we ensure that we don't have lapses in time
where we're lacking mappings among the SMRs.
> > The other thing I noticed in your proposal was that I have a whole bunch
> > of DT nodes with both iommus and memory-region properties that I really
> > don't care to set up mappings for, but I've not finalized my thoughts on
> > this causing actual problems...
>
> Can you be more specific? It'd be useful to understand all of the
> existing uses of reserved memory regions in order to make sure we
> accomodate all of them.
>
I have reserved-memory regions for a number of co-processors, to
facilitate static access configuration, as such I ended up with having
100+MB of various DSP memory mapped into the temporary context.
> I'd be surprised, though, if setting up mappings for any of these
> regions would actually cause breakage. If a device tree node has a
> memory-region property it means that this memory region is eventually
> going to be used by the device and if the device tree node also has an
> iommus property it means that it's meant to use the IOMMU for
> translating memory accesses. It's therefore very likely that the memory
> region will need to be mapped. Whether it needs to be a direct mapping
> or not might be worth having a discussion about.
>
Right, I think that before any of these are actually used we will have a
device attached and the region properly mapped (as is done today
already).
> > > Just to highlight: the IOMMU framework already provides infrastructure
> > > to create direct mappings (via iommu_get_resv_regions(), called from
> > > iommu_create_device_direct_mappings()). I have patches that make use of
> > > this on Tegra210 and earlier where a non-ARM SMMU is used and where the
> > > IOMMU driver enables translations (and doesn't fault by default) only at
> > > device attachment time. That works perfectly using reserved-memory
> > > regions. Perhaps that infrastructure could be extended to cover the
> > > kinds of early mappings that we're discussing here. On the other hand it
> > > might be a bit premature at this point because the ARM SMMU is the only
> > > device that currently needs this, as far as I can tell.
> > >
> >
> > For Qualcomm we got patches picked up for 5.8 that will cause the
> > display controller to be attached with direct mapping, so I think all
> > missing now is the lack of stream mappings between arm-smmu probe and
> > display driver probe...
>
> Can you point me at those patches? I'd like to look at them and see if
> they match what I have planned for Tegra or if we can somehow converge
> on a common scheme.
>
You can find this in:
0e764a01015d ("iommu/arm-smmu: Allow client devices to select direct mapping")
Which conceptually follows what is done on x86 machines.
Regards,
Bjorn
^ permalink raw reply
* Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
From: Bjorn Andersson @ 2020-06-03 17:23 UTC (permalink / raw)
To: Will Deacon
Cc: John Stultz, Thierry Reding, Robin Murphy,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, linux-arm-msm, Joerg Roedel,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-arm-kernel
In-Reply-To: <20200603111159.GA8408@willie-the-truck>
On Wed 03 Jun 04:11 PDT 2020, Will Deacon wrote:
> On Mon, Jun 01, 2020 at 11:32:10PM -0700, Bjorn Andersson wrote:
> > On Wed 27 May 04:03 PDT 2020, Will Deacon wrote:
> >
> > > Hi John, Bjorn,
> > >
> > > On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote:
> > > > On Thu, May 14, 2020 at 12:34 PM <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > > > >
> > > > > On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote:
> > > > >
> > > > > Rob, Will, we're reaching the point where upstream has enough
> > > > > functionality that this is becoming a critical issue for us.
> > > > >
> > > > > E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot
> > > > > mainline with display, GPU, WiFi and audio working and the story is
> > > > > similar on several devboards.
> > > > >
> > > > > As previously described, the only thing I want is the stream mapping
> > > > > related to the display controller in place, either with the CB with
> > > > > translation disabled or possibly with a way to specify the framebuffer
> > > > > region (although this turns out to mess things up in the display
> > > > > driver...)
> > > > >
> > > > > I did pick this up again recently and concluded that by omitting the
> > > > > streams for the USB controllers causes an instability issue seen on one
> > > > > of the controller to disappear. So I would prefer if we somehow could
> > > > > have a mechanism to only pick the display streams and the context
> > > > > allocation for this.
> > > > >
> > > > >
> > > > > Can you please share some pointers/insights/wishes for how we can
> > > > > conclude on this subject?
> > > >
> > > > Ping? I just wanted to follow up on this discussion as this small
> > > > series is crucial for booting mainline on the Dragonboard 845c
> > > > devboard. It would be really valuable to be able to get some solution
> > > > upstream so we can test mainline w/o adding additional patches.
> > >
> > > Sorry, it's been insanely busy recently and I haven't had a chance to think
> > > about this on top of everything else. We're also carrying a hack in Android
> > > for you :)
> > >
> >
> > Thanks for taking the time to get back to us on this!
> >
> > > > The rest of the db845c series has been moving forward smoothly, but
> > > > this set seems to be very stuck with no visible progress since Dec.
> > > >
> > > > Are there any pointers for what folks would prefer to see?
> > >
> > > I've had a chat with Robin about this. Originally, I was hoping that
> > > people would all work together towards an idyllic future where firmware
> > > would be able to describe arbitrary pre-existing mappings for devices,
> > > irrespective of the IOMMU through which they master and Linux could
> > > inherit this configuration. However, that hasn't materialised (there was
> > > supposed to be an IORT update, but I don't know what happened to that)
> > > and, in actual fact, the problem that you have on db845 is /far/ more
> > > restricted than the general problem.
> > >
> > > Could you please try hacking something along the following lines and see
> > > how you get on? You may need my for-joerg/arm-smmu/updates branch for
> > > all the pieces:
> > >
> > > 1. Use the ->cfg_probe() callback to reserve the SMR/S2CRs you need
> > > "pinning" and configure for bypass.
> > >
> > > 2. Use the ->def_domain_type() callback to return IOMMU_DOMAIN_IDENTITY
> > > for the display controller
> > >
> > > I /think/ that's sufficient, but note that it differs from the current
> > > approach because we don't end up reserving a CB -- bypass is configured
> > > in the S2CR instead. Some invalidation might therefore be needed in
> > > ->cfg_probe() after unhooking the CB.
> > >
> > > Thanks, and please yell if you run into problems with this approach.
> > >
> >
> > This sounded straight forward and cleaner, so I implemented it...
> >
> > Unfortunately the hypervisor is playing tricks on me when writing to
> > S2CR registers:
> > - TRANS writes lands as requested
> > - BYPASS writes ends up in the register as requested, with type FAULT
> > - FAULT writes are ignored
> >
> > In other words, the Qualcomm firmware prevents us from relying on
> > marking the relevant streams as BYPASS type.
>
> Is this for all S2CR registers, or only the ones in use by the display
> controller? Is there any scope for stopping the hypervisor from doing this?
>
This is for all S2CR registers. There's no chance to change this and get
it deployed on all the devices people care about.
As you know John need this for the RB3/db845c, where we might have a
chance to modify the firmware.
But I'm writing this on my Lenovo Yoga C630 and John and his colleagues
are working on various phones, such as Pixel 3. None of these boots
without this "workaround" and I don't expect that we can propagate a
firmware modification to any of them - and definitely not all of them.
> It makes it really difficult for the driver when the hardware is emulated
> in a way that doesn't match the architecture...
>
Understood.
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCHv2 0/7] Support inhibiting input devices
From: Hans de Goede @ 2020-06-03 17:38 UTC (permalink / raw)
To: Andrzej Pietrasiewicz, Dmitry Torokhov
Cc: linux-input, linux-acpi, linux-iio, linux-arm-kernel,
linux-samsung-soc, linux-tegra, patches, ibm-acpi-devel,
platform-driver-x86, Rafael J . Wysocki, Len Brown,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Kukjin Kim, Krzysztof Kozlowski,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP
In-Reply-To: <fb5bee72-6a75-88aa-8157-75f07c491eeb@collabora.com>
Hi,
On 6/3/20 3:07 PM, Andrzej Pietrasiewicz wrote:
> Hi Hans, hi Dmitry,
<snip>
> I'm taking one step back and looking at the ->open() and ->close()
> driver callbacks. They are called from input_open_device() and
> input_close_device(), respectively:
>
> input_open_device():
> "This function should be called by input handlers when they
> want to start receive events from given input device."
>
> ->open() callback:
> "this method is called when the very first user calls
> input_open_device(). The driver must prepare the device to start
> generating events (start polling thread, request an IRQ, submit
> URB, etc.)"
>
> input_close_device():
> "This function should be called by input handlers when they
> want to stop receive events from given input device."
>
> ->close() callback:
> "this method is called when the very last user calls
> input_close_device()"
>
> It seems to me that the callback names do not reflect their
> purpose: their meaning is not to "open" or to "close" but to
> give drivers a chance to control when they start or stop
> providing events to the input core.
>
> What would you say about changing the callbacks' names?
> I'd envsion: ->provide_events() instead of ->open() and
> ->stop_events() instead of ->close(). Of course drivers can
> exploit the fact of knowing that nobody wants any events
> from them and do whatever they consider appropriate, for
> example go into a low power mode - but the latter is beyond
> the scope of the input subsystem and is driver-specific.
I don't have much of an opinion on changing the names,
to me open/close have always means start/stop receiving
events. This follows the everything is a file philosophy,
e.g. you can also not really "open" a serial port,
yet opening /dev/ttyS0 will activate the receive IRQ
of the UART, etc. So maybe we just need to make the
docs clearer rather then do the rename? Doing the
rename is certainly going to cause a lot of churn.
Anyways as said, I don't have much of an opinion,
so I'll leave commenting (more) on this to Dmitry.
> With such a naming change in mind let's consider inhibiting.
> We want to be able to control when to disregard events from
> a given device. It makes sense to do it at device level, otherwise
> such an operation would have to be invoked in all associated
> handlers (those that have an open handle associating them with
> the device in question). But of course we can do better than
> merely ignoring the events received: we can tell the drivers
> that we don't want any events from them, and later, at uninhibit
> time, tell them to start providing the events again. Conceptually,
> the two operations (provide or don't provide envents) are exactly
> the same thing we want to be happening at input_open_device() and
> input_close_device() time. To me, changing the names of
> ->open() and ->close() exposes this fact very well.
>
> Consequently, ->inhibit() and ->uninhibit() won't be needed,
> and drivers which already implement ->provide_events() (formerly
> ->open()) and ->stop_events() (formerly ->close()) will receive
> full inhibit/uninhibit support for free (subject to how well they
> implement ->provide_events()/->stop_events()). Unless we can come
> up with what the drivers might be doing on top of ->stop_events()
> and ->provide_events() when inhibiting/uninhibiting, but it seems
> to me we can't. Can we?
Right. I'm happy that you've come to see that both on open/close
and on inhibit/uninhibit we want to "start receiving events" and
"stop receiving events", so that we only need one set of callbacks.
> Optionally ->close() (only the callback, not input_close_device())
> can be made return a value, just as Hans suggests. The value
> can be ignored in input_close_device() but used in input_inhibit().
> No strong opinion here, though. (btw it seems to me that
> input_inhibit() should be renamed to input_inhibit_device()).
Ack.
Regards,
Hans
^ permalink raw reply
* Re: [PATCHv2 0/7] Support inhibiting input devices
From: Andrzej Pietrasiewicz @ 2020-06-03 17:54 UTC (permalink / raw)
To: Hans de Goede, Dmitry Torokhov
Cc: linux-input, linux-acpi, linux-iio, linux-arm-kernel,
linux-samsung-soc, linux-tegra, patches, ibm-acpi-devel,
platform-driver-x86, Rafael J . Wysocki, Len Brown,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Kukjin Kim, Krzysztof Kozlowski,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP
In-Reply-To: <681abc14-ef0f-ff15-68ed-944b2f96bdaf@redhat.com>
W dniu 03.06.2020 o 19:38, Hans de Goede pisze:
> Hi,
>
> On 6/3/20 3:07 PM, Andrzej Pietrasiewicz wrote:
>> Hi Hans, hi Dmitry,
>
> <snip>
>
>> I'm taking one step back and looking at the ->open() and ->close()
>> driver callbacks. They are called from input_open_device() and
>> input_close_device(), respectively:
>>
>> input_open_device():
>> "This function should be called by input handlers when they
>> want to start receive events from given input device."
>>
>> ->open() callback:
>> "this method is called when the very first user calls
>> input_open_device(). The driver must prepare the device to start
>> generating events (start polling thread, request an IRQ, submit
>> URB, etc.)"
>>
>> input_close_device():
>> "This function should be called by input handlers when they
>> want to stop receive events from given input device."
>>
>> ->close() callback:
>> "this method is called when the very last user calls
>> input_close_device()"
>>
>> It seems to me that the callback names do not reflect their
>> purpose: their meaning is not to "open" or to "close" but to
>> give drivers a chance to control when they start or stop
>> providing events to the input core.
>>
>> What would you say about changing the callbacks' names?
>> I'd envsion: ->provide_events() instead of ->open() and
>> ->stop_events() instead of ->close(). Of course drivers can
>> exploit the fact of knowing that nobody wants any events
>> from them and do whatever they consider appropriate, for
>> example go into a low power mode - but the latter is beyond
>> the scope of the input subsystem and is driver-specific.
>
> I don't have much of an opinion on changing the names,
> to me open/close have always means start/stop receiving
> events. This follows the everything is a file philosophy,
> e.g. you can also not really "open" a serial port,
> yet opening /dev/ttyS0 will activate the receive IRQ
> of the UART, etc. So maybe we just need to make the
> docs clearer rather then do the rename? Doing the
> rename is certainly going to cause a lot of churn.
Right, I can see now that the suggestion to change names is
too far fetched. (I feel that release() would be better
than close(), though). But it exposes the message I wanted to
pass.
>
> Anyways as said, I don't have much of an opinion,
> so I'll leave commenting (more) on this to Dmitry.
>
>> With such a naming change in mind let's consider inhibiting.
>> We want to be able to control when to disregard events from
>> a given device. It makes sense to do it at device level, otherwise
>> such an operation would have to be invoked in all associated
>> handlers (those that have an open handle associating them with
>> the device in question). But of course we can do better than
>> merely ignoring the events received: we can tell the drivers
>> that we don't want any events from them, and later, at uninhibit
>> time, tell them to start providing the events again. Conceptually,
>> the two operations (provide or don't provide envents) are exactly
>> the same thing we want to be happening at input_open_device() and
>> input_close_device() time. To me, changing the names of
>> ->open() and ->close() exposes this fact very well.
>>
>> Consequently, ->inhibit() and ->uninhibit() won't be needed,
>> and drivers which already implement ->provide_events() (formerly
>> ->open()) and ->stop_events() (formerly ->close()) will receive
>> full inhibit/uninhibit support for free (subject to how well they
>> implement ->provide_events()/->stop_events()). Unless we can come
>> up with what the drivers might be doing on top of ->stop_events()
>> and ->provide_events() when inhibiting/uninhibiting, but it seems
>> to me we can't. Can we?
>
> Right. I'm happy that you've come to see that both on open/close
> and on inhibit/uninhibit we want to "start receiving events" and
> "stop receiving events", so that we only need one set of callbacks.
>
Yeah, that's my conclusion - at least on a conceptual level.
That said, what I can imagine is an existing driver (e.g. elan_i2c)
which does not implement neither open() nor close(), but does have
suspend() and resume(). Then it is maybe a bit easier to add inhibit()
and uninhibit() /they would be similar to suspend and resume/ instead
of open() and close(): If only open() and close() are possible, then
the probe function needs to be extended to "close" the device before it
gets registered, because from the moment it is registered it might be
opened right away. And the device must be available earlier during the
course of probe to query some parameters through i2c:
+static int elan_reactivate(struct elan_tp_data *data)
+{
+ struct device *dev = &data->client->dev;
+ int ret;
+
+ ret = elan_enable_power(data);
+ if (ret)
+ dev_err(dev, "failed to restore power: %d\n", ret);
+
+ ret = elan_initialize(data);
+ if (ret)
+ dev_err(dev, "failed to re-initialize touchpad: %d\n", ret);
+
+ return ret;
+}
+
+static int elan_open(struct input_dev *input)
+{
+ struct elan_tp_data *data = input_get_drvdata(input);
+ struct i2c_client *client = data->client;
+ int ret;
+
+ dev_dbg(&client->dev, "uninhibiting\n");
+
+ ret = mutex_lock_interruptible(&data->sysfs_mutex);
+ if (ret)
+ return ret;
+
+ ret = elan_reactivate(data);
+ if (ret == 0)
+ enable_irq(client->irq);
+
+ mutex_unlock(&data->sysfs_mutex);
+
+ return ret;
+}
+
+static int elan_inhibit(struct input_dev *input)
+{
+ struct elan_tp_data *data = input_get_drvdata(input);
+ struct i2c_client *client = data->client;
+ int ret;
+
+ dev_dbg(&client->dev, "closing\n");
+
+ /*
+ * We are taking the mutex to make sure sysfs operations are
+ * complete before we attempt to bring the device into low[er]
+ * power mode.
+ */
+ ret = mutex_lock_interruptible(&data->sysfs_mutex);
+ if (ret)
+ return ret;
+
+ disable_irq(client->irq);
+
+ ret = elan_disable_power(data);
+ if (ret)
+ enable_irq(client->irq);
+
+ mutex_unlock(&data->sysfs_mutex);
+
+ return ret;
+}
+
+static void elan_close(struct input_dev *input)
+{
+ elan_inhibit(input);
+}
+
static int elan_query_device_info(struct elan_tp_data *data)
{
int error;
u16 ic_type;
error = data->ops->get_version(data->client, false, &data->fw_version);
if (error)
return error;
error = data->ops->get_checksum(data->client, false,
&data->fw_checksum);
if (error)
return error;
error = data->ops->get_version(data->client, true, &data->iap_version);
if (error)
return error;
@@ -1071,34 +1141,36 @@ static int elan_setup_trackpoint_input_device(struct
elan_tp_data *data)
static int elan_setup_input_device(struct elan_tp_data *data)
{
struct device *dev = &data->client->dev;
struct input_dev *input;
unsigned int max_width = max(data->width_x, data->width_y);
unsigned int min_width = min(data->width_x, data->width_y);
int error;
input = devm_input_allocate_device(dev);
if (!input)
return -ENOMEM;
input->name = "Elan Touchpad";
input->id.bustype = BUS_I2C;
input->id.vendor = ELAN_VENDOR_ID;
input->id.product = data->product_id;
+ input->open = elan_open;
+ input->close = elan_close;
input_set_drvdata(input, data);
error = input_mt_init_slots(input, ETP_MAX_FINGERS,
INPUT_MT_POINTER | INPUT_MT_DROP_UNUSED);
if (error) {
dev_err(dev, "failed to initialize MT slots: %d\n", error);
return error;
}
__set_bit(EV_ABS, input->evbit);
__set_bit(INPUT_PROP_POINTER, input->propbit);
if (data->clickpad) {
__set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
} else {
__set_bit(BTN_RIGHT, input->keybit);
if (data->middle_button)
__set_bit(BTN_MIDDLE, input->keybit);
@@ -1253,34 +1325,40 @@ static int elan_probe(struct i2c_client *client,
if (!irqflags)
irqflags = IRQF_TRIGGER_FALLING;
error = devm_request_threaded_irq(dev, client->irq, NULL, elan_isr,
irqflags | IRQF_ONESHOT,
client->name, data);
if (error) {
dev_err(dev, "cannot register irq=%d\n", client->irq);
return error;
}
error = devm_device_add_groups(dev, elan_sysfs_groups);
if (error) {
dev_err(dev, "failed to create sysfs attributes: %d\n", error);
return error;
}
+ error = elan_inhibit(data->input);
+ if (error) {
+ dev_err(dev, "failed to inhibit input device before registering: %d\n", error);
+ return error;
+ }
+
error = input_register_device(data->input);
if (error) {
dev_err(dev, "failed to register input device: %d\n", error);
return error;
}
if (data->tp_input) {
error = input_register_device(data->tp_input);
if (error) {
dev_err(&client->dev,
"failed to register TrackPoint input device: %d\n",
error);
return error;
}
}
/*
@@ -1294,72 +1372,71 @@ static int elan_probe(struct i2c_client *client,
}
static int __maybe_unused elan_suspend(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
struct elan_tp_data *data = i2c_get_clientdata(client);
int ret;
/*
* We are taking the mutex to make sure sysfs operations are
* complete before we attempt to bring the device into low[er]
* power mode.
*/
ret = mutex_lock_interruptible(&data->sysfs_mutex);
if (ret)
return ret;
- disable_irq(client->irq);
+ mutex_lock(&data->input->mutex);
+ if (input_device_enabled(data->input)) {
+ disable_irq(client->irq);
- if (device_may_wakeup(dev)) {
- ret = elan_sleep(data);
- /* Enable wake from IRQ */
- data->irq_wake = (enable_irq_wake(client->irq) == 0);
- } else {
- ret = elan_disable_power(data);
+ if (device_may_wakeup(dev)) {
+ ret = elan_sleep(data);
+ /* Enable wake from IRQ */
+ data->irq_wake = (enable_irq_wake(client->irq) == 0);
+ } else {
+ ret = elan_disable_power(data);
+ }
}
+ mutex_unlock(&data->input->mutex);
mutex_unlock(&data->sysfs_mutex);
return ret;
}
static int __maybe_unused elan_resume(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
struct elan_tp_data *data = i2c_get_clientdata(client);
- int error;
+ int ret = 0;
- if (device_may_wakeup(dev) && data->irq_wake) {
- disable_irq_wake(client->irq);
- data->irq_wake = false;
- }
+ mutex_lock(&data->input->mutex);
+ if (input_device_enabled(data->input)) {
+ if (data->irq_wake) {
+ disable_irq_wake(client->irq);
+ data->irq_wake = false;
+ }
- error = elan_enable_power(data);
- if (error) {
- dev_err(dev, "power up when resuming failed: %d\n", error);
- goto err;
+ ret = elan_reactivate(data);
+ enable_irq(data->client->irq);
}
+ mutex_unlock(&data->input->mutex);
- error = elan_initialize(data);
- if (error)
- dev_err(dev, "initialize when resuming failed: %d\n", error);
-
-err:
- enable_irq(data->client->irq);
- return error;
+ return ret;
}
Regards,
Andrzej
^ permalink raw reply
* [PATCH] dmaengine: tegra210-adma: fix pm_runtime_get_sync failure
From: Navid Emamdoost @ 2020-06-03 18:38 UTC (permalink / raw)
To: Laxman Dewangan, Jon Hunter, Dan Williams, Vinod Koul,
Thierry Reding, dmaengine-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: emamd001-OJFnDUYgAso, wu000273-OJFnDUYgAso, kjlu-OJFnDUYgAso,
smccaman-OJFnDUYgAso, Navid Emamdoost
Calling pm_runtime_get_sync increments the counter even in case of
failure, causing incorrect ref count. Call pm_runtime_put if
pm_runtime_get_sync fails.
Signed-off-by: Navid Emamdoost <navid.emamdoost-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/dma/tegra210-adma.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/dma/tegra210-adma.c b/drivers/dma/tegra210-adma.c
index c4ce5dfb149b..87f2a1bed3aa 100644
--- a/drivers/dma/tegra210-adma.c
+++ b/drivers/dma/tegra210-adma.c
@@ -869,8 +869,10 @@ static int tegra_adma_probe(struct platform_device *pdev)
pm_runtime_enable(&pdev->dev);
ret = pm_runtime_get_sync(&pdev->dev);
- if (ret < 0)
+ if (ret < 0) {
+ pm_runtime_put_sync(&pdev->dev);
goto rpm_disable;
+ }
ret = tegra_adma_init(tdma);
if (ret)
--
2.17.1
^ permalink raw reply related
* [PATCH] dmaengine: tegra210-adma: fix pm_runtime_get_sync failure
From: Navid Emamdoost @ 2020-06-03 18:41 UTC (permalink / raw)
To: Laxman Dewangan, Jon Hunter, Vinod Koul, Dan Williams,
Thierry Reding, dmaengine-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: emamd001-OJFnDUYgAso, wu000273-OJFnDUYgAso, kjlu-OJFnDUYgAso,
smccaman-OJFnDUYgAso, Navid Emamdoost
Calling pm_runtime_get_sync increments the counter even in case of
failure, causing incorrect ref count. Call pm_runtime_put if
pm_runtime_get_sync fails.
Signed-off-by: Navid Emamdoost <navid.emamdoost-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/dma/tegra210-adma.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/dma/tegra210-adma.c b/drivers/dma/tegra210-adma.c
index c4ce5dfb149b..e8c749cd3fe8 100644
--- a/drivers/dma/tegra210-adma.c
+++ b/drivers/dma/tegra210-adma.c
@@ -659,6 +659,7 @@ static int tegra_adma_alloc_chan_resources(struct dma_chan *dc)
ret = pm_runtime_get_sync(tdc2dev(tdc));
if (ret < 0) {
free_irq(tdc->irq, tdc);
+ pm_runtime_put(tdc2dev(tdc));
return ret;
}
--
2.17.1
^ permalink raw reply related
* Re: [PATCHv2 0/7] Support inhibiting input devices
From: Hans de Goede @ 2020-06-03 19:37 UTC (permalink / raw)
To: Andrzej Pietrasiewicz, Dmitry Torokhov
Cc: linux-input, linux-acpi, linux-iio, linux-arm-kernel,
linux-samsung-soc, linux-tegra, patches, ibm-acpi-devel,
platform-driver-x86, Rafael J . Wysocki, Len Brown,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Kukjin Kim, Krzysztof Kozlowski,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP
In-Reply-To: <025361f4-5b1b-6669-ffa0-a6e8ad43940c@collabora.com>
Hi,
On 6/3/20 7:54 PM, Andrzej Pietrasiewicz wrote:
> W dniu 03.06.2020 o 19:38, Hans de Goede pisze:
>> Hi,
>>
>> On 6/3/20 3:07 PM, Andrzej Pietrasiewicz wrote:
>>> Hi Hans, hi Dmitry,
>>
>> <snip>
>>
>>> I'm taking one step back and looking at the ->open() and ->close()
>>> driver callbacks. They are called from input_open_device() and
>>> input_close_device(), respectively:
>>>
>>> input_open_device():
>>> "This function should be called by input handlers when they
>>> want to start receive events from given input device."
>>>
>>> ->open() callback:
>>> "this method is called when the very first user calls
>>> input_open_device(). The driver must prepare the device to start
>>> generating events (start polling thread, request an IRQ, submit
>>> URB, etc.)"
>>>
>>> input_close_device():
>>> "This function should be called by input handlers when they
>>> want to stop receive events from given input device."
>>>
>>> ->close() callback:
>>> "this method is called when the very last user calls
>>> input_close_device()"
>>>
>>> It seems to me that the callback names do not reflect their
>>> purpose: their meaning is not to "open" or to "close" but to
>>> give drivers a chance to control when they start or stop
>>> providing events to the input core.
>>>
>>> What would you say about changing the callbacks' names?
>>> I'd envsion: ->provide_events() instead of ->open() and
>>> ->stop_events() instead of ->close(). Of course drivers can
>>> exploit the fact of knowing that nobody wants any events
>>> from them and do whatever they consider appropriate, for
>>> example go into a low power mode - but the latter is beyond
>>> the scope of the input subsystem and is driver-specific.
>>
>> I don't have much of an opinion on changing the names,
>> to me open/close have always means start/stop receiving
>> events. This follows the everything is a file philosophy,
>> e.g. you can also not really "open" a serial port,
>> yet opening /dev/ttyS0 will activate the receive IRQ
>> of the UART, etc. So maybe we just need to make the
>> docs clearer rather then do the rename? Doing the
>> rename is certainly going to cause a lot of churn.
>
> Right, I can see now that the suggestion to change names is
> too far fetched. (I feel that release() would be better
> than close(), though). But it exposes the message I wanted to
> pass.
>
>>
>> Anyways as said, I don't have much of an opinion,
>> so I'll leave commenting (more) on this to Dmitry.
>>
>>> With such a naming change in mind let's consider inhibiting.
>>> We want to be able to control when to disregard events from
>>> a given device. It makes sense to do it at device level, otherwise
>>> such an operation would have to be invoked in all associated
>>> handlers (those that have an open handle associating them with
>>> the device in question). But of course we can do better than
>>> merely ignoring the events received: we can tell the drivers
>>> that we don't want any events from them, and later, at uninhibit
>>> time, tell them to start providing the events again. Conceptually,
>>> the two operations (provide or don't provide envents) are exactly
>>> the same thing we want to be happening at input_open_device() and
>>> input_close_device() time. To me, changing the names of
>>> ->open() and ->close() exposes this fact very well.
>>>
>>> Consequently, ->inhibit() and ->uninhibit() won't be needed,
>>> and drivers which already implement ->provide_events() (formerly
>>> ->open()) and ->stop_events() (formerly ->close()) will receive
>>> full inhibit/uninhibit support for free (subject to how well they
>>> implement ->provide_events()/->stop_events()). Unless we can come
>>> up with what the drivers might be doing on top of ->stop_events()
>>> and ->provide_events() when inhibiting/uninhibiting, but it seems
>>> to me we can't. Can we?
>>
>> Right. I'm happy that you've come to see that both on open/close
>> and on inhibit/uninhibit we want to "start receiving events" and
>> "stop receiving events", so that we only need one set of callbacks.
>>
>
> Yeah, that's my conclusion - at least on a conceptual level.
>
> That said, what I can imagine is an existing driver (e.g. elan_i2c)
> which does not implement neither open() nor close(), but does have
> suspend() and resume(). Then it is maybe a bit easier to add inhibit()
> and uninhibit() /they would be similar to suspend and resume/ instead
> of open() and close(): If only open() and close() are possible, then
> the probe function needs to be extended to "close" the device before it
> gets registered, because from the moment it is registered it might be
> opened right away.
The probe only needs to "close" it if for some reason it
starts directly sending events in most cases the driver
must actively do something to get it to send events.
So in most cases this should be pretty straight forward,
as for having to do some init / power-on during probe
and then power-off at the end of the probe. Yes sometimes
something like that might be necessary.
Looking at your suggested elan_i2c changes I think they
look fine. I have the feeling that with some refactoring
they can be made a bit cleaner (I did not look a the
changes in too much detail) but overall I think they
look ok.
Note you may also want to look at using the runtime
suspend framework for this, doing a pm_runtime_get_sync()
in open() and then letting (runtime) suspend do the power
off if you set a reasonable timeout for autosuspend after
the last user is gone then that will also avoid an
unnecessary suspend / resume cycle between probe()
exiting and the first open() call and this avoids the
need to do a poweroff() at the end of probe(), the
runtime-pm framework will autosuspend the device after
the timeout expires.
Regards,
Hans
> And the device must be available earlier during the
> course of probe to query some parameters through i2c:
>
> +static int elan_reactivate(struct elan_tp_data *data)
> +{
> + struct device *dev = &data->client->dev;
> + int ret;
> +
> + ret = elan_enable_power(data);
> + if (ret)
> + dev_err(dev, "failed to restore power: %d\n", ret);
> +
> + ret = elan_initialize(data);
> + if (ret)
> + dev_err(dev, "failed to re-initialize touchpad: %d\n", ret);
> +
> + return ret;
> +}
> +
> +static int elan_open(struct input_dev *input)
> +{
> + struct elan_tp_data *data = input_get_drvdata(input);
> + struct i2c_client *client = data->client;
> + int ret;
> +
> + dev_dbg(&client->dev, "uninhibiting\n");
> +
> + ret = mutex_lock_interruptible(&data->sysfs_mutex);
> + if (ret)
> + return ret;
> +
> + ret = elan_reactivate(data);
> + if (ret == 0)
> + enable_irq(client->irq);
> +
> + mutex_unlock(&data->sysfs_mutex);
> +
> + return ret;
> +}
> +
> +static int elan_inhibit(struct input_dev *input)
> +{
> + struct elan_tp_data *data = input_get_drvdata(input);
> + struct i2c_client *client = data->client;
> + int ret;
> +
> + dev_dbg(&client->dev, "closing\n");
> +
> + /*
> + * We are taking the mutex to make sure sysfs operations are
> + * complete before we attempt to bring the device into low[er]
> + * power mode.
> + */
> + ret = mutex_lock_interruptible(&data->sysfs_mutex);
> + if (ret)
> + return ret;
> +
> + disable_irq(client->irq);
> +
> + ret = elan_disable_power(data);
> + if (ret)
> + enable_irq(client->irq);
> +
> + mutex_unlock(&data->sysfs_mutex);
> +
> + return ret;
> +}
> +
> +static void elan_close(struct input_dev *input)
> +{
> + elan_inhibit(input);
> +}
> +
> static int elan_query_device_info(struct elan_tp_data *data)
> {
> int error;
> u16 ic_type;
>
> error = data->ops->get_version(data->client, false, &data->fw_version);
> if (error)
> return error;
>
> error = data->ops->get_checksum(data->client, false,
> &data->fw_checksum);
> if (error)
> return error;
>
> error = data->ops->get_version(data->client, true, &data->iap_version);
> if (error)
> return error;
> @@ -1071,34 +1141,36 @@ static int elan_setup_trackpoint_input_device(struct elan_tp_data *data)
>
> static int elan_setup_input_device(struct elan_tp_data *data)
> {
> struct device *dev = &data->client->dev;
> struct input_dev *input;
> unsigned int max_width = max(data->width_x, data->width_y);
> unsigned int min_width = min(data->width_x, data->width_y);
> int error;
>
> input = devm_input_allocate_device(dev);
> if (!input)
> return -ENOMEM;
>
> input->name = "Elan Touchpad";
> input->id.bustype = BUS_I2C;
> input->id.vendor = ELAN_VENDOR_ID;
> input->id.product = data->product_id;
> + input->open = elan_open;
> + input->close = elan_close;
> input_set_drvdata(input, data);
>
> error = input_mt_init_slots(input, ETP_MAX_FINGERS,
> INPUT_MT_POINTER | INPUT_MT_DROP_UNUSED);
> if (error) {
> dev_err(dev, "failed to initialize MT slots: %d\n", error);
> return error;
> }
>
> __set_bit(EV_ABS, input->evbit);
> __set_bit(INPUT_PROP_POINTER, input->propbit);
> if (data->clickpad) {
> __set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
> } else {
> __set_bit(BTN_RIGHT, input->keybit);
> if (data->middle_button)
> __set_bit(BTN_MIDDLE, input->keybit);
> @@ -1253,34 +1325,40 @@ static int elan_probe(struct i2c_client *client,
> if (!irqflags)
> irqflags = IRQF_TRIGGER_FALLING;
>
> error = devm_request_threaded_irq(dev, client->irq, NULL, elan_isr,
> irqflags | IRQF_ONESHOT,
> client->name, data);
> if (error) {
> dev_err(dev, "cannot register irq=%d\n", client->irq);
> return error;
> }
>
> error = devm_device_add_groups(dev, elan_sysfs_groups);
> if (error) {
> dev_err(dev, "failed to create sysfs attributes: %d\n", error);
> return error;
> }
>
> + error = elan_inhibit(data->input);
> + if (error) {
> + dev_err(dev, "failed to inhibit input device before registering: %d\n", error);
> + return error;
> + }
> +
> error = input_register_device(data->input);
> if (error) {
> dev_err(dev, "failed to register input device: %d\n", error);
> return error;
> }
>
> if (data->tp_input) {
> error = input_register_device(data->tp_input);
> if (error) {
> dev_err(&client->dev,
> "failed to register TrackPoint input device: %d\n",
> error);
> return error;
> }
> }
>
> /*
> @@ -1294,72 +1372,71 @@ static int elan_probe(struct i2c_client *client,
> }
>
> static int __maybe_unused elan_suspend(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
> struct elan_tp_data *data = i2c_get_clientdata(client);
> int ret;
>
> /*
> * We are taking the mutex to make sure sysfs operations are
> * complete before we attempt to bring the device into low[er]
> * power mode.
> */
> ret = mutex_lock_interruptible(&data->sysfs_mutex);
> if (ret)
> return ret;
>
> - disable_irq(client->irq);
> + mutex_lock(&data->input->mutex);
> + if (input_device_enabled(data->input)) {
> + disable_irq(client->irq);
>
> - if (device_may_wakeup(dev)) {
> - ret = elan_sleep(data);
> - /* Enable wake from IRQ */
> - data->irq_wake = (enable_irq_wake(client->irq) == 0);
> - } else {
> - ret = elan_disable_power(data);
> + if (device_may_wakeup(dev)) {
> + ret = elan_sleep(data);
> + /* Enable wake from IRQ */
> + data->irq_wake = (enable_irq_wake(client->irq) == 0);
> + } else {
> + ret = elan_disable_power(data);
> + }
> }
> + mutex_unlock(&data->input->mutex);
>
> mutex_unlock(&data->sysfs_mutex);
> return ret;
> }
>
> static int __maybe_unused elan_resume(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
> struct elan_tp_data *data = i2c_get_clientdata(client);
> - int error;
> + int ret = 0;
>
> - if (device_may_wakeup(dev) && data->irq_wake) {
> - disable_irq_wake(client->irq);
> - data->irq_wake = false;
> - }
> + mutex_lock(&data->input->mutex);
> + if (input_device_enabled(data->input)) {
> + if (data->irq_wake) {
> + disable_irq_wake(client->irq);
> + data->irq_wake = false;
> + }
>
> - error = elan_enable_power(data);
> - if (error) {
> - dev_err(dev, "power up when resuming failed: %d\n", error);
> - goto err;
> + ret = elan_reactivate(data);
> + enable_irq(data->client->irq);
> }
> + mutex_unlock(&data->input->mutex);
>
> - error = elan_initialize(data);
> - if (error)
> - dev_err(dev, "initialize when resuming failed: %d\n", error);
> -
> -err:
> - enable_irq(data->client->irq);
> - return error;
> + return ret;
> }
>
> Regards,
>
> Andrzej
>
^ permalink raw reply
* Re: [Nouveau] [PATCH] drm/nouveau: gr/gk20a: Use firmware version 0
From: Ben Skeggs @ 2020-06-03 19:50 UTC (permalink / raw)
To: Emil Velikov
Cc: Thierry Reding, linux-tegra-u79uwXL29TY76Z2rM5mHXA, ML nouveau,
Ben Skeggs, ML dri-devel
In-Reply-To: <CACvgo529m+fub=ZddGkjRXEY-7i3rRUs0EssYGf+DJgkcDm3Zg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thu, 4 Jun 2020 at 01:43, Emil Velikov <emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> On Wed, 3 Jun 2020 at 15:20, Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >
> > Tegra firmware doesn't actually use any version numbers and passing -1
> > causes the existing firmware binaries not to be found. Use version 0 to
> > find the correct files.
> >
> > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> Fixes: ef16dc278ec2 ("drm/nouveau/gr/gf100-: select implementation
> based on available FW")
> Reviewed-by: Emil Velikov <emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Oops, my bad. Merged.
Ben.
>
> -Emil
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply
* Re: [PATCHv2 0/7] Support inhibiting input devices
From: Dmitry Torokhov @ 2020-06-04 7:28 UTC (permalink / raw)
To: Hans de Goede
Cc: Andrzej Pietrasiewicz, linux-input, linux-acpi, linux-iio,
linux-arm-kernel, linux-samsung-soc, linux-tegra, patches,
ibm-acpi-devel, platform-driver-x86, Rafael J . Wysocki,
Len Brown, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Kukjin Kim, Krzysztof Kozlowski,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team
In-Reply-To: <01b902dd-8841-e697-5ba7-96fa6b73c1cd@redhat.com>
Hi Hans, Andrzej,
On Wed, Jun 03, 2020 at 09:37:10PM +0200, Hans de Goede wrote:
> Hi,
>
> On 6/3/20 7:54 PM, Andrzej Pietrasiewicz wrote:
> > W dniu 03.06.2020 o 19:38, Hans de Goede pisze:
> > > Hi,
> > >
> > > On 6/3/20 3:07 PM, Andrzej Pietrasiewicz wrote:
> > > > Hi Hans, hi Dmitry,
> > >
> > > <snip>
> > >
> > > > I'm taking one step back and looking at the ->open() and ->close()
> > > > driver callbacks. They are called from input_open_device() and
> > > > input_close_device(), respectively:
> > > >
> > > > input_open_device():
> > > > "This function should be called by input handlers when they
> > > > want to start receive events from given input device."
> > > >
> > > > ->open() callback:
> > > > "this method is called when the very first user calls
> > > > input_open_device(). The driver must prepare the device to start
> > > > generating events (start polling thread, request an IRQ, submit
> > > > URB, etc.)"
> > > >
> > > > input_close_device():
> > > > "This function should be called by input handlers when they
> > > > want to stop receive events from given input device."
> > > >
> > > > ->close() callback:
> > > > "this method is called when the very last user calls
> > > > input_close_device()"
> > > >
> > > > It seems to me that the callback names do not reflect their
> > > > purpose: their meaning is not to "open" or to "close" but to
> > > > give drivers a chance to control when they start or stop
> > > > providing events to the input core.
> > > >
> > > > What would you say about changing the callbacks' names?
> > > > I'd envsion: ->provide_events() instead of ->open() and
> > > > ->stop_events() instead of ->close(). Of course drivers can
> > > > exploit the fact of knowing that nobody wants any events
> > > > from them and do whatever they consider appropriate, for
> > > > example go into a low power mode - but the latter is beyond
> > > > the scope of the input subsystem and is driver-specific.
> > >
> > > I don't have much of an opinion on changing the names,
> > > to me open/close have always means start/stop receiving
> > > events. This follows the everything is a file philosophy,
> > > e.g. you can also not really "open" a serial port,
> > > yet opening /dev/ttyS0 will activate the receive IRQ
> > > of the UART, etc. So maybe we just need to make the
> > > docs clearer rather then do the rename? Doing the
> > > rename is certainly going to cause a lot of churn.
> >
> > Right, I can see now that the suggestion to change names is
> > too far fetched. (I feel that release() would be better
> > than close(), though). But it exposes the message I wanted to
> > pass.
release() usually means that the object is destroyedm, i.e this action,
unlike close() is irrevocable.
Let's leave the names as is, and adjust kerneldoc comments as needed.
> >
> > >
> > > Anyways as said, I don't have much of an opinion,
> > > so I'll leave commenting (more) on this to Dmitry.
> > >
> > > > With such a naming change in mind let's consider inhibiting.
> > > > We want to be able to control when to disregard events from
> > > > a given device. It makes sense to do it at device level, otherwise
> > > > such an operation would have to be invoked in all associated
> > > > handlers (those that have an open handle associating them with
> > > > the device in question). But of course we can do better than
> > > > merely ignoring the events received: we can tell the drivers
> > > > that we don't want any events from them, and later, at uninhibit
> > > > time, tell them to start providing the events again. Conceptually,
> > > > the two operations (provide or don't provide envents) are exactly
> > > > the same thing we want to be happening at input_open_device() and
> > > > input_close_device() time. To me, changing the names of
> > > > ->open() and ->close() exposes this fact very well.
> > > >
> > > > Consequently, ->inhibit() and ->uninhibit() won't be needed,
> > > > and drivers which already implement ->provide_events() (formerly
> > > > ->open()) and ->stop_events() (formerly ->close()) will receive
> > > > full inhibit/uninhibit support for free (subject to how well they
> > > > implement ->provide_events()/->stop_events()). Unless we can come
> > > > up with what the drivers might be doing on top of ->stop_events()
> > > > and ->provide_events() when inhibiting/uninhibiting, but it seems
> > > > to me we can't. Can we?
> > >
> > > Right. I'm happy that you've come to see that both on open/close
> > > and on inhibit/uninhibit we want to "start receiving events" and
> > > "stop receiving events", so that we only need one set of callbacks.
> > >
> >
> > Yeah, that's my conclusion - at least on a conceptual level.
> >
> > That said, what I can imagine is an existing driver (e.g. elan_i2c)
> > which does not implement neither open() nor close(), but does have
> > suspend() and resume(). Then it is maybe a bit easier to add inhibit()
> > and uninhibit() /they would be similar to suspend and resume/ instead
> > of open() and close(): If only open() and close() are possible, then
> > the probe function needs to be extended to "close" the device before it
> > gets registered, because from the moment it is registered it might be
> > opened right away.
>
> The probe only needs to "close" it if for some reason it
> starts directly sending events in most cases the driver
> must actively do something to get it to send events.
>
> So in most cases this should be pretty straight forward,
> as for having to do some init / power-on during probe
> and then power-off at the end of the probe. Yes sometimes
> something like that might be necessary.
>
> Looking at your suggested elan_i2c changes I think they
> look fine. I have the feeling that with some refactoring
> they can be made a bit cleaner (I did not look a the
> changes in too much detail) but overall I think they
> look ok.
>
> Note you may also want to look at using the runtime
> suspend framework for this, doing a pm_runtime_get_sync()
> in open() and then letting (runtime) suspend do the power
> off if you set a reasonable timeout for autosuspend after
> the last user is gone then that will also avoid an
> unnecessary suspend / resume cycle between probe()
> exiting and the first open() call and this avoids the
> need to do a poweroff() at the end of probe(), the
> runtime-pm framework will autosuspend the device after
> the timeout expires.
Yes, plugging into runtime PM would be nice, as as it currently stands
the driver will be broken with regard to trying access sysfs for
firmware update/calibration/etc if device happens to be inhibited.
The version of the driver in Chrome OS tree is similarly broken, but
because we control both kernel and the rest of the stack we know that we
do not poke at sysfs when device is inhibited. It will not be acceptable
for mainline (and that is one of reasons why elan_i2c does not have
open/close methods at the moment).
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH V3 2/8] usb: gadget: tegra-xudc: Add vbus_draw support
From: Thierry Reding @ 2020-06-04 13:54 UTC (permalink / raw)
To: Nagarjuna Kristam
Cc: balbi-DgEjT+Ai2ygdnm+yROfE0A,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
jonathanh-DDmLM1+adcrQT0dZR+AlfA, mark.rutland-5wv7dgnIgG8,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, kishon-l0cyMroinI0,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1589437363-16727-3-git-send-email-nkristam-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 551 bytes --]
On Thu, May 14, 2020 at 11:52:37AM +0530, Nagarjuna Kristam wrote:
> Register vbus_draw to gadget ops and update corresponding vbus
> draw current to usb_phy.
>
> Signed-off-by: Nagarjuna Kristam <nkristam-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> V3:
> - Propogated usb_phy->set_power error to vbus_draw caller.
> ---
> V2:
> - Patch re-based.
> ---
> drivers/usb/gadget/udc/tegra-xudc.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
Acked-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH V3 5/8] phy: tegra: xusb: Add soc ops API to enable UTMI PAD protection
From: Thierry Reding @ 2020-06-04 13:55 UTC (permalink / raw)
To: Nagarjuna Kristam
Cc: balbi, gregkh, jonathanh, mark.rutland, robh+dt, kishon,
devicetree, linux-tegra, linux-usb, linux-kernel
In-Reply-To: <1589437363-16727-6-git-send-email-nkristam@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 771 bytes --]
On Thu, May 14, 2020 at 11:52:40AM +0530, Nagarjuna Kristam wrote:
> When USB charger is enabled, UTMI PAD needs to be protected according
> to the direction and current level. Add support for the same on Tegra210
> and Tegra186.
>
> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> ---
> V3:
> - Alligned function and its arguments.
> - Fixed other comments from Thierry.
> ---
> V2:
> - Commit message coorected.
> - Patch re-based.
> ---
> drivers/phy/tegra/xusb-tegra186.c | 40 +++++++++++++++++++++++++++++++++++++++
> drivers/phy/tegra/xusb-tegra210.c | 32 +++++++++++++++++++++++++++++++
> drivers/phy/tegra/xusb.h | 13 +++++++++++++
> 3 files changed, 85 insertions(+)
Acked-by: Thierry Reding <treding@nvidia.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH V3 6/8] phy: tegra: xusb: Add support for charger detect
From: Thierry Reding @ 2020-06-04 13:56 UTC (permalink / raw)
To: Nagarjuna Kristam
Cc: balbi-DgEjT+Ai2ygdnm+yROfE0A,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
jonathanh-DDmLM1+adcrQT0dZR+AlfA, mark.rutland-5wv7dgnIgG8,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, kishon-l0cyMroinI0,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1589437363-16727-7-git-send-email-nkristam-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]
On Thu, May 14, 2020 at 11:52:41AM +0530, Nagarjuna Kristam wrote:
> Perform charger-detect operation if corresponding dt property is enabled.
> Update usb-phy with the detected charger state and max current values.
> Register charger-detect API's of usb-phy to provide needed functionalities.
>
> Signed-off-by: Nagarjuna Kristam <nkristam-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> V3:
> - Allighed functions and its arguments.
> - replaced spaced by tabs for MACRO definition allignments.
> - Unified primary and secondary charger detect API's.
> - Used readl_poll_timeout instead of while loop condition check for register.
> - Fixed other comments as per inputs from Thierry.
> ---
> V2:
> - Patch re-based.
> ---
> drivers/phy/tegra/Makefile | 2 +-
> drivers/phy/tegra/cd.c | 283 +++++++++++++++++++++++++++++++++++++++++++++
> drivers/phy/tegra/xusb.c | 80 +++++++++++++
> drivers/phy/tegra/xusb.h | 7 ++
> 4 files changed, 371 insertions(+), 1 deletion(-)
> create mode 100644 drivers/phy/tegra/cd.c
Acked-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH V3 5/8] phy: tegra: xusb: Add soc ops API to enable UTMI PAD protection
From: Thierry Reding @ 2020-06-04 13:58 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: Nagarjuna Kristam, balbi-DgEjT+Ai2ygdnm+yROfE0A,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
jonathanh-DDmLM1+adcrQT0dZR+AlfA, mark.rutland-5wv7dgnIgG8,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <db698a53-c5f6-d03f-edf0-f4fb38963e1f-l0cyMroinI0@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 623 bytes --]
On Mon, May 18, 2020 at 05:54:03PM +0530, Kishon Vijay Abraham I wrote:
> Thierry,
>
> On 5/14/2020 11:52 AM, Nagarjuna Kristam wrote:
> > When USB charger is enabled, UTMI PAD needs to be protected according
> > to the direction and current level. Add support for the same on Tegra210
> > and Tegra186.
> >
> > Signed-off-by: Nagarjuna Kristam <nkristam-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> Can you give your Acked by for pending patches in this series?
I went through these patches one more time and I think they're all good
now. I've acked the ones that didn't have my Acked-by yet.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] dmaengine: tegra210-adma: fix pm_runtime_get_sync failure
From: Jon Hunter @ 2020-06-04 17:43 UTC (permalink / raw)
To: Navid Emamdoost, Laxman Dewangan, Dan Williams, Vinod Koul,
Thierry Reding, dmaengine-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: emamd001-OJFnDUYgAso, wu000273-OJFnDUYgAso, kjlu-OJFnDUYgAso,
smccaman-OJFnDUYgAso
In-Reply-To: <20200603183845.91054-1-navid.emamdoost-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 03/06/2020 19:38, Navid Emamdoost wrote:
> Calling pm_runtime_get_sync increments the counter even in case of
> failure, causing incorrect ref count. Call pm_runtime_put if
> pm_runtime_get_sync fails.
>
> Signed-off-by: Navid Emamdoost <navid.emamdoost-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> drivers/dma/tegra210-adma.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/tegra210-adma.c b/drivers/dma/tegra210-adma.c
> index c4ce5dfb149b..87f2a1bed3aa 100644
> --- a/drivers/dma/tegra210-adma.c
> +++ b/drivers/dma/tegra210-adma.c
> @@ -869,8 +869,10 @@ static int tegra_adma_probe(struct platform_device *pdev)
> pm_runtime_enable(&pdev->dev);
>
> ret = pm_runtime_get_sync(&pdev->dev);
> - if (ret < 0)
> + if (ret < 0) {
> + pm_runtime_put_sync(&pdev->dev);
> goto rpm_disable;
> + }
I would prefer it if you did not add the pm_runtime_put_sync() call here
because there is already one in the error path that can be used.
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH] dmaengine: tegra210-adma: fix pm_runtime_get_sync failure
From: Jon Hunter @ 2020-06-04 17:45 UTC (permalink / raw)
To: Navid Emamdoost, Laxman Dewangan, Vinod Koul, Dan Williams,
Thierry Reding, dmaengine-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: emamd001-OJFnDUYgAso, wu000273-OJFnDUYgAso, kjlu-OJFnDUYgAso,
smccaman-OJFnDUYgAso
In-Reply-To: <20200603184104.4475-1-navid.emamdoost-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 03/06/2020 19:41, Navid Emamdoost wrote:
> Calling pm_runtime_get_sync increments the counter even in case of
> failure, causing incorrect ref count. Call pm_runtime_put if
> pm_runtime_get_sync fails.
>
> Signed-off-by: Navid Emamdoost <navid.emamdoost-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> drivers/dma/tegra210-adma.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/dma/tegra210-adma.c b/drivers/dma/tegra210-adma.c
> index c4ce5dfb149b..e8c749cd3fe8 100644
> --- a/drivers/dma/tegra210-adma.c
> +++ b/drivers/dma/tegra210-adma.c
> @@ -659,6 +659,7 @@ static int tegra_adma_alloc_chan_resources(struct dma_chan *dc)
> ret = pm_runtime_get_sync(tdc2dev(tdc));
> if (ret < 0) {
> free_irq(tdc->irq, tdc);
> + pm_runtime_put(tdc2dev(tdc));
> return ret;
> }
Please do not send two patches with the same $subject that are fixing
two different areas of the driver. In fact, please squash these two
patches into a single fix and resend because they are fixing the same issue.
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH] dmaengine: tegra210-adma: fix pm_runtime_get_sync failure
From: Navid Emamdoost @ 2020-06-04 17:48 UTC (permalink / raw)
To: Jon Hunter
Cc: Laxman Dewangan, Vinod Koul, Dan Williams, Thierry Reding,
dmaengine-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, LKML, Navid Emamdoost,
Qiushi Wu, Kangjie Lu, Stephen McCamant
In-Reply-To: <900909fe-fa15-fbca-80f7-79aeee721ed9-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On Thu, Jun 4, 2020 at 12:45 PM Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>
>
> On 03/06/2020 19:41, Navid Emamdoost wrote:
> > Calling pm_runtime_get_sync increments the counter even in case of
> > failure, causing incorrect ref count. Call pm_runtime_put if
> > pm_runtime_get_sync fails.
> >
> > Signed-off-by: Navid Emamdoost <navid.emamdoost-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> > drivers/dma/tegra210-adma.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/dma/tegra210-adma.c b/drivers/dma/tegra210-adma.c
> > index c4ce5dfb149b..e8c749cd3fe8 100644
> > --- a/drivers/dma/tegra210-adma.c
> > +++ b/drivers/dma/tegra210-adma.c
> > @@ -659,6 +659,7 @@ static int tegra_adma_alloc_chan_resources(struct dma_chan *dc)
> > ret = pm_runtime_get_sync(tdc2dev(tdc));
> > if (ret < 0) {
> > free_irq(tdc->irq, tdc);
> > + pm_runtime_put(tdc2dev(tdc));
> > return ret;
> > }
>
>
> Please do not send two patches with the same $subject that are fixing
> two different areas of the driver. In fact, please squash these two
> patches into a single fix and resend because they are fixing the same issue.
Sure, I will prepare a version 2 with your suggestions.
>
> Jon
>
> --
> nvpublic
--
Navid.
^ permalink raw reply
* [PATCH] pinctrl: tegra: Use noirq suspend/resume callbacks
From: Vidya Sagar @ 2020-06-04 17:49 UTC (permalink / raw)
To: linus.walleij-QSEj5FYQhm4dnm+yROfE0A,
treding-DDmLM1+adcrQT0dZR+AlfA, jonathanh-DDmLM1+adcrQT0dZR+AlfA,
digetx-Re5JQEeQqe8AvxtiuMwx3w, skomatineni-DDmLM1+adcrQT0dZR+AlfA
Cc: linux-gpio-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
kthota-DDmLM1+adcrQT0dZR+AlfA, mmaddireddy-DDmLM1+adcrQT0dZR+AlfA,
vidyas-DDmLM1+adcrQT0dZR+AlfA, sagar.tv-Re5JQEeQqe8AvxtiuMwx3w
Use noirq suspend/resume callbacks as other drivers which implement
noirq suspend/resume callbacks (Ex:- PCIe) depend on pinctrl driver to
configure the signals used by their respective devices in the noirq phase.
Signed-off-by: Vidya Sagar <vidyas-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
drivers/pinctrl/tegra/pinctrl-tegra.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
index 21661f6490d6..195cfe557511 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra.c
+++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
@@ -731,8 +731,8 @@ static int tegra_pinctrl_resume(struct device *dev)
}
const struct dev_pm_ops tegra_pinctrl_pm = {
- .suspend = &tegra_pinctrl_suspend,
- .resume = &tegra_pinctrl_resume
+ .suspend_noirq = &tegra_pinctrl_suspend,
+ .resume_noirq = &tegra_pinctrl_resume
};
static bool tegra_pinctrl_gpio_node_has_range(struct tegra_pmx *pmx)
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] pinctrl: tegra: Use noirq suspend/resume callbacks
From: Dmitry Osipenko @ 2020-06-04 18:00 UTC (permalink / raw)
To: Vidya Sagar, linus.walleij-QSEj5FYQhm4dnm+yROfE0A,
treding-DDmLM1+adcrQT0dZR+AlfA, jonathanh-DDmLM1+adcrQT0dZR+AlfA,
skomatineni-DDmLM1+adcrQT0dZR+AlfA
Cc: linux-gpio-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
kthota-DDmLM1+adcrQT0dZR+AlfA, mmaddireddy-DDmLM1+adcrQT0dZR+AlfA,
sagar.tv-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <20200604174935.26560-1-vidyas-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
04.06.2020 20:49, Vidya Sagar пишет:
> Use noirq suspend/resume callbacks as other drivers which implement
> noirq suspend/resume callbacks (Ex:- PCIe) depend on pinctrl driver to
> configure the signals used by their respective devices in the noirq phase.
>
> Signed-off-by: Vidya Sagar <vidyas-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> drivers/pinctrl/tegra/pinctrl-tegra.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
> index 21661f6490d6..195cfe557511 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
> @@ -731,8 +731,8 @@ static int tegra_pinctrl_resume(struct device *dev)
> }
>
> const struct dev_pm_ops tegra_pinctrl_pm = {
> - .suspend = &tegra_pinctrl_suspend,
> - .resume = &tegra_pinctrl_resume
> + .suspend_noirq = &tegra_pinctrl_suspend,
> + .resume_noirq = &tegra_pinctrl_resume
> };
>
> static bool tegra_pinctrl_gpio_node_has_range(struct tegra_pmx *pmx)
>
That's a good catch! Perhaps I2C on later Tegra SoCs also should suffer
similarly to the PCIe.
Reviewed-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox