|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing.
Thanks for the patch! I would like to see it in Xen 4.9.
On Fri, 16 Sep 2016, Konrad Rzeszutek Wilk wrote:
> On Mon, Sep 05, 2016 at 06:13:59AM -0400, Kyle Temkin wrote:
> > From: "Kyle J. Temkin" <temkink@xxxxxxxxxxxx>
> >
> > Tegra devices have a legacy interrupt controller (lic, or ictlr) that
> > must be programmed in parallel with their primary GIC. For all intents
> > and purposes, we treat this devices attached to this controller as
^ these
> > connected to the primary GIC, as it will be handling their interrupts.
>
> Is there a link to the specification? Could that be included in the file
> or in the commit description?
>
> >
> > This commit adds support for exposing the ictlr to the hardware domain;
> > but a future commit will extend this to support exposing a virtualized
> > version of the ictlr to the hardware domain, and to ensure that
> > interrupts are unmasked properly when routed to a Xen, or to a domain
> > other than the hardware domain.
> >
> > Signed-off-by: Kyle Temkin <temkink@xxxxxxxxxxxx>
> > ---
> > xen/arch/arm/platforms/Makefile | 2 +
> > xen/arch/arm/platforms/tegra.c | 339
> > ++++++++++++++++++++++++++++++++++
> > xen/include/asm-arm/platforms/tegra.h | 50 +++++
> > 3 files changed, 391 insertions(+)
> > create mode 100644 xen/arch/arm/platforms/tegra.c
> > create mode 100644 xen/include/asm-arm/platforms/tegra.h
> >
> > diff --git a/xen/arch/arm/platforms/Makefile
> > b/xen/arch/arm/platforms/Makefile
> > index 49fa683..0c3a7f4 100644
> > --- a/xen/arch/arm/platforms/Makefile
> > +++ b/xen/arch/arm/platforms/Makefile
> > @@ -4,7 +4,9 @@ obj-$(CONFIG_ARM_32) += exynos5.o
> > obj-$(CONFIG_ARM_32) += midway.o
> > obj-$(CONFIG_ARM_32) += omap5.o
> > obj-$(CONFIG_ARM_32) += rcar2.o
> > +obj-$(CONFIG_ARM_32) += tegra.o
> > obj-$(CONFIG_ARM_64) += seattle.o
> > obj-$(CONFIG_ARM_32) += sunxi.o
> > obj-$(CONFIG_ARM_64) += xgene-storm.o
> > obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
> > +obj-$(CONFIG_ARM_64) += tegra.o
Are there both 32bit and 64bit versions of tegra?
> > diff --git a/xen/arch/arm/platforms/tegra.c b/xen/arch/arm/platforms/tegra.c
> > new file mode 100644
> > index 0000000..e75242c
> > --- /dev/null
> > +++ b/xen/arch/arm/platforms/tegra.c
> > @@ -0,0 +1,339 @@
> > +/*
> > + * NVIDIA Tegra specific settings
> > + *
> > + * Ian Campbell; Copyright (c) 2014 Citrix Systems
> > + * Kyle Temkin; Copyright (c) 2016 Assured Information Security, Inc.
> > + * Chris Patterson; Copyright (c) 2016 Assured Information Security, Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <xen/config.h>
>
> No config.h pls.
> > +#include <xen/lib.h>
> > +#include <xen/stdbool.h>
> > +#include <xen/sched.h>
> > +#include <xen/vmap.h>
> > +
> > +#include <asm/io.h>
> > +#include <asm/gic.h>
> > +#include <asm/platform.h>
> > +#include <asm/platforms/tegra.h>
> > +
> > +
> > +/* Permanent mapping to the Tegra legacy interrupt controller. */
> > +static void __iomem *tegra_ictlr_base = NULL;
>
> No need for NULL.
> > +
> > +/*
> > + * List of legacy interrupt controllers that can be used to route
> > + * Tegra interrupts.
> > + */
> > +static const char * const tegra_interrupt_compat[] __initconst =
> > +{
> > + "nvidia,tegra120-ictlr", /* Tegra K1 controllers */
> > + "nvidia,tegra210-ictlr" /* Tegra X1 controllers */
> > +};
> > +
> > +
> > +/**
> > + * Returns true if the given IRQ belongs to a supported tegra interrupt
> > + * controller.
> > + *
> > + * @param rirq The raw IRQ to be identified.
> > + * @return True iff the given IRQ belongs to a Tegra ictlr.
> > + */
> > +static bool_t tegra_irq_belongs_to_ictlr(struct dt_raw_irq * rirq) {
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(tegra_interrupt_compat); i++)
>
> Something is off with your style ({, and the lack of spaces around ()).
>
> > + {
> > + if ( dt_device_is_compatible(rirq->controller,
> > tegra_interrupt_compat[i]) )
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > +
> > +/**
> > + * Returns true iff the given IRQ is routable -- that is, if it is
> > descended
> > + * from the platform's primary GIC.
> > + *
> > + * @param rirq The raw IRQ in question.
> > + * @return True iff the given IRQ routes to a platform GIC.
> > + */
> > +static bool_t tegra_irq_is_routable(struct dt_raw_irq * rirq)
>
> And as I found out just recently, use 'bool' instead of bool_t.
>
> > +{
> > + /* If the IRQ connects directly to our GIC, it's trivially routable. */
> > + if ( rirq->controller == dt_interrupt_controller )
> > + return true;
> > +
> > + /*
> > + * If the IRQ belongs to a legacy interrupt controller, then it's
> > + * effectively owned by the GIC, and is routable.
> > + */
> > + if ( tegra_irq_belongs_to_ictlr(rirq) )
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > +/**
> > + * Returns the IRQ number for a given device. Tegra IRQs transalate using
> > the
> > + * same algorithm as normal GIC IRQs, but aren't parented by the system
> > GIC.
> > + *
> > + * As a result, translation fails an assertion in the normal translation
> > path.
> > + * The normal version is essentially dt_irq_xlate wrapped with an assert,
> > so
> > + * we'll just call dt_irq_xlate directly.
> > + *
> > + * @param device The DT node describing the device.
> > + * @param index The index of the interrupt within the device node.
> > + * @return The translated number of the IRQ, or a negative error code.
> > + */
> > +static int tegra_irq_for_device(const struct dt_device_node *device, int
> > index)
> > +{
> > + struct dt_raw_irq raw;
> > + struct dt_irq dt_irq;
> > + int res;
> > +
> > + res = dt_device_get_raw_irq(device, index, &raw);
> > + if ( res )
> > + return -ENODEV;
> > +
> > + /*
> > + * The translation function for the Tegra ictlr happens to match the
> > + * translation function for the normal GIC, so we'll use that in either
> > + * case.
> > + */
> > + res = dt_irq_xlate(raw.specifier, raw.size, &dt_irq.irq, &dt_irq.type);
> > + if ( res )
> > + return -ENODEV;
> > +
> > + if ( irq_set_type(dt_irq.irq, dt_irq.type) )
> > + return -ENODEV;
> > +
> > + return dt_irq.irq;
> > +}
Instead of duplicating this code, I am tempted to suggest to modify
dt_irq_translate to make it work for tegra too. You just need to change
the raw->controller != dt_interrupt_controller check to also check for
tegra right?
> > +/**
> > + * Platform-specific reset code for the Tegra devices.
> > + * Should not return.
> > + */
> > +static void tegra_reset(void)
> > +{
> > + void __iomem *addr;
> > + u32 val;
> > +
> > + addr = ioremap_nocache(TEGRA_RESET_BASE, TEGRA_RESET_SIZE);
> > + if ( !addr )
> > + {
> > + printk(XENLOG_ERR "Tegra: Unable to map tegra reset address. Reset
> > failed!\n");
> > + return;
> > + }
> > +
> > + /* Write into the reset device. */
> > + val = readl(addr) | TEGRA_RESET_MASK;
> > + writel(val, addr);
>
> No need to re-read the register?
>
> > +
> > + iounmap(addr);
> > +}
> > +
> > +/**
> > + * Applies an interrupt enable to a given interrupt via the legacy
> > + * interrupt controller, and marks that interrupt as a normal interrupt,
> > + * rather than a fast IRQ.
> > + *
> > + * @param irq The hardware IRQ number for the given interrupt.
> > + */
> > +static void tegra_ictlr_set_interrupt_enable(unsigned int irq, bool
> > enabled)
> > +{
> > + uint32_t previous_iep_class;
> > +
> > + /* If we're enabling a given bit, use the SET register; otherwise CLR.
> > */
> > + unsigned int register_number =
> > + enabled ? TEGRA_ICTLR_CPU_IER_SET : TEGRA_ICTLR_CPU_IER_CLR;
> > +
> > + /*
> > + * Determine the IRQ number in the ictlr domain, and figure out the
> > indexA
> > + * of the individual controller we're working with. */
>
> Put the */ on its own line please.
>
> > + unsigned int ictlr_irq = irq - NR_LOCAL_IRQS;
> > + unsigned int ictlr_number = ictlr_irq / TEGRA_IRQS_PER_ICTLR;
> > +
> > + /* Get a pointer to the target ictlr. */
> > + void __iomem * target_ictlr = tegra_ictlr_base + TEGRA_ICTLR_SIZE *
> > ictlr_number;
> > +
> > + /* Determine the mask we'll be working with. */
> > + uint32_t mask = BIT(ictlr_irq % TEGRA_IRQS_PER_ICTLR);
> > +
> > + /* Sanity check our memory access. */
> > + ASSERT(tegra_ictlr_base);
> > + ASSERT(ictlr_number < TEGRA_ICTLR_COUNT);
> > + ASSERT(irq >= NR_LOCAL_IRQS);
> > +
> > + /* Enable the given IRQ. */
> > + writel(mask, target_ictlr + register_number);
> > +
> > + /* Mark the interrupt as a normal interrupt-- not a fast IRQ. */
> > + previous_iep_class = readl(target_ictlr + TEGRA_ICTLR_CPU_IEP_CLASS);
> > + writel(previous_iep_class & ~mask, target_ictlr +
> > TEGRA_ICTLR_CPU_IEP_CLASS);
> > +}
I think it makes sense to split this function into two: one that set the
interrupt as normal, and the other to enable/disable it.
> > +
> > +/**
> > + * Routes an IRQ to a guest, applying sane values to the ictlr masks.
> > + *
> > + * @param domain The domain to which the IRQ will be routed.
> > + * @param virq The virtual IRQ number.
> > + * @param desc The IRQ to be routed.
> > + * @param priority The IRQ priority.
> > + * @return 0 on success, or an error code on failure.
> > + */
> > +static int tegra_route_irq_to_guest(struct domain *d, unsigned int virq,
> > + struct irq_desc *desc, unsigned int
> > priority)
> > +{
> > + /* Program the core GIC to deliver the interrupt to the guest. */
> > + int rc = gic_route_irq_to_guest(d, virq, desc, priority);
> > +
> > + /* If we couldn't route the IRQ via the GIC, bail out. */
> > + if(rc)
>
> Uh oh.
Style. No need to add another warning message here, given that this
error doesn't seem to be tegra specific.
> > + {
> > + printk(XENLOG_ERR "Tegra LIC: Couldn't program GIC to route vIRQ
> > %d (%d).\n",
> > + desc->irq, rc);
> > + return rc;
> > + }
> > +
> > + /*
> > + * If this is a local IRQ, it's not masked by the ictlr, so we
> > + * don't need to perform any ictlr manipulation.
> > + */
> > + if( desc->irq < NR_LOCAL_IRQS )
> > + return rc;
> > +
> > + /*
> > + * If this is the hardware domain, it will have real access to the
> > ictlr,
> > + * and will program the ictlr itself, so it should start with the ictlr
> > + * disabled. If we're not the hwdom, the domain won't interact with the
> > + * ictlr, and the interrupt shouldn't be masked.
> > + */
> > + tegra_ictlr_set_interrupt_enable(desc->irq, !is_hardware_domain(d));
> > + return rc;
> > +}
> > +
> > +
> > +/**
> > + * Routes an IRQ to Xen. This method both performs the core IRQ routing,
> > and
> > + * sets up any ictlr routing necessary.
> > + *
> > + * @param desc The IRQ to be routed.
> > + * @param priority The IRQ priority.
> > + */
> > +static void tegra_route_irq_to_xen(struct irq_desc *desc, unsigned int
> > priority)
> > +{
> > + unsigned int irq = desc->irq;
> > +
> > + /* Program the core GIC to deliver the interrupt to Xen. */
> > + gic_route_irq_to_xen(desc, priority);
> > +
> > + /*
> > + * If this is a local IRQ, it's not masked by the ictlr, so we
> > + * don't need to perform any ictlr manipulation.
> > + */
> > + if( irq < NR_LOCAL_IRQS )
> > + return;
> > +
> > + /*
> > + * Enable the interrupt in the ictlr. Xen only uses the GIC to
> > + * perform masking, so we'll enable the interrupt to prevent ictlr
> > + * gating of the interrupt.
> > + */
> > + tegra_ictlr_set_interrupt_enable(irq, true);
> > +
> > +}
> > +
> > +/**
> > + * Initialize the Tegra legacy interrupt controller, placing each interrupt
> > + * into a default state. These defaults ensure that stray interrupts don't
> > + * affect Xen.
> > + */
> > +static int tegra_initialize_legacy_interrupt_controller(void)
> > +{
> > + int i;
> > +
> > + /* Map in the tegra ictlr. */
> > + tegra_ictlr_base = ioremap_nocache(TEGRA_ICTLR_BASE,
> > + TEGRA_ICTLR_SIZE * TEGRA_ICTLR_COUNT);
>
> Odd, style?
>
> > +
> > + if ( !tegra_ictlr_base )
> > + panic("Failed to map in the Tegra legacy interrupt controller!\n");
> > +
> > + /* Initialize each of the legacy interrupt controllers. */
> > + for (i = 0; i < TEGRA_ICTLR_COUNT; i++)
>
> Style.
> > + {
> > + void __iomem *ictlr_n = tegra_ictlr_base + TEGRA_ICTLR_SIZE * i;
> > +
> > + /* Clear the interrupt enables for every interrupt. */
> > + writel(~0, ictlr_n + TEGRA_ICTLR_CPU_IER_CLR);
> > +
> > + /*
> > + * Mark all of our interrupts as normal ARM interrupts (as opposed
> > + * to Fast Interrupts.)
> > + */
> > + writel(0, ictlr_n + TEGRA_ICTLR_CPU_IEP_CLASS);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
style
> > + * Startup code for the Tegra.
> > + */
> > +static int tegra_init(void)
>
> __init ?
> > +{
> > + int rc;
> > +
> > + rc = tegra_initialize_legacy_interrupt_controller();
>
> How about you jsut make this:
>
> return tegra_initializa...()
> > +
> > + /*
> > + * TODO: Virtualize the MMIO regions for the ictlr, ensuring the hwdom
> > + * (and possibly passhtrough domains) can only access ictlr registers
> > for
> > + * interrupts they actually own.
> > + */
> > +
> > + return rc;
> > +}
> > +
> > +
> > +static const char * const tegra_dt_compat[] __initconst =
> > +{
> > + "nvidia,tegra120", /* Tegra K1 */
> > + "nvidia,tegra210", /* Tegra X1 */
I can find "nvidia,tegra210" under arch/arm64/boot/dts/nvidia, but I
cannot find "nvidia,tegra120" anywhere. Do you have a pointer? If it
hasn't been upstreamed in Linux, we need to provide a pointer to the
Linux tree and the device tree for K1.
> > + NULL
> > +};
> > +
> > +static const struct dt_device_match tegra_blacklist_dev[] __initconst =
> > +{
> > + /*
> > + * The UARTs share a page which runs the risk of mapping the Xen
> > console
> > + * UART to dom0, so don't map any of them.
> > + */
> > + DT_MATCH_COMPATIBLE("nvidia,tegra20-uart"),
> > + { /* sentinel */ },
> > +};
> > +
> > +PLATFORM_START(tegra, "Tegra")
> > + .blacklist_dev = tegra_blacklist_dev,
> > + .compatible = tegra_dt_compat,
> > + .init = tegra_init,
> > + .reset = tegra_reset,
> > + .irq_is_routable = tegra_irq_is_routable,
> > + .irq_for_device = tegra_irq_for_device,
> > + .route_irq_to_xen = tegra_route_irq_to_xen,
> > + .route_irq_to_guest = tegra_route_irq_to_guest,
> > +PLATFORM_END
>
> You are missing this:
>
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
>
> > diff --git a/xen/include/asm-arm/platforms/tegra.h
> > b/xen/include/asm-arm/platforms/tegra.h
> > new file mode 100644
> > index 0000000..821add5
> > --- /dev/null
> > +++ b/xen/include/asm-arm/platforms/tegra.h
> > @@ -0,0 +1,50 @@
> > +/*
> > + * NVIDIA Tegra platform definitions
> > + *
> > + * Kyle Temkin; Copyright (c) 2016 Assured Information Security, Inc.
> > + * Chris Patterson; Copyright (c) 2016 Assured Information Security, Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +
> > +#ifndef __ASM_ARM_PLATFORMS_TEGRA_H
>
> Add please two underscores at the far end.
>
> > +#define __ASM_ARM_PLATFORMS_TEGRA_H
> > +
> > +#define TEGRA_ICTLR_BASE 0x60004000
> > +#define TEGRA_ICTLR_SIZE 0x00000100
> > +#define TEGRA_ICTLR_COUNT 6
> > +#define TEGRA_IRQS_PER_ICTLR 32
> > +
> > +#define TEGRA_RESET_BASE 0x7000e400
> > +#define TEGRA_RESET_SIZE 4
> > +#define TEGRA_RESET_MASK 0x10
Please keep all the TEGRA_ICTLR* defines together: move TEGRA_RESET*
below.
> > +#define TEGRA_ICTLR_CPU_IER 0x20
> > +#define TEGRA_ICTLR_CPU_IER_SET 0x24
> > +#define TEGRA_ICTLR_CPU_IER_CLR 0x28
> > +#define TEGRA_ICTLR_CPU_IEP_CLASS 0x2C
> > +
> > +#define TEGRA_ICTLR_COP_IER 0x30
> > +#define TEGRA_ICTLR_COP_IER_SET 0x34
> > +#define TEGRA_ICTLR_COP_IER_CLR 0x38
> > +#define TEGRA_ICTLR_COP_IEP_CLASS 0x3c
> > +
> > +
> > +#endif /* __ASM_ARM_PLATFORMS_TEGRA_H */
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |