> 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:
signature.asc
Description: PGP signature
