diff mbox series

[RESEND,v2] remoteproc/mediatek: read IPI buffer offset from FW

Message ID 20201127092941.1646260-1-tzungbi@google.com (mailing list archive)
State New
Headers show
Series [RESEND,v2] remoteproc/mediatek: read IPI buffer offset from FW | expand

Commit Message

Tzung-Bi Shih Nov. 27, 2020, 9:29 a.m. UTC
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(-)

Comments

Mathieu Poirier Nov. 27, 2020, 4:11 p.m. UTC | #1
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
>
Tzung-Bi Shih Nov. 27, 2020, 5:25 p.m. UTC | #2
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)?
Mathieu Poirier Nov. 27, 2020, 6:25 p.m. UTC | #3
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.
Tzung-Bi Shih Nov. 30, 2020, 3:47 a.m. UTC | #4
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 mbox series

Patch

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) {