Re: [PATCH V8 5/8] irqchip/gicv3-its: Refactor ITS DT init code to prepare for ACPI

[Hanjun Guo]

On 2016/8/18 14:42, Tomasz Nowicki wrote:
> On 17.08.2016 17:58, Bjorn Helgaas wrote:
>> On Wed, Aug 17, 2016 at 04:33:02PM +0800, Hanjun Guo wrote:
>>> On 2016/8/11 18:06, Tomasz Nowicki wrote:
>>>> In order to add ACPI support we need to isolate ACPI&DT common code and
>>>> move DT logic to corresponding functions. To achieve this we are using
>>>> firmware agnostic handle which can be unpacked to either DT or ACPI node.
>>>>
>>>> No functional changes other than a very minor one:
>>>> 1. Terminate its_init call with -ENODEV for non-DT case which allows
>>>> to remove hack from its-gic-v3.c.
>>>> 2. Fix ITS base register address type (from 'unsigned long' to 'phys_addr_t'),
>>>> as a bonus we get nice string formatting.
>>>> 3. Since there is only one of ITS parent domain convert it to static global
>>>> variable and drop the parameter from its_probe_one. Users can refer to it
>>>> in more convenient way then.
>>> [...]
>>>> -static int __init its_probe(struct device_node *node,
>>>> -                struct irq_domain *parent)
>>>> +static int __init its_probe_one(struct resource *res,
>>>> +                struct fwnode_handle *handle, int numa_node)
>>>>  {
>>>> -    struct resource res;
>>>>      struct its_node *its;
>>>>      void __iomem *its_base;
>>>>      u32 val;
>>>>      u64 baser, tmp;
>>>>      int err;
>>>>
>>>> -    err = of_address_to_resource(node, 0, &res);
>>>> -    if (err) {
>>>> -        pr_warn("%s: no regs?\n", node->full_name);
>>>> -        return -ENXIO;
>>>> -    }
>>>> -
>>>> -    its_base = ioremap(res.start, resource_size(&res));
>>>> +    its_base = ioremap(res->start, resource_size(res));
>>>>      if (!its_base) {
>>>> -        pr_warn("%s: unable to map registers\n", node->full_name);
>>>> +        pr_warn("ITS@%pa: Unable to map ITS registers\n", &res->start);
>>>>          return -ENOMEM;
>>>>      }
>>>>
>>>>      val = readl_relaxed(its_base + GITS_PIDR2) & GIC_PIDR2_ARCH_MASK;
>>>>      if (val != 0x30 && val != 0x40) {
>>>> -        pr_warn("%s: no ITS detected, giving up\n", node->full_name);
>>>> +        pr_warn("ITS@%pa: No ITS detected, giving up\n", &res->start);
>>>>          err = -ENODEV;
>>>>          goto out_unmap;
>>>>      }
>>>>
>>>>      err = its_force_quiescent(its_base);
>>>>      if (err) {
>>>> -        pr_warn("%s: failed to quiesce, giving up\n",
>>>> -            node->full_name);
>>>> +        pr_warn("ITS@%pa: Failed to quiesce, giving up\n", &res->start);
>>>>          goto out_unmap;
>>>>      }
>>>>
>>>> -    pr_info("ITS: %s\n", node->full_name);
>>>> +    pr_info("ITS@%pa\n", &res->start);
>>>                 ^^
>>>
>>> When I was testing this patch set I found message printed as below:
>>>
>>> [    0.000000] ITS@0x00000000c6000000
>>
>> I think it'd be nicer to print the resource with %pR so we see the
>> type and size in a way that matches other physical address usage.
>
> The intention was to keep previous message layout but while we are here, %pR usage for this one pr_info seems nice to me.
>
>>
>> I don't know whether there is or should be a struct device associated
>> with the ITS.  The its_probe_one() function looks similar to regular
>> driver probe functions, so maybe there should be.
>>
>> If there were a struct device associated with the ITS, it'd be nicer
>> to use dev_info() as well, of course.
>
> Indeed dev_info() would be nice but there is no struct device for ITS.

Yes, it's configured in the MADT table not in the DSDT, which has no struct
device associated with it.

Thanks
Hanjun