* [GIT PULL 2/5] i.MX soc updates for 4.10
From: Olof Johansson @ 2016-11-18 17:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479200431-1765-2-git-send-email-shawnguo@kernel.org>
On Tue, Nov 15, 2016 at 05:00:28PM +0800, Shawn Guo wrote:
> The following changes since commit 1001354ca34179f3db924eb66672442a173147dc:
>
> Linux 4.9-rc1 (2016-10-15 12:17:50 -0700)
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git tags/imx-soc-4.10
>
> for you to fetch changes up to 75b832fea2564c547dfd0a2377a5893f41aefec6:
>
> ARM: imx: mach-imx6ul: add imx6ull support (2016-11-15 08:58:43 +0800)
>
> ----------------------------------------------------------------
> i.MX SoC changes for 4.10:
> - Drop PL310_ERRATA_769419 for Vybrid, as it turns out that the SoC
> integrates revision r3p2 of the L2C-310, which is not affected by
> errata 769419.
> - Support perf for i.MX6 Multi-Mode DDR Controller (MMDC), so that we
> can profile memory access performance.
> - Support i.MX6ULL SoC using i.MX6UL base, since it's a derivative of
> i.MX6UL and pin-to-pin compatible with i.MX6UL.
Merged, thanks.
-Olof
^ permalink raw reply
* [GIT PULL 1/5] i.MX non-critical fixes for 4.10
From: Olof Johansson @ 2016-11-18 17:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479200431-1765-1-git-send-email-shawnguo@kernel.org>
On Tue, Nov 15, 2016 at 05:00:27PM +0800, Shawn Guo wrote:
> The following changes since commit 1001354ca34179f3db924eb66672442a173147dc:
>
> Linux 4.9-rc1 (2016-10-15 12:17:50 -0700)
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git tags/imx-fix-nc-4.10
>
> for you to fetch changes up to 72649a46067903d00f46e2ebef6543768224f1a0:
>
> ARM: dts: imx6q-cm-fx6: fix fec pinctrl (2016-11-14 15:31:01 +0800)
>
> ----------------------------------------------------------------
> i.MX non-critical fixes for 4.10:
> - A series from Vladimir to fix broken i.MX31 DT clock initialization.
> As i.MX31 DT support is still not quite complete, the changes are
> tested on qemu kzm target and mx31lite board with simple written DTS
> files.
> - A fix for CompuLab's sbc-fx6 baseboard to remove wrong fec pinctrl
> setting.
> - A DTS correction for i.MX6QP to reflect the change that the gate of
> LDB clock has been moved before the divider.
> - An imx7d-pinfunc fix for UART pinmux defines
Merged, thanks.
-Olof
^ permalink raw reply
* [PATCHv3 0/6] CONFIG_DEBUG_VIRTUAL for arm64
From: Mark Rutland @ 2016-11-18 17:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479431816-5028-1-git-send-email-labbott@redhat.com>
On Thu, Nov 17, 2016 at 05:16:50PM -0800, Laura Abbott wrote:
> Hi,
Hi,
Thanks again for putting this together.
> This is v3 of the series to add CONFIG_DEBUG_VIRTUAL for arm64.
> The biggest change from v2 is the conversion of more __pa sites
> to __pa_symbol for stricter checks.
Patches 1-4 look good to me, and I've given them a spin in a few
configurations on arm64. For those:
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Patches 5 and 6 look like they're mostly there, but there are still a
few issues which I've commented on. Hopefully those aren't too painful
to sort out; it would be great to have this available.
Thanks,
Mark.
^ permalink raw reply
* [GIT PULL] add #pinctrl-cells for v4.10
From: Olof Johansson @ 2016-11-18 17:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <582a5813.1bc0620a.e65a9.b2cbSMTPIN_ADDED_BROKEN@mx.google.com>
Hi,
On Mon, Nov 14, 2016 at 04:34:19PM -0800, Tony Lindgren wrote:
> The following changes since commit 1001354ca34179f3db924eb66672442a173147dc:
>
> Linux 4.9-rc1 (2016-10-15 12:17:50 -0700)
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap tags/omap-for-v4.10/pinctrl-cells-signed
>
> for you to fetch changes up to be76fd3197df608e1b010bf5ab90377205f54344:
>
> ARM: dts: Add #pinctrl-cells for pinctrl-single instances (2016-11-07 08:27:49 -0700)
>
> ----------------------------------------------------------------
> Add #pinctrl-cells for pinctrl-single using dts files. This allows
> us to use generic parser later on. Note that the driver supports
> handling the legacy binding also with no #pinctrl-cells so these
> changes can be queued separately from the driver changes.
You forgot to make one commit per file to maximize your patch contribution
count! :-)
Merged, but note that we normally keep arm64 and arm DT updates
separate. I'm gambling on not seeing any conflicts on the hi6220 dtsi
this time around though, and took it in as-is.
-Olof
^ permalink raw reply
* [PATCH] PCI: Add information about describing PCI in ACPI
From: Bjorn Helgaas @ 2016-11-18 17:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <EE11001F9E5DDD47B7634E2F8A612F2E1F921A88@lhreml507-mbx>
On Fri, Nov 18, 2016 at 05:17:34PM +0000, Gabriele Paoloni wrote:
> > -----Original Message-----
> > From: linux-kernel-owner at vger.kernel.org [mailto:linux-kernel-
> > owner at vger.kernel.org] On Behalf Of Bjorn Helgaas
> > Sent: 17 November 2016 18:00
> > +Static tables like MCFG, HPET, ECDT, etc., are *not* mechanisms for
> > +reserving address space! The static tables are for things the OS
> > +needs to know early in boot, before it can parse the ACPI namespace.
> > +If a new table is defined, an old OS needs to operate correctly even
> > +though it ignores the table. _CRS allows that because it is generic
> > +and understood by the old OS; a static table does not.
>
> Right so if my understanding is correct you are saying that resources
> described in the MCFG table should also be declared in PNP0C02 devices
> so that the PNP driver can reserve these resources.
Yes.
> On the other side the PCI Root bridge driver should not reserve such
> resources.
>
> Well if my understanding is correct I think we have a problem here:
> http://lxr.free-electrons.com/source/drivers/pci/ecam.c#L74
>
> As you can see pci_ecam_create() will conflict with the pnp driver
> as it will try to reserve the resources from the MCFG table...
>
> Maybe we need to rework pci_ecam_create() ?
I think it's OK as it is.
The pnp/system.c driver does try to reserve PNP0C02 resources, and it
marks them as "not busy". That way they appear in /proc/iomem and
won't be allocated for anything else, but they can still be requested
by drivers, e.g., pci/ecam.c, which will mark them "busy".
This is analogous to what the PCI core does in pci_claim_resource().
This is really a function of the ACPI/PNP *core*, which should reserve
all _CRS resources for all devices (not just PNP0C02 devices). But
it's done by pnp/system.c, and only for PNP0C02, because there's a
bunch of historical baggage there.
You'll also notice that in this case, things are out of order:
logically the pnp/system.c reservation should happen first, but in
fact the pci/ecam.c request happens *before* the pnp/system.c one.
That means the pnp/system.c one might fail and complain "[mem ...]
could not be reserved".
Bjorn
^ permalink raw reply
* [GIT PULL] ARM: at91: dt for 4.10
From: Olof Johansson @ 2016-11-18 17:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161114183923.k5eki5fzzdfsx2j6@piout.net>
On Mon, Nov 14, 2016 at 07:39:23PM +0100, Alexandre Belloni wrote:
> Hi Arnd, Olof
>
> at91 DT changes for 4.10, I don't expect much more this cycle.
>
> The following changes since commit 1001354ca34179f3db924eb66672442a173147dc:
>
> Linux 4.9-rc1 (2016-10-15 12:17:50 -0700)
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git tags/at91-ab-4.10-dt1
>
> for you to fetch changes up to b662a9dd8a1a03e6e2d61e74d3e7a08400edefb7:
>
> ARM: dts: at91: replace gpio-key,wakeup with wakeup-source for sam9260ek (2016-11-14 17:04:24 +0100)
>
> ----------------------------------------------------------------
> DT changes for 4.10:
>
> - Many additions for sama5d2
> - few non urgent fixes for sam9260ek, sama5d4 and sama5d2
Merged, thanks.
-Olof
^ permalink raw reply
* [PATCHv3 6/6] arm64: Add support for CONFIG_DEBUG_VIRTUAL
From: Mark Rutland @ 2016-11-18 17:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479431816-5028-7-git-send-email-labbott@redhat.com>
Hi,
On Thu, Nov 17, 2016 at 05:16:56PM -0800, Laura Abbott wrote:
>
> x86 has an option CONFIG_DEBUG_VIRTUAL to do additional checks
> on virt_to_phys calls. The goal is to catch users who are calling
> virt_to_phys on non-linear addresses immediately. This inclues callers
> using virt_to_phys on image addresses instead of __pa_symbol. As features
> such as CONFIG_VMAP_STACK get enabled for arm64, this becomes increasingly
> important. Add checks to catch bad virt_to_phys usage.
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> v3: Make use of __pa_symbol required via debug checks. It's a WARN for now but
> it can become a BUG after wider testing. __virt_to_phys is
> now for linear addresses only. Dropped the VM_BUG_ON from Catalin's suggested
> version from the nodebug version since having that in the nodebug version
> essentially made them the debug version. Changed to KERNEL_START/KERNEL_END
> for bounds checking. More comments.
I gave this a go with DEBUG_VIRTUAL && KASAN_INLINE selected, and the
kernel dies somewhere before bringing up earlycon. :(
I mentioned some possible reasons in a reply to pastch 5, and I have
some more comments below.
[...]
> -#define __virt_to_phys(x) ({ \
> +
> +
> +/*
> + * This is for translation from the standard linear map to physical addresses.
> + * It is not to be used for kernel symbols.
> + */
> +#define __virt_to_phys_nodebug(x) ({ \
> phys_addr_t __x = (phys_addr_t)(x); \
> - __x & BIT(VA_BITS - 1) ? (__x & ~PAGE_OFFSET) + PHYS_OFFSET : \
> - (__x - kimage_voffset); })
> + ((__x & ~PAGE_OFFSET) + PHYS_OFFSET); \
> +})
Given the KASAN failure, and the strong possibility that there's even
more stuff lurking in common code, I think we should retain the logic to
handle kernel image addresses for the timebeing (as x86 does). Once
we've merged DEBUG_VIRTUAL, it will be easier to track those down.
Catalin, I think you suggested removing that logic; are you happy for it
to be restored?
See below for a refactoring that retains this logic.
[...]
> +/*
> + * This is for translation from a kernel image/symbol address to a
> + * physical address.
> + */
> +#define __pa_symbol_nodebug(x) ({ \
> + phys_addr_t __x = (phys_addr_t)(x); \
> + (__x - kimage_voffset); \
> +})
We can avoid duplication here (and in physaddr.c) if we factor the logic
into helpers, e.g.
/*
* The linear kernel range starts in the middle of the virtual adddress
* space. Testing the top bit for the start of the region is a
* sufficient check.
*/
#define __is_lm_address(addr) (!!((addr) & BIT(VA_BITS - 1)))
#define __lm_to_phys(addr) (((addr) & ~PAGE_OFFSET) + PHYS_OFFSET)
#define __kimg_to_phys(addr) ((addr) - kimage_voffset)
#define __virt_to_phys_nodebug(x) ({ \
phys_addr_t __x = (phys_addr_t)(x); \
__is_lm_address(__x) ? __lm_to_phys(__x) : \
__kimg_to_phys(__x); \
})
#define __pa_symbol_nodebug(x) __kimg_to_phys((phys_addr_t)(x))
> +#ifdef CONFIG_DEBUG_VIRTUAL
> +extern unsigned long __virt_to_phys(unsigned long x);
> +extern unsigned long __phys_addr_symbol(unsigned long x);
It would be better for both of these to return phys_addr_t.
[...]
> diff --git a/arch/arm64/mm/physaddr.c b/arch/arm64/mm/physaddr.c
> new file mode 100644
> index 0000000..f8eb781
> --- /dev/null
> +++ b/arch/arm64/mm/physaddr.c
> @@ -0,0 +1,39 @@
> +#include <linux/mm.h>
> +
> +#include <asm/memory.h>
We also need:
#include <linux/bug.h>
#include <linux/export.h>
#include <linux/types.h>
#include <linux/mmdebug.h>
> +unsigned long __virt_to_phys(unsigned long x)
> +{
> + phys_addr_t __x = (phys_addr_t)x;
> +
> + if (__x & BIT(VA_BITS - 1)) {
> + /*
> + * The linear kernel range starts in the middle of the virtual
> + * adddress space. Testing the top bit for the start of the
> + * region is a sufficient check.
> + */
> + return (__x & ~PAGE_OFFSET) + PHYS_OFFSET;
> + } else {
> + /*
> + * __virt_to_phys should not be used on symbol addresses.
> + * This should be changed to a BUG once all basic bad uses have
> + * been cleaned up.
> + */
> + WARN(1, "Do not use virt_to_phys on symbol addresses");
> + return __phys_addr_symbol(x);
> + }
> +}
> +EXPORT_SYMBOL(__virt_to_phys);
I think this would be better something like:
phys_addr_t __virt_to_phys(unsigned long x)
{
WARN(!__is_lm_address(x),
"virt_to_phys() used for non-linear address: %pK\n",
(void*)x);
return __virt_to_phys_nodebug(x);
}
EXPORT_SYMBOL(__virt_to_phys);
> +
> +unsigned long __phys_addr_symbol(unsigned long x)
> +{
> + phys_addr_t __x = (phys_addr_t)x;
> +
> + /*
> + * This is bounds checking against the kernel image only.
> + * __pa_symbol should only be used on kernel symbol addresses.
> + */
> + VIRTUAL_BUG_ON(x < (unsigned long) KERNEL_START || x > (unsigned long) KERNEL_END);
> + return (__x - kimage_voffset);
> +}
> +EXPORT_SYMBOL(__phys_addr_symbol);
Similarly:
phys_addr_t __phys_addr_symbol(unsigned long x)
{
/*
* This is bounds checking against the kernel image only.
* __pa_symbol should only be used on kernel symbol addresses.
*/
VIRTUAL_BUG_ON(x < (unsigned long) KERNEL_START ||
x > (unsigned long) KERNEL_END);
return __pa_symbol_nodebug(x);
}
EXPORT_SYMBOL(__phys_addr_symbol);
Thanks,
Mark.
^ permalink raw reply
* [GIT PULL] Integrator DTS and defconfig changes
From: Olof Johansson @ 2016-11-18 17:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACRpkdYoornJx1fTN1C5OiR16w9DbTcr9ze4gc9PLkt7LCqqCQ@mail.gmail.com>
On Fri, Nov 18, 2016 at 09:26:56AM +0100, Linus Walleij wrote:
> On Fri, Nov 18, 2016 at 8:25 AM, Olof Johansson <olof@lixom.net> wrote:
>
> > I also sympathize that it's extra annoying having to split just three
> > patches across two branches. So, if it's easier we can just cherry-pick
> > apart the patches here across the branches (your comment about next
> > coverage makes me suspect you have no direct downstream users of this
> > branch). If that's OK, let me know and I'll do that tomorrow.
>
> It's fine to cherry-pick, I can also send two separate branches (or
> just 2+1 stand-alone patches).
>
> Any way you like it :) just tell me what to do, all I want is to get this
> in for v4.10.
Cherry-picking is actually faster on our side than merging, so I've done that
now. Queued into next/dt and next/defconfig for 4.10.
Thanks!
-Olof
^ permalink raw reply
* [PATCH] arm: spin one more cycle in timer-based delays
From: Mason @ 2016-11-18 17:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <582F0DD2.3030805@free.fr>
On 18/11/2016 15:18, Mason wrote:
> On 18/11/2016 13:54, Russell King - ARM Linux wrote:
>> On Fri, Nov 18, 2016 at 12:06:30PM +0000, Will Deacon wrote:
>>> On Tue, Nov 15, 2016 at 02:36:33PM +0100, Mason wrote:
>>>> When polling a tick counter in a busy loop, one might sample the
>>>> counter just *before* it is updated, and then again just *after*
>>>> it is updated. In that case, while mathematically v2-v1 equals 1,
>>>> only epsilon has really passed.
>>>>
>>>> So, if a caller requests an N-cycle delay, we spin until v2-v1
>>>> is strictly greater than N to avoid these random corner cases.
>>>>
>>>> Signed-off-by: Mason <slash.tmp@free.fr>
>>>> ---
>>>> arch/arm/lib/delay.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm/lib/delay.c b/arch/arm/lib/delay.c
>>>> index 69aad80a3af4..3f1cd15ca102 100644
>>>> --- a/arch/arm/lib/delay.c
>>>> +++ b/arch/arm/lib/delay.c
>>>> @@ -60,7 +60,7 @@ static void __timer_delay(unsigned long cycles)
>>>> {
>>>> cycles_t start = get_cycles();
>>>>
>>>> - while ((get_cycles() - start) < cycles)
>>>> + while ((get_cycles() - start) <= cycles)
>>>> cpu_relax();
>>>> }
>>>
>>> I thought a bit about this last night. It is well known that the delay
>>> routines are not guaranteed to be accurate, and I don't *think* that's
>>> limited to over-spinning, so arguably this isn't a bug. However, taking
>>> the approach that "drivers should figure it out" is also unhelpful,
>>> because the frequency of the underlying counter isn't generally known
>>> and so drivers can't simply adjust the delay parameter.
>>
>> I don't think this change makes any sense whatsoever. udelay() is
>> inaccurate, period. It _will_ give delays shorter than requested
>> for many reasons, many of which can't be fixed.
>
> If I understand correctly, udelay is, in fact, a wrapper around
> several possible implementations. (Run-time decision on arch/arm)
>
> 1) The "historical" software loop-based method, which is calibrated
> during init.
>
> 2) A hardware solution, based on an actual timer, ticking away
> at a constant frequency.
>
> 3) others?
>
> Solution 1 (which probably dates back to Linux 0.1) suffers from
> the inaccuracies you point out, because of interrupt latency
> during calibration, DVFS, etc.
>
> On the other hand, it is simple enough to implement solution 2
> in a way that guarantees that spin_for_n_cycles(n) spins
> /at least/ for n cycles.
>
> On platforms using timer-based delays, there can indeed exist
> a function guaranteeing a minimal delay.
>
>
>> Having a super-accurate version just encourages people to write broken
>> drivers which assume (eg) that udelay(10) will give _at least_ a 10us
>> delay. However, there is no such guarantee.
>
> You say that calling udelay(10) expecting at least 10 ?s is broken.
> This fails the principle of least astonishment in a pretty big way.
> (If it returns after 9.9 ?s, I suppose it could be OK. But for
> shorter delays, the relative error grows.)
>
> A lot of drivers implement a spec that says
> "do this, wait 1 ?s, do that".
>
> Driver writers would typically write
> do_this(); udelay(1); do_that();
>
> Do you suggest they should write udelay(2); ?
> But then, it's not so easy to follow the spec because everything
> has been translated to a different number.
> Hide everything behind a macro?
>
> Note that people have been using ndelay too.
> (I count 182 in drivers, 288 overall.)
>
> drivers/cpufreq/s3c2440-cpufreq.c: ndelay(20);
>
> I don't think the author expects this to return immediately.
>
>
>> So, having udelay() for timers return slightly short is actually a good
>> thing - it causes people not to make the mistake to be soo accurate
>> with their delay specifications.
>
> I disagree that having an implementation which introduces
> hard-to-track-down random heisenbugs is a good thing.
>
>
>> So, NAK on this change. udelay is not super-accurate.
>
> usleep_range() fixed this issue recently.
> 6c5e9059692567740a4ee51530dffe51a4b9584d
> https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=timers/core&id=6c5e9059692567740a4ee51530dffe51a4b9584d
>
> Do you suggest driver writers should use usleep_range()
> instead of udelay?
> (When does usleep_range start making sense? Around 50/100 ?s
> and above?)
>
>> Reference (and this is the most important one):
>>
>> http://lists.openwall.net/linux-kernel/2011/01/12/372
I forgot to explain how I came to work in this area of the code.
When my NAND controller's driver probes, it scans the chips
for bad blocks, which generates 65000 calls to ndelay(100);
(for 1 GB of NAND). For larger sizes of NAND, the number can
climb to 500k calls.
Currently, on arch/arm, ndelay is rounded *up* to the
nearest microsecond. So this might generate a total
delay of up to 500 ms, when 50 ms would be sufficient.
So I locally defined ndelay as
#define NDELAY_MULT ((UL(2199) * HZ) >> 11)
#define ndelay(n) __const_udelay((n) * NDELAY_MULT)
I use a 27 MHz tick counter (37 ns period).
ndelay() was resolving to __timer_delay(2)
which might delay only a single tick, so 37 ns.
So the NAND framework occasionally (falsely) flags a block
as bad (random).
There are two sources of error in the timer-based code:
1) rounding down in __timer_const_udelay => loses 1 cycle
2) spinning one cycle too few => loses 1 cycle
With these two errors corrected, I am able to init the
NAND driver faster, with no blocks incorrectly marked
as bad.
Regards.
^ permalink raw reply
* [PATCH v5 0/3] driver: Add DT support for DA8xx
From: Bin Liu @ 2016-11-18 17:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479293545-7516-1-git-send-email-abailon@baylibre.com>
On Wed, Nov 16, 2016 at 11:52:22AM +0100, Alexandre Bailon wrote:
> Changes in v2:
> * Remove unrelated changes in patch 3
> * Rename the device node in patch 4
>
> Changes in v3:
> * Fix few mistakes in DT binding sample
> * Only build the device table if DT is enabled
>
> Change in v4:
> * Fix a nit
>
> Changes in v5:
> * Nothing. Resent the v4 in two seppaated series: one for platform and one for
> driver.
>
> Petr Kulhavy (3):
> dt/bindings: Add binding for the DA8xx MUSB driver
> usb: musb: core: added helper function for parsing DT
> usb: musb: da8xx: Add DT support for the DA8xx driver
Applied. Thanks.
-Bin.
>
> .../devicetree/bindings/usb/da8xx-usb.txt | 43 ++++++++++++++++++++
> drivers/usb/musb/da8xx.c | 46 ++++++++++++++++++++++
> drivers/usb/musb/musb_core.c | 19 +++++++++
> drivers/usb/musb/musb_core.h | 6 +++
> 4 files changed, 114 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/usb/da8xx-usb.txt
>
> --
> 2.7.3
>
^ permalink raw reply
* [PATCH v8 13/16] drivers: iommu: arm-smmu: add IORT configuration
From: Robin Murphy @ 2016-11-18 17:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161116152936.22955-14-lorenzo.pieralisi@arm.com>
On 16/11/16 15:29, Lorenzo Pieralisi wrote:
> In ACPI bases systems, in order to be able to create platform
based?
> devices and initialize them for ARM SMMU components, the IORT
> kernel implementation requires a set of static functions to be
> used by the IORT kernel layer to configure platform devices for
> ARM SMMU components.
>
> Add static configuration functions to the IORT kernel layer for
> the ARM SMMU components, so that the ARM SMMU driver can
> initialize its respective platform device by relying on the IORT
> kernel infrastructure and by adding a corresponding ACPI device
> early probe section entry.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Reviewed-by: Tomasz Nowicki <tn@semihalf.com>
> Tested-by: Hanjun Guo <hanjun.guo@linaro.org>
> Tested-by: Tomasz Nowicki <tn@semihalf.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> ---
> drivers/acpi/arm64/iort.c | 81 +++++++++++++++++++++++++++++++++++++++++++++
> drivers/iommu/arm-smmu.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++-
> include/linux/acpi_iort.h | 3 ++
> 3 files changed, 166 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index fd52e4c..4708806 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -548,6 +548,78 @@ static bool __init arm_smmu_v3_is_coherent(struct acpi_iort_node *node)
> return smmu->flags & ACPI_IORT_SMMU_V3_COHACC_OVERRIDE;
> }
>
> +static int __init arm_smmu_count_resources(struct acpi_iort_node *node)
> +{
> + struct acpi_iort_smmu *smmu;
> + int num_irqs;
> + u64 *glb_irq;
> +
> + /* Retrieve SMMU specific data */
> + smmu = (struct acpi_iort_smmu *)node->node_data;
> +
> + glb_irq = ACPI_ADD_PTR(u64, node, smmu->global_interrupt_offset);
> + if (!IORT_IRQ_MASK(glb_irq[1])) /* 0 means not implemented */
> + num_irqs = 1;
> + else
> + num_irqs = 2;
Do we actually need this - I mean, the configuration access interrupt is
of somewhat limited utility, implementation-defined, and we don't have
any handling for it. Nor should it, if present, ever happen anyway,
since it's not like anyone else should be randomly poking our SMMU in
invalid ways. Can we simply ignore it?
> +
> + num_irqs += smmu->context_interrupt_count;
> +
> + return num_irqs + 1;
> +}
> +
> +static void __init arm_smmu_init_resources(struct resource *res,
> + struct acpi_iort_node *node)
> +{
> + struct acpi_iort_smmu *smmu;
> + int i, hw_irq, trigger, num_res = 0;
> + u64 *ctx_irq, *glb_irq;
> +
> + /* Retrieve SMMU specific data */
> + smmu = (struct acpi_iort_smmu *)node->node_data;
> +
> + res[num_res].start = smmu->base_address;
> + res[num_res].end = smmu->base_address + smmu->span - 1;
> + res[num_res].flags = IORESOURCE_MEM;
> + num_res++;
> +
> + glb_irq = ACPI_ADD_PTR(u64, node, smmu->global_interrupt_offset);
> + /* Global IRQs */
> + hw_irq = IORT_IRQ_MASK(glb_irq[0]);
> + trigger = IORT_IRQ_TRIGGER_MASK(glb_irq[0]);
> +
> + acpi_iort_register_irq(hw_irq, "arm-smmu-global", trigger,
> + &res[num_res++]);
> +
> + /* Global IRQs */
> + hw_irq = IORT_IRQ_MASK(glb_irq[1]);
> + if (hw_irq) {
> + trigger = IORT_IRQ_TRIGGER_MASK(glb_irq[1]);
> + acpi_iort_register_irq(hw_irq, "arm-smmu-global", trigger,
> + &res[num_res++]);
> + }
Related to the above, I think the driver generally assumes these to be
the global fault interrupt. If we *are* going to claim the config
interrupt as well, we should probably disambiguate them, although
admittedly we can't really do that retrospectively on the DT side.
> +
> + /* Context IRQs */
> + ctx_irq = ACPI_ADD_PTR(u64, node, smmu->context_interrupt_offset);
> + for (i = 0; i < smmu->context_interrupt_count; i++) {
> + hw_irq = IORT_IRQ_MASK(ctx_irq[i]);
> + trigger = IORT_IRQ_TRIGGER_MASK(ctx_irq[i]);
> +
> + acpi_iort_register_irq(hw_irq, "arm-smmu-context", trigger,
> + &res[num_res++]);
> + }
> +}
> +
> +static bool __init arm_smmu_is_coherent(struct acpi_iort_node *node)
> +{
> + struct acpi_iort_smmu *smmu;
> +
> + /* Retrieve SMMU specific data */
> + smmu = (struct acpi_iort_smmu *)node->node_data;
> +
> + return smmu->flags & ACPI_IORT_SMMU_COHERENT_WALK;
> +}
> +
> struct iort_iommu_config {
> const char *name;
> int (*iommu_init)(struct acpi_iort_node *node);
> @@ -564,12 +636,21 @@ static const struct iort_iommu_config iort_arm_smmu_v3_cfg __initconst = {
> .iommu_init_resources = arm_smmu_v3_init_resources
> };
>
> +static const struct iort_iommu_config iort_arm_smmu_cfg __initconst = {
> + .name = "arm-smmu",
> + .iommu_is_coherent = arm_smmu_is_coherent,
> + .iommu_count_resources = arm_smmu_count_resources,
> + .iommu_init_resources = arm_smmu_init_resources
> +};
> +
> static __init
> const struct iort_iommu_config *iort_get_iommu_cfg(struct acpi_iort_node *node)
> {
> switch (node->type) {
> case ACPI_IORT_NODE_SMMU_V3:
> return &iort_arm_smmu_v3_cfg;
> + case ACPI_IORT_NODE_SMMU:
> + return &iort_arm_smmu_cfg;
> default:
> return NULL;
> }
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 573b2b6..21d1892 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -28,6 +28,8 @@
>
> #define pr_fmt(fmt) "arm-smmu: " fmt
>
> +#include <linux/acpi.h>
> +#include <linux/acpi_iort.h>
> #include <linux/atomic.h>
> #include <linux/delay.h>
> #include <linux/dma-iommu.h>
> @@ -1904,6 +1906,70 @@ static const struct of_device_id arm_smmu_of_match[] = {
> };
> MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>
> +#ifdef CONFIG_ACPI
> +static int acpi_smmu_get_data(u32 model, u32 *version, u32 *impl)
> +{
> + int ret = 0;
> +
> + switch (model) {
> + case ACPI_IORT_SMMU_V1:
> + case ACPI_IORT_SMMU_CORELINK_MMU400:
> + *version = ARM_SMMU_V1;
> + *impl = GENERIC_SMMU;
Further to Will's comment, I'd say just pass the smmu pointer in and set
the fields explicitly here.
Robin.
> + break;
> + case ACPI_IORT_SMMU_V2:
> + *version = ARM_SMMU_V2;
> + *impl = GENERIC_SMMU;
> + break;
> + case ACPI_IORT_SMMU_CORELINK_MMU500:
> + *version = ARM_SMMU_V2;
> + *impl = ARM_MMU500;
> + break;
> + default:
> + ret = -ENODEV;
> + }
> +
> + return ret;
> +}
> +
> +static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
> + struct arm_smmu_device *smmu)
> +{
> + struct device *dev = smmu->dev;
> + struct acpi_iort_node *node =
> + *(struct acpi_iort_node **)dev_get_platdata(dev);
> + struct acpi_iort_smmu *iort_smmu;
> + u64 *glb_irq;
> + int ret;
> +
> + /* Retrieve SMMU1/2 specific data */
> + iort_smmu = (struct acpi_iort_smmu *)node->node_data;
> +
> + ret = acpi_smmu_get_data(iort_smmu->model, &smmu->version,
> + &smmu->model);
> + if (ret < 0)
> + return ret;
> +
> + glb_irq = ACPI_ADD_PTR(u64, node, iort_smmu->global_interrupt_offset);
> +
> + if (!IORT_IRQ_MASK(glb_irq[1])) /* 0 means not implemented */
> + smmu->num_global_irqs = 1;
> + else
> + smmu->num_global_irqs = 2;
> +
> + if (iort_smmu->flags & ACPI_IORT_SMMU_COHERENT_WALK)
> + smmu->features |= ARM_SMMU_FEAT_COHERENT_WALK;
> +
> + return 0;
> +}
> +#else
> +static inline int arm_smmu_device_acpi_probe(struct platform_device *pdev,
> + struct arm_smmu_device *smmu)
> +{
> + return -ENODEV;
> +}
> +#endif
> +
> static int arm_smmu_device_dt_probe(struct platform_device *pdev,
> struct arm_smmu_device *smmu)
> {
> @@ -1955,7 +2021,11 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> }
> smmu->dev = dev;
>
> - err = arm_smmu_device_dt_probe(pdev, smmu);
> + if (dev->of_node)
> + err = arm_smmu_device_dt_probe(pdev, smmu);
> + else
> + err = arm_smmu_device_acpi_probe(pdev, smmu);
> +
> if (err)
> return err;
>
> @@ -2103,6 +2173,17 @@ IOMMU_OF_DECLARE(arm_mmu401, "arm,mmu-401", arm_smmu_of_init);
> IOMMU_OF_DECLARE(arm_mmu500, "arm,mmu-500", arm_smmu_of_init);
> IOMMU_OF_DECLARE(cavium_smmuv2, "cavium,smmu-v2", arm_smmu_of_init);
>
> +#ifdef CONFIG_ACPI
> +static int __init arm_smmu_acpi_init(struct acpi_table_header *table)
> +{
> + if (iort_node_match(ACPI_IORT_NODE_SMMU))
> + return arm_smmu_init();
> +
> + return 0;
> +}
> +IORT_ACPI_DECLARE(arm_smmu, ACPI_SIG_IORT, arm_smmu_acpi_init);
> +#endif
> +
> MODULE_DESCRIPTION("IOMMU API for ARM architected SMMU implementations");
> MODULE_AUTHOR("Will Deacon <will.deacon@arm.com>");
> MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> index 17bb078..79ba1bb 100644
> --- a/include/linux/acpi_iort.h
> +++ b/include/linux/acpi_iort.h
> @@ -23,6 +23,9 @@
> #include <linux/fwnode.h>
> #include <linux/irqdomain.h>
>
> +#define IORT_IRQ_MASK(irq) (irq & 0xffffffffULL)
> +#define IORT_IRQ_TRIGGER_MASK(irq) ((irq >> 32) & 0xffffffffULL)
> +
> int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node);
> void iort_deregister_domain_token(int trans_id);
> struct fwnode_handle *iort_find_domain_token(int trans_id);
>
^ permalink raw reply
* [PATCH] arm: spin one more cycle in timer-based delays
From: Russell King - ARM Linux @ 2016-11-18 17:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAD=FV=VoycW9btkdKubq_Uxh52PYK5R1X2GD10DcxRQj2K-_UQ@mail.gmail.com>
On Fri, Nov 18, 2016 at 09:13:18AM -0800, Doug Anderson wrote:
> Mason's change adds no complexity to the code and seems like a
> generally good idea.
>
> As per John Stultz [1] there is agreement that udelay() really
> shouldn't delay less than the requested amount.
There is NO such agreement, read the reference that I gave, which is
a statement from Linus who clearly says that if you're stupid enough
to want udelay() to be accurate to within 5% then you're doing
something wrong.
FACT: software-based udelay()s are _always_ short by the very nature
of the way the delay loop is calibrated.
It's not something that's going to get fixed either - read the full
thread from my reference, and you'll see that Linus has said that
udelay() being short is not something anyone should care about.
That's at odds from John, and Linus' opinion rather trumps everyone
elses - it's Linus' kernel, his is the ultimate last say on anything.
> In fact, we even have
> a test committed to the tree: "kernel/time/test_udelay.c". As your
> reference says, maybe 1% isn't enough to throw a hissyfit about, but
> when the change is so simple and adds no complexity then it seems like
> a worthwhile thing to do.
Maybe, but my point is that it's just encouraging this fiction that
udelay() never delays shorter than the requested delay.
Also, given that this is architecture code, it's my decision to make,
not yours. So while you can supply any attributation you like, I'm
saying no at the moment - unless someone can show that it causes
_significant_ errors.
In other words, if it causes significantly short or long delays that
the reasonable inflation of delay value that drivers are expected to
make becomes a problem, then yes, it should be fixed. If it's merely
"lets stop udelay() returning slightly early" then no, I'm not
interested because it's encouraging the fiction.
And... if a data sheet says "needs a delay of 2us" and you put in the
driver udelay(2) then you're doing it wrong, and you need to read
Linus' mails on this subject, such as the one I've provided a link
to... that udelay() must be _at least_ udelay(3), if not 4.
The best thing when using udelay() is to remember the most important
point - udelay() is very _approximate_.
Adding Linus in case he wishes to add to the discussion.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* [PATCH 1/3] Documentation: dt: Add TI SCI clock driver
From: Rob Herring @ 2016-11-18 17:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <41c58712-bc00-ed05-9d1d-42e31397a70c@ti.com>
On Mon, Oct 31, 2016 at 7:50 AM, Tero Kristo <t-kristo@ti.com> wrote:
> On 30/10/16 22:41, Rob Herring wrote:
>>
>> On Fri, Oct 21, 2016 at 03:45:59PM +0300, Tero Kristo wrote:
>>>
>>> Add a clock implementation, TI SCI clock, that will hook to the common
>>> clock framework, and allow each clock to be controlled via TI SCI
>>> protocol.
>>>
>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>> ---
>>> .../devicetree/bindings/clock/ti,sci-clk.txt | 37
>>> ++++++++++++++++++++++
>>> MAINTAINERS | 1 +
>>> 2 files changed, 38 insertions(+)
>>> create mode 100644
>>> Documentation/devicetree/bindings/clock/ti,sci-clk.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/ti,sci-clk.txt
>>> b/Documentation/devicetree/bindings/clock/ti,sci-clk.txt
>>> new file mode 100644
>>> index 0000000..bfc3ca4
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/ti,sci-clk.txt
>>> @@ -0,0 +1,37 @@
>>> +Texas Instruments TI-SCI Clocks
>>> +===============================
>>> +
>>> +All clocks on Texas Instruments' SoCs that contain a System Controller,
>>> +are only controlled by this entity. Communication between a host
>>> processor
>>> +running an OS and the System Controller happens through a protocol known
>>> +as TI-SCI[1]. This clock implementation plugs into the common clock
>>> +framework and makes use of the TI-SCI protocol on clock API requests.
>>> +
>>> +[1] Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
>>> +
>>> +Required properties:
>>> +-------------------
>>> +- compatible: Must be "ti,k2g-sci-clk"
>>> +- #clock-cells: Shall be 2.
>>> + In clock consumers, this cell represents the device ID and clock ID
>>> + exposed by the PM firmware. The assignments can be found in the header
>>> + files <dt-bindings/genpd/<soc>.h> (which covers the device IDs) and
>>> + <dt-bindings/clock/<soc>.h> (which covers the clock IDs), where <soc>
>>> + is the SoC involved, for example 'k2g'.
>>> +
>>> +Examples:
>>> +--------
>>> +
>>> +pmmc: pmmc {
>>> + compatible = "ti,k2g-sci";
>>> +
>>> + k2g_clks: k2g_clks {
>>
>>
>> Use "clocks" for node name instead.
>>
>>> + compatible = "ti,k2g-sci-clk";
>>
>>
>> I'm starting to think all these child nodes for SCI are pointless. Is
>> there any reason why the parent node can't be the clock provider (along
>> with all the other providers it acks as)?
>
>
> I believe the only reason to keep them separate is to have kernel side of
> things modular. If we have separate nodes, the drivers can be probed
> separately.
>
> If not, we need to build one huge blob with all the features in it, so the
> main driver can probe everything in one go, with annoying back-and-forth
> callbacks in place (assuming we still want to keep stuff somehow modular.)
Since when is DT the only way to create a device? The main driver can
create devices for all the sub-functions like clocks. This is the same
as MFDs which have been done both ways.
Rob
^ permalink raw reply
* [PATCH] PCI: Add information about describing PCI in ACPI
From: Gabriele Paoloni @ 2016-11-18 17:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161117175938.17465.45820.stgit@bhelgaas-glaptop.roam.corp.google.com>
Hi Bjorn
Many thanks for putting this together, it really helps!
One thing below..
> -----Original Message-----
> From: linux-kernel-owner at vger.kernel.org [mailto:linux-kernel-
> owner at vger.kernel.org] On Behalf Of Bjorn Helgaas
> Sent: 17 November 2016 18:00
> To: linux-pci at vger.kernel.org
> Cc: linux-acpi at vger.kernel.org; linux-kernel at vger.kernel.org; linux-
> arm-kernel at lists.infradead.org; linaro-acpi at lists.linaro.org
> Subject: [PATCH] PCI: Add information about describing PCI in ACPI
>
> Add a writeup about how PCI host bridges should be described in ACPI
> using PNP0A03/PNP0A08 devices, PNP0C02 devices, and the MCFG table.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
> Documentation/PCI/00-INDEX | 2 +
> Documentation/PCI/acpi-info.txt | 136
> +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 138 insertions(+)
> create mode 100644 Documentation/PCI/acpi-info.txt
>
> diff --git a/Documentation/PCI/00-INDEX b/Documentation/PCI/00-INDEX
> index 147231f..0780280 100644
> --- a/Documentation/PCI/00-INDEX
> +++ b/Documentation/PCI/00-INDEX
> @@ -1,5 +1,7 @@
> 00-INDEX
> - this file
> +acpi-info.txt
> + - info on how PCI host bridges are represented in ACPI
> MSI-HOWTO.txt
> - the Message Signaled Interrupts (MSI) Driver Guide HOWTO and
> FAQ.
> PCIEBUS-HOWTO.txt
> diff --git a/Documentation/PCI/acpi-info.txt b/Documentation/PCI/acpi-
> info.txt
> new file mode 100644
> index 0000000..ccbcfda
> --- /dev/null
> +++ b/Documentation/PCI/acpi-info.txt
> @@ -0,0 +1,136 @@
> + ACPI considerations for PCI host bridges
> +
> +The basic requirement is that the ACPI namespace should describe
> +*everything* that consumes address space unless there's another
> +standard way for the OS to find it [1, 2]. ?For example, windows that
> +are forwarded to PCI by a PCI host bridge should be described via ACPI
> +devices, since the OS can't locate the host bridge by itself. ?PCI
> +devices *below* the host bridge do not need to be described via ACPI,
> +because the resources they consume are inside the host bridge windows,
> +and the OS can discover them via the standard PCI enumeration
> +mechanism (using config accesses to read and size the BARs).
> +
> +This ACPI resource description is done via _CRS methods of devices in
> +the ACPI namespace [2]. ? _CRS methods are like generalized PCI BARs:
> +the OS can read _CRS and figure out what resource is being consumed
> +even if it doesn't have a driver for the device [3]. ?That's important
> +because it means an old OS can work correctly even on a system with
> +new devices unknown to the OS. ?The new devices won't do anything, but
> +the OS can at least make sure no resources conflict with them.
> +
> +Static tables like MCFG, HPET, ECDT, etc., are *not* mechanisms for
> +reserving address space! The static tables are for things the OS
> +needs to know early in boot, before it can parse the ACPI namespace.
> +If a new table is defined, an old OS needs to operate correctly even
> +though it ignores the table. _CRS allows that because it is generic
> +and understood by the old OS; a static table does not.
Right so if my understanding is correct you are saying that resources
described in the MCFG table should also be declared in PNP0C02 devices
so that the PNP driver can reserve these resources.
On the other side the PCI Root bridge driver should not reserve such
resources.
Well if my understanding is correct I think we have a problem here:
http://lxr.free-electrons.com/source/drivers/pci/ecam.c#L74
As you can see pci_ecam_create() will conflict with the pnp driver
as it will try to reserve the resources from the MCFG table...
Maybe we need to rework pci_ecam_create() ?
Thanks
Gab
> +
> +If the OS is expected to manage an ACPI device, that device will have
> +a specific _HID/_CID that tells the OS what driver to bind to it, and
> +the _CRS tells the OS and the driver where the device's registers are.
> +
> +PNP0C02 "motherboard" devices are basically a catch-all. ?There's no
> +programming model for them other than "don't use these resources for
> +anything else." ?So any address space that is (1) not claimed by some
> +other ACPI device and (2) should not be assigned by the OS to
> +something else, should be claimed by a PNP0C02 _CRS method.
> +
> +PCI host bridges are PNP0A03 or PNP0A08 devices. ?Their _CRS should
> +describe all the address space they consume. ?In principle, this would
> +be all the windows they forward down to the PCI bus, as well as the
> +bridge registers themselves. ?The bridge registers include things like
> +secondary/subordinate bus registers that determine the bus range below
> +the bridge, window registers that describe the apertures, etc. ?These
> +are all device-specific, non-architected things, so the only way a
> +PNP0A03/PNP0A08 driver can manage them is via _PRS/_CRS/_SRS, which
> +contain the device-specific details. ?These bridge registers also
> +include ECAM space, since it is consumed by the bridge.
> +
> +ACPI defined a Producer/Consumer bit that was intended to distinguish
> +the bridge apertures from the bridge registers [4, 5]. ?However,
> +BIOSes didn't use that bit correctly, and the result is that OSes have
> +to assume that everything in a PCI host bridge _CRS is a window. ?That
> +leaves no way to describe the bridge registers in the PNP0A03/PNP0A08
> +device itself.
> +
> +The workaround is to describe the bridge registers (including ECAM
> +space) in PNP0C02 catch-all devices [6]. ?With the exception of ECAM,
> +the bridge register space is device-specific anyway, so the generic
> +PNP0A03/PNP0A08 driver (pci_root.c) has no need to know about it. ?For
> +ECAM, pci_root.c learns about the space from either MCFG or the _CBA
> +method.
> +
> +Note that the PCIe spec actually does require ECAM unless there's a
> +standard firmware interface for config access, e.g., the ia64 SAL
> +interface [7]. One reason is that we want a generic host bridge
> +driver (pci_root.c), and a generic driver requires a generic way to
> +access config space.
> +
> +
> +[1] ACPI 6.0, sec 6.1:
> + For any device that is on a non-enumerable type of bus (for
> + example, an ISA bus), OSPM enumerates the devices' identifier(s)
> + and the ACPI system firmware must supply an _HID object ... for
> + each device to enable OSPM to do that.
> +
> +[2] ACPI 6.0, sec 3.7:
> + The OS enumerates motherboard devices simply by reading through
> + the ACPI Namespace looking for devices with hardware IDs.
> +
> + Each device enumerated by ACPI includes ACPI-defined objects in
> + the ACPI Namespace that report the hardware resources the device
> + could occupy [_PRS], an object that reports the resources that are
> + currently used by the device [_CRS], and objects for configuring
> + those resources [_SRS]. The information is used by the Plug and
> + Play OS (OSPM) to configure the devices.
> +
> +[3] ACPI 6.0, sec 6.2:
> + OSPM uses device configuration objects to configure hardware
> + resources for devices enumerated via ACPI. Device configuration
> + objects provide information about current and possible resource
> + requirements, the relationship between shared resources, and
> + methods for configuring hardware resources.
> +
> + When OSPM enumerates a device, it calls _PRS to determine the
> + resource requirements of the device. It may also call _CRS to
> + find the current resource settings for the device. Using this
> + information, the Plug and Play system determines what resources
> + the device should consume and sets those resources by calling the
> + device?s _SRS control method.
> +
> + In ACPI, devices can consume resources (for example, legacy
> + keyboards), provide resources (for example, a proprietary PCI
> + bridge), or do both. Unless otherwise specified, resources for a
> + device are assumed to be taken from the nearest matching resource
> + above the device in the device hierarchy.
> +
> +[4] ACPI 6.0, sec 6.4.3.5.4:
> + Extended Address Space Descriptor
> + General Flags: Bit [0] Consumer/Producer:
> + 1?This device consumes this resource
> + 0?This device produces and consumes this resource
> +
> +[5] ACPI 6.0, sec 19.6.43:
> + ResourceUsage specifies whether the Memory range is consumed by
> + this device (ResourceConsumer) or passed on to child devices
> + (ResourceProducer). If nothing is specified, then
> + ResourceConsumer is assumed.
> +
> +[6] PCI Firmware 3.0, sec 4.1.2:
> + If the operating system does not natively comprehend reserving the
> + MMCFG region, the MMCFG region must be reserved by firmware. The
> + address range reported in the MCFG table or by _CBA method (see
> + Section 4.1.3) must be reserved by declaring a motherboard
> + resource. For most systems, the motherboard resource would appear
> + at the root of the ACPI namespace (under \_SB) in a node with a
> + _HID of EISAID (PNP0C02), and the resources in this case should
> + not be claimed in the root PCI bus?s _CRS. The resources can
> + optionally be returned in Int15 E820 or EFIGetMemoryMap as
> + reserved memory but must always be reported through ACPI as a
> + motherboard resource.
> +
> +[7] PCI Express 3.0, sec 7.2.2:
> + For systems that are PC-compatible, or that do not implement a
> + processor-architecture-specific firmware interface standard that
> + allows access to the Configuration Space, the ECAM is required as
> + defined in this section.
^ permalink raw reply
* [PATCH] arm: spin one more cycle in timer-based delays
From: Doug Anderson @ 2016-11-18 17:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118125409.GK1041@n2100.armlinux.org.uk>
Hi,
On Fri, Nov 18, 2016 at 4:54 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Fri, Nov 18, 2016 at 12:06:30PM +0000, Will Deacon wrote:
>> On Tue, Nov 15, 2016 at 02:36:33PM +0100, Mason wrote:
>> > When polling a tick counter in a busy loop, one might sample the
>> > counter just *before* it is updated, and then again just *after*
>> > it is updated. In that case, while mathematically v2-v1 equals 1,
>> > only epsilon has really passed.
>> >
>> > So, if a caller requests an N-cycle delay, we spin until v2-v1
>> > is strictly greater than N to avoid these random corner cases.
>> >
>> > Signed-off-by: Mason <slash.tmp@free.fr>
>> > ---
>> > arch/arm/lib/delay.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/arch/arm/lib/delay.c b/arch/arm/lib/delay.c
>> > index 69aad80a3af4..3f1cd15ca102 100644
>> > --- a/arch/arm/lib/delay.c
>> > +++ b/arch/arm/lib/delay.c
>> > @@ -60,7 +60,7 @@ static void __timer_delay(unsigned long cycles)
>> > {
>> > cycles_t start = get_cycles();
>> >
>> > - while ((get_cycles() - start) < cycles)
>> > + while ((get_cycles() - start) <= cycles)
>> > cpu_relax();
>> > }
>>
>> I thought a bit about this last night. It is well known that the delay
>> routines are not guaranteed to be accurate, and I don't *think* that's
>> limited to over-spinning, so arguably this isn't a bug. However, taking
>> the approach that "drivers should figure it out" is also unhelpful,
>> because the frequency of the underlying counter isn't generally known
>> and so drivers can't simply adjust the delay parameter.
>
> I don't think this change makes any sense whatsoever. udelay() is
> inaccurate, period. It _will_ give delays shorter than requested
> for many reasons, many of which can't be fixed.
>
> Having a super-accurate version just encourages people to write broken
> drivers which assume (eg) that udelay(10) will give _at least_ a 10us
> delay. However, there is no such guarantee.
>
> So, having udelay() for timers return slightly short is actually a good
> thing - it causes people not to make the mistake to be soo accurate
> with their delay specifications.
>
> So, NAK on this change. udelay is not super-accurate.
>
> Reference (and this is the most important one):
>
> http://lists.openwall.net/linux-kernel/2011/01/12/372
Mason's change adds no complexity to the code and seems like a
generally good idea.
As per John Stultz [1] there is agreement that udelay() really
shouldn't delay less than the requested amount. In fact, we even have
a test committed to the tree: "kernel/time/test_udelay.c". As your
reference says, maybe 1% isn't enough to throw a hissyfit about, but
when the change is so simple and adds no complexity then it seems like
a worthwhile thing to do. Why make things inaccurate for no reason?
[1] http://www.gossamer-threads.com/lists/linux/kernel/1918249
For what it's worth, Mason's change is:
Reviewed-by: Douglas Anderson <dianders@chromium.org>
-Doug
^ permalink raw reply
* [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06
From: Gabriele Paoloni @ 2016-11-18 17:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <2364530.A9QSbaqvfm@wuerfel>
> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd at arndb.de]
> Sent: 18 November 2016 16:35
> To: Gabriele Paoloni
> Cc: linux-arm-kernel at lists.infradead.org; mark.rutland at arm.com;
> benh at kernel.crashing.org; catalin.marinas at arm.com; liviu.dudau at arm.com;
> Linuxarm; lorenzo.pieralisi at arm.com; xuwei (O); Jason Gunthorpe; linux-
> serial at vger.kernel.org; linux-pci at vger.kernel.org;
> devicetree at vger.kernel.org; minyard at acm.org; will.deacon at arm.com; John
> Garry; zourongrong at gmail.com; robh+dt at kernel.org; bhelgaas at go og
> le.com; kantyzc at 163.com; zhichang.yuan02 at gmail.com; T homas Petazzoni;
> linux-kernel at vger.kernel.org; Yuanzhichang; olof at lixom.net
> Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> Hip06
>
> On Friday, November 18, 2016 4:18:07 PM CET Gabriele Paoloni wrote:
> > From: Arnd Bergmann [mailto:arnd at arndb.de]
> > > On Friday, November 18, 2016 12:53:08 PM CET Gabriele Paoloni
> wrote:
> > > > From: Arnd Bergmann [mailto:arnd at arndb.de]
> > > > > On Friday, November 18, 2016 12:07:28 PM CET Gabriele Paoloni
> > > > > The easiest change compared to the v5 code would be to walk
> > > > > a linked list of 'struct extio_ops' structures rather than
> > > > > assuming there is only ever one of them. I think one of the
> > > > > earlier versions actually did this.
> > > >
> > > > Right but if my understanding is correct if we live in a multi-
> > > > domain I/O space world when you have an input addr in the I/O
> > > > accessors this addr can be duplicated (for example for the
> standard
> > > > PCI IO domain and for our special LPC domain).
> > > > So effectively even if you walk a linked list there is a problem
> > > > of disambiguation...am I right?
> > >
> > > No, unlike the PCI memory space, the PIO addresses are not
> > > usually distinct, i.e. every PCI bus has its own 64K I/O
> > > addresses starting at zero. We linearize them into the
> > > Linux I/O space using the per-domain io_offset value.
> >
> > I am going to summarize my understanding here below:
> >
> > It seems to me that what is linearized is the virtual address
> > space associated to the IO address space. This address space
> > goes from PCI_IOBASE to (PCI_IOBASE + IO_SPACE_LIMIT).
> >
> > The I/O accessors perform rd/wr operation on this address
> > space using a port IO token.
> >
> > Each token map into a cpu physical address range
> > Each cpu physical address range maps to a bus address range
> > if the bus controller specifies a range property.
> >
> > Devices under a bus controller specify the bus addresses that
> > they operate on in their reg property.
> >
> > So each device can use the same bus addresses under two
> > separate bus controllers as long as the bus controller
> > use the range properties to map the same bus range to different
> > cpu address range.
>
> Sounds all correct to me so far, yes.
>
> > > For the ISA/LPC spaces there are only 4k of addresses, they
> > > the bus addresses always overlap, but we can trivially
> > > figure out the bus address from Linux I/O port number
> > > by subtracting the start of the range.
> >
> > Are you saying that our LPC controller should specify a
> > range property to map bus addresses into a cpu address range?
>
> No. There is not CPU address associated with it, because it's
> not memory mapped.
>
> Instead, we need to associate a bus address with a logical
> Linux port number, both in of_address_to_resource and
> in inb()/outb().
I think this is effectively what we are doing so far with patch 2/3.
The problem with this patch is that we are carving out a "forbidden"
IO tokens range that goes from 0 to PCIBIOS_MIN_IO.
I think that the proper solution would be to have the LPC driver to
set the carveout threshold used in pci_register_io_range(),
pci_pio_to_address(), pci_address_to_pio(), but this would impose
a probe dependency on the LPC itself that should be probed before
the PCI controller (or before any other devices calling these
functions...)
>
> > > > > Another option the IA64 approach mentioned in another subthread
> > > > > today, looking up the operations based on an index from the
> > > > > upper bits of the port number. If we do this, we probably
> > > > > want to do that for all PIO access and replace the entire
> > > > > virtual address remapping logic with that. I think Bjorn
> > > > > in the past argued in favor of such an approach, while I
> > > > > advocated the current scheme for simplicity based on how
> > > > > every I/O space these days is just memory mapped (which now
> > > > > turned out to be false, both on powerpc and arm64).
> > > >
> > > > This seems really complex...I am a bit worried that possibly
> > > > we end up in having the maintainers saying that it is not worth
> > > > to re-invent the world just for this special LPC device...
> > >
> > > It would clearly be a rather invasive change, but the
> > > end-result isn't necessarily more complex than what we
> > > have today, as we'd kill off the crazy pci_pio_to_address()
> > > and pci_address_to_pio() hacks in address translation.
> >
> > I have to look better into this...can you provide me a reference
> > to the Bjorn argument in favor of this approach?
>
> The thread seems to have been pci: Introduce pci_register_io_range()
> helper function, e.g. in https://lkml.org/lkml/2014/7/8/969
Ok many thanks I am going to look at it
>
> > > > To be honest with you I would keep things simple for this
> > > > LPC and introduce more complex reworks later if more devices
> > > > need to be introduced.
> > > >
> > > > What if we stick on a single domain now where we introduce a
> > > > reserved threshold for the IO space (say INDIRECT_MAX_IO).
> > >
> > > I said having a single domain is fine, but I still don't
> > > like the idea of reserving low port numbers for this hack,
> > > it would mean that the numbers change for everyone else.
> >
> > I don't get this much...I/O tokens that are passed to the I/O
> > accessors are not fixed anyway and they vary depending on the order
> > of adding ranges to io_range_list...so I don't see a big issue
> > with this...
>
> On machines with a legacy devices behind the PCI bridge,
> there may still be a reason to have the low I/O port range
> reserved for the primary bus, e.g. to get a VGA text console
> to work.
>
> On powerpc, this is called the "primary" PCI host, i.e. the
> only one that is allowed to have an ISA bridge.
Yes but
1) isn't the PCI controller range property that defines how IO bus address
map into physical CPU addresses?
2) How can you guarantee that the cpu range associated with this
IO bus range is the first to be registered in pci_register_io_range()?
( i.e. are you saying that they are just relying on the fact that it is the
only IO range in the system and by chance the IO tokens and corresponding
bus addresses are the same? )
Thanks
Gab
>
> Arnd
^ permalink raw reply
* [PATCH v8 6/7] arm/arm64: vgic: Implement VGICv3 CPU interface access
From: Vijay Kilari @ 2016-11-18 16:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161117160951.GB23588@cbox>
On Thu, Nov 17, 2016 at 9:39 PM, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
>> On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
>> <christoffer.dall@linaro.org> wrote:
>> > On Fri, Nov 04, 2016 at 04:43:32PM +0530, vijay.kilari at gmail.com wrote:
>> >> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>> >>
>> >> VGICv3 CPU interface registers are accessed using
>> >> KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
>> >> as 64-bit. The cpu MPIDR value is passed along with register id.
>> >> is used to identify the cpu for registers access.
>> >>
>> >> The version of VGIC v3 specification is define here
>> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>> >>
>> >> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
>> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>> >> ---
>> >> arch/arm64/include/uapi/asm/kvm.h | 3 +
>> >> arch/arm64/kvm/Makefile | 1 +
>> >> include/kvm/arm_vgic.h | 9 +
>> >> virt/kvm/arm/vgic/vgic-kvm-device.c | 27 +++
>> >> virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 +++
>> >> virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 ++++++++++++++++++++++++++++++++++++
>> >> virt/kvm/arm/vgic/vgic-v3.c | 8 +
>> >> virt/kvm/arm/vgic/vgic.h | 4 +
>> >> 8 files changed, 395 insertions(+)
>> >>
>> >> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
>> >> index 56dc08d..91c7137 100644
>> >> --- a/arch/arm64/include/uapi/asm/kvm.h
>> >> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> >> @@ -206,9 +206,12 @@ struct kvm_arch_memory_slot {
>> >> (0xffffffffULL << KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
>> >> #define KVM_DEV_ARM_VGIC_OFFSET_SHIFT 0
>> >> #define KVM_DEV_ARM_VGIC_OFFSET_MASK (0xffffffffULL << KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
>> >> +#define KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK (0xffff)
>> >> #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
>> >> #define KVM_DEV_ARM_VGIC_GRP_CTRL 4
>> >> #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
>> >> +#define KVM_DEV_ARM_VGIC_CPU_SYSREGS 6
>> >> +
>> >> #define KVM_DEV_ARM_VGIC_CTRL_INIT 0
>> >>
>> >> /* Device Control API on vcpu fd */
>> >> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> >> index d50a82a..1a14e29 100644
>> >> --- a/arch/arm64/kvm/Makefile
>> >> +++ b/arch/arm64/kvm/Makefile
>> >> @@ -32,5 +32,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v3.o
>> >> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
>> >> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
>> >> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
>> >> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
>> >
>> > Thi is making me wonder: Are we properly handling GICv3 save/restore
>> > for AArch32 now that we have GICv3 support for AArch32? By properly I
>> > mean that either it is clearly only supported on AArch64 systems or it's
>> > supported on both AArch64 and AArch32, but it shouldn't break randomly
>> > on AArch32.
>>
>> It supports both AArch64 and AArch64 in handling of system registers
>> save/restore.
>> All system registers that we save/restore are 32-bit for both aarch64
>> and aarch32.
>> Though opcode op0 should be zero for aarch32, the remaining Op and CRn codes
>> are same. However the codes sent by qemu is matched and register
>> are handled properly irrespective of AArch32 or AArch64.
>>
>> I don't have platform which support AArch32 guests to verify.
>
> Actually this is not about the guest, it's about an ARMv8 AArch32 host
> that has a GICv3.
>
> I just tried to do a v7 compile with your patches, and it results in an
> epic failure, so there's something for you to look at.
>
>> >
>> >> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
>> >> kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
>> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> >> index 002f092..730a18a 100644
>> >> --- a/include/kvm/arm_vgic.h
>> >> +++ b/include/kvm/arm_vgic.h
>> >> @@ -71,6 +71,9 @@ struct vgic_global {
>> >>
>> >> /* GIC system register CPU interface */
>> >> struct static_key_false gicv3_cpuif;
>> >> +
>> >> + /* Cache ICH_VTR_EL2 reg value */
>> >> + u32 ich_vtr_el2;
>> >> };
>> >>
>> >> extern struct vgic_global kvm_vgic_global_state;
>> >> @@ -269,6 +272,12 @@ struct vgic_cpu {
>> >> u64 pendbaser;
>> >>
>> >> bool lpis_enabled;
>> >> +
>> >> + /* Cache guest priority bits */
>> >> + u32 num_pri_bits;
>> >> +
>> >> + /* Cache guest interrupt ID bits */
>> >> + u32 num_id_bits;
>> >> };
>> >>
>> >> extern struct static_key_false vgic_v2_cpuif_trap;
>> >> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
>> >> index 6c7d30c..da532d1 100644
>> >> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
>> >> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
>> >> @@ -504,6 +504,14 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
>> >> if (!is_write)
>> >> *reg = tmp32;
>> >> break;
>> >> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
>> >> + u64 regid;
>> >> +
>> >> + regid = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK);
>> >> + ret = vgic_v3_cpu_sysregs_uaccess(vcpu, is_write,
>> >> + regid, reg);
>> >> + break;
>> >> + }
>> >> default:
>> >> ret = -EINVAL;
>> >> break;
>> >> @@ -537,6 +545,15 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
>> >> reg = tmp32;
>> >> return vgic_v3_attr_regs_access(dev, attr, ®, true);
>> >> }
>> >> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
>> >> + u64 __user *uaddr = (u64 __user *)(long)attr->addr;
>> >> + u64 reg;
>> >> +
>> >> + if (get_user(reg, uaddr))
>> >> + return -EFAULT;
>> >> +
>> >> + return vgic_v3_attr_regs_access(dev, attr, ®, true);
>> >> + }
>> >> }
>> >> return -ENXIO;
>> >> }
>> >> @@ -563,6 +580,15 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
>> >> tmp32 = reg;
>> >> return put_user(tmp32, uaddr);
>> >> }
>> >> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
>> >> + u64 __user *uaddr = (u64 __user *)(long)attr->addr;
>> >> + u64 reg;
>> >> +
>> >> + ret = vgic_v3_attr_regs_access(dev, attr, ®, false);
>> >> + if (ret)
>> >> + return ret;
>> >> + return put_user(reg, uaddr);
>> >> + }
>> >> }
>> >>
>> >> return -ENXIO;
>> >> @@ -581,6 +607,7 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
>> >> break;
>> >> case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
>> >> case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
>> >> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS:
>> >> return vgic_v3_has_attr_regs(dev, attr);
>> >> case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
>> >> return 0;
>> >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> >> index b35fb83..519b919 100644
>> >> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> >> @@ -23,6 +23,7 @@
>> >>
>> >> #include "vgic.h"
>> >> #include "vgic-mmio.h"
>> >> +#include "sys_regs.h"
>> >>
>> >> /* extract @num bytes at @offset bytes offset in data */
>> >> unsigned long extract_bytes(u64 data, unsigned int offset,
>> >> @@ -639,6 +640,24 @@ int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
>> >> nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers);
>> >> break;
>> >> }
>> >> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
>> >> + u64 reg, id;
>> >> + unsigned long vgic_mpidr, mpidr_reg;
>> >> + struct kvm_vcpu *vcpu;
>> >> +
>> >> + vgic_mpidr = (attr->attr & KVM_DEV_ARM_VGIC_V3_MPIDR_MASK) >>
>> >> + KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT;
>> >> +
>> >> + /* Convert plain mpidr value to MPIDR reg format */
>> >> + mpidr_reg = VGIC_TO_MPIDR(vgic_mpidr);
>> >> +
>> >> + vcpu = kvm_mpidr_to_vcpu(dev->kvm, mpidr_reg);
>> >> + if (!vcpu)
>> >> + return -EINVAL;
>> >> +
>> >> + id = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK);
>> >> + return vgic_v3_has_cpu_sysregs_attr(vcpu, 0, id, ®);
>> >> + }
>> >> default:
>> >> return -ENXIO;
>> >> }
>> >> diff --git a/virt/kvm/arm/vgic/vgic-sys-reg-v3.c b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
>> >> new file mode 100644
>> >> index 0000000..69d8597
>> >> --- /dev/null
>> >> +++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
>> >
>> > Shouldn't we have a GPL header here?
>> >
>> >> @@ -0,0 +1,324 @@
>> >> +#include <linux/irqchip/arm-gic-v3.h>
>> >> +#include <linux/kvm.h>
>> >> +#include <linux/kvm_host.h>
>> >> +#include <kvm/iodev.h>
>> >> +#include <kvm/arm_vgic.h>
>> >> +#include <asm/kvm_emulate.h>
>> >> +#include <asm/kvm_arm.h>
>> >> +#include <asm/kvm_mmu.h>
>> >> +
>> >> +#include "vgic.h"
>> >> +#include "vgic-mmio.h"
>> >> +#include "sys_regs.h"
>> >> +
>> >> +static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> >> + const struct sys_reg_desc *r)
>> >> +{
>> >> + struct vgic_cpu *vgic_v3_cpu = &vcpu->arch.vgic_cpu;
>> >> + struct vgic_vmcr vmcr;
>> >> + u64 val;
>> >> + u32 num_pri_bits, num_id_bits;
>> >> +
>> >> + vgic_get_vmcr(vcpu, &vmcr);
>> >> + if (p->is_write) {
>> >> + val = p->regval;
>> >> +
>> >> + /*
>> >> + * Does not allow update of ICC_CTLR_EL1 if HW does not support
>> >> + * guest programmed ID and PRI bits
>> >> + */
>> >
>> > I would suggest rewording this comment:
>> > Disallow restoring VM state not supported by this hardware.
>> >
>> >> + num_pri_bits = ((val & ICC_CTLR_EL1_PRI_BITS_MASK) >>
>> >> + ICC_CTLR_EL1_PRI_BITS_SHIFT) + 1;
>> >> + if (num_pri_bits > vgic_v3_cpu->num_pri_bits)
>> >> + return false;
>> >> +
>> >> + vgic_v3_cpu->num_pri_bits = num_pri_bits;
>> >
>> > hmmm, this looks weird to me, because vgic_v3_cpu->num_pri_bits I don't
>> > understand which effect this is intended to have?
>> >
>> > Sure, it may limit what you do with other registers later, but since
>> > there's no ordering requirement that the ctlr be restored first, I'm not
>> > sure it makes sense.
>> >
>> > Also, since this field is RO in the ICH_VTR, we'll have a strange
>> > situation during runtime after a GICv3 restore where the
>> > vgic_v3_cpu->num_pri_its differs from the hardware's ICH_VTR_EL2 field,
>> > which is never the case if you didn't do a save/restore.
>>
>> Yes, but in any case, vgic_v3_cpu->num_pri_bits will be always less
>> than HW supported
>> value.
>>
>
> So answer my question: What is the intended effect of writing this
> value? Is it just so that if you migrate this platform back again, then
> you're checking compatibility with what the guest would potentially do,
Yes and also to limit the valid aprn registers access as you said above.
But that has ordering restriction. Which I think we should follow.
> or should you maintain the num_pri_bits limitation during runtime
> somehow?
Once after checking compatibility, at runtime it is not updated
and this value is not used at all in VGIC further
>
>> >
>> > Finally, should we somehow ensure that this field is set to the same
>> > value across VCPUs or is that not an architectural requirement?
>> >
>> Yes it is nice to have it same across VCPUs. But should be ok as
>> we are ensuring value is not greater than HW supported value.
>
> Does the architecture allow having a different number of priority bits
> supported across CPUs? If not, you shouldn't allow a VM programming
> things that way either.
AFAIK, architecturally it is not mentioned any where in the spec that priority
bits should be same across CPUs.
>
>> There is no single point of place where we can make such a check
>>
>> >> +
>> >> + num_id_bits = (val & ICC_CTLR_EL1_ID_BITS_MASK) >>
>> >> + ICC_CTLR_EL1_ID_BITS_SHIFT;
>> >> + if (num_id_bits > vgic_v3_cpu->num_id_bits)
>> >> + return false;
>> >> +
>> >> + vgic_v3_cpu->num_id_bits = num_id_bits;
>> >
>> > same questions
>> >
>> >> +
>> >> + vmcr.ctlr &= ~(ICH_VMCR_CBPR_MASK | ICH_VMCR_EOIM_MASK);
>> >> + vmcr.ctlr |= ((val & ICC_CTLR_EL1_CBPR_MASK) >>
>> >> + ICC_CTLR_EL1_CBPR_SHIFT) << ICH_VMCR_CBPR_SHIFT;
>> >> + vmcr.ctlr |= ((val & ICC_CTLR_EL1_EOImode_MASK) >>
>> >> + ICC_CTLR_EL1_EOImode_SHIFT) <<
>> >> + ICH_VMCR_EOIM_SHIFT;
>> >
>> > I'm really confused here. Is the vmcr.ctlr field in the ICC_CTLR_EL1
>> > format or in the VMCR format? I would assume the former, since
>> > otherwise I don't get the point with this indirection, and for GICv2
>> > vmcr.ctlr captures the GICC_CTLR value and git_set_vmcr transforms this
>> > into VMCR values.
>> >
>> > Having a line that says "ctlr &= ~ICH_VMCR" should make some alarm bells
>> > ring.
>>
>> I will check and fix it.
>> >
>> >> + vgic_set_vmcr(vcpu, &vmcr);
>> >
>> > Should we check compatibility between the source and destination for the
>> > SEIS and A3V support here?
>>
>> Can be checked. But I feel A3V check makes more sense than checking for
>> SEIS.
>>
>
> Please argue the *why* for whatever you end up doing with respect to
> both bits in the commit message of your next patch revision.
>
>> >
>> >> + } else {
>> >> + val = 0;
>> >> + val |= (vgic_v3_cpu->num_pri_bits - 1) <<
>> >> + ICC_CTLR_EL1_PRI_BITS_SHIFT;
>> >> + val |= vgic_v3_cpu->num_id_bits <<
>> >> + ICC_CTLR_EL1_ID_BITS_SHIFT;
>> >> + val |= ((kvm_vgic_global_state.ich_vtr_el2 &
>> >> + ICH_VTR_SEIS_MASK) >> ICH_VTR_SEIS_SHIFT) <<
>> >> + ICC_CTLR_EL1_SEIS_SHIFT;
>> >> + val |= ((kvm_vgic_global_state.ich_vtr_el2 &
>> >> + ICH_VTR_A3V_MASK) >> ICH_VTR_A3V_SHIFT) <<
>> >> + ICC_CTLR_EL1_A3V_SHIFT;
>> >> + val |= ((vmcr.ctlr & ICH_VMCR_CBPR_MASK) >>
>> >> + ICH_VMCR_CBPR_SHIFT) << ICC_CTLR_EL1_CBPR_SHIFT;
>> >> + val |= ((vmcr.ctlr & ICH_VMCR_EOIM_MASK) >>
>> >> + ICH_VMCR_EOIM_SHIFT) << ICC_CTLR_EL1_EOImode_SHIFT;
>> >
>> > again, these last two look weird to me.
>> >
>> >> +
>> >> + p->regval = val;
>> >> + }
>> >> +
>> >> + return true;
>> >> +}
>> >> +
>> >> +static bool access_gic_pmr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> >> + const struct sys_reg_desc *r)
>> >> +{
>> >> + struct vgic_vmcr vmcr;
>> >> +
>> >> + vgic_get_vmcr(vcpu, &vmcr);
>> >> + if (p->is_write) {
>> >> + vmcr.pmr = (p->regval & ICC_PMR_EL1_MASK) >> ICC_PMR_EL1_SHIFT;
>> >> + vgic_set_vmcr(vcpu, &vmcr);
>> >> + } else {
>> >> + p->regval = (vmcr.pmr << ICC_PMR_EL1_SHIFT) & ICC_PMR_EL1_MASK;
>> >> + }
>> >> +
>> >> + return true;
>> >> +}
>> >> +
>> >> +static bool access_gic_bpr0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> >> + const struct sys_reg_desc *r)
>> >> +{
>> >> + struct vgic_vmcr vmcr;
>> >> +
>> >> + vgic_get_vmcr(vcpu, &vmcr);
>> >> + if (p->is_write) {
>> >> + vmcr.bpr = (p->regval & ICC_BPR0_EL1_MASK) >>
>> >> + ICC_BPR0_EL1_SHIFT;
>> >> + vgic_set_vmcr(vcpu, &vmcr);
>> >> + } else {
>> >> + p->regval = (vmcr.bpr << ICC_BPR0_EL1_SHIFT) &
>> >> + ICC_BPR0_EL1_MASK;
>> >> + }
>> >> +
>> >> + return true;
>> >> +}
>> >> +
>> >> +static bool access_gic_bpr1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> >> + const struct sys_reg_desc *r)
>> >> +{
>> >> + struct vgic_vmcr vmcr;
>> >> +
>> >> + if (!p->is_write)
>> >> + p->regval = 0;
>> >> +
>> >> + vgic_get_vmcr(vcpu, &vmcr);
>> >> + if (!((vmcr.ctlr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT)) {
>> >> + if (p->is_write) {
>> >> + vmcr.abpr = (p->regval & ICC_BPR1_EL1_MASK) >>
>> >> + ICC_BPR1_EL1_SHIFT;
>> >> + vgic_set_vmcr(vcpu, &vmcr);
>> >> + } else {
>> >> + p->regval = (vmcr.abpr << ICC_BPR1_EL1_SHIFT) &
>> >> + ICC_BPR1_EL1_MASK;
>> >> + }
>> >> + } else {
>> >> + if (!p->is_write)
>> >> + p->regval = min((vmcr.bpr + 1), 7U);
>> >> + }
>> >> +
>> >> + return true;
>> >> +}
>> >> +
>> >> +static bool access_gic_grpen0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> >> + const struct sys_reg_desc *r)
>> >> +{
>> >> + struct vgic_vmcr vmcr;
>> >> +
>> >> + vgic_get_vmcr(vcpu, &vmcr);
>> >> + if (p->is_write) {
>> >> + vmcr.grpen0 = (p->regval & ICC_IGRPEN0_EL1_MASK) >>
>> >> + ICC_IGRPEN0_EL1_SHIFT;
>> >> + vgic_set_vmcr(vcpu, &vmcr);
>> >> + } else {
>> >> + p->regval = (vmcr.grpen0 << ICC_IGRPEN0_EL1_SHIFT) &
>> >> + ICC_IGRPEN0_EL1_MASK;
>> >> + }
>> >> +
>> >> + return true;
>> >> +}
>> >> +
>> >> +static bool access_gic_grpen1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> >> + const struct sys_reg_desc *r)
>> >> +{
>> >> + struct vgic_vmcr vmcr;
>> >> +
>> >> + vgic_get_vmcr(vcpu, &vmcr);
>> >> + if (p->is_write) {
>> >> + vmcr.grpen1 = (p->regval & ICC_IGRPEN1_EL1_MASK) >>
>> >> + ICC_IGRPEN1_EL1_SHIFT;
>> >> + vgic_set_vmcr(vcpu, &vmcr);
>> >> + } else {
>> >> + p->regval = (vmcr.grpen1 << ICC_IGRPEN1_EL1_SHIFT) &
>> >> + ICC_IGRPEN1_EL1_MASK;
>> >> + }
>> >> +
>> >> + return true;
>> >> +}
>> >> +
>> >> +static void vgic_v3_access_apr_reg(struct kvm_vcpu *vcpu,
>> >> + struct sys_reg_params *p, u8 apr, u8 idx)
>> >> +{
>> >> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
>> >> + uint32_t *ap_reg;
>> >> +
>> >> + if (apr)
>> >> + ap_reg = &vgicv3->vgic_ap1r[idx];
>> >> + else
>> >> + ap_reg = &vgicv3->vgic_ap0r[idx];
>> >> +
>> >> + if (p->is_write)
>> >> + *ap_reg = p->regval;
>> >> + else
>> >> + p->regval = *ap_reg;
>> >> +}
>> >> +
>> >> +static void access_gic_aprn(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> >> + const struct sys_reg_desc *r, u8 apr)
>> >> +{
>> >> + struct vgic_cpu *vgic_v3_cpu = &vcpu->arch.vgic_cpu;
>> >> + u8 idx = r->Op2 & 3;
>> >> +
>> >> + switch (vgic_v3_cpu->num_pri_bits) {
>> >> + case 7:
>> >> + if (idx > 3)
>> >> + goto err;
>> >
>> > idx cannot be higher than three given the mask above, right?
>> >
>> >> + vgic_v3_access_apr_reg(vcpu, p, apr, idx);
>> >> + break;
>> >> + case 6:
>> >> + if (idx > 1)
>> >> + goto err;
>> >> + vgic_v3_access_apr_reg(vcpu, p, apr, idx);
>> >> + break;
>> >> + default:
>> >> + if (idx > 0)
>> >> + goto err;
>> >> + vgic_v3_access_apr_reg(vcpu, p, apr, idx);
>> >> + }
>> >
>> > what's the rationale behind ignoring the case where userspace is using
>> > unsupported priorities? Is it that this will be checked during
>> > save/restore of the ctlr?
>> >
>> > This sort of thing just looks like the case that's impossible to debug,
>> > because userspace could be scratching its head trying to understand why
>> > the value it wrote isn't recorded anywhere...
>> >
>> > If there's a good rationale for doing it this way, then could we have a
>> > comment to that effect?
>>
>> Accessing umplemented priority registers raised UNDEF exception.
>> So userspace accesing should be ignored instead of recording unsupported
>> values.
>>
>
> That's not what I asked.
>
> I asked why it's silently ignored as opposed to raising an error visible
> to user space?
>
>> >
>> >> +
>> >> + return;
>> >> +err:
>> >> + if (!p->is_write)
>> >> + p->regval = 0;
>> >> +}
>> >> +
>> >> +static bool access_gic_ap0r(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> >> + const struct sys_reg_desc *r)
>> >> +{
>> >> + access_gic_aprn(vcpu, p, r, 0);
>> >> +
>> >> + return true;
>> >> +}
>> >> +
>> >> +static bool access_gic_ap1r(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> >> + const struct sys_reg_desc *r)
>> >> +{
>> >> + access_gic_aprn(vcpu, p, r, 1);
>> >> +
>> >> + return true;
>> >> +}
>> >> +
>> >> +static bool access_gic_sre(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> >> + const struct sys_reg_desc *r)
>> >> +{
>> >> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
>> >> +
>> >> + /* Validate SRE bit */
>> >> + if (p->is_write) {
>> >> + if (!(p->regval & ICC_SRE_EL1_SRE))
>> >> + return false;
>> >> + } else {
>> >> + p->regval = vgicv3->vgic_sre;
>> >> + }
>> >> +
>> >> + return true;
>> >> +}
>> >> +
>> >> +static const struct sys_reg_desc gic_v3_icc_reg_descs[] = {
>> >> + /* ICC_PMR_EL1 */
>> >> + { Op0(3), Op1(0), CRn(4), CRm(6), Op2(0), access_gic_pmr },
>> >> + /* ICC_BPR0_EL1 */
>> >> + { Op0(3), Op1(0), CRn(12), CRm(8), Op2(3), access_gic_bpr0 },
>> >> + /* ICC_AP0R0_EL1 */
>> >> + { Op0(3), Op1(0), CRn(12), CRm(8), Op2(4), access_gic_ap0r },
>> >> + /* ICC_AP0R1_EL1 */
>> >> + { Op0(3), Op1(0), CRn(12), CRm(8), Op2(5), access_gic_ap0r },
>> >> + /* ICC_AP0R2_EL1 */
>> >> + { Op0(3), Op1(0), CRn(12), CRm(8), Op2(6), access_gic_ap0r },
>> >> + /* ICC_AP0R3_EL1 */
>> >> + { Op0(3), Op1(0), CRn(12), CRm(8), Op2(7), access_gic_ap0r },
>> >> + /* ICC_AP1R0_EL1 */
>> >> + { Op0(3), Op1(0), CRn(12), CRm(9), Op2(0), access_gic_ap1r },
>> >> + /* ICC_AP1R1_EL1 */
>> >> + { Op0(3), Op1(0), CRn(12), CRm(9), Op2(1), access_gic_ap1r },
>> >> + /* ICC_AP1R2_EL1 */
>> >> + { Op0(3), Op1(0), CRn(12), CRm(9), Op2(2), access_gic_ap1r },
>> >> + /* ICC_AP1R3_EL1 */
>> >> + { Op0(3), Op1(0), CRn(12), CRm(9), Op2(3), access_gic_ap1r },
>> >> + /* ICC_BPR1_EL1 */
>> >> + { Op0(3), Op1(0), CRn(12), CRm(12), Op2(3), access_gic_bpr1 },
>> >> + /* ICC_CTLR_EL1 */
>> >> + { Op0(3), Op1(0), CRn(12), CRm(12), Op2(4), access_gic_ctlr },
>> >> + /* ICC_SRE_EL1 */
>> >> + { Op0(3), Op1(0), CRn(12), CRm(12), Op2(5), access_gic_sre },
>> >> + /* ICC_IGRPEN0_EL1 */
>> >> + { Op0(3), Op1(0), CRn(12), CRm(12), Op2(6), access_gic_grpen0 },
>> >> + /* ICC_GRPEN1_EL1 */
>> >> + { Op0(3), Op1(0), CRn(12), CRm(12), Op2(7), access_gic_grpen1 },
>> >> +};
>> >> +
>> >> +int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
>> >> + u64 *reg)
>> >> +{
>> >> + struct sys_reg_params params;
>> >> + u64 sysreg = (id & KVM_DEV_ARM_VGIC_SYSREG_MASK) | KVM_REG_SIZE_U64;
>> >> +
>> >> + params.regval = *reg;
>> >> + params.is_write = is_write;
>> >> + params.is_aarch32 = false;
>> >> + params.is_32bit = false;
>> >> +
>> >> + if (find_reg_by_id(sysreg, ¶ms, gic_v3_icc_reg_descs,
>> >> + ARRAY_SIZE(gic_v3_icc_reg_descs)))
>> >> + return 0;
>> >> +
>> >> + return -ENXIO;
>> >> +}
>> >> +
>> >> +int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id,
>> >> + u64 *reg)
>> >> +{
>> >> + struct sys_reg_params params;
>> >> + const struct sys_reg_desc *r;
>> >> + u64 sysreg = (id & KVM_DEV_ARM_VGIC_SYSREG_MASK) | KVM_REG_SIZE_U64;
>> >> +
>> >> + if (is_write)
>> >> + params.regval = *reg;
>> >> + params.is_write = is_write;
>> >> + params.is_aarch32 = false;
>> >> + params.is_32bit = false;
>> >> +
>> >> + r = find_reg_by_id(sysreg, ¶ms, gic_v3_icc_reg_descs,
>> >> + ARRAY_SIZE(gic_v3_icc_reg_descs));
>> >> + if (!r)
>> >> + return -ENXIO;
>> >> +
>> >> + if (!r->access(vcpu, ¶ms, r))
>> >> + return -EINVAL;
>> >
>> > According to the API, EINVAL meansinvalid mpidr. Should we expand on
>> > how it can be used or allocate a new error code?
>> How abt EACCES error code?
>>
>
> That would mean permission denied, which is a bit weird.
Yes I agree, but you can suggest.
>
>> >
>> >> +
>> >> + if (!is_write)
>> >> + *reg = params.regval;
>> >> +
>> >> + return 0;
>> >> +}
>> >> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
>> >> index 967c295..1139971 100644
>> >> --- a/virt/kvm/arm/vgic/vgic-v3.c
>> >> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>> >> @@ -228,6 +228,13 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu)
>> >> vgic_v3->vgic_sre = 0;
>> >> }
>> >>
>> >> + vcpu->arch.vgic_cpu.num_id_bits = (kvm_vgic_global_state.ich_vtr_el2 &
>> >> + ICH_VTR_ID_BITS_MASK) >>
>> >> + ICH_VTR_ID_BITS_SHIFT;
>> >> + vcpu->arch.vgic_cpu.num_pri_bits = ((kvm_vgic_global_state.ich_vtr_el2 &
>> >> + ICH_VTR_PRI_BITS_MASK) >>
>> >> + ICH_VTR_PRI_BITS_SHIFT) + 1;
>> >> +
>> >> /* Get the show on the road... */
>> >> vgic_v3->vgic_hcr = ICH_HCR_EN;
>> >> }
>> >> @@ -328,6 +335,7 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
>> >> */
>> >> kvm_vgic_global_state.nr_lr = (ich_vtr_el2 & 0xf) + 1;
>> >> kvm_vgic_global_state.can_emulate_gicv2 = false;
>> >> + kvm_vgic_global_state.ich_vtr_el2 = ich_vtr_el2;
>> >>
>> >> if (!info->vcpu.start) {
>> >> kvm_info("GICv3: no GICV resource entry\n");
>> >> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
>> >> index c461f6b..0e632d0 100644
>> >> --- a/virt/kvm/arm/vgic/vgic.h
>> >> +++ b/virt/kvm/arm/vgic/vgic.h
>> >> @@ -126,6 +126,10 @@ int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
>> >> int offset, u32 *val);
>> >> int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
>> >> int offset, u32 *val);
>> >> +int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write,
>> >> + u64 id, u64 *val);
>> >> +int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
>> >> + u64 *reg);
>> >> #else
>> >> static inline int vgic_register_its_iodevs(struct kvm *kvm)
>> >> {
>> >> --
>
> Thanks,
> -Christoffer
^ permalink raw reply
* [PATCH 8/9] mfd: wm97xx-core: core support for wm97xx Codec
From: Lee Jones @ 2016-11-18 16:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477510907-23495-9-git-send-email-robert.jarzmik@free.fr>
On Wed, 26 Oct 2016, Robert Jarzmik wrote:
> The WM9705, WM9712 and WM9713 are highly integrated codecs, with an
> audio codec, DAC and ADC, GPIO unit and a touchscreen interface.
>
> Historically the support was spread across drivers/input/touchscreen and
> sound/soc/codecs. The sharing was done through ac97 bus sharing. This
> model will not withstand the new AC97 bus model, where codecs are
> discovered on runtime.
>
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
> drivers/mfd/Kconfig | 14 +++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/wm97xx-core.c | 282 +++++++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/wm97xx.h | 31 +++++
> 4 files changed, 328 insertions(+)
> create mode 100644 drivers/mfd/wm97xx-core.c
> create mode 100644 include/linux/mfd/wm97xx.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index c6df6442ba2b..2ac818127b0a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1597,6 +1597,20 @@ config MFD_WM8994
> core support for the WM8994, in order to use the actual
> functionaltiy of the device other drivers must be enabled.
>
> +config MFD_WM97xx
> + tristate "Wolfson Microelectronics WM97xx"
> + select MFD_CORE
> + select REGMAP_AC97
> + select AC97_BUS_COMPAT if AC97_BUS_NEW
> + help
> +
Surplus '\n' here.
> + The WM9705, WM9712 and WM9713 is a highly integrated hi-fi CODEC
> + designed for smartphone applications. As well as audio functionality
> + it has on board GPIO and a touchscreen functionality which is
> + supported via the relevant subsystems. This driver provides core
> + support for the WM97xx, in order to use the actual functionaltiy of
Always spell check your work.
> + the device other drivers must be enabled.
> +
> config MFD_STW481X
> tristate "Support for ST Microelectronics STw481x"
> depends on I2C && (ARCH_NOMADIK || COMPILE_TEST)
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 9834e669d985..5c3534f4c7c3 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_MFD_WM8350) += wm8350.o
> obj-$(CONFIG_MFD_WM8350_I2C) += wm8350-i2c.o
> wm8994-objs := wm8994-core.o wm8994-irq.o wm8994-regmap.o
> obj-$(CONFIG_MFD_WM8994) += wm8994.o
> +obj-$(CONFIG_MFD_WM97xx) += wm97xx-core.o
>
> obj-$(CONFIG_TPS6105X) += tps6105x.o
> obj-$(CONFIG_TPS65010) += tps65010.o
> diff --git a/drivers/mfd/wm97xx-core.c b/drivers/mfd/wm97xx-core.c
> new file mode 100644
> index 000000000000..f2cd80354b4a
> --- /dev/null
> +++ b/drivers/mfd/wm97xx-core.c
> @@ -0,0 +1,282 @@
> +/*
> + * Wolfson WM97xx -- Core device
> + *
> + * Copyright (C) 2016 Robert Jarzmik
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * Features:
> + * - an AC97 audio codec
> + * - a touchscreen driver
> + * - a GPIO block
Place this above the Copyright.
> + */
> +
> +#include <linux/module.h>
Why is this seperted?
> +#include <linux/device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/wm97xx.h>
> +#include <linux/wm97xx.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <sound/ac97/codec.h>
> +#include <sound/ac97/compat.h>
Alphabetical.
> +#define WM9705_VENDOR_ID 0x574d4c05
> +#define WM9712_VENDOR_ID 0x574d4c12
> +#define WM9713_VENDOR_ID 0x574d4c13
> +#define WM97xx_VENDOR_ID_MASK 0xffffffff
Use tabs, not spaces.
These are probably better represented as enums.
> +struct wm97xx_priv {
> + struct regmap *regmap;
> + struct snd_ac97 *ac97;
> + struct device *dev;
> +};
Replace _priv with _ddata.
Also document using kerneldoc.
> +static bool wm97xx_readable_reg(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case AC97_RESET ... AC97_PCM_SURR_DAC_RATE:
> + case AC97_PCM_LR_ADC_RATE:
> + case AC97_CENTER_LFE_MASTER:
> + case AC97_SPDIF ... AC97_LINE1_LEVEL:
> + case AC97_GPIO_CFG ... 0x5c:
Please define.
> + case AC97_CODEC_CLASS_REV ... AC97_PCI_SID:
> + case 0x74 ... AC97_VENDOR_ID2:
As above.
> + default:
> + return false;
> + }
> +}
> +
> +static bool wm97xx_writeable_reg(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case AC97_VENDOR_ID1:
> + case AC97_VENDOR_ID2:
> + return false;
> + default:
> + return wm97xx_readable_reg(dev, reg);
> + }
> +}
> +
> +static const struct reg_default wm97xx_reg_defaults[] = {
> +};
What's the point in this?
> +static const struct regmap_config wm9705_regmap_config = {
> + .reg_bits = 16,
> + .reg_stride = 2,
> + .val_bits = 16,
> + .max_register = 0x7e,
> + .cache_type = REGCACHE_RBTREE,
> +
> + .reg_defaults = wm97xx_reg_defaults,
> + .num_reg_defaults = ARRAY_SIZE(wm97xx_reg_defaults),
> + .volatile_reg = regmap_ac97_default_volatile,
> + .readable_reg = wm97xx_readable_reg,
> + .writeable_reg = wm97xx_writeable_reg,
> +};
> +
> +static const struct regmap_config wm9712_regmap_config = {
> + .reg_bits = 16,
> + .reg_stride = 2,
> + .val_bits = 16,
> + .max_register = 0x7e,
> + .cache_type = REGCACHE_RBTREE,
> +
> + .reg_defaults = wm97xx_reg_defaults,
> + .num_reg_defaults = ARRAY_SIZE(wm97xx_reg_defaults),
> + .volatile_reg = regmap_ac97_default_volatile,
> + .readable_reg = wm97xx_readable_reg,
> + .writeable_reg = wm97xx_writeable_reg,
> +};
> +
> +static int wm9705_register(struct wm97xx_priv *wm97xx)
> +{
> + return 0;
> +}
> +
> +static int wm9712_register(struct wm97xx_priv *wm97xx)
> +{
> + return 0;
> +}
I don't get it?
Either populate or don't provide.
> +static const struct reg_default wm9713_reg_defaults[] = {
> + { 0x02, 0x8080 }, /* Speaker Output Volume */
> + { 0x04, 0x8080 }, /* Headphone Output Volume */
> + { 0x06, 0x8080 }, /* Out3/OUT4 Volume */
> + { 0x08, 0xc880 }, /* Mono Volume */
> + { 0x0a, 0xe808 }, /* LINEIN Volume */
> + { 0x0c, 0xe808 }, /* DAC PGA Volume */
> + { 0x0e, 0x0808 }, /* MIC PGA Volume */
> + { 0x10, 0x00da }, /* MIC Routing Control */
> + { 0x12, 0x8000 }, /* Record PGA Volume */
> + { 0x14, 0xd600 }, /* Record Routing */
> + { 0x16, 0xaaa0 }, /* PCBEEP Volume */
> + { 0x18, 0xaaa0 }, /* VxDAC Volume */
> + { 0x1a, 0xaaa0 }, /* AUXDAC Volume */
> + { 0x1c, 0x0000 }, /* Output PGA Mux */
> + { 0x1e, 0x0000 }, /* DAC 3D control */
> + { 0x20, 0x0f0f }, /* DAC Tone Control*/
> + { 0x22, 0x0040 }, /* MIC Input Select & Bias */
> + { 0x24, 0x0000 }, /* Output Volume Mapping & Jack */
> + { 0x26, 0x7f00 }, /* Powerdown Ctrl/Stat*/
> + { 0x28, 0x0405 }, /* Extended Audio ID */
> + { 0x2a, 0x0410 }, /* Extended Audio Start/Ctrl */
> + { 0x2c, 0xbb80 }, /* Audio DACs Sample Rate */
> + { 0x2e, 0xbb80 }, /* AUXDAC Sample Rate */
> + { 0x32, 0xbb80 }, /* Audio ADCs Sample Rate */
> + { 0x36, 0x4523 }, /* PCM codec control */
> + { 0x3a, 0x2000 }, /* SPDIF control */
> + { 0x3c, 0xfdff }, /* Powerdown 1 */
> + { 0x3e, 0xffff }, /* Powerdown 2 */
> + { 0x40, 0x0000 }, /* General Purpose */
> + { 0x42, 0x0000 }, /* Fast Power-Up Control */
> + { 0x44, 0x0080 }, /* MCLK/PLL Control */
> + { 0x46, 0x0000 }, /* MCLK/PLL Control */
> +
> + { 0x4c, 0xfffe }, /* GPIO Pin Configuration */
> + { 0x4e, 0xffff }, /* GPIO Pin Polarity / Type */
> + { 0x50, 0x0000 }, /* GPIO Pin Sticky */
> + { 0x52, 0x0000 }, /* GPIO Pin Wake-Up */
> + /* GPIO Pin Status */
> + { 0x56, 0xfffe }, /* GPIO Pin Sharing */
> + { 0x58, 0x4000 }, /* GPIO PullUp/PullDown */
> + { 0x5a, 0x0000 }, /* Additional Functions 1 */
> + { 0x5c, 0x0000 }, /* Additional Functions 2 */
> + { 0x60, 0xb032 }, /* ALC Control */
> + { 0x62, 0x3e00 }, /* ALC / Noise Gate Control */
> + { 0x64, 0x0000 }, /* AUXDAC input control */
> + { 0x74, 0x0000 }, /* Digitiser Reg 1 */
> + { 0x76, 0x0006 }, /* Digitiser Reg 2 */
> + { 0x78, 0x0001 }, /* Digitiser Reg 3 */
> + { 0x7a, 0x0000 }, /* Digitiser Read Back */
> +};
> +
> +static const struct regmap_config wm9713_regmap_config = {
> + .reg_bits = 16,
> + .reg_stride = 2,
> + .val_bits = 16,
> + .max_register = 0x7e,
> + .cache_type = REGCACHE_RBTREE,
> +
> + .reg_defaults = wm9713_reg_defaults,
> + .num_reg_defaults = ARRAY_SIZE(wm9713_reg_defaults),
> + .volatile_reg = regmap_ac97_default_volatile,
> + .readable_reg = wm97xx_readable_reg,
> + .writeable_reg = wm97xx_writeable_reg,
> +};
> +
> +static int wm9713_register(struct wm97xx_priv *wm97xx,
> + struct wm97xx_pdata *pdata)
> +{
What are you lining this up with?
> + struct wm97xx_platform_data codec_pdata;
> + const struct mfd_cell cells[] = {
> + {
> + .name = "wm9713-codec",
> + .platform_data = &codec_pdata,
> + .pdata_size = sizeof(codec_pdata),
> + },
> + {
> + .name = "wm97xx-ts",
> + .platform_data = &codec_pdata,
> + .pdata_size = sizeof(codec_pdata),
> + },
> + };
> +
> + codec_pdata.ac97 = wm97xx->ac97;
> + codec_pdata.regmap = devm_regmap_init_ac97(wm97xx->ac97,
> + &wm9713_regmap_config);
> + codec_pdata.batt_pdata = pdata->batt_pdata;
> + if (IS_ERR(codec_pdata.regmap))
> + return PTR_ERR(codec_pdata.regmap);
This doesn't look like pdata. You can get rid of all of this hoop
jumping if you add it to ddata and set it as such.
> + return devm_mfd_add_devices(wm97xx->dev, -1, cells,
Use the defines.
> + ARRAY_SIZE(cells), NULL, 0, NULL);
> +}
> +
> +static int wm97xx_ac97_probe(struct ac97_codec_device *adev)
This looks sound specific.
Why isn't this a plane platform driver?
> +{
> + struct wm97xx_priv *wm97xx;
> + int ret;
> + void *pdata = snd_ac97_codec_get_platdata(adev);
> +
> + wm97xx = devm_kzalloc(ac97_codec_dev2dev(adev),
> + sizeof(*wm97xx), GFP_KERNEL);
> + if (!wm97xx)
> + return -ENOMEM;
> +
> + wm97xx->dev = ac97_codec_dev2dev(adev);
> + wm97xx->ac97 = snd_ac97_compat_alloc(adev);
> + if (IS_ERR(wm97xx->ac97))
> + return PTR_ERR(wm97xx->ac97);
> +
> +
> + ac97_set_drvdata(adev, wm97xx);
> + dev_info(wm97xx->dev, "wm97xx core found, id=0x%x\n",
> + adev->vendor_id);
All of this ac97/sound stuff should be done in the ac97/sound driver.
> + switch (adev->vendor_id) {
> + case WM9705_VENDOR_ID:
> + ret = wm9705_register(wm97xx);
> + break;
> + case WM9712_VENDOR_ID:
> + ret = wm9712_register(wm97xx);
> + break;
> + case WM9713_VENDOR_ID:
> + ret = wm9713_register(wm97xx, pdata);
> + break;
> + default:
> + ret = -ENODEV;
> + }
> +
> + if (ret)
> + snd_ac97_compat_release(wm97xx->ac97);
> +
> + return ret;
> +}
> +
> +static int wm97xx_ac97_remove(struct ac97_codec_device *adev)
> +{
> + struct wm97xx_priv *wm97xx = ac97_get_drvdata(adev);
> +
> + snd_ac97_compat_release(wm97xx->ac97);
> +
> + return 0;
> +}
> +
> +static const struct ac97_id wm97xx_ac97_ids[] = {
> + { .id = WM9705_VENDOR_ID, .mask = WM97xx_VENDOR_ID_MASK },
> + { .id = WM9712_VENDOR_ID, .mask = WM97xx_VENDOR_ID_MASK },
> + { .id = WM9713_VENDOR_ID, .mask = WM97xx_VENDOR_ID_MASK },
> + { }
> +};
> +
> +static struct ac97_codec_driver wm97x3_ac97_driver = {
> + .driver = {
> + .name = "wm97xx-core",
> + },
> + .probe = wm97xx_ac97_probe,
> + .remove = wm97xx_ac97_remove,
> + .id_table = wm97xx_ac97_ids,
> +};
> +
> +static int __init wm97xx_module_init(void)
> +{
> + return snd_ac97_codec_driver_register(&wm97x3_ac97_driver);
> +}
> +module_init(wm97xx_module_init);
> +
> +static void __exit wm97xx_module_exit(void)
> +{
> + snd_ac97_codec_driver_unregister(&wm97x3_ac97_driver);
> +}
> +module_exit(wm97xx_module_exit);
> +
> +MODULE_DESCRIPTION("WM9712, WM9713 core driver");
> +MODULE_AUTHOR("Robert Jarzmik <robert.jarzmik@free.fr>");
> +MODULE_LICENSE("GPL");
> +
> diff --git a/include/linux/mfd/wm97xx.h b/include/linux/mfd/wm97xx.h
> new file mode 100644
> index 000000000000..627322f14d48
> --- /dev/null
> +++ b/include/linux/mfd/wm97xx.h
> @@ -0,0 +1,31 @@
> +/*
> + * wm97xx client interface
> + *
> + * Copyright (C) 2016 Robert Jarzmik
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __LINUX_MFD_WM97XX_H
> +#define __LINUX_MFD_WM97XX_H
> +
> +struct regmap;
> +struct wm97xx_batt_pdata;
> +struct snd_ac97;
Why can't you just add the include files?
> +struct wm97xx_platform_data {
> + struct snd_ac97 *ac97;
> + struct regmap *regmap;
> + struct wm97xx_batt_pdata *batt_pdata;
> +};
> +
> +
> +#endif
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [arm-soc:qcom/arm64 13/13] arch/arm64/boot/dts/qcom/msm8994.dtsi:14:48: fatal error: dt-bindings/clock/qcom,gcc-msm8994.h: No such file or directory
From: Andy Gross @ 2016-11-18 16:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201611182005.RuBSvaJ7%fengguang.wu@intel.com>
On Fri, Nov 18, 2016 at 08:20:07PM +0800, kbuild test robot wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git qcom/arm64
> head: feeaf56ac78d283efe65ea60ec999d4bf3cf395e
> commit: feeaf56ac78d283efe65ea60ec999d4bf3cf395e [13/13] arm64: dts: msm8994 SoC and Huawei Angler (Nexus 6P) support
> config: arm64-defconfig (attached as .config)
> compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
> wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> git checkout feeaf56ac78d283efe65ea60ec999d4bf3cf395e
> # save the attached .config to linux build tree
> make.cross ARCH=arm64
>
> All errors (new ones prefixed by >>):
>
> In file included from arch/arm64/boot/dts/qcom/msm8994-angler-rev-101.dts:16:0:
> >> arch/arm64/boot/dts/qcom/msm8994.dtsi:14:48: fatal error: dt-bindings/clock/qcom,gcc-msm8994.h: No such file or directory
> #include <dt-bindings/clock/qcom,gcc-msm8994.h>
So I completely forgot to add the point that the 8994 dt-include information is
coming in through the clk-tree. We've been working with a immutable branch and
both will land for the next rc1. As such this hasn't been a problem in next,
but it will be for arm-soc for standalone testing.
Andy
^ permalink raw reply
* [PATCHv3 5/6] arm64: Use __pa_symbol for kernel symbols
From: Mark Rutland @ 2016-11-18 16:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118143543.GC1197@leverpostej>
On Fri, Nov 18, 2016 at 02:35:44PM +0000, Mark Rutland wrote:
> Hi Laura,
>
> On Thu, Nov 17, 2016 at 05:16:55PM -0800, Laura Abbott wrote:
> >
> > __pa_symbol is technically the marco that should be used for kernel
> > symbols. Switch to this as a pre-requisite for DEBUG_VIRTUAL which
> > will do bounds checking.
> >
> > Signed-off-by: Laura Abbott <labbott@redhat.com>
> > ---
> > v3: Conversion of more sites besides just _end. Addition of __lm_sym_addr
> > macro to take care of the _va(__pa_symbol(..)) idiom.
> >
> > Note that a copy of __pa_symbol was added to avoid a mess of headers
> > since the #ifndef __pa_symbol case is defined in linux/mm.h
>
> I think we also need to fix up virt_to_phys(__cpu_soft_restart) in
> arch/arm64/kernel/cpu-reset.h. Otherwise, this looks complete for uses
> falling under arch/arm64/.
I think I spoke too soon. :(
In the kasan code, use of tmp_pg_dir, kasan_zero_{page,pte,pmd,pud} all
need to be vetted, as those are in the image, but get passed directly to
functions which will end up doing a virt_to_phys behind the scenes (e.g.
cpu_replace_ttbr1(), pmd_populate_kernel()).
There's also some virt_to_pfn(<symbol>) usage that needs to be fixed up
in arch/arm64/kernel/hibernate.c.
... there's also more of that in common kernel code. :(
Thanks,
Mark.
^ permalink raw reply
* [PATCH V8 1/3] tracing: add a possibility of exporting function trace to other places instead of ring buffer only
From: Steven Rostedt @ 2016-11-18 16:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <87vavkesy6.fsf@ashishki-desk.ger.corp.intel.com>
On Fri, 18 Nov 2016 16:57:53 +0200
Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote:
> Steven Rostedt <rostedt@goodmis.org> writes:
>
> > This looks good to me, although I would like this to go through my tree
> > (to make sure it gets all my testing). I understand the next two
> > patches depend on this, how would you want to go about that?
> >
> > One is that I can pull it in the next merge window, and the rest go in
> > after that. Or I can take the other two patches with the proper acks as
> > well.
>
> I just asked for the last patch be split 4 ways, but otherwise, they
> have my acks. If Chunyan can do that, you can take all of them into your
> tree.
>
OK, I'll wait for the split then.
Thanks,
-- Steve
^ permalink raw reply
* [PATCH v8 12/16] drivers: iommu: arm-smmu: split probe functions into DT/generic portions
From: Robin Murphy @ 2016-11-18 16:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118160006.GW13470@arm.com>
On 18/11/16 16:00, Will Deacon wrote:
> On Wed, Nov 16, 2016 at 03:29:32PM +0000, Lorenzo Pieralisi wrote:
>> Current ARM SMMU probe functions intermingle HW and DT probing
>> in the initialization functions to detect and programme the ARM SMMU
>> driver features. In order to allow probing the ARM SMMU with other
>> firmwares than DT, this patch splits the ARM SMMU init functions into
>> DT and HW specific portions so that other FW interfaces (ie ACPI) can
>> reuse the HW probing functions and skip the DT portion accordingly.
>>
>> This patch implements no functional change, only code reshuffling.
>>
>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Reviewed-by: Tomasz Nowicki <tn@semihalf.com>
>> Tested-by: Hanjun Guo <hanjun.guo@linaro.org>
>> Tested-by: Tomasz Nowicki <tn@semihalf.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Hanjun Guo <hanjun.guo@linaro.org>
>> Cc: Robin Murphy <robin.murphy@arm.com>
>> ---
>> drivers/iommu/arm-smmu.c | 62 +++++++++++++++++++++++++++++-------------------
>> 1 file changed, 37 insertions(+), 25 deletions(-)
>
> Reviewed-by: Will Deacon <will.deacon@arm.com>
>
> Although I'd also like Robin to have a look at this.
Sorry, I've been slow this week. Having finally got round to taking a
proper look, I swear this seems even tidier than the last version I
looked at closely. It doesn't break DT boot on my Juno, either.
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>
> Will
>
^ permalink raw reply
* [RFC 6/6] ARM: dts: am57xx-beagle-x15-common: enable etnaviv
From: Rob Herring @ 2016-11-18 16:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <e69a797c-bec3-2243-5c19-9d7633092369@ti.com>
On Thu, Nov 17, 2016 at 08:56:18PM -0600, Nishanth Menon wrote:
> On 11/17/2016 08:44 PM, Robert Nelson wrote:
> again.. a short commit message at least please? :)
>
> > Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
> > CC: Julien <jboulnois@gmail.com>
> > CC: Nishanth Menon <nm@ti.com>
> > CC: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > CC: Tony Lindgren <tony@atomide.com>
> > ---
> > arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
> > index 6df7829..3bc47be 100644
> > --- a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
> > +++ b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
> > @@ -97,6 +97,12 @@
> > #cooling-cells = <2>;
> > };
> >
> > + gpu-subsystem {
> A) do we want to make things clear that this is gpu subsystem for gc320?
No.
> B) How about other platforms that could equally reuse?
Better yet, get rid of this nonsense. You are defining a wrapper to
group 1 nodes. Yes, I know this is what the driver wants, but you should
be able to make it deal with this situation and only have a gc320 node.
> > + compatible = "ti,gc320-gpu-subsystem";
> > + cores = <&bb2d>;
> > + status = "okay";
> > + };
> > +
^ permalink raw reply
* [PATCH v10 01/11] remoteproc: st_slim_rproc: add a slimcore rproc driver
From: Bjorn Andersson @ 2016-11-18 16:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161117095252.GC2698@localhost>
On Thu 17 Nov 01:52 PST 2016, Vinod Koul wrote:
> On Wed, Nov 16, 2016 at 10:36:46PM -0800, Bjorn Andersson wrote:
> > On Sun 13 Nov 21:18 PST 2016, Vinod Koul wrote:
> >
> > > On Mon, Nov 07, 2016 at 01:57:35PM +0000, Peter Griffin wrote:
> > > > >
> > > > > As you now make changes to the entire remoteproc Kconfig file, rather
> > > > > than simply add a Kconfig symbol we can't bring this in via Vinod's tree
> > > > > without providing Linus with a messy merge conflict.
> > > > >
> > > > > So the remoteproc parts now has to go through my tree.
> > > >
> > > > OK, I think the best approach is for Vinod to create an immutable
> > > > branch with the entire fdma series on, and then both of you merge that branch into
> > > > your respective trees.
> > >
> > > my topic/st_fdma is immutable branch. You cna merge it, if you need a signed
> > > tag, please do let me know
> > >
> >
> > Hi Vinod,
> >
> > It looks like you reverted the wrong Kconfig fix, the one I objected to
> > was the change in drivers/remoteproc, not the one in drivers/dma.
> >
> > The ST_FMDA depends on functions exposed by REMOTEPROC and
> > ST_SLIM_REMOTEPROC, the latter in turn depends on REMOTEPROC, which you
> > guys made user selectable - and as such should not be selected - but I
> > think we should move forward and get everything merged and then we can
> > go back and figure out how this should be addressed (or left alone?).
> >
> > I have merged "topic/st_fdma" into rproc-next, so that I can fix up the
> > now broken drivers/remoteproc/Kconfig.
> >
> > We do however both need to revert the revert or there will be link
> > errors if you build the dma driver with remoteproc=n. If you do this I
> > can merge the topic once more and we'll keep the set of changes in sync.
>
> Oops my bad, thanks for letting me know. I have reverted this now and
> pushing out. Please do let me know if this was fine
>
Thanks for the update Vinod, I've merged in your updated branch into
mine, so now we should be good.
Regards,
Bjorn
^ permalink raw reply
* [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06
From: Arnd Bergmann @ 2016-11-18 16:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <EE11001F9E5DDD47B7634E2F8A612F2E1F921858@lhreml507-mbx>
On Friday, November 18, 2016 4:18:07 PM CET Gabriele Paoloni wrote:
> From: Arnd Bergmann [mailto:arnd at arndb.de]
> > On Friday, November 18, 2016 12:53:08 PM CET Gabriele Paoloni wrote:
> > > From: Arnd Bergmann [mailto:arnd at arndb.de]
> > > > On Friday, November 18, 2016 12:07:28 PM CET Gabriele Paoloni
> > > > The easiest change compared to the v5 code would be to walk
> > > > a linked list of 'struct extio_ops' structures rather than
> > > > assuming there is only ever one of them. I think one of the
> > > > earlier versions actually did this.
> > >
> > > Right but if my understanding is correct if we live in a multi-
> > > domain I/O space world when you have an input addr in the I/O
> > > accessors this addr can be duplicated (for example for the standard
> > > PCI IO domain and for our special LPC domain).
> > > So effectively even if you walk a linked list there is a problem
> > > of disambiguation...am I right?
> >
> > No, unlike the PCI memory space, the PIO addresses are not
> > usually distinct, i.e. every PCI bus has its own 64K I/O
> > addresses starting at zero. We linearize them into the
> > Linux I/O space using the per-domain io_offset value.
>
> I am going to summarize my understanding here below:
>
> It seems to me that what is linearized is the virtual address
> space associated to the IO address space. This address space
> goes from PCI_IOBASE to (PCI_IOBASE + IO_SPACE_LIMIT).
>
> The I/O accessors perform rd/wr operation on this address
> space using a port IO token.
>
> Each token map into a cpu physical address range
> Each cpu physical address range maps to a bus address range
> if the bus controller specifies a range property.
>
> Devices under a bus controller specify the bus addresses that
> they operate on in their reg property.
>
> So each device can use the same bus addresses under two
> separate bus controllers as long as the bus controller
> use the range properties to map the same bus range to different
> cpu address range.
Sounds all correct to me so far, yes.
> > For the ISA/LPC spaces there are only 4k of addresses, they
> > the bus addresses always overlap, but we can trivially
> > figure out the bus address from Linux I/O port number
> > by subtracting the start of the range.
>
> Are you saying that our LPC controller should specify a
> range property to map bus addresses into a cpu address range?
No. There is not CPU address associated with it, because it's
not memory mapped.
Instead, we need to associate a bus address with a logical
Linux port number, both in of_address_to_resource and
in inb()/outb().
> > > > Another option the IA64 approach mentioned in another subthread
> > > > today, looking up the operations based on an index from the
> > > > upper bits of the port number. If we do this, we probably
> > > > want to do that for all PIO access and replace the entire
> > > > virtual address remapping logic with that. I think Bjorn
> > > > in the past argued in favor of such an approach, while I
> > > > advocated the current scheme for simplicity based on how
> > > > every I/O space these days is just memory mapped (which now
> > > > turned out to be false, both on powerpc and arm64).
> > >
> > > This seems really complex...I am a bit worried that possibly
> > > we end up in having the maintainers saying that it is not worth
> > > to re-invent the world just for this special LPC device...
> >
> > It would clearly be a rather invasive change, but the
> > end-result isn't necessarily more complex than what we
> > have today, as we'd kill off the crazy pci_pio_to_address()
> > and pci_address_to_pio() hacks in address translation.
>
> I have to look better into this...can you provide me a reference
> to the Bjorn argument in favor of this approach?
The thread seems to have been pci: Introduce pci_register_io_range()
helper function, e.g. in https://lkml.org/lkml/2014/7/8/969
> > > To be honest with you I would keep things simple for this
> > > LPC and introduce more complex reworks later if more devices
> > > need to be introduced.
> > >
> > > What if we stick on a single domain now where we introduce a
> > > reserved threshold for the IO space (say INDIRECT_MAX_IO).
> >
> > I said having a single domain is fine, but I still don't
> > like the idea of reserving low port numbers for this hack,
> > it would mean that the numbers change for everyone else.
>
> I don't get this much...I/O tokens that are passed to the I/O
> accessors are not fixed anyway and they vary depending on the order
> of adding ranges to io_range_list...so I don't see a big issue
> with this...
On machines with a legacy devices behind the PCI bridge,
there may still be a reason to have the low I/O port range
reserved for the primary bus, e.g. to get a VGA text console
to work.
On powerpc, this is called the "primary" PCI host, i.e. the
only one that is allowed to have an ISA bridge.
Arnd
^ 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