Messages in this thread | | | From | "Lad, Prabhakar" <> | Date | Fri, 31 Mar 2023 20:17:39 +0000 | Subject | Re: [PATCH v7 5/6] cache: Add L2 cache management for Andes AX45MP RISC-V core |
| |
Hi Conor,
Thank you for the review.
On Fri, Mar 31, 2023 at 1:45 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > On Thu, Mar 30, 2023 at 09:42:16PM +0100, Prabhakar wrote: > > > +STANDALONE CACHE CONTROLLER DRIVERS > > > +F: include/cache > > This can go since the file no longer exists. > Agreed I will drop this.
> > +config AX45MP_L2_CACHE > > + bool "Andes Technology AX45MP L2 Cache controller" > > + depends on RISCV && RISCV_DMA_NONCOHERENT > > This can just be depends on RISCV_DMA_NONCOHERENT, since that's only > defined on RISC-V. > Agreed.
> > +static void ax45mp_get_l2_line_size(struct platform_device *pdev) > > +{ > > + struct device_node *np = pdev->dev.of_node; > > + struct device *dev = &pdev->dev; > > + int ret; > > + > > + ret = of_property_read_u32(np, "cache-line-size", &ax45mp_priv->ax45mp_cache_line_size); > > + if (ret) { > > + dev_err(dev, "Failed to get cache-line-size, defaulting to 64 bytes\n"); > > + ax45mp_priv->ax45mp_cache_line_size = AX45MP_CACHE_LINE_SIZE; > > + } > > + > > + if (ax45mp_priv->ax45mp_cache_line_size != AX45MP_CACHE_LINE_SIZE) { > > + dev_err(dev, "Expected cache-line-size to be 64 bytes (found:%u). Defaulting to 64 bytes\n", > > + ax45mp_priv->ax45mp_cache_line_size); > > + ax45mp_priv->ax45mp_cache_line_size = AX45MP_CACHE_LINE_SIZE; > > + } > > I forget, why are we doing this defaulting rather than falling over > immediately if we detect the property is missing or wrong? > No reason as such on not failing on property not existing/Invalid. I will bail out in an error case now.
> > +} > > > +static const struct riscv_cache_ops ax45mp_cmo_ops = { > > + .clean_range = &ax45mp_cpu_dma_wb_range, > > + .inv_range = &ax45mp_cpu_dma_inval_range, > > + .flush_range = &ax45mp_cpu_dma_flush_range, > > +}; > > I think it would be nice if your driver functions matched the names used > by the ops. (and as I said on the other patch, I think the ops should > match the cross-arch naming. > Agreed, will do.
> Otherwise, looks grand - although I think I was mostly happy with the > last revision too.a > I know you had provided the RB for the last version ;)
Cheers, Prabhakar
| |