Re: [PATCH net-next v3 13/16] net: airoha: Introduce PPE initialization via NPU

[Krzysztof Kozlowski <krzk@kernel.org>]

On 11/02/2025 17:31, Lorenzo Bianconi wrote:
>> On Sun, Feb 09, 2025 at 01:09:06PM +0100, Lorenzo Bianconi wrote:
>>> +static irqreturn_t airoha_npu_wdt_handler(int irq, void *core_instance)
>>> +{
>>> +	struct airoha_npu_core *core = core_instance;
>>> +	struct airoha_npu *npu = core->npu;
>>> +	int c = core - &npu->cores[0];
>>> +	u32 val;
>>> +
>>> +	airoha_npu_rmw(npu, REG_WDT_TIMER_CTRL(c), 0, WDT_INTR_MASK);
>>> +	val = airoha_npu_rr(npu, REG_WDT_TIMER_CTRL(c));
>>> +	if (FIELD_GET(WDT_EN_MASK, val))
>>> +		schedule_work(&core->wdt_work);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +struct airoha_npu *airoha_npu_init(struct airoha_eth *eth)
>>> +{
>>> +	struct reserved_mem *rmem;
>>> +	int i, irq, err = -ENODEV;
>>> +	struct airoha_npu *npu;
>>> +	struct device_node *np;
>>> +
>>> +	npu = devm_kzalloc(eth->dev, sizeof(*npu), GFP_KERNEL);
>>> +	if (!npu)
>>> +		return ERR_PTR(-ENOMEM);
>>> +
>>> +	npu->np = of_parse_phandle(eth->dev->of_node, "airoha,npu", 0);
>>> +	if (!npu->np)
>>> +		return ERR_PTR(-ENODEV);
>>
>> Why? The property is not required, so how can missing property fail the
>> probe?
> 
> similar to mtk_wed device, airoha_npu is not modeled as a standalone driver,
> but it is part of the airoha_eth driver. If you think it is better, I can
> rework it implementing a dedicated driver for it. What do you think?


Whether it is separate or not, does not matter. Behavior in both cases
would be the same so does not answer my question. But below does however:

> 
>>
>> This is also still unnecessary ABI break without explanation/reasoning.
> 
> At the moment if airoha_npu_init() fails (e.g. if the npu node is not present),
> it will not cause any failure in airoha_hw_init() (so in the core ethernet
> driver probing).


Indeed, it will fail airoha_ppe_init() but airoha_ppe_init() is not
fatal. You will have dmesg errors though, so this should be probably
dev_warn.

> 
>>
>>> +
>>> +	npu->pdev = of_find_device_by_node(npu->np);
>>> +	if (!npu->pdev)
>>> +		goto error_of_node_put;
>>
>> You should also add device link and probably try_module_get. See
>> qcom,ice (patch for missing try_module_get is on the lists).
> 
> thx for the pointer, I will take a look to it.
> 
>>
>>> +
>>> +	get_device(&npu->pdev->dev);
>>
>> Why? of_find_device_by_node() does it.
> 
> ack, I will fix it.
> 
>>
>>> +
>>> +	npu->base = devm_platform_ioremap_resource(npu->pdev, 0);
>>> +	if (IS_ERR(npu->base))
>>> +		goto error_put_dev;
>>> +
>>> +	np = of_parse_phandle(npu->np, "memory-region", 0);
>>> +	if (!np)
>>> +		goto error_put_dev;
>>> +
>>> +	rmem = of_reserved_mem_lookup(np);
>>> +	of_node_put(np);
>>> +
>>> +	if (!rmem)
>>> +		goto error_put_dev;
>>> +
>>> +	irq = platform_get_irq(npu->pdev, 0);
>>> +	if (irq < 0) {
>>> +		err = irq;
>>> +		goto error_put_dev;
>>> +	}
>>> +
>>> +	err = devm_request_irq(&npu->pdev->dev, irq, airoha_npu_mbox_handler,
>>> +			       IRQF_SHARED, "airoha-npu-mbox", npu);
>>> +	if (err)
>>> +		goto error_put_dev;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(npu->cores); i++) {
>>> +		struct airoha_npu_core *core = &npu->cores[i];
>>> +
>>> +		spin_lock_init(&core->lock);
>>> +		core->npu = npu;
>>> +
>>> +		irq = platform_get_irq(npu->pdev, i + 1);
>>> +		if (irq < 0) {
>>> +			err = irq;
>>> +			goto error_put_dev;
>>> +		}
>>
>> This is all confusing. Why are you requesting IRQs for other - the npu -
>> device? That device driver is responsible for its interrupts, not you
>> here. This breaks encapsulation. And what do you do if the other device
>> starts handling interrupts on its own? This is really unexpected to see
>> here.
> 
> As pointed out above, there is no other driver for airoha_npu at the moment,
> but I am fine to implement it.

I see. The second driver is there - syscon - just provided by MFD core.
It also looks like the NPU is some sort of mailbox, so maybe NPU should
not have been syscon in the first place, but mailbox?



Best regards,
Krzysztof