* [PATCH] mfd: twl-core: export twl_get_regmap
From: Nicolae Rosia @ 2016-11-21 10:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161121093144.GE23750@n2100.armlinux.org.uk>
On Mon, Nov 21, 2016 at 11:31 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> Passing data between drivers using *_set_drvdata() is a layering
> violation:
>
> 1. Driver data is supposed to be driver private data associated with
> the currently bound driver.
> 2. The driver data pointer is NULL'd when the driver unbinds from the
> device. See __device_release_driver() and the
> dev_set_drvdata(dev, NULL).
> 3. It will break with CONFIG_DEBUG_TEST_DRIVER_REMOVE enabled for a
> similar reason to (2).
>
> So, do not pass data between drivers using *_set_drvdata() - any
> examples in the kernel already are founded on bad practice, are
> fragile, and are already broken for some kernel configurations.
After inspecting mfd_add_device, it seems that it creates a
platform_device which has the parent set to the driver calling the
function.
Isn't module unloading forbidden if there is a parent->child
relationship in place and you're removing the parent?
What should be the best practice to share data between drivers?
Reference counted data?
In the case of TWL, the twl-core is just a simple container for
regmaps - all other "sub devices" are using those regmaps to access
the I2C device's registers, it makes no sense to remove the parent
driver since it does *nothing*.
^ permalink raw reply
* [PATCH 8/9] mfd: wm97xx-core: core support for wm97xx Codec
From: Lee Jones @ 2016-11-21 10:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <87twb3r94s.fsf@belgarion.home>
Mark, please see below:
On Sat, 19 Nov 2016, Robert Jarzmik wrote:
> Lee Jones <lee.jones@linaro.org> writes:
>
> >> +#define WM9705_VENDOR_ID 0x574d4c05
> >> +#define WM9712_VENDOR_ID 0x574d4c12
> >> +#define WM9713_VENDOR_ID 0x574d4c13
> >> +#define WM97xx_VENDOR_ID_MASK 0xffffffff
> >
> > These are probably better represented as enums.
> These are ids, just as devicetree ids, or PCI ids, I don't think an enum will
> fit.
That's fine. We can use enums to simply group items of a similar
type. Representing these as enums would only serve to benefit code
cleanliness. What makes you think they won't fit?
> >> +struct wm97xx_priv {
> >> + struct regmap *regmap;
> >> + struct snd_ac97 *ac97;
> >> + struct device *dev;
> >> +};
> >
> > Replace _priv with _ddata.
> Ok, will do, would you care to explain if this is something specific to your mfd
> tree, or is it a kernel overall recommendation ?
It's personal preference. But these aren't necessarily private, so
priv doesn't really make a great deal of sense. These types of saved
pointers are better described as device data (ddata).
[...]
> >> +static const struct reg_default wm97xx_reg_defaults[] = {
> >> +};
> >
> > What's the point in this?
> Ah, that's a reminder I have still more work on this patch ... Either I remove
> support for wm9705 and wm9712 in the first version, or I complete it and add the
> tables :
> - wm9705_reg_defaults
> - wm9712_reg_defaults
Please don't add redundant code. I suggest you remove it for this
submission.
> >> +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.
> Same story as above, either I complete wm9705 and wm9712 support or I remove it.
Remove it please.
> >> +static int wm9713_register(struct wm97xx_priv *wm97xx,
> >> + struct wm97xx_pdata *pdata)
> >> +{
> >
> > What are you lining this up with?
> Emacs ... I don't see your point though, seeing it not aligned in the diff chunk
> doesn't mean it's not properly aligned.
That is true. So the two "scruct"s are line up in the source file,
right?
[...]
> >> + 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.
> You mean I should pass ddata to mfd sub-cells, right ?
Correct.
[...]
> >> +static int wm97xx_ac97_probe(struct ac97_codec_device *adev)
> >
> > This looks sound specific.
> >
> > Why isn't this a plane platform driver?
> That's the whole purpose of the patch serie.
>
> This serie transforms the wm97xx drivers from a platform driver model to an ac97
> model, where the ac97 devices are automatically discovered. The long explanation
> is in patch 0/9, on how it started and what is the global purpose.
>
> The short story is : switch to the new AC97 bus driver model.
>
> As for the "sound specific part", it's because AC97 bus is mainly used in sound
> oriented drivers, but still the codec IPs provide more than just sound, as the
> Wolfson codecs for instance.
I'd like to get Mark Brown's opinion on this.
> >> +{
> >> + 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.
>
> Nope, it's not sound adherence you're seeing here, it's ac97 bus and driver
> model adherence you're seeing. Would the bus be in drivers/ac97 instead of
> sound/ac97, the code would remain the same, would be bus be i2c you would see
> the same kind of calls but with i2c_xxx and not ac97_xxx.
>
> The wm97xx needs an ac97 bus to interact with the hardware, to provide sound,
> gpio, adc, etc ... functions. That's what is expressed here, and the fact that
> this ac97 access has to shared amongst the mfd sub-cells, and that these cells
> require :
> - wm97xx->ac97 : this one is for drivers/input/touchscreen/wm97xx-core.c
>
> So the requirement in this case is not for ac97/sound but input/touchscreen.
>
> >> 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?
> I could, but I don't need to, do I ?
> Moreover, if a mfd sub-cell doesn't use regmap for example, I won't include a
> useless define.
>
> Thanks for the review, Lee. This will iterate, I'll split out mfd patch(es) to
> follow up the review with you and Mark to lessen the burden on your mailbox.
>
> Cheers.
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [GIT PULL] Second Round of Renesas ARM Based SoC Drivers Updates for v4.10
From: Geert Uytterhoeven @ 2016-11-21 10:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161119013551.GB2543@localhost>
Hi Olof,
On Sat, Nov 19, 2016 at 2:35 AM, Olof Johansson <olof@lixom.net> wrote:
> On Thu, Nov 17, 2016 at 03:04:35PM +0100, Simon Horman wrote:
>> Please consider these second round of Renesas ARM based SoC drivers updates for v4.10.
>>
>> This pull request is based on a merge of:
>>
>> * The previous round of such requests, tagged as renesas-drivers-for-v4.10,
>> which you have already pulled.
>> * The soc-device-match-tag1 tag of Geert Uytterhoeven's renesas-driver's tree.
>> This is included to provide core soc_device_match() infrastructure which
>> is a dependency of identifying SoC and registering with SoC bus.
>>
>>
>> The following changes since commit 437c4eeb0bd4c1d68817be997716f52b8c22a9c3:
>>
>> Merge tag 'soc-device-match-tag1' of https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers into HEAD (2016-11-15 14:12:57 +0100)
>>
>> are available in the git repository at:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git tags/renesas-drivers2-for-v4.10
>>
>> for you to fetch changes up to 63ee9e2ba47dbdb42156c9b940515cfd49e78c91:
>>
>> soc: renesas: Identify SoC and register with the SoC bus (2016-11-17 14:37:20 +0100)
>>
>> ----------------------------------------------------------------
>> Second Round of Renesas ARM Based SoC Drivers Updates for v4.10
>>
>> * Identify SoC and register with the SoC bus
>> * Add support for the r8a7745 SoC to rcar-sysc
>>
>> ----------------------------------------------------------------
>> Geert Uytterhoeven (2):
>> ARM: shmobile: Document DT bindings for Product Register
>> soc: renesas: Identify SoC and register with the SoC bus
>>
>> Sergei Shtylyov (2):
>> ARM: shmobile: r8a7745: add power domain index macros
>> soc: renesas: rcar-sysc: add R8A7745 support
>>
>
> So, this pull request contains 8 patches, not 4. Seems like your pull
> request doesn't show any of the code from Geert's branch, didn't mention
> it in the tag and only in the email text above. Furthermore, Geert's
> branch modifies driver core code, so it's extra important to make sure
> it's clear that it's an unusual pull request.
Please accept our apologies for failing to make this clearer.
> Given that this modifies driver core, please either merge that code
> through Greg first, or get an ack from him. If you merge through him,
> make sure it's on a standalone topic branch that we can share.
I provided my soc-device-match branch (tag soc-device-match-tag1) as an
immutable base, to be included by all interested parties that need the
soc_device_match() infrastructure (Freescale/NXP, Samsung, Renesas).
Its core parts have been acked by Greg, and the fixes by Arnd and/or Greg
(the last fix only received an informal ack, that's why I hadn't added the
ack).
This branch has already been pulled by Ulf, and is present in mmc/next, as
a dependency for a Freescale/NXP driver update.
I hope my explanation helps. If anything is still unclear, please ask.
Thanks for reconsidering!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH 1/6] ASoC: samsung: Remove non-existing MACH dependencies
From: Sylwester Nawrocki @ 2016-11-21 10:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479566911-5580-2-git-send-email-krzk@kernel.org>
On 11/19/2016 03:48 PM, Krzysztof Kozlowski wrote:
> MACH_SMDKC100 was removed in commit b8529ec1c1b0 ("ARM: S5PC100: no more
> support S5PC100 SoC"). MACH_SMDKV210 and MACH_SMDKC110 in commit
> 28c8331d386 ("ARM: S5PV210: Remove support for board files").
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
^ permalink raw reply
* [PATCH v8 6/7] arm/arm64: vgic: Implement VGICv3 CPU interface access
From: Christoffer Dall @ 2016-11-21 10:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CALicx6uia5_H5Zg_x6GP0nKgZi_513w97igCuDdezySOUNQtBw@mail.gmail.com>
On Fri, Nov 18, 2016 at 10:28:34PM +0530, Vijay Kilari wrote:
> 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
Then add a comment explaining that
> and also to limit the valid aprn registers access as you said above.
> But that has ordering restriction. Which I think we should follow.
>
I'm sorry, now I'm confused. Is there an ordering requirement in the
API, or how should we follow this?
> > 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.
ok
> >
> >> 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.
You could expand the meaning in the API doc for gicv3 and use EINVAL, or
you could expand on what ENXIO means.
Thanks,
-Christoffer
^ permalink raw reply
* [GIT PULL v2 9/10] arm64: tegra: Device tree changes for v4.10-rc1
From: Thierry Reding @ 2016-11-21 10:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118161719.24153-9-thierry.reding@gmail.com>
Hi ARM SoC maintainers,
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/tegra/linux.git tags/tegra-for-4.10-arm64-dt-numeric-ids
for you to fetch changes up to 99575bceebd60b572f0ccf9a900fdb970922ca49:
arm64: tegra: Add NVIDIA P2771 board support (2016-11-21 10:43:42 +0100)
The only change since the original pull request is the replacement of
symbolic names in DTS files by their numerical equivalents to remove the
dependency on the driver branchs, as requested by Olof.
I have three patches to reintroduce these symbols that I can resend after
v4.10-rc1.
Thanks,
Thierry
----------------------------------------------------------------
arm64: tegra: Device tree changes for v4.10-rc1
This adds initial support for Tegra186, the P3310 processor module as
well as the P2771 development board. Not much is functional, but there
is enough to boot to an initial ramdisk with debug serial output.
----------------------------------------------------------------
Joseph Lo (3):
arm64: tegra: Add Tegra186 support
arm64: tegra: Add NVIDIA P3310 processor module support
arm64: tegra: Add NVIDIA P2771 board support
Thierry Reding (6):
arm64: tegra: Add CPU nodes for Tegra186
arm64: tegra: Add serial ports on Tegra186
arm64: tegra: Add I2C controllers on Tegra186
arm64: tegra: Add SDHCI controllers on Tegra186
arm64: tegra: Add GPIO controllers on Tegra186
arm64: tegra: Enable PSCI on P3310
arch/arm64/boot/dts/nvidia/Makefile | 1 +
arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts | 8 +
arch/arm64/boot/dts/nvidia/tegra186-p3310.dtsi | 64 ++++
arch/arm64/boot/dts/nvidia/tegra186.dtsi | 398 +++++++++++++++++++++
4 files changed, 471 insertions(+)
create mode 100644 arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts
create mode 100644 arch/arm64/boot/dts/nvidia/tegra186-p3310.dtsi
create mode 100644 arch/arm64/boot/dts/nvidia/tegra186.dtsi
^ permalink raw reply
* [PATCH v3 10/10] ARM: dts: da850: add usb device node
From: Axel Haslam @ 2016-11-21 10:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <48d0c158-f7c8-0d1e-f06e-b7b28d6f2b93@lechnology.com>
On Mon, Nov 21, 2016 at 3:42 AM, David Lechner <david@lechnology.com> wrote:
> On 11/07/2016 02:39 PM, Axel Haslam wrote:
>>
>> This adds the ohci device node for the da850 soc.
>> It also enables it for the omapl138 hawk board.
>>
>> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
>> ---
>> arch/arm/boot/dts/da850-lcdk.dts | 8 ++++++++
>> arch/arm/boot/dts/da850.dtsi | 8 ++++++++
>> 2 files changed, 16 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/da850-lcdk.dts
>> b/arch/arm/boot/dts/da850-lcdk.dts
>> index 7b8ab21..aaf533e 100644
>> --- a/arch/arm/boot/dts/da850-lcdk.dts
>> +++ b/arch/arm/boot/dts/da850-lcdk.dts
>> @@ -86,6 +86,14 @@
>> };
>> };
>>
>> +&usb_phy {
>> + status = "okay";
>> +};
>> +
>> +&ohci {
>> + status = "okay";
>> +};
>> +
>> &serial2 {
>> pinctrl-names = "default";
>> pinctrl-0 = <&serial2_rxtx_pins>;
>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>> index 2534aab..50e86da 100644
>> --- a/arch/arm/boot/dts/da850.dtsi
>> +++ b/arch/arm/boot/dts/da850.dtsi
>> @@ -405,6 +405,14 @@
>> >;
>> status = "disabled";
>> };
>> + ohci: usb at 0225000 {
>
>
> In commit 2957e36e76c836b167e5e0c1edb578d8a9bd7af6 in the linux-davinci
> tree, the alias for the musb device is usb0. So, I think we should use usb1
> here instead of ohci - or change the usb0 alias to musb.
>
> https://git.kernel.org/cgit/linux/kernel/git/nsekhar/linux-davinci.git/commit/?h=v4.10/dt&id=2957e36e76c836b167e5e0c1edb578d8a9bd7af6
ok, i will change to usb1, since i will be resubmiting this.
>
>> + compatible = "ti,da830-ohci";
>> + reg = <0x225000 0x1000>;
>> + interrupts = <59>;
>> + phys = <&usb_phy 1>;
>> + phy-names = "usb-phy";
>> + status = "disabled";
>> + };
>> gpio: gpio at 226000 {
>> compatible = "ti,dm6441-gpio";
>> gpio-controller;
>>
>
^ permalink raw reply
* [BUG] i2c-designware silently fails on long transfers
From: Mika Westerberg @ 2016-11-21 10:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118193542.GO1041@n2100.armlinux.org.uk>
On Fri, Nov 18, 2016 at 07:35:42PM +0000, Russell King - ARM Linux wrote:
> With reference to this commit:
>
> commit d39f77b06a712fcba6185a20bb209e357923d980
> Author: Andrew Jackson <Andrew.Jackson@arm.com>
> Date: Fri Nov 7 12:10:44 2014 +0000
>
> i2c: designware: prevent early stop on TX FIFO empty
>
> If the Designware core is configured with IC_EMPTYFIFO_HOLD_MASTER_EN
> set to zero, allowing the TX FIFO to become empty causes a STOP
> condition to be generated on the I2C bus. If the transmit FIFO
> threshold is set too high, an erroneous STOP condition can be
> generated on long transfers - particularly where the interrupt
> latency is extended.
>
> Signed-off-by: Andrew Jackson <Andrew.Jackson@arm.com>
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
>
> The TDA998x driver issues long I2C transfers to read the EDID from the
> device - and userspace can also issue large transfers too. However,
> if a DW core is configured with IC_EMPTYFIFO_HOLD_MASTER_EN set as
> zero, the above commit doesn't seem to solve the problem. During
> boot, with the patch below, I see:
>
> [ 1.736549] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x10
> [ 1.736564] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x510
> [ 1.736608] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x504
> [ 1.736799] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x514
> [ 1.736819] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x510
> ...
> [ 1.737986] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x504
> [ 1.738010] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x514
> [ 1.738034] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x504
> [ 1.738039] random: fast init done
> [ 1.740120] i2c_designware 7ffa0000.i2c: i2c_dw_isr: enabled=0x1 stat=0x714
> [ 1.740231] i2c_dw_xfer: ffffffc97657b770:1 -> ffffffc97657b770:1 (0:0) [0 0 3 0] 8 [tx:ffffffc976682380:47] [rx:ffffffc9766823c9:55]
> [ 1.740249] [drm:drm_edid_block_valid] *ERROR* EDID checksum is invalid, remainder is 93
> [ 1.746979] Raw EDID:
> [ 1.747934] 00 ff ff ff ff ff ff 00 34 a9 96 a2 01 01 01 01
> [ 1.752342] 00 17 01 03 80 80 48 78 0a da ff a3 58 4a a2 29
> [ 1.756748] 17 49 4b 21 08 00 31 40 45 40 61 40 81 80 01 01
> [ 1.761153] 01 01 01 01 01 01 02 3a 80 d0 72 38 2d 40 10 2c
> [ 1.765555] 45 80 ba 88 21 00 00 1e 02 00 d0 4e 30 09 12 54
> [ 1.769958] 01 08 02 00 23 36 01 40 01 05 00 80 a1 4c 4b 49
> [ 1.774361] 22 00 00 40 03 00 28 00 23 01 20 00 01 88 00 01
> [ 1.778762] 08 00 00 40 00 02 03 04 0a 00 80 00 02 00 00 40
>
> The significant thing is the "i2c_dw_xfer" line, where I add a print of
> the current state. Here, we can see that the transfer is mid-way, but
> a stop condition has been generated by the hardware, leaving 55 bytes
> to be received.
>
> Unfortunately, the i2c-designware driver ignores this, and believes that
> the transfer completed both fully and successfully, but returns bogus
> data to userspace or the kernel driver. That's really _bad_ behaviour
> by the driver - it should at least return an error.
Totally agree.
> This problem is _soo_ bad that on my Juno, I can't run Xorg (it hits
> this every time we try to read the EDID) nor can I boot with the TV
> connected (it hits this every boot as well.)
>
> I'd go as far as to say that the i2c-designware hardware, when
> configured with this option set to zero, is fundamentally broken for OS
> which do not provide any guarantee for interrupt latency, such as Linux.
>
> The commit above tries to mitigate this by reducing the Tx FIFO
> threshold, so the interrupt is raised sooner, but that's clearly not
> enough for reliable operation.
>
> Another mitigation would be to lower the I2C bus frequency on Juno from
> 400kHz to 100kHz, so that there's 4x longer IRQ latency possible.
> However, even that isn't going to be reliable - even going to 100kHz
> isn't going to allow the above case to be solved - the interrupt is
> delayed by around 2ms, and it takes about 1.4ms to send/receive 16 bytes
> at 100kHz. (9 * 16 / (100*10^3)).
>
> So, I think all hope is lost for i2c-designware on Juno to cope with
> reading the EDID from TDA998x reliably.
:-(
I wonder if we can get it work more reliably by using DMA (provided that
there are DMA channels available for I2C in Juno)? That would allow the
hardware to perform longer reads without relying on how fast the
interrupt handler is able to empty the Rx FIFO.
^ permalink raw reply
* [RFT v2 2/5] ASoC: samsung: smdk_wm8580: Remove old platforms and drop mach-types usage
From: Sylwester Nawrocki @ 2016-11-21 10:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479669895-19124-3-git-send-email-krzk@kernel.org>
On 11/20/2016 08:24 PM, Krzysztof Kozlowski wrote:
> MACH_SMDKC100, MACH_SMDKV210 and MACH_SMDKC110 are no longer supported
> so we can drop the dead code. After this the driver no longer
> differentiates between machines (S3C24xx machines are not supported by
> it) so there is no need to override I2S device id in cpu_dai_name and
> SEC_PLAYBACK dai_link can be removed as well.
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>
> Not tested. The driver did not override .platform_name which looks
> suspicious to me. However I did not want to add changes which could have
> some visible impact on output code.
The patch looks good to me. However the existing smdk64xx sound support
less so. I don't have smdk6410 set up for testing yet, possibly I get
around that next week.
Indeed it's strange .platform_name is not also "samsung-i2s.2".
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
^ permalink raw reply
* [PATCH 1/2] i2c: designware: report short transfers
From: Mika Westerberg @ 2016-11-21 10:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <E1c7p12-0000R0-S8@rmk-PC.armlinux.org.uk>
On Fri, Nov 18, 2016 at 07:40:04PM +0000, Russell King wrote:
> Rather than reporting success for a short transfer due to interrupt
> latency, report an error both to the caller, as well as to the kernel
> log.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
^ permalink raw reply
* [RFT v2 2/5] ASoC: samsung: smdk_wm8580: Remove old platforms and drop mach-types usage
From: Lars-Peter Clausen @ 2016-11-21 10:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <8b81b058-0ba9-f1bc-a0ed-d30604853110@samsung.com>
On 11/21/2016 11:30 AM, Sylwester Nawrocki wrote:
> On 11/20/2016 08:24 PM, Krzysztof Kozlowski wrote:
>> MACH_SMDKC100, MACH_SMDKV210 and MACH_SMDKC110 are no longer supported
>> so we can drop the dead code. After this the driver no longer
>> differentiates between machines (S3C24xx machines are not supported by
>> it) so there is no need to override I2S device id in cpu_dai_name and
>> SEC_PLAYBACK dai_link can be removed as well.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>> ---
>>
>> Not tested. The driver did not override .platform_name which looks
>> suspicious to me. However I did not want to add changes which could have
>> some visible impact on output code.
>
> The patch looks good to me. However the existing smdk64xx sound support
> less so. I don't have smdk6410 set up for testing yet, possibly I get
> around that next week.
> Indeed it's strange .platform_name is not also "samsung-i2s.2".
I think that is a fallout from commit a08485d8fdf6f ("ASoC: Samsung: Do not
register samsung audio dma device as pdev"). Given nobody noticed this in
the last 4 years maybe its time to drop this machine driver as well.
^ permalink raw reply
* [PATCH 2/2] i2c: designware: fix rx fifo depth tracking
From: Mika Westerberg @ 2016-11-21 10:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <E1c7p18-0000R6-4w@rmk-PC.armlinux.org.uk>
On Fri, Nov 18, 2016 at 07:40:10PM +0000, Russell King wrote:
> When loading the TX fifo to receive bytes on the I2C bus, we incorrectly
> count the number of bytes:
>
> rx_limit = dev->rx_fifo_depth - dw_readl(dev, DW_IC_RXFLR);
>
> while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
> if (rx_limit - dev->rx_outstanding <= 0)
> break;
> rx_limit--;
> dev->rx_outstanding++;
> }
>
> DW_IC_RXFLR indicates how many bytes are available to be read in the
> FIFO, dev->rx_fifo_depth is the FIFO size, and dev->rx_outstanding is
> the number of bytes that we've requested to be read so far, but which
> have not been read.
>
> Firstly, increasing dev->rx_outstanding and decreasing rx_limit and then
> comparing them results in each byte consuming "two" bytes in this
> tracking, so this is obviously wrong.
>
> Secondly, the number of bytes that _could_ be received into the FIFO at
> any time is the number of bytes we have so far requested but not yet
> read from the FIFO - in other words dev->rx_outstanding.
>
> So, in order to request enough bytes to fill the RX FIFO, we need to
> request dev->rx_fifo_depth - dev->rx_outstanding bytes.
>
> Modifying the code thusly results in us reaching the maximum number of
> bytes outstanding each time we queue more "receive" operations, provided
> the transfer allows that to happen.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
^ permalink raw reply
* [BUG] i2c-designware silently fails on long transfers
From: Russell King - ARM Linux @ 2016-11-21 10:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161121102901.GF1446@lahna.fi.intel.com>
On Mon, Nov 21, 2016 at 12:29:01PM +0200, Mika Westerberg wrote:
> On Fri, Nov 18, 2016 at 07:35:42PM +0000, Russell King - ARM Linux wrote:
> > Another mitigation would be to lower the I2C bus frequency on Juno from
> > 400kHz to 100kHz, so that there's 4x longer IRQ latency possible.
> > However, even that isn't going to be reliable - even going to 100kHz
> > isn't going to allow the above case to be solved - the interrupt is
> > delayed by around 2ms, and it takes about 1.4ms to send/receive 16 bytes
> > at 100kHz. (9 * 16 / (100*10^3)).
> >
> > So, I think all hope is lost for i2c-designware on Juno to cope with
> > reading the EDID from TDA998x reliably.
>
> :-(
>
> I wonder if we can get it work more reliably by using DMA (provided that
> there are DMA channels available for I2C in Juno)? That would allow the
> hardware to perform longer reads without relying on how fast the
> interrupt handler is able to empty the Rx FIFO.
It would need to DMA to the Tx FIFO to keep it filled - it triggers the
stop condition when the Tx FIFO empties. From what I can see in the
driver, the Tx FIFO not only takes the data but also a "command" to tell
the hardware what to do.
The Rx FIFO would also need DMA to avoid it overflowing due to high
interrupt latency.
I don't know what state DMA is in on the Juno, or even whether it has
DMA - it has a PL330 DMA controller, but I see nothing in the DT files
making use of it.
--
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 v3 10/10] ARM: dts: da850: add usb device node
From: Sekhar Nori @ 2016-11-21 10:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKXjFTN_VvxJ+mk=ooUiGqZJnuJJJn33KsBF2n6SvZNjWpYm3Q@mail.gmail.com>
On Monday 21 November 2016 03:57 PM, Axel Haslam wrote:
> On Mon, Nov 21, 2016 at 3:42 AM, David Lechner <david@lechnology.com> wrote:
>> On 11/07/2016 02:39 PM, Axel Haslam wrote:
>>>
>>> This adds the ohci device node for the da850 soc.
>>> It also enables it for the omapl138 hawk board.
>>>
>>> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
>>> ---
>>> arch/arm/boot/dts/da850-lcdk.dts | 8 ++++++++
>>> arch/arm/boot/dts/da850.dtsi | 8 ++++++++
>>> 2 files changed, 16 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/da850-lcdk.dts
>>> b/arch/arm/boot/dts/da850-lcdk.dts
>>> index 7b8ab21..aaf533e 100644
>>> --- a/arch/arm/boot/dts/da850-lcdk.dts
>>> +++ b/arch/arm/boot/dts/da850-lcdk.dts
>>> @@ -86,6 +86,14 @@
>>> };
>>> };
>>>
>>> +&usb_phy {
>>> + status = "okay";
>>> +};
>>> +
>>> +&ohci {
>>> + status = "okay";
>>> +};
>>> +
>>> &serial2 {
>>> pinctrl-names = "default";
>>> pinctrl-0 = <&serial2_rxtx_pins>;
>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>> index 2534aab..50e86da 100644
>>> --- a/arch/arm/boot/dts/da850.dtsi
>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>> @@ -405,6 +405,14 @@
>>> >;
>>> status = "disabled";
>>> };
>>> + ohci: usb at 0225000 {
>>
>>
>> In commit 2957e36e76c836b167e5e0c1edb578d8a9bd7af6 in the linux-davinci
>> tree, the alias for the musb device is usb0. So, I think we should use usb1
>> here instead of ohci - or change the usb0 alias to musb.
>>
>> https://git.kernel.org/cgit/linux/kernel/git/nsekhar/linux-davinci.git/commit/?h=v4.10/dt&id=2957e36e76c836b167e5e0c1edb578d8a9bd7af6
>
> ok, i will change to usb1, since i will be resubmiting this.
I have already applied a version of this patch. Please re-base against
linux-davinci/master and send a delta patch.
Thanks,
Sekhar
^ permalink raw reply
* [PATCH v3 10/10] ARM: dts: da850: add usb device node
From: Sekhar Nori @ 2016-11-21 10:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <c812d211-fff8-520f-cc1f-6fa69a765498@ti.com>
On Monday 21 November 2016 04:16 PM, Sekhar Nori wrote:
>>> In commit 2957e36e76c836b167e5e0c1edb578d8a9bd7af6 in the linux-davinci
>>> >> tree, the alias for the musb device is usb0. So, I think we should use usb1
>>> >> here instead of ohci - or change the usb0 alias to musb.
>>> >>
>>> >> https://git.kernel.org/cgit/linux/kernel/git/nsekhar/linux-davinci.git/commit/?h=v4.10/dt&id=2957e36e76c836b167e5e0c1edb578d8a9bd7af6
>> >
>> > ok, i will change to usb1, since i will be resubmiting this.
> I have already applied a version of this patch. Please re-base against
> linux-davinci/master and send a delta patch.
Hmm, no. scratch that. I mixed this up with the musb patch I applied.
usb1 sounds good. Please also separate out the soc and board specific
dts additions for your next version.
Thanks,
Sekhar
^ permalink raw reply
* [GIT PULL] Second Round of Renesas ARM Based SoC DT Updates for v4.10
From: Simon Horman @ 2016-11-21 10:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161119014929.GE2543@localhost>
On Fri, Nov 18, 2016 at 05:49:29PM -0800, Olof Johansson wrote:
> On Thu, Nov 17, 2016 at 03:11:45PM +0100, Simon Horman wrote:
> > Hi Olof, Hi Kevin, Hi Arnd,
> >
> > Please consider these second round of Renesas ARM based SoC DT updates for v4.10.
> >
> > This pull request is based on a merge of:
> >
> > * The previous round of such requests, tagged as renesas-dt-for-v4.10,
> > which I have already sent a pull-request for.
> > * The rzg-clock-defs tag of Geert Uytterhoeven's renesas-driver's tree.
> > This is to provide dependencies for adding the r8a7743 and r8a7745 SoCs.
> > * The "Second Round of Renesas ARM Based SoC Drivers Updates for v4.10",
> > tagged as renesas-drivers2-for-v4.10, which I have also sent a pull
> > request for. This is included to provide dependencies for adding device
> > nodes for PRR, and adding the r8a7743 and r8a7745 SoCs.
>
> Again, nack. And again, I don't understand why you create dependencies that
> aren't needed. Please fix.
Hi Olof,
I agree that calling out PRR above was incorrect. Please disregard that.
However, there are dependencies for adding r8a7743 and r8a7745 SoCs
in the form of header files:
* The rzg-clock-defs tag provides dt-bindings/clock/r8a774[35]-cpg-mssr.h
* The renesas-drivers2-for-v4.10 tag provides
dt-bindings/power/r8a774[35]-sysc.h
The drivers branches are usually pretty light-weight. But this time it is a
bit heavy and you rightly raised some questions about it. After some
discussion with Geert we'd like to suggest that for future releases
we provide a "driver-defs" branch which both driver code and DT can
depend on. Thus avoiding pulling (non essential) driver changes into the DT
branch.
Unfortunately its a bit late to do that for v4.10 as the r8a7743 sysc
driver and its defines were already accepted accepted together
(renesas-drivers-for-v4.10 tag). So for this release we would be grateful
if you could re-consider the renesas-drivers2-for-v4.10 tag given the
feedback which Geert has provided. And in turn re-consider this pull
request.
^ permalink raw reply
* [PATCH v3 10/10] ARM: dts: da850: add usb device node
From: Axel Haslam @ 2016-11-21 10:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <e8bc1888-4936-7d8e-cf8a-a3f2b5644ef5@ti.com>
On Mon, Nov 21, 2016 at 11:49 AM, Sekhar Nori <nsekhar@ti.com> wrote:
> On Monday 21 November 2016 04:16 PM, Sekhar Nori wrote:
>>>> In commit 2957e36e76c836b167e5e0c1edb578d8a9bd7af6 in the linux-davinci
>>>> >> tree, the alias for the musb device is usb0. So, I think we should use usb1
>>>> >> here instead of ohci - or change the usb0 alias to musb.
>>>> >>
>>>> >> https://git.kernel.org/cgit/linux/kernel/git/nsekhar/linux-davinci.git/commit/?h=v4.10/dt&id=2957e36e76c836b167e5e0c1edb578d8a9bd7af6
>>> >
>>> > ok, i will change to usb1, since i will be resubmiting this.
>
>> I have already applied a version of this patch. Please re-base against
>> linux-davinci/master and send a delta patch.
>
> Hmm, no. scratch that. I mixed this up with the musb patch I applied.
> usb1 sounds good. Please also separate out the soc and board specific
> dts additions for your next version.
Ok will do.
>
> Thanks,
> Sekhar
^ permalink raw reply
* [GIT PULL] Second Round of Renesas ARM64 Based SoC DT Updates for v4.10
From: Simon Horman @ 2016-11-21 10:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161119014150.GC2543@localhost>
On Fri, Nov 18, 2016 at 05:41:50PM -0800, Olof Johansson wrote:
> On Thu, Nov 17, 2016 at 03:04:55PM +0100, Simon Horman wrote:
> > Hi Olof, Hi Kevin, Hi Arnd,
> >
> > Please consider these second round of Renesas ARM64 based SoC DT updates
> > for v4.10.
> >
> > This pull request is based on a merge of:
> >
> > * The previous round of such requests, tagged as renesas-arm64-dt-for-v4.10,
> > which I have already sent a pull-request for.
> > * The "Second Round of Renesas ARM Based SoC Drivers Updates for v4.10",
> > tagged as renesas-drivers2-for-v4.10, which I have also sent a pull
> > request for. This is included to provide dependencies for adding device
> > nodes for PRR.
>
> Please avoid entangling these pull requests, since now I can't merge DT since
> I'm rejecting your drivers pull request.
>
> I also don't understand why you need a driver branch as base. The PRR
> nodes are literally just a compatible field and a reg entry. It should
> depend on nothing.
>
> So, I'm a little confused here. Please disentangle this and send a separate
> pull request, I'll be happy to merge that.
Sorry about this.
As you correctly point out the PRR patches do not depend on anything and
the merge described above is unnecessary.
I will prepare a fresh pull request based (only) on
renesas-arm64-dt-for-v4.10.
^ permalink raw reply
* [GIT PULL] Renesas ARM Based SoC EtherAVB Updates for v4.10
From: Simon Horman @ 2016-11-21 10:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161119014327.GD2543@localhost>
On Fri, Nov 18, 2016 at 05:43:27PM -0800, Olof Johansson wrote:
> On Thu, Nov 17, 2016 at 03:05:08PM +0100, Simon Horman wrote:
> > Hi Olof, Hi Kevin, Hi Arnd,
> >
> > Please consider these Renesas ARM based SoC EtherAVB updates for v4.10.
> >
> > This pull request is based on the "Second Round of Renesas ARM64 Based SoC
> > DT Updates for v4.10", tagged as arm64-dt-for-v4.10, which I have also sent
> > a pull-request for.
> >
> > The reason for this base is to provide dependencies. For the same reason
> > an ack has been provided by David Miller to facilitate a merge of the patch
> > via the Renesas tree.
>
> And now we have cascading dependencies. Can't merge this either.
>
> Why is the DT branch and this driver branch entangled?! Again, there is
> absolutely no functional dependency between them.
Sorry once again. I would like to withdraw this pull request.
^ permalink raw reply
* serial: imx: regression triggered by newly introduced DSR irq handling
From: Christoph Fritz @ 2016-11-21 11:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160805065845.GI17874@pengutronix.de>
Hey Uwe
On Fri, 2016-08-05 at 08:58 +0200, Uwe Kleine-K?nig wrote:
> Once this is done (and still an issue)
Yes, patch b5ca028fe9ba2090a (ARM: dts: imx6sx: document SION necessity
of ENET1_REF_CLK1) is done and mainline and DSR irq handling is still an
issue.
> I'd suggest to standardize a dt
> property
>
> disable-dsr;
Would you go forward?
> (and the same for the other handshaking lines) and support th{is,ese} in
> the imx uart driver. Reverting 27e16501052e IMO isn't a sensible option.
Okay, I'm fine with a 'disable-dsr' solution.
Thanks
-- Christoph
^ permalink raw reply
* [PATCH] mtd: nand: tango: Use nand_to_mtd() instead of directly accessing chip->mtd
From: Boris Brezillon @ 2016-11-21 11:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5832C579.10101@sigmadesigns.com>
On Mon, 21 Nov 2016 10:59:21 +0100
Marc Gonzalez <marc_gonzalez@sigmadesigns.com> wrote:
> On 21/11/2016 10:03, Boris Brezillon wrote:
>
> > The nand_to_mtd() helper is here to hide internal mtd_info <-> nand_chip
> > association and ease future refactors.
> >
> > Make use of this helper instead of directly accessing chip->mtd.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > drivers/mtd/nand/tango_nand.c | 25 ++++++++++++++++---------
> > 1 file changed, 16 insertions(+), 9 deletions(-)
>
> Acked-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
Applied.
Thanks,
Boris
>
> Regards.
>
^ permalink raw reply
* [BUG] hdlcd gets confused about base address
From: Liviu Dudau @ 2016-11-21 11:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118233733.GP1041@n2100.armlinux.org.uk>
On Fri, Nov 18, 2016 at 11:37:33PM +0000, Russell King - ARM Linux wrote:
> Hi,
Hi Russell,
>
> While testing HDMI with Xorg on the Juno board, I find that when Xorg
> starts up or shuts down, the display is shifted significantly to the
> right and wrapped in the active region. (No sync bars are visible.)
> The timings are correct, it behaves as if the start address has been
> shifted many pixels _into_ the framebuffer.
>
> This occurs whenever the display mode size is changed - using xrandr
> in Xorg shows that changing the resolution triggers the problem
> almost every time, but changing the refresh rate does not.
Thanks for reporting this. To double check your issue, you are booting
with HDLCD using the native monitor resolution as detected via EDID
and then using xrandr to change the display mode. When you do that you
are seeing the image being shifted to the right. Is that a correct
description? (I'm trying to reproduce it here and want to make sure
I've got the details right).
>
> Using devmem2 to disable and re-enable the HDLCD resolves the issue,
> and repeated disable/enable cycles do not make the issue re-appear.
Do you resize the display mode as well afer re-enabling HDLCD?
>
> So, I patched the HDLCD to do this, and testing it with Xorg after
> several repetitions seems to work.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> What I think is going on is that the FIFO or address generator for
> reading data from the AXI bus is not properly reset when changing the
> resolution, and the enable-disable-enable cycle causes the HDLCD
> hardware to sort itself out.
That is likely what is happening. According to the datasheet, changing
the resolution should be done while the HDLCD command mode is disabled,
which is what writing 0 into HDLCD_REG_COMMAND does.
> It's (eg) significantly out - for example,
> to properly align the display, I have to program an address of
> 0xf4ff0200 into the hardware rather than 0xf5000000 - that's 896 pixels
> before the real start of the frame buffer.
What is the resolution you are using?
>
> With this patch, a patch to TDA998x to avoid the i2c-designware issue,
> and xf86-video-armada, I have LXDE running on the Juno.
Can you tell me more about the TDA998x and i2c-designware issue?
Also, I don't think you need to use xf86-video-armada, the mode-setting
driver built into Xorg should be working fine (that is what I've used
in my testing).
>
> Something I also noticed is this:
>
> scanout_start = gem->paddr + plane->state->fb->offsets[0] +
> plane->state->crtc_y * plane->state->fb->pitches[0] +
> plane->state->crtc_x * bpp / 8;
>
> Surely this should be using src_[xy] (which are the position in the
> source - iow, memory, and not crtc_[xy] which is the position on the
> CRTC displayed window. To put it another way, the src_* define the
> region of the source material that is mapped onto a rectangular area
> on the display defined by crtc_*.
Yes, that is a bug and most likely the source of the issue that you are
seeing if my understanding of your testing is correct.
>
> Another note is that since the CRTC can't place the plane in arbitary
> positions and sizes within the active area, should the atomic_check
> ensure that crtc_x = crtc_y = 0, and the crtc width/height are the
> size of the active area?
That should be the case, indeed. I'm going prepare a patch to do that.
>
> drivers/gpu/drm/arm/hdlcd_crtc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
> index 48019ae22ddb..3e97acf6e2a7 100644
> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> @@ -150,6 +150,8 @@ static void hdlcd_crtc_enable(struct drm_crtc *crtc)
> clk_prepare_enable(hdlcd->clk);
> hdlcd_crtc_mode_set_nofb(crtc);
> hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1);
> + hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
> + hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1);
I am not convinced that this is the right fix. If anything, I would put a
hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0); line before hdlcd_crtc_mode_set_nofs(crtc);
line to make sure the command mode is disabled before setting the mode, but
again, I need to understand your use case to make sure that this indeed fixes it.
Best regards,
Liviu
> }
>
> static void hdlcd_crtc_disable(struct drm_crtc *crtc)
>
>
> --
> 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.
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
?\_(?)_/?
^ permalink raw reply
* serial: imx: regression triggered by newly introduced DSR irq handling
From: Uwe Kleine-König @ 2016-11-21 11:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479726011.2807.11.camel@googlemail.com>
Hello Christoph,
On Mon, Nov 21, 2016 at 12:00:11PM +0100, Christoph Fritz wrote:
> On Fri, 2016-08-05 at 08:58 +0200, Uwe Kleine-K?nig wrote:
> > Once this is done (and still an issue)
>
> Yes, patch b5ca028fe9ba2090a (ARM: dts: imx6sx: document SION necessity
> of ENET1_REF_CLK1) is done and mainline and DSR irq handling is still an
> issue.
>
> > I'd suggest to standardize a dt
> > property
> >
> > disable-dsr;
>
> Would you go forward?
Currently I don't have the need and time to do this myself. So if you
want to propose a patch and send if for review to the dt and serial MLs
that would be a good next step. If you add me to Cc (or To) I can take a
look.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* [BUG] i2c-designware silently fails on long transfers
From: Mika Westerberg @ 2016-11-21 11:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161121104329.GB1041@n2100.armlinux.org.uk>
On Mon, Nov 21, 2016 at 10:43:29AM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 12:29:01PM +0200, Mika Westerberg wrote:
> > On Fri, Nov 18, 2016 at 07:35:42PM +0000, Russell King - ARM Linux wrote:
> > > Another mitigation would be to lower the I2C bus frequency on Juno from
> > > 400kHz to 100kHz, so that there's 4x longer IRQ latency possible.
> > > However, even that isn't going to be reliable - even going to 100kHz
> > > isn't going to allow the above case to be solved - the interrupt is
> > > delayed by around 2ms, and it takes about 1.4ms to send/receive 16 bytes
> > > at 100kHz. (9 * 16 / (100*10^3)).
> > >
> > > So, I think all hope is lost for i2c-designware on Juno to cope with
> > > reading the EDID from TDA998x reliably.
> >
> > :-(
> >
> > I wonder if we can get it work more reliably by using DMA (provided that
> > there are DMA channels available for I2C in Juno)? That would allow the
> > hardware to perform longer reads without relying on how fast the
> > interrupt handler is able to empty the Rx FIFO.
>
> It would need to DMA to the Tx FIFO to keep it filled - it triggers the
> stop condition when the Tx FIFO empties. From what I can see in the
> driver, the Tx FIFO not only takes the data but also a "command" to tell
> the hardware what to do.
Yes, the command is either "read" or "write" (meaning even for Rx a
write to the Tx FIFO is needed).
> The Rx FIFO would also need DMA to avoid it overflowing due to high
> interrupt latency.
I guess for Rx we would need to supply a dummy buffer of "read" commands
for the Tx FIFO in addition to reading bytes from Rx FIFO. But still
that might help to improve reliability in case of Juno.
> I don't know what state DMA is in on the Juno, or even whether it has
> DMA - it has a PL330 DMA controller, but I see nothing in the DT files
> making use of it.
OK
^ permalink raw reply
* [PATCH] [media] VPU: mediatek: fix dereference of pdev before checking it is null
From: andrew-ct chen @ 2016-11-21 11:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161116191650.11486-1-colin.king@canonical.com>
On Wed, 2016-11-16 at 19:16 +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> pdev is dereferenced using platform_get_drvdata before a check to
> see if it is null, hence there could be a potential null pointer
> dereference issue. Instead, first check if pdev is null and only then
> deference pdev when initializing vpu.
>
> Found with static analysis by CoverityScan, CID 1357797
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
Reviewed-by: Andrew-CT Chen <andrew-ct.chen@mediatek.com>
> drivers/media/platform/mtk-vpu/mtk_vpu.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/mtk-vpu/mtk_vpu.c b/drivers/media/platform/mtk-vpu/mtk_vpu.c
> index c9bf58c..41f31b2 100644
> --- a/drivers/media/platform/mtk-vpu/mtk_vpu.c
> +++ b/drivers/media/platform/mtk-vpu/mtk_vpu.c
> @@ -523,9 +523,9 @@ static int load_requested_vpu(struct mtk_vpu *vpu,
>
> int vpu_load_firmware(struct platform_device *pdev)
> {
> - struct mtk_vpu *vpu = platform_get_drvdata(pdev);
> + struct mtk_vpu *vpu;
> struct device *dev = &pdev->dev;
> - struct vpu_run *run = &vpu->run;
> + struct vpu_run *run;
> const struct firmware *vpu_fw = NULL;
> int ret;
>
> @@ -533,6 +533,8 @@ int vpu_load_firmware(struct platform_device *pdev)
> dev_err(dev, "VPU platform device is invalid\n");
> return -EINVAL;
> }
> + vpu = platform_get_drvdata(pdev);
> + run = &vpu->run;
>
> mutex_lock(&vpu->vpu_mutex);
> if (vpu->fw_loaded) {
^ 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