Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210102205904.2691120-1-martin.blumenstingl@googlemail.com>
 <20210102205904.2691120-5-martin.blumenstingl@googlemail.com> <20210112234330.GA192175@xps15>
In-Reply-To: <20210112234330.GA192175@xps15>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Sat, 16 Jan 2021 22:01:19 +0100
Message-ID: <CAFBinCCfiC9a6u2qAs8-pEUB299C=vHut5=1ZPVPoCs0w-+r-Q@mail.gmail.com>
Subject: Re: [PATCH v2 4/5] remoteproc: meson-mx-ao-arc: Add a driver for the
 AO ARC remote procesor
To:     Mathieu Poirier <mathieu.poirier@linaro.org>
Cc:     linux-remoteproc@vger.kernel.org,
        linux-amlogic@lists.infradead.org, ohad@wizery.com,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        bjorn.andersson@linaro.org, robh+dt@kernel.org,
        linux-arm-kernel@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Mathieu,

thank you for taking the time to go through my patch!

On Wed, Jan 13, 2021 at 12:43 AM Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
[...]
> > +       If unusre say N.
>
> s/unusre/unsure
godo catch, noted.

[...]
> > +#include <linux/property.h>
>
> Is it possible for this to go after platform_device.h?
I think so, not sure why this is not in alphabetical order

[...]
> > +#define AO_CPU_CNTL                                  0x0
> > +     #define AO_CPU_CNTL_MEM_ADDR_UPPER              GENMASK(28, 16)
> > +     #define AO_CPU_CNTL_HALT                        BIT(9)
> > +     #define AO_CPU_CNTL_UNKNONWN                    BIT(8)
> > +     #define AO_CPU_CNTL_RUN                         BIT(0)
>
> Any reason for the extra tabulation at the beginning of the lines?
not really, I think I did the same thing as in
drivers/iio/adc/meson_saradc.c where the register itself starts at the
beginning of the line and each bit(mask) starts indented
I'll change this for the next version

[...]
> > +#define MESON_AO_RPROC_SRAM_USABLE_BITS                      GENMASK(31, 20)
>
> As per your comments in the cover letter I assume we don't know more about this?
unfortunately not, but I'll still try to get some more information
from someone at Amlogic.
That said, this is "legacy" hardware for them so I can't make any promises.

> > +#define MESON_AO_RPROC_MEMORY_OFFSET                 0x10000000
> > +
> > +struct meson_mx_ao_arc_rproc_priv {
> > +     void __iomem            *remap_base;
> > +     void __iomem            *cpu_base;
> > +     unsigned long           sram_va;
> > +     phys_addr_t             sram_pa;
> > +     size_t                  sram_size;
> > +     struct gen_pool         *sram_pool;
> > +     struct reset_control    *arc_reset;
> > +     struct clk              *arc_pclk;
> > +     struct regmap           *secbus2_regmap;
> > +};
> > +
> > +static int meson_mx_ao_arc_rproc_start(struct rproc *rproc)
> > +{
> > +     struct meson_mx_ao_arc_rproc_priv *priv = rproc->priv;
> > +     phys_addr_t phys_addr;
> > +     int ret;
> > +
> > +     ret = clk_prepare_enable(priv->arc_pclk);
> > +     if (ret)
> > +             return ret;
> > +
> > +     writel(0, priv->remap_base + AO_REMAP_REG0);
> > +     usleep_range(10, 100);
>
> That's wonderful - here too I assume there is no indication as to why this is
> needed?
looking at this again: the vendor driver only has a delay after
pulsing the reset line
I will double check and hopefully remove this usleep_range and only
keep the one below (after pulsing the reset line)

[...]
> > +static void *meson_mx_ao_arc_rproc_da_to_va(struct rproc *rproc, u64 da,
> > +                                         size_t len)
> > +{
> > +     struct meson_mx_ao_arc_rproc_priv *priv = rproc->priv;
> > +
> > +     if ((da + len) >= priv->sram_size)
> > +             return NULL;
>
> This isn't an index so it should be '>' rather than '>='.  You should be able to
> ask for the whole range and get it, which the above prevents you from doing.
>
> Moreover are you sure 'da' always starts at 0? This seems to be at odds with
> your comment in meson_mx_ao_arc_rproc_start() about converting from 0xd9000000
> to 0xc9000000.
thanks for both of these comments, I'll address this in the next version

[...]
> > +     priv->arc_reset = devm_reset_control_get_exclusive(dev, NULL);
> > +     if (IS_ERR(priv->arc_reset)) {
>
> Looking at __devm_reset_control_get(), this should probably be IS_ERR_OR_NULL().
as far as I know only devm_reset_control_get_optional_exclusive (the
important bit is "optional" - I am using the "mandatory/not optional"
variant) can return NULL, all other variants return PTR_ERR or a valid
reset_control.


Best regards,
Martin