From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 21 Apr 2020 17:43:44 +0200 From: Paul Cercueil Subject: Re: [PATCH v6 4/5] remoteproc: ingenic: Added remoteproc driver Message-Id: In-Reply-To: <20200420063714.GA1868936@builder.lan> References: <20200417170040.174319-1-paul@crapouillou.net> <20200417170040.174319-4-paul@crapouillou.net> <20200420063714.GA1868936@builder.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable To: Bjorn Andersson Cc: Ohad Ben-Cohen , Arnaud Pouliquen , od@zcrc.me, linux-remoteproc@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, kbuild test robot , Julia Lawall , Mathieu Poirier List-ID: Hi Bjorn, Le dim. 19 avril 2020 =E0 23:37, Bjorn Andersson=20 a =E9crit : > On Fri 17 Apr 10:00 PDT 2020, Paul Cercueil wrote: >=20 >> This driver is used to boot, communicate with and load firmwares to=20 >> the >> MIPS co-processor found in the VPU hardware of the JZ47xx SoCs from >> Ingenic. >>=20 >> Signed-off-by: Paul Cercueil >> Signed-off-by: kbuild test robot >> Signed-off-by: Julia Lawall >=20 > Please read Documentation/process/submitting-patches.rst about > "Developer's Certificate of Origin". >=20 > I suspect that you incorporated review feedback on previous revisions > from kbuild and Julia, this is generally omitted from the actual=20 > commit > message. Julia / kbuild sent a patch to fix an error in the driver, so my patch=20 now has code from Julia / kbuild. That document clearly says that I=20 should add their signed-off-by. Or what do you mean? >> Acked-by: Mathieu Poirier >> --- >>=20 >> Notes: >> v2: Remove exception for always-mapped memories >> v3: - Use clk_bulk API >> - Move device-managed code to its own patch [3/4] >> - Move devicetree table right above ingenic_rproc_driver >> - Removed #ifdef CONFIG_OF around devicetree table >> - Removed .owner =3D THIS_MODULE in ingenic_rproc_driver >> - Removed useless platform_set_drvdata() >> v4: - Add fix reported by Julia >> - Change Kconfig symbol to INGENIC_VPU_RPROC >> - Add documentation to struct vpu >> - disable_irq_nosync() -> disable_irq() >> v5: No change >> v6: Instead of prepare/unprepare callbacks, use PM runtime=20 >> callbacks >>=20 >> drivers/remoteproc/Kconfig | 8 + >> drivers/remoteproc/Makefile | 1 + >> drivers/remoteproc/ingenic_rproc.c | 282=20 >> +++++++++++++++++++++++++++++ >> 3 files changed, 291 insertions(+) >> create mode 100644 drivers/remoteproc/ingenic_rproc.c >>=20 >> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig >> index fbaed079b299..31da3e6c6281 100644 >> --- a/drivers/remoteproc/Kconfig >> +++ b/drivers/remoteproc/Kconfig >> @@ -240,6 +240,14 @@ config STM32_RPROC >>=20 >> This can be either built-in or a loadable module. >>=20 >> +config INGENIC_VPU_RPROC >=20 > Please try to keep things alphabetically ordered. >=20 >> + tristate "Ingenic JZ47xx VPU remoteproc support" >> + depends on MIPS || COMPILE_TEST >> + help >> + Say y or m here to support the VPU in the JZ47xx SoCs from=20 >> Ingenic. >> + This can be either built-in or a loadable module. >> + If unsure say N. >> + >> endif # REMOTEPROC >>=20 >> endmenu > [..] >> diff --git a/drivers/remoteproc/ingenic_rproc.c=20 >> b/drivers/remoteproc/ingenic_rproc.c > [..] >> +/** >> + * struct vpu - Ingenic VPU remoteproc private structure >> + * @irq: interrupt number >> + * @clks: pointers to the VPU and AUX clocks >=20 > aux_base is missing >=20 >> + * @mem_info: array of struct vpu_mem_info, which contain the=20 >> mapping info of >> + * each of the external memories >> + * @dev: private pointer to the device >> + */ >> +struct vpu { >> + int irq; >> + struct clk_bulk_data clks[2]; >> + void __iomem *aux_base; >> + struct vpu_mem_info mem_info[ARRAY_SIZE(vpu_mem_map)]; >> + struct device *dev; >> +}; > [..] >> +static void *ingenic_rproc_da_to_va(struct rproc *rproc, u64 da,=20 >> size_t len) >> +{ >> + struct vpu *vpu =3D rproc->priv; >> + void __iomem *va =3D NULL; >> + unsigned int i; >> + >> + if (len <=3D 0) >=20 > len can't be negative (also, does it add value to check for and fail=20 > len > =3D=3D 0?) >=20 >> + return NULL; >> + >> + for (i =3D 0; i < ARRAY_SIZE(vpu_mem_map); i++) { >> + const struct vpu_mem_info *info =3D &vpu->mem_info[i]; >> + const struct vpu_mem_map *map =3D info->map; >> + >> + if (da >=3D map->da && (da + len) < (map->da + info->len)) { >> + va =3D info->base + (da - map->da); >> + break; >> + } >> + } >> + >> + return (__force void *)va; >> +} > [..] >> +static struct platform_driver ingenic_rproc_driver =3D { >> + .probe =3D ingenic_rproc_probe, >> + .driver =3D { >> + .name =3D "ingenic-vpu", >> +#ifdef CONFIG_PM >=20 > Please omit the #ifdef here. If I do, then the PM callbacks will be compiled in even if CONFIG_PM is=20 disabled. That means dead code and I see no reason why you would want=20 that. If you don't mind, I'd like to keep the #ifdef CONFIG_PM for now, until=20 this patchset is merged: https://lkml.org/lkml/2020/4/13/582 Then it would become a one-liner: .pm =3D pm_ptr(&ingenic_rproc_pm), Cheers, -Paul >> + .pm =3D &ingenic_rproc_pm, >> +#endif >> + .of_match_table =3D of_match_ptr(ingenic_rproc_of_matches), >=20 > Please omit the of_match_ptr() >=20 > Regards, > Bjorn >=20 >> + }, >> +}; >> +module_platform_driver(ingenic_rproc_driver); >> + >> +MODULE_LICENSE("GPL"); >> +MODULE_AUTHOR("Paul Cercueil "); >> +MODULE_DESCRIPTION("Ingenic JZ47xx Remote Processor control=20 >> driver"); >> -- >> 2.25.1 >>=20