Re: [PATCH 3/8] spi: dw: Add support for Pensando Elba SoC SPI

[Brad Larson]

Hi Sergey,

Thanks again for the reviews.  I've been able to work on this recently
and test the changes using 5.10.28 on a production server.  I'm going
back to the beginning to reply to each comment and work towards
closure of open issues before preparing patchset v3 which will need to
be re-done against the latest linux-next.

On Wed, Mar 3, 2021 at 10:44 PM Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
>
> Hello Brad.
> Thanks for the patch. See my comments below.
>
> On Wed, Mar 03, 2021 at 07:41:36PM -0800, Brad Larson wrote:
> > The Pensando Elba SoC uses a GPIO based chip select
> > for two DW SPI busses with each bus having two
> > chip selects.
>
> I see a contradiction here. Normally GPIO-based chip-select is a
> property of a platform, but not a SoC/CPU/MCU/etc. Most of the time
> SoC SPI interfaces still get to have native CS pins, while at some
> platform configurations (like in case of DW APB SPI, which doesn't
> provide a way to directly toggle its native CSs) it's easier or even
> safer to use GPIOs as CS signals. Of course theoretically a SoC could
> be synthesized so it doesn't have native CS output pins, but only some
> virtual internal CS flags, but I've never seen such. Anyway according
> to the custom CS method below it's not your case because you still
> provide support for SPI-devices handled by native CS (else branch in
> the if (spi->cs_gpiod) {} else {} statement).

The native DW CS is not supported, that code is removed which caused
the confusion.  The existing dw_spi_set_cs() works fine with the
updated version of this function being

/*
 * Using a GPIO-based chip-select, the DW SPI controller still needs
 * its own CS bit selected to start the serial engine.  On Elba the
 * specific CS doesn't matter, so use CS0.
 */
static void dw_spi_elba_set_cs(struct spi_device *spi, bool enable)
{
        spi->chip_select = 0;
        dw_spi_set_cs(spi, enable);
}

which is much better than the original version shown below

> > +static void dw_spi_elba_set_cs(struct spi_device *spi, bool enable)
> > +{
> > +     struct dw_spi *dws = spi_master_get_devdata(spi->master);
> > +
> > +     if (!enable) {
> > +             if (spi->cs_gpiod) {
> > +                     /*
> > +                      * Using a GPIO-based chip-select, the DW SPI
> > +                      * controller still needs its own CS bit selected
> > +                      * to start the serial engine.  On Elba the specific
> > +                      * CS doesn't matter, so use CS0.
> > +                      */
> > +                     dw_writel(dws, DW_SPI_SER, BIT(0));
> > +             } else {
> > +                     /*
> > +                      * Using the intrinsic DW chip-select; set the
> > +                      * appropriate CS.
> > +                      */
> > +                     dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));
> > +             }
> > -     } else
>   +     } else {
> > +             dw_writel(dws, DW_SPI_SER, 0);
>   +     } /* See [1] */
> > +}
>
> The custom CS-method above doesn't look much different from the
> dw_spi_set_cs() method defined in the spi-dw-core.o driver, except
> having at least two problems:
> 1) It assumes that "enable" argument means the CS-enabling flag, while
> in fact it's the CS-level which depending on the SPI_CS_HIGH flag
> set/cleared will be 1/0 respectively if CS is supposed to be enabled.
> That aspect has already been fixed in the dw_spi_set_cs() method.
> 2) The method enables CS[0] if GPIO-CS is used for a particular SPI
> device. That will cause problems for a GPIO/native CS intermixed case
> of having for instance one SPI-device connected to native CS[0] and
> another one - to a GPIO. So trying to communicate with the second SPI
> device you'll end up having the native CS[0] activated too thus
> having an SPI transfer sent to two SPI-device at the same time.
> Of course that's not what you'd want.
>
> Anyway I don't really see why you even need a custom CS method here. A
> generic method dw_spi_set_cs() shall work for your SPI interface.
> If I am wrong, please explain why. Did you try the generic CS method
> on your platform?
>
> [1] Placing Braces and Spaces. Chapter 3). Documentation/process/coding-style.rst

Yes, exactly.  The generic method dw_spi_set_cs() works ok and
correctly handles active high/low.

> > +static int dw_spi_elba_init(struct platform_device *pdev,
> > +                         struct dw_spi_mmio *dwsmmio)
> > +{
> > +     dwsmmio->dws.set_cs = dw_spi_elba_set_cs;
> > +
> > +     return 0;
> > +}
> > +
> >  static int dw_spi_mmio_probe(struct platform_device *pdev)
> >  {
> >       int (*init_func)(struct platform_device *pdev,
> > @@ -351,6 +383,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
> >       { .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
> >       { .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
> >       { .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
>
> > +     { .compatible = "pensando,elba-spi", .data = dw_spi_elba_init },
>
> If you agree with me and remove the custom CS-method defined above in
> this patch, then all you'll need is just to add "pensando,elba-spi" here
> with generic init-callback set - dw_spi_dw_apb_init.

The existing dw_spi_set_cs() is now being used.  Using
dw_spi_dw_apb_init results in every spi transfer failing which is why
dw_spi_elba_init() is still proposed which results in set_cs calling
dw_spi_elba_set_cs().

> Finally defining new compatible string requires the bindings update.
> In the framework of DW APB SPI interface they are defined in:
> Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> So you need to have that DT-schema accordingly altered.
>
> The bindings note concerns the rest of the updates in your patchset too.
>
> -Sergey

Patchset v2 separated out the bindings updates.  There will be more
bindings needed for v3 of the patchset.  I won't be sending v3 until
all discussions are resolved.

Regards,
Brad