* Re: AMD IOMMU + SME + amdgpu regression
From: Joerg Roedel @ 2020-06-22 10:02 UTC (permalink / raw)
To: Alex Xu (Hello71)
Cc: Heiko Stuebner, Bjorn Andersson, linux-tegra, Thierry Reding,
Daniel Drake, Will Deacon, Christoph Hellwig, Marek Szyprowski,
Jean-Philippe Brucker, linux-samsung-soc, Joerg Roedel,
Krzysztof Kozlowski, Jonathan Hunter, linux-rockchip, Kukjin Kim,
Andy Gross, David Rientjes, linux-s390, Gerald Schaefer,
linux-arm-msm, linux-mediatek, Matthias Brugger, virtualization,
jonathan.derrick
In-Reply-To: <1591915710.rakbpzst8h.none@localhost>
Hi Alex,
On Thu, Jun 11, 2020 at 07:05:21PM -0400, Alex Xu (Hello71) wrote:
> I am using an ASRock B450 Pro4 with Ryzen 1600 and ASUS RX 480. I don't
> understand this code at all, but let me know what I can do to
> troubleshoot.
Does it boot without SME enabled?
Regards,
Joerg
^ permalink raw reply
* Re: [PATCH AUTOSEL 5.7 004/388] ASoC: tegra: tegra_wm8903: Support nvidia, headset property
From: Mark Brown @ 2020-06-22 11:23 UTC (permalink / raw)
To: Sasha Levin
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
stable-u79uwXL29TY76Z2rM5mHXA, Dmitry Osipenko,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20200621233352.GA1931@sasha-vm>
[-- Attachment #1: Type: text/plain, Size: 633 bytes --]
On Sun, Jun 21, 2020 at 07:33:52PM -0400, Sasha Levin wrote:
> On Thu, Jun 18, 2020 at 03:39:30PM +0100, Mark Brown wrote:
> > On Thu, Jun 18, 2020 at 10:30:46AM -0400, Sasha Levin wrote:
> > > On Thu, Jun 18, 2020 at 12:00:23PM +0100, Mark Brown wrote:
> > > > This is a new feature not a bugfix.
> > > I saw this patch more as a hardware quirk.
> > Pretty much any DT property is a hardware quirk :(
> Which is why we're taking most of them :)
That's concerning - please don't do this. It's not what stable is
expected to be and there's no guarantee that you're getting all the
changes required to actually make things work.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH AUTOSEL 5.7 004/388] ASoC: tegra: tegra_wm8903: Support nvidia, headset property
From: Sasha Levin @ 2020-06-22 12:31 UTC (permalink / raw)
To: Mark Brown
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
stable-u79uwXL29TY76Z2rM5mHXA, Dmitry Osipenko,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20200622112321.GB4560-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
On Mon, Jun 22, 2020 at 12:23:21PM +0100, Mark Brown wrote:
>On Sun, Jun 21, 2020 at 07:33:52PM -0400, Sasha Levin wrote:
>> On Thu, Jun 18, 2020 at 03:39:30PM +0100, Mark Brown wrote:
>> > On Thu, Jun 18, 2020 at 10:30:46AM -0400, Sasha Levin wrote:
>> > > On Thu, Jun 18, 2020 at 12:00:23PM +0100, Mark Brown wrote:
>
>> > > > This is a new feature not a bugfix.
>
>> > > I saw this patch more as a hardware quirk.
>
>> > Pretty much any DT property is a hardware quirk :(
>
>> Which is why we're taking most of them :)
>
>That's concerning - please don't do this. It's not what stable is
>expected to be and there's no guarantee that you're getting all the
>changes required to actually make things work.
How come? This is one of the things stable rules explicitly call for:
"New device IDs and quirks are also accepted".
If we're missing anything, the solution is to make sure we stop missing
it rather than not take anything to begin with :)
--
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH AUTOSEL 5.7 004/388] ASoC: tegra: tegra_wm8903: Support nvidia, headset property
From: Mark Brown @ 2020-06-22 13:27 UTC (permalink / raw)
To: Sasha Levin
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
stable-u79uwXL29TY76Z2rM5mHXA, Dmitry Osipenko,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20200622123118.GF1931@sasha-vm>
[-- Attachment #1: Type: text/plain, Size: 1039 bytes --]
On Mon, Jun 22, 2020 at 08:31:18AM -0400, Sasha Levin wrote:
> On Mon, Jun 22, 2020 at 12:23:21PM +0100, Mark Brown wrote:
> > That's concerning - please don't do this. It's not what stable is
> > expected to be and there's no guarantee that you're getting all the
> > changes required to actually make things work.
> How come? This is one of the things stable rules explicitly call for:
> "New device IDs and quirks are also accepted".
I would expect that to be data only additions, I would not expect that
to be adding new code.
> If we're missing anything, the solution is to make sure we stop missing
> it rather than not take anything to begin with :)
It would be much better to not have to watch stable constantly like we
currently do - we're seeing people report breakage often enough to be a
concern as things are, we don't need to be trying to pile extra stuff in
there because there's some keywords in a changelog or whatever. The
testing coverage for drivers is weak, increasing the change rate puts
more stress on that.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v1 1/2] drm/panel-simple: Correct EDT ET057090DHU connector type
From: Dmitry Osipenko @ 2020-06-22 13:40 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Thierry Reding, Sam Ravnborg,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20200621222922.GA25355-N3hz7ZxfLydczECFQUw77jytWr6r+dGw0E9HWUfgJXw@public.gmane.org>
22.06.2020 01:29, Laurent Pinchart пишет:
> Hi Dmitry,
>
> Thank you for the patch.
>
> On Mon, Jun 22, 2020 at 01:27:41AM +0300, Dmitry Osipenko wrote:
>> The EDT ET057090DHU panel has a DPI connector and not LVDS. This patch
>> corrects the panel's description.
>>
>> Reported-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>> Fixes: 94f07917ebe8 ("drm/panel-simple: Add missing connector type for some panels")
>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
Thank you! :)
^ permalink raw reply
* Re: [PATCH AUTOSEL 5.7 004/388] ASoC: tegra: tegra_wm8903: Support nvidia, headset property
From: Sasha Levin @ 2020-06-22 14:44 UTC (permalink / raw)
To: Mark Brown; +Cc: linux-kernel, stable, Dmitry Osipenko, alsa-devel, linux-tegra
In-Reply-To: <20200622132757.GG4560@sirena.org.uk>
On Mon, Jun 22, 2020 at 02:27:57PM +0100, Mark Brown wrote:
>On Mon, Jun 22, 2020 at 08:31:18AM -0400, Sasha Levin wrote:
>> On Mon, Jun 22, 2020 at 12:23:21PM +0100, Mark Brown wrote:
>
>> > That's concerning - please don't do this. It's not what stable is
>> > expected to be and there's no guarantee that you're getting all the
>> > changes required to actually make things work.
>
>> How come? This is one of the things stable rules explicitly call for:
>> "New device IDs and quirks are also accepted".
>
>I would expect that to be data only additions, I would not expect that
>to be adding new code.
These come hand in hand. Take a look at the more complex cases such as
sound/pci/hda/patch_*
>> If we're missing anything, the solution is to make sure we stop missing
>> it rather than not take anything to begin with :)
>
>It would be much better to not have to watch stable constantly like we
>currently do - we're seeing people report breakage often enough to be a
>concern as things are, we don't need to be trying to pile extra stuff in
>there because there's some keywords in a changelog or whatever. The
>testing coverage for drivers is weak, increasing the change rate puts
>more stress on that.
Shouldn't we instead improve testing here? nvidia for example already
provides Tegra testing for stable releases, if the coverage isn't
sufficient then let's work on making it better.
--
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH 09/73] ARM: tegra: gr2d is not backwards-compatible
From: Thierry Reding @ 2020-06-22 15:22 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jon Hunter
In-Reply-To: <9f56be9a-1a79-07cc-371f-f3abcd20701e-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1720 bytes --]
On Wed, Jun 17, 2020 at 07:21:30PM +0300, Dmitry Osipenko wrote:
> 16.06.2020 16:51, Thierry Reding пишет:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >
> > The instantiation of gr2d in Tegra114 is not backwards-compatible with
> > the version found on earlier chips. Remove the misleading compatible
> > string.
> >
> > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> > arch/arm/boot/dts/tegra114.dtsi | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
> > index a0ac9ea9ec9d..d583dfba688f 100644
> > --- a/arch/arm/boot/dts/tegra114.dtsi
> > +++ b/arch/arm/boot/dts/tegra114.dtsi
> > @@ -35,7 +35,7 @@ host1x@50000000 {
> > ranges = <0x54000000 0x54000000 0x01000000>;
> >
> > gr2d@54140000 {
> > - compatible = "nvidia,tegra114-gr2d", "nvidia,tegra20-gr2d";
> > + compatible = "nvidia,tegra114-gr2d";
> > reg = <0x54140000 0x00040000>;
> > interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> > clocks = <&tegra_car TEGRA114_CLK_GR2D>;
> >
>
> Could you please explain what's the difference? AFAIK, the 2D HW is
> identical on T20/30/114.
My understanding is that the IP core itself is identical. However, the
compatible string really describes the integration of the IP, which in
case of Tegra114 is slightly different in that it's part of the HEG
power partition, whereas it wasn't previously.
That means that without knowledge of the power partition we won't be
able to access the gr2d IP on Tegra114 at all and hence it isn't
backwards compatible with Tegra20.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 09/73] ARM: tegra: gr2d is not backwards-compatible
From: Dmitry Osipenko @ 2020-06-22 15:29 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jon Hunter
In-Reply-To: <20200622152209.GA4008275@ulmo>
22.06.2020 18:22, Thierry Reding пишет:
> On Wed, Jun 17, 2020 at 07:21:30PM +0300, Dmitry Osipenko wrote:
>> 16.06.2020 16:51, Thierry Reding пишет:
>>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> The instantiation of gr2d in Tegra114 is not backwards-compatible with
>>> the version found on earlier chips. Remove the misleading compatible
>>> string.
>>>
>>> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>> arch/arm/boot/dts/tegra114.dtsi | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
>>> index a0ac9ea9ec9d..d583dfba688f 100644
>>> --- a/arch/arm/boot/dts/tegra114.dtsi
>>> +++ b/arch/arm/boot/dts/tegra114.dtsi
>>> @@ -35,7 +35,7 @@ host1x@50000000 {
>>> ranges = <0x54000000 0x54000000 0x01000000>;
>>>
>>> gr2d@54140000 {
>>> - compatible = "nvidia,tegra114-gr2d", "nvidia,tegra20-gr2d";
>>> + compatible = "nvidia,tegra114-gr2d";
>>> reg = <0x54140000 0x00040000>;
>>> interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
>>> clocks = <&tegra_car TEGRA114_CLK_GR2D>;
>>>
>>
>> Could you please explain what's the difference? AFAIK, the 2D HW is
>> identical on T20/30/114.
>
> My understanding is that the IP core itself is identical. However, the
> compatible string really describes the integration of the IP, which in
> case of Tegra114 is slightly different in that it's part of the HEG
> power partition, whereas it wasn't previously.
>
> That means that without knowledge of the power partition we won't be
> able to access the gr2d IP on Tegra114 at all and hence it isn't
> backwards compatible with Tegra20.
Thank you very much for the clarification! Will nice if this could be
added into the commit's description.
^ permalink raw reply
* Re: AMD IOMMU + SME + amdgpu regression
From: Alex Xu (Hello71) @ 2020-06-22 15:30 UTC (permalink / raw)
To: Joerg Roedel
Cc: Andy Gross, Lu Baolu, Bjorn Andersson, Daniel Drake,
David Woodhouse, Gerald Schaefer, Christoph Hellwig,
Heiko Stuebner, Jean-Philippe Brucker,
jonathan.derrick-ral2JQCrhuEAvxtiuMwx3w, Jonathan Hunter,
Joerg Roedel, Kukjin Kim, Krzysztof Kozlowski,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-s390-u79uwXL29TY76Z2rM5mHXA,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Matthias Brugger <matt>
In-Reply-To: <20200622100257.GD31822-l3A5Bk7waGM@public.gmane.org>
Excerpts from Joerg Roedel's message of June 22, 2020 6:02 am:
> Hi Alex,
>
> On Thu, Jun 11, 2020 at 07:05:21PM -0400, Alex Xu (Hello71) wrote:
>> I am using an ASRock B450 Pro4 with Ryzen 1600 and ASUS RX 480. I don't
>> understand this code at all, but let me know what I can do to
>> troubleshoot.
>
> Does it boot without SME enabled?
>
>
> Regards,
>
> Joerg
>
Yes, it works with SME off with dbed452a078 ("dma-pool: decouple
DMA_REMAP from DMA_COHERENT_POOL") applied.
^ permalink raw reply
* URGENT REPLY.
From: Karim Zakari @ 2020-06-22 16:02 UTC (permalink / raw)
In-Reply-To: <1507214802.1850985.1592841739314.ref@mail.yahoo.com>
Good-Day Friend,
Hope you are doing great Today. I have a proposed business deal worthy (US$16.5 Million Dollars) that will benefit both parties. This is legitimate' legal and your personality will not be compromised.
Waiting for your response for more details, As you are willing to execute this business opportunity with me.
Sincerely Yours,
Mr. Karim Zakari.
^ permalink raw reply
* Re: [PATCH AUTOSEL 5.7 004/388] ASoC: tegra: tegra_wm8903: Support nvidia, headset property
From: Mark Brown @ 2020-06-22 17:57 UTC (permalink / raw)
To: Sasha Levin
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
stable-u79uwXL29TY76Z2rM5mHXA, Dmitry Osipenko,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20200622144402.GH1931@sasha-vm>
[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]
On Mon, Jun 22, 2020 at 10:44:02AM -0400, Sasha Levin wrote:
> On Mon, Jun 22, 2020 at 02:27:57PM +0100, Mark Brown wrote:
> > On Mon, Jun 22, 2020 at 08:31:18AM -0400, Sasha Levin wrote:
> > > How come? This is one of the things stable rules explicitly call for:
> > > "New device IDs and quirks are also accepted".
> > I would expect that to be data only additions, I would not expect that
> > to be adding new code.
> These come hand in hand. Take a look at the more complex cases such as
> sound/pci/hda/patch_*
There are more complex cases, I'm just not sure how good an idea
backporting them.
> > It would be much better to not have to watch stable constantly like we
> > currently do - we're seeing people report breakage often enough to be a
> > concern as things are, we don't need to be trying to pile extra stuff in
> > there because there's some keywords in a changelog or whatever. The
> > testing coverage for drivers is weak, increasing the change rate puts
> > more stress on that.
> Shouldn't we instead improve testing here? nvidia for example already
> provides Tegra testing for stable releases, if the coverage isn't
> sufficient then let's work on making it better.
Obviously it'd be good to improve the test coverage, but I think that's
something that needs doing before backporting loads of stuff rather than
after. For this automated stuff I'd much rather see positive
confirmation that the change had been tested on relevant systems (not
just something with a similar SoC), especially on the edges where we're
getting to board specific things.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] clk: tegra: Capitalization fixes
From: Stephen Boyd @ 2020-06-23 2:07 UTC (permalink / raw)
To: Thierry Reding
Cc: Michael Turquette, Jon Hunter, LABBE Corentin, Dmitry Osipenko,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20200603111923.3545261-1-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Quoting Thierry Reding (2020-06-03 04:19:22)
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> HW, XUSB and PLL are abbreviations and should be all-uppercase.
>
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
Acked-by: Stephen Boyd <sboyd-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
^ permalink raw reply
* Re: [PATCH 2/2] clk: tegra: Always program PLL_E when enabled
From: Stephen Boyd @ 2020-06-23 2:07 UTC (permalink / raw)
To: Thierry Reding
Cc: Michael Turquette, Jon Hunter, LABBE Corentin, Dmitry Osipenko,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20200603111923.3545261-2-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Quoting Thierry Reding (2020-06-03 04:19:23)
> 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>
> ---
Acked-by: Stephen Boyd <sboyd-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
^ permalink raw reply
* Re: [TEGRA194_CPUFREQ Patch v3 3/4] cpufreq: Add Tegra194 cpufreq driver
From: Sumit Gupta @ 2020-06-23 5:19 UTC (permalink / raw)
To: Viresh Kumar
Cc: rjw-LthD3rsA81gm4RdzfppkhA, catalin.marinas-5wv7dgnIgG8,
will-DgEjT+Ai2ygdnm+yROfE0A,
thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
jonathanh-DDmLM1+adcrQT0dZR+AlfA, talho-DDmLM1+adcrQT0dZR+AlfA,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, bbasu-DDmLM1+adcrQT0dZR+AlfA,
mperttunen-DDmLM1+adcrQT0dZR+AlfA, Sumit Gupta
In-Reply-To: <20200622072052.uryxo4hri6gzrkku@vireshk-i7>
Hi Viresh,
Thank you for the review. please find my reply inline.
>> +++ b/drivers/cpufreq/tegra194-cpufreq.c
>> @@ -0,0 +1,403 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved
>
> 2020
>
>> + */
>> +
>> +#include <linux/cpu.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/delay.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +
>> +#include <asm/smp_plat.h>
>> +
>> +#include <soc/tegra/bpmp.h>
>> +#include <soc/tegra/bpmp-abi.h>
>> +
>> +#define KHZ 1000
>> +#define REF_CLK_MHZ 408 /* 408 MHz */
>> +#define US_DELAY 500
>> +#define US_DELAY_MIN 2
>> +#define CPUFREQ_TBL_STEP_HZ (50 * KHZ * KHZ)
>> +#define MAX_CNT ~0U
>> +
>> +/* cpufreq transisition latency */
>> +#define TEGRA_CPUFREQ_TRANSITION_LATENCY (300 * 1000) /* unit in nanoseconds */
>> +
>> +#define LOOP_FOR_EACH_CPU_OF_CLUSTER(cl) for (cpu = (cl * 2); \
>> + cpu < ((cl + 1) * 2); cpu++)
>
> Both latency and this loop are used only once in the code, maybe just open code
> it. Also you should have passed cpu as a parameter to the macro, even if it
> works fine without it, for better readability.
>
Ok, i will open code the loop in next version. For latency value, i feel
named macro makes readability better. So, prefer keeping it.
>> +
>> +u16 map_freq_to_ndiv(struct mrq_cpu_ndiv_limits_response *nltbl, u32 freq)
>
> Unused routine
>
Sure, will remove it.
>> +{
>> + return DIV_ROUND_UP(freq * nltbl->pdiv * nltbl->mdiv,
>> + nltbl->ref_clk_hz / KHZ);
>> +}
>
>> +static int tegra194_cpufreq_init(struct cpufreq_policy *policy)
>> +{
>> + struct tegra194_cpufreq_data *data = cpufreq_get_driver_data();
>> + int cl = get_cpu_cluster(policy->cpu);
>> + u32 cpu;
>> +
>> + if (cl >= data->num_clusters)
>> + return -EINVAL;
>> +
>> + policy->cur = tegra194_fast_get_speed(policy->cpu); /* boot freq */
>> +
>> + /* set same policy for all cpus in a cluster */
>> + LOOP_FOR_EACH_CPU_OF_CLUSTER(cl)
>> + cpumask_set_cpu(cpu, policy->cpus);
>> +
>> + policy->freq_table = data->tables[cl];
>> + policy->cpuinfo.transition_latency = TEGRA_CPUFREQ_TRANSITION_LATENCY;
>> +
>> + return 0;
>> +}
>
>> +static int tegra194_cpufreq_set_target(struct cpufreq_policy *policy,
>> + unsigned int index)
>> +{
>> + struct cpufreq_frequency_table *tbl = policy->freq_table + index;
>> +
>> + on_each_cpu_mask(policy->cpus, set_cpu_ndiv, tbl, true);
>
> I am still a bit confused. While setting the frequency you are calling this
> routine for each CPU of the policy (cluster). Does that mean that CPUs within a
> cluster can actually run at different frequencies at any given point of time ?
>
> If cpufreq terms, a cpufreq policy represents a group of CPUs that change
> frequency together, i.e. they share the clk line. If all CPUs in your system can
> do DVFS separately, then you must have policy per CPU, instead of cluster.
>
T194 supports four CPU clusters, each with two cores. Each CPU cluster
is capable of running at a specific frequency sourced by respective
NAFLL to provide cluster specific clocks. Individual cores within a
cluster write freq in per core register. Cluster h/w forwards the
max(core0, core1) request to per cluster NAFLL.
>> +static void tegra194_cpufreq_free_resources(void)
>> +{
>> + flush_workqueue(read_counters_wq);
>
> Why is this required exactly? I see that you add the work request and
> immediately flush it, then why would you need to do this separately ?
>
Ya, will remove flush_workqueue().
>> + destroy_workqueue(read_counters_wq);
>> +}
>> +
>> +static struct cpufreq_frequency_table *
>> +init_freq_table(struct platform_device *pdev, struct tegra_bpmp *bpmp,
>> + unsigned int cluster_id)
>> +{
>> + struct cpufreq_frequency_table *freq_table;
>> + struct mrq_cpu_ndiv_limits_response resp;
>> + unsigned int num_freqs, ndiv, delta_ndiv;
>> + struct mrq_cpu_ndiv_limits_request req;
>> + struct tegra_bpmp_message msg;
>> + u16 freq_table_step_size;
>> + int err, index;
>> +
>> + memset(&req, 0, sizeof(req));
>> + req.cluster_id = cluster_id;
>> +
>> + memset(&msg, 0, sizeof(msg));
>> + msg.mrq = MRQ_CPU_NDIV_LIMITS;
>> + msg.tx.data = &req;
>> + msg.tx.size = sizeof(req);
>> + msg.rx.data = &resp;
>> + msg.rx.size = sizeof(resp);
>> +
>> + err = tegra_bpmp_transfer(bpmp, &msg);
>
> So the firmware can actually return different frequency tables for the clusters,
> right ? Else you could have received the table only once and used it for all the
> CPUs.
>
Yes, frequency tables are returned per cluster by BPMP firmware. In T194
SOC, currently same table values are used for all clusters. This might
change in future.
>> + if (err)
>> + return ERR_PTR(err);
>> +
>> + /*
>> + * Make sure frequency table step is a multiple of mdiv to match
>> + * vhint table granularity.
>> + */
>> + freq_table_step_size = resp.mdiv *
>> + DIV_ROUND_UP(CPUFREQ_TBL_STEP_HZ, resp.ref_clk_hz);
>> +
>> + dev_dbg(&pdev->dev, "cluster %d: frequency table step size: %d\n",
>> + cluster_id, freq_table_step_size);
>> +
>> + delta_ndiv = resp.ndiv_max - resp.ndiv_min;
>> +
>> + if (unlikely(delta_ndiv == 0))
>> + num_freqs = 1;
>> + else
>> + /* We store both ndiv_min and ndiv_max hence the +1 */
>> + num_freqs = delta_ndiv / freq_table_step_size + 1;
>> +
>> + num_freqs += (delta_ndiv % freq_table_step_size) ? 1 : 0;
>> +
>> + freq_table = devm_kcalloc(&pdev->dev, num_freqs + 1,
>> + sizeof(*freq_table), GFP_KERNEL);
>> + if (!freq_table)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + for (index = 0, ndiv = resp.ndiv_min;
>> + ndiv < resp.ndiv_max;
>> + index++, ndiv += freq_table_step_size) {
>> + freq_table[index].driver_data = ndiv;
>> + freq_table[index].frequency = map_ndiv_to_freq(&resp, ndiv);
>> + }
>> +
>> + freq_table[index].driver_data = resp.ndiv_max;
>> + freq_table[index++].frequency = map_ndiv_to_freq(&resp, resp.ndiv_max);
>> + freq_table[index].frequency = CPUFREQ_TABLE_END;
>> +
>> + return freq_table;
>> +}
>
> --
> viresh
>
^ permalink raw reply
* Re: [TEGRA194_CPUFREQ Patch v3 1/4] dt-bindings: arm: Add t194 ccplex compatible and bpmp property
From: Sumit Gupta @ 2020-06-23 6:05 UTC (permalink / raw)
To: Viresh Kumar
Cc: rjw-LthD3rsA81gm4RdzfppkhA, catalin.marinas-5wv7dgnIgG8,
will-DgEjT+Ai2ygdnm+yROfE0A,
thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
jonathanh-DDmLM1+adcrQT0dZR+AlfA, talho-DDmLM1+adcrQT0dZR+AlfA,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, bbasu-DDmLM1+adcrQT0dZR+AlfA,
mperttunen-DDmLM1+adcrQT0dZR+AlfA, Sumit Gupta
In-Reply-To: <20200622072247.agrvmw6sl3jwgjkz@vireshk-i7>
On 22/06/20 12:52 PM, Viresh Kumar wrote:
> External email: Use caution opening links or attachments
>
>
> On 22-06-20, 03:04, Sumit Gupta wrote:
>> To do frequency scaling on all CPUs within T194 CPU Complex, we need
>> to query BPMP for data on valid operating points. Document a compatible
>> string under 'cpus' node to represent the CPU Complex for binding drivers
>> like cpufreq which don't have their node or CPU Complex node to bind to.
>> Also, document a property to point to the BPMP device that can be queried
>> for all CPUs.
>
> You shouldn't be putting how linux is going to use this information and entries
> shouldn't be made just so cpufreq can bind to a driver.
>
> Though I see that this is a real hardware register which you can use to interact
> with the firmware ? And so it makes sense to have it, maybe in different form
> though.
>
CPUFREQ driver doesn't communicate directly with BPMP firmware. It uses
BPMP node's reference to call api's exported by BPMP driver which
communicates with BPMP firmware.
> I will let Rob explain what would be the right way of doing this though.
>
This is already discussed by Thierry with Rob.
Please refer https://marc.info/?l=linux-arm-kernel&m=158999171528418&w=2
>>
>> Signed-off-by: Sumit Gupta <sumitg-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>> Documentation/devicetree/bindings/arm/cpus.yaml | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
>> index a018147..737b55e 100644
>> --- a/Documentation/devicetree/bindings/arm/cpus.yaml
>> +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
>> @@ -162,6 +162,7 @@ properties:
>> - nvidia,tegra132-denver
>> - nvidia,tegra186-denver
>> - nvidia,tegra194-carmel
>> + - nvidia,tegra194-ccplex
>> - qcom,krait
>> - qcom,kryo
>> - qcom,kryo260
>> @@ -255,6 +256,14 @@ properties:
>>
>> where voltage is in V, frequency is in MHz.
>>
>> + nvidia,bpmp:
>> + $ref: '/schemas/types.yaml#/definitions/phandle'
>> + descrption: |
>> + Specifies the bpmp node that needs to be queried to get
>> + operating point data for all CPUs.
>> +
>> + Optional for NVIDIA Tegra194 Carmel CPUs
>> +
>> power-domains:
>> $ref: '/schemas/types.yaml#/definitions/phandle-array'
>> description:
>> --
>> 2.7.4
>
> --
> viresh
>
^ permalink raw reply
* Re: [TEGRA194_CPUFREQ Patch v3 3/4] cpufreq: Add Tegra194 cpufreq driver
From: Viresh Kumar @ 2020-06-23 6:20 UTC (permalink / raw)
To: Sumit Gupta
Cc: rjw, catalin.marinas, will, thierry.reding, robh+dt, devicetree,
jonathanh, talho, linux-pm, linux-tegra, linux-arm-kernel,
linux-kernel, bbasu, mperttunen
In-Reply-To: <ed6956a3-3f77-2943-6387-5affc25b59d2@nvidia.com>
On 23-06-20, 10:49, Sumit Gupta wrote:
> Hi Viresh,
>
> Thank you for the review. please find my reply inline.
>
>
> > > +++ b/drivers/cpufreq/tegra194-cpufreq.c
> > > @@ -0,0 +1,403 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved
> >
> > 2020
You missed this ?
> T194 supports four CPU clusters, each with two cores. Each CPU cluster is
> capable of running at a specific frequency sourced by respective NAFLL to
> provide cluster specific clocks. Individual cores within a cluster write
> freq in per core register. Cluster h/w forwards the max(core0, core1)
> request to per cluster NAFLL.
Okay, this is clear now. Add a comment about this max thing in the
target routine to show why you need to do this on all CPUs.
--
viresh
^ permalink raw reply
* Re: [PATCH v6 4/4] iommu/arm-smmu-nvidia: fix the warning reported by kbuild test robot
From: Thierry Reding @ 2020-06-23 8:33 UTC (permalink / raw)
To: Krishna Reddy
Cc: snikam-DDmLM1+adcrQT0dZR+AlfA, mperttunen-DDmLM1+adcrQT0dZR+AlfA,
kbuild test robot, bhuntsman-DDmLM1+adcrQT0dZR+AlfA,
will-DgEjT+Ai2ygdnm+yROfE0A, joro-zLv9SwRftAIdnm+yROfE0A,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
praithatha-DDmLM1+adcrQT0dZR+AlfA, talho-DDmLM1+adcrQT0dZR+AlfA,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
nicolinc-DDmLM1+adcrQT0dZR+AlfA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, yhsu-DDmLM1+adcrQT0dZR+AlfA,
treding-DDmLM1+adcrQT0dZR+AlfA, robin.murphy-5wv7dgnIgG8,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
bbiswas-DDmLM1+adcrQT0dZR+AlfA
In-Reply-To: <20200604234414.21912-5-vdumpa-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 527 bytes --]
On Thu, Jun 04, 2020 at 04:44:14PM -0700, Krishna Reddy wrote:
> >> drivers/iommu/arm-smmu-nvidia.c:151:33: sparse: sparse: cast removes
> >> address space '<asn:2>' of expression
>
> Reported-by: kbuild test robot <lkp-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Krishna Reddy <vdumpa-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> drivers/iommu/arm-smmu-nvidia.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
This should be folded into the patch that introduced this error.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v6 3/4] iommu/arm-smmu: Add global/context fault implementation hooks
From: Thierry Reding @ 2020-06-23 8:36 UTC (permalink / raw)
To: Krishna Reddy
Cc: snikam-DDmLM1+adcrQT0dZR+AlfA, mperttunen-DDmLM1+adcrQT0dZR+AlfA,
bhuntsman-DDmLM1+adcrQT0dZR+AlfA, will-DgEjT+Ai2ygdnm+yROfE0A,
joro-zLv9SwRftAIdnm+yROfE0A, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
praithatha-DDmLM1+adcrQT0dZR+AlfA, talho-DDmLM1+adcrQT0dZR+AlfA,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
nicolinc-DDmLM1+adcrQT0dZR+AlfA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, yhsu-DDmLM1+adcrQT0dZR+AlfA,
treding-DDmLM1+adcrQT0dZR+AlfA, robin.murphy-5wv7dgnIgG8,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
bbiswas-DDmLM1+adcrQT0dZR+AlfA
In-Reply-To: <20200604234414.21912-4-vdumpa-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 6461 bytes --]
On Thu, Jun 04, 2020 at 04:44:13PM -0700, Krishna Reddy wrote:
> Add global/context fault hooks to allow NVIDIA SMMU implementation
> handle faults across multiple SMMUs.
>
> Signed-off-by: Krishna Reddy <vdumpa-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> drivers/iommu/arm-smmu-nvidia.c | 100 ++++++++++++++++++++++++++++++++
> drivers/iommu/arm-smmu.c | 11 +++-
> drivers/iommu/arm-smmu.h | 3 +
> 3 files changed, 112 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-nvidia.c b/drivers/iommu/arm-smmu-nvidia.c
> index dafc293a45217..5999b6a770992 100644
> --- a/drivers/iommu/arm-smmu-nvidia.c
> +++ b/drivers/iommu/arm-smmu-nvidia.c
> @@ -117,6 +117,104 @@ static int nsmmu_reset(struct arm_smmu_device *smmu)
> return 0;
> }
>
> +static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
> +{
> + return container_of(dom, struct arm_smmu_domain, domain);
> +}
> +
> +static irqreturn_t nsmmu_global_fault_inst(int irq,
> + struct arm_smmu_device *smmu,
> + int inst)
> +{
> + u32 gfsr, gfsynr0, gfsynr1, gfsynr2;
> +
> + gfsr = readl_relaxed(nsmmu_page(smmu, inst, 0) + ARM_SMMU_GR0_sGFSR);
> + gfsynr0 = readl_relaxed(nsmmu_page(smmu, inst, 0) +
> + ARM_SMMU_GR0_sGFSYNR0);
> + gfsynr1 = readl_relaxed(nsmmu_page(smmu, inst, 0) +
> + ARM_SMMU_GR0_sGFSYNR1);
> + gfsynr2 = readl_relaxed(nsmmu_page(smmu, inst, 0) +
> + ARM_SMMU_GR0_sGFSYNR2);
> +
> + if (!gfsr)
> + return IRQ_NONE;
> +
> + dev_err_ratelimited(smmu->dev,
> + "Unexpected global fault, this could be serious\n");
> + dev_err_ratelimited(smmu->dev,
> + "\tGFSR 0x%08x, GFSYNR0 0x%08x, GFSYNR1 0x%08x, GFSYNR2 0x%08x\n",
> + gfsr, gfsynr0, gfsynr1, gfsynr2);
> +
> + writel_relaxed(gfsr, nsmmu_page(smmu, inst, 0) + ARM_SMMU_GR0_sGFSR);
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t nsmmu_global_fault(int irq, void *dev)
> +{
> + int inst;
> + irqreturn_t irq_ret = IRQ_NONE;
> + struct arm_smmu_device *smmu = dev;
> +
> + for (inst = 0; inst < to_nvidia_smmu(smmu)->num_inst; inst++) {
> + irq_ret = nsmmu_global_fault_inst(irq, smmu, inst);
> + if (irq_ret == IRQ_HANDLED)
> + return irq_ret;
> + }
> +
> + return irq_ret;
> +}
> +
> +static irqreturn_t nsmmu_context_fault_bank(int irq,
> + struct arm_smmu_device *smmu,
> + int idx, int inst)
> +{
> + u32 fsr, fsynr, cbfrsynra;
> + unsigned long iova;
> +
> + fsr = arm_smmu_cb_read(smmu, idx, ARM_SMMU_CB_FSR);
> + if (!(fsr & ARM_SMMU_FSR_FAULT))
> + return IRQ_NONE;
> +
> + fsynr = readl_relaxed(nsmmu_page(smmu, inst, smmu->numpage + idx) +
> + ARM_SMMU_CB_FSYNR0);
> + iova = readq_relaxed(nsmmu_page(smmu, inst, smmu->numpage + idx) +
> + ARM_SMMU_CB_FAR);
> + cbfrsynra = readl_relaxed(nsmmu_page(smmu, inst, 1) +
> + ARM_SMMU_GR1_CBFRSYNRA(idx));
> +
> + dev_err_ratelimited(smmu->dev,
> + "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, cbfrsynra=0x%x, cb=%d\n",
> + fsr, iova, fsynr, cbfrsynra, idx);
> +
> + writel_relaxed(fsr, nsmmu_page(smmu, inst, smmu->numpage + idx) +
> + ARM_SMMU_CB_FSR);
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t nsmmu_context_fault(int irq, void *dev)
> +{
> + int inst, idx;
> + irqreturn_t irq_ret = IRQ_NONE;
> + struct iommu_domain *domain = dev;
> + struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> + struct arm_smmu_device *smmu = smmu_domain->smmu;
> +
> + for (inst = 0; inst < to_nvidia_smmu(smmu)->num_inst; inst++) {
> + /* Interrupt line shared between all context faults.
> + * Check for faults across all contexts.
> + */
> + for (idx = 0; idx < smmu->num_context_banks; idx++) {
> + irq_ret = nsmmu_context_fault_bank(irq, smmu,
> + idx, inst);
> +
> + if (irq_ret == IRQ_HANDLED)
> + return irq_ret;
> + }
> + }
> +
> + return irq_ret;
> +}
> +
> static const struct arm_smmu_impl nvidia_smmu_impl = {
> .read_reg = nsmmu_read_reg,
> .write_reg = nsmmu_write_reg,
> @@ -124,6 +222,8 @@ static const struct arm_smmu_impl nvidia_smmu_impl = {
> .write_reg64 = nsmmu_write_reg64,
> .reset = nsmmu_reset,
> .tlb_sync = nsmmu_tlb_sync,
> + .global_fault = nsmmu_global_fault,
> + .context_fault = nsmmu_context_fault,
> };
>
> struct arm_smmu_device *nvidia_smmu_impl_init(struct arm_smmu_device *smmu)
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 243bc4cb2705b..d720e1e191176 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -673,6 +673,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> enum io_pgtable_fmt fmt;
> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> + irqreturn_t (*context_fault)(int irq, void *dev);
>
> mutex_lock(&smmu_domain->init_mutex);
> if (smmu_domain->smmu)
> @@ -835,7 +836,9 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> * handler seeing a half-initialised domain state.
> */
> irq = smmu->irqs[smmu->num_global_irqs + cfg->irptndx];
> - ret = devm_request_irq(smmu->dev, irq, arm_smmu_context_fault,
> + context_fault = (smmu->impl && smmu->impl->context_fault) ?
> + smmu->impl->context_fault : arm_smmu_context_fault;
A simpler way might have been to assign arm_smmu_context_fault to all
implementations. That way we wouldn't have to perform this check here
and instead just always using smmu->impl->context_fault.
> + ret = devm_request_irq(smmu->dev, irq, context_fault,
> IRQF_SHARED, "arm-smmu-context-fault", domain);
> if (ret < 0) {
> dev_err(smmu->dev, "failed to request context IRQ %d (%u)\n",
> @@ -2107,6 +2110,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> struct arm_smmu_device *smmu;
> struct device *dev = &pdev->dev;
> int num_irqs, i, err;
> + irqreturn_t (*global_fault)(int irq, void *dev);
>
> smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
> if (!smmu) {
> @@ -2193,9 +2197,12 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> smmu->num_context_irqs = smmu->num_context_banks;
> }
>
> + global_fault = (smmu->impl && smmu->impl->global_fault) ?
> + smmu->impl->global_fault : arm_smmu_global_fault;
> +
Same as above.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v6 2/4] dt-bindings: arm-smmu: Add binding for Tegra194 SMMU
From: Thierry Reding @ 2020-06-23 8:38 UTC (permalink / raw)
To: Krishna Reddy
Cc: snikam-DDmLM1+adcrQT0dZR+AlfA, mperttunen-DDmLM1+adcrQT0dZR+AlfA,
bhuntsman-DDmLM1+adcrQT0dZR+AlfA, will-DgEjT+Ai2ygdnm+yROfE0A,
joro-zLv9SwRftAIdnm+yROfE0A, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
praithatha-DDmLM1+adcrQT0dZR+AlfA, talho-DDmLM1+adcrQT0dZR+AlfA,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
nicolinc-DDmLM1+adcrQT0dZR+AlfA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, yhsu-DDmLM1+adcrQT0dZR+AlfA,
treding-DDmLM1+adcrQT0dZR+AlfA, robin.murphy-5wv7dgnIgG8,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
bbiswas-DDmLM1+adcrQT0dZR+AlfA
In-Reply-To: <20200604234414.21912-3-vdumpa-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1066 bytes --]
On Thu, Jun 04, 2020 at 04:44:12PM -0700, Krishna Reddy wrote:
> Add binding for NVIDIA's Tegra194 Soc SMMU that is based
> on ARM MMU-500.
>
> Signed-off-by: Krishna Reddy <vdumpa-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> index e3ef1c69d1326..8f7ffd248f303 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> @@ -37,6 +37,11 @@ properties:
> - qcom,sc7180-smmu-500
> - qcom,sdm845-smmu-500
> - const: arm,mmu-500
> + - description: NVIDIA SoCs that use more than one "arm,mmu-500"
> + items:
> + - enum:
> + - nvdia,tegra194-smmu-500
The -500 suffix here seems a bit redundant since there's no other type
of SMMU in Tegra194, correct?
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] [v4] dmaengine: tegra210-adma: Fix runtime PM imbalance on error
From: Jon Hunter @ 2020-06-23 10:13 UTC (permalink / raw)
To: Dinghao Liu, kjlu-OJFnDUYgAso
Cc: Laxman Dewangan, Vinod Koul, Dan Williams, Thierry Reding,
dmaengine-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20200621054710.9915-1-dinghao.liu-Y5EWUtBUdg4nDS1+zs4M5A@public.gmane.org>
On 21/06/2020 06:47, Dinghao Liu wrote:
> pm_runtime_get_sync() increments the runtime PM usage counter even
> when it returns an error code. Thus a pairing decrement is needed on
> the error handling path to keep the counter balanced.
So you have not mentioned here why you are using _noidle and not _put.
Furthermore, in this patch [0] you are not using _noidle to fix the same
problem in another driver. We should fix this in a consistent manner
across all drivers, otherwise it leads to more confusion.
Finally, Rafael mentions we should just use _put [0] and so I think we
should follow his recommendation.
Jon
[0] https://lkml.org/lkml/2020/5/21/601
--
nvpublic
^ permalink raw reply
* Re: [PATCH v6 1/4] iommu/arm-smmu: add NVIDIA implementation for dual ARM MMU-500 usage
From: Thierry Reding @ 2020-06-23 10:29 UTC (permalink / raw)
To: Krishna Reddy
Cc: snikam-DDmLM1+adcrQT0dZR+AlfA, mperttunen-DDmLM1+adcrQT0dZR+AlfA,
bhuntsman-DDmLM1+adcrQT0dZR+AlfA, will-DgEjT+Ai2ygdnm+yROfE0A,
joro-zLv9SwRftAIdnm+yROfE0A, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
praithatha-DDmLM1+adcrQT0dZR+AlfA, talho-DDmLM1+adcrQT0dZR+AlfA,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
nicolinc-DDmLM1+adcrQT0dZR+AlfA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, yhsu-DDmLM1+adcrQT0dZR+AlfA,
treding-DDmLM1+adcrQT0dZR+AlfA, robin.murphy-5wv7dgnIgG8,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
bbiswas-DDmLM1+adcrQT0dZR+AlfA
In-Reply-To: <20200604234414.21912-2-vdumpa-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 10687 bytes --]
On Thu, Jun 04, 2020 at 04:44:11PM -0700, Krishna Reddy wrote:
> NVIDIA's Tegra194 soc uses two ARM MMU-500s together to interleave
s/soc/SoC/
> IOVA accesses across them.
> Add NVIDIA implementation for dual ARM MMU-500s and add new compatible
> string for Tegra194 soc.
Same here.
>
> Signed-off-by: Krishna Reddy <vdumpa-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> MAINTAINERS | 2 +
> drivers/iommu/Makefile | 2 +-
> drivers/iommu/arm-smmu-impl.c | 3 +
> drivers/iommu/arm-smmu-nvidia.c | 161 ++++++++++++++++++++++++++++++++
> drivers/iommu/arm-smmu.h | 1 +
> 5 files changed, 168 insertions(+), 1 deletion(-)
> create mode 100644 drivers/iommu/arm-smmu-nvidia.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 50659d76976b7..118da0893c964 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16572,9 +16572,11 @@ F: drivers/i2c/busses/i2c-tegra.c
>
> TEGRA IOMMU DRIVERS
> M: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> +R: Krishna Reddy <vdumpa-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> L: linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> S: Supported
> F: drivers/iommu/tegra*
> +F: drivers/iommu/arm-smmu-nvidia.c
>
> TEGRA KBC DRIVER
> M: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 57cf4ba5e27cb..35542df00da72 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -15,7 +15,7 @@ obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o amd_iommu_quirks.o
> obj-$(CONFIG_AMD_IOMMU_DEBUGFS) += amd_iommu_debugfs.o
> obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o
> obj-$(CONFIG_ARM_SMMU) += arm_smmu.o
> -arm_smmu-objs += arm-smmu.o arm-smmu-impl.o arm-smmu-qcom.o
> +arm_smmu-objs += arm-smmu.o arm-smmu-impl.o arm-smmu-qcom.o arm-smmu-nvidia.o
> obj-$(CONFIG_ARM_SMMU_V3) += arm-smmu-v3.o
> obj-$(CONFIG_DMAR_TABLE) += dmar.o
> obj-$(CONFIG_INTEL_IOMMU) += intel-iommu.o intel-pasid.o
> diff --git a/drivers/iommu/arm-smmu-impl.c b/drivers/iommu/arm-smmu-impl.c
> index c75b9d957b702..52c84c30f83e4 100644
> --- a/drivers/iommu/arm-smmu-impl.c
> +++ b/drivers/iommu/arm-smmu-impl.c
> @@ -160,6 +160,9 @@ struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu)
> */
> switch (smmu->model) {
> case ARM_MMU500:
> + if (of_device_is_compatible(smmu->dev->of_node,
> + "nvidia,tegra194-smmu-500"))
> + return nvidia_smmu_impl_init(smmu);
Should NVIDIA_TEGRA194_SMMU be a separate value for smmu->model,
perhaps? That way we avoid this somewhat odd check here.
> smmu->impl = &arm_mmu500_impl;
> break;
> case CAVIUM_SMMUV2:
> diff --git a/drivers/iommu/arm-smmu-nvidia.c b/drivers/iommu/arm-smmu-nvidia.c
I wonder if it would be better to name this arm-smmu-tegra.c to make it
clearer that this is for a Tegra chip. We do have regular expressions in
MAINTAINERS that catch anything with "tegra" in it to make this easier.
> new file mode 100644
> index 0000000000000..dafc293a45217
> --- /dev/null
> +++ b/drivers/iommu/arm-smmu-nvidia.c
> @@ -0,0 +1,161 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Nvidia ARM SMMU v2 implementation quirks
s/Nvidia/NVIDIA/
> +// Copyright (C) 2019 NVIDIA CORPORATION. All rights reserved.
I suppose this should now also include 2020.
> +
> +#define pr_fmt(fmt) "nvidia-smmu: " fmt
Same here. Might be worth making this "tegra-smmu: " for consistency.
> +
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include "arm-smmu.h"
> +
> +/* Tegra194 has three ARM MMU-500 Instances.
> + * Two of them are used together for Interleaved IOVA accesses and
> + * used by Non-Isochronous Hw devices for SMMU translations.
"non-isochronous", s/Hw/HW/
> + * Third one is used for SMMU translations from Isochronous HW devices.
"isochronous"
> + * It is possible to use this Implementation to program either
"implementation"
> + * all three or two of the instances identically as desired through
> + * DT node.
> + *
> + * Programming all the three instances identically comes with redundant tlb
s/tlb/TLB/
> + * invalidations as all three never need to be tlb invalidated for a HW device.
Same here.
> + *
> + * When Linux Kernel supports multiple SMMU devices, The SMMU device used for
"kernel" and "..., the SMMU device"
> + * Isochornous HW devices should be added as a separate ARM MMU-500 device
"isochronous"
> + * in DT and be programmed independently for efficient tlb invalidates.
"TLB"
> + *
> + */
> +#define MAX_SMMU_INSTANCES 3
> +
> +#define TLB_LOOP_TIMEOUT 1000000 /* 1s! */
USEC_PER_SEC?
> +#define TLB_SPIN_COUNT 10
> +
> +struct nvidia_smmu {
> + struct arm_smmu_device smmu;
> + unsigned int num_inst;
> + void __iomem *bases[MAX_SMMU_INSTANCES];
> +};
> +
> +#define to_nvidia_smmu(s) container_of(s, struct nvidia_smmu, smmu)
Making this static inline can make error messages more readable.
> +
> +#define nsmmu_page(smmu, inst, page) \
> + (((inst) ? to_nvidia_smmu(smmu)->bases[(inst)] : smmu->base) + \
> + ((page) << smmu->pgshift))
Can we simply define to_nvidia_smmu(smmu)->bases[0] = smmu->base in
nvidia_smmu_impl_init()? Then this would become just:
to_nvidia_smmu(smmu)->bases[inst] + ((page) << (smmu)->pgshift)
Also, the nsmmu_ prefix looks somewhat odd here. You already use struct
nvidia_smmu as the name of the structure, so why not be consistent and
continue to use nvidia_smmu_ as the prefix for function names?
Or perhaps even use tegra_smmu_ as the prefix to match the filename
change I suggested earlier.
> +
> +static u32 nsmmu_read_reg(struct arm_smmu_device *smmu,
> + int page, int offset)
> +{
> + return readl_relaxed(nsmmu_page(smmu, 0, page) + offset);
> +}
> +
> +static void nsmmu_write_reg(struct arm_smmu_device *smmu,
> + int page, int offset, u32 val)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < to_nvidia_smmu(smmu)->num_inst; i++)
> + writel_relaxed(val, nsmmu_page(smmu, i, page) + offset);
> +}
> +
> +static u64 nsmmu_read_reg64(struct arm_smmu_device *smmu,
> + int page, int offset)
> +{
> + return readq_relaxed(nsmmu_page(smmu, 0, page) + offset);
> +}
> +
> +static void nsmmu_write_reg64(struct arm_smmu_device *smmu,
> + int page, int offset, u64 val)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < to_nvidia_smmu(smmu)->num_inst; i++)
> + writeq_relaxed(val, nsmmu_page(smmu, i, page) + offset);
> +}
> +
> +static void nsmmu_tlb_sync(struct arm_smmu_device *smmu, int page,
> + int sync, int status)
> +{
> + u32 reg;
I see this is being used to store the value read from a register. I find
it clearer to call this "value" or "val" (or in this case perhaps even
"status") because whenever I read "reg" I immediately think this is
meant to be a register offset, which can then be confusing when I see it
used in I/O accessors because it is in the wrong position.
But anyway, that's just my opinion and this is a bit subjective, so feel
free to ignore.
> + unsigned int i;
> + unsigned int spin_cnt, delay;
> +
> + arm_smmu_writel(smmu, page, sync, 0);
> +
> + for (delay = 1; delay < TLB_LOOP_TIMEOUT; delay *= 2) {
> + for (spin_cnt = TLB_SPIN_COUNT; spin_cnt > 0; spin_cnt--) {
> + reg = 0;
You may want to declare the variable at this scope since you never need
it outside. Also, use a space between variable initialization and the
for block below for better readability.
> + for (i = 0; i < to_nvidia_smmu(smmu)->num_inst; i++) {
> + reg |= readl_relaxed(
> + nsmmu_page(smmu, i, page) + status);
> + }
Maybe add a separate variable for the page address so this can be a bit
uncluttered. Also, I'd prefer a blank line after the block for
readability.
> + if (!(reg & ARM_SMMU_sTLBGSTATUS_GSACTIVE))
> + return;
> + cpu_relax();
Blank line above cpu_relax() for readability.
> + }
> + udelay(delay);
Again, a blank line between blocks and subsequent statements can help
readability.
> + }
> + dev_err_ratelimited(smmu->dev,
> + "TLB sync timed out -- SMMU may be deadlocked\n");
Same here.
Also, is there anything we can do when this happens?
> +}
> +
> +static int nsmmu_reset(struct arm_smmu_device *smmu)
> +{
> + u32 reg;
> + unsigned int i;
> +
> + for (i = 0; i < to_nvidia_smmu(smmu)->num_inst; i++) {
> + /* clear global FSR */
> + reg = readl_relaxed(nsmmu_page(smmu, i, ARM_SMMU_GR0) +
> + ARM_SMMU_GR0_sGFSR);
> + writel_relaxed(reg, nsmmu_page(smmu, i, ARM_SMMU_GR0) +
> + ARM_SMMU_GR0_sGFSR);
> + }
> +
> + return 0;
> +}
> +
> +static const struct arm_smmu_impl nvidia_smmu_impl = {
> + .read_reg = nsmmu_read_reg,
> + .write_reg = nsmmu_write_reg,
> + .read_reg64 = nsmmu_read_reg64,
> + .write_reg64 = nsmmu_write_reg64,
> + .reset = nsmmu_reset,
> + .tlb_sync = nsmmu_tlb_sync,
> +};
> +
> +struct arm_smmu_device *nvidia_smmu_impl_init(struct arm_smmu_device *smmu)
> +{
> + unsigned int i;
> + struct nvidia_smmu *nsmmu;
> + struct resource *res;
> + struct device *dev = smmu->dev;
> + struct platform_device *pdev = to_platform_device(smmu->dev);
> +
> + nsmmu = devm_kzalloc(smmu->dev, sizeof(*nsmmu), GFP_KERNEL);
> + if (!nsmmu)
> + return ERR_PTR(-ENOMEM);
> +
> + nsmmu->smmu = *smmu;
> + /* Instance 0 is ioremapped by arm-smmu.c */
> + nsmmu->num_inst = 1;
Maybe add this here to simplify the nsmmu_page() macro above:
nsmmu->bases[0] = smmu->base;
> +
> + for (i = 1; i < MAX_SMMU_INSTANCES; i++) {
> + res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> + if (!res)
> + break;
> + nsmmu->bases[i] = devm_ioremap_resource(dev, res);
> + if (IS_ERR(nsmmu->bases[i]))
> + return (struct arm_smmu_device *)nsmmu->bases[i];
ERR_CAST()?
> + nsmmu->num_inst++;
> + }
More blank lines would make this much easier to read, in my opinion.
> +
> + nsmmu->smmu.impl = &nvidia_smmu_impl;
> + devm_kfree(smmu->dev, smmu);
Maybe a comment here would be useful for readers to immediately
understand why you're doing this here.
> + pr_info("NVIDIA ARM SMMU Implementation, Instances=%d\n",
> + nsmmu->num_inst);
I think I'd just omit this. In general you should only let the user know
when things go wrong, but the above is printed when everything goes as
expected.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Greetings From Mrs. Sarah Koffi
From: Sarah Koffi @ 2020-06-23 11:16 UTC (permalink / raw)
To: sarahkoffi389-/E1597aS9LR3+QwDJ9on6Q
Greetings From Mrs. Sarah Koffi
I'm contacting you based on your good profiles I read and for a good
reasons, I am in search of a property to buy in your country as I
intended to come over to your
country for investment, Though I have not meet with you before but I
believe that one has to risk confiding in someone to succeed sometimes
in life.
My name is Mrs. Sarah Koffi. My late husband deals on Crude Oil with
Federal Government of Sudan and he has a personal Oil firm in Bentiu
Oil zone town and Upper
Nile city. What I have experience physically, I don't wish to
experience it again in my life due to the recent civil Ethnic war
cause by our President Mr. Salva Kiir
and the rebel leader Mr Riek Machar, I have been Under United Nation
refuge camp in chad to save my life and that of my little daughter.
Though, I do not know how you will feel to my proposal, but the truth
is that I sneaked into Chad our neighboring country where I am living
now as a refugee.
I escaped with my little daughter when the rebels bust into our house
and killed my husband as one of the big oil dealers in the country,
ever since then, I have being on the run.
I left my country and move to Chad our neighboring country with the
little ceasefire we had, due to the face to face peace meeting accord
coordinated by the US Secretary of State, Mr John Kerry and United
Nations in Ethiopia (Addis Ababa) between our President Mr Salva Kiir
and the rebel leader Mr Riek Machar to stop this war.
I want to solicit for your partnership with trust to invest the $8
million dollars deposited by my late husband in Bank because my life
is no longer safe in our country, since the rebels are looking for the
families of all the oil business men in the country to kill, saying
that they are they one that is milking the country dry.
I will offer you 20% of the total fund for your help while I will
partner with you for the investment in your country.
If I get your reply.
I will wait to hear from you so as to give you details.With love from
i need you to contact me here sarahkoffi389-/E1597aS9LR3+QwDJ9on6Q@public.gmane.org
Mrs. Sarah Koffi
^ permalink raw reply
* Re: [PATCH v6 1/4] iommu/arm-smmu: add NVIDIA implementation for dual ARM MMU-500 usage
From: Robin Murphy @ 2020-06-23 11:16 UTC (permalink / raw)
To: Thierry Reding, Krishna Reddy
Cc: snikam-DDmLM1+adcrQT0dZR+AlfA, praithatha-DDmLM1+adcrQT0dZR+AlfA,
bhuntsman-DDmLM1+adcrQT0dZR+AlfA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
mperttunen-DDmLM1+adcrQT0dZR+AlfA, talho-DDmLM1+adcrQT0dZR+AlfA,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
nicolinc-DDmLM1+adcrQT0dZR+AlfA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, yhsu-DDmLM1+adcrQT0dZR+AlfA,
treding-DDmLM1+adcrQT0dZR+AlfA, will-DgEjT+Ai2ygdnm+yROfE0A,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
bbiswas-DDmLM1+adcrQT0dZR+AlfA
In-Reply-To: <20200623102927.GD4098287@ulmo>
On 2020-06-23 11:29, Thierry Reding wrote:
[...]
>> diff --git a/drivers/iommu/arm-smmu-impl.c b/drivers/iommu/arm-smmu-impl.c
>> index c75b9d957b702..52c84c30f83e4 100644
>> --- a/drivers/iommu/arm-smmu-impl.c
>> +++ b/drivers/iommu/arm-smmu-impl.c
>> @@ -160,6 +160,9 @@ struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu)
>> */
>> switch (smmu->model) {
>> case ARM_MMU500:
>> + if (of_device_is_compatible(smmu->dev->of_node,
>> + "nvidia,tegra194-smmu-500"))
>> + return nvidia_smmu_impl_init(smmu);
>
> Should NVIDIA_TEGRA194_SMMU be a separate value for smmu->model,
> perhaps? That way we avoid this somewhat odd check here.
No, this is simply in the wrong place. The design here is that we pick
up anything related to the basic SMMU IP (model) first, then make any
platform-specific integration checks. That way a platform-specific init
function can see the model impl set and subclass it if necessary
(although nobody's actually done that yet). The setup for Cavium is just
a short-cut since their model is unique to their integration, so the
lines get a bit blurred and there's little benefit to trying to separate
it out.
In short, put this down below with the other of_device_is_compatible()
checks.
>> smmu->impl = &arm_mmu500_impl;
>> break;
>> case CAVIUM_SMMUV2:
>> diff --git a/drivers/iommu/arm-smmu-nvidia.c b/drivers/iommu/arm-smmu-nvidia.c
>
> I wonder if it would be better to name this arm-smmu-tegra.c to make it
> clearer that this is for a Tegra chip. We do have regular expressions in
> MAINTAINERS that catch anything with "tegra" in it to make this easier.
There was a notion that these would be grouped by vendor, but if there's
a strong preference for all NVIDIA-SoC-related stuff to be named "Tegra"
then I'm not going to complain too much.
>> new file mode 100644
>> index 0000000000000..dafc293a45217
>> --- /dev/null
>> +++ b/drivers/iommu/arm-smmu-nvidia.c
>> @@ -0,0 +1,161 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +// Nvidia ARM SMMU v2 implementation quirks
>
> s/Nvidia/NVIDIA/
>
>> +// Copyright (C) 2019 NVIDIA CORPORATION. All rights reserved.
>
> I suppose this should now also include 2020.
>
>> +
>> +#define pr_fmt(fmt) "nvidia-smmu: " fmt
>
> Same here. Might be worth making this "tegra-smmu: " for consistency.
On the other hand, a log prefix that is literally the name of a
completely unrelated driver seems more confusing to users than useful.
Same for the function naming - the tegra_smmu_* namespace is already
owned by that driver.
Robin.
^ permalink raw reply
* Re: [PATCH v6 3/4] iommu/arm-smmu: Add global/context fault implementation hooks
From: Robin Murphy @ 2020-06-23 11:30 UTC (permalink / raw)
To: Thierry Reding, Krishna Reddy
Cc: snikam-DDmLM1+adcrQT0dZR+AlfA, mperttunen-DDmLM1+adcrQT0dZR+AlfA,
bhuntsman-DDmLM1+adcrQT0dZR+AlfA, will-DgEjT+Ai2ygdnm+yROfE0A,
joro-zLv9SwRftAIdnm+yROfE0A, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
praithatha-DDmLM1+adcrQT0dZR+AlfA, talho-DDmLM1+adcrQT0dZR+AlfA,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
nicolinc-DDmLM1+adcrQT0dZR+AlfA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, yhsu-DDmLM1+adcrQT0dZR+AlfA,
treding-DDmLM1+adcrQT0dZR+AlfA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
bbiswas-DDmLM1+adcrQT0dZR+AlfA
In-Reply-To: <20200623083643.GB4098287@ulmo>
On 2020-06-23 09:36, Thierry Reding wrote:
[...]
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 243bc4cb2705b..d720e1e191176 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -673,6 +673,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>> enum io_pgtable_fmt fmt;
>> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>> + irqreturn_t (*context_fault)(int irq, void *dev);
>>
>> mutex_lock(&smmu_domain->init_mutex);
>> if (smmu_domain->smmu)
>> @@ -835,7 +836,9 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>> * handler seeing a half-initialised domain state.
>> */
>> irq = smmu->irqs[smmu->num_global_irqs + cfg->irptndx];
>> - ret = devm_request_irq(smmu->dev, irq, arm_smmu_context_fault,
>> + context_fault = (smmu->impl && smmu->impl->context_fault) ?
>> + smmu->impl->context_fault : arm_smmu_context_fault;
>
> A simpler way might have been to assign arm_smmu_context_fault to all
> implementations. That way we wouldn't have to perform this check here
> and instead just always using smmu->impl->context_fault.
But smmu->impl can still be NULL...
Everything in impl, including the presence of impl itself, is optional,
so the notion of overriding a default with the same default doesn't
really make much sense, and would go against the pattern everywhere else.
Robin.
>> + ret = devm_request_irq(smmu->dev, irq, context_fault,
>> IRQF_SHARED, "arm-smmu-context-fault", domain);
>> if (ret < 0) {
>> dev_err(smmu->dev, "failed to request context IRQ %d (%u)\n",
>> @@ -2107,6 +2110,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>> struct arm_smmu_device *smmu;
>> struct device *dev = &pdev->dev;
>> int num_irqs, i, err;
>> + irqreturn_t (*global_fault)(int irq, void *dev);
>>
>> smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
>> if (!smmu) {
>> @@ -2193,9 +2197,12 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>> smmu->num_context_irqs = smmu->num_context_banks;
>> }
>>
>> + global_fault = (smmu->impl && smmu->impl->global_fault) ?
>> + smmu->impl->global_fault : arm_smmu_global_fault;
>> +
>
> Same as above.
>
> Thierry
>
^ permalink raw reply
* Re: [PATCH] [v4] dmaengine: tegra210-adma: Fix runtime PM imbalance on error
From: Geert Uytterhoeven @ 2020-06-23 12:08 UTC (permalink / raw)
To: Jon Hunter
Cc: Dinghao Liu, Kangjie Lu, Laxman Dewangan, Vinod Koul,
Dan Williams, Thierry Reding, dmaengine, linux-tegra,
Linux Kernel Mailing List
In-Reply-To: <44d7771e-5600-19c2-888a-dd226cbc4b50-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Hi Jon,
More stirring in the cesspool ;-)
On Tue, Jun 23, 2020 at 12:13 PM Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> On 21/06/2020 06:47, Dinghao Liu wrote:
> > pm_runtime_get_sync() increments the runtime PM usage counter even
> > when it returns an error code. Thus a pairing decrement is needed on
> > the error handling path to keep the counter balanced.
>
> So you have not mentioned here why you are using _noidle and not _put.
> Furthermore, in this patch [0] you are not using _noidle to fix the same
> problem in another driver. We should fix this in a consistent manner
> across all drivers, otherwise it leads to more confusion.
>
> Finally, Rafael mentions we should just use _put [0] and so I think we
> should follow his recommendation.
>
> Jon
>
> [0] https://lkml.org/lkml/2020/5/21/601
"_noidle() is the simplest one and it is sufficient."
https://lore.kernel.org/linux-i2c/CAJZ5v0i87NGcy9+kxubScdPDyByr8ypQWcGgBFn+V-wDd69BHQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org/
You never know what additional things the other put* variants
will start doing in the future...
Gr{oetje,eeting}s,
Geert
^ 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