Hi
> On Fri, Jun 07, 2024 at 08:18:29AM -0700, Witold Sadowski wrote:
> > This commit adds support for the basic v2 Marvell overlay block. Key
> > features included are:
> > - Clock configuration
> > - PHY configuration
> > - Interrupt configuration (enabling)
>
> This feels like it could usefully be split up so these three bits are
> separate, and there appear to be other changes buried in here as well.
> I can't tell what changes either the PHY or interrupt configuration might
> be referencing.
That changes are in single commit as, using not all of them will result in
total xSPI failure. Configuring PHY makes no sense if clock is not enabled.
But I can try to split that into 3 separate commits.
>
> > @@ -295,6 +450,10 @@ static void cdns_xspi_set_interrupts(struct
> cdns_xspi_dev *cdns_xspi,
> > bool enabled)
> > {
> > u32 intr_enable;
> > + u32 irq_status;
> > +
> > + irq_status = readl(cdns_xspi->iobase + CDNS_XSPI_INTR_STATUS_REG);
> > + writel(irq_status, cdns_xspi->iobase + CDNS_XSPI_INTR_STATUS_REG);
> >
> > intr_enable = readl(cdns_xspi->iobase + CDNS_XSPI_INTR_ENABLE_REG);
> > if (enabled)
>
> This seems like a separate change which applies to everything, not just
> Marvell versions of the IP, and should be split out and explained.
I will move that to separate commit too. It is possible that previous
stage will not clear that register correctly, and that will lead to
interrupt fail - at least in Marvell implementation.
>
> > @@ -319,6 +478,9 @@ static int cdns_xspi_controller_init(struct
> cdns_xspi_dev *cdns_xspi)
> > return -EIO;
> > }
> >
> > + writel(FIELD_PREP(CDNS_XSPI_CTRL_WORK_MODE,
> CDNS_XSPI_WORK_MODE_STIG),
> > + cdns_xspi->iobase + CDNS_XSPI_CTRL_CONFIG_REG);
> > +
>
> This wasn't clearly mentioned in the changelog and is again being done
> unconditionally for all instances of the IP, probably best to split out
> and explain.
Ok, I can move to separate commit too.
>
> > +static void mrvl_ioreadq(void __iomem *addr, void *buf, int len) {
> > + int i = 0;
> > + int rcount = len / 8;
> > + int rcount_nf = len % 8;
> > + uint64_t tmp;
> > + uint64_t *buf64 = (uint64_t *)buf;
>
> Any need to cast away from void * indicates a problem.
I will check that, but code is checking alignment of that pointer.
>
> > @@ -337,13 +563,11 @@ static void cdns_xspi_sdma_handle(struct
> > cdns_xspi_dev *cdns_xspi)
> >
> > switch (sdma_dir) {
> > case CDNS_XSPI_SDMA_DIR_READ:
> > - ioread8_rep(cdns_xspi->sdmabase,
> > - cdns_xspi->in_buffer, sdma_size);
> > + cdns_xspi_sdma_memread(cdns_xspi, sdma_size);
> > break;
>
> It's feeling like it might make sense to have an ops structure rather than
> sprinkling checks for the Marvell overlay everywhere.
Won't it cause big code duplication? There are some differences, but whole
Part of SPI stig mode configuration is the same.
Regards
Witek
