* [PATCH v2 7/7] soc: renesas: Identify SoC and register with the SoC bus
From: Arnd Bergmann @ 2016-11-09 16:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477913455-5314-8-git-send-email-geert+renesas@glider.be>
On Monday, October 31, 2016 12:30:55 PM CET Geert Uytterhoeven wrote:
> v2:
> - Drop SoC families and family names; use fixed "Renesas" instead,
I think I'd rather have seen the family names left in there, but it's
not important, so up to you.
> - Use "renesas,prr" and "renesas,cccr" device nodes in DT if
> available, else fall back to hardcoded addresses for compatibility
> with existing DTBs,
I only see patches 2, 3, 5, and 7 in my inbox, so I'm lacking the DT
binding for these, among other things.
It does seem wrong to have a device node for a specific register though.
Shouldn't the node be for the block of registers that these are inside
of?
> - Don't register the SoC bus if the chip ID register is missing,
Why? My objection was to hardcoding the register, not to registering
the device? I think I'd rather see the device registered with an
empty revision string.
> +#define CCCR 0xe600101c /* Common Chip Code Register */
> +#define PRR 0xff000044 /* Product Register */
> +#define PRR3 0xfff00044 /* Product Register on R-Car Gen3 */
> +
> +static const struct of_device_id renesas_socs[] __initconst = {
> +#ifdef CONFIG_ARCH_R8A73A4
> + { .compatible = "renesas,r8a73a4", .data = (void *)PRR, },
> +#endif
> +#ifdef CONFIG_ARCH_R8A7740
> + { .compatible = "renesas,r8a7740", .data = (void *)CCCR, },
> +#endif
My preference here would be to list the register address only for
SoCs that are known to need them, while also having .dtb files that
don't have the nodes.
> +static int __init renesas_soc_init(void)
> +{
> + struct soc_device_attribute *soc_dev_attr;
> + const struct of_device_id *match;
> + void __iomem *chipid = NULL;
> + struct soc_device *soc_dev;
> + struct device_node *np;
> + unsigned int product;
> +
> + np = of_find_matching_node_and_match(NULL, renesas_socs, &match);
> + if (!np)
> + return -ENODEV;
> +
> + of_node_put(np);
> +
> + /* Try PRR first, then CCCR, then hardcoded fallback */
> + np = of_find_compatible_node(NULL, NULL, "renesas,prr");
> + if (!np)
> + np = of_find_compatible_node(NULL, NULL, "renesas,cccr");
> + if (np) {
> + chipid = of_iomap(np, 0);
> + of_node_put(np);
> + } else if (match->data) {
> + chipid = ioremap((uintptr_t)match->data, 4);
> + }
> + if (!chipid)
>
Here, I'd turn the order around and look for the DT nodes of the
devices first. Only if they are not found, look at the compatible
string of the root node. No need to search for a node though,
you know which one it is when you look for a compatible =
"renesas,r8a73a4".
Arnd
^ permalink raw reply
* [PATCH v2 0/7] soc: renesas: Identify SoC and register with the SoC bus
From: Arnd Bergmann @ 2016-11-09 16:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAMuHMdUmpMpizZpq1V-sLA8Cf2q5oOgOVxGOvKXqTHvn+Mj7Tg@mail.gmail.com>
On Wednesday, November 9, 2016 2:34:33 PM CET Geert Uytterhoeven wrote:
> >
> > And Samsung.
> > Shall I create the immutable branch now?
>
> Arnd: are you happy with the new patches and changes?
I still had some comments for patch 7, but that shouldn't stop
you from creating a branch for the first three so everyone can
build on top of that.
Arnd
^ permalink raw reply
* [PATCH V3 0/8] IOMMU probe deferral support
From: Will Deacon @ 2016-11-09 16:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <002001d23a51$ecb01390$c6103ab0$@codeaurora.org>
On Wed, Nov 09, 2016 at 11:54:20AM +0530, Sricharan wrote:
> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> >> index 71ce4b6..a1d0b3c 100644
> >> --- a/drivers/iommu/arm-smmu.c
> >> +++ b/drivers/iommu/arm-smmu.c
> >> @@ -1516,8 +1516,10 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
> >> group = smmu->s2crs[idx].group;
> >> }
> >>
> >> - if (group)
> >> + if (group) {
> >> + iommu_group_get_by_id(iommu_group_id(group));
> >> return group;
> >
> >This might as well just be inline, i.e.:
> >
> > return iommu_group_get_by_id(iommu_group_id(group));
> >
> >It's a shame we have to go all round the houses when we have the group
> >right there, but this is probably the most expedient fix. I guess we can
> >extend the API with some sort of iommu_group_get(group) overload in
> >future if we really want to.
> >
>
> ok, i can send this fix separately then. Otherwise, Will was suggesting on the
> other thread that there should probably be a separate API to increment
> the group refcount or get the group from the existing aliasing device.
> As per me adding the api, looks like another option or post the above ?
I think adding a new function to the API is the way to go -- having code
like what you propose above littered around the drivers is hard to read and
pretty error-prone, since it looks like a NOP to people who aren't already
thinking about ref counts.
Will
^ permalink raw reply
* Summary of LPC guest MSI discussion in Santa Fe
From: Will Deacon @ 2016-11-09 17:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <58228F71.6020108@redhat.com>
On Tue, Nov 08, 2016 at 09:52:33PM -0500, Don Dutile wrote:
> On 11/08/2016 06:35 PM, Alex Williamson wrote:
> >On Tue, 8 Nov 2016 21:29:22 +0100
> >Christoffer Dall <christoffer.dall@linaro.org> wrote:
> >>Is my understanding correct, that you need to tell userspace about the
> >>location of the doorbell (in the IOVA space) in case (2), because even
> >>though the configuration of the device is handled by the (host) kernel
> >>through trapping of the BARs, we have to avoid the VFIO user programming
> >>the device to create other DMA transactions to this particular address,
> >>since that will obviously conflict and either not produce the desired
> >>DMA transactions or result in unintended weird interrupts?
Yes, that's the crux of the issue.
> >Correct, if the MSI doorbell IOVA range overlaps RAM in the VM, then
> >it's potentially a DMA target and we'll get bogus data on DMA read from
> >the device, and lose data and potentially trigger spurious interrupts on
> >DMA write from the device. Thanks,
> >
> That's b/c the MSI doorbells are not positioned *above* the SMMU, i.e.,
> they address match before the SMMU checks are done. if
> all DMA addrs had to go through SMMU first, then the DMA access could
> be ignored/rejected.
That's actually not true :( The SMMU can't generally distinguish between MSI
writes and DMA writes, so it would just see a write transaction to the
doorbell address, regardless of how it was generated by the endpoint.
Will
^ permalink raw reply
* [PATCH] arm64: percpu: kill off final ACCESS_ONCE() uses
From: Catalin Marinas @ 2016-11-09 17:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478283426-6649-1-git-send-email-mark.rutland@arm.com>
On Fri, Nov 04, 2016 at 06:17:06PM +0000, Mark Rutland wrote:
> For several reasons it is preferable to use {READ,WRITE}_ONCE() rather than
> ACCESS_ONCE(). For example, these handle aggregate types, result in shorter
> source code, and better document the intended access (which may be useful for
> instrumentation features such as the upcoming KTSAN).
>
> Over a number of patches, most uses of ACCESS_ONCE() in arch/arm64 have been
> migrated to {READ,WRITE}_ONCE(). For consistency, and the above reasons, this
> patch migrates the final remaining uses.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Will Deacon <will.deacon@arm.com>
I'll queue this for 4.10. Thanks.
--
Catalin
^ permalink raw reply
* [PATCH v2 0/7] soc: renesas: Identify SoC and register with the SoC bus
From: Geert Uytterhoeven @ 2016-11-09 17:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1626072.dpWPnWgGl9@wuerfel>
Hi Arnd,
On Wed, Nov 9, 2016 at 5:56 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday, November 9, 2016 2:34:33 PM CET Geert Uytterhoeven wrote:
>> > And Samsung.
>> > Shall I create the immutable branch now?
>>
>> Arnd: are you happy with the new patches and changes?
>
> I still had some comments for patch 7, but that shouldn't stop
> you from creating a branch for the first three so everyone can
> build on top of that.
Thanks!
What about patch [4/7]?
Haven't you received it? Your address was in the To-line for all 7 patches.
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 v15 4/4] arm: dts: mt2701: Use real clock for UARTs
From: Matthias Brugger @ 2016-11-09 17:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478245388-1412-5-git-send-email-erin.lo@mediatek.com>
On 11/04/2016 08:43 AM, Erin Lo wrote:
> We used to use a fixed rate clock for the UARTs. Now that we have clock
> support we can associate the correct clocks to the UARTs and drop the
> 26MHz fixed rate UART clock.
>
> Signed-off-by: Erin Lo <erin.lo@mediatek.com>
Applied, thanks.
> ---
> arch/arm/boot/dts/mt2701.dtsi | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/boot/dts/mt2701.dtsi b/arch/arm/boot/dts/mt2701.dtsi
> index c9a8dbf..7eab6f4 100644
> --- a/arch/arm/boot/dts/mt2701.dtsi
> +++ b/arch/arm/boot/dts/mt2701.dtsi
> @@ -73,12 +73,6 @@
> #clock-cells = <0>;
> };
>
> - uart_clk: dummy26m {
> - compatible = "fixed-clock";
> - clock-frequency = <26000000>;
> - #clock-cells = <0>;
> - };
> -
> clk26m: oscillator at 0 {
> compatible = "fixed-clock";
> #clock-cells = <0>;
> @@ -186,7 +180,8 @@
> "mediatek,mt6577-uart";
> reg = <0 0x11002000 0 0x400>;
> interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_LOW>;
> - clocks = <&uart_clk>;
> + clocks = <&pericfg CLK_PERI_UART0_SEL>, <&pericfg CLK_PERI_UART0>;
> + clock-names = "baud", "bus";
> status = "disabled";
> };
>
> @@ -195,7 +190,8 @@
> "mediatek,mt6577-uart";
> reg = <0 0x11003000 0 0x400>;
> interrupts = <GIC_SPI 52 IRQ_TYPE_LEVEL_LOW>;
> - clocks = <&uart_clk>;
> + clocks = <&pericfg CLK_PERI_UART1_SEL>, <&pericfg CLK_PERI_UART1>;
> + clock-names = "baud", "bus";
> status = "disabled";
> };
>
> @@ -204,7 +200,8 @@
> "mediatek,mt6577-uart";
> reg = <0 0x11004000 0 0x400>;
> interrupts = <GIC_SPI 53 IRQ_TYPE_LEVEL_LOW>;
> - clocks = <&uart_clk>;
> + clocks = <&pericfg CLK_PERI_UART2_SEL>, <&pericfg CLK_PERI_UART2>;
> + clock-names = "baud", "bus";
> status = "disabled";
> };
>
> @@ -213,7 +210,8 @@
> "mediatek,mt6577-uart";
> reg = <0 0x11005000 0 0x400>;
> interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_LOW>;
> - clocks = <&uart_clk>;
> + clocks = <&pericfg CLK_PERI_UART3_SEL>, <&pericfg CLK_PERI_UART3>;
> + clock-names = "baud", "bus";
> status = "disabled";
> };
> };
>
^ permalink raw reply
* [PATCH 1/5] iommu: Allow taking a reference on a group directly
From: Will Deacon @ 2016-11-09 17:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3922e1f14d8ecb50440b2d9b0d1123f3c9307fc5.1478695557.git.robin.murphy@arm.com>
On Wed, Nov 09, 2016 at 12:47:24PM +0000, Robin Murphy wrote:
> iommu_group_get_for_dev() expects that the IOMMU driver's device_group
> callback return a group with a reference held for the given device.
> Whilst allocating a new group is fine, and pci_device_group() correctly
> handles reusing an existing group, there is no general means for IOMMU
> drivers doing their own group lookup to take additional references on an
> existing group pointer without having to also store device pointers or
> resort to elaborate trickery.
>
> Add an IOMMU-driver-specific function to fill the hole.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> drivers/iommu/iommu.c | 14 ++++++++++++++
> include/linux/iommu.h | 1 +
> 2 files changed, 15 insertions(+)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 9a2f1960873b..b0b052bc6bb5 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -552,6 +552,20 @@ struct iommu_group *iommu_group_get(struct device *dev)
> EXPORT_SYMBOL_GPL(iommu_group_get);
>
> /**
> + * __iommu_group_get - Increment reference on a group
> + * @group: the group to use, must not be NULL
> + *
> + * This function may be called by internal iommu driver group management
> + * when the context of a struct device pointer is not available. It is
> + * not for general use. Returns the given group for convenience.
> + */
> +struct iommu_group *__iommu_group_get(struct iommu_group *group)
> +{
> + kobject_get(group->devices_kobj);
> + return group;
> +}
This probably either wants sticking in a header or exporting to modules.
That said, why do we need the underscores and the comment about internal
group management? That's pretty much already the case for iommu_group_get.
Of course, removing the underscores gives you a naming conflict, but we
could just call it something like "iommu_group_get_ref".
Will
^ permalink raw reply
* [PATCH] fpga zynq: Check the bitstream for validity
From: Mike Looijmans @ 2016-11-09 17:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161109155651.GA15076@obsidianresearch.com>
?On 09-11-16 16:56, Jason Gunthorpe wrote:
> On Wed, Nov 09, 2016 at 03:21:52PM +0100, Mike Looijmans wrote:
>> I think the basic idea behind the commit is flawed to begin with and the
>> patch should be discarded completely. The same discussion was done for the
>> Xilinx FPGA manager driver, which resulted in the decision that the tooling
>> should create a proper file.
>
> That hasn't changed at all, this just produces a clear and obvious
> message about what is wrong instead of 'timed out'.
>
> Remember, again, the Xilinx tools do not produce an acceptable
> bitstream file by default. You need to do very strange and special
> things to get something acceptable to this driver. It is a very easy
> mistake to make and hard to track down if you don't already know these
> details.
>
>> This driver should either become obsolete or at least move into the
>> same direction as the FPGA manager rather than away from that.
>
> I don't understand what you are talking about here, this is a fpga mgr
> driver already, and is consistent with the FPGA manger - the auto
> detect stuff was removed a while ago and isn't coming back.
That's exactly what I mean - the code was kept simple.
> It is perfectly reasonable for fpga manager drivers to pre-validate
> the proposed bitstream, IMHO all of the drivers should try and do
> that step.
>
> Remember, it is deeply unlikely but sending garbage to an FPGA could
> damage it.
Then what's the purpose of pre-validation? Sending valid data should be
the normal case, not the exception.
>> Besides political arguments, there's a more pressing technical argument
>> against this theck as well: The whole check is pointless since the hardware
>> itself already verifies the validity of the stream.
>
> The purpose of the check is not to protect the hardware. The check is
> to help the user provide the correct input because the hardware
> provides no feedback at all on what is wrong with the input.
>
> And again, the out-of-tree Xilinx driver accepted files that this
> driver does not, so having a clear and understandable message is going
> to be very important for users.
Then just create a "validate my bitstream" tool.
I wrote a Python script to do what Xilinx refused to do years ago:
https://github.com/topic-embedded-products/meta-topic/blob/master/recipes-bsp/fpga/fpga-bit-to-bin/fpga-bit-to-bin.py
So apparently it wasn't hard to figure out what to do.
>> doesn't have any effect, the hardware will discard it. There's no reason to
>> waste CPU cycles duplicating this check in software.
>
> In the typical success case we are talking about 5 32 bit compares,
> which isn't even worth considering.
I'm mostly against the complication of the code. The code is more
complex, and that's bad. It's firmware loading code, and it need not be
aware of exactly what it is doing. I did not see any checks like this in
other firmware loading code I've come across.
What you're creating here will require active maintenance.
There are already 7007 and 7012 devices which aren't in your list so the
driver will refuse to load them until someone fills in the IDs.
The bitstream format is actually proprietary and undocumented. Any
"checks" in code are likely based on guesswork and reverse engineering.
We also use partial reprogramming a lot. Did you test that? On all
devices? And we're actually planning to go beyond that. Maybe I'll be
providing parts of the data through ICAP and some through PCAP, that
might even work, but the driver would block it for no apparent reason.
--
Mike Looijmans
Kind regards,
Mike Looijmans
System Expert
TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans at topicproducts.com
Website: www.topicproducts.com
Please consider the environment before printing this e-mail
^ permalink raw reply
* [PATCH V2 2/5] arm64: Allow hw watchpoint at varied offset from base address
From: Pratyush Anand @ 2016-11-09 17:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161108032658.GB20591@arm.com>
Hi Will,
Thanks a lot for your review.
On Tuesday 08 November 2016 08:56 AM, Will Deacon wrote:
> Hi Pratyush,
>
> On Thu, Oct 20, 2016 at 11:18:14AM +0530, Pratyush Anand wrote:
>> ARM64 hardware supports watchpoint at any double word aligned address.
>> However, it can select any consecutive bytes from offset 0 to 7 from that
>> base address. For example, if base address is programmed as 0x420030 and
>> byte select is 0x1C, then access of 0x420032,0x420033 and 0x420034 will
>> generate a watchpoint exception.
>>
>> Currently, we do not have such modularity. We can only program byte,
>> halfword, word and double word access exception from any base address.
>>
>> This patch adds support to overcome above limitations.
>>
>> Signed-off-by: Pratyush Anand <panand@redhat.com>
>> ---
>> arch/arm64/include/asm/hw_breakpoint.h | 2 +-
>> arch/arm64/kernel/hw_breakpoint.c | 45 ++++++++++++++++------------------
>> arch/arm64/kernel/ptrace.c | 5 ++--
>> 3 files changed, 25 insertions(+), 27 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
>> index 115ea2a64520..4f4e58bee9bc 100644
>> --- a/arch/arm64/include/asm/hw_breakpoint.h
>> +++ b/arch/arm64/include/asm/hw_breakpoint.h
>> @@ -118,7 +118,7 @@ struct perf_event;
>> struct pmu;
>>
>> extern int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
>> - int *gen_len, int *gen_type);
>> + int *gen_len, int *gen_type, int *offset);
>> extern int arch_check_bp_in_kernelspace(struct perf_event *bp);
>> extern int arch_validate_hwbkpt_settings(struct perf_event *bp);
>> extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
>> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
>> index 26a6bf77d272..3c2b96803eba 100644
>> --- a/arch/arm64/kernel/hw_breakpoint.c
>> +++ b/arch/arm64/kernel/hw_breakpoint.c
>> @@ -349,7 +349,7 @@ int arch_check_bp_in_kernelspace(struct perf_event *bp)
>> * to generic breakpoint descriptions.
>> */
>> int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
>> - int *gen_len, int *gen_type)
>> + int *gen_len, int *gen_type, int *offset)
>> {
>> /* Type */
>> switch (ctrl.type) {
>> @@ -369,8 +369,10 @@ int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
>> return -EINVAL;
>> }
>>
>> + *offset = ffs(ctrl.len) - 1;
>
> Do we want to fail the length == 0 case early? If you do add that check,
> then you can use __ffs here instead.
Yes, I think, your point can be taken.
>
>> /* Len */
>> - switch (ctrl.len) {
>> + switch (ctrl.len >> *offset) {
>> case ARM_BREAKPOINT_LEN_1:
>> *gen_len = HW_BREAKPOINT_LEN_1;
>> break;
>> @@ -517,18 +519,17 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
>> default:
>> return -EINVAL;
>> }
>> -
>> - info->address &= ~alignment_mask;
>> - info->ctrl.len <<= offset;
>> } else {
>> if (info->ctrl.type == ARM_BREAKPOINT_EXECUTE)
>> alignment_mask = 0x3;
>> else
>> alignment_mask = 0x7;
>> - if (info->address & alignment_mask)
>> - return -EINVAL;
>> + offset = info->address & alignment_mask;
>> }
>>
>> + info->address &= ~alignment_mask;
>> + info->ctrl.len <<= offset;
>
> Urgh, I really hate all this converting to and fro to keep perf happy.
> FWIW, I'm at the point where I would seriously consider ripping out the
> hw_breakpoint code and replacing it with a ptrace-specific backend so we
> just drop all this crap. The only people that seem to use the perf interface
> are those running the (failing) selftests. Given that we have to have
> a bloody thread switch notifier *anyway*, all perf seems to give us is
> complexity and restrictions.
Yes, I agree.
>
>> /*
>> * Disallow per-task kernel breakpoints since these would
>> * complicate the stepping code.
>> @@ -665,8 +666,8 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
>> struct pt_regs *regs)
>> {
>> int i, step = 0, *kernel_step, access;
>> - u32 ctrl_reg;
>> - u64 val, alignment_mask;
>> + u32 ctrl_reg, lens, lene;
>> + u64 val;
>> struct perf_event *wp, **slots;
>> struct debug_info *debug_info;
>> struct arch_hw_breakpoint *info;
>> @@ -684,25 +685,21 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
>> goto unlock;
>>
>> info = counter_arch_bp(wp);
>> - /* AArch32 watchpoints are either 4 or 8 bytes aligned. */
>> - if (is_compat_task()) {
>> - if (info->ctrl.len == ARM_BREAKPOINT_LEN_8)
>> - alignment_mask = 0x7;
>> - else
>> - alignment_mask = 0x3;
>> - } else {
>> - alignment_mask = 0x7;
>> - }
>>
>> - /* Check if the watchpoint value matches. */
>> + /* Check if the watchpoint value and byte select match. */
>> val = read_wb_reg(AARCH64_DBG_REG_WVR, i);
>> - if (val != (addr & ~alignment_mask))
>> - goto unlock;
>> -
>> - /* Possible match, check the byte address select to confirm. */
>> ctrl_reg = read_wb_reg(AARCH64_DBG_REG_WCR, i);
>> decode_ctrl_reg(ctrl_reg, &ctrl);
>> - if (!((1 << (addr & alignment_mask)) & ctrl.len))
>> + lens = ffs(ctrl.len) - 1;
>> + lene = fls(ctrl.len) - 1;
>
> Again, you can use the '__' variants to avoid the '- 1'.
Ok.
>
>> + /*
>> + * FIXME: reported address can be anywhere between "the
>> + * lowest address accessed by the memory access that
>> + * triggered the watchpoint" and "the highest watchpointed
>> + * address accessed by the memory access". So, it may not
>> + * lie in the interval of watchpoint address range.
>> + */
>> + if (addr < val + lens || addr > val + lene)
>> goto unlock;
>>
>> /*
>> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
>> index e0c81da60f76..0eb366a94382 100644
>> --- a/arch/arm64/kernel/ptrace.c
>> +++ b/arch/arm64/kernel/ptrace.c
>> @@ -327,13 +327,13 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned int note_type,
>> struct arch_hw_breakpoint_ctrl ctrl,
>> struct perf_event_attr *attr)
>> {
>> - int err, len, type, disabled = !ctrl.enabled;
>> + int err, len, type, offset, disabled = !ctrl.enabled;
>>
>> attr->disabled = disabled;
>> if (disabled)
>> return 0;
>>
>> - err = arch_bp_generic_fields(ctrl, &len, &type);
>> + err = arch_bp_generic_fields(ctrl, &len, &type, &offset);
>> if (err)
>> return err;
>>
>> @@ -352,6 +352,7 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned int note_type,
>>
>> attr->bp_len = len;
>> attr->bp_type = type;
>> + attr->bp_addr += offset;
>
> That's going to look pretty bizarre from userspace if it decides to read
> back the address registers to find that they've mysteriously changed.
>
> Perhaps ptrace_hbp_get_addr needs to retrieve the address from the
> arch_hw_breakpoint, like we do for the ctrl register. What do you think?
..and that should help...I will give it a try and will test before next
revision.
~Pratyush
^ permalink raw reply
* [PATCH V2 3/5] arm64: hw_breakpoint: Handle inexact watchpoint addresses
From: Pratyush Anand @ 2016-11-09 17:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAJt8pk9tH5nm5sUsKx-dAvW4uzKP+EoRumhYV1mXsU_mk6dRDg@mail.gmail.com>
On Tuesday 08 November 2016 05:28 PM, Pavel Labath wrote:
>>> + if (min_dist > 0 && min_dist != -1) {
>>> >> + /* No exact match found. */
>>> >> + wp = slots[closest_match];
>>> >> + info = counter_arch_bp(wp);
>>> >> + info->trigger = addr;
>>> >> + perf_bp_event(wp, regs);
>>> >> + }
>> >
>> > Why don't we need to bother with the stepping in the case of a non-exact
>> > match?
> Good catch. I think we do. I must have dropped that part somehow.
>
> Pratyush, could you include the attached fixup in the next batch?
Ok, will do.
~Pratyush
^ permalink raw reply
* [PATCH 1/5] iommu: Allow taking a reference on a group directly
From: Robin Murphy @ 2016-11-09 17:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161109172543.GI17771@arm.com>
On 09/11/16 17:25, Will Deacon wrote:
> On Wed, Nov 09, 2016 at 12:47:24PM +0000, Robin Murphy wrote:
>> iommu_group_get_for_dev() expects that the IOMMU driver's device_group
>> callback return a group with a reference held for the given device.
>> Whilst allocating a new group is fine, and pci_device_group() correctly
>> handles reusing an existing group, there is no general means for IOMMU
>> drivers doing their own group lookup to take additional references on an
>> existing group pointer without having to also store device pointers or
>> resort to elaborate trickery.
>>
>> Add an IOMMU-driver-specific function to fill the hole.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>> drivers/iommu/iommu.c | 14 ++++++++++++++
>> include/linux/iommu.h | 1 +
>> 2 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 9a2f1960873b..b0b052bc6bb5 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -552,6 +552,20 @@ struct iommu_group *iommu_group_get(struct device *dev)
>> EXPORT_SYMBOL_GPL(iommu_group_get);
>>
>> /**
>> + * __iommu_group_get - Increment reference on a group
>> + * @group: the group to use, must not be NULL
>> + *
>> + * This function may be called by internal iommu driver group management
>> + * when the context of a struct device pointer is not available. It is
>> + * not for general use. Returns the given group for convenience.
>> + */
>> +struct iommu_group *__iommu_group_get(struct iommu_group *group)
>> +{
>> + kobject_get(group->devices_kobj);
>> + return group;
>> +}
>
> This probably either wants sticking in a header or exporting to modules.
> That said, why do we need the underscores and the comment about internal
> group management? That's pretty much already the case for iommu_group_get.
The definition of struct iommu_group is private to iommu.c, so any
touching of the members has to be in here. The comment is to contrast
with iommu_group_get()'s "This function is called by iommu drivers and
users". This one is explicitly not for users of the API (DMA mapping,
VFIO, etc.), as they really have no business messing with refcounts
directly, and should always be operating in the context of a device;
it's only for the benefit of anyone *implementing* the API. And since
IOMMU drivers aren't modular (yet... ;)) there's no cause for an export.
> Of course, removing the underscores gives you a naming conflict, but we
> could just call it something like "iommu_group_get_ref".
Ideally, this would be the iommu_group_get() to precisely match
iommu_group_put(), and the existing function would renamed something
like iommu_dev_group_get() (or perhaps even all external uses converted
over to iommu_group_get_for_dev()), but that would be an awful lot of
churn for little obvious benefit. Similarly, I nearly added the below
hunk, but it didn't seem worth the bother.
Robin.
----->8-----
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 9a2f1960873b..89d509c59019 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -545,7 +545,7 @@ struct iommu_group *iommu_group_get(struct device *dev)
struct iommu_group *group = dev->iommu_group;
if (group)
- kobject_get(group->devices_kobj);
+ __iommu_group_get(group);
return group;
}
^ permalink raw reply related
* [PATCH net-next 0/2] ARM64: Add Internal PHY support for Meson GXL
From: David Miller @ 2016-11-09 17:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478274683-1503-1-git-send-email-narmstrong@baylibre.com>
From: Neil Armstrong <narmstrong@baylibre.com>
Date: Fri, 4 Nov 2016 16:51:21 +0100
> The Amlogic Meson GXL SoCs have an internal RMII PHY that is muxed with the
> external RGMII pins.
>
> In order to support switching between the two PHYs links, extended registers
> size for mdio-mux-mmioreg must be added.
>
> The DT related patches submitted as RFC in [3] will be sent in a separate
> patchset due to multiple patchsets and DTSI migrations.
>
> Changes since v2 RFC patchset at : [3]
> - Change phy Kconfig/Makefile alphabetic order
> - GXL dtsi cleanup
>
> Changes since original RFC patchset at : [2]
> - Remove meson8b experimental phy switching
> - Switch to mdio-mux-mmioreg with extennded size support
> - Add internal phy support for S905x and p231
> - Add external PHY support for p230
>
> [1] http://lkml.kernel.org/r/1477932286-27482-1-git-send-email-narmstrong at baylibre.com
> [2] http://lkml.kernel.org/r/1477060838-14164-1-git-send-email-narmstrong at baylibre.com
> [3] http://lkml.kernel.org/r/1477932987-27871-1-git-send-email-narmstrong at baylibre.com
Series applied, thanks.
^ permalink raw reply
* [PATCH v7] soc: qcom: add l2 cache perf events driver
From: Mark Rutland @ 2016-11-09 17:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477687813-11412-1-git-send-email-nleeder@codeaurora.org>
Hi Neil,
Apologies for the delay in replying to this.
This is looking good. I have a few specific comments and a couple of
general concerns below.
Will, please see the bit below about cluster/socket aggregation (grep
for your name).
On Fri, Oct 28, 2016 at 04:50:13PM -0400, Neil Leeder wrote:
> +#include <linux/acpi.h>
> +#include <linux/interrupt.h>
> +#include <linux/perf_event.h>
> +#include <linux/platform_device.h>
While scanning the driver. I noticed a number of other headers that are
required for things the driver explicitly references. Please add the
following so that we're not relying on (fragile) transitive includes:
#include <linux/bitops.h>
#include <linux/bug.h>
#include <linux/cpuhotplug.h>
#include <linux/cpumask.h>
#include <linux/device.h>
#include <linux/errno.h>
#include <linux/kernel.h>
#include <linux/list.h>
#include <linux/percpu.h>
#include <linux/smp.h>
#include <linux/spinlock.h>
#include <linux/sysfs.h>
#include <linux/types.h>
#include <asm/barrier.h>
#include <asm/local64.h>
#include <asm/sysreg.h>
[...]
> +/*
> + * Aggregate PMU. Implements the core pmu functions and manages
> + * the hardware PMUs.
> + */
> +struct l2cache_pmu {
> + struct hlist_node node;
> + u32 num_pmus;
> + struct pmu pmu;
> + int num_counters;
> + cpumask_t cpumask;
> + struct platform_device *pdev;
> +};
> +
> +/*
> + * The cache is made up of one or more clusters, each cluster has its own PMU.
> + * Each cluster is associated with one or more CPUs.
> + * This structure represents one of the hardware PMUs.
> + *
> + * Events can be envisioned as a 2-dimensional array. Each column represents
> + * a group of events. There are 8 groups. Only one entry from each
> + * group can be in use at a time. When an event is assigned a counter
> + * by *_event_add(), the counter index is assigned to group_to_counter[group].
If I've followed correctly, this means each group has a dedicated
counter?
I take it groups are not symmetric (i.e. each column has different
events)? Or does each column contain the same events?
Is there any overlap?
> + * This allows *filter_match() to detect and reject conflicting events in
> + * the same group.
> + * Events are specified as 0xCCG, where CC is 2 hex digits specifying
> + * the code (array row) and G specifies the group (column).
> + *
> + * In addition there is a cycle counter event specified by L2CYCLE_CTR_RAW_CODE
> + * which is outside the above scheme.
> + */
> +struct hml2_pmu {
It's not clear to me what "hml2" is, and now we seem to use "cluster"
and "hml2" interchangeably in comments and code. Can you please define
what "hml2" is in a comment, mentioning that we refer to this (or its
container?) as a cluster?
Can we also please s/hml2/cluster/ in the code, for consistency?
> + struct perf_event *events[MAX_L2_CTRS];
> + struct l2cache_pmu *l2cache_pmu;
> + DECLARE_BITMAP(used_counters, MAX_L2_CTRS);
> + DECLARE_BITMAP(used_groups, L2_EVT_GROUP_MAX + 1);
> + int group_to_counter[L2_EVT_GROUP_MAX + 1];
> + int irq;
> + /* The CPU that is used for collecting events on this cluster */
> + int on_cpu;
> + /* All the CPUs associated with this cluster */
> + cpumask_t cluster_cpus;
I'm still uncertain about aggregating all cluster PMUs into a larger
PMU, given the cluster PMUs are logically independent (at least in terms
of the programming model).
However, from what I understand the x86 uncore PMU drivers aggregate
symmetric instances of uncore PMUs (and also aggregate across packages
to the same logical PMU).
Whatever we do, it would be nice for the uncore drivers to align on a
common behaviour (and I think we're currently going the oppposite route
with Cavium's uncore PMU). Will, thoughts?
> + spinlock_t pmu_lock;
> +};
[...]
> +static void hml2_pmu__set_resr(struct hml2_pmu *cluster,
> + u32 event_group, u32 event_cc)
> +{
> + u64 field;
> + u64 resr_val;
> + u32 shift;
> + unsigned long flags;
> +
> + shift = L2PMRESR_GROUP_BITS * event_group;
> + field = ((u64)(event_cc & L2PMRESR_GROUP_MASK) << shift) | L2PMRESR_EN;
The L2PMRESR_EN isn't part of the shifted field...
> +
> + spin_lock_irqsave(&cluster->pmu_lock, flags);
> +
> + resr_val = get_l2_indirect_reg(L2PMRESR);
> + resr_val &= ~(L2PMRESR_GROUP_MASK << shift);
> + resr_val |= field;
... so can we please or that in on a separate line here, e.g.
resr_val |= L2PMRESR_EN;
That said, do we need to set this bit each time we program an event?
> + set_l2_indirect_reg(L2PMRESR, resr_val);
> +
> + spin_unlock_irqrestore(&cluster->pmu_lock, flags);
> +}
[...]
> +static void l2_cache__event_update_from_cluster(struct perf_event *event,
> + struct hml2_pmu *cluster)
> +{
> + struct hw_perf_event *hwc = &event->hw;
> + u64 delta64, prev, now;
> + u32 delta;
> + u32 idx = hwc->idx;
> +
> + do {
> + prev = local64_read(&hwc->prev_count);
> + now = hml2_pmu__counter_get_value(idx);
> + } while (local64_cmpxchg(&hwc->prev_count, prev, now) != prev);
> +
> + if (idx == l2_cycle_ctr_idx) {
> + /*
> + * The cycle counter is 64-bit so needs separate handling
> + * of 64-bit delta.
> + */
> + delta64 = now - prev;
> + local64_add(delta64, &event->count);
> + } else {
> + /*
> + * 32-bit counters need the unsigned 32-bit math to handle
> + * overflow and now < prev
> + */
> + delta = now - prev;
> + local64_add(delta, &event->count);
> + }
> +}
I believe you can get rid of the delta/delta64 split with something like
the below (with a 64-bit delta):
/*
* The cycle counter is 64-bit, but all other counters are
* 32-bit, and we must handle 32-bit overflow explicitly.
*/
delta = now - prev;
if (idx != l2_cycle_ctr_idx)
delta &= 0xffffffff;
local64_add(delta, &event->count);
> +
> +static void l2_cache__cluster_set_period(struct hml2_pmu *cluster,
> + struct hw_perf_event *hwc)
> +{
> + u64 base = L2_MAX_PERIOD - (L2_CNT_PERIOD - 1);
Is L2_CNT_PERIOD a hw-derived value? Or just something that happened to
be small enough to avoid overflow in testing?
> + u32 idx = hwc->idx;
> + u64 prev = local64_read(&hwc->prev_count);
> + u64 value;
> +
> + /*
> + * Limit the maximum period to prevent the counter value
> + * from overtaking the one we are about to program.
> + * Use a starting value which is high enough that after
> + * an overflow, interrupt latency will not cause the count
> + * to reach the base value. If the previous value
> + * is below the base, increase it to be above the base
> + * and update prev_count accordingly. Otherwise if
> + * the previous value is already above the base
> + * nothing needs to be done to prev_count.
> + */
> + if (prev < base) {
> + value = base + prev;
> + local64_set(&hwc->prev_count, value);
> + } else {
> + value = prev;
> + }
> +
> + hml2_pmu__counter_set_value(idx, value);
> +}
Given that we don't do sampling, and are only using the interrupt to
prevent losing events in the case of overflow, I think we can use a
constant value to handle all cases.
Additionally, I think we need to handle the cycle counter separately,
since it has a different width. Something like the below (perhaps with
different constants):
static void l2_cache__cluster_set_period(struct hml2_pmu *cluster,
struct hw_perf_event *hwc)
{
u64 new;
/*
* We limit the max period to half of the max counter value so
* that even in the case of extreme interrupt latency the
* counter will (hopefully) not wrap past its initial value.
*/
if (hwc->idx == l2_cycle_ctr_idx)
new = BIT_ULL(63);
else
new = BIT_ULL(31);
local64_set(&hwc->prev_count, new);
hml2_pmu__counter_set_value(idx, new);
}
> +static int l2_cache__get_event_idx(struct hml2_pmu *cluster,
> + struct perf_event *event)
> +{
> + struct hw_perf_event *hwc = &event->hw;
> + int idx;
> +
> + if (hwc->config_base == L2CYCLE_CTR_RAW_CODE) {
> + if (test_and_set_bit(l2_cycle_ctr_idx, cluster->used_counters))
> + return -EAGAIN;
> +
> + return l2_cycle_ctr_idx;
> + }
> +
> + for (idx = 0; idx < cluster->l2cache_pmu->num_counters - 1; idx++) {
> + if (!test_and_set_bit(idx, cluster->used_counters)) {
> + set_bit(L2_EVT_GROUP(hwc->config_base),
> + cluster->used_groups);
> + return idx;
> + }
> + }
> + /* The counters are all in use. */
> + return -EAGAIN;
> +}
Are we racing with another bitmap manipulation here? Or can we split the find
and set of the bit, i.e.
int num_ctrs = cluster->l2cache_pmu->num_counters;
int idx = find_first_zero_bit(cluster->used_counters, num_ctrs);
if (idx == num_ctrs)
return -EAGAIN;
set_bit(idx, cluster->used_counters)
Otherwise, I think the outer loop can use for_each_clear_bit().
[...]
> +static int l2_cache__event_init(struct perf_event *event)
> +{
> + struct hw_perf_event *hwc = &event->hw;
> + struct hml2_pmu *cluster;
> + struct perf_event *sibling;
> + struct l2cache_pmu *l2cache_pmu;
> +
> + if (event->attr.type != event->pmu->type)
> + return -ENOENT;
> +
> + l2cache_pmu = to_l2cache_pmu(event->pmu);
> +
> + if (hwc->sample_period) {
> + dev_warn(&l2cache_pmu->pdev->dev, "Sampling not supported\n");
> + return -EOPNOTSUPP;
> + }
Please remove these dev_warns, or at lease relegate them to ratelimited
debug messages rather than warning messages. Users can easily trigger
these by accident and/or when probing capabilities in the usual fashion,
and there's no need for them.
The CCN driver is an unfortunate bad example in this respect.
[...]
> + /* Don't allow groups with mixed PMUs, except for s/w events */
> + if (event->group_leader->pmu != event->pmu &&
> + !is_software_event(event->group_leader)) {
> + dev_warn(&l2cache_pmu->pdev->dev,
> + "Can't create mixed PMU group\n");
> + return -EINVAL;
> + }
> +
> + list_for_each_entry(sibling, &event->group_leader->sibling_list,
> + group_entry)
> + if (sibling->pmu != event->pmu &&
> + !is_software_event(sibling)) {
> + dev_warn(&l2cache_pmu->pdev->dev,
> + "Can't create mixed PMU group\n");
> + return -EINVAL;
> + }
> +
> + /* Ensure all events in a group are on the same cpu */
> + cluster = get_hml2_pmu(event->cpu);
> + if ((event->group_leader != event) &&
> + (cluster->on_cpu != event->group_leader->cpu)) {
> + dev_warn(&l2cache_pmu->pdev->dev,
> + "Can't create group on CPUs %d and %d",
> + event->cpu, event->group_leader->cpu);
> + return -EINVAL;
> + }
It's probably worth also checking that the events are co-schedulable
(e.g. they don't conflict column-wise).
[...]
> +static void l2_cache__event_start(struct perf_event *event, int flags)
> +{
> + struct hml2_pmu *cluster;
> + struct hw_perf_event *hwc = &event->hw;
> + int idx = hwc->idx;
> + u32 config;
> + u32 event_cc, event_group;
> +
> + hwc->state = 0;
> +
> + cluster = get_hml2_pmu(event->cpu);
> + l2_cache__cluster_set_period(cluster, hwc);
> +
> + if (hwc->config_base == L2CYCLE_CTR_RAW_CODE) {
> + hml2_pmu__set_evccntcr(0x0);
Nit: s/0x0/0/ -- there's no other hex usage to be consistent with, and a
single digit is clearer.
> + } else {
> + config = hwc->config_base;
> + event_cc = L2_EVT_CODE(config);
> + event_group = L2_EVT_GROUP(config);
> +
> + hml2_pmu__set_evcntcr(idx, 0x0);
Nit: s/0x0/0/ -- as above.
> + hml2_pmu__set_evtyper(idx, event_group);
> + hml2_pmu__set_resr(cluster, event_group, event_cc);
> + hml2_pmu__set_evfilter_sys_mode(idx);
> + }
> +
> + hml2_pmu__counter_enable_interrupt(idx);
> + hml2_pmu__counter_enable(idx);
> +}
> +
> +static void l2_cache__event_stop(struct perf_event *event, int flags)
> +{
> + struct hml2_pmu *cluster;
> + struct hw_perf_event *hwc = &event->hw;
> + int idx = hwc->idx;
> +
> + if (!(hwc->state & PERF_HES_STOPPED)) {
It's better to have an early return, e.g.
/* If already stopped, we have nothing to do */
if (hwc->state & PERF_HES_STOPPED)
return;
...
remaining logic here, not in a block
...
> + cluster = get_hml2_pmu(event->cpu);
> + hml2_pmu__counter_disable_interrupt(idx);
> + hml2_pmu__counter_disable(idx);
> +
> + if (flags & PERF_EF_UPDATE)
> + l2_cache__event_update_from_cluster(event, cluster);
> + hwc->state |= PERF_HES_STOPPED | PERF_HES_UPTODATE;
> + }
> +}
> +
> +static int l2_cache__event_add(struct perf_event *event, int flags)
> +{
> + struct hw_perf_event *hwc = &event->hw;
> + int idx;
> + int err = 0;
> + struct hml2_pmu *cluster;
> +
> + cluster = get_hml2_pmu(event->cpu);
> +
> + idx = l2_cache__get_event_idx(cluster, event);
> + if (idx < 0) {
> + err = idx;
> + return err;
> + }
You can return idx directly and avoid the block here.
> + hwc->idx = idx;
> + hwc->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
> + cluster->events[idx] = event;
> + cluster->group_to_counter[L2_EVT_GROUP(hwc->config_base)] = idx;
> + local64_set(&hwc->prev_count, 0ULL);
Nit: s/0ULL/0/
[...]
> +static int l2_cache_filter_match(struct perf_event *event)
> +{
> + struct hw_perf_event *hwc = &event->hw;
> + struct hml2_pmu *cluster = get_hml2_pmu(event->cpu);
> + unsigned int group = L2_EVT_GROUP(hwc->config_base);
> +
> + /* check for column exclusion: group already in use by another event */
> + if (test_bit(group, cluster->used_groups) &&
> + cluster->events[cluster->group_to_counter[group]] != event)
> + return 0;
> +
> + return 1;
> +}
I'm still not keen on this. I'm still hoping to kill off filter_match,
as for many reasons it is not great, and I know that x86 have similar
scheduling requirements, and somehow manage without this.
It would be worth adding justification for this in a comment block
above, such as why you need this, and why this won't result in
sub-optimal behaviour.
[...]
> + if (acpi_bus_get_device(ACPI_HANDLE(dev), &device))
> + return -ENODEV;
> +
> + if (kstrtol(device->pnp.unique_id, 10, &fw_cluster_id) < 0) {
> + dev_err(&pdev->dev, "unable to read ACPI uid\n");
> + return -ENODEV;
> + }
> + cluster->l2cache_pmu = l2cache_pmu;
> + for_each_present_cpu(cpu) {
> + if (topology_physical_package_id(cpu) == fw_cluster_id) {
> + cpumask_set_cpu(cpu, &cluster->cluster_cpus);
> + per_cpu(pmu_cluster, cpu) = cluster;
> + }
> + }
I'm still uneasy about this.
The topology_* API doesn't have a well-defined mapping to the MPIDR.Aff*
levels, which itself also don't have a well-defined mapping to your
hardware's clusters (and therefore fw_cluster_id).
Thus, I'm rather worried that this is going to get broken easily, either
by changes in the kernel, or in future HW revisions where the mapping of
clusters to MPIDR.Aff* fields changes.
[...]
> + err = devm_request_irq(&pdev->dev, irq, l2_cache__handle_irq,
> + IRQF_NOBALANCING, "l2-cache-pmu", cluster);
I believe this also needs IRQF_NO_THREAD.
[...]
> + cluster->pmu_lock = __SPIN_LOCK_UNLOCKED(cluster->pmu_lock);
Please use:
spin_lock_init(&cluster->pmu_lock);
[...]
> + platform_set_drvdata(pdev, l2cache_pmu);
> + l2cache_pmu->pmu = (struct pmu) {
> + /* suffix is instance id for future use with multiple sockets */
> + .name = "l2cache_0",
Sorry to go back and forth on this, but as above with the aggregation
comments, we might not need/want the suffix after all, or we might want
per-cluster PMUs.
> + .task_ctx_nr = perf_invalid_context,
> + .pmu_enable = l2_cache__pmu_enable,
> + .pmu_disable = l2_cache__pmu_disable,
> + .event_init = l2_cache__event_init,
> + .add = l2_cache__event_add,
> + .del = l2_cache__event_del,
> + .start = l2_cache__event_start,
> + .stop = l2_cache__event_stop,
> + .read = l2_cache__event_read,
> + .attr_groups = l2_cache_pmu_attr_grps,
> + .filter_match = l2_cache_filter_match,
> + };
As a general note, can we please s/__/_/ for function names? The double
underscore gains us nothing.
[...]
> + l2_counter_present_mask = GENMASK(l2cache_pmu->num_counters - 2, 0) |
> + L2PM_CC_ENABLE;
This looks confusing, and it's the only use of L2PM_CC_ENABLE.
Perhaps use BIT(L2CYCLE_CTR_BIT) instead, and get rid of L2PM_CC_ENABLE?
[...]
> + err = perf_pmu_register(&l2cache_pmu->pmu, l2cache_pmu->pmu.name, -1);
> + if (err < 0) {
> + dev_err(&pdev->dev, "Error %d registering L2 cache PMU\n", err);
> + return err;
> + }
> +
> + dev_info(&pdev->dev, "Registered L2 cache PMU using %d HW PMUs\n",
> + l2cache_pmu->num_pmus);
> +
> + err = cpuhp_state_add_instance_nocalls(CPUHP_AP_PERF_ARM_QCOM_L2_ONLINE,
> + &l2cache_pmu->node);
> +
> + return err;
> +}
Can we race with hotplug? I believe the hotplug callback registration
should come before the perf_pmu_register()
> +static int l2_cache_pmu_remove(struct platform_device *pdev)
> +{
> + struct l2cache_pmu *l2cache_pmu =
> + to_l2cache_pmu(platform_get_drvdata(pdev));
> +
> + cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_QCOM_L2_ONLINE,
> + &l2cache_pmu->node);
> + perf_pmu_unregister(&l2cache_pmu->pmu);
> + return 0;
> +}
Mirroring the above, I think we should unregister the PMU before
removing the hotplug callbacks.
Thanks,
Mark.
^ permalink raw reply
* [PATCH 1/5] iommu: Allow taking a reference on a group directly
From: Will Deacon @ 2016-11-09 18:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <8b35afe8-7e09-c2d3-91ae-5d2a10da6fc8@arm.com>
On Wed, Nov 09, 2016 at 05:46:16PM +0000, Robin Murphy wrote:
> On 09/11/16 17:25, Will Deacon wrote:
> > On Wed, Nov 09, 2016 at 12:47:24PM +0000, Robin Murphy wrote:
> >> iommu_group_get_for_dev() expects that the IOMMU driver's device_group
> >> callback return a group with a reference held for the given device.
> >> Whilst allocating a new group is fine, and pci_device_group() correctly
> >> handles reusing an existing group, there is no general means for IOMMU
> >> drivers doing their own group lookup to take additional references on an
> >> existing group pointer without having to also store device pointers or
> >> resort to elaborate trickery.
> >>
> >> Add an IOMMU-driver-specific function to fill the hole.
> >>
> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> >> ---
> >> drivers/iommu/iommu.c | 14 ++++++++++++++
> >> include/linux/iommu.h | 1 +
> >> 2 files changed, 15 insertions(+)
> >>
> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >> index 9a2f1960873b..b0b052bc6bb5 100644
> >> --- a/drivers/iommu/iommu.c
> >> +++ b/drivers/iommu/iommu.c
> >> @@ -552,6 +552,20 @@ struct iommu_group *iommu_group_get(struct device *dev)
> >> EXPORT_SYMBOL_GPL(iommu_group_get);
> >>
> >> /**
> >> + * __iommu_group_get - Increment reference on a group
> >> + * @group: the group to use, must not be NULL
> >> + *
> >> + * This function may be called by internal iommu driver group management
> >> + * when the context of a struct device pointer is not available. It is
> >> + * not for general use. Returns the given group for convenience.
> >> + */
> >> +struct iommu_group *__iommu_group_get(struct iommu_group *group)
> >> +{
> >> + kobject_get(group->devices_kobj);
> >> + return group;
> >> +}
> >
> > This probably either wants sticking in a header or exporting to modules.
> > That said, why do we need the underscores and the comment about internal
> > group management? That's pretty much already the case for iommu_group_get.
>
> The definition of struct iommu_group is private to iommu.c, so any
> touching of the members has to be in here. The comment is to contrast
> with iommu_group_get()'s "This function is called by iommu drivers and
> users". This one is explicitly not for users of the API (DMA mapping,
> VFIO, etc.), as they really have no business messing with refcounts
> directly, and should always be operating in the context of a device;
But they can already do it if they want to, using the horrible group id
hack that's been doing the rounds. The IOMMU API is already low-level
enough, so I don't think trying to split it up like this is helpful.
Hell, people can even just dip in and bump the kobject directly, or grab a
handle to a device in the group already and call iommu_group_get.
That said, it doesn't look like iommu_group_get_by_id actually has any
callers in tree, so maybe we could kill it.
> it's only for the benefit of anyone *implementing* the API. And since
> IOMMU drivers aren't modular (yet... ;)) there's no cause for an export.
>
> > Of course, removing the underscores gives you a naming conflict, but we
> > could just call it something like "iommu_group_get_ref".
>
> Ideally, this would be the iommu_group_get() to precisely match
> iommu_group_put(), and the existing function would renamed something
> like iommu_dev_group_get() (or perhaps even all external uses converted
> over to iommu_group_get_for_dev()), but that would be an awful lot of
> churn for little obvious benefit. Similarly, I nearly added the below
> hunk, but it didn't seem worth the bother.
I'd still rather the new function was renamed. We already have the group,
so calling a weird underscore version of iommu_group_get is really
counter-intuitive.
Joerg -- do you have a preference?
Will
^ permalink raw reply
* [PATCH 1/3] Documentation: DT: Add entry for FSL LS1012A RDB, FRDM, QDS boards
From: Harninder Rai @ 2016-11-09 18:04 UTC (permalink / raw)
To: linux-arm-kernel
Signed-off-by: Harninder Rai <harninder.rai@nxp.com>
Signed-off-by: Bhaskar Upadhaya <Bhaskar.Upadhaya@nxp.com>
---
Documentation/devicetree/bindings/arm/fsl.txt | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
index d6ee9c6..3b01338 100644
--- a/Documentation/devicetree/bindings/arm/fsl.txt
+++ b/Documentation/devicetree/bindings/arm/fsl.txt
@@ -139,6 +139,22 @@ Example:
Freescale ARMv8 based Layerscape SoC family Device Tree Bindings
----------------------------------------------------------------
+LS1012A SoC
+Required root node properties:
+ - compatible = "fsl,ls1012a";
+
+LS1012A ARMv8 based RDB Board
+Required root node properties:
+ - compatible = "fsl,ls1012a-rdb", "fsl,ls1012a";
+
+LS1012A ARMv8 based FRDM Board
+Required root node properties:
+ - compatible = "fsl,ls1012a-frdm", "fsl,ls1012a";
+
+LS1012A ARMv8 based QDS Board
+Required root node properties:
+ - compatible = "fsl,ls1012a-qds", "fsl,ls1012a";
+
LS1043A SoC
Required root node properties:
- compatible = "fsl,ls1043a";
--
2.7.4
^ permalink raw reply related
* [PATCH 2/3] Documentation: DT: add LS1012A compatible for SCFG and DCFG
From: Harninder Rai @ 2016-11-09 18:04 UTC (permalink / raw)
To: linux-arm-kernel
Signed-off-by: Harninder Rai <harninder.rai@nxp.com>
Signed-off-by: Bhaskar Upadhaya <Bhaskar.Upadhaya@nxp.com>
---
Documentation/devicetree/bindings/arm/fsl.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
index 3b01338..c9c567a 100644
--- a/Documentation/devicetree/bindings/arm/fsl.txt
+++ b/Documentation/devicetree/bindings/arm/fsl.txt
@@ -108,7 +108,7 @@ status.
- compatible: Should contain a chip-specific compatible string,
Chip-specific strings are of the form "fsl,<chip>-scfg",
The following <chip>s are known to be supported:
- ls1021a, ls1043a, ls1046a, ls2080a.
+ ls1012a, ls1021a, ls1043a, ls1046a, ls2080a.
- reg: should contain base address and length of SCFG memory-mapped registers
@@ -126,7 +126,7 @@ core start address and release the secondary core from holdoff and startup.
- compatible: Should contain a chip-specific compatible string,
Chip-specific strings are of the form "fsl,<chip>-dcfg",
The following <chip>s are known to be supported:
- ls1021a, ls1043a, ls1046a, ls2080a.
+ ls1012a, ls1021a, ls1043a, ls1046a, ls2080a.
- reg : should contain base address and length of DCFG memory-mapped registers
--
2.7.4
^ permalink raw reply related
* [PATCH 3/3] dt-bindings: clockgen: Add compatible string for LS1012A
From: Harninder Rai @ 2016-11-09 18:10 UTC (permalink / raw)
To: linux-arm-kernel
Signed-off-by: Harninder Rai <harninder.rai@nxp.com>
Signed-off-by: Bhaskar Upadhaya <Bhaskar.Upadhaya@nxp.com>
---
Documentation/devicetree/bindings/clock/qoriq-clock.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/clock/qoriq-clock.txt b/Documentation/devicetree/bindings/clock/qoriq-clock.txt
index df9cb5a..aa3526f 100644
--- a/Documentation/devicetree/bindings/clock/qoriq-clock.txt
+++ b/Documentation/devicetree/bindings/clock/qoriq-clock.txt
@@ -31,6 +31,7 @@ Required properties:
* "fsl,t4240-clockgen"
* "fsl,b4420-clockgen"
* "fsl,b4860-clockgen"
+ * "fsl,ls1012a-clockgen"
* "fsl,ls1021a-clockgen"
* "fsl,ls1043a-clockgen"
* "fsl,ls1046a-clockgen"
--
2.7.4
^ permalink raw reply related
* [PATCH] arm64: Add DTS support for FSL's LS1012A SoC
From: Harninder Rai @ 2016-11-09 18:11 UTC (permalink / raw)
To: linux-arm-kernel
Add the device tree support for FSL LS1012A SoC.
Following levels of DTSI/DTS files have been created for the LS1012A
SoC family:
- fsl-ls1012a.dtsi:
DTS-Include file for FSL LS1012A SoC.
- fsl-ls1012a-frdm.dts:
DTS file for FSL LS1012A FRDM board.
- fsl-ls1012a-qds.dts:
DTS file for FSL LS1012A QDS board.
- fsl-ls1012a-rdb.dts:
DTS file for FSL LS1012A RDB board.
Signed-off-by: Harninder Rai <harninder.rai@nxp.com>
Signed-off-by: Bhaskar Upadhaya <Bhaskar.Upadhaya@nxp.com>
---
arch/arm64/boot/dts/freescale/Makefile | 3 +
arch/arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts | 115 ++++++++++
arch/arm64/boot/dts/freescale/fsl-ls1012a-qds.dts | 128 +++++++++++
arch/arm64/boot/dts/freescale/fsl-ls1012a-rdb.dts | 59 +++++
arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi | 248 +++++++++++++++++++++
5 files changed, 553 insertions(+)
create mode 100644 arch/arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
create mode 100644 arch/arm64/boot/dts/freescale/fsl-ls1012a-qds.dts
create mode 100644 arch/arm64/boot/dts/freescale/fsl-ls1012a-rdb.dts
create mode 100644 arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi
diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
index 6602718..39db645 100644
--- a/arch/arm64/boot/dts/freescale/Makefile
+++ b/arch/arm64/boot/dts/freescale/Makefile
@@ -1,3 +1,6 @@
+dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1012a-frdm.dtb
+dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1012a-qds.dtb
+dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1012a-rdb.dtb
dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1043a-qds.dtb
dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1043a-rdb.dtb
dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1046a-qds.dtb
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts b/arch/arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
new file mode 100644
index 0000000..1f2da79
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
@@ -0,0 +1,115 @@
+/*
+ * Device Tree file for Freescale LS1012A Freedom Board.
+ *
+ * Copyright 2016, Freescale Semiconductor
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPLv2 or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ * a) This library 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 library 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.
+ *
+ * Or, alternatively,
+ *
+ * b) Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+/dts-v1/;
+
+#include "fsl-ls1012a.dtsi"
+
+/ {
+ model = "LS1012A Freedom Board";
+ compatible = "fsl,ls1012a-frdm", "fsl,ls1012a";
+
+ sys_mclk: clock-mclk {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <25000000>;
+ };
+
+ reg_1p8v: regulator at 0 {
+ compatible = "regulator-fixed";
+ regulator-name = "1P8V";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-always-on;
+ };
+
+ sound {
+ compatible = "simple-audio-card";
+ simple-audio-card,format = "i2s";
+ simple-audio-card,widgets =
+ "Microphone", "Microphone Jack",
+ "Headphone", "Headphone Jack",
+ "Speaker", "Speaker Ext",
+ "Line", "Line In Jack";
+ simple-audio-card,routing =
+ "MIC_IN", "Microphone Jack",
+ "Microphone Jack", "Mic Bias",
+ "LINE_IN", "Line In Jack",
+ "Headphone Jack", "HP_OUT",
+ "Speaker Ext", "LINE_OUT";
+
+ simple-audio-card,cpu {
+ sound-dai = <&sai2>;
+ frame-master;
+ bitclock-master;
+ };
+
+ simple-audio-card,codec {
+ sound-dai = <&codec>;
+ frame-master;
+ bitclock-master;
+ system-clock-frequency = <25000000>;
+ };
+ };
+};
+
+&i2c0 {
+ status = "okay";
+
+ codec: sgtl5000 at a {
+ #sound-dai-cells = <0>;
+ compatible = "fsl,sgtl5000";
+ reg = <0xa>;
+ VDDA-supply = <®_1p8v>;
+ VDDIO-supply = <®_1p8v>;
+ clocks = <&sys_mclk>;
+ };
+};
+
+&duart0 {
+ status = "okay";
+};
+
+&sai2 {
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1012a-qds.dts b/arch/arm64/boot/dts/freescale/fsl-ls1012a-qds.dts
new file mode 100644
index 0000000..ca680a7
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1012a-qds.dts
@@ -0,0 +1,128 @@
+/*
+ * Device Tree file for Freescale LS1012A QDS Board.
+ *
+ * Copyright 2016, Freescale Semiconductor
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPLv2 or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ * a) This library 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 library 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.
+ *
+ * Or, alternatively,
+ *
+ * b) Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+/dts-v1/;
+
+#include "fsl-ls1012a.dtsi"
+
+/ {
+ model = "LS1012A QDS Board";
+ compatible = "fsl,ls1012a-qds", "fsl,ls1012a";
+
+ sys_mclk: clock-mclk {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <24576000>;
+ };
+
+ reg_3p3v: regulator at 0 {
+ compatible = "regulator-fixed";
+ regulator-name = "3P3V";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-always-on;
+ };
+
+ sound {
+ compatible = "simple-audio-card";
+ simple-audio-card,format = "i2s";
+ simple-audio-card,widgets =
+ "Microphone", "Microphone Jack",
+ "Headphone", "Headphone Jack",
+ "Speaker", "Speaker Ext",
+ "Line", "Line In Jack";
+ simple-audio-card,routing =
+ "MIC_IN", "Microphone Jack",
+ "Microphone Jack", "Mic Bias",
+ "LINE_IN", "Line In Jack",
+ "Headphone Jack", "HP_OUT",
+ "Speaker Ext", "LINE_OUT";
+
+ simple-audio-card,cpu {
+ sound-dai = <&sai2>;
+ frame-master;
+ bitclock-master;
+ };
+
+ simple-audio-card,codec {
+ sound-dai = <&codec>;
+ frame-master;
+ bitclock-master;
+ system-clock-frequency = <24576000>;
+ };
+ };
+};
+
+&i2c0 {
+ status = "okay";
+
+ pca9547 at 77 {
+ compatible = "nxp,pca9547";
+ reg = <0x77>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ i2c at 4 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x4>;
+
+ codec: sgtl5000 at a {
+ #sound-dai-cells = <0>;
+ compatible = "fsl,sgtl5000";
+ reg = <0xa>;
+ VDDA-supply = <®_3p3v>;
+ VDDIO-supply = <®_3p3v>;
+ clocks = <&sys_mclk>;
+ };
+ };
+ };
+};
+
+&duart0 {
+ status = "okay";
+};
+
+&sai2 {
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1012a-rdb.dts b/arch/arm64/boot/dts/freescale/fsl-ls1012a-rdb.dts
new file mode 100644
index 0000000..924dad6
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1012a-rdb.dts
@@ -0,0 +1,59 @@
+/*
+ * Device Tree file for Freescale LS1012A RDB Board.
+ *
+ * Copyright 2016, Freescale Semiconductor
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPLv2 or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ * a) This library 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 library 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.
+ *
+ * Or, alternatively,
+ *
+ * b) Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+/dts-v1/;
+
+#include "fsl-ls1012a.dtsi"
+
+/ {
+ model = "LS1012A RDB Board";
+ compatible = "fsl,ls1012a-rdb", "fsl,ls1012a";
+};
+
+&i2c0 {
+ status = "okay";
+};
+
+&duart0 {
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi
new file mode 100644
index 0000000..0bf5b64
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi
@@ -0,0 +1,248 @@
+/*
+ * Device Tree Include file for Freescale Layerscape-1012A family SoC.
+ *
+ * Copyright 2016, Freescale Semiconductor
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPLv2 or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ * a) This library 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 library 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.
+ *
+ * Or, alternatively,
+ *
+ * b) Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include <dt-bindings/interrupt-controller/irq.h>
+
+/ {
+ compatible = "fsl,ls1012a";
+ interrupt-parent = <&gic>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ cpus {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ cpu0: cpu at 0 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53";
+ reg = <0x0>;
+ clocks = <&clockgen 1 0>;
+ #cooling-cells = <2>;
+ };
+ };
+
+ sysclk: sysclk {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <100000000>;
+ clock-output-names = "sysclk";
+ };
+
+ timer {
+ compatible = "arm,armv8-timer";
+ /* Physical Secure PPI */
+ interrupts = <1 13 IRQ_TYPE_LEVEL_LOW>,
+ /* Physical Non-Secure PPI */
+ <1 14 IRQ_TYPE_LEVEL_LOW>,
+ /* Virtual PPI */
+ <1 11 IRQ_TYPE_LEVEL_LOW>,
+ /* Hypervisor PPI */
+ <1 10 IRQ_TYPE_LEVEL_LOW>;
+ };
+
+ pmu {
+ compatible = "arm,armv8-pmuv3";
+ interrupts = <0 106 IRQ_TYPE_LEVEL_LOW>;
+ };
+
+ gic: interrupt-controller at 1400000 {
+ compatible = "arm,gic-400";
+ #interrupt-cells = <3>;
+ interrupt-controller;
+ reg = <0x0 0x1401000 0 0x1000>, /* GICD */
+ <0x0 0x1402000 0 0x2000>, /* GICC */
+ <0x0 0x1404000 0 0x2000>, /* GICH */
+ <0x0 0x1406000 0 0x2000>; /* GICV */
+ interrupts = <1 9 IRQ_TYPE_LEVEL_LOW>;
+ };
+
+ reboot {
+ compatible = "syscon-reboot";
+ regmap = <&dcfg>;
+ offset = <0xb0>;
+ mask = <0x02>;
+ };
+
+ soc {
+ compatible = "simple-bus";
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+
+ clockgen: clocking at 1ee1000 {
+ compatible = "fsl,ls1012a-clockgen";
+ reg = <0x0 0x1ee1000 0x0 0x1000>;
+ #clock-cells = <2>;
+ clocks = <&sysclk>;
+ };
+
+ scfg: scfg at 1570000 {
+ compatible = "fsl,ls1012a-scfg", "syscon";
+ reg = <0x0 0x1570000 0x0 0x10000>;
+ big-endian;
+ };
+
+ dcfg: dcfg at 1ee0000 {
+ compatible = "fsl,ls1012a-dcfg",
+ "syscon";
+ reg = <0x0 0x1ee0000 0x0 0x10000>;
+ big-endian;
+ };
+
+ i2c0: i2c at 2180000 {
+ compatible = "fsl,vf610-i2c";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x0 0x2180000 0x0 0x10000>;
+ interrupts = <0 56 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&clockgen 4 0>;
+ status = "disabled";
+ };
+
+ i2c1: i2c at 2190000 {
+ compatible = "fsl,vf610-i2c";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x0 0x2190000 0x0 0x10000>;
+ interrupts = <0 57 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&clockgen 4 0>;
+ status = "disabled";
+ };
+
+ duart0: serial at 21c0500 {
+ compatible = "fsl,ns16550", "ns16550a";
+ reg = <0x00 0x21c0500 0x0 0x100>;
+ interrupts = <0 54 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clockgen 4 0>;
+ };
+
+ duart1: serial at 21c0600 {
+ compatible = "fsl,ns16550", "ns16550a";
+ reg = <0x00 0x21c0600 0x0 0x100>;
+ interrupts = <0 54 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clockgen 4 0>;
+ };
+
+ gpio0: gpio at 2300000 {
+ compatible = "fsl,qoriq-gpio";
+ reg = <0x0 0x2300000 0x0 0x10000>;
+ interrupts = <0 66 IRQ_TYPE_LEVEL_LOW>;
+ gpio-controller;
+ #gpio-cells = <2>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ };
+
+ gpio1: gpio at 2310000 {
+ compatible = "fsl,qoriq-gpio";
+ reg = <0x0 0x2310000 0x0 0x10000>;
+ interrupts = <0 67 IRQ_TYPE_LEVEL_LOW>;
+ gpio-controller;
+ #gpio-cells = <2>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ };
+
+ wdog0: wdog at 2ad0000 {
+ compatible = "fsl,ls1012a-wdt",
+ "fsl,imx21-wdt";
+ reg = <0x0 0x2ad0000 0x0 0x10000>;
+ interrupts = <0 83 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&clockgen 4 0>;
+ big-endian;
+ };
+
+ sai1: sai at 2b50000 {
+ #sound-dai-cells = <0>;
+ compatible = "fsl,vf610-sai";
+ reg = <0x0 0x2b50000 0x0 0x10000>;
+ interrupts = <0 148 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&clockgen 4 3>, <&clockgen 4 3>,
+ <&clockgen 4 3>, <&clockgen 4 3>;
+ clock-names = "bus", "mclk1", "mclk2", "mclk3";
+ dma-names = "tx", "rx";
+ dmas = <&edma0 1 47>,
+ <&edma0 1 46>;
+ status = "disabled";
+ };
+
+ sai2: sai at 2b60000 {
+ #sound-dai-cells = <0>;
+ compatible = "fsl,vf610-sai";
+ reg = <0x0 0x2b60000 0x0 0x10000>;
+ interrupts = <0 149 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&clockgen 4 3>, <&clockgen 4 3>,
+ <&clockgen 4 3>, <&clockgen 4 3>;
+ clock-names = "bus", "mclk1", "mclk2", "mclk3";
+ dma-names = "tx", "rx";
+ dmas = <&edma0 1 45>,
+ <&edma0 1 44>;
+ status = "disabled";
+ };
+
+ edma0: edma at 2c00000 {
+ #dma-cells = <2>;
+ compatible = "fsl,vf610-edma";
+ reg = <0x0 0x2c00000 0x0 0x10000>,
+ <0x0 0x2c10000 0x0 0x10000>,
+ <0x0 0x2c20000 0x0 0x10000>;
+ interrupts = <0 103 IRQ_TYPE_LEVEL_LOW>,
+ <0 103 IRQ_TYPE_LEVEL_LOW>;
+ interrupt-names = "edma-tx", "edma-err";
+ dma-channels = <32>;
+ big-endian;
+ clock-names = "dmamux0", "dmamux1";
+ clocks = <&clockgen 4 3>,
+ <&clockgen 4 3>;
+ };
+
+ sata: sata at 3200000 {
+ compatible = "fsl,ls1012a-ahci";
+ reg = <0x0 0x3200000 0x0 0x10000>;
+ interrupts = <0 69 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&clockgen 4 0>;
+ };
+ };
+};
--
2.7.4
^ permalink raw reply related
* [PATCH] arm64: acpi: arch_apei_get_mem_attributes() should use memblock
From: James Morse @ 2016-11-09 18:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKv+Gu9+T0TYqk+uTUWa5K0E49WXDBpUK4Px_q=52jR0ON1zGA@mail.gmail.com>
Hi Ard,
On 09/11/16 12:12, Ard Biesheuvel wrote:
> On 8 November 2016 at 10:27, James Morse <james.morse@arm.com> wrote:
>> arm64's arch_apei_get_mem_attributes() tries to use
>> efi_mem_attributes() to read the memory attributes from the efi
>> memory map.
>>
>> drivers/firmware/efi/arm-init.c:efi_init() calls efi_memmap_unmap(),
>> which clears the EFI_MEMMAP bit from efi.flags once efi_init() has
>> finished with the memory map. This causes efi_mem_attributes() to
>> return 0 meaning PROT_DEVICE_nGnRnE is the chosen memory type for
>> all ACPI APEI mappings.
>>
>> APEI may call this from NMI context, so we can't re-map the EFI
>> memory map as this may need to allocate virtual address space.
>> 'ghes_ioremap_area' is APEIs cache of virtual address space to
>> access the buffer once we tell it the memory attributes.
>>
>> Do as acpi_os_ioremap() does, and consult memblock to learn if
>> the provided address is memory, or not.
>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
>> @@ -241,22 +238,13 @@ void __init acpi_boot_table_init(void)
>> pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
>> {
>> /*
>> - * According to "Table 8 Map: EFI memory types to AArch64 memory
>> - * types" of UEFI 2.5 section 2.3.6.1, each EFI memory type is
>> - * mapped to a corresponding MAIR attribute encoding.
>> - * The EFI memory attribute advises all possible capabilities
>> - * of a memory region. We use the most efficient capability.
>> + * The EFI memmap isn't mapped, instead read the version exported
>> + * into memblock. EFI's reserve_regions() call adds memory with the
>> + * WB attribute to memblock via early_init_dt_add_memory_arch().
>> */
>> + if (!memblock_is_memory(addr))
>> + return __pgprot(PROT_DEVICE_nGnRnE);
>>
>> - u64 attr;
>> -
>> - attr = efi_mem_attributes(addr);
>> - if (attr & EFI_MEMORY_WB)
>> - return PAGE_KERNEL;
>> - if (attr & EFI_MEMORY_WT)
>> - return __pgprot(PROT_NORMAL_WT);
>> - if (attr & EFI_MEMORY_WC)
>> - return __pgprot(PROT_NORMAL_NC);
>> - return __pgprot(PROT_DEVICE_nGnRnE);
>> + return PAGE_KERNEL;
>
> As you know, we recently applied [0] to ensure that memory regions
> that are marked by UEFI was write-through cacheable (WT) or
> write-combining (WC) are not misidentified by the kernel as having
> strongly ordered semantics.
Ah, I'd forgotten all about that...
> This means that in this case, you may be returning PAGE_KERNEL for
> regions that are lacking the EFI_MEMORY_WB attribute, which kind of
> defeats the purpose of this function (AIUI), to align the kernel's
> view of the region's attributes with the view of the firmware. I would
> expect that, for use cases like this one, the burden is on the
> firmware to ensure that only a single EFI_MEMORY_xx attribute is set
> if any kind of coherency between UEFI and the kernel is expected. If
> that single attribute is WT or WC, PAGE_KERNEL is most certainly
> wrong.
I've been trying to test some of the APEI code using some hacks on top of
kvmtool. (actually, quite a lot of hacks).
When I trigger an event, I see efi_mem_attributes() always return 0 because the
EFI memory map isn't mapped. This turns out to be because I have 'efi=noruntime'
on the command line. I stopped digging when I found this previously-dead code,
but should have gone further.
If this is an expected interaction, we can ignore this as a bug in my test
setup. (and I have to work out how to get efi runtime services going...)
If not, I can try always mapping the EFI memory map in
arm_enable_runtime_services() if we booted via ACPI, as in this case runtime
services isn't the only user of the memory map.
My intention here was to just mirror acpi_os_ioremap(), which will call
ioremap_cache() for WB/WC/WT regions, which (also) ends up using PROT_NORMAL. We
may get away with this, but you're right, we are less likely to here.
Thanks,
James
^ permalink raw reply
* KASAN & the vmalloc area
From: Dmitry Vyukov @ 2016-11-09 18:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161109105624.GA17020@leverpostej>
On Wed, Nov 9, 2016 at 2:56 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Nov 08, 2016 at 02:09:27PM -0800, Dmitry Vyukov wrote:
>> On Tue, Nov 8, 2016 at 11:03 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > When KASAN is selected, we allocate shadow for the whole vmalloc area,
>> > using common zero pte, pmd, pud tables. Walking over these in the ptdump
>> > code takes a *very* long time (I've seen up to 15 minutes with
>> > KASAN_OUTLINE enabled). For DEBUG_WX [3], this means boot hangs for that
>> > long, too.
>
> [...]
>
>> I've seen the same iteration slowness problem on x86 with
>> CONFIG_DEBUG_RODATA which walks all pages. The is about 1 minute, but
>> it is enough to trigger rcu stall warning.
>
> Interesting; do you know where that happens? I can't spot any obvious
> case where we'd have to walk all the page tables for DEBUG_RODATA.
As far as I remember it was this path:
mark_readonly in main.c -> mark_rodata_ro -> debug_checkwx ->
ptdump_walk_pgd_level_checkwx -> ptdump_walk_pgd_level_core.
>> The zero pud and vmalloc-ed stacks looks like different problems.
>> To overcome the slowness we could map zero shadow for vmalloc area lazily.
>> However for vmalloc-ed stacks we need to map actual memory, because
>> stack instrumentation will read/write into the shadow.
>
> Sure. The point I was trying to make is that there' be fewer page tables
> to walk (unless the vmalloc area was exhausted), assuming we also lazily
> mapped the common zero shadow for the vmalloc area.
>
>> One downside here is that vmalloc shadow can be as large as 1:1 (if we
>> allocate 1 page in vmalloc area we need to allocate 1 page for
>> shadow).
>
> I thought per prior discussion we'd only need to allocate new pages for
> the stacks in the vmalloc region, and we could re-use the zero pages?
We can't reuse zero ro pages for stacks, because stack instrumentation
writes to stack shadow.
When we have a large continuous range of memory, shadow for it is
1/8th. However, if we have a separate page, we will need to map whole
page of shadow for it, i.e. 1:1 shadow overhead.
> ... or are you trying to quantify the cost of the page tables?
>
>> Re slowness: could we just skip the KASAN zero puds (the top level)
>> while walking? Can they be interesting for anybody?
>
> They're interesting for the ptdump case (which allows privileged users
> to dump the tables via /sys/kernel/debug/kernel_page_tables). I've seen
> 25+ minute hangs there.
>
>> We can just pretend that they are not there. Looks like a trivial
>> solution for the problem at hand.
>
> For the boot time hang it's option. Though I'd prefer that the sanity
> checks applied to all of tables, shadow regions included.
>
> Thanks,
> Mark.
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe at googlegroups.com.
> To post to this group, send email to kasan-dev at googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20161109105624.GA17020%40leverpostej.
> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* [PATCH v7] soc: qcom: add l2 cache perf events driver
From: Will Deacon @ 2016-11-09 18:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161109175413.GE17020@leverpostej>
On Wed, Nov 09, 2016 at 05:54:13PM +0000, Mark Rutland wrote:
> On Fri, Oct 28, 2016 at 04:50:13PM -0400, Neil Leeder wrote:
> > + struct perf_event *events[MAX_L2_CTRS];
> > + struct l2cache_pmu *l2cache_pmu;
> > + DECLARE_BITMAP(used_counters, MAX_L2_CTRS);
> > + DECLARE_BITMAP(used_groups, L2_EVT_GROUP_MAX + 1);
> > + int group_to_counter[L2_EVT_GROUP_MAX + 1];
> > + int irq;
> > + /* The CPU that is used for collecting events on this cluster */
> > + int on_cpu;
> > + /* All the CPUs associated with this cluster */
> > + cpumask_t cluster_cpus;
>
> I'm still uncertain about aggregating all cluster PMUs into a larger
> PMU, given the cluster PMUs are logically independent (at least in terms
> of the programming model).
>
> However, from what I understand the x86 uncore PMU drivers aggregate
> symmetric instances of uncore PMUs (and also aggregate across packages
> to the same logical PMU).
>
> Whatever we do, it would be nice for the uncore drivers to align on a
> common behaviour (and I think we're currently going the oppposite route
> with Cavium's uncore PMU). Will, thoughts?
I'm not a big fan of aggregating this stuff. Ultimately, the user in the
driving seat of perf is going to need some knowledge about the toplogy of
the system in order to perform sensible profiling using an uncore PMU.
If the kernel tries to present a single, unified PMU then we paint ourselves
into a corner when the hardware isn't as symmetric as we want it to be
(big/little on the CPU side is the extreme example of this). If we want
to be consistent, then exposing each uncore unit as a separate PMU is
the way to go. That doesn't mean we can't aggregate the components of a
distributed PMU (e.g. the CCN or the SMMU), but we don't want to aggregate
at the programming interface/IP block level.
We could consider exposing some topology information in sysfs if that's
seen as an issue with the non-aggregated case.
Will
^ permalink raw reply
* KASAN & the vmalloc area
From: Dmitry Vyukov @ 2016-11-09 18:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <8abab822-4d93-a336-b00f-7aa9b1f47f8c@virtuozzo.com>
On Wed, Nov 9, 2016 at 8:53 AM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> On 11/08/2016 10:03 PM, Mark Rutland wrote:
>> Hi,
>>
>> I see a while back [1] there was a discussion of what to do about KASAN
>> and vmapped stacks, but it doesn't look like that was solved, judging by
>> the vmapped stacks pull [2] for v4.9.
>>
>> I wondered whether anyone had looked at that since?
>>
>
> Sadly, but I didn't have much time for this yet, so it's in an initial state.
>
>> I have an additional reason to want to dynamically allocate the vmalloc
>> area shadow: it turns out that KASAN currently interacts rather poorly
>> with the arm64 ptdump code.
>>
>> When KASAN is selected, we allocate shadow for the whole vmalloc area,
>> using common zero pte, pmd, pud tables. Walking over these in the ptdump
>> code takes a *very* long time (I've seen up to 15 minutes with
>> KASAN_OUTLINE enabled). For DEBUG_WX [3], this means boot hangs for that
>> long, too.
>>
>
> I didn't look at any code, but we probably could can remember last visited pgd
> and skip next pgd if it's the same as previous.
Good idea.
> Alternatively - just skip kasan_zero_p*d in ptdump walker.
Mark have concern with the fact that we hide the mapping from the
walker this way. But your above idea with deduping pgd's during walk
both fast and doesn't hide anything from walker.
>> If I don't allocate vmalloc shadow (and remove the apparently pointlesss
>> shadow of the shadow area), and only allocate shadow for the image,
>> fixmap, vmemmap and so on, that delay gets cut to a few seconds, which
>> is tolerable for a debug configuration...
>>
>> ... however, things blow up when the kernel touches vmalloc'd memory for
>> the first time, as we don't install shadow for that dynamically.
>>
>> Thanks,
>> Mark.
>>
>> [1] https://lkml.kernel.org/r/CALCETrWucrYp+yq8RHSDqf93xtg793duByirurzJbLRhrz=tcA at mail.gmail.com
>> [2] https://lkml.kernel.org/r/20161003092940.GA691 at gmail.com
>> [3] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/464191.html
>>
^ permalink raw reply
* [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Rob Herring @ 2016-11-09 18:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <ee296deafdcbeb431a592b591ae38a758ba4cce7.1477911954.git-series.gregory.clement@free-electrons.com>
On Mon, Oct 31, 2016 at 12:09:54PM +0100, Gregory CLEMENT wrote:
> From: Ziji Hu <huziji@marvell.com>
>
> Marvell Xenon SDHC can support eMMC/SD/SDIO.
> Add Xenon-specific properties.
> Also add properties for Xenon PHY setting.
>
> Signed-off-by: Hu Ziji <huziji@marvell.com>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
> Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt | 161 +++++++-
> MAINTAINERS | 1 +-
> 2 files changed, 162 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
>
> diff --git a/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt b/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
> new file mode 100644
> index 000000000000..0d2d139494d3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
> @@ -0,0 +1,161 @@
> +Marvell's Xenon SDHCI Controller device tree bindings
> +This file documents differences between the core mmc properties
> +described by mmc.txt and the properties used by the Xenon implementation.
> +
> +A single Xenon IP can support multiple slots.
> +Each slot acts as an independent SDHC. It owns independent resources, such
> +as register sets clock and PHY.
> +Each slot should have an independent device tree node.
> +
> +Required Properties:
> +- compatible: should be one of the following
> + - "marvell,armada-3700-sdhci": For controllers on Armada-3700 SOC.
> + Must provide a second register area and marvell,pad-type.
> + - "marvell,xenon-sdhci": For controllers on all the SOCs, other than
> + Armada-3700.
Need SoC specific compatible strings.
> +
> +- clocks:
> + Array of clocks required for SDHCI.
> + Requires at least one for Xenon IP core.
> + Some SOCs require additional clock for AXI bus.
> +
> +- clock-names:
> + Array of names corresponding to clocks property.
> + The input clock for Xenon IP core should be named as "core".
> + The optional AXI clock should be named as "axi".
When is AXI clock optional? This should be required for ?? compatible
strings.
> +
> +- reg:
> + * For "marvell,xenon-sdhci", one register area for Xenon IP.
> +
> + * For "marvell,armada-3700-sdhci", two register areas.
> + The first one for Xenon IP register. The second one for the Armada 3700 SOC
> + PHY PAD Voltage Control register.
> + Please follow the examples with compatible "marvell,armada-3700-sdhci"
> + in below.
> + Please also check property marvell,pad-type in below.
> +
> +Optional Properties:
> +- marvell,xenon-slotno:
Multiple slots should be represented as child nodes IMO. I think some
other bindings already do this.
> + Indicate the corresponding bit index of current Xenon SDHC slot in
> + SDHC System Operation Control Register Bit[7:0].
> + Set/clear the corresponding bit to enable/disable current Xenon SDHC
> + slot.
> + If this property is not provided, Xenon IP should contain only one
> + slot.
> +
> +- marvell,xenon-phy-type:
> + Xenon support mutilple types of PHYs.
> + To select eMMC 5.1 PHY, set:
> + marvell,xenon-phy-type = "emmc 5.1 phy"
> + eMMC 5.1 PHY is the default choice if this property is not provided.
> + To select eMMC 5.0 PHY, set:
> + marvell,xenon-phy-type = "emmc 5.0 phy"
> + To select SDH PHY, set:
> + marvell,xenon-phy-type = "sdh phy"
> + Please note that eMMC PHY is a general PHY for eMMC/SD/SDIO, other than for
> + eMMC only.
Does this vary per instance on a single SoC? If not, then an SoC
specific compatible should determine this.
Also, the " phy" part is redundant.
> +
> +- marvell,xenon-phy-znr:
> + Set PHY ZNR value.
> + Only available for eMMC PHY 5.1 and eMMC PHY 5.0.
> + valid range = [0:0x1F].
> + ZNR is set as 0xF by default if this property is not provided.
> +
> +- marvell,xenon-phy-zpr:
> + Set PHY ZPR value.
> + Only available for eMMC PHY 5.1 and eMMC PHY 5.0.
> + valid range = [0:0x1F].
> + ZPR is set as 0xF by default if this property is not provided.
> +
> +- marvell,xenon-phy-nr-success-tun:
> + Set the number of required consecutive successful sampling points used to
> + identify a valid sampling window, in tuning process.
> + Valid range = [1:7]. Set as 0x4 by default if this property is not provided.
> +
> +- marvell,xenon-phy-tun-step-divider:
> + Set the divider for calculating TUN_STEP.
> + Set as 64 by default if this property is not provided.
> +
> +- marvell,xenon-phy-slow-mode:
> + Force PHY into slow mode.
> + Only available when bus frequency lower than 50MHz in SDR mde.
> + Disabled by default. Please do not enable it unless it is necessary.
> +
> +- marvell,xenon-mask-conflict-err:
> + Mask Conflict Error alert on some SOC. Disabled by default.
> +
> +- marvell,xenon-tun-count:
> + Xenon SDHC SOC usually doesn't provide re-tuning counter in
> + Capabilities Register 3 Bit[11:8].
> + This property provides the re-tuning counter.
> + If this property is not set, default re-tuning counter will
> + be set as 0x9 in driver.
> +
> +- marvell,pad-type:
> + Type of Armada 3700 SOC PHY PAD Voltiage Controller register.
> + Only valid when "marvell,armada-3700-sdhci" is selected.
> + Two types: "sd" and "fixed-1-8v".
> + If "sd" is slected, SOC PHY PAD is set as 3.3V at the beginning and is
> + switched to 1.8V when SD in UHS-I.
> + If "fixed-1-8v" is slected, SOC PHY PAD is fixed 1.8V, such as for eMMC.
> + Please follow the examples with compatible "marvell,armada-3700-sdhci"
> + in below.
> +
> +Example:
> +- For eMMC slot:
> +
> + sdhci at aa0000 {
> + compatible = "marvell,xenon-sdhci";
> + reg = <0xaa0000 0x1000>;
> + interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>
> + clocks = <&emmc_clk>, <&axi_clock>;
> + clock-names = "core", "axi";
> + bus-width = <8>;
> + marvell,xenon-emmc;
Not documented. If we need to specify the type of slot/card, then we
need to come up with a standard property. This was either already done
or attempted IIRC.
> + marvell,xenon-slotno = <0>;
> + marvell,xenon-phy-type = "emmc 5.1 phy";
> + marvell,xenon-tun-count = <11>;
> + };
> +
> +- For SD/SDIO slot:
> +
> + sdhci at ab0000 {
> + compatible = "marvell,xenon-sdhci";
> + reg = <0xab0000 0x1000>;
> + interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>
> + vqmmc-supply = <&sd_regulator>;
> + clocks = <&sdclk>;
> + clock-names = "core";
> + bus-width = <4>;
> + marvell,xenon-tun-count = <9>;
> + };
> +
> +- For eMMC slot with compatible "marvell,armada-3700-sdhci":
> +
> + sdhci at aa0000 {
> + compatible = "marvell,armada-3700-sdhci";
> + reg = <0xaa0000 0x1000>,
> + <phy_addr 0x4>;
> + interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>
> + clocks = <&emmcclk>;
> + clock-names = "core";
> + bus-width = <8>;
> + marvell,xenon-emmc;
> +
> + marvell,pad-type = "fixed-1-8v";
> + };
> +
> +- For SD/SDIO slot with compatible "marvell,armada-3700-sdhci":
> +
> + sdhci at ab0000 {
> + compatible = "marvell,armada-3700-sdhci";
> + reg = <0xab0000 0x1000>,
> + <phy_addr 0x4>;
> + interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>
> + vqmmc-supply = <&sd_regulator>;
> + clocks = <&sdclk>;
> + clock-names = "core";
> + bus-width = <4>;
> +
> + marvell,pad-type = "sd";
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1a5c4c30ea24..850a0afb0c8d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7608,6 +7608,7 @@ MARVELL XENON MMC/SD/SDIO HOST CONTROLLER DRIVER
> M: Ziji Hu <huziji@marvell.com>
> L: linux-mmc at vger.kernel.org
> S: Supported
> +F: Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
>
> MATROX FRAMEBUFFER DRIVER
> L: linux-fbdev at vger.kernel.org
> --
> git-series 0.8.10
^ permalink raw reply
* [PATCH v2 5/7] ARM: shmobile: Document DT bindings for CCCR and PRR
From: Rob Herring @ 2016-11-09 18:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477913455-5314-6-git-send-email-geert+renesas@glider.be>
On Mon, Oct 31, 2016 at 12:30:53PM +0100, Geert Uytterhoeven wrote:
> Add device tree binding documentation for the Common Chip Code Register
> and Product Register, which provide SoC product and revision
> information.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
> - New.
> ---
> Documentation/devicetree/bindings/arm/shmobile.txt | 26 ++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox