Message ID | 20201214050521.845396-1-tzungbi@google.com |
---|---|
Headers | show |
Series | remoteproc/mediatek: support L1TCM for MT8192 SCP | expand |
Hi Shih, On Mon, Dec 14, 2020 at 01:05:21PM +0800, Tzung-Bi Shih wrote: > L1TCM is a high performance memory region in MT8192 SCP. > > Reads L1TCM memory region from DTS to determine if the machine supports. > Loads L1TCM memory region to SCP sys if the firmware provides. > > Starts from MT8192 SCP, the firmware contains physical addresses for > each memory region, for instance: > > Program Headers: > Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align > LOAD 0xXXXXXX 0xXXXXXXXX 0x10500000 0xXXXXX 0xXXXXX XXX 0xXXXX > LOAD 0xXXXXXX 0xXXXXXXXX 0x10700000 0xXXXXX 0xXXXXX XXX 0xXXXX > LOAD 0xXXXXXX 0xXXXXXXXX 0x50000000 0xXXXXX 0xXXXXX XXX 0xXXXX > > Kernel driver can use the "PhysAddr" (i.e. da in the da_to_va callbacks) > to know the ELF segment belongs to which region. > > To backward compatible to MT8183 SCP, separates the da_to_va callbacks > for new and legacy version. > > Signed-off-by: Tzung-Bi Shih <tzungbi@google.com> > --- > drivers/remoteproc/mtk_common.h | 5 +++ > drivers/remoteproc/mtk_scp.c | 54 +++++++++++++++++++++++++++++++-- > 2 files changed, 57 insertions(+), 2 deletions(-) > > diff --git a/drivers/remoteproc/mtk_common.h b/drivers/remoteproc/mtk_common.h > index 988edb4977c3..94bc54b224ee 100644 > --- a/drivers/remoteproc/mtk_common.h > +++ b/drivers/remoteproc/mtk_common.h > @@ -75,6 +75,7 @@ struct mtk_scp_of_data { > void (*scp_reset_assert)(struct mtk_scp *scp); > void (*scp_reset_deassert)(struct mtk_scp *scp); > void (*scp_stop)(struct mtk_scp *scp); > + void *(*scp_da_to_va)(struct mtk_scp *scp, u64 da, size_t len); > > u32 host_to_scp_reg; > u32 host_to_scp_int_bit; > @@ -89,6 +90,10 @@ struct mtk_scp { > void __iomem *reg_base; > void __iomem *sram_base; > size_t sram_size; > + phys_addr_t sram_phys; > + void __iomem *l1tcm_base; > + size_t l1tcm_size; > + phys_addr_t l1tcm_phys; > > const struct mtk_scp_of_data *data; > > diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c > index e0c235690361..f025aba67abc 100644 > --- a/drivers/remoteproc/mtk_scp.c > +++ b/drivers/remoteproc/mtk_scp.c > @@ -458,9 +458,8 @@ static int scp_start(struct rproc *rproc) > return ret; > } > > -static void *scp_da_to_va(struct rproc *rproc, u64 da, size_t len) > +static void *mt8183_scp_da_to_va(struct mtk_scp *scp, u64 da, size_t len) > { > - struct mtk_scp *scp = (struct mtk_scp *)rproc->priv; > int offset; > > if (da < scp->sram_size) { > @@ -476,6 +475,42 @@ static void *scp_da_to_va(struct rproc *rproc, u64 da, size_t len) > return NULL; > } > > +static void *mt8192_scp_da_to_va(struct mtk_scp *scp, u64 da, size_t len) > +{ > + int offset; > + > + if (da >= scp->sram_phys && > + (da + len) <= scp->sram_phys + scp->sram_size) { > + offset = da - scp->sram_phys; > + return (void __force *)scp->sram_base + offset; > + } > + > + /* optional memory region */ > + if (scp->l1tcm_size && > + da >= scp->l1tcm_phys && > + (da + len) <= scp->l1tcm_phys + scp->l1tcm_size) { > + offset = da - scp->l1tcm_phys; > + return (void __force *)scp->l1tcm_base + offset; > + } > + > + /* optional memory region */ > + if (scp->dram_size && > + da >= scp->dma_addr && > + (da + len) <= scp->dma_addr + scp->dram_size) { > + offset = da - scp->dma_addr; > + return scp->cpu_addr + offset; > + } > + > + return NULL; > +} > + > +static void *scp_da_to_va(struct rproc *rproc, u64 da, size_t len) > +{ > + struct mtk_scp *scp = (struct mtk_scp *)rproc->priv; > + > + return scp->data->scp_da_to_va(scp, da, len); > +} > + > static void mt8183_scp_stop(struct mtk_scp *scp) > { > /* Disable SCP watchdog */ > @@ -714,6 +749,19 @@ static int scp_probe(struct platform_device *pdev) > goto free_rproc; > } > scp->sram_size = resource_size(res); > + scp->sram_phys = res->start; > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "l1tcm"); > + if (res) { As far as I can tell the if() condition isn't needed since platform_get_resource_byname() returns NULL on error and devm_ioremap_resource() is capable of handling that condition. As such the code to parse "l1tcm" can be the same as what is done for "sram". With the above: Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> > + scp->l1tcm_base = devm_ioremap_resource(dev, res); > + if (IS_ERR((__force void *)scp->l1tcm_base)) { > + dev_err(dev, "Failed to parse and map l1tcm memory\n"); > + ret = PTR_ERR((__force void *)scp->l1tcm_base); > + goto free_rproc; > + } > + scp->l1tcm_size = resource_size(res); > + scp->l1tcm_phys = res->start; > + } > > mutex_init(&scp->send_lock); > for (i = 0; i < SCP_IPI_MAX; i++) > @@ -803,6 +851,7 @@ static const struct mtk_scp_of_data mt8183_of_data = { > .scp_reset_assert = mt8183_scp_reset_assert, > .scp_reset_deassert = mt8183_scp_reset_deassert, > .scp_stop = mt8183_scp_stop, > + .scp_da_to_va = mt8183_scp_da_to_va, > .host_to_scp_reg = MT8183_HOST_TO_SCP, > .host_to_scp_int_bit = MT8183_HOST_IPC_INT_BIT, > .ipi_buf_offset = 0x7bdb0, > @@ -814,6 +863,7 @@ static const struct mtk_scp_of_data mt8192_of_data = { > .scp_reset_assert = mt8192_scp_reset_assert, > .scp_reset_deassert = mt8192_scp_reset_deassert, > .scp_stop = mt8192_scp_stop, > + .scp_da_to_va = mt8192_scp_da_to_va, > .host_to_scp_reg = MT8192_GIPC_IN_SET, > .host_to_scp_int_bit = MT8192_HOST_IPC_INT_BIT, > }; > -- > 2.29.2.684.gfbc64c5ab5-goog >
On Thu, Jan 7, 2021 at 7:15 AM Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > > > static void mt8183_scp_stop(struct mtk_scp *scp) > > { > > /* Disable SCP watchdog */ > > @@ -714,6 +749,19 @@ static int scp_probe(struct platform_device *pdev) > > goto free_rproc; > > } > > scp->sram_size = resource_size(res); > > + scp->sram_phys = res->start; > > + > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "l1tcm"); > > + if (res) { > > As far as I can tell the if() condition isn't needed since > platform_get_resource_byname() returns NULL on error and devm_ioremap_resource() > is capable of handling that condition. As such the code to parse "l1tcm" can be > the same as what is done for "sram". The "l1tcm" memory region is optional. The if() condition is for: if DTS doesn't provide the memory region, kernel can skip the code block. > > With the above: > > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > > + scp->l1tcm_base = devm_ioremap_resource(dev, res); > > + if (IS_ERR((__force void *)scp->l1tcm_base)) { > > + dev_err(dev, "Failed to parse and map l1tcm memory\n"); > > + ret = PTR_ERR((__force void *)scp->l1tcm_base); > > + goto free_rproc; > > + } > > + scp->l1tcm_size = resource_size(res); > > + scp->l1tcm_phys = res->start; > > + }
On Thu, Jan 07, 2021 at 09:50:21AM +0800, Tzung-Bi Shih wrote: > On Thu, Jan 7, 2021 at 7:15 AM Mathieu Poirier > <mathieu.poirier@linaro.org> wrote: > > > > > static void mt8183_scp_stop(struct mtk_scp *scp) > > > { > > > /* Disable SCP watchdog */ > > > @@ -714,6 +749,19 @@ static int scp_probe(struct platform_device *pdev) > > > goto free_rproc; > > > } > > > scp->sram_size = resource_size(res); > > > + scp->sram_phys = res->start; > > > + > > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "l1tcm"); > > > + if (res) { > > > > As far as I can tell the if() condition isn't needed since > > platform_get_resource_byname() returns NULL on error and devm_ioremap_resource() > > is capable of handling that condition. As such the code to parse "l1tcm" can be > > the same as what is done for "sram". > > The "l1tcm" memory region is optional. The if() condition is for: if > DTS doesn't provide the memory region, kernel can skip the code block. > Very well - thanks for the clarification. > > > > With the above: > > > > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > > > > + scp->l1tcm_base = devm_ioremap_resource(dev, res); > > > + if (IS_ERR((__force void *)scp->l1tcm_base)) { > > > + dev_err(dev, "Failed to parse and map l1tcm memory\n"); > > > + ret = PTR_ERR((__force void *)scp->l1tcm_base); > > > + goto free_rproc; > > > + } > > > + scp->l1tcm_size = resource_size(res); > > > + scp->l1tcm_phys = res->start; > > > + }
On Wed 06 Jan 19:50 CST 2021, Tzung-Bi Shih wrote: > On Thu, Jan 7, 2021 at 7:15 AM Mathieu Poirier > <mathieu.poirier@linaro.org> wrote: > > > > > static void mt8183_scp_stop(struct mtk_scp *scp) > > > { > > > /* Disable SCP watchdog */ > > > @@ -714,6 +749,19 @@ static int scp_probe(struct platform_device *pdev) > > > goto free_rproc; > > > } > > > scp->sram_size = resource_size(res); > > > + scp->sram_phys = res->start; > > > + > > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "l1tcm"); > > > + if (res) { > > > > As far as I can tell the if() condition isn't needed since > > platform_get_resource_byname() returns NULL on error and devm_ioremap_resource() > > is capable of handling that condition. As such the code to parse "l1tcm" can be > > the same as what is done for "sram". > > The "l1tcm" memory region is optional. The if() condition is for: if > DTS doesn't provide the memory region, kernel can skip the code block. > People are actively looking for platform_get_resource_byname + devm_ioremap_resource() pairs to replace with devm_platform_ioremap_resource_byname(), so we're probably going to have someone try to patch this soon... So please change the pair to devm_platform_ioremap_resource_byname() and treat a returned -EINVAL as the memory isn't specified and other IS_ERR() as errors. Thanks, Bjorn > > > > With the above: > > > > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > > > > + scp->l1tcm_base = devm_ioremap_resource(dev, res); > > > + if (IS_ERR((__force void *)scp->l1tcm_base)) { > > > + dev_err(dev, "Failed to parse and map l1tcm memory\n"); > > > + ret = PTR_ERR((__force void *)scp->l1tcm_base); > > > + goto free_rproc; > > > + } > > > + scp->l1tcm_size = resource_size(res); > > > + scp->l1tcm_phys = res->start; > > > + }
On Fri, Jan 8, 2021 at 2:02 AM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Wed 06 Jan 19:50 CST 2021, Tzung-Bi Shih wrote: > > > On Thu, Jan 7, 2021 at 7:15 AM Mathieu Poirier > > <mathieu.poirier@linaro.org> wrote: > > > > > > > static void mt8183_scp_stop(struct mtk_scp *scp) > > > > { > > > > /* Disable SCP watchdog */ > > > > @@ -714,6 +749,19 @@ static int scp_probe(struct platform_device *pdev) > > > > goto free_rproc; > > > > } > > > > scp->sram_size = resource_size(res); > > > > + scp->sram_phys = res->start; > > > > + > > > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "l1tcm"); > > > > + if (res) { > > > > > > As far as I can tell the if() condition isn't needed since > > > platform_get_resource_byname() returns NULL on error and devm_ioremap_resource() > > > is capable of handling that condition. As such the code to parse "l1tcm" can be > > > the same as what is done for "sram". > > > > The "l1tcm" memory region is optional. The if() condition is for: if > > DTS doesn't provide the memory region, kernel can skip the code block. > > > > People are actively looking for platform_get_resource_byname + > devm_ioremap_resource() pairs to replace with > devm_platform_ioremap_resource_byname(), so we're probably going to have > someone try to patch this soon... > > So please change the pair to devm_platform_ioremap_resource_byname() and > treat a returned -EINVAL as the memory isn't specified and other > IS_ERR() as errors. Will do but what if we need to access the "res"? For example: scp->sram_size = resource_size(res); By using devm_platform_ioremap_resource_byname() which will drop the access to res. Shall we add a new variant of devm_platform_ioremap_resource_byname()?