| Message ID | 20230913111644.29889-1-hnagalla@ti.com |
|---|---|
| Headers | show |
| Series | TI K3 M4F support on AM64x and AM62x SoCs | expand |
On 13/09/2023 13:16, Hari Nagalla wrote: > From: Martyn Welch <martyn.welch@collabora.com> > > We will be adding the M4F driver which shares a lot of commonality > with the DSP driver. Common data structures are introduced here. > > Signed-off-by: Martyn Welch <martyn.welch@collabora.com> > Signed-off-by: Hari Nagalla <hnagalla@ti.com> > --- > Changes in v6: > - Created a separate patch for data structures to ease review > > drivers/remoteproc/ti_k3_common.h | 103 ++++++++++++++++++++++++++++++ > 1 file changed, 103 insertions(+) > create mode 100644 drivers/remoteproc/ti_k3_common.h > > diff --git a/drivers/remoteproc/ti_k3_common.h b/drivers/remoteproc/ti_k3_common.h > new file mode 100644 > index 000000000000..5e1f27741183 > --- /dev/null > +++ b/drivers/remoteproc/ti_k3_common.h > @@ -0,0 +1,103 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * TI K3 Remote Processor(s) driver common code > + * > + * Refactored from ti_k3_dsp_remoteproc.c. > + * > + * ti_k3_dsp_remoteproc.c: > + * Copyright (C) 2018-2022 Texas Instruments Incorporated - https://www.ti.com/ > + * Suman Anna <s-anna@ti.com> > + */ > + > +#ifndef REMOTEPROC_TI_K3_COMMON_H > +#define REMOTEPROC_TI_K3_COMMON_H > + > +#define KEYSTONE_RPROC_LOCAL_ADDRESS_MASK (SZ_16M - 1) > + > +/** > + * struct k3_rproc_mem - internal memory structure > + * @cpu_addr: MPU virtual address of the memory region > + * @bus_addr: Bus address used to access the memory region > + * @dev_addr: Device address of the memory region from DSP view > + * @size: Size of the memory region > + */ > +struct k3_rproc_mem { > + void __iomem *cpu_addr; > + phys_addr_t bus_addr; > + u32 dev_addr; > + size_t size; Where is the split? I see only addition here. Where is the usage of this header? This is basically dead code. Don't add dead code, but instead actually move the structures here! Move is cut and paste, not just paste. Best regards, Krzysztof
On 13/09/2023 13:16, Hari Nagalla wrote: > From: Martyn Welch <martyn.welch@collabora.com> > > The AM62x and AM64x SoCs of the TI K3 family has a Cortex M4F core in > the MCU domain. This core is typically used for safety applications in a > stand alone mode. However, some application (non safety related) may > want to use the M4F core as a generic remote processor with IPC to the > host processor. The M4F core has internal IRAM and DRAM memories and are > exposed to the system bus for code and data loading. > > A remote processor driver is added to support this subsystem, including > being able to load and boot the M4F core. Loading includes to M4F > internal memories and predefined external code/data memories. The > carve outs for external contiguous memory is defined in the M4F device > node and should match with the external memory declarations in the M4F > image binary. The M4F subsystem has two resets. One reset is for the > entire subsystem i.e including the internal memories and the other, a > local reset is only for the M4F processing core. When loading the image, > the driver first releases the subsystem reset, loads the firmware image > and then releases the local reset to let the M4F processing core run. > > Signed-off-by: Martyn Welch <martyn.welch@collabora.com> > Signed-off-by: Hari Nagalla <hnagalla@ti.com> > --- > Changes since v1: > - Addressed minor review comments (refactoring completed in separate > patch) > > Changes since v2: > - Refactoring completed first, thus smaller change > > Changes since v3: > - Removed 'ipc_only' flag and made changes in probe() to enact right > operations > - Fixed spelling mistakes in commit message > - Changed some 'dev_err' messages to 'dev_info' > - Removed unnecessary checks rproc state > > Changes since v4: > - None > > Changes since v5: > - None > > drivers/remoteproc/Kconfig | 13 + > drivers/remoteproc/Makefile | 1 + > drivers/remoteproc/ti_k3_m4_remoteproc.c | 331 +++++++++++++++++++++++ > 3 files changed, 345 insertions(+) > create mode 100644 drivers/remoteproc/ti_k3_m4_remoteproc.c > > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig > index 48845dc8fa85..85c1a3a2b987 100644 > --- a/drivers/remoteproc/Kconfig > +++ b/drivers/remoteproc/Kconfig > @@ -339,6 +339,19 @@ config TI_K3_DSP_REMOTEPROC > It's safe to say N here if you're not interested in utilizing > the DSP slave processors. > > +config TI_K3_M4_REMOTEPROC > + tristate "TI K3 M4 remoteproc support" > + depends on ARCH_K3 > + select MAILBOX > + select OMAP2PLUS_MBOX > + help > + Say m here to support TI's M4 remote processor subsystems > + on various TI K3 family of SoCs through the remote processor > + framework. > + > + It's safe to say N here if you're not interested in utilizing > + a remote processor. > + > config TI_K3_R5_REMOTEPROC > tristate "TI K3 R5 remoteproc support" > depends on ARCH_K3 > diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile > index 55c552e27a45..e30908ca4bfc 100644 > --- a/drivers/remoteproc/Makefile > +++ b/drivers/remoteproc/Makefile > @@ -37,5 +37,6 @@ obj-$(CONFIG_ST_REMOTEPROC) += st_remoteproc.o > obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o > obj-$(CONFIG_STM32_RPROC) += stm32_rproc.o > obj-$(CONFIG_TI_K3_DSP_REMOTEPROC) += ti_k3_dsp_remoteproc.o ti_k3_common.o > +obj-$(CONFIG_TI_K3_M4_REMOTEPROC) += ti_k3_m4_remoteproc.o ti_k3_common.o Nope, please compile your code and fix all the warnings. There is a big fat warning about including objects twice. Best regards, Krzysztof
On 13/09/2023 13:16, Hari Nagalla wrote: > From: Martyn Welch <martyn.welch@collabora.com> > > The AM62x and AM64x SoCs of the TI K3 family has a Cortex M4F core in > the MCU domain. This core is typically used for safety applications in a > stand alone mode. However, some application (non safety related) may > want to use the M4F core as a generic remote processor with IPC to the > host processor. The M4F core has internal IRAM and DRAM memories and are > exposed to the system bus for code and data loading. > > drivers/remoteproc/Kconfig | 13 + > drivers/remoteproc/Makefile | 1 + > drivers/remoteproc/ti_k3_m4_remoteproc.c | 331 +++++++++++++++++++++++ > 3 files changed, 345 insertions(+) > create mode 100644 drivers/remoteproc/ti_k3_m4_remoteproc.c > > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig > index 48845dc8fa85..85c1a3a2b987 100644 > --- a/drivers/remoteproc/Kconfig > +++ b/drivers/remoteproc/Kconfig > @@ -339,6 +339,19 @@ config TI_K3_DSP_REMOTEPROC > It's safe to say N here if you're not interested in utilizing > the DSP slave processors. > > +config TI_K3_M4_REMOTEPROC > + tristate "TI K3 M4 remoteproc support" > + depends on ARCH_K3 Missing compile testing. ... > + > +static int k3_m4_rproc_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + const struct k3_rproc_dev_data *data; > + struct k3_rproc *kproc; > + struct rproc *rproc; > + const char *fw_name; > + bool r_state = false; > + bool p_state = false; > + int ret = 0; > + int ret1; > + > + data = of_device_get_match_data(dev); > + if (!data) > + return -ENODEV; > + > + ret = rproc_of_parse_firmware(dev, 0, &fw_name); > + if (ret) { > + dev_err(dev, "failed to parse firmware-name property, ret = %d\n", > + ret); > + return ret; Nope, the syntax is dev_err_probe(). > + } > + > + rproc = rproc_alloc(dev, dev_name(dev), &k3_m4_rproc_ops, fw_name, > + sizeof(*kproc)); > + if (!rproc) > + return -ENOMEM; > + > + rproc->has_iommu = false; > + rproc->recovery_disabled = true; > + if (data->uses_lreset) { > + rproc->ops->prepare = k3_rproc_prepare; > + rproc->ops->unprepare = k3_rproc_unprepare; > + } > + kproc = rproc->priv; > + kproc->rproc = rproc; > + kproc->dev = dev; > + kproc->data = data; > + > + kproc->ti_sci = ti_sci_get_by_phandle(np, "ti,sci"); > + if (IS_ERR(kproc->ti_sci)) { > + ret = PTR_ERR(kproc->ti_sci); > + if (ret != -EPROBE_DEFER) { No, really, do not open-code existing code. > + dev_err(dev, "failed to get ti-sci handle, ret = %d\n", > + ret); > + } > + kproc->ti_sci = NULL; > + goto free_rproc; > + } > + > + ret = of_property_read_u32(np, "ti,sci-dev-id", &kproc->ti_sci_id); > + if (ret) { > + dev_err(dev, "missing 'ti,sci-dev-id' property\n"); > + goto put_sci; > + } > + > + kproc->reset = devm_reset_control_get_exclusive(dev, NULL); > + if (IS_ERR(kproc->reset)) { > + ret = PTR_ERR(kproc->reset); > + dev_err(dev, "failed to get reset, status = %d\n", ret); Syntax is return dev_err_probe. And everywhere else as well... Best regards, Krzysztof
On 13/09/2023 13:16, Hari Nagalla wrote: > From: Martyn Welch <martyn.welch@collabora.com> > > In the next commit we will be adding the M4F driver which shares a lot of > commonality with the DSP driver. Split this shared functionality out so > that it can be used by both drivers. > > Signed-off-by: Martyn Welch <martyn.welch@collabora.com> > Signed-off-by: Hari Nagalla <hnagalla@ti.com> > --- > Changes since v2: > - New patch (reordered refactored from v2) > > Changes since v3: > - Removed "ipc_only" element from k3_rproc structure > - Refactored to bring 3 more common functions > > Changes since v4: > - None > > Changes since v5: > - Rearranged the functions order to match with the functions in > ti_k3_dsp_remoteproc.c to ease review. > > drivers/remoteproc/Makefile | 2 +- > drivers/remoteproc/ti_k3_common.c | 513 +++++++++++++++++++ > drivers/remoteproc/ti_k3_dsp_remoteproc.c | 598 ++-------------------- Generate your patch correctly with -M/-B/-C so the move will be detected. Best regards, Krzysztof