| Message ID | 20201127092941.1646260-1-tzungbi@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,v2] remoteproc/mediatek: read IPI buffer offset from FW | expand |
On Fri, 27 Nov 2020 at 02:30, Tzung-Bi Shih <tzungbi@google.com> wrote: > > Reads the IPI buffer offset from the FW binary. The information resides > in addr of .ipi_buffer section. > > Moves scp_ipi_init() to scp_load() phase. The IPI buffer can be > initialized only if the offset is clear. > > Signed-off-by: Tzung-Bi Shih <tzungbi@google.com> > --- > The patch bases on https://git.kernel.org/pub/scm/linux/kernel/git/andersson/remoteproc.git/log/?h=for-next > > The first 2 patches in the series > https://patchwork.kernel.org/project/linux-remoteproc/cover/20201116084413.3312631-1-tzungbi@google.com/ > have been merged to remoteproc for-next branch. > > Follow up the discussion in > https://patchwork.kernel.org/project/linux-remoteproc/patch/20201116084413.3312631-4-tzungbi@google.com/#23784483 > > The patch breaks MTK SCP when working with legacy SCP firmware. We're > aware of it and will upgrade the devices' kernel and SCP firmware > carefully. Other than that, AFAICT, no other devices in the wild are > using this driver. > > drivers/remoteproc/mtk_scp.c | 73 ++++++++++++++++++++++++------------ > 1 file changed, 49 insertions(+), 24 deletions(-) This is the exact same patch that you sent here [1], that I commented on, and that you agreed with my assessment. What do you want me to do here? What am I missing? [1]. https://patchwork.kernel.org/project/linux-remoteproc/patch/20201116084413.3312631-4-tzungbi@google.com/ > > diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c > index 7e0f1e1a335b..4467ed646bb1 100644 > --- a/drivers/remoteproc/mtk_scp.c > +++ b/drivers/remoteproc/mtk_scp.c > @@ -21,7 +21,7 @@ > #include "remoteproc_internal.h" > > #define MAX_CODE_SIZE 0x500000 > -#define SCP_FW_END 0x7C000 > +#define SECTION_NAME_IPI_BUFFER ".ipi_buffer" > > /** > * scp_get() - get a reference to SCP. > @@ -119,16 +119,24 @@ static void scp_ipi_handler(struct mtk_scp *scp) > wake_up(&scp->ack_wq); > } > > -static int scp_ipi_init(struct mtk_scp *scp) > +static int scp_elf_read_ipi_buf_addr(struct mtk_scp *scp, > + const struct firmware *fw, > + size_t *offset); > + > +static int scp_ipi_init(struct mtk_scp *scp, const struct firmware *fw) > { > - size_t send_offset = SCP_FW_END - sizeof(struct mtk_share_obj); > - size_t recv_offset = send_offset - sizeof(struct mtk_share_obj); > + int ret; > + size_t offset; > + > + ret = scp_elf_read_ipi_buf_addr(scp, fw, &offset); > + if (ret) > + return ret; > + dev_info(scp->dev, "IPI buf addr %#010zx\n", offset); > > - /* shared buffer initialization */ > - scp->recv_buf = > - (struct mtk_share_obj __iomem *)(scp->sram_base + recv_offset); > - scp->send_buf = > - (struct mtk_share_obj __iomem *)(scp->sram_base + send_offset); > + scp->recv_buf = (struct mtk_share_obj __iomem *) > + (scp->sram_base + offset); > + scp->send_buf = (struct mtk_share_obj __iomem *) > + (scp->sram_base + offset + sizeof(*scp->recv_buf)); > memset_io(scp->recv_buf, 0, sizeof(*scp->recv_buf)); > memset_io(scp->send_buf, 0, sizeof(*scp->send_buf)); > > @@ -271,6 +279,32 @@ static int scp_elf_load_segments(struct rproc *rproc, const struct firmware *fw) > return ret; > } > > +static int scp_elf_read_ipi_buf_addr(struct mtk_scp *scp, > + const struct firmware *fw, > + size_t *offset) > +{ > + struct elf32_hdr *ehdr; > + struct elf32_shdr *shdr, *shdr_strtab; > + int i; > + const u8 *elf_data = fw->data; > + const char *strtab; > + > + ehdr = (struct elf32_hdr *)elf_data; > + shdr = (struct elf32_shdr *)(elf_data + ehdr->e_shoff); > + shdr_strtab = shdr + ehdr->e_shstrndx; > + strtab = (const char *)(elf_data + shdr_strtab->sh_offset); > + > + for (i = 0; i < ehdr->e_shnum; i++, shdr++) { > + if (strcmp(strtab + shdr->sh_name, > + SECTION_NAME_IPI_BUFFER) == 0) { > + *offset = shdr->sh_addr; > + return 0; > + } > + } > + > + return -ENOENT; > +} > + > static int mt8183_scp_before_load(struct mtk_scp *scp) > { > /* Clear SCP to host interrupt */ > @@ -350,11 +384,15 @@ static int scp_load(struct rproc *rproc, const struct firmware *fw) > > ret = scp->data->scp_before_load(scp); > if (ret < 0) > - return ret; > + goto leave; > > ret = scp_elf_load_segments(rproc, fw); > - clk_disable_unprepare(scp->clk); > + if (ret) > + goto leave; > > + ret = scp_ipi_init(scp, fw); > +leave: > + clk_disable_unprepare(scp->clk); > return ret; > } > > @@ -680,19 +718,6 @@ static int scp_probe(struct platform_device *pdev) > goto release_dev_mem; > } > > - ret = clk_prepare_enable(scp->clk); > - if (ret) { > - dev_err(dev, "failed to enable clocks\n"); > - goto release_dev_mem; > - } > - > - ret = scp_ipi_init(scp); > - clk_disable_unprepare(scp->clk); > - if (ret) { > - dev_err(dev, "Failed to init ipi\n"); > - goto release_dev_mem; > - } > - > /* register SCP initialization IPI */ > ret = scp_ipi_register(scp, SCP_IPI_INIT, scp_init_ipi_handler, scp); > if (ret) { > -- > 2.29.2.454.gaff20da3a2-goog >
On Sat, Nov 28, 2020 at 12:11 AM Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > On Fri, 27 Nov 2020 at 02:30, Tzung-Bi Shih <tzungbi@google.com> wrote: > > The patch breaks MTK SCP when working with legacy SCP firmware. We're > > aware of it and will upgrade the devices' kernel and SCP firmware > > carefully. Other than that, AFAICT, no other devices in the wild are > > using this driver. > > > > This is the exact same patch that you sent here [1], that I commented > on, and that you agreed with my assessment. > > What do you want me to do here? What am I missing? Yes, this is a resend patch because only the first 2 patches in the previous series have merged. I agree the patch is aggressive which would break machines with old SCP firmware. But AFAICT, no other devices are using this driver; and we'll take care of our devices to upgrade SCP firmware first and then kernel drivers. Thus, ideally, no real device breakage is expected. Would the patch be acceptable? Or would you suggest we consider backward-compatible anyway (even if with the context mentioned above)?
On Fri, 27 Nov 2020 at 10:25, Tzung-Bi Shih <tzungbi@google.com> wrote: > > On Sat, Nov 28, 2020 at 12:11 AM Mathieu Poirier > <mathieu.poirier@linaro.org> wrote: > > On Fri, 27 Nov 2020 at 02:30, Tzung-Bi Shih <tzungbi@google.com> wrote: > > > The patch breaks MTK SCP when working with legacy SCP firmware. We're > > > aware of it and will upgrade the devices' kernel and SCP firmware > > > carefully. Other than that, AFAICT, no other devices in the wild are > > > using this driver. > > > > > > > This is the exact same patch that you sent here [1], that I commented > > on, and that you agreed with my assessment. > > > > What do you want me to do here? What am I missing? > > Yes, this is a resend patch because only the first 2 patches in the > previous series have merged. > The first two patches were merged because they made sense. > I agree the patch is aggressive which would break machines with old > SCP firmware. But AFAICT, no other devices are using this driver; and > we'll take care of our devices to upgrade SCP firmware first and then > kernel drivers. Thus, ideally, no real device breakage is expected. > How do you know about all the systems out there that use this SoC? Moreover why would the original author have implemented the driver the way they did if it didn't work for them? > Would the patch be acceptable? Definitely not. > Or would you suggest we consider > backward-compatible anyway (even if with the context mentioned above)? That is the only way this patch will get merged.
On Sat, Nov 28, 2020 at 2:25 AM Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > > On Fri, 27 Nov 2020 at 10:25, Tzung-Bi Shih <tzungbi@google.com> wrote: > > > > On Sat, Nov 28, 2020 at 12:11 AM Mathieu Poirier > > <mathieu.poirier@linaro.org> wrote: > > > On Fri, 27 Nov 2020 at 02:30, Tzung-Bi Shih <tzungbi@google.com> wrote: > > > > The patch breaks MTK SCP when working with legacy SCP firmware. We're > > > > aware of it and will upgrade the devices' kernel and SCP firmware > > > > carefully. Other than that, AFAICT, no other devices in the wild are > > > > using this driver. > > > > > > I agree the patch is aggressive which would break machines with old > > SCP firmware. But AFAICT, no other devices are using this driver; and > > we'll take care of our devices to upgrade SCP firmware first and then > > kernel drivers. Thus, ideally, no real device breakage is expected. > > > > How do you know about all the systems out there that use this SoC? > Moreover why would the original author have implemented the driver the > way they did if it didn't work for them? I am trying not to repeat my statements. - AFAIK, MT8183-based chromebooks are the only user of the driver. We're happy to provide fixups if any other devices break due to the aggressive patch. - The original author Erin Lo <erin.lo@mediatek.com> adds the driver for MT8183-based chromebooks. - The IPI buffer address was hard-coded because we didn't foresee new feature changes in the next generation MTK SCP. If it still makes no sense, here is v3 which is backward compatible with legacy firmwares. (https://patchwork.kernel.org/project/linux-remoteproc/patch/20201130034025.3232229-1-tzungbi@google.com/)
diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c index 7e0f1e1a335b..4467ed646bb1 100644 --- a/drivers/remoteproc/mtk_scp.c +++ b/drivers/remoteproc/mtk_scp.c @@ -21,7 +21,7 @@ #include "remoteproc_internal.h" #define MAX_CODE_SIZE 0x500000 -#define SCP_FW_END 0x7C000 +#define SECTION_NAME_IPI_BUFFER ".ipi_buffer" /** * scp_get() - get a reference to SCP. @@ -119,16 +119,24 @@ static void scp_ipi_handler(struct mtk_scp *scp) wake_up(&scp->ack_wq); } -static int scp_ipi_init(struct mtk_scp *scp) +static int scp_elf_read_ipi_buf_addr(struct mtk_scp *scp, + const struct firmware *fw, + size_t *offset); + +static int scp_ipi_init(struct mtk_scp *scp, const struct firmware *fw) { - size_t send_offset = SCP_FW_END - sizeof(struct mtk_share_obj); - size_t recv_offset = send_offset - sizeof(struct mtk_share_obj); + int ret; + size_t offset; + + ret = scp_elf_read_ipi_buf_addr(scp, fw, &offset); + if (ret) + return ret; + dev_info(scp->dev, "IPI buf addr %#010zx\n", offset); - /* shared buffer initialization */ - scp->recv_buf = - (struct mtk_share_obj __iomem *)(scp->sram_base + recv_offset); - scp->send_buf = - (struct mtk_share_obj __iomem *)(scp->sram_base + send_offset); + scp->recv_buf = (struct mtk_share_obj __iomem *) + (scp->sram_base + offset); + scp->send_buf = (struct mtk_share_obj __iomem *) + (scp->sram_base + offset + sizeof(*scp->recv_buf)); memset_io(scp->recv_buf, 0, sizeof(*scp->recv_buf)); memset_io(scp->send_buf, 0, sizeof(*scp->send_buf)); @@ -271,6 +279,32 @@ static int scp_elf_load_segments(struct rproc *rproc, const struct firmware *fw) return ret; } +static int scp_elf_read_ipi_buf_addr(struct mtk_scp *scp, + const struct firmware *fw, + size_t *offset) +{ + struct elf32_hdr *ehdr; + struct elf32_shdr *shdr, *shdr_strtab; + int i; + const u8 *elf_data = fw->data; + const char *strtab; + + ehdr = (struct elf32_hdr *)elf_data; + shdr = (struct elf32_shdr *)(elf_data + ehdr->e_shoff); + shdr_strtab = shdr + ehdr->e_shstrndx; + strtab = (const char *)(elf_data + shdr_strtab->sh_offset); + + for (i = 0; i < ehdr->e_shnum; i++, shdr++) { + if (strcmp(strtab + shdr->sh_name, + SECTION_NAME_IPI_BUFFER) == 0) { + *offset = shdr->sh_addr; + return 0; + } + } + + return -ENOENT; +} + static int mt8183_scp_before_load(struct mtk_scp *scp) { /* Clear SCP to host interrupt */ @@ -350,11 +384,15 @@ static int scp_load(struct rproc *rproc, const struct firmware *fw) ret = scp->data->scp_before_load(scp); if (ret < 0) - return ret; + goto leave; ret = scp_elf_load_segments(rproc, fw); - clk_disable_unprepare(scp->clk); + if (ret) + goto leave; + ret = scp_ipi_init(scp, fw); +leave: + clk_disable_unprepare(scp->clk); return ret; } @@ -680,19 +718,6 @@ static int scp_probe(struct platform_device *pdev) goto release_dev_mem; } - ret = clk_prepare_enable(scp->clk); - if (ret) { - dev_err(dev, "failed to enable clocks\n"); - goto release_dev_mem; - } - - ret = scp_ipi_init(scp); - clk_disable_unprepare(scp->clk); - if (ret) { - dev_err(dev, "Failed to init ipi\n"); - goto release_dev_mem; - } - /* register SCP initialization IPI */ ret = scp_ipi_register(scp, SCP_IPI_INIT, scp_init_ipi_handler, scp); if (ret) {
Reads the IPI buffer offset from the FW binary. The information resides in addr of .ipi_buffer section. Moves scp_ipi_init() to scp_load() phase. The IPI buffer can be initialized only if the offset is clear. Signed-off-by: Tzung-Bi Shih <tzungbi@google.com> --- The patch bases on https://git.kernel.org/pub/scm/linux/kernel/git/andersson/remoteproc.git/log/?h=for-next The first 2 patches in the series https://patchwork.kernel.org/project/linux-remoteproc/cover/20201116084413.3312631-1-tzungbi@google.com/ have been merged to remoteproc for-next branch. Follow up the discussion in https://patchwork.kernel.org/project/linux-remoteproc/patch/20201116084413.3312631-4-tzungbi@google.com/#23784483 The patch breaks MTK SCP when working with legacy SCP firmware. We're aware of it and will upgrade the devices' kernel and SCP firmware carefully. Other than that, AFAICT, no other devices in the wild are using this driver. drivers/remoteproc/mtk_scp.c | 73 ++++++++++++++++++++++++------------ 1 file changed, 49 insertions(+), 24 deletions(-)