Re: [PATCH 1/8] gpio: Add Elba SoC gpio driver for spi cs control

From: Serge Semin <fancer.lancer@gmail.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Brad Larson <brad@pensando.io>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Arnd Bergmann <arnd@arndb.de>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Mark Brown <broonie@kernel.org>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	Olof Johansson <olof@lixom.net>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	linux-spi <linux-spi@vger.kernel.org>,
	linux-mmc <linux-mmc@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/8] gpio: Add Elba SoC gpio driver for spi cs control
Date: Thu, 4 Mar 2021 12:10:25 +0300	[thread overview]
Message-ID: <20210304091025.ny52qjm7wbfvmjgl@mobilestation> (raw)
In-Reply-To: <CACRpkdbQD6p7fbGtuu1c92uXfSFDCTwqjqsXHpgnD5Lg4v0Okw@mail.gmail.com>

Hello Linus,

I started reviewing from the DW APB SPI driver part of this series,
that's why I suggested to remove the CS callback from there seeing it
doesn't really differ much from the generic one. But after looking at
the dts file and in this driver I think that the alterations layout
needs to be a bit different.

This module looks more like being a part of a SoC System Controller
seeing it's just a single register. Corresponding pins seem like
being multiplexed between SPI controller and GPO (being directly driven
by setting a bit in the corresponding register). See the next comment.

On Thu, Mar 04, 2021 at 09:29:33AM +0100, Linus Walleij wrote:
> Hi Brad,
> 
> thanks for your patch!
> 
> On Thu, Mar 4, 2021 at 4:42 AM Brad Larson <brad@pensando.io> wrote:
> 
> > This GPIO driver is for the Pensando Elba SoC which
> > provides control of four chip selects on two SPI busses.
> >
> > Signed-off-by: Brad Larson <brad@pensando.io>
> (...)
> 
> > +#include <linux/gpio.h>
> 
> Use this in new drivers:
> #include <linux/gpio/driver.h>
> 
> > + * pin:             3            2        |       1            0
> > + * bit:         7------6------5------4----|---3------2------1------0
> > + *             cs1  cs1_ovr  cs0  cs0_ovr |  cs1  cs1_ovr  cs0  cs0_ovr
> > + *                        ssi1            |             ssi0
> > + */
> > +#define SPICS_PIN_SHIFT(pin)   (2 * (pin))
> > +#define SPICS_MASK(pin)                (0x3 << SPICS_PIN_SHIFT(pin))
> > +#define SPICS_SET(pin, val)    ((((val) << 1) | 0x1) << SPICS_PIN_SHIFT(pin))
> 

> So 2 bits per GPIO line in one register? (Nice doc!)

I suppose the first bit is the CS-pin-override flag. So when it's set
the output is directly driven by the second bit, otherwise the
corresponding DW APB SPI controller drives it. That's how the
multiplexing is implemented here.

> 
> > +struct elba_spics_priv {
> > +       void __iomem *base;
> > +       spinlock_t lock;
> > +       struct gpio_chip chip;
> > +};
> > +
> > +static int elba_spics_get_value(struct gpio_chip *chip, unsigned int pin)
> > +{
> > +       return -ENXIO;
> > +}
> 
> Write a comment that the chip only supports output mode,
> because it repurposes SPI CS pins as generic GPIO out,
> maybe at the top of the file?
> 

> I suppose these systems also actually (ab)use the SPI cs
> for things that are not really SPI CS?

I haven't noticed that in the dts file submitted by Brad. So most
likely these are just CS pins, which can be either automatically
driven by the DW APB SPI controller (yeah, DW APB SPI controller
doesn't provide a way to directly set he native CS value, it
sets the CS value low automatically when starts SPI xfers) or can be
manually set low/high by means of that SPI-CS register.

> Because otherwise
> this could just be part of the SPI driver (native chip select).

That's what I suggested in my comment to the patch
[PATCH 7/8] arm64: dts: Add Pensando Elba SoC support
in this series. Although imho it's better to be done by means
of a System Controller.

-Sergey

> 
> > +static const struct of_device_id ebla_spics_of_match[] = {
> > +       { .compatible = "pensando,elba-spics" },
> 
> Have you documented this?
> 
> Other than that this is a nice and complete driver.
> 
> Yours,
> Linus Walleij