Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-09-28 Thread Marc Zyngier 
On 25/09/15 12:56, majun (F) wrote:
> 
> 
> 在 2015/9/25 3:30, Marc Zyngier 写道:
>> On Wed, 23 Sep 2015 15:24:50 +0800
>> "majun (F)"  wrote:
>>
>> [...]
>>
> +static int mbigen_device_init(struct mbigen_chip *chip,
> +   struct device_node *node)
> [...]
> +
> +core_initcall(mbigen_init);

 That's the wrong thing to do. The interrupt controller should be
 probed with IRQCHIP_DECLARE(). Yes, this creates a dependency
 problem between the MSI layer and the irqchip layer, but that
 should be easy (-ish) to solve.
>>>
>>> Based on our discusstion about DTS,I will change the code likes below:
>>> IRQCHIP_DECLARE(hisi_mbigen, "hisilicon,mbigen-v2", mbigen_of_init);
>>>
>>> Mbigen device is initialized as a interrupt controller.
>>>
>>> But I still can't call platform_msi_domain_alloc_irqs()
>>> to apply the msi irqs.
>>>
>>> It think this is what you said "dependency problem between
>>>  the MSI layer and the irqchip layer" , am i right ?
>>>
>>> Do you have any idea about this problem?
>>
>> You need to have multiple phases for initializing this beast:
>> - IRQCHIP_DECLARE() to create the irqchips, allocate the domains and
>>   the main data structures,
>> - platform device probing of the top-level device to do some HW probing
>>   and to kick of_platform_populate on the subnodes,
>> - Handle the the subnode probing, allocate the MSIs, and finish the
>>   initialization of the irqchip how that you have all the information.
> 
> Ok, I got it. But I still have a question.
> According to your suggestions, the initial flow is:
> 
> Step1: IRQCHIP_DECLARE() to create the irqchips, allocate the domains and
>   the main data structures,
> Step2: Parse the device node and apply the irq(named as *device-virq*) within 
> mbigen-device   
>   domain (handled by irq core code).
> Step3: platform device probing of the top-level device to do some HW probing
>   and to kick of_platform_populate on the subnodes,
> Step4: Handle the the subnode probing, allocate the MSIs(named as 
> *msi-virq*), and finish the
>   initialization of the irqchip how that you have all the information.
> 
> My questions is:
>   How to connect msi-virq and device-virq ?
> 
> In my v4 version, I used the
> 
> irq_set_chained_handler(msi-virq, mbigen_handle_cascade_irq);
> 
> as msi-virq primary handler function .
> 
> Then find  device-virq in mbigen_handle_cascade_irq()
> and call device-virq corresponding handler fucntion.
> 
> But it seems not a right solution.
> 
> So I want try another solution for this problem.
> 
> [1] In step2, when the interrupt controller map function is called, using
>   irq_set_chip_and_handler(device-irq, _irq_chip, 
> handle_fasteoi_irq);
> [2] In step4, using
>   for_each_msi_entry(desc, _dev->dev) {
>   request_irq( msi-virq, msi_irq_handler, **);
>   }
> 
> But I am not sure about this solution, please review this and offer me some 
> suggestions.

I don't see what is wrong with keeping it as a chained irq handler.
Actually, you really need it to be a chained handler (because this is
what it is). So using irq_set_chained_handler_and_data() is probably the
right thing to do.

Thanks,

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

Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-09-28 Thread Marc Zyngier 
On 25/09/15 12:56, majun (F) wrote:
> 
> 
> 在 2015/9/25 3:30, Marc Zyngier 写道:
>> On Wed, 23 Sep 2015 15:24:50 +0800
>> "majun (F)"  wrote:
>>
>> [...]
>>
> +static int mbigen_device_init(struct mbigen_chip *chip,
> +   struct device_node *node)
> [...]
> +
> +core_initcall(mbigen_init);

 That's the wrong thing to do. The interrupt controller should be
 probed with IRQCHIP_DECLARE(). Yes, this creates a dependency
 problem between the MSI layer and the irqchip layer, but that
 should be easy (-ish) to solve.
>>>
>>> Based on our discusstion about DTS,I will change the code likes below:
>>> IRQCHIP_DECLARE(hisi_mbigen, "hisilicon,mbigen-v2", mbigen_of_init);
>>>
>>> Mbigen device is initialized as a interrupt controller.
>>>
>>> But I still can't call platform_msi_domain_alloc_irqs()
>>> to apply the msi irqs.
>>>
>>> It think this is what you said "dependency problem between
>>>  the MSI layer and the irqchip layer" , am i right ?
>>>
>>> Do you have any idea about this problem?
>>
>> You need to have multiple phases for initializing this beast:
>> - IRQCHIP_DECLARE() to create the irqchips, allocate the domains and
>>   the main data structures,
>> - platform device probing of the top-level device to do some HW probing
>>   and to kick of_platform_populate on the subnodes,
>> - Handle the the subnode probing, allocate the MSIs, and finish the
>>   initialization of the irqchip how that you have all the information.
> 
> Ok, I got it. But I still have a question.
> According to your suggestions, the initial flow is:
> 
> Step1: IRQCHIP_DECLARE() to create the irqchips, allocate the domains and
>   the main data structures,
> Step2: Parse the device node and apply the irq(named as *device-virq*) within 
> mbigen-device   
>   domain (handled by irq core code).
> Step3: platform device probing of the top-level device to do some HW probing
>   and to kick of_platform_populate on the subnodes,
> Step4: Handle the the subnode probing, allocate the MSIs(named as 
> *msi-virq*), and finish the
>   initialization of the irqchip how that you have all the information.
> 
> My questions is:
>   How to connect msi-virq and device-virq ?
> 
> In my v4 version, I used the
> 
> irq_set_chained_handler(msi-virq, mbigen_handle_cascade_irq);
> 
> as msi-virq primary handler function .
> 
> Then find  device-virq in mbigen_handle_cascade_irq()
> and call device-virq corresponding handler fucntion.
> 
> But it seems not a right solution.
> 
> So I want try another solution for this problem.
> 
> [1] In step2, when the interrupt controller map function is called, using
>   irq_set_chip_and_handler(device-irq, _irq_chip, 
> handle_fasteoi_irq);
> [2] In step4, using
>   for_each_msi_entry(desc, _dev->dev) {
>   request_irq( msi-virq, msi_irq_handler, **);
>   }
> 
> But I am not sure about this solution, please review this and offer me some 
> suggestions.

I don't see what is wrong with keeping it as a chained irq handler.
Actually, you really need it to be a chained handler (because this is
what it is). So using irq_set_chained_handler_and_data() is probably the
right thing to do.

Thanks,

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

Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-09-25 Thread majun (F) 


在 2015/9/25 3:30, Marc Zyngier 写道:
> On Wed, 23 Sep 2015 15:24:50 +0800
> "majun (F)"  wrote:
> 
> [...]
> 
 +static int mbigen_device_init(struct mbigen_chip *chip,
 +   struct device_node *node)
[...]
 +
 +core_initcall(mbigen_init);
>>>
>>> That's the wrong thing to do. The interrupt controller should be
>>> probed with IRQCHIP_DECLARE(). Yes, this creates a dependency
>>> problem between the MSI layer and the irqchip layer, but that
>>> should be easy (-ish) to solve.
>>
>> Based on our discusstion about DTS,I will change the code likes below:
>> IRQCHIP_DECLARE(hisi_mbigen, "hisilicon,mbigen-v2", mbigen_of_init);
>>
>> Mbigen device is initialized as a interrupt controller.
>>
>> But I still can't call platform_msi_domain_alloc_irqs()
>> to apply the msi irqs.
>>
>> It think this is what you said "dependency problem between
>>  the MSI layer and the irqchip layer" , am i right ?
>>
>> Do you have any idea about this problem?
> 
> You need to have multiple phases for initializing this beast:
> - IRQCHIP_DECLARE() to create the irqchips, allocate the domains and
>   the main data structures,
> - platform device probing of the top-level device to do some HW probing
>   and to kick of_platform_populate on the subnodes,
> - Handle the the subnode probing, allocate the MSIs, and finish the
>   initialization of the irqchip how that you have all the information.

Ok, I got it. But I still have a question.
According to your suggestions, the initial flow is:

Step1: IRQCHIP_DECLARE() to create the irqchips, allocate the domains and
the main data structures,
Step2: Parse the device node and apply the irq(named as *device-virq*) within 
mbigen-device 
domain (handled by irq core code).
Step3: platform device probing of the top-level device to do some HW probing
and to kick of_platform_populate on the subnodes,
Step4: Handle the the subnode probing, allocate the MSIs(named as *msi-virq*), 
and finish the
initialization of the irqchip how that you have all the information.

My questions is:
How to connect msi-virq and device-virq ?

In my v4 version, I used the

irq_set_chained_handler(msi-virq, mbigen_handle_cascade_irq);

as msi-virq primary handler function .

Then find  device-virq in mbigen_handle_cascade_irq()
and call device-virq corresponding handler fucntion.

But it seems not a right solution.

So I want try another solution for this problem.

[1] In step2, when the interrupt controller map function is called, using
irq_set_chip_and_handler(device-irq, _irq_chip, 
handle_fasteoi_irq);
[2] In step4, using
for_each_msi_entry(desc, _dev->dev) {
request_irq( msi-virq, msi_irq_handler, **);
}

But I am not sure about this solution, please review this and offer me some 
suggestions.

Best Regards,
Ma Jun






--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-09-25 Thread majun (F) 


在 2015/9/25 3:30, Marc Zyngier 写道:
> On Wed, 23 Sep 2015 15:24:50 +0800
> "majun (F)"  wrote:
> 
> [...]
> 
 +static int mbigen_device_init(struct mbigen_chip *chip,
 +   struct device_node *node)
[...]
 +
 +core_initcall(mbigen_init);
>>>
>>> That's the wrong thing to do. The interrupt controller should be
>>> probed with IRQCHIP_DECLARE(). Yes, this creates a dependency
>>> problem between the MSI layer and the irqchip layer, but that
>>> should be easy (-ish) to solve.
>>
>> Based on our discusstion about DTS,I will change the code likes below:
>> IRQCHIP_DECLARE(hisi_mbigen, "hisilicon,mbigen-v2", mbigen_of_init);
>>
>> Mbigen device is initialized as a interrupt controller.
>>
>> But I still can't call platform_msi_domain_alloc_irqs()
>> to apply the msi irqs.
>>
>> It think this is what you said "dependency problem between
>>  the MSI layer and the irqchip layer" , am i right ?
>>
>> Do you have any idea about this problem?
> 
> You need to have multiple phases for initializing this beast:
> - IRQCHIP_DECLARE() to create the irqchips, allocate the domains and
>   the main data structures,
> - platform device probing of the top-level device to do some HW probing
>   and to kick of_platform_populate on the subnodes,
> - Handle the the subnode probing, allocate the MSIs, and finish the
>   initialization of the irqchip how that you have all the information.

