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

The Pensando Elba SoC uses a GPIO based chip select
for two DW SPI busses with each bus having two
chip selects.

Signed-off-by: Brad Larson <brad@pensando.io>
---
 drivers/spi/spi-dw-mmio.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

On Thu, Mar 4, 2021 at 12:48 AM Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, Mar 4, 2021 at 4:42 AM Brad Larson <brad@pensando.io> wrote:
>
> > The Pensando Elba SoC uses a GPIO based chip select
> > for two DW SPI busses with each bus having two
> > chip selects.
> >
> > Signed-off-by: Brad Larson <brad@pensando.io>
>
> I agree with Serge's comments here: the existing cs callback should
> work for your SoC, you should only need the new compatible string.
>
> I see why you need the special GPIO driver for this now, as that
> is obviously driven by totally different hardware.
>
> Yours,
> Linus Walleij

Thanks Serge and Linus for the review.

In the SPI driver, the reason we need our own set_cs function is that
our DW SPI controller only supports intrinsic 2 chip-select pins.

This is the standard DW set_cs function:

void dw_spi_set_cs(struct spi_device *spi, bool enable)
{
        struct dw_spi *dws = spi_controller_get_devdata(spi->controller);
        bool cs_high = !!(spi->mode & SPI_CS_HIGH);

        /*
         * DW SPI controller demands any native CS being set in order to
         * proceed with data transfer. So in order to activate the SPI
         * communications we must set a corresponding bit in the Slave
         * Enable register no matter whether the SPI core is configured to
         * support active-high or active-low CS level.
         */
        if (cs_high == enable)
                dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));
        else
                dw_writel(dws, DW_SPI_SER, 0);
}

The dw_writel function argument DW_SPI_SER, BIT(spi->chip_select)
works for chip-select 0 & 1, but not for 2 & 3, as the IP only
implements bits [1:0] in the DW_SPI_SER register.  In the Elba SoC we
require GPIO-style chip-selects, our own set_cs function, and we
always use bit 0 of DW_SPI_SER to start the serial machine, not as a
chip-select control.  In the dw_spi_set_cs() function the below else
clause is never taken and leads to confusion.

             } else {
                        /*
                         * Using the intrinsic DW chip-select; set the
                         * appropriate CS.
                         */
                        dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));
                }

This else clause will be removed in patch set V2.  I tried the generic
dw_spi_set_cs() thinking it would just start the serial machine while
the Elba spics drives the gpio chip select, that didn't work.  I will
take another look at it as I work on V2 of the patchset to see exactly
what breaks during spi init.

Best,
Brad

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@gmail.com> 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