Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230327043405.881645-1-joychakr@google.com> <20230327043405.881645-3-joychakr@google.com>
 <20230328180338.bpxvjwx4tn4ter3f@mobilestation>
In-Reply-To: <20230328180338.bpxvjwx4tn4ter3f@mobilestation>
From:   Joy Chakraborty <joychakr@google.com>
Date:   Wed, 29 Mar 2023 09:26:47 +0530
Message-ID: <CAOSNQF3r+6Oz=chnA94=4q7pugGFKm2W09ry+pS2F0oe1HZmcA@mail.gmail.com>
Subject: Re: [PATCH v4 2/2] spi: dw: Add dma controller capability checks
To:     Serge Semin <fancer.lancer@gmail.com>
Cc:     Mark Brown <broonie@kernel.org>, linux-spi@vger.kernel.org,
        linux-kernel@vger.kernel.org, manugautam@google.com,
        rohitner@google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Serge(y),

On Tue, Mar 28, 2023 at 11:33=E2=80=AFPM Serge Semin <fancer.lancer@gmail.c=
om> wrote:
>
> On Mon, Mar 27, 2023 at 04:34:05AM +0000, Joy Chakraborty wrote:
> > Check capabilities of DMA controller during init to make sure it is
> > capable of handling MEM2DEV for tx channel, DEV2MEM for rx channel
> > and store addr_width capabilities to check per transfer to make sure th=
e
> > bits/word requirement can be met for that transfer.
> >
> > Signed-off-by: Joy Chakraborty <joychakr@google.com>
> > ---
> >  drivers/spi/spi-dw-dma.c | 54 ++++++++++++++++++++++++++++++++--------
> >  drivers/spi/spi-dw.h     |  1 +
> >  2 files changed, 44 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
> > index b3a88bb75907..f47483ec369f 100644
> > --- a/drivers/spi/spi-dw-dma.c
> > +++ b/drivers/spi/spi-dw-dma.c
> > @@ -23,6 +23,8 @@
> >  #define DW_SPI_TX_BUSY               1
> >  #define DW_SPI_TX_BURST_LEVEL        16
> >
> > +static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes);
> > +
> >  static bool dw_spi_dma_chan_filter(struct dma_chan *chan, void *param)
> >  {
> >       struct dw_dma_slave *s =3D param;
> > @@ -72,12 +74,15 @@ static void dw_spi_dma_maxburst_init(struct dw_spi =
*dws)
> >       dw_writel(dws, DW_SPI_DMATDLR, dws->txburst);
> >  }
> >
> > -static void dw_spi_dma_sg_burst_init(struct dw_spi *dws)
> > +static int dw_spi_dma_caps_init(struct dw_spi *dws)
> >  {
>
> > +     int ret;
> >       struct dma_slave_caps tx =3D {0}, rx =3D {0};
>
> 1. Preserve the reverse xmas tree order please (driver local convention).
> 2. The zero-initialization can be dropped since the function will halt
> on further procedures if any of the dma_get_slave_caps() calls fail.
> Meanwhile if all of them are successfully executed the capability
> structures will be sanely initialized.
>

Sure, will update

> >
> > -     dma_get_slave_caps(dws->txchan, &tx);
> > -     dma_get_slave_caps(dws->rxchan, &rx);
>
> > +     ret =3D dma_get_slave_caps(dws->txchan, &tx);
>         if (ret)
>                 return ret;
> < newline
> > +     ret |=3D dma_get_slave_caps(dws->rxchan, &rx);
> > +     if (ret)
> > +             return ret;
>
> No OR-ing the errnos please. They aren't bitfields.

Sure, I will update this.
In this case do you think we need an error print here to differentiate
which slave caps failed ?

>
> >
> >       if (tx.max_sg_burst > 0 && rx.max_sg_burst > 0)
> >               dws->dma_sg_burst =3D min(tx.max_sg_burst, rx.max_sg_burs=
t);
> > @@ -87,6 +92,18 @@ static void dw_spi_dma_sg_burst_init(struct dw_spi *=
dws)
> >               dws->dma_sg_burst =3D rx.max_sg_burst;
> >       else
> >               dws->dma_sg_burst =3D 0;
> > +
> > +     /*
> > +      * Assuming both channels belong to the same DMA controller hence=
 the
> > +      * address width capabilities most likely would be the same.
> > +      */
> > +     dws->dma_addr_widths =3D tx.dst_addr_widths & rx.src_addr_widths;
> > +
>
> > +     if (!(tx.directions & BIT(DMA_MEM_TO_DEV) &&
> > +           rx.directions & BIT(DMA_DEV_TO_MEM)))
> > +             return -ENXIO;
>
> Please move this to be right after the capabilities are retrieved.
> There is no point in the SG-burst and addr-width data initializations
> if a DMA-controller isn't suitable for the Tx/Rx DMAs.

On second thought I see that dma_get_slave_caps already checks if
direction exists in dmaengine.c:
...
int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
...
    if (!device->directions)
        return -ENXIO;
...
But it does not check the capability w.r.t the type of channel i.e.
tx/rx , so we can keep this check as well.
Either Way, in case we keep it I shall move this as you suggested.

>
> -Serge(y)
>
> > +
> > +     return 0;
> >  }
> >
> >  static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws=
)
> > @@ -95,6 +112,7 @@ static int dw_spi_dma_init_mfld(struct device *dev, =
struct dw_spi *dws)
> >       struct dw_dma_slave dma_rx =3D { .src_id =3D 0 }, *rx =3D &dma_rx=
;
> >       struct pci_dev *dma_dev;
> >       dma_cap_mask_t mask;
> > +     int ret =3D -EBUSY;
> >
> >       /*
> >        * Get pci device for DMA controller, currently it could only
> > @@ -124,20 +142,24 @@ static int dw_spi_dma_init_mfld(struct device *de=
v, struct dw_spi *dws)
> >
> >       init_completion(&dws->dma_completion);
> >
> > -     dw_spi_dma_maxburst_init(dws);
> > +     ret =3D dw_spi_dma_caps_init(dws);
> > +     if (ret)
> > +             goto free_txchan;
> >
> > -     dw_spi_dma_sg_burst_init(dws);
> > +     dw_spi_dma_maxburst_init(dws);
> >
> >       pci_dev_put(dma_dev);
> >
> >       return 0;
> > -
> > +free_txchan:
> > +     dma_release_channel(dws->txchan);
> > +     dws->txchan =3D NULL;
> >  free_rxchan:
> >       dma_release_channel(dws->rxchan);
> >       dws->rxchan =3D NULL;
> >  err_exit:
> >       pci_dev_put(dma_dev);
> > -     return -EBUSY;
> > +     return ret;
> >  }
> >
> >  static int dw_spi_dma_init_generic(struct device *dev, struct dw_spi *=
dws)
> > @@ -163,12 +185,16 @@ static int dw_spi_dma_init_generic(struct device =
*dev, struct dw_spi *dws)
> >
> >       init_completion(&dws->dma_completion);
> >
> > -     dw_spi_dma_maxburst_init(dws);
> > +     ret =3D dw_spi_dma_caps_init(dws);
> > +     if (ret)
> > +             goto free_txchan;
> >
> > -     dw_spi_dma_sg_burst_init(dws);
> > +     dw_spi_dma_maxburst_init(dws);
> >
> >       return 0;
> > -
> > +free_txchan:
> > +     dma_release_channel(dws->txchan);
> > +     dws->txchan =3D NULL;
> >  free_rxchan:
> >       dma_release_channel(dws->rxchan);
> >       dws->rxchan =3D NULL;
> > @@ -202,8 +228,14 @@ static bool dw_spi_can_dma(struct spi_controller *=
master,
> >                          struct spi_device *spi, struct spi_transfer *x=
fer)
> >  {
> >       struct dw_spi *dws =3D spi_controller_get_devdata(master);
> > +     enum dma_slave_buswidth dma_bus_width;
> > +
> > +     if (xfer->len <=3D dws->fifo_len)
> > +             return false;
> > +
> > +     dma_bus_width =3D dw_spi_dma_convert_width(dws->n_bytes);
> >
> > -     return xfer->len > dws->fifo_len;
> > +     return dws->dma_addr_widths & BIT(dma_bus_width);
> >  }
> >
> >  static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes)
> > diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> > index 9e8eb2b52d5c..3962e6dcf880 100644
> > --- a/drivers/spi/spi-dw.h
> > +++ b/drivers/spi/spi-dw.h
> > @@ -190,6 +190,7 @@ struct dw_spi {
> >       struct dma_chan         *rxchan;
> >       u32                     rxburst;
> >       u32                     dma_sg_burst;
> > +     u32                     dma_addr_widths;
> >       unsigned long           dma_chan_busy;
> >       dma_addr_t              dma_addr; /* phy address of the Data regi=
ster */
> >       const struct dw_spi_dma_ops *dma_ops;
> > --
> > 2.40.0.348.gf938b09366-goog
> >

Thanks
Joy