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: <20230330063450.2289058-1-joychakr@google.com> <20230330063450.2289058-3-joychakr@google.com>
 <ZDVQGu/gBPTNbQPU@smile.fi.intel.com> <CAOSNQF0R7p7kW04w0nKF2egv1+8NrMhSXpggunYhjQnA+v-qMQ@mail.gmail.com>
In-Reply-To: <CAOSNQF0R7p7kW04w0nKF2egv1+8NrMhSXpggunYhjQnA+v-qMQ@mail.gmail.com>
From:   Joy Chakraborty <joychakr@google.com>
Date:   Tue, 11 Apr 2023 20:37:33 +0530
Message-ID: <CAOSNQF2zg5ymTfZWWbFLAgvKdcxRcggAjGt+Zy+qUzrR5=MERw@mail.gmail.com>
Subject: Re: [PATCH v5 2/2] spi: dw: Add dma controller capability checks
To:     Andy Shevchenko <andriy.shevchenko@intel.com>
Cc:     Serge Semin <fancer.lancer@gmail.com>,
        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

Sorry I think the emails crossed.

So as per the discussion, are these the possible changes:
1> Move "dw_spi_dma_convert_width" to avoid forward declaration .
2> Update the commit text to include more explanation.
3> Divide this into 2 patches?

Thanks
Joy

Joy

On Tue, Apr 11, 2023 at 8:30=E2=80=AFPM Joy Chakraborty <joychakr@google.co=
m> wrote:
>
> Hello Andy,
>
> On Tue, Apr 11, 2023 at 5:48=E2=80=AFPM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> >
> > On Thu, Mar 30, 2023 at 06:34:50AM +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 =
the
> > > bits/word requirement can be met for that transfer.
> >
> > ...
> >
> > > +static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes);
> >
> > Can we avoid forward declarations please?
>
> We can, but for that I would have to move this api somewhere else in
> the file is that acceptable?
>
> >
> > ...
> >
> > > +     if (!(tx.directions & BIT(DMA_MEM_TO_DEV) &&
> > > +           rx.directions & BIT(DMA_DEV_TO_MEM)))
> > > +             return -ENXIO;
> >
> > What about simplex transfers where we only care about sending or receiv=
ing data
> > and using dummy data for the other channel? Doesn't this make a regress=
ion for
> > that types of transfers? (Or, if we don't support such, this should be =
explained
> > in the commit message at least.)
> >
>
> Yes we can have simplex transfers for TX only, but for RX only there
> is dummy data added by the SPI core as the dw-core driver adds
> "SPI_CONTROLLER_MUST_TX".
>
> But transfers aside, as per the current driver design the dw dma
> driver needs both valid tx and rx channels to exist and be functional
> as per implementation of api "dw_spi_dma_init_generic" :
> ...
> dws->rxchan =3D dma_request_chan(dev, "rx");
> if (IS_ERR(dws->rxchan)) {
> ret =3D PTR_ERR(dws->rxchan);
> dws->rxchan =3D NULL;
> goto err_exit;
> }
>
> dws->txchan =3D dma_request_chan(dev, "tx");
> if (IS_ERR(dws->txchan)) {
> ret =3D PTR_ERR(dws->txchan);
> dws->txchan =3D NULL;
> goto free_rxchan;
> }
> ...
>
> Hence in terms of capability check we should check for directional
> capability of both TX and RX is what I understand.
> Please let me know if you think otherwise.
>
> > ...
> >
> > > +     /*
> > > +      * Assuming both channels belong to the same DMA controller hen=
ce the
> > > +      * address width capabilities most likely would be the same.
> > > +      */
> > > +     dws->dma_addr_widths =3D tx.dst_addr_widths & rx.src_addr_width=
s;
> >
> > I don't think so this is correct.
> >
> > Theoretically it's possible to have simplex transfers on which the one =
of
> > the channel is simply ignored / not used. See above.
> >
>
> Yes, it is possible to have tx only transfers which would still be
> possible to do with this implementation. What we have assumed here is
> since the tx and rx channel both are a requirement for the dw dma
> driver to be functional it would most likely have the same address
> width capability.
>
> But we can make this more elaborate by checking it for both tx and rx
> separately.
> Something like this
> ...
>  dws->tx_dma_addr_widths =3D tx.dst_addr_widths;
>  dws->rx_dma_addr_widths =3D rx.src_addr_widths;
> ...
> ...
> static bool dw_spi_can_dma(struct spi_controller *master,
> struct spi_device *spi, struct spi_transfer *xfer)
> {
> 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);
>
> if (xfer->rx_buf && !(dws->rx_dma_addr_widths & BIT(dma_bus_width)))
> return false;
>
> return dws->tx_dma_addr_widths & BIT(dma_bus_width);
> }
> ...
>
> @Serge Semin Please share your thoughts on the same.
>
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
>
> I shall break this into 2 patches as per Serge(y)'s recommendation and
> make changes as per replies.
>
> Thanks
> Joy