Ok, I got it. But I still have a question.
According to your suggestions, the initial flow is:

Step1: IRQCHIP_DECLARE() to create the irqchips, allocate the domains and
the main data structures,
Step2: Parse the device node and apply the irq(named as *device-virq*) within 
mbigen-device 
domain (handled by irq core code).
Step3: platform device probing of the top-level device to do some HW probing
and to kick of_platform_populate on the subnodes,
Step4: Handle the the subnode probing, allocate the MSIs(named as *msi-virq*), 
and finish the
initialization of the irqchip how that you have all the information.

My questions is:
How to connect msi-virq and device-virq ?

In my v4 version, I used the

irq_set_chained_handler(msi-virq, mbigen_handle_cascade_irq);

as msi-virq primary handler function .

Then find  device-virq in mbigen_handle_cascade_irq()
and call device-virq corresponding handler fucntion.

But it seems not a right solution.

So I want try another solution for this problem.

[1] In step2, when the interrupt controller map function is called, using
irq_set_chip_and_handler(device-irq, _irq_chip, 
handle_fasteoi_irq);
[2] In step4, using
for_each_msi_entry(desc, _dev->dev) {
request_irq( msi-virq, msi_irq_handler, **);
}

But I am not sure about this solution, please review this and offer me some 
suggestions.

Best Regards,
Ma Jun






--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-09-24 Thread Marc Zyngier 
On Wed, 23 Sep 2015 15:24:50 +0800
"majun (F)"  wrote:

[...]

