| Message ID | 20210304034141.7062-1-brad@pensando.io |
|---|---|
| Headers | show |
| Series | Support Pensando Elba SoC | expand |
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). > > Signed-off-by: Brad Larson <brad@pensando.io> > --- > drivers/spi/spi-dw-mmio.c | 35 ++++++++++++++++++++++++++++++++++- > 1 file changed, 34 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c > index 17c06039a74d..417bd2125c07 100644 > --- a/drivers/spi/spi-dw-mmio.c > +++ b/drivers/spi/spi-dw-mmio.c > @@ -56,7 +56,7 @@ struct dw_spi_mscc { > /* > * The Designware SPI controller (referred to as master in the documentation) > * automatically deasserts chip select when the tx fifo is empty. The chip > - * selects then needs to be either driven as GPIOs or, for the first 4 using the > + * selects then needs to be either driven as GPIOs or, for the first 4 using > * the SPI boot controller registers. the final chip select is an OR gate > * between the Designware SPI controller and the SPI boot controller. > */ > @@ -237,6 +237,38 @@ static int dw_spi_canaan_k210_init(struct platform_device *pdev, > return 0; > } > > +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 > + > +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. 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 > { /* end of table */} > }; > MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match); > -- > 2.17.1 >
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!) > +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? Because otherwise this could just be part of the SPI driver (native chip select). > +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
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
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
On Thu, Mar 4, 2021 at 4:41 AM Brad Larson <brad@pensando.io> wrote: > > Add QSPI controller support fo Pensando Elba SoC. > > Signed-off-by: Brad Larson <brad@pensando.io> > --- > drivers/spi/spi-cadence-quadspi.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c > index 442cc7c53a47..fb0d9b0bd596 100644 > --- a/drivers/spi/spi-cadence-quadspi.c > +++ b/drivers/spi/spi-cadence-quadspi.c > @@ -1353,6 +1353,7 @@ static int cqspi_request_mmap_dma(struct cqspi_st *cqspi) > cqspi->rx_chan = dma_request_chan_by_mask(&mask); > if (IS_ERR(cqspi->rx_chan)) { > int ret = PTR_ERR(cqspi->rx_chan); > + > cqspi->rx_chan = NULL; > return dev_err_probe(&cqspi->pdev->dev, ret, "No Rx DMA available\n"); > } Please don't mix whitespace changes with code changes. > @@ -1632,6 +1633,10 @@ static const struct cqspi_driver_platdata intel_lgm_qspi = { > .quirks = CQSPI_DISABLE_DAC_MODE, > }; > > +static const struct cqspi_driver_platdata pen_cdns_qspi = { > + .quirks = CQSPI_NEEDS_WR_DELAY | CQSPI_DISABLE_DAC_MODE, > +}; > + > static const struct of_device_id cqspi_dt_ids[] = { > { > .compatible = "cdns,qspi-nor", > @@ -1649,6 +1654,10 @@ static const struct of_device_id cqspi_dt_ids[] = { > .compatible = "intel,lgm-qspi", > .data = &intel_lgm_qspi, > }, > + { > + .compatible = "pensando,cdns-qspi", > + .data = &pen_cdns_qspi, > + }, > { /* end of table */ } As mentioned in my reply to the dts file, the compatible string needs to be somewhat more specific. I also wonder if it would be better to define separate DT properties for the quirks at this point, so not every new SoC using this device needs to have its own quirks definition. Arnd
On Thu, Mar 4, 2021 at 4:41 AM Brad Larson <brad@pensando.io> wrote: > > Pensando Elba SoC platforms have a SPI connected CPLD > for platform management. > > Signed-off-by: Brad Larson <brad@pensando.io> > --- > drivers/spi/spidev.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c > index 8cb4d923aeaa..8b285852ce82 100644 > --- a/drivers/spi/spidev.c > +++ b/drivers/spi/spidev.c > @@ -683,6 +683,7 @@ static const struct of_device_id spidev_dt_ids[] = { > { .compatible = "dh,dhcom-board" }, > { .compatible = "menlo,m53cpld" }, > { .compatible = "cisco,spi-petra" }, > + { .compatible = "pensando,cpld" }, > {}, > }; This does not seem appropriate, I think a platform management driver should have a proper kernel abstraction instead of a user passthrough. As mentioned elsewhere, it also needs to be way more specific. If this is a programmable block, the compatible string might in fact need to contain both a board identifier and a revision number for the programmable logic, to ensure that the driver knows how to talk to it. Arnd
On Thu, Mar 4, 2021 at 4:41 AM Brad Larson <brad@pensando.io> wrote: > + > +static void elba_write_l(struct sdhci_host *host, u32 val, int reg) > +{ > + struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host); > + unsigned long flags; > + > + spin_lock_irqsave(&priv->wrlock, flags); > + writel(0x78, priv->ctl_addr); > + writel(val, host->ioaddr + reg); > + spin_unlock_irqrestore(&priv->wrlock, flags); > +} Please be aware that the spinlock does not actually guarantee serializing a series of mmio writes unless the MMIO mapping is non-posted, or you follow it with a readl() from the same device before the spin_unlock(). > @@ -453,8 +441,14 @@ static const struct dev_pm_ops sdhci_cdns_pm_ops = { > static const struct of_device_id sdhci_cdns_match[] = { > { > .compatible = "socionext,uniphier-sd4hc", > - .data = &sdhci_cdns_uniphier_pltfm_data, > + .data = &sdhci_cdns_uniphier_drv_data, > }, > +#ifdef CONFIG_MMC_SDHCI_CADENCE_ELBA > + { > + .compatible = "pensando,elba-emmc", > + .data = &sdhci_elba_drv_data > + }, > +#endif > { .compatible = "cdns,sd4hc" }, > { /* sentinel */ } > }; This introduces a reverse dependency between the modules, which will cause problems at link time depending on how you configure it. There are two ways to avoid this: a) the simple method is to always link every backend into the driver module, usually leaving them all enabled at compile time. b) once this gets out of hand because there are too many variants, or the differences between them are too big, you refactor the common code into a library module that just exports a functions but has no module_init() itself, plus a front-end driver for each variant, which now calls into the common code rather than being called by the common code. Arnd
On Thu, Mar 4, 2021 at 4:41 AM Brad Larson <brad@pensando.io> wrote: > > Add ARCH_PENSANDO configuration option for Pensando SoC > based platforms. > > Signed-off-by: Brad Larson <brad@pensando.io> The changelog and the platform help text could use a little more information about what that platform is and where to find more information. This will help users decide whether they should enable support for the platform or not. Arnd
On Thu, Mar 4, 2021 at 10:10 AM Serge Semin <fancer.lancer@gmail.com> wrote: > On Thu, Mar 04, 2021 at 09:29:33AM +0100, Linus Walleij wrote: > > > + * 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. If these output lines are so tightly coupled to the SPI block and will not be used for any other GPO (general purpose output) I think it makes more sense to bundle the handling into the DW SPI driver, and activate it based on the Elba compatible string (if of_is_compatible(...)). I am a bit cautious because it has happened in the past that people repurpose CS lines who were originally for SPI CS to all kind of other purposes, such as a power-on LED and in that case it needs to be a separate GPIO driver. So the author needs to have a good idea about what is a realistic use case here. Yours, Linus Walleij
> -----Original Message----- > From: Brad Larson <brad@pensando.io> > Sent: Wednesday, March 3, 2021 9:42 PM > Subject: [PATCH 1/8] gpio: Add Elba SoC gpio driver for spi cs control . > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig ... > +config GPIO_ELBA_SPICS > + bool "Pensando Elba SPI chip-select" > + depends on ARCH_PENSANDO_ELBA_SOC > + help > + Say yes here to support the Pensndo Elba SoC SPI chip-select > driver Pensndo should be Pensando > diff --git a/drivers/gpio/gpio-elba-spics.c b/drivers/gpio/gpio-elba-spics.c > + * Pensando Elba ASIC SPI chip select driver ... > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("Elba SPI chip-select driver"); I think it's conventional to include the company name there, so start that with "Pensando Elba" Also, "SoC" and "ASIC" are sometimes included after Elba, but sometimes are not. Consistency might be helpful. > +static const struct of_device_id ebla_spics_of_match[] = { ... > + .of_match_table = ebla_spics_of_match, ebla should be elba
On 04/03/2021 04:41, Brad Larson 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> > --- > drivers/gpio/Kconfig | 6 ++ > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-elba-spics.c | 120 +++++++++++++++++++++++++++++++++ > 3 files changed, 127 insertions(+) > create mode 100644 drivers/gpio/gpio-elba-spics.c (...) > +static int elba_spics_probe(struct platform_device *pdev) > +{ > + struct elba_spics_priv *p; > + struct resource *res; > + int ret; > + > + p = devm_kzalloc(&pdev->dev, sizeof(*p), GFP_KERNEL); > + if (!p) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + p->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(p->base)) { > + dev_err(&pdev->dev, "failed to remap I/O memory\n"); > + return PTR_ERR(p->base); > + } > + spin_lock_init(&p->lock); > + platform_set_drvdata(pdev, p); > + > + p->chip.ngpio = 4; /* 2 cs pins for spi0, and 2 for spi1 */ > + p->chip.base = -1; > + p->chip.direction_input = elba_spics_direction_input; > + p->chip.direction_output = elba_spics_direction_output; > + p->chip.get = elba_spics_get_value; > + p->chip.set = elba_spics_set_value; > + p->chip.label = dev_name(&pdev->dev); > + p->chip.parent = &pdev->dev; > + p->chip.owner = THIS_MODULE; > + > + ret = devm_gpiochip_add_data(&pdev->dev, &p->chip, p); > + if (ret) { > + dev_err(&pdev->dev, "unable to add gpio chip\n"); > + return ret; > + } > + > + dev_info(&pdev->dev, "elba spics registered\n"); Don't print trivial probe results, unless you print here something useful. If you need it for debugging, keep it dev_dbg. Best regards, Krzysztof
Hi Brad, On Thu, Mar 4, 2021 at 4:59 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> Thanks for your patch! > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -241,6 +241,12 @@ config GPIO_EIC_SPRD > help > Say yes here to support Spreadtrum EIC device. > > +config GPIO_ELBA_SPICS > + bool "Pensando Elba SPI chip-select" > + depends on ARCH_PENSANDO_ELBA_SOC Any specific reason this can't be "... || COMPILE_TEST"? > + help > + Say yes here to support the Pensndo Elba SoC SPI chip-select driver > + > config GPIO_EM > tristate "Emma Mobile GPIO" > depends on (ARCH_EMEV2 || COMPILE_TEST) && OF_GPIO Gr{oetje,eeting}s, Geert
On Thu, Mar 4, 2021 at 4:40 PM 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. I will try to avoid repeating otheris in their reviews, but my comments below. ... > +config GPIO_ELBA_SPICS > + bool "Pensando Elba SPI chip-select" Can't it be a module? Why? > + depends on ARCH_PENSANDO_ELBA_SOC > + help > + Say yes here to support the Pensndo Elba SoC SPI chip-select driver Please give more explanation what it is and why users might need it, and also tell users how the module will be named (if there is no strong argument why it can't be a module). ... > +#include <linux/of.h> It's not used here, but you missed mod_devicetable.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)) Isn't it easier to define as ((value) << (2 * (pin) + 1) | BIT(2 * (pin))) ... > +struct elba_spics_priv { > + void __iomem *base; > + spinlock_t lock; > + struct gpio_chip chip; If you put it as a first member a container_of() becomes a no-op. OTOH dunno if there is any such container_of() use in the code. > +}; ... > +static int elba_spics_get_value(struct gpio_chip *chip, unsigned int pin) > +{ > + return -ENXIO; Hmm... Is it really acceptable error code here? > +} ... > +static int elba_spics_direction_input(struct gpio_chip *chip, unsigned int pin) > +{ > + return -ENXIO; Ditto. > +} ... > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + p->base = devm_ioremap_resource(&pdev->dev, res); p->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(p->base)) { > + dev_err(&pdev->dev, "failed to remap I/O memory\n"); Duplicate noisy message. > + return PTR_ERR(p->base); > + } > + ret = devm_gpiochip_add_data(&pdev->dev, &p->chip, p); > + if (ret) { > + dev_err(&pdev->dev, "unable to add gpio chip\n"); > + return ret; > + } > + > + dev_info(&pdev->dev, "elba spics registered\n"); > + return 0; if (ret) dev_err(...); return ret; > +}
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
On Sun, Mar 7, 2021 at 11:21 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Mar 4, 2021 at 4:40 PM 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. > > > +config GPIO_ELBA_SPICS > > + bool "Pensando Elba SPI chip-select" > > Can't it be a module? Why? All Elba SoC based platforms require this driver to be built-in to boot and removing the module would result in a variety of exceptions/errors. > > + depends on ARCH_PENSANDO_ELBA_SOC > > + help > > + Say yes here to support the Pensndo Elba SoC SPI chip-select driver > > Please give more explanation what it is and why users might need it, > and also tell users how the module will be named (if there is no > strong argument why it can't be a module). > Fixed the typo. > > +#include <linux/of.h> > > It's not used here, but you missed mod_devicetable.h. Removed <linux/of.h>. There is no dependency on mod_devicetable.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)) > > Isn't it easier to define as ((value) << (2 * (pin) + 1) | BIT(2 * (pin))) > > ... > > > +struct elba_spics_priv { > > + void __iomem *base; > > + spinlock_t lock; > > > + struct gpio_chip chip; > > If you put it as a first member a container_of() becomes a no-op. OTOH > dunno if there is any such container_of() use in the code. > There is no use of container_of() > > +static int elba_spics_get_value(struct gpio_chip *chip, unsigned int pin) > > +{ > > + return -ENXIO; > > Hmm... Is it really acceptable error code here? > > > +static int elba_spics_direction_input(struct gpio_chip *chip, unsigned int pin) > > +{ > > + return -ENXIO; > > Ditto. > Changed both to -ENOTSUPP. > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + p->base = devm_ioremap_resource(&pdev->dev, res); > > p->base = devm_platform_ioremap_resource(pdev, 0); Implementation follows devm_ioremap_resource() example in lib/devres.c. > > + if (IS_ERR(p->base)) { > > > + dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > Duplicate noisy message. > > > + return PTR_ERR(p->base); > > + } > > > + ret = devm_gpiochip_add_data(&pdev->dev, &p->chip, p); > > + if (ret) { > > + dev_err(&pdev->dev, "unable to add gpio chip\n"); > > > + return ret; > > + } > > + > > + dev_info(&pdev->dev, "elba spics registered\n"); > > + return 0; > > if (ret) > dev_err(...); > return ret; Cleaned this up in patchset v2.
On Mon, Mar 29, 2021 at 4:19 AM Brad Larson <brad@pensando.io> wrote: > On Sun, Mar 7, 2021 at 11:21 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Thu, Mar 4, 2021 at 4:40 PM Brad Larson <brad@pensando.io> wrote: ... > > > +config GPIO_ELBA_SPICS > > > + bool "Pensando Elba SPI chip-select" > > > > Can't it be a module? Why? > > All Elba SoC based platforms require this driver to be built-in to boot and > removing the module would result in a variety of exceptions/errors. Needs to be at least in the commit message. > > > + depends on ARCH_PENSANDO_ELBA_SOC > > > + help > > > + Say yes here to support the Pensndo Elba SoC SPI chip-select driver > > > > Please give more explanation what it is and why users might need it, > > and also tell users how the module will be named (if there is no > > strong argument why it can't be a module). > > > Fixed the typo. Yeah, according to the above, you better elaborate what this module is and why people would need it. Also can be a good hint to add default ARCH_MY_COOL_PLATFORM ... > > > +#include <linux/of.h> > > > > It's not used here, but you missed mod_devicetable.h. > > Removed <linux/of.h>. There is no dependency on mod_devicetable.h. What do you mean? You don't use data structures from that? of_device_id or other ID structures are defined there. Your module works without them? ... > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + p->base = devm_ioremap_resource(&pdev->dev, res); > > > > p->base = devm_platform_ioremap_resource(pdev, 0); > > Implementation follows devm_ioremap_resource() example in lib/devres.c. So? How does this make it impossible to address my comment? > > > + if (IS_ERR(p->base)) { > > > > > + dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > > Duplicate noisy message. > > > > > + return PTR_ERR(p->base); > > > + }
On Thu, Mar 4, 2021 at 12:29 AM Linus Walleij <linus.walleij@linaro.org> 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!) > > > +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'll add a comment regarding gpio pin mode. Yes output only mode as SPI chip-selects. > I suppose these systems also actually (ab)use the SPI cs > for things that are not really SPI CS? Because otherwise > this could just be part of the SPI driver (native chip select). > > > +static const struct of_device_id ebla_spics_of_match[] = { > > + { .compatible = "pensando,elba-spics" }, > > Have you documented this? Yes in Documentation/devicetree/bindings, I'll double check the content for completeness. The SPI CS isn't used for something else, the integrated DesignWare IP doesn't support 4 chip-selects on two spi busses. > > Other than that this is a nice and complete driver. > > Yours, > Linus Walleij Thanks for the review!
Hi Linus, On Thu, Mar 4, 2021 at 12:29 AM Linus Walleij <linus.walleij@linaro.org> wrote: > 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. [...] > > +#include <linux/gpio.h> > > Use this in new drivers: > #include <linux/gpio/driver.h> The updated patchset will use 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!) > > > +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? The top of the file will look like this in the updated patchset. * Pensando Elba ASIC SPI chip select driver. The SoC supports output * direction only as it uses a generic GPIO pin for SPI CS. > I suppose these systems also actually (ab)use the SPI cs > for things that are not really SPI CS? Because otherwise > this could just be part of the SPI driver (native chip select). The SPI cs are not used for any other purpose, we needed four chip selects and native DW supports two. > > +static const struct of_device_id ebla_spics_of_match[] = { > > + { .compatible = "pensando,elba-spics" }, > > Have you documented this? Yes as part of patchset v2: [PATCH v2 11/13] dt-bindings: gpio: Add Pensando Elba SoC support which documents "pensando,elba-spics" in new file bindings/gpio/pensando,elba-spics.yaml. Regards, Brad
Hi Linus, On Thu, Mar 4, 2021 at 5:38 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Thu, Mar 4, 2021 at 10:10 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Thu, Mar 04, 2021 at 09:29:33AM +0100, Linus Walleij wrote: > > > > > + * 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. > > If these output lines are so tightly coupled to the SPI block > and will not be used for any other GPO (general purpose output) > I think it makes more sense to bundle the handling into the > DW SPI driver, and activate it based on the Elba compatible > string (if of_is_compatible(...)). > > I am a bit cautious because it has happened in the past that > people repurpose CS lines who were originally for SPI CS > to all kind of other purposes, such as a power-on LED and > in that case it needs to be a separate GPIO driver. So the > author needs to have a good idea about what is a realistic > use case here. The gpio pins being used for the Elba SoC SPI CS are dedicated to this function. Are you recommending that the code in drivers/gpio/gpio-elba-spics.c be integrated into drivers/spi/spi-dw-mmio.c? Regards, Brad
Hi Elliott, On Thu, Mar 4, 2021 at 12:44 PM Elliott, Robert (Servers) <elliott@hpe.com> wrote: [...] > > +config GPIO_ELBA_SPICS > > + bool "Pensando Elba SPI chip-select" > > + depends on ARCH_PENSANDO_ELBA_SOC > > + help > > + Say yes here to support the Pensndo Elba SoC SPI chip-select > > driver > > Pensndo should be Pensando Fixed the typo, thanks. > > diff --git a/drivers/gpio/gpio-elba-spics.c b/drivers/gpio/gpio-elba-spics.c > > + * Pensando Elba ASIC SPI chip select driver > ... > > +MODULE_LICENSE("GPL v2"); > > +MODULE_DESCRIPTION("Elba SPI chip-select driver"); > > I think it's conventional to include the company name there, so > start that with "Pensando Elba" Ok, thanks. BTW I deleted these lines as I should have used builtin_platform_driver() and not a loadable module. > Also, "SoC" and "ASIC" are sometimes included after Elba, but sometimes > are not. Consistency might be helpful. > > > +static const struct of_device_id ebla_spics_of_match[] = { > ... > > + .of_match_table = ebla_spics_of_match, > > ebla should be elba Fixed the typo and using SoC which is more accurate. Regards, Brad
Hi Krzysztof, On Fri, Mar 5, 2021 at 3:25 AM Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 04/03/2021 04:41, Brad Larson wrote: > > This GPIO driver is for the Pensando Elba SoC which > > provides control of four chip selects on two SPI busses. [...] > > + ret = devm_gpiochip_add_data(&pdev->dev, &p->chip, p); > > + if (ret) { > > + dev_err(&pdev->dev, "unable to add gpio chip\n"); > > + return ret; > > + } > > + > > + dev_info(&pdev->dev, "elba spics registered\n"); > > Don't print trivial probe results, unless you print here something > useful. If you need it for debugging, keep it dev_dbg. Removed that extraneous logging Regards, Brad
Hi Geert, On Fri, Mar 5, 2021 at 5:57 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Brad, > > On Thu, Mar 4, 2021 at 4:59 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. [...] > > +config GPIO_ELBA_SPICS > > + bool "Pensando Elba SPI chip-select" > > + depends on ARCH_PENSANDO_ELBA_SOC > > Any specific reason this can't be "... || COMPILE_TEST"? Added COMPILE_TEST Regards, Brad
Hi Andy, On Mon, Mar 29, 2021 at 3:40 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Mar 29, 2021 at 4:19 AM Brad Larson <brad@pensando.io> wrote: > > On Sun, Mar 7, 2021 at 11:21 AM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Thu, Mar 4, 2021 at 4:40 PM Brad Larson <brad@pensando.io> wrote: > > ... > > > > > +config GPIO_ELBA_SPICS > > > > + bool "Pensando Elba SPI chip-select" > > > > > > Can't it be a module? Why? > > > > All Elba SoC based platforms require this driver to be built-in to boot and > > removing the module would result in a variety of exceptions/errors. > > Needs to be at least in the commit message. > > > > > > > + depends on ARCH_PENSANDO_ELBA_SOC > > > > + help > > > > + Say yes here to support the Pensndo Elba SoC SPI chip-select driver > > > > > > Please give more explanation what it is and why users might need it, > > > and also tell users how the module will be named (if there is no > > > strong argument why it can't be a module). > > > > > Fixed the typo. > > Yeah, according to the above, you better elaborate what this module is > and why people would need it. > Also can be a good hint to add > default ARCH_MY_COOL_PLATFORM Regarding the above module question and Kconfig definition, since I first looked at this and reviewed the comments I realized I should be using builtin. The file gpio/Kconfig is currently this config GPIO_ELBA_SPICS def_bool y depends on ARCH_PENSANDO_ELBA_SOC || COMPILE_TEST > ... > > > > > +#include <linux/of.h> > > > > > > It's not used here, but you missed mod_devicetable.h. > > > > Removed <linux/of.h>. There is no dependency on mod_devicetable.h. > > What do you mean? You don't use data structures from that? > of_device_id or other ID structures are defined there. Your module > works without them? > I typed the wrong filename. I do still have <linux/of.h> > > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > + p->base = devm_ioremap_resource(&pdev->dev, res); > > > > > > p->base = devm_platform_ioremap_resource(pdev, 0); > > > > Implementation follows devm_ioremap_resource() example in lib/devres.c. > > So? How does this make it impossible to address my comment? I was simply stating that I followed the recommended API per the source code although I don't recall if I was looking at 4.14, 5.10 or linux-next at the time. Changed to using devm_platform_ioremap_resource(). > > > > + if (IS_ERR(p->base)) { > > > > > > > + dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > > > > Duplicate noisy message. > > > > > > > + return PTR_ERR(p->base); > > > > + } Yep, I've removed the extraneous log message. Regards, 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
Hi Brad, On Mon, Aug 23, 2021 at 3:14 AM Brad Larson <brad@pensando.io> wrote: > On Mon, Mar 29, 2021 at 3:40 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Mon, Mar 29, 2021 at 4:19 AM Brad Larson <brad@pensando.io> wrote: > > > On Sun, Mar 7, 2021 at 11:21 AM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > > On Thu, Mar 4, 2021 at 4:40 PM Brad Larson <brad@pensando.io> wrote: > > > > ... > > > > > > > +config GPIO_ELBA_SPICS > > > > > + bool "Pensando Elba SPI chip-select" > > > > > > > > Can't it be a module? Why? > > > > > > All Elba SoC based platforms require this driver to be built-in to boot and > > > removing the module would result in a variety of exceptions/errors. > > > > Needs to be at least in the commit message. > > > > > > > + depends on ARCH_PENSANDO_ELBA_SOC > > > > > + help > > > > > + Say yes here to support the Pensndo Elba SoC SPI chip-select driver Pensando > > > > > > > > Please give more explanation what it is and why users might need it, > > > > and also tell users how the module will be named (if there is no > > > > strong argument why it can't be a module). > > > > > > > Fixed the typo. > > > > Yeah, according to the above, you better elaborate what this module is > > and why people would need it. > > Also can be a good hint to add > > default ARCH_MY_COOL_PLATFORM > > Regarding the above module question and Kconfig definition, since I > first looked at this and reviewed the comments I realized I should be > using builtin. The file gpio/Kconfig is currently this > > config GPIO_ELBA_SPICS > def_bool y > depends on ARCH_PENSANDO_ELBA_SOC || COMPILE_TEST That means the driver will default to yes by merely enabling COMPILE_TEST, which is a no-go. config GPIO_ELBA_SPICS bool "one-line summary" depends on ARCH_PENSANDO_ELBA_SOC || COMPILE_TEST default y if ARCH_PENSANDO_ELBA_SOC Gr{oetje,eeting}s, Geert
Hi Brad, On Mon, Aug 23, 2021 at 6:31 PM Brad Larson <brad@pensando.io> wrote: > On Mon, Aug 23, 2021 at 12:50 AM Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > > On Mon, Aug 23, 2021 at 3:14 AM Brad Larson <brad@pensando.io> wrote: > > > On Mon, Mar 29, 2021 at 3:40 AM Andy Shevchenko > [...] > > > Regarding the above module question and Kconfig definition, since I > > > first looked at this and reviewed the comments I realized I should be > > > using builtin. The file gpio/Kconfig is currently this > > > > > > config GPIO_ELBA_SPICS > > > def_bool y > > > depends on ARCH_PENSANDO_ELBA_SOC || COMPILE_TEST > > > > That means the driver will default to yes by merely enabling > > COMPILE_TEST, which is a no-go. > > > > config GPIO_ELBA_SPICS > > bool "one-line summary" > > depends on ARCH_PENSANDO_ELBA_SOC || COMPILE_TEST > > default y if ARCH_PENSANDO_ELBA_SOC > > Thanks Geert, changed to this > > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -241,8 +241,9 @@ config GPIO_EIC_SPRD > Say yes here to support Spreadtrum EIC device. > > config GPIO_ELBA_SPICS > + bool "Pensando Elba SoC SPI Chip Select as GPIO support" > + depends on ARCH_PENSANDO_ELBA_SOC > def_bool y > - depends on ARCH_PENSANDO_ELBA_SOC || COMPILE_TEST So we're losing the COMPILE_TEST ability again? Gr{oetje,eeting}s, Geert
On Mon, Aug 23, 2021 at 3:06 AM Brad Larson <brad@pensando.io> wrote: > The gpio pins being used for the Elba SoC SPI CS are dedicated to this > function. Are you recommending that the code in > drivers/gpio/gpio-elba-spics.c be integrated into > drivers/spi/spi-dw-mmio.c? That makes most sense does it not? Special purpose pins should be managed by that special purpose hardware driver, DW SPI in this case. The compatible string etc should be enough to determine that we need some extra GPIO control here, possibly specify extra registers for the SPI host etc. The struct spi_master has a special callback .set_cs() and you should make this behave special for your special hardware. In the case of the DW driver it appears that even subdrivers can pass a custom version of this call in struct dw_spi. Yours, Linus Walleij
On Mon, Aug 23, 2021 at 1:11 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Brad, > > On Mon, Aug 23, 2021 at 6:31 PM Brad Larson <brad@pensando.io> wrote: > > On Mon, Aug 23, 2021 at 12:50 AM Geert Uytterhoeven > > <geert@linux-m68k.org> wrote: > > > On Mon, Aug 23, 2021 at 3:14 AM Brad Larson <brad@pensando.io> wrote: > > > > On Mon, Mar 29, 2021 at 3:40 AM Andy Shevchenko > > [...] > > > > Regarding the above module question and Kconfig definition, since I > > > > first looked at this and reviewed the comments I realized I should be > > > > using builtin. The file gpio/Kconfig is currently this > > > > > > > > config GPIO_ELBA_SPICS > > > > def_bool y > > > > depends on ARCH_PENSANDO_ELBA_SOC || COMPILE_TEST > > > > > > That means the driver will default to yes by merely enabling > > > COMPILE_TEST, which is a no-go. > > > > > > config GPIO_ELBA_SPICS > > > bool "one-line summary" > > > depends on ARCH_PENSANDO_ELBA_SOC || COMPILE_TEST > > > default y if ARCH_PENSANDO_ELBA_SOC > > > > Thanks Geert, changed to this > > > > --- a/drivers/gpio/Kconfig > > +++ b/drivers/gpio/Kconfig > > @@ -241,8 +241,9 @@ config GPIO_EIC_SPRD > > Say yes here to support Spreadtrum EIC device. > > > > config GPIO_ELBA_SPICS > > + bool "Pensando Elba SoC SPI Chip Select as GPIO support" > > + depends on ARCH_PENSANDO_ELBA_SOC > > def_bool y > > - depends on ARCH_PENSANDO_ELBA_SOC || COMPILE_TEST > > So we're losing the COMPILE_TEST ability again? > Hi Geert, The gpio-elba-spics.c driver is being deleted with the spi chip-select control integrated into spi-dw-mmio.c. The GPIO_ELBA_SPICS config option goes away and fixes my breakage of COMPILE_TEST. Best, Brad
Hi Brad, On Mon, Oct 4, 2021 at 7:14 PM Brad Larson <brad@pensando.io> wrote: > On Mon, Aug 23, 2021 at 1:11 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Mon, Aug 23, 2021 at 6:31 PM Brad Larson <brad@pensando.io> wrote: > > > On Mon, Aug 23, 2021 at 12:50 AM Geert Uytterhoeven > > > <geert@linux-m68k.org> wrote: > > > > On Mon, Aug 23, 2021 at 3:14 AM Brad Larson <brad@pensando.io> wrote: > > > > > On Mon, Mar 29, 2021 at 3:40 AM Andy Shevchenko > > > [...] > > > > > Regarding the above module question and Kconfig definition, since I > > > > > first looked at this and reviewed the comments I realized I should be > > > > > using builtin. The file gpio/Kconfig is currently this > > > > > > > > > > config GPIO_ELBA_SPICS > > > > > def_bool y > > > > > depends on ARCH_PENSANDO_ELBA_SOC || COMPILE_TEST > > > > > > > > That means the driver will default to yes by merely enabling > > > > COMPILE_TEST, which is a no-go. > > > > > > > > config GPIO_ELBA_SPICS > > > > bool "one-line summary" > > > > depends on ARCH_PENSANDO_ELBA_SOC || COMPILE_TEST > > > > default y if ARCH_PENSANDO_ELBA_SOC > > > > > > Thanks Geert, changed to this > > > > > > --- a/drivers/gpio/Kconfig > > > +++ b/drivers/gpio/Kconfig > > > @@ -241,8 +241,9 @@ config GPIO_EIC_SPRD > > > Say yes here to support Spreadtrum EIC device. > > > > > > config GPIO_ELBA_SPICS > > > + bool "Pensando Elba SoC SPI Chip Select as GPIO support" > > > + depends on ARCH_PENSANDO_ELBA_SOC > > > def_bool y > > > - depends on ARCH_PENSANDO_ELBA_SOC || COMPILE_TEST > > > > So we're losing the COMPILE_TEST ability again? > > > > Hi Geert, > > The gpio-elba-spics.c driver is being deleted with the spi chip-select > control integrated into spi-dw-mmio.c. The GPIO_ELBA_SPICS config > option goes away and fixes my breakage of COMPILE_TEST. OK. Thanks for the follow-up. Gr{oetje,eeting}s, Geert
On Mon, Oct 4, 2021 at 6:46 PM Brad Larson <brad@pensando.io> wrote: > Yes that works, please see the diff below where the file > gpio-elba-spics.c goes away. The original implementation was > motivated by gpio-spear-spics.c. This looks good to me :) Yours, Linus Walleij
On Tue, Oct 12, 2021 at 4:52 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Mon, Oct 4, 2021 at 6:46 PM Brad Larson <brad@pensando.io> wrote: > > > Yes that works, please see the diff below where the file > > gpio-elba-spics.c goes away. The original implementation was > > motivated by gpio-spear-spics.c. > > This looks good to me :) > > Yours, > Linus Walleij Hi Linus, :-) It's better to not have to look at a related gpio driver file to the spi-dw-mmio.c driver and think it could possibly be used as general purpose gpio. Here is a response summary per patch. Should I start respinning the patchset against the latest linux-next tag? The changes are merged to our production 5.10.28 kernel and the next step is to re-spin the set against the latest linux-next which has a newer dtc, run checkpatch, etc. For reference as this has been cooking for awhile here is the overview from V2 patchset cover letter. This series enables support for Pensando Elba SoC based platforms. The Elba SoC has the following features: - Sixteen ARM64 A72 cores - Dual DDR 4/5 memory controllers - 32 lanes of PCIe Gen3/4 to the Host - Network interfaces: Dual 200GE, Quad 100GE, 50GE, 25GE, 10GE and also a single 1GE management port. - Storage/crypto offloads and 144 programmable P4 cores. - QSPI and EMMC for SoC storage - Two SPI interfaces for peripheral management - I2C bus for platform management Summary of response to V1/V2 patchset 0001-gpio-Add-Elba-SoC-gpio-driver-for-spi-cs-control.patch - This patch is deleted. Elba SOC specific gpio spics control is integrated into spi-dw-mmio.c. 0002-spi-cadence-quadspi-Add-QSPI-support-for-Pensando-El.patch - Changed compatible to "pensando,elba-qspi" to be more descriptive in spi-cadence-quadspi.c. - Arnd wondered if moving to DT properties for quirks may be the way to go. Feedback I've received on other patches was don't mix two efforts in one patch so I'm currently just adding the Elba support to the current design. 0003-spi-dw-Add-support-for-Pensando-Elba-SoC-SPI.patch - Changed the implementation to use existing dw_spi_set_cs() and integrated Elba specific CS control into spi-dw-mmio.c. The native designware support is for two chip-selects while Elba provides 4 chip-selects. Instead of adding a new file for this support in gpio-elba-spics.c the support is in one file (spi-dw-mmio.c). 0004-spidev-Add-Pensando-CPLD-compatible.patch - This patch is deleted. The addition of compatible "pensando,cpld" to spidev.c is removed. 0005-mmc-sdhci-cadence-Add-Pensando-Elba-SoC-support.patch - Ulf and Yamada-san agreed the amount of code for this support is not enough to need a new file. The support is added into sdhci-cadence.c and new files sdhci-cadence-elba.c and sdhci-cadence.h are deleted. - Redundant defines are removed (e.g. use SDHCI_CDNS_HRS04 and remove SDIO_REG_HRS4). - Removed phy init function sd4_set_dlyvr() and used existing sdhci_cdns_phy_init(). Init values are from DT properties. - Replace devm_ioremap_resource(&pdev->dev, iomem) with devm_platform_ioremap_resource(pdev, 1) - Refactored the elba priv_writ_l() and elba_write_l() to remove a little redundant code. - The config option CONFIG_MMC_SDHCI_CADENCE_ELBA goes away. - Only C syntax and Elba functions are prefixed with elba_ 0006-arm64-Add-config-for-Pensando-SoC-platforms.patch - Added a little more info to the platform help text to assist users to decide on including platform support or not. 0007-arm64-dts-Add-Pensando-Elba-SoC-support.patch - Node names changed to DT generic names - Changed from using 'spi@' which is reserved - The elba-flash-parts.dtsi is kept separate as it is included in multiple dts files. - SPDX license tags at the top of each file - The compatible = "pensando,elba" and 'model' are now together in the board file. - UIO nodes removed - Ordered nodes by increasing unit address - Removed an unreferenced container node. - Dropped deprecated 'device_type' for uart0 node. - Added syscon usage 0010-dt-bindings-spi-cadence-qspi-Add-support-for-Pensand.patch - Updated since the latest documentation has been converted to yaml 0011-dt-bindings-gpio-Add-Pensando-Elba-SoC-support.patch - This patch is deleted since the Elba gpio spics is added to the spi dw driver and documented there. Best, Brad