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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5292 bytes --]

> 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?

> 
> 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).

> 
> > +
> > +	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.

Regards,
Lorenzo

> 
> > +
> > +		err = devm_request_irq(&npu->pdev->dev, irq,
> > +				       airoha_npu_wdt_handler, IRQF_SHARED,
> > +				       "airoha-npu-wdt", core);
> > +		if (err)
> > +			goto error_put_dev;
> > +
> > +		INIT_WORK(&core->wdt_work, airoha_npu_wdt_work);
> > +	}
> > +
> > +	if (dma_set_coherent_mask(&npu->pdev->dev, 0xbfffffff))
> > +		dev_err(&npu->pdev->dev,
> > +			"failed coherent DMA configuration\n");
> > +
> > +	err = airoha_npu_run_firmware(npu, rmem);
> > +	if (err)
> > +		goto error_put_dev;
> > +
> > +	airoha_npu_wr(npu, REG_CR_NPU_MIB(10),
> > +		      rmem->base + NPU_EN7581_FIRMWARE_RV32_MAX_SIZE);
> > +	airoha_npu_wr(npu, REG_CR_NPU_MIB(11), 0x40000); /* SRAM 256K */
> > +	airoha_npu_wr(npu, REG_CR_NPU_MIB(12), 0);
> > +	airoha_npu_wr(npu, REG_CR_NPU_MIB(21), 1);
> > +	msleep(100);
> > +
> > +	/* setting booting address */
> > +	for (i = 0; i < AIROHA_NPU_NUM_CORES; i++)
> > +		airoha_npu_wr(npu, REG_CR_BOOT_BASE(i), rmem->base);
> > +	usleep_range(1000, 2000);
> > +
> > +	/* enable NPU cores */
> > +	/* do not start core3 since it is used for WiFi offloading */
> > +	airoha_npu_wr(npu, REG_CR_BOOT_CONFIG, 0xf7);
> > +	airoha_npu_wr(npu, REG_CR_BOOT_TRIGGER, 0x1);
> > +	msleep(100);
> > +
> > +	return npu;
> > +
> > +error_put_dev:
> > +	put_device(&npu->pdev->dev);
> 
> Missing platform_device_put()
> 
> > +error_of_node_put:
> > +	of_node_put(npu->np);
> > +
> > +	return ERR_PTR(err);
> > +}
> > +
> > +void airoha_npu_deinit(struct airoha_npu *npu)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(npu->cores); i++)
> > +		cancel_work_sync(&npu->cores[i].wdt_work);
> > +
> 
> Leaking device put.
> 
> > +	put_device(&npu->pdev->dev);
> > +	of_node_put(npu->np);
> 
> Best regards,
> Krzysztof
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]