Hi Mathieu, thank you for taking the time to go through my patch!
On Wed, Jan 13, 2021 at 12:43 AM Mathieu Poirier <[email protected]> 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