> >> +static int mbigen_device_init(struct mbigen_chip *chip,
> >> +   struct device_node *node)
> >> +{
> >> +   struct mbigen_device *mgn_dev;
> >> +   struct device_node *msi_np;
> >> +   struct irq_domain *msi_domain;
> >> +   struct msi_desc *desc;
> >> +   struct mbigen_irq_data *mgn_irq_data;
> >> +   u32 nvec, dev_id;
> >> +   int ret;
> >> +
> >> +   of_property_read_u32(node, "nr-interrupts", );
> >> +   if (!nvec)
> >> +   return -EINVAL;
> >> +
> >> +   ret = of_property_read_u32_index(node, "msi-parent", 1, _id);
> >> +   if (ret)
> >> +   return -EINVAL;
> >> +
> >> +   msi_np = of_parse_phandle(node, "msi-parent", 0);
> >> +   if (!msi_np) {
> >> +   pr_err("%s- no msi node found: %s\n", __func__,
> >> +   node->full_name);
> >> +   return -ENXIO;
> >> +   }
> >> +
> >> +   msi_domain = irq_find_matching_host(msi_np, 
> >> DOMAIN_BUS_PLATFORM_MSI);
> >> +   if (!msi_domain) {
> >> +   pr_err("MBIGEN: no platform-msi domain for %s\n",
> >> +   msi_np->full_name);
> >> +   return -ENXIO;
> >> +   }
> > 
> > There shouldn't be any need for all this msi-parent handling if you
> > correctly created the platform-devices for the mbigen devices. I've
> > gone though quite some effort to make sure none of that was necessary,
> 
> Do you mean I should use the of_msi_configure() function at here,
> or
> msi-parent should be handled during initial when this function be
> called in function of_platform_device_create_pdata() ?

The second option. I understand this is a bit complicated because your
node is both an interrupt controller and an MSI client.

You need a platform device to represent the MSI client and use this
to get the MSI setup to be done automatically by the core code.
of_platform_populate() should normally solve this neatly. On top of
that, you need to use the normal irqchip infrastructure to probe the
irqchip side of the node.

[...]

> >> +static int __init mbigen_init(void)
> >> +{
> >> +   struct device_node *np;
> >> +
> >> +   for (np = of_find_matching_node(NULL, mbigen_chip_id); np;
> >> +np = of_find_matching_node(np, mbigen_chip_id)) {
> >> +   mbigen_of_init(np);
> >> +   }
> >> +
> >> +   return 0;
> >> +}
> >> +
> >> +core_initcall(mbigen_init);
> > 
> > That's the wrong thing to do. The interrupt controller should be
> > probed with IRQCHIP_DECLARE(). Yes, this creates a dependency
> > problem between the MSI layer and the irqchip layer, but that
> > should be easy (-ish) to solve.
> 
> Based on our discusstion about DTS,I will change the code likes below:
> IRQCHIP_DECLARE(hisi_mbigen, "hisilicon,mbigen-v2", mbigen_of_init);
> 
> Mbigen device is initialized as a interrupt controller.
> 
> But I still can't call platform_msi_domain_alloc_irqs()
> to apply the msi irqs.
> 
> It think this is what you said "dependency problem between
>  the MSI layer and the irqchip layer" , am i right ?
> 
> Do you have any idea about this problem?

You need to have multiple phases for initializing this beast:
- IRQCHIP_DECLARE() to create the irqchips, allocate the domains and
  the main data structures,
- platform device probing of the top-level device to do some HW probing
  and to kick of_platform_populate on the subnodes,
- Handle the the subnode probing, allocate the MSIs, and finish the
  initialization of the irqchip how that you have all the information.

Thanks,

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

Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-09-24 Thread Marc Zyngier 
On Wed, 23 Sep 2015 15:24:50 +0800
"majun (F)"  wrote:

[...]

> >> +static int mbigen_device_init(struct mbigen_chip *chip,
> >> +   struct device_node *node)
> >> +{
> >> +   struct mbigen_device *mgn_dev;
> >> +   struct device_node *msi_np;
> >> +   struct irq_domain *msi_domain;
> >> +   struct msi_desc *desc;
> >> +   struct mbigen_irq_data *mgn_irq_data;
> >> +   u32 nvec, dev_id;
> >> +   int ret;
> >> +
> >> +   of_property_read_u32(node, "nr-interrupts", );
> >> +   if (!nvec)
> >> +   return -EINVAL;
> >> +
> >> +   ret = of_property_read_u32_index(node, "msi-parent", 1, _id);
> >> +   if (ret)
> >> +   return -EINVAL;
> >> +
> >> +   msi_np = of_parse_phandle(node, "msi-parent", 0);
> >> +   if (!msi_np) {
> >> +   pr_err("%s- no msi node found: %s\n", __func__,
> >> +   node->full_name);
> >> +   return -ENXIO;
> >> +   }
> >> +
> >> +   msi_domain = irq_find_matching_host(msi_np, 
> >> DOMAIN_BUS_PLATFORM_MSI);
> >> +   if (!msi_domain) {
> >> +   pr_err("MBIGEN: no platform-msi domain for %s\n",
> >> +   msi_np->full_name);
> >> +   return -ENXIO;
> >> +   }
> > 
> > There shouldn't be any need for all this msi-parent handling if you
> > correctly created the platform-devices for the mbigen devices. I've
> > gone though quite some effort to make sure none of that was necessary,
> 
> Do you mean I should use the of_msi_configure() function at here,
> or
> msi-parent should be handled during initial when this function be
> called in function of_platform_device_create_pdata() ?

The second option. I understand this is a bit complicated because your
node is both an interrupt controller and an MSI client.

You need a platform device to represent the MSI client and use this
to get the MSI setup to be done automatically by the core code.
of_platform_populate() should normally solve this neatly. On top of
that, you need to use the normal irqchip infrastructure to probe the
irqchip side of the node.

[...]

> >> +static int __init mbigen_init(void)
> >> +{
> >> +   struct device_node *np;
> >> +
> >> +   for (np = of_find_matching_node(NULL, mbigen_chip_id); np;
> >> +np = of_find_matching_node(np, mbigen_chip_id)) {
> >> +   mbigen_of_init(np);
> >> +   }
> >> +
> >> +   return 0;
> >> +}
> >> +
> >> +core_initcall(mbigen_init);
> > 
> > That's the wrong thing to do. The interrupt controller should be
> > probed with IRQCHIP_DECLARE(). Yes, this creates a dependency
> > problem between the MSI layer and the irqchip layer, but that
> > should be easy (-ish) to solve.
> 
> Based on our discusstion about DTS,I will change the code likes below:
> IRQCHIP_DECLARE(hisi_mbigen, "hisilicon,mbigen-v2", mbigen_of_init);
> 
> Mbigen device is initialized as a interrupt controller.
> 
> But I still can't call platform_msi_domain_alloc_irqs()
> to apply the msi irqs.
> 
> It think this is what you said "dependency problem between
>  the MSI layer and the irqchip layer" , am i right ?
> 
> Do you have any idea about this problem?

You need to have multiple phases for initializing this beast:
- IRQCHIP_DECLARE() to create the irqchips, allocate the domains and
  the main data structures,
- platform device probing of the top-level device to do some HW probing
  and to kick of_platform_populate on the subnodes,
- Handle the the subnode probing, allocate the MSIs, and finish the
  initialization of the irqchip how that you have all the information.

Thanks,

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

Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-09-23 Thread majun (F) 
Hi Marc:
Thanks for reviewing this huge patch.

在 2015/9/22 5:53, Marc Zyngier 写道:
> On Wed, 19 Aug 2015 03:55:19 +0100
> MaJun  wrote:
> 
>> From: Ma Jun 
>>
[...]
>> + * hwirq[32:12]: did. device id
>> + * hwirq[11:8]: nid. mbigen node number
>> + * hwirq[7:0]: pin. hardware pin offset of this interrupt
>> + */
>> +#defineCOMPOSE_MBIGEN_HWIRQ(did, nid, pin) \
>> +   (((did) << MBIGEN_DEV_SHIFT) | \
>> +   ((nid) << MBIGEN_NODE_SHIFT) | (pin))
> 
> The fact that you have to create a "virtual" hwirq number is an
> indication that you are doing something wrong. It seems to me that it
> would be much simpler if you had one domain per mode, with the pin
> being the actual hwirq (as you would expect it), you'd have a much
> simpler design overall.
> 

I will remove the mbigen node to make hardware structure simpler as below.
After changing, I can have one domain per mbigen chip.

 mbigen chip
  --
  |  |   ||
 dev1  dev2 dev3   dev4


> Do not try to have a global hwirq space unless you really need it. So
> far, I haven't seen anything here that mandate such a complex solution.
> 
>> +
>> +/* get the interrupt pin offset from mbigen hwirq */
[...]
>> +
>> +static void mbigen_eoi_irq(struct irq_data *data)
>> +{
>> +
>> +   struct mbigen_irq_data *mgn_irq_data = 
>> irq_data_get_irq_chip_data(data);
>> +   struct irq_chip *chip = irq_get_chip(mgn_irq_data->parent_irq);
>> +   struct irq_data *parent_d = 
>> irq_get_irq_data(mgn_irq_data->parent_irq);
>> +   u32 pin_offset, ofst, mask;
>> +
>> +   pin_offset = GET_IRQ_PIN_OFFSET(data->hwirq);
>> +   ofst = pin_offset / 32 * 4;
>> +   mask = 1 << (pin_offset % 32);
>> +
>> +   writel_relaxed(mask, mgn_irq_data->base + ofst
>> +   + REG_MBIGEN_CLEAR_OFFSET);
> 
> What exactly is the effect of that write? Does it allow the resampling
> of a level interrupt so that a LPI can be injected again if level is
> still high?

Yes.
So we cleared irq status at here.

> 
>> +
>> +   if (chip && chip->irq_eoi)
>> +   chip->irq_eoi(parent_d);
>> +}
>> +
[...]
>> +
>> +static struct irq_domain_ops mbigen_domain_ops = {
>> +   .xlate = mbigen_domain_xlate,
>> +   .map = mbigen_domain_map,
>> +};
>> +
>> +static void mbigen_handle_cascade_irq(unsigned int irq, struct irq_desc 
>> *desc)
> 
> No. This is a chained interrupt handler, not a standard interrupt
> handler.
>> +{
>> +   struct mbigen_irq_data *mgn_irq_data = irq_get_handler_data(irq);
>> +   struct mbigen_device *mgn_dev = mgn_irq_data->dev;
>> +   struct irq_domain *domain = mgn_dev->chip->domain;
>> +   unsigned int cascade_irq;
>> +   u32 hwirq;
>> +
>> +   hwirq = COMPOSE_MBIGEN_HWIRQ(mgn_irq_data->dev_id,
>> +   mgn_irq_data->nid,
>> +   mgn_irq_data->pin_offset);
>> +
>> +   /* find cascade_irq within mbigen domain */
>> +   cascade_irq = irq_find_mapping(domain, hwirq);
>> +
>> +   if (unlikely(!cascade_irq))
>> +   handle_bad_irq(irq, desc);
>> +   else
>> +   generic_handle_irq(cascade_irq);
> 
> A consequence of the above is missing chained_irq_{enter,exit}.
> 
>> +}
>> +
>> +/*
>> + * get_mbigen_node_info() - Get the mbigen node information
>> + * and compose a new hwirq.
>> + *
>> + * @irq: irq number need to be handled
>> + * @device_id: id of device which generates this interrupt
>> + * @node_num: number of mbigen node this interrupt connected.
>> + * @offset: interrupt pin offset in a mbigen node.
>> + */
>> +static int get_mbigen_node_info(u32 irq, struct mbigen_device *dev,
>> +   struct mbigen_irq_data *mgn_irq_data)
>> +{
>> +   struct irq_data *irq_data = irq_get_irq_data(irq);
>> +   struct mbigen_node *mgn_node;
>> +   u32 irqs_range = 0, tmp;
>> +   u32 msi_index;
>> +
>> +   mgn_irq_data->dev_id = dev->did;
>> +   msi_index = irq_data->hwirq & 0xff;
>> +
>> +   raw_spin_lock(>lock);
>> +
>> +   list_for_each_entry(mgn_node, >mbigen_node_list, entry) {
>> +   tmp = irqs_range;
>> +   irqs_range += mgn_node->irq_nr;
>> +
>> +   if (msi_index < irqs_range) {
>> +   mgn_irq_data->nid = mgn_node->node_num;
>> +   mgn_irq_data->pin_offset =
>> +   mgn_node->pin_offset + (msi_index - tmp);
>> +   break;
>> +   }
>> +   }
>> +   raw_spin_unlock(>lock);
>> +
>> +   return 0;
>> +}
>> +
>> +/*
>> + * parse the information of mbigen node included in
>> + * mbigen device node.
>> + * @dev: the mbigen device pointer
>> + *
>> + * Some devices in hisilicon soc has more than 128
>> + * interrupts and beyond a mbigen node can connect.
>> + * So It need to be

Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-09-23 Thread majun (F) 
Hi Marc:
Thanks for reviewing this huge patch.

在 2015/9/22 5:53, Marc Zyngier 写道:
> On Wed, 19 Aug 2015 03:55:19 +0100
> MaJun  wrote:
> 
>> From: Ma Jun 
>>
[...]
>> + * hwirq[32:12]: did. device id
>> + * hwirq[11:8]: nid. mbigen node number
>> + * hwirq[7:0]: pin. hardware pin offset of this interrupt
>> + */
>> +#defineCOMPOSE_MBIGEN_HWIRQ(did, nid, pin) \
>> +   (((did) << MBIGEN_DEV_SHIFT) | \
>> +   ((nid) << MBIGEN_NODE_SHIFT) | (pin))
> 
> The fact that you have to create a "virtual" hwirq number is an
> indication that you are doing something wrong. It seems to me that it
> would be much simpler if you had one domain per mode, with the pin
> being the actual hwirq (as you would expect it), you'd have a much
> simpler design overall.
> 

I will remove the mbigen node to make hardware structure simpler as below.
After changing, I can have one domain per mbigen chip.

 mbigen chip
  --
  |  |   ||
 dev1  dev2 dev3   dev4


> Do not try to have a global hwirq space unless you really need it. So
> far, I haven't seen anything here that mandate such a complex solution.
> 
>> +
>> +/* get the interrupt pin offset from mbigen hwirq */
[...]
>> +
>> +static void mbigen_eoi_irq(struct irq_data *data)
>> +{
>> +
>> +   struct mbigen_irq_data *mgn_irq_data = 
>> irq_data_get_irq_chip_data(data);
>> +   struct irq_chip *chip = irq_get_chip(mgn_irq_data->parent_irq);
>> +   struct irq_data *parent_d = 
>> irq_get_irq_data(mgn_irq_data->parent_irq);
>> +   u32 pin_offset, ofst, mask;
>> +
>> +   pin_offset = GET_IRQ_PIN_OFFSET(data->hwirq);
>> +   ofst = pin_offset / 32 * 4;
>> +   mask = 1 << (pin_offset % 32);
>> +
>> +   writel_relaxed(mask, mgn_irq_data->base + ofst
>> +   + REG_MBIGEN_CLEAR_OFFSET);
> 
> What exactly is the effect of that write? Does it allow the resampling
> of a level interrupt so that a LPI can be injected again if level is
> still high?

Yes.
So we cleared irq status at here.

> 
>> +
>> +   if (chip && chip->irq_eoi)
>> +   chip->irq_eoi(parent_d);
>> +}
>> +
[...]
>> +
>> +static struct irq_domain_ops mbigen_domain_ops = {
>> +   .xlate = mbigen_domain_xlate,
>> +   .map = mbigen_domain_map,
>> +};
>> +
>> +static void mbigen_handle_cascade_irq(unsigned int irq, struct irq_desc 
>> *desc)
> 
> No. This is a chained interrupt handler, not a standard interrupt
> handler.
>> +{
>> +   struct mbigen_irq_data *mgn_irq_data = irq_get_handler_data(irq);
>> +   struct mbigen_device *mgn_dev = mgn_irq_data->dev;
>> +   struct irq_domain *domain = mgn_dev->chip->domain;
>> +   unsigned int cascade_irq;
>> +   u32 hwirq;
>> +
>> +   hwirq = COMPOSE_MBIGEN_HWIRQ(mgn_irq_data->dev_id,
>> +   mgn_irq_data->nid,
>> +   mgn_irq_data->pin_offset);
>> +
>> +   /* find cascade_irq within mbigen domain */
>> +   cascade_irq = irq_find_mapping(domain, hwirq);
>> +
>> +   if (unlikely(!cascade_irq))
>> +   handle_bad_irq(irq, desc);
>> +   else
>> +   generic_handle_irq(cascade_irq);
> 
> A consequence of the above is missing chained_irq_{enter,exit}.
> 
>> +}
>> +
>> +/*
>> + * get_mbigen_node_info() - Get the mbigen node information
>> + * and compose a new hwirq.
>> + *
>> + * @irq: irq number need to be handled
>> + * @device_id: id of device which generates this interrupt
>> + * @node_num: number of mbigen node this interrupt connected.
>> + * @offset: interrupt pin offset in a mbigen node.
>> + */
>> +static int get_mbigen_node_info(u32 irq, struct mbigen_device *dev,
>> +   struct mbigen_irq_data *mgn_irq_data)
>> +{
>> +   struct irq_data *irq_data = irq_get_irq_data(irq);
>> +   struct mbigen_node *mgn_node;
>> +   u32 irqs_range = 0, tmp;
>> +   u32 msi_index;
>> +
>> +   mgn_irq_data->dev_id = dev->did;
>> +   msi_index = irq_data->hwirq & 0xff;
>> +
>> +   raw_spin_lock(>lock);
>> +
>> +   list_for_each_entry(mgn_node, >mbigen_node_list, entry) {
>> +   tmp = irqs_range;
>> +   irqs_range += mgn_node->irq_nr;
>> +
>> +   if (msi_index < irqs_range) {
>> +   mgn_irq_data->nid = mgn_node->node_num;
>> +   mgn_irq_data->pin_offset =
>> +   mgn_node->pin_offset + (msi_index - tmp);
>> +   break;
>> +   }
>> +   }
>> +   raw_spin_unlock(>lock);
>> +
>> +   return 0;
>> +}
>> +
>> +/*
>> + * parse the information of mbigen node included in
>> + * mbigen device node.
>> + * @dev: the mbigen device pointer
>> + *
>> + * Some devices in hisilicon soc has more than 128
>> + * interrupts and beyond a mbigen node

Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-09-21 Thread Marc Zyngier 
On Wed, 19 Aug 2015 03:55:19 +0100
MaJun  wrote:

> From: Ma Jun 
> 
> Mbigen means Message Based Interrupt Generator(MBIGEN).
> 
> Its a kind of interrupt controller that collects
> 
> the interrupts from external devices and generate msi interrupt.
> 
> Mbigen is applied to reduce the number of wire connected interrupts.
> 
> As the peripherals increasing, the interrupts lines needed is
> increasing much, especially on the Arm64 server soc.
> 
> Therefore, the interrupt pin in gic is not enough to cover so
> many peripherals.
> 
> Mbigen is designed to fix this problem.
> 
> Mbigen chip locates in ITS or outside of ITS.
> 
> Mbigen chip hardware structure shows as below:
> 
> mbigen chip
> |-|---|
> mgn_node0 mgn_node1 mgn_node2
>  |   |---|  |---|--|
> dev1dev1dev2dev1   dev3   dev4
> 
> Each mbigen chip contains several mbigen nodes.
> 
> External devices can connects to mbigen node through wire connecting way.
> 
> Because a mbigen node only can support 128 interrupt maximum, depends
> on the interrupt lines number of devices, a device can connects to one
> more mbigen nodes.
> 
> Also, several different devices can connect to a same mbigen node.
> 
> When devices triggered interrupt,mbigen chip detects and collects
> the interrupts and generates the MBI interrupts by writing the ITS
> Translator register.

First remark: this patch is *huge*, which makes it very hard to review.
Can you please split future revisions into smaller bits that can be
more easily reviewed independently? MSI handling in one patch,
interrupt controller in another would be a sensible split.

> Signed-off-by: Ma Jun 
> ---
>  drivers/irqchip/Kconfig  |8 +
>  drivers/irqchip/Makefile |1 +
>  drivers/irqchip/irq-mbigen.c |  732 
> ++
>  3 files changed, 741 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/irqchip/irq-mbigen.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 120d815..356507f 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -27,6 +27,14 @@ config ARM_GIC_V3_ITS
> bool
> select PCI_MSI_IRQ_DOMAIN
> 
> +config HISILICON_IRQ_MBIGEN
> +   bool "Support mbigen interrupt controller"
> +   default n
> +   depends on ARM_GIC_V3 && ARM_GIC_V3_ITS
> +   help
> +Enable the mbigen interrupt controller used on
> +Hisilicon platform.
> +
>  config ARM_NVIC
> bool
> select IRQ_DOMAIN
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 11d08c9..c6f3d66 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_ARM_GIC) += irq-gic.o 
> irq-gic-common.o
>  obj-$(CONFIG_ARM_GIC_V2M)  += irq-gic-v2m.o
>  obj-$(CONFIG_ARM_GIC_V3)   += irq-gic-v3.o irq-gic-common.o
>  obj-$(CONFIG_ARM_GIC_V3_ITS)   += irq-gic-v3-its.o 
> irq-gic-v3-its-pci-msi.o irq-gic-v3-its-platform-msi.o
> +obj-$(CONFIG_HISILICON_IRQ_MBIGEN) += irq-mbigen.o
>  obj-$(CONFIG_ARM_NVIC) += irq-nvic.o
>  obj-$(CONFIG_ARM_VIC)  += irq-vic.o
>  obj-$(CONFIG_ATMEL_AIC_IRQ)+= irq-atmel-aic-common.o 
> irq-atmel-aic.o
> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
> new file mode 100644
> index 000..4bbbd76
> --- /dev/null
> +++ b/drivers/irqchip/irq-mbigen.c
> @@ -0,0 +1,732 @@
> +/*
> + * Copyright (C) 2014 Hisilicon Limited, All Rights Reserved.
> + * Author: Jun Ma 
> + * Author: Yun Wu 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "irqchip.h"
> +
> +#defineMBIGEN_NODE_SHIFT   (8)
> +#define MBIGEN_DEV_SHIFT   (12)
> +
> +/*
> + * To avoid the duplicate hwirq number problem
> + * we use device id, mbigen node number and interrupt
> + * pin offset to generate a new hwirq number in mbigen
> + * domain.
> + *
> + * hwirq[32:12]: did. device id
> + * hwirq[11:8]: nid. mbigen node number
> + * hwirq[7:0]: pin. hardware pin offset of this interrupt
> + */
> +#defineCOMPOSE_MBIGEN_HWIRQ(did, nid, pin) \

Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-09-21 Thread Marc Zyngier 
On Wed, 19 Aug 2015 03:55:19 +0100
MaJun  wrote:

> From: Ma Jun 
> 
> Mbigen means Message Based Interrupt Generator(MBIGEN).
> 
> Its a kind of interrupt controller that collects
> 
> the interrupts from external devices and generate msi interrupt.
> 
> Mbigen is applied to reduce the number of wire connected interrupts.
> 
> As the peripherals increasing, the interrupts lines needed is
> increasing much, especially on the Arm64 server soc.
> 
> Therefore, the interrupt pin in gic is not enough to cover so
> many peripherals.
> 
> Mbigen is designed to fix this problem.
> 
> Mbigen chip locates in ITS or outside of ITS.
> 
> Mbigen chip hardware structure shows as below:
> 
> mbigen chip
> |-|---|
> mgn_node0 mgn_node1 mgn_node2
>  |   |---|  |---|--|
> dev1dev1dev2dev1   dev3   dev4
> 
> Each mbigen chip contains several mbigen nodes.
> 
> External devices can connects to mbigen node through wire connecting way.
> 
> Because a mbigen node only can support 128 interrupt maximum, depends
> on the interrupt lines number of devices, a device can connects to one
> more mbigen nodes.
> 
> Also, several different devices can connect to a same mbigen node.
> 
> When devices triggered interrupt,mbigen chip detects and collects
> the interrupts and generates the MBI interrupts by writing the ITS
> Translator register.

First remark: this patch is *huge*, which makes it very hard to review.
Can you please split future revisions into smaller bits that can be
more easily reviewed independently? MSI handling in one patch,
interrupt controller in another would be a sensible split.

> Signed-off-by: Ma Jun 
> ---
>  drivers/irqchip/Kconfig  |8 +
>  drivers/irqchip/Makefile |1 +
>  drivers/irqchip/irq-mbigen.c |  732 
> ++
>  3 files changed, 741 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/irqchip/irq-mbigen.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 120d815..356507f 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -27,6 +27,14 @@ config ARM_GIC_V3_ITS
> bool
> select PCI_MSI_IRQ_DOMAIN
> 
> +config HISILICON_IRQ_MBIGEN
> +   bool "Support mbigen interrupt controller"
> +   default n
> +   depends on ARM_GIC_V3 && ARM_GIC_V3_ITS
> +   help
> +Enable the mbigen interrupt controller used on
> +Hisilicon platform.
> +
>  config ARM_NVIC
> bool
> select IRQ_DOMAIN
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 11d08c9..c6f3d66 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_ARM_GIC) += irq-gic.o 
> irq-gic-common.o
>  obj-$(CONFIG_ARM_GIC_V2M)  += irq-gic-v2m.o
>  obj-$(CONFIG_ARM_GIC_V3)   += irq-gic-v3.o irq-gic-common.o
>  obj-$(CONFIG_ARM_GIC_V3_ITS)   += irq-gic-v3-its.o 
> irq-gic-v3-its-pci-msi.o irq-gic-v3-its-platform-msi.o
> +obj-$(CONFIG_HISILICON_IRQ_MBIGEN) += irq-mbigen.o
>  obj-$(CONFIG_ARM_NVIC) += irq-nvic.o
>  obj-$(CONFIG_ARM_VIC)  += irq-vic.o
>  obj-$(CONFIG_ATMEL_AIC_IRQ)+= irq-atmel-aic-common.o 
> irq-atmel-aic.o
> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
> new file mode 100644
> index 000..4bbbd76
> --- /dev/null
> +++ b/drivers/irqchip/irq-mbigen.c
> @@ -0,0 +1,732 @@
> +/*
> + * Copyright (C) 2014 Hisilicon Limited, All Rights Reserved.
> + * Author: Jun Ma 
> + * Author: Yun Wu 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "irqchip.h"
> +
> +#defineMBIGEN_NODE_SHIFT   (8)
> +#define MBIGEN_DEV_SHIFT   (12)
> +
> +/*
> + * To avoid the duplicate hwirq number problem
> + * we use device id, mbigen node number and interrupt
> + * pin offset to generate a new hwirq number in mbigen
> + * domain.
> + *
> + * hwirq[32:12]: did. device id
> + * hwirq[11:8]: nid. mbigen node number
> + * hwirq[7:0]: pin.

Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-09-06 Thread majun (F) 
Hi Alexey:

在 2015/9/4 8:56, Alexey Klimov 写道:
> Hi Ma Jun,
> 
> On Tue, Sep 1, 2015 at 4:45 AM, majun (F)  wrote:
>> Hi Alexey:
>>
[...]
 +   mgn_chip->base = base;
 +   mgn_chip->node = node;
 +
 +   domain = irq_domain_add_tree(node, _domain_ops, mgn_chip);
 +   mgn_chip->domain = domain;
 +
 +   raw_spin_lock_init(_chip->lock);
 +   INIT_LIST_HEAD(_chip->entry);
 +   INIT_LIST_HEAD(_chip->mbigen_device_list);
 +
 +   for_each_child_of_node(node, child) {
 +   mbigen_device_init(mgn_chip, child);
>>>
>>> You don't check error from mbigen_device_init()
>> I don't think we need to check errors here.
>> mbigen_device_init() handle all errors.
> 
> For my eyes, mbigen_device_init() is static and used only once here.
> Here you don't check return value from it. If this is correct you can
> convert mbigen_device_init() to return void unless you have future
> plans to modify it.
> 
> Or you have option to check return value here and add error path.

Ok, This will be changed in next version.

> 
> Please add me to cc to next version of patch set which I guess will be v5.
> 

Got it. I will add you to cc list in next version.
Thanks for you review !

Ma Jun


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-09-06 Thread majun (F) 
Hi Alexey:

在 2015/9/4 8:56, Alexey Klimov 写道:
> Hi Ma Jun,
> 
> On Tue, Sep 1, 2015 at 4:45 AM, majun (F)  wrote:
>> Hi Alexey:
>>
[...]
 +   mgn_chip->base = base;
 +   mgn_chip->node = node;
 +
 +   domain = irq_domain_add_tree(node, _domain_ops, mgn_chip);
 +   mgn_chip->domain = domain;
 +
 +   raw_spin_lock_init(_chip->lock);
 +   INIT_LIST_HEAD(_chip->entry);
 +   INIT_LIST_HEAD(_chip->mbigen_device_list);
 +
 +   for_each_child_of_node(node, child) {
 +   mbigen_device_init(mgn_chip, child);
>>>
>>> You don't check error from mbigen_device_init()
>> I don't think we need to check errors here.
>> mbigen_device_init() handle all errors.
> 
> For my eyes, mbigen_device_init() is static and used only once here.
> Here you don't check return value from it. If this is correct you can
> convert mbigen_device_init() to return void unless you have future
> plans to modify it.
> 
> Or you have option to check return value here and add error path.

Ok, This will be changed in next version.

> 
> Please add me to cc to next version of patch set which I guess will be v5.
> 

Got it. I will add you to cc list in next version.
Thanks for you review !

Ma Jun


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-09-03 Thread Alexey Klimov 
Hi Ma Jun,

On Tue, Sep 1, 2015 at 4:45 AM, majun (F)  wrote:
> Hi Alexey:
>
> 在 2015/8/29 11:13, Alexey Klimov 写道:

[..]

>>> +*/
>>> +static u32 calc_irq_index(struct mbigen_device *dev, u32 nid, u32 offset)
>>> +{
>>> +   struct mbigen_node *mgn_node = NULL, *tmp;
>>> +   unsigned long flags;
>>> +   u32 index = 0;
>>> +
>>> +   raw_spin_lock_irqsave(>lock, flags);
>>> +   list_for_each_entry(tmp, >mbigen_node_list, entry) {
>>> +   if (tmp->node_num == nid)
>>> +   mgn_node = tmp;
>>> +   }
>>> +   raw_spin_unlock_irqrestore(>lock, flags);
>>> +
>>> +   if (mgn_node == NULL) {
>>> +   pr_err("No mbigen node found in device:%s\n",
>>> +dev->node->full_name);
>>> +   return -ENXIO;
>>> +   }
>>> +
>>> +   if ((offset <= (mgn_node->pin_offset + mgn_node->irq_nr))
>>> +   && (offset >= mgn_node->pin_offset))
>>> +   index = mgn_node->index_offset + (offset - 
>>> mgn_node->pin_offset);
>>> +   else {
>>> +   pr_err("Err: no invalid index\n");
>>
>> Please check this message.
>> 1. I don't know all details about this driver but is it really correct
>> "no invalid index"? Maybe you mean "no vaild index" or just "invalid
>> index"?
>> Just checking if i correctly understand this.
>>
> You are right.  This should be "no valid index"
>> 2. Imagine what info user/dmesg reader gets when she or he will see
>> such message? I suggest to add some info about driver that printed
>> this message.
>> You already have nice name in mbigen_irq_chip: "MBIGEN-v2". What do
>> you think about using it as prefix in your printk-based messages?
>> Please also consider revisiting other messages in this patch.
>>
> good suggestion.
>>
>>> +   index = -EINVAL;
>>> +   }
>>> +
>>> +   return index;
>>> +}
> [...]
>>> +   INIT_LIST_HEAD(dev_to_msi_list(_dev->dev));
>>> +
>>> +   ret = platform_msi_domain_alloc_irqs(_dev->dev,
>>> +nvec, mbigen_write_msg);
>>> +   if (ret)
>>> +   goto out_free_dev;
>>> +
>>> +   INIT_LIST_HEAD(_dev->entry);
>>> +   raw_spin_lock_init(_dev->lock);
>>> +   INIT_LIST_HEAD(_dev->mbigen_node_list);
>>> +
>>> +   /* Parse and get the info of mbigen nodes this
>>> +* device connected
>>> +*/
>>> +   parse_mbigen_node(mgn_dev);
>>> +
>>> +   mgn_irq_data = kcalloc(nvec, sizeof(*mgn_irq_data), GFP_KERNEL);
>>> +   if (!mgn_irq_data)
>>> +   return -ENOMEM;
>>
>> Hm. Do you need error path here instead of simple return -ENOMEM?
>> Maybe 'goto out_free_dev' will work for you.
> Right. Memory leak happened.
>>
>>> +   mgn_dev->mgn_data = mgn_irq_data;
>>> +
>>> +   for_each_msi_entry(desc, _dev->dev) {
>>> +   mbigen_set_irq_handler_data(desc, mgn_dev,
>>> +   
>>> _irq_data[desc->platform.msi_index]);
>>> +   irq_set_chained_handler(desc->irq, 
>>> mbigen_handle_cascade_irq);
>>> +   }
>>> +
>>> +   raw_spin_lock(>lock);
>>> +   list_add(_dev->entry, >mbigen_device_list);
>>> +   raw_spin_unlock(>lock);
>>> +
>>> +   return 0;
>>> +
>>> +out_free_dev:
>>> +   kfree(mgn_dev);
>>> +   pr_err("failed to get MSIs for device:%s (%d)\n", node->full_name,
>>> +   ret);
>>> +   return ret;
>>> +}
>>> +
>>> +/*
>>> + * Early initialization as an interrupt controller
>>> + */
>>> +static int __init mbigen_of_init(struct device_node *node)
>>> +{
>>> +   struct mbigen_chip *mgn_chip;
>>> +   struct device_node *child;
>>> +   struct irq_domain *domain;
>>> +   void __iomem *base;
>>> +   int err;
>>> +
>>> +   base = of_iomap(node, 0);
>>> +   if (!base) {
>>> +   pr_err("%s: unable to map registers\n", node->full_name);
>>> +   return -ENOMEM;
>>> +   }
>>> +
>>> +   mgn_chip = kzalloc(sizeof(*mgn_chip), GFP_KERNEL);
>>> +   if (!mgn_chip) {
>>> +   err = -ENOMEM;
>>> +   goto unmap_reg;
>>> +   }
>>> +
>>> +   mgn_chip->base = base;
>>> +   mgn_chip->node = node;
>>> +
>>> +   domain = irq_domain_add_tree(node, _domain_ops, mgn_chip);
>>> +   mgn_chip->domain = domain;
>>> +
>>> +   raw_spin_lock_init(_chip->lock);
>>> +   INIT_LIST_HEAD(_chip->entry);
>>> +   INIT_LIST_HEAD(_chip->mbigen_device_list);
>>> +
>>> +   for_each_child_of_node(node, child) {
>>> +   mbigen_device_init(mgn_chip, child);
>>
>> You don't check error from mbigen_device_init()
> I don't think we need to check errors here.
> mbigen_device_init() handle all errors.

For my eyes, mbigen_device_init() is static and used only once here.
Here you don't check return value from it. If this is correct you can
convert mbigen_device_init() to return void unless you have future
plans to modify it.

Or you have option

Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-09-03 Thread Alexey Klimov 
Hi Ma Jun,

On Tue, Sep 1, 2015 at 4:45 AM, majun (F)  wrote:
> Hi Alexey:
>
> 在 2015/8/29 11:13, Alexey Klimov 写道:

[..]

>>> +*/
>>> +static u32 calc_irq_index(struct mbigen_device *dev, u32 nid, u32 offset)
>>> +{
>>> +   struct mbigen_node *mgn_node = NULL, *tmp;
>>> +   unsigned long flags;
>>> +   u32 index = 0;
>>> +
>>> +   raw_spin_lock_irqsave(>lock, flags);
>>> +   list_for_each_entry(tmp, >mbigen_node_list, entry) {
>>> +   if (tmp->node_num == nid)
>>> +   mgn_node = tmp;
>>> +   }
>>> +   raw_spin_unlock_irqrestore(>lock, flags);
>>> +
>>> +   if (mgn_node == NULL) {
>>> +   pr_err("No mbigen node found in device:%s\n",
>>> +dev->node->full_name);
>>> +   return -ENXIO;
>>> +   }
>>> +
>>> +   if ((offset <= (mgn_node->pin_offset + mgn_node->irq_nr))
>>> +   && (offset >= mgn_node->pin_offset))
>>> +   index = mgn_node->index_offset + (offset - 
>>> mgn_node->pin_offset);
>>> +   else {
>>> +   pr_err("Err: no invalid index\n");
>>
>> Please check this message.
>> 1. I don't know all details about this driver but is it really correct
>> "no invalid index"? Maybe you mean "no vaild index" or just "invalid
>> index"?
>> Just checking if i correctly understand this.
>>
> You are right.  This should be "no valid index"
>> 2. Imagine what info user/dmesg reader gets when she or he will see
>> such message? I suggest to add some info about driver that printed
>> this message.
>> You already have nice name in mbigen_irq_chip: "MBIGEN-v2". What do
>> you think about using it as prefix in your printk-based messages?
>> Please also consider revisiting other messages in this patch.
>>
> good suggestion.
>>
>>> +   index = -EINVAL;
>>> +   }
>>> +
>>> +   return index;
>>> +}
> [...]
>>> +   INIT_LIST_HEAD(dev_to_msi_list(_dev->dev));
>>> +
>>> +   ret = platform_msi_domain_alloc_irqs(_dev->dev,
>>> +nvec, mbigen_write_msg);
>>> +   if (ret)
>>> +   goto out_free_dev;
>>> +
>>> +   INIT_LIST_HEAD(_dev->entry);
>>> +   raw_spin_lock_init(_dev->lock);
>>> +   INIT_LIST_HEAD(_dev->mbigen_node_list);
>>> +
>>> +   /* Parse and get the info of mbigen nodes this
>>> +* device connected
>>> +*/
>>> +   parse_mbigen_node(mgn_dev);
>>> +
>>> +   mgn_irq_data = kcalloc(nvec, sizeof(*mgn_irq_data), GFP_KERNEL);
>>> +   if (!mgn_irq_data)
>>> +   return -ENOMEM;
>>
>> Hm. Do you need error path here instead of simple return -ENOMEM?
>> Maybe 'goto out_free_dev' will work for you.
> Right. Memory leak happened.
>>
>>> +   mgn_dev->mgn_data = mgn_irq_data;
>>> +
>>> +   for_each_msi_entry(desc, _dev->dev) {
>>> +   mbigen_set_irq_handler_data(desc, mgn_dev,
>>> +   
>>> _irq_data[desc->platform.msi_index]);
>>> +   irq_set_chained_handler(desc->irq, 
>>> mbigen_handle_cascade_irq);
>>> +   }
>>> +
>>> +   raw_spin_lock(>lock);
>>> +   list_add(_dev->entry, >mbigen_device_list);
>>> +   raw_spin_unlock(>lock);
>>> +
>>> +   return 0;
>>> +
>>> +out_free_dev:
>>> +   kfree(mgn_dev);
>>> +   pr_err("failed to get MSIs for device:%s (%d)\n", node->full_name,
>>> +   ret);
>>> +   return ret;
>>> +}
>>> +
>>> +/*
>>> + * Early initialization as an interrupt controller
>>> + */
>>> +static int __init mbigen_of_init(struct device_node *node)
>>> +{
>>> +   struct mbigen_chip *mgn_chip;
>>> +   struct device_node *child;
>>> +   struct irq_domain *domain;
>>> +   void __iomem *base;
>>> +   int err;
>>> +
>>> +   base = of_iomap(node, 0);
>>> +   if (!base) {
>>> +   pr_err("%s: unable to map registers\n", node->full_name);
>>> +   return -ENOMEM;
>>> +   }
>>> +
>>> +   mgn_chip = kzalloc(sizeof(*mgn_chip), GFP_KERNEL);
>>> +   if (!mgn_chip) {
>>> +   err = -ENOMEM;
>>> +   goto unmap_reg;
>>> +   }
>>> +
>>> +   mgn_chip->base = base;
>>> +   mgn_chip->node = node;
>>> +
>>> +   domain = irq_domain_add_tree(node, _domain_ops, mgn_chip);
>>> +   mgn_chip->domain = domain;
>>> +
>>> +   raw_spin_lock_init(_chip->lock);
>>> +   INIT_LIST_HEAD(_chip->entry);
>>> +   INIT_LIST_HEAD(_chip->mbigen_device_list);
>>> +
>>> +   for_each_child_of_node(node, child) {
>>> +   mbigen_device_init(mgn_chip, child);
>>
>> You don't check error from mbigen_device_init()
> I don't think we need to check errors here.
> mbigen_device_init() handle all errors.

For my eyes, mbigen_device_init() is static and used only once here.
Here you don't check return value from it. If this is correct you can
convert mbigen_device_init() to return void unless you have future
plans to modify

Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-08-31 Thread majun (F) 
Hi Alexey:

在 2015/8/29 11:13, Alexey Klimov 写道:
> Hi Ma Jun,
> 
> On Wed, Aug 19, 2015 at 5:55 AM, MaJun  wrote:
>> From: Ma Jun 
>>
>> Mbigen means Message Based Interrupt Generator(MBIGEN).
>>
>> Its a kind of interrupt controller that collects
>>
>> the interrupts from external devices and generate msi interrupt.
>>


>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
> 
> What do you think about sorting this?
ok
> 
> 
>> +#include "irqchip.h"
>> +
[...]
>> +*/
>> +static u32 calc_irq_index(struct mbigen_device *dev, u32 nid, u32 offset)
>> +{
>> +   struct mbigen_node *mgn_node = NULL, *tmp;
>> +   unsigned long flags;
>> +   u32 index = 0;
>> +
>> +   raw_spin_lock_irqsave(>lock, flags);
>> +   list_for_each_entry(tmp, >mbigen_node_list, entry) {
>> +   if (tmp->node_num == nid)
>> +   mgn_node = tmp;
>> +   }
>> +   raw_spin_unlock_irqrestore(>lock, flags);
>> +
>> +   if (mgn_node == NULL) {
>> +   pr_err("No mbigen node found in device:%s\n",
>> +dev->node->full_name);
>> +   return -ENXIO;
>> +   }
>> +
>> +   if ((offset <= (mgn_node->pin_offset + mgn_node->irq_nr))
>> +   && (offset >= mgn_node->pin_offset))
>> +   index = mgn_node->index_offset + (offset - 
>> mgn_node->pin_offset);
>> +   else {
>> +   pr_err("Err: no invalid index\n");
> 
> Please check this message.
> 1. I don't know all details about this driver but is it really correct
> "no invalid index"? Maybe you mean "no vaild index" or just "invalid
> index"?
> Just checking if i correctly understand this.
> 
You are right.  This should be "no valid index"
> 2. Imagine what info user/dmesg reader gets when she or he will see
> such message? I suggest to add some info about driver that printed
> this message.
> You already have nice name in mbigen_irq_chip: "MBIGEN-v2". What do
> you think about using it as prefix in your printk-based messages?
> Please also consider revisiting other messages in this patch.
> 
good suggestion.
> 
>> +   index = -EINVAL;
>> +   }
>> +
>> +   return index;
>> +}
[...]
>> +   INIT_LIST_HEAD(dev_to_msi_list(_dev->dev));
>> +
>> +   ret = platform_msi_domain_alloc_irqs(_dev->dev,
>> +nvec, mbigen_write_msg);
>> +   if (ret)
>> +   goto out_free_dev;
>> +
>> +   INIT_LIST_HEAD(_dev->entry);
>> +   raw_spin_lock_init(_dev->lock);
>> +   INIT_LIST_HEAD(_dev->mbigen_node_list);
>> +
>> +   /* Parse and get the info of mbigen nodes this
>> +* device connected
>> +*/
>> +   parse_mbigen_node(mgn_dev);
>> +
>> +   mgn_irq_data = kcalloc(nvec, sizeof(*mgn_irq_data), GFP_KERNEL);
>> +   if (!mgn_irq_data)
>> +   return -ENOMEM;
> 
> Hm. Do you need error path here instead of simple return -ENOMEM?
> Maybe 'goto out_free_dev' will work for you.
Right. Memory leak happened.
> 
>> +   mgn_dev->mgn_data = mgn_irq_data;
>> +
>> +   for_each_msi_entry(desc, _dev->dev) {
>> +   mbigen_set_irq_handler_data(desc, mgn_dev,
>> +   
>> _irq_data[desc->platform.msi_index]);
>> +   irq_set_chained_handler(desc->irq, 
>> mbigen_handle_cascade_irq);
>> +   }
>> +
>> +   raw_spin_lock(>lock);
>> +   list_add(_dev->entry, >mbigen_device_list);
>> +   raw_spin_unlock(>lock);
>> +
>> +   return 0;
>> +
>> +out_free_dev:
>> +   kfree(mgn_dev);
>> +   pr_err("failed to get MSIs for device:%s (%d)\n", node->full_name,
>> +   ret);
>> +   return ret;
>> +}
>> +
>> +/*
>> + * Early initialization as an interrupt controller
>> + */
>> +static int __init mbigen_of_init(struct device_node *node)
>> +{
>> +   struct mbigen_chip *mgn_chip;
>> +   struct device_node *child;
>> +   struct irq_domain *domain;
>> +   void __iomem *base;
>> +   int err;
>> +
>> +   base = of_iomap(node, 0);
>> +   if (!base) {
>> +   pr_err("%s: unable to map registers\n", node->full_name);
>> +   return -ENOMEM;
>> +   }
>> +
>> +   mgn_chip = kzalloc(sizeof(*mgn_chip), GFP_KERNEL);
>> +   if (!mgn_chip) {
>> +   err = -ENOMEM;
>> +   goto unmap_reg;
>> +   }
>> +
>> +   mgn_chip->base = base;
>> +   mgn_chip->node = node;
>> +
>> +   domain = irq_domain_add_tree(node, _domain_ops, mgn_chip);
>> +   mgn_chip->domain = domain;
>> +
>> +   raw_spin_lock_init(_chip->lock);
>> +   INIT_LIST_HEAD(_chip->entry);
>> +   INIT_LIST_HEAD(_chip->mbigen_device_list);
>> +
>> +   for_each_child_of_node(node, child) {
>> +   mbigen_device_init(mgn_chip, child);
> 
> You don't check error from mbigen_device_init()
I

Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-08-31 Thread majun (F) 
Hi Alexey:

在 2015/8/29 11:13, Alexey Klimov 写道:
> Hi Ma Jun,
> 
> On Wed, Aug 19, 2015 at 5:55 AM, MaJun  wrote:
>> From: Ma Jun 
>>
>> Mbigen means Message Based Interrupt Generator(MBIGEN).
>>
>> Its a kind of interrupt controller that collects
>>
>> the interrupts from external devices and generate msi interrupt.
>>


>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
> 
> What do you think about sorting this?
ok
> 
> 
>> +#include "irqchip.h"
>> +
[...]
>> +*/
>> +static u32 calc_irq_index(struct mbigen_device *dev, u32 nid, u32 offset)
>> +{
>> +   struct mbigen_node *mgn_node = NULL, *tmp;
>> +   unsigned long flags;
>> +   u32 index = 0;
>> +
>> +   raw_spin_lock_irqsave(>lock, flags);
>> +   list_for_each_entry(tmp, >mbigen_node_list, entry) {
>> +   if (tmp->node_num == nid)
>> +   mgn_node = tmp;
>> +   }
>> +   raw_spin_unlock_irqrestore(>lock, flags);
>> +
>> +   if (mgn_node == NULL) {
>> +   pr_err("No mbigen node found in device:%s\n",
>> +dev->node->full_name);
>> +   return -ENXIO;
>> +   }
>> +
>> +   if ((offset <= (mgn_node->pin_offset + mgn_node->irq_nr))
>> +   && (offset >= mgn_node->pin_offset))
>> +   index = mgn_node->index_offset + (offset - 
>> mgn_node->pin_offset);
>> +   else {
>> +   pr_err("Err: no invalid index\n");
> 
> Please check this message.
> 1. I don't know all details about this driver but is it really correct
> "no invalid index"? Maybe you mean "no vaild index" or just "invalid
> index"?
> Just checking if i correctly understand this.
> 
You are right.  This should be "no valid index"
> 2. Imagine what info user/dmesg reader gets when she or he will see
> such message? I suggest to add some info about driver that printed
> this message.
> You already have nice name in mbigen_irq_chip: "MBIGEN-v2". What do
> you think about using it as prefix in your printk-based messages?
> Please also consider revisiting other messages in this patch.
> 
good suggestion.
> 
>> +   index = -EINVAL;
>> +   }
>> +
>> +   return index;
>> +}
[...]
>> +   INIT_LIST_HEAD(dev_to_msi_list(_dev->dev));
>> +
>> +   ret = platform_msi_domain_alloc_irqs(_dev->dev,
>> +nvec, mbigen_write_msg);
>> +   if (ret)
>> +   goto out_free_dev;
>> +
>> +   INIT_LIST_HEAD(_dev->entry);
>> +   raw_spin_lock_init(_dev->lock);
>> +   INIT_LIST_HEAD(_dev->mbigen_node_list);
>> +
>> +   /* Parse and get the info of mbigen nodes this
>> +* device connected
>> +*/
>> +   parse_mbigen_node(mgn_dev);
>> +
>> +   mgn_irq_data = kcalloc(nvec, sizeof(*mgn_irq_data), GFP_KERNEL);
>> +   if (!mgn_irq_data)
>> +   return -ENOMEM;
> 
> Hm. Do you need error path here instead of simple return -ENOMEM?
> Maybe 'goto out_free_dev' will work for you.
Right. Memory leak happened.
> 
>> +   mgn_dev->mgn_data = mgn_irq_data;
>> +
>> +   for_each_msi_entry(desc, _dev->dev) {
>> +   mbigen_set_irq_handler_data(desc, mgn_dev,
>> +   
>> _irq_data[desc->platform.msi_index]);
>> +   irq_set_chained_handler(desc->irq, 
>> mbigen_handle_cascade_irq);
>> +   }
>> +
>> +   raw_spin_lock(>lock);
>> +   list_add(_dev->entry, >mbigen_device_list);
>> +   raw_spin_unlock(>lock);
>> +
>> +   return 0;
>> +
>> +out_free_dev:
>> +   kfree(mgn_dev);
>> +   pr_err("failed to get MSIs for device:%s (%d)\n", node->full_name,
>> +   ret);
>> +   return ret;
>> +}
>> +
>> +/*
>> + * Early initialization as an interrupt controller
>> + */
>> +static int __init mbigen_of_init(struct device_node *node)
>> +{
>> +   struct mbigen_chip *mgn_chip;
>> +   struct device_node *child;
>> +   struct irq_domain *domain;
>> +   void __iomem *base;
>> +   int err;
>> +
>> +   base = of_iomap(node, 0);
>> +   if (!base) {
>> +   pr_err("%s: unable to map registers\n", node->full_name);
>> +   return -ENOMEM;
>> +   }
>> +
>> +   mgn_chip = kzalloc(sizeof(*mgn_chip), GFP_KERNEL);
>> +   if (!mgn_chip) {
>> +   err = -ENOMEM;
>> +   goto unmap_reg;
>> +   }
>> +
>> +   mgn_chip->base = base;
>> +   mgn_chip->node = node;
>> +
>> +   domain = irq_domain_add_tree(node, _domain_ops, mgn_chip);
>> +   mgn_chip->domain = domain;
>> +
>> +   raw_spin_lock_init(_chip->lock);
>> +   INIT_LIST_HEAD(_chip->entry);
>> +   INIT_LIST_HEAD(_chip->mbigen_device_list);
>> +
>> +   for_each_child_of_node(node, child) {
>> +   mbigen_device_init(mgn_chip, child);
> 
> You

Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-08-28 Thread Alexey Klimov 
Hi Ma Jun,

On Wed, Aug 19, 2015 at 5:55 AM, MaJun  wrote:
> From: Ma Jun 
>
> Mbigen means Message Based Interrupt Generator(MBIGEN).
>
> Its a kind of interrupt controller that collects
>
> the interrupts from external devices and generate msi interrupt.
>
> Mbigen is applied to reduce the number of wire connected interrupts.
>
> As the peripherals increasing, the interrupts lines needed is
> increasing much, especially on the Arm64 server soc.
>
> Therefore, the interrupt pin in gic is not enough to cover so
> many peripherals.
>
> Mbigen is designed to fix this problem.
>
> Mbigen chip locates in ITS or outside of ITS.
>
> Mbigen chip hardware structure shows as below:
>
> mbigen chip
> |-|---|
> mgn_node0 mgn_node1 mgn_node2
>  |   |---|  |---|--|
> dev1dev1dev2dev1   dev3   dev4
>
> Each mbigen chip contains several mbigen nodes.
>
> External devices can connects to mbigen node through wire connecting way.

s/connects/connect

>
> Because a mbigen node only can support 128 interrupt maximum, depends
> on the interrupt lines number of devices, a device can connects to one
> more mbigen nodes.
>
> Also, several different devices can connect to a same mbigen node.
>
> When devices triggered interrupt, mbigen chip detects and collects
> the interrupts and generates the MBI interrupts by writing the ITS
> Translator register.
>
>
> Signed-off-by: Ma Jun 
> ---
>  drivers/irqchip/Kconfig  |8 +
>  drivers/irqchip/Makefile |1 +
>  drivers/irqchip/irq-mbigen.c |  732 
> ++
>  3 files changed, 741 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/irqchip/irq-mbigen.c
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 120d815..356507f 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -27,6 +27,14 @@ config ARM_GIC_V3_ITS
> bool
> select PCI_MSI_IRQ_DOMAIN
>
> +config HISILICON_IRQ_MBIGEN
> +   bool "Support mbigen interrupt controller"
> +   default n
> +   depends on ARM_GIC_V3 && ARM_GIC_V3_ITS
> +   help
> +Enable the mbigen interrupt controller used on
> +Hisilicon platform.
> +
>  config ARM_NVIC
> bool
> select IRQ_DOMAIN
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 11d08c9..c6f3d66 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_ARM_GIC) += irq-gic.o 
> irq-gic-common.o
>  obj-$(CONFIG_ARM_GIC_V2M)  += irq-gic-v2m.o
>  obj-$(CONFIG_ARM_GIC_V3)   += irq-gic-v3.o irq-gic-common.o
>  obj-$(CONFIG_ARM_GIC_V3_ITS)   += irq-gic-v3-its.o 
> irq-gic-v3-its-pci-msi.o irq-gic-v3-its-platform-msi.o
> +obj-$(CONFIG_HISILICON_IRQ_MBIGEN) += irq-mbigen.o
>  obj-$(CONFIG_ARM_NVIC) += irq-nvic.o
>  obj-$(CONFIG_ARM_VIC)  += irq-vic.o
>  obj-$(CONFIG_ATMEL_AIC_IRQ)+= irq-atmel-aic-common.o 
> irq-atmel-aic.o
> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
> new file mode 100644
> index 000..4bbbd76
> --- /dev/null
> +++ b/drivers/irqchip/irq-mbigen.c
> @@ -0,0 +1,732 @@
> +/*
> + * Copyright (C) 2014 Hisilicon Limited, All Rights Reserved.

maybe 2014-2015 or 2015?

> + * Author: Jun Ma 
> + * Author: Yun Wu 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

What do you think about sorting this?


> +#include "irqchip.h"
> +
> +#defineMBIGEN_NODE_SHIFT   (8)
> +#define MBIGEN_DEV_SHIFT   (12)
> +
> +/*
> + * To avoid the duplicate hwirq number problem
> + * we use device id, mbigen node number and interrupt
> + * pin offset to generate a new hwirq number in mbigen
> + * domain.
> + *
> + * hwirq[32:12]: did. device id
> + * hwirq[11:8]: nid. mbigen node number
> + * hwirq[7:0]: pin. hardware pin offset of this interrupt
> + */
> +#defineCOMPOSE_MBIGEN_HWIRQ(did, nid, pin) \
> +   (((did) << MBIGEN_DEV_SHIFT) | \
> +   ((nid) << MBIGEN_NODE_SHIFT) | (pin))
> +
> +/* get the interrupt pin offset from mbigen hwirq */
> +#define

Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-08-28 Thread Alexey Klimov 
Hi Ma Jun,

On Wed, Aug 19, 2015 at 5:55 AM, MaJun majun...@huawei.com wrote:
 From: Ma Jun majun...@huawei.com

 Mbigen means Message Based Interrupt Generator(MBIGEN).

 Its a kind of interrupt controller that collects

 the interrupts from external devices and generate msi interrupt.

 Mbigen is applied to reduce the number of wire connected interrupts.

 As the peripherals increasing, the interrupts lines needed is
 increasing much, especially on the Arm64 server soc.

 Therefore, the interrupt pin in gic is not enough to cover so
 many peripherals.

 Mbigen is designed to fix this problem.

 Mbigen chip locates in ITS or outside of ITS.

 Mbigen chip hardware structure shows as below:

 mbigen chip
 |-|---|
 mgn_node0 mgn_node1 mgn_node2
  |   |---|  |---|--|
 dev1dev1dev2dev1   dev3   dev4

 Each mbigen chip contains several mbigen nodes.

 External devices can connects to mbigen node through wire connecting way.

s/connects/connect


 Because a mbigen node only can support 128 interrupt maximum, depends
 on the interrupt lines number of devices, a device can connects to one
 more mbigen nodes.

 Also, several different devices can connect to a same mbigen node.

 When devices triggered interrupt, mbigen chip detects and collects
 the interrupts and generates the MBI interrupts by writing the ITS
 Translator register.


 Signed-off-by: Ma Jun majun...@huawei.com
 ---
  drivers/irqchip/Kconfig  |8 +
  drivers/irqchip/Makefile |1 +
  drivers/irqchip/irq-mbigen.c |  732 
 ++
  3 files changed, 741 insertions(+), 0 deletions(-)
  create mode 100644 drivers/irqchip/irq-mbigen.c

 diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
 index 120d815..356507f 100644
 --- a/drivers/irqchip/Kconfig
 +++ b/drivers/irqchip/Kconfig
 @@ -27,6 +27,14 @@ config ARM_GIC_V3_ITS
 bool
 select PCI_MSI_IRQ_DOMAIN

 +config HISILICON_IRQ_MBIGEN
 +   bool Support mbigen interrupt controller
 +   default n
 +   depends on ARM_GIC_V3  ARM_GIC_V3_ITS
 +   help
 +Enable the mbigen interrupt controller used on
 +Hisilicon platform.
 +
  config ARM_NVIC
 bool
 select IRQ_DOMAIN
 diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
 index 11d08c9..c6f3d66 100644
 --- a/drivers/irqchip/Makefile
 +++ b/drivers/irqchip/Makefile
 @@ -23,6 +23,7 @@ obj-$(CONFIG_ARM_GIC) += irq-gic.o 
 irq-gic-common.o
  obj-$(CONFIG_ARM_GIC_V2M)  += irq-gic-v2m.o
  obj-$(CONFIG_ARM_GIC_V3)   += irq-gic-v3.o irq-gic-common.o
  obj-$(CONFIG_ARM_GIC_V3_ITS)   += irq-gic-v3-its.o 
 irq-gic-v3-its-pci-msi.o irq-gic-v3-its-platform-msi.o
 +obj-$(CONFIG_HISILICON_IRQ_MBIGEN) += irq-mbigen.o
  obj-$(CONFIG_ARM_NVIC) += irq-nvic.o
  obj-$(CONFIG_ARM_VIC)  += irq-vic.o
  obj-$(CONFIG_ATMEL_AIC_IRQ)+= irq-atmel-aic-common.o 
 irq-atmel-aic.o
 diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
 new file mode 100644
 index 000..4bbbd76
 --- /dev/null
 +++ b/drivers/irqchip/irq-mbigen.c
 @@ -0,0 +1,732 @@
 +/*
 + * Copyright (C) 2014 Hisilicon Limited, All Rights Reserved.

maybe 2014-2015 or 2015?

 + * Author: Jun Ma majun...@huawei.com
 + * Author: Yun Wu wuyun...@huawei.com
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License version 2 as
 + * published by the Free Software Foundation.
 + *
 + * 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.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program.  If not, see http://www.gnu.org/licenses/.
 + */
 +
 +#include linux/init.h
 +#include linux/io.h
 +#include linux/slab.h
 +#include linux/interrupt.h
 +#include linux/irqchip/chained_irq.h
 +#include linux/of_address.h
 +#include linux/of_irq.h
 +#include linux/of_platform.h
 +#include linux/kernel.h
 +#include linux/platform_device.h
 +#include linux/module.h
 +#include linux/msi.h

What do you think about sorting this?


 +#include irqchip.h
 +
 +#defineMBIGEN_NODE_SHIFT   (8)
 +#define MBIGEN_DEV_SHIFT   (12)
 +
 +/*
 + * To avoid the duplicate hwirq number problem
 + * we use device id, mbigen node number and interrupt
 + * pin offset to generate a new hwirq number in mbigen
 + * domain.
 + *
 + * hwirq[32:12]: did. device id
 + * hwirq[11:8]: nid. mbigen node number
 + * hwirq[7:0]: pin. hardware pin offset of this interrupt
 + */
 +#defineCOMPOSE_MBIGEN_HWIRQ(did, nid, pin) \
 +   (((did)

[PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-08-18 Thread MaJun 
From: Ma Jun 

Mbigen means Message Based Interrupt Generator(MBIGEN).

Its a kind of interrupt controller that collects

the interrupts from external devices and generate msi interrupt.

Mbigen is applied to reduce the number of wire connected interrupts.

As the peripherals increasing, the interrupts lines needed is 
increasing much, especially on the Arm64 server soc.

Therefore, the interrupt pin in gic is not enough to cover so
many peripherals.

Mbigen is designed to fix this problem.

Mbigen chip locates in ITS or outside of ITS.

Mbigen chip hardware structure shows as below:

mbigen chip
|-|---|
mgn_node0 mgn_node1 mgn_node2
 |   |---|  |---|--|
dev1dev1dev2dev1   dev3   dev4

Each mbigen chip contains several mbigen nodes.

External devices can connects to mbigen node through wire connecting way.

Because a mbigen node only can support 128 interrupt maximum, depends
on the interrupt lines number of devices, a device can connects to one
more mbigen nodes.

Also, several different devices can connect to a same mbigen node.

When devices triggered interrupt,mbigen chip detects and collects 
the interrupts and generates the MBI interrupts by writing the ITS
Translator register.


Signed-off-by: Ma Jun 
---
 drivers/irqchip/Kconfig  |8 +
 drivers/irqchip/Makefile |1 +
 drivers/irqchip/irq-mbigen.c |  732 ++
 3 files changed, 741 insertions(+), 0 deletions(-)
 create mode 100644 drivers/irqchip/irq-mbigen.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 120d815..356507f 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -27,6 +27,14 @@ config ARM_GIC_V3_ITS
bool
select PCI_MSI_IRQ_DOMAIN
 
+config HISILICON_IRQ_MBIGEN
+   bool "Support mbigen interrupt controller"
+   default n
+   depends on ARM_GIC_V3 && ARM_GIC_V3_ITS
+   help
+Enable the mbigen interrupt controller used on
+Hisilicon platform.
+
 config ARM_NVIC
bool
select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 11d08c9..c6f3d66 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_ARM_GIC) += irq-gic.o 
irq-gic-common.o
 obj-$(CONFIG_ARM_GIC_V2M)  += irq-gic-v2m.o
 obj-$(CONFIG_ARM_GIC_V3)   += irq-gic-v3.o irq-gic-common.o
 obj-$(CONFIG_ARM_GIC_V3_ITS)   += irq-gic-v3-its.o 
irq-gic-v3-its-pci-msi.o irq-gic-v3-its-platform-msi.o
+obj-$(CONFIG_HISILICON_IRQ_MBIGEN) += irq-mbigen.o
 obj-$(CONFIG_ARM_NVIC) += irq-nvic.o
 obj-$(CONFIG_ARM_VIC)  += irq-vic.o
 obj-$(CONFIG_ATMEL_AIC_IRQ)+= irq-atmel-aic-common.o 
irq-atmel-aic.o
diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
new file mode 100644
index 000..4bbbd76
--- /dev/null
+++ b/drivers/irqchip/irq-mbigen.c
@@ -0,0 +1,732 @@
+/*
+ * Copyright (C) 2014 Hisilicon Limited, All Rights Reserved.
+ * Author: Jun Ma 
+ * Author: Yun Wu 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "irqchip.h"
+
+#defineMBIGEN_NODE_SHIFT   (8)
+#define MBIGEN_DEV_SHIFT   (12)
+
+/*
+ * To avoid the duplicate hwirq number problem
+ * we use device id, mbigen node number and interrupt
+ * pin offset to generate a new hwirq number in mbigen
+ * domain.
+ *
+ * hwirq[32:12]: did. device id
+ * hwirq[11:8]: nid. mbigen node number
+ * hwirq[7:0]: pin. hardware pin offset of this interrupt
+ */
+#defineCOMPOSE_MBIGEN_HWIRQ(did, nid, pin) \
+   (((did) << MBIGEN_DEV_SHIFT) | \
+   ((nid) << MBIGEN_NODE_SHIFT) | (pin))
+
+/* get the interrupt pin offset from mbigen hwirq */
+#defineGET_IRQ_PIN_OFFSET(hwirq)   ((hwirq) & 0xff)
+/* get the mbigen node number from mbigen hwirq */
+#define GET_MBIGEN_NODE_NUM(hwirq) (((hwirq) >> MBIGEN_NODE_SHIFT) & 0xf)
+/* get the mbigen device id from mbigen hwirq */
+#define GET_MBIGEN_DEVICE_ID(hwirq)\
+   (((hwirq) >> MBIGEN_DEV_SHIFT) & 0xf)
+
+/*
+ * In mbigen vector register
+ * bit[21:12]: event id value
+

[PATCH v4 1/2] Add the driver of mbigen interrupt controller

2015-08-18 Thread MaJun 
From: Ma Jun majun...@huawei.com

Mbigen means Message Based Interrupt Generator(MBIGEN).

Its a kind of interrupt controller that collects

the interrupts from external devices and generate msi interrupt.

Mbigen is applied to reduce the number of wire connected interrupts.

As the peripherals increasing, the interrupts lines needed is 
increasing much, especially on the Arm64 server soc.

Therefore, the interrupt pin in gic is not enough to cover so
many peripherals.

Mbigen is designed to fix this problem.

Mbigen chip locates in ITS or outside of ITS.

Mbigen chip hardware structure shows as below:

mbigen chip
|-|---|
mgn_node0 mgn_node1 mgn_node2
 |   |---|  |---|--|
dev1dev1dev2dev1   dev3   dev4

Each mbigen chip contains several mbigen nodes.

External devices can connects to mbigen node through wire connecting way.

Because a mbigen node only can support 128 interrupt maximum, depends
on the interrupt lines number of devices, a device can connects to one
more mbigen nodes.

Also, several different devices can connect to a same mbigen node.

When devices triggered interrupt,mbigen chip detects and collects 
the interrupts and generates the MBI interrupts by writing the ITS
Translator register.


Signed-off-by: Ma Jun majun...@huawei.com
---
 drivers/irqchip/Kconfig  |8 +
 drivers/irqchip/Makefile |1 +
 drivers/irqchip/irq-mbigen.c |  732 ++
 3 files changed, 741 insertions(+), 0 deletions(-)
 create mode 100644 drivers/irqchip/irq-mbigen.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 120d815..356507f 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -27,6 +27,14 @@ config ARM_GIC_V3_ITS
bool
select PCI_MSI_IRQ_DOMAIN
 
+config HISILICON_IRQ_MBIGEN
+   bool Support mbigen interrupt controller
+   default n
+   depends on ARM_GIC_V3  ARM_GIC_V3_ITS
+   help
+Enable the mbigen interrupt controller used on
+Hisilicon platform.
+
 config ARM_NVIC
bool
select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 11d08c9..c6f3d66 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_ARM_GIC) += irq-gic.o 
irq-gic-common.o
 obj-$(CONFIG_ARM_GIC_V2M)  += irq-gic-v2m.o
 obj-$(CONFIG_ARM_GIC_V3)   += irq-gic-v3.o irq-gic-common.o
 obj-$(CONFIG_ARM_GIC_V3_ITS)   += irq-gic-v3-its.o 
irq-gic-v3-its-pci-msi.o irq-gic-v3-its-platform-msi.o
+obj-$(CONFIG_HISILICON_IRQ_MBIGEN) += irq-mbigen.o
 obj-$(CONFIG_ARM_NVIC) += irq-nvic.o
 obj-$(CONFIG_ARM_VIC)  += irq-vic.o
 obj-$(CONFIG_ATMEL_AIC_IRQ)+= irq-atmel-aic-common.o 
irq-atmel-aic.o
diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
new file mode 100644
index 000..4bbbd76
--- /dev/null
+++ b/drivers/irqchip/irq-mbigen.c
@@ -0,0 +1,732 @@
+/*
+ * Copyright (C) 2014 Hisilicon Limited, All Rights Reserved.
+ * Author: Jun Ma majun...@huawei.com
+ * Author: Yun Wu wuyun...@huawei.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/.
+ */
+
+#include linux/init.h
+#include linux/io.h
+#include linux/slab.h
+#include linux/interrupt.h
+#include linux/irqchip/chained_irq.h
+#include linux/of_address.h
+#include linux/of_irq.h
+#include linux/of_platform.h
+#include linux/kernel.h
+#include linux/platform_device.h
+#include linux/module.h
+#include linux/msi.h
+#include irqchip.h
+
+#defineMBIGEN_NODE_SHIFT   (8)
+#define MBIGEN_DEV_SHIFT   (12)
+
+/*
+ * To avoid the duplicate hwirq number problem
+ * we use device id, mbigen node number and interrupt
+ * pin offset to generate a new hwirq number in mbigen
+ * domain.
+ *
+ * hwirq[32:12]: did. device id
+ * hwirq[11:8]: nid. mbigen node number
+ * hwirq[7:0]: pin. hardware pin offset of this interrupt
+ */
+#defineCOMPOSE_MBIGEN_HWIRQ(did, nid, pin) \
+   (((did)  MBIGEN_DEV_SHIFT) | \
+   ((nid)  MBIGEN_NODE_SHIFT) | (pin))
+
+/* get the interrupt pin offset from mbigen hwirq */
+#defineGET_IRQ_PIN_OFFSET(hwirq)   ((hwirq)  0xff)
+/* get the mbigen node number from mbigen hwirq */
+#define GET_MBIGEN_NODE_NUM(hwirq)