Re: [PATCH v4 06/10] irqchip / GICv3: Add ACPI support for GICv3+ initialization

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 05/08/15 15:00, Hanjun Guo wrote:
> On 08/04/2015 09:17 PM, Marc Zyngier wrote:
>> On 29/07/15 11:08, Hanjun Guo wrote:
>>> From: Tomasz Nowicki <tomasz.nowicki@xxxxxxxxxx>
>>>
>>> With the refator of gic_of_init(), GICv3/4 can be initialized
>>> by gic_init_bases() with gic distributor base address and gic
>>> redistributor region(s).
>>>
>>> So get the redistributor region base addresses from MADT GIC
>>> redistributor subtable, and the distributor base address from
>>> GICD subtable to init GICv3 irqchip in ACPI way.
>>>
>>> Note: GIC redistributor base address may also be provided in
>>> GICC structures on systems supporting GICv3 and above if the GIC
>>> Redistributors are not in the always-on power domain, this
>>> patch didn't implement such feature yet.
>>>
>>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@xxxxxxxxxx>
>>> [hj: Rework this patch and fix multi issues]
>>> Signed-off-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
>>> ---
>>>   drivers/irqchip/irq-gic-v3.c | 174 +++++++++++++++++++++++++++++++++++++++++--
>>>   1 file changed, 169 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>> index c0b96c6..ebc5604 100644
>>> --- a/drivers/irqchip/irq-gic-v3.c
>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>> @@ -15,6 +15,7 @@
>>>    * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>    */
>>>
>>> +#include <linux/acpi.h>
>>>   #include <linux/cpu.h>
>>>   #include <linux/cpu_pm.h>
>>>   #include <linux/delay.h>
>>> @@ -25,6 +26,7 @@
>>>   #include <linux/percpu.h>
>>>   #include <linux/slab.h>
>>>
>>> +#include <linux/irqchip/arm-gic-acpi.h>
>>>   #include <linux/irqchip/arm-gic-v3.h>
>>>
>>>   #include <asm/cputype.h>
>>> @@ -802,7 +804,8 @@ static int __init gic_init_bases(void __iomem *dist_base,
>>>      set_handle_irq(gic_handle_irq);
>>>
>>>      if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
>>> -            its_init(domain_token, &gic_data.rdists, gic_data.domain);
>>> +            its_init(irq_domain_token_to_of_node(domain_token),
>>> +                     &gic_data.rdists, gic_data.domain);
>>
>> This doesn't make much sense. The first parameter to its_init is indeed
>> supposed to be an of_node, but what is the point of calling its_init if
>> you *know* you don't have the necessary topological information to parse
>> the firmware tables?
>>
>> I don't see *any* code that is going to parse the ITS table in this
>> series, so please don't call its_init passing a NULL pointer to it. This
>> is just gross.
> 
> OK, the ITS ACPI code is in later patch which combined with IORT. How
> about moving it to the GIC of init code temporary?

Just don't call its_init if irq_domain_token_to_of_node(domain_token) is
NULL. But don't call it with a NULL parameter.

>>
>>>
>>>      gic_smp_init();
>>>      gic_dist_init();
>>> @@ -818,6 +821,16 @@ out_free:
>>>      return err;
>>>   }
>>>
>>> +static int __init detect_distributor(void __iomem *dist_base)
>>
>> We do have a naming convention in this file. All functions start with gic_.
>>
>>> +{
>>> +    u32 reg = readl_relaxed(dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
>>> +
>>> +    if (reg != GIC_PIDR2_ARCH_GICv3 && reg != GIC_PIDR2_ARCH_GICv4)
>>> +            return -ENODEV;
>>> +
>>> +    return 0;
>>> +}
>>
>> This function doesn't detect anything, it validates that we have
>> something sensible. Please rename it to gic_validate_dist_version, or
>> something similar.
> 
> Ok.
> 
>>
>>> +
>>>   #ifdef CONFIG_OF
>>>   static int __init gic_of_init(struct device_node *node, struct device_node *parent)
>>>   {
>>> @@ -825,7 +838,6 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
>>>      struct redist_region *rdist_regs;
>>>      u64 redist_stride;
>>>      u32 nr_redist_regions;
>>> -    u32 reg;
>>>      int err, i;
>>>
>>>      dist_base = of_iomap(node, 0);
>>> @@ -835,11 +847,10 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
>>>              return -ENXIO;
>>>      }
>>>
>>> -    reg = readl_relaxed(dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
>>> -    if (reg != GIC_PIDR2_ARCH_GICv3 && reg != GIC_PIDR2_ARCH_GICv4) {
>>> +    err = detect_distributor(dist_base);
>>> +    if (err) {
>>>              pr_err("%s: no distributor detected, giving up\n",
>>>                      node->full_name);
>>> -            err = -ENODEV;
>>>              goto out_unmap_dist;
>>>      }
>>>
>>> @@ -887,3 +898,156 @@ out_unmap_dist:
>>>
>>>   IRQCHIP_DECLARE(gic_v3, "arm,gic-v3", gic_of_init);
>>>   #endif
>>> +
>>> +#ifdef CONFIG_ACPI
>>> +static struct redist_region *redist_regs __initdata;
>>> +static u32 nr_redist_regions __initdata;
>>> +static phys_addr_t dist_phys_base __initdata;
>>> +
>>> +static int __init
>>> +gic_acpi_register_redist(u64 phys_base, u64 size)
>>
>> A physical address must use phys_addr_t.
> 
> OK.
> 
>>
>>> +{
>>> +    struct redist_region *redist_regs_new;
>>> +    void __iomem *redist_base;
>>> +
>>> +    redist_regs_new = krealloc(redist_regs,
>>> +                               sizeof(*redist_regs) * (nr_redist_regions + 1),
>>> +                               GFP_KERNEL);
>>
>> NAK. If you have multiple regions, you count them, you allocate the
>> right number of regions. This will be far more efficient than doing this
>> realloc dance. It is not like we're not parsing the tables a zillion
>> times already. Doing it one more time won't hurt.
> 
> Agreed, will update in next version.
> 
>>
>>> +    if (!redist_regs_new) {
>>> +            pr_err("Couldn't allocate resource for GICR region\n");
>>> +            return -ENOMEM;
>>> +    }
>>> +
>>> +    redist_regs = redist_regs_new;
>>> +
>>> +    redist_base = ioremap(phys_base, size);
>>> +    if (!redist_base) {
>>> +            pr_err("Couldn't map GICR region @%llx\n", phys_base);
>>> +            return -ENOMEM;
>>> +    }
>>> +
>>> +    redist_regs[nr_redist_regions].phys_base = phys_base;
>>> +    redist_regs[nr_redist_regions].redist_base = redist_base;
>>> +    nr_redist_regions++;
>>> +    return 0;
>>> +}
>>> +
>>> +static int __init
>>> +gic_acpi_parse_madt_redist(struct acpi_subtable_header *header,
>>> +                    const unsigned long end)
>>> +{
>>> +    struct acpi_madt_generic_redistributor *redist;
>>> +
>>> +    if (BAD_MADT_ENTRY(header, end))
>>> +            return -EINVAL;
>>> +
>>> +    redist = (struct acpi_madt_generic_redistributor *)header;
>>> +    if (!redist->base_address)
>>> +            return -EINVAL;
>>> +
>>> +    return gic_acpi_register_redist(redist->base_address, redist->length);
>>> +}
>>> +
>>> +static int __init
>>> +gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header,
>>> +                            const unsigned long end)
>>> +{
>>
>> How many versions of gic_acpi_parse_madt_distributor are we going to
>> get? Why isn't the ACPI parsing in a common file? Why do I have to read
>> the same code on and on until my eyes bleed?
> 
> I will try to move it to common file irq-gic-acpi.c.
> 
>>
>>> +    struct acpi_madt_generic_distributor *dist;
>>> +
>>> +    dist = (struct acpi_madt_generic_distributor *)header;
>>> +
>>> +    if (BAD_MADT_ENTRY(dist, end))
>>> +            return -EINVAL;
>>> +
>>> +    dist_phys_base = dist->base_address;
>>> +    return 0;
>>> +}
>>> +
>>> +static int gic_acpi_gsi_desc_populate(struct acpi_gsi_descriptor *data,
>>> +                                  u32 gsi, unsigned int irq_type)
>>> +{
>>> +    /*
>>> +     * Encode GSI and triggering information the way the GIC likes
>>> +     * them.
>>> +     */
>>> +    if (WARN_ON(gsi < 16))
>>> +            return -EINVAL;
>>> +
>>> +    if (gsi >= 32) {
>>> +            data->param[0] = 0;             /* SPI */
>>> +            data->param[1] = gsi - 32;
>>> +            data->param[2] = irq_type;
>>> +    } else {
>>> +            data->param[0] = 1;             /* PPI */
>>> +            data->param[1] = gsi - 16;
>>> +            data->param[2] = 0xff << 4 | irq_type;
>>> +    }
>>> +
>>> +    data->param_count = 3;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int __init
>>> +gic_acpi_init(struct acpi_table_header *table)
>>> +{
>>> +    int count, i, err = 0;
>>> +    void __iomem *dist_base;
>>> +
>>> +    /* Get distributor base address */
>>> +    count = acpi_parse_entries(ACPI_SIG_MADT,
>>> +                            sizeof(struct acpi_table_madt),
>>> +                            gic_acpi_parse_madt_distributor, table,
>>> +                            ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0);
>>> +    if (count <= 0) {
>>> +            pr_err("No valid GICD entry exist\n");
>>> +            return -EINVAL;
>>> +    } else if (count > 1) {
>>> +            pr_err("More than one GICD entry detected\n");
>>> +            return -EINVAL;
>>> +    }
>>> +
>>> +    dist_base = ioremap(dist_phys_base, ACPI_GICV3_DIST_MEM_SIZE);
>>> +    if (!dist_base) {
>>> +            pr_err("Unable to map GICD registers\n");
>>> +            return -ENOMEM;
>>> +    }
>>> +
>>> +    err = detect_distributor(dist_base);
>>> +    if (err) {
>>> +            pr_err("No distributor detected at @%p, giving up", dist_base);
>>> +            goto out_dist_unmap;
>>> +    }
>>> +
>>> +    /* Collect redistributor base addresses */
>>> +    count = acpi_parse_entries(ACPI_SIG_MADT,
>>> +                    sizeof(struct acpi_table_madt),
>>> +                    gic_acpi_parse_madt_redist, table,
>>> +                    ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, 0);
>>> +    if (count <= 0) {
>>> +            pr_info("No valid GICR entries exist\n");
>>> +            err = -EINVAL;
>>> +            goto out_redist_unmap;
>>> +    }
>>> +
>>> +    err = gic_init_bases(dist_base, redist_regs, nr_redist_regions, 0,
>>> +                         (void *)ACPI_IRQ_MODEL_GIC);
>>
>> There is no other global identifier for GICv3? It feels a bit weird to
>> use the same ID as GICv2 (though probably not harmful as you can't have
>> both as the same time). What are you planning to use for the ITS then?
>> You must make sure you have a global namespace.
> 
> I will use the ITS physical base address as the token for ITS, maybe I
> can use the GICD physical base address here instead?

That should be fine as long as the physical address cannot be
interpreted as a kernel address. Which is still a bit dodgy. I'd be more
happy if you had a proper namespace.

>>
>>> +    if (err)
>>> +            goto out_redist_unmap;
>>> +
>>> +    acpi_set_irq_model(ACPI_IRQ_MODEL_GIC, ACPI_IRQ_MODEL_GIC,
>>> +                       gic_acpi_gsi_desc_populate);
>>> +    return 0;
>>> +
>>> +out_redist_unmap:
>>> +    for (i = 0; i < nr_redist_regions; i++)
>>> +            if (redist_regs[i].redist_base)
>>> +                    iounmap(redist_regs[i].redist_base);
>>> +    kfree(redist_regs);
>>> +out_dist_unmap:
>>> +    iounmap(dist_base);
>>> +    return err;
>>> +}
>>> +IRQCHIP_ACPI_DECLARE(gic_v3, ACPI_MADT_GIC_VERSION_V3, gic_acpi_init);
>>> +IRQCHIP_ACPI_DECLARE(gic_v4, ACPI_MADT_GIC_VERSION_V4, gic_acpi_init);
>>
>> As mentioned before, this doesn't work.
> 
> hmm, I think we need more discussion for this one, but we need to match
> V4 for GICv3 drivers, and everything will be the same in the dirver
> as I said before.

And as I said before, you don't need to distinguish v3 from v4 in the
ACPI code. Matching GICv3 is enough.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux