Thanks for the review. I'll address most of the comments in the next version.
On Mon, Jul 22, 2019 at 5:37 PM Alexandre Courbot <acourbot@xxxxxxxxxxxx> wrote:
>
> Hi Pi-Hsun,
>
> On Tue, Jul 9, 2019 at 4:27 PM Pi-Hsun Shih <pihsun@xxxxxxxxxxxx> wrote:
> > +static void *scp_da_to_va(struct rproc *rproc, u64 da, int len)
> > +{
> > + struct mtk_scp *scp = (struct mtk_scp *)rproc->priv;
> > + int offset;
> > +
> > + if (da < scp->sram_size) {
> > + offset = da;
> > + if (offset >= 0 && ((offset + len) < scp->sram_size))
> > + return (__force void *)(scp->sram_base + offset);
> > + } else if (da >= scp->sram_size &&
> > + da < (scp->sram_size + MAX_CODE_SIZE)) {
> > + offset = da;
>
> This line looks suspicious. Shouldn't it be
>
> offset = da - scp->sram_size?
>
Actually the whole "else if (...)" is not used. Would remove this in
next version.
> > +
> > +/*
> > + * Copy src to dst, where dst is in SCP SRAM region.
> > + * Since AP access of SCP SRAM don't support byte write, this always write a
> > + * full word at a time, and may cause some extra bytes to be written at the
> > + * beginning & ending of dst.
> > + */
> > +void scp_memcpy_aligned(void *dst, const void *src, unsigned int len)
> > +{
> > + void *ptr;
> > + u32 val;
> > + unsigned int i = 0;
> > +
> > + if (!IS_ALIGNED((unsigned long)dst, 4)) {
> > + ptr = (void *)ALIGN_DOWN((unsigned long)dst, 4);
> > + i = 4 - (dst - ptr);
> > + val = readl_relaxed(ptr);
> > + memcpy((u8 *)&val + (4 - i), src, i);
> > + writel_relaxed(val, ptr);
> > + }
> > +
> > + while (i + 4 <= len) {
> > + val = *((u32 *)(src + i));
> > + writel_relaxed(val, dst + i);
>
> If dst is not aligned to 4, this is going to write to an address that
> is not a multiple of 4, even though it writes a long. Is this ok?
> Typically limitations in write size come with alignment limitations.
>
If dst is not aligned to 4, the first if (!IS_ALIGNED(...)) block
should make that the (dst + i) is a multiple of 4, so the write here
is aligned.
> > + i += 4;
> > + }
> > + if (i < len) {
> > + val = readl_relaxed(dst + i);
> > + memcpy(&val, src + i, len - i);
> > + writel_relaxed(val, dst + i);
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(scp_memcpy_aligned);
>
> IIUC this function's symbol does not need to be exported since it is
> only used in the current kernel module.
>
I've tried to remove this EXPORT line, but then there would be error
while compiling:
ERROR: "scp_memcpy_aligned" [drivers/remoteproc/mtk_scp.ko] undefined!
I think it's because the mtk_scp.c and mtk_scp_ipi.c both use the
scp_memcpy_aligned, but is compiled as separate .o files. So the
EXPORT_SYMBOL is needed.
