Re: [PATCH] irqchip/mbigen: Display message of MBIGEN domain created

[Marc Zyngier]

On 08/04/16 11:33, Kefeng Wang wrote:
> 
> 
> On 2016/4/8 16:53, Marc Zyngier wrote:
>> On Fri, 8 Apr 2016 16:26:21 +0800
>> Kefeng Wang <wangkefeng.wang@xxxxxxxxxx> wrote:
> 
>>>> Overall, this doesn't look like a critical patch to me. I think Ma Jun
>>>> is working on separate series reworking the way the mgigen is getting
>>>> probed, so I'd advise you to work with him in order to integrate this
>>>> patch in his series, as it would make a lot more sense.
>>>
>>> When try to enable hip06 d03 board[1], we met following error log, so I add
>>> some debug message. The mbigen driver use module_platform_driver, the driver
>>> initialization is too late, and it is without any message,  we don't know
>>> about any info of mbigen.  I think we should show something about the mbigen
>>> domain creation at least. What's your option?
>>>
>>> Is there a way to solve this improper printï
>>> -----------
>>> [    1.345945] irq: no irq domain found for /mbigen_pcie@a0080000/intc_usb !
>>> [    1.353660] irq: no irq domain found for /mbigen_pcie@a0080000/intc_usb !
>>
>> How can printing anything solve this issue? Furthermore, the error
>> message you quote is pretty explicit: no mbigen for you, move along.
>>
>> There is a long standing dependency issue for interrupt controllers
>> that are also platform devices, and until you resolve (or help
>> resolving) that issue, you will get that kind of problem. As I
>> mentioned countless times (both on list and in person), you only have
>> two options:
>>
>> - either you defer probing devices behind the mbigen until the mbigen
>>   itself is up and running
>> - or you solve the generic dependency problem.
>>
> Okïgot itïthanks for your explanation.
>> I feel a bit like a stuck record here.
> 
> But there is still one issue in the for_each_child_of_node loop of
> mbigen_device_probe. Assume that there are 3+ child node(mbigen_gmac,
> mbigen_i2c, mbigen_xxx, etc) in mbigen_chip_dsa, if one of them with
> incorrect configuration, then the loop will end, but we still need 
> parse the left child node. we should consider this situation, right?

That's up to whoever designed the driver to decide, really. I don't know
if it is worth continuing in that case (and you are in a better position
than me to find out).

> How about split that part into a new mbigen_create_domain(), shown below, and display
> the result of mbigen domain creation in it.
> 
> 
> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
> index d67baa2..9c1d22e 100644
> --- a/drivers/irqchip/irq-mbigen.c
> +++ b/drivers/irqchip/irq-mbigen.c
> @@ -236,15 +236,41 @@ static struct irq_domain_ops mbigen_domain_ops = {
>  	.free		= irq_domain_free_irqs_common,
>  };
> 
> -static int mbigen_device_probe(struct platform_device *pdev)
> +static void mbigen_create_domain(struct mbigen_device *mgn_dev, struct device_node *np)
>  {
> -	struct mbigen_device *mgn_chip;
> +	struct device *parent = platform_bus_type.dev_root;
> +	struct device *dev = &mgn_dev->pdev->dev;
>  	struct platform_device *child;
>  	struct irq_domain *domain;
> +	u32 num_pins;
> +
> +	if (!of_property_read_bool(np, "interrupt-controller"))
> +		goto err;
> +
> +	if (of_property_read_u32(np, "num-pins", &num_pins))
> +		goto err;
> +
> +	child = of_platform_device_create(np, NULL, parent);
> +	if (IS_ERR(child))
> +		goto err;
> +
> +	domain = platform_msi_create_device_domain(&child->dev, num_pins,
> +						   mbigen_write_msg,
> +						   &mbigen_domain_ops,
> +						   mgn_dev);
> +	if (!domain)
> +		goto err;
> +
> +	dev_info(dev, "%s domain created\n", np->full_name);

You're probably missing a "return" here.

> +err:
> +	dev_err(dev, "unable to create %s domain\n", np->full_name);
> +}
> +
> +static int mbigen_device_probe(struct platform_device *pdev)
> +{
> +	struct mbigen_device *mgn_chip;
>  	struct device_node *np;
> -	struct device *parent;
>  	struct resource *res;
> -	u32 num_pins;
> 
>  	mgn_chip = devm_kzalloc(&pdev->dev, sizeof(*mgn_chip), GFP_KERNEL);
>  	if (!mgn_chip)
> @@ -257,28 +283,8 @@ static int mbigen_device_probe(struct platform_device *pdev)
>  	if (IS_ERR(mgn_chip->base))
>  		return PTR_ERR(mgn_chip->base);
> 
> -	for_each_child_of_node(pdev->dev.of_node, np) {
> -		if (!of_property_read_bool(np, "interrupt-controller"))
> -			continue;
> -
> -		parent = platform_bus_type.dev_root;
> -		child = of_platform_device_create(np, NULL, parent);
> -		if (IS_ERR(child))
> -			return PTR_ERR(child);
> -
> -		if (of_property_read_u32(child->dev.of_node, "num-pins",
> -					 &num_pins) < 0) {
> -			dev_err(&pdev->dev, "No num-pins property\n");
> -			return -EINVAL;
> -		}
> -
> -		domain = platform_msi_create_device_domain(&child->dev, num_pins,
> -							   mbigen_write_msg,
> -							   &mbigen_domain_ops,
> -							   mgn_chip);
> -		if (!domain)
> -			return -ENOMEM;
> -	}
> +	for_each_child_of_node(pdev->dev.of_node, np)
> +		mbigen_create_domain(mgn_chip, np);
> 
>  	platform_set_drvdata(pdev, mgn_chip);

In general, I'd suggest you work with Ma Jun, as he maintains that driver.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...