Hi Jiaxing, On Thu Aug 6, 2026 at 8:34 AM CEST, Jiaxing Hu wrote: > The RK3576 carries the same RKNN block with a different set of clocks and
nit: same RKNN block as what? And if it has a different set of clocks/resets, is it still the same (HW) block? > resets, so the counts cannot stay compile-time constants. Add a soc_data > struct to the of_device_id match data and take the bulk counts from it. > RK3588 keeps four clocks and two resets, so nothing changes for it. > > rocket_core_reset() is switched over as well. It is the same array, and > leaving it on ARRAY_SIZE() would walk entries that were never acquired > once a SoC asks for fewer. but now comes the point I actually want to make: > While moving through this path, take the two register writes in > rocket_job_handle_irq() under job_lock. rocket_job_hw_submit() writes > OPERATION_ENABLE from inside the lock, so a completion handled outside it > can write the zero after that one and stop a task that has just started. This should be its own separate commit (with a Fixes tag) ... which probably doesn't have/should be part of this (RFC) series? > > Signed-off-by: Jiaxing Hu <[email protected]> > --- > drivers/accel/rocket/rocket_core.c | 8 ++++---- > drivers/accel/rocket/rocket_core.h | 9 ++++++++- > drivers/accel/rocket/rocket_drv.c | 12 +++++++++--- > drivers/accel/rocket/rocket_job.c | 12 +++++++++--- > 4 files changed, 30 insertions(+), 11 deletions(-) > > diff --git a/drivers/accel/rocket/rocket_core.c > b/drivers/accel/rocket/rocket_core.c > index 5dd260bac..b202d1581 100644 > --- a/drivers/accel/rocket/rocket_core.c > +++ b/drivers/accel/rocket/rocket_core.c > @@ -23,7 +23,7 @@ int rocket_core_init(struct rocket_core *core) > > core->resets[0].id = "srst_a"; > core->resets[1].id = "srst_h"; > - err = devm_reset_control_bulk_get_exclusive(&pdev->dev, > ARRAY_SIZE(core->resets), > + err = devm_reset_control_bulk_get_exclusive(&pdev->dev, > core->soc->num_resets, > core->resets); > if (err) > return dev_err_probe(dev, err, "failed to get resets for core > %d\n", core->index); > @@ -32,7 +32,7 @@ int rocket_core_init(struct rocket_core *core) > core->clks[1].id = "hclk"; > core->clks[2].id = "npu"; > core->clks[3].id = "pclk"; > - err = devm_clk_bulk_get(dev, ARRAY_SIZE(core->clks), core->clks); > + err = devm_clk_bulk_get(dev, core->soc->num_clks, core->clks); > if (err) > return dev_err_probe(dev, err, "failed to get clocks for core > %d\n", core->index); > > @@ -109,9 +109,9 @@ void rocket_core_fini(struct rocket_core *core) > > void rocket_core_reset(struct rocket_core *core) > { > - reset_control_bulk_assert(ARRAY_SIZE(core->resets), core->resets); > + reset_control_bulk_assert(core->soc->num_resets, core->resets); > > udelay(10); > > - reset_control_bulk_deassert(ARRAY_SIZE(core->resets), core->resets); > + reset_control_bulk_deassert(core->soc->num_resets, core->resets); > } > diff --git a/drivers/accel/rocket/rocket_core.h > b/drivers/accel/rocket/rocket_core.h > index f6d738285..0f424bb86 100644 > --- a/drivers/accel/rocket/rocket_core.h > +++ b/drivers/accel/rocket/rocket_core.h > @@ -27,16 +27,23 @@ > #define rocket_core_writel(core, reg, value) \ > writel(value, (core)->core_iomem + (REG_CORE_##reg) - REG_CORE_S_STATUS) > > +/* Per-SoC differences, selected by the of_device_id match data. */ > +struct rocket_soc_data { > + unsigned int num_clks; /* clk_bulk count */ > + unsigned int num_resets; /* reset_bulk count */ > +}; > + > struct rocket_core { > struct device *dev; > struct rocket_device *rdev; > + const struct rocket_soc_data *soc; > unsigned int index; > > int irq; > void __iomem *pc_iomem; > void __iomem *cna_iomem; > void __iomem *core_iomem; > - struct clk_bulk_data clks[4]; > + struct clk_bulk_data clks[6]; > struct reset_control_bulk_data resets[2]; > > struct iommu_group *iommu_group; > diff --git a/drivers/accel/rocket/rocket_drv.c > b/drivers/accel/rocket/rocket_drv.c > index 8bbbce594..6e7dc91c5 100644 > --- a/drivers/accel/rocket/rocket_drv.c > +++ b/drivers/accel/rocket/rocket_drv.c > @@ -176,6 +176,7 @@ static int rocket_probe(struct platform_device *pdev) > > rdev->cores[core].rdev = rdev; > rdev->cores[core].dev = &pdev->dev; > + rdev->cores[core].soc = of_device_get_match_data(&pdev->dev); > rdev->cores[core].index = core; > > rdev->num_cores++; > @@ -213,8 +214,13 @@ static void rocket_remove(struct platform_device *pdev) > } > } > > +static const struct rocket_soc_data rk3588_soc_data = { > + .num_clks = 4, > + .num_resets = 2, > +}; > + > static const struct of_device_id dt_match[] = { > - { .compatible = "rockchip,rk3588-rknn-core" }, > + { .compatible = "rockchip,rk3588-rknn-core", .data = &rk3588_soc_data }, While it technically fits on 1 line, putting the ``.data = `` part on its own line is easier to read and likely more future proof. Cheers, Diederik > {} > }; > MODULE_DEVICE_TABLE(of, dt_match); > @@ -240,7 +246,7 @@ static int rocket_device_runtime_resume(struct device > *dev) > if (core < 0) > return -ENODEV; > > - err = clk_bulk_prepare_enable(ARRAY_SIZE(rdev->cores[core].clks), > rdev->cores[core].clks); > + err = clk_bulk_prepare_enable(rdev->cores[core].soc->num_clks, > rdev->cores[core].clks); > if (err) { > dev_err(dev, "failed to enable (%d) clocks for core %d\n", err, > core); > return err; > @@ -260,7 +266,7 @@ static int rocket_device_runtime_suspend(struct device > *dev) > if (!rocket_job_is_idle(&rdev->cores[core])) > return -EBUSY; > > - clk_bulk_disable_unprepare(ARRAY_SIZE(rdev->cores[core].clks), > rdev->cores[core].clks); > + clk_bulk_disable_unprepare(rdev->cores[core].soc->num_clks, > rdev->cores[core].clks); > > return 0; > } > diff --git a/drivers/accel/rocket/rocket_job.c > b/drivers/accel/rocket/rocket_job.c > index bb77b6bf0..aa26e2977 100644 > --- a/drivers/accel/rocket/rocket_job.c > +++ b/drivers/accel/rocket/rocket_job.c > @@ -345,10 +345,15 @@ static void rocket_job_handle_irq(struct rocket_core > *core) > { > pm_runtime_mark_last_busy(core->dev); > > - rocket_pc_writel(core, OPERATION_ENABLE, 0x0); > - rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff); > + scoped_guard(mutex, &core->job_lock) { > + /* > + * Stopping the block belongs under the lock. hw_submit() writes > + * OPERATION_ENABLE too, and outside the lock this zero can land > + * after that one and kill a task that has only just started. > + */ > + rocket_pc_writel(core, OPERATION_ENABLE, 0x0); > + rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff); > > - scoped_guard(mutex, &core->job_lock) > if (core->in_flight_job) { > if (core->in_flight_job->next_task_idx < > core->in_flight_job->task_count) { > rocket_job_hw_submit(core, core->in_flight_job); > @@ -360,6 +365,7 @@ static void rocket_job_handle_irq(struct rocket_core > *core) > pm_runtime_put_autosuspend(core->dev); > core->in_flight_job = NULL; > } > + } > } > > static void
