| Message ID | 20210304034141.7062-2-brad@pensando.io |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support Pensando Elba SoC | expand |
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 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
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 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 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 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. > > 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. Based on the feedback I realized this should not be a loadable module. I should be using builtin_platform_driver(elba_spics_driver). Currently I have this for gpio/Kconfig config GPIO_ELBA_SPICS def_bool y depends on ARCH_PENSANDO_ELBA_SOC || COMPILE_TEST > > +/* > > + * 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))) Both are functionally correct. I don't have a preference, do you want this change? > > +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() for this structure > > +static int elba_spics_get_value(struct gpio_chip *chip, unsigned int pin) > > +{ > > + return -ENXIO; > > Hmm... Is it really acceptable error code here? No it's not, thanks. Changed to -ENOTSUPP as gpio output direction only is supported. > > +static int elba_spics_direction_input(struct gpio_chip *chip, unsigned int pin) > > +{ > > + return -ENXIO; > > Ditto. Changed 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); Changed to single call to 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. Removed extra log 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; Yes, made this change and will include in v3 patchset --- a/drivers/gpio/gpio-elba-spics.c +++ b/drivers/gpio/gpio-elba-spics.c @@ -91,13 +91,9 @@ static int elba_spics_probe(struct platform_device *pdev) ret = devm_gpiochip_add_data(&pdev->dev, &p->chip, p); - if (ret) { + if (ret) dev_err(&pdev->dev, "unable to add gpio chip\n"); - return ret; - } - - dev_info(&pdev->dev, "elba spics registered\n"); - return 0; + return ret; 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 Geert, On Mon, Aug 23, 2021 at 12:50 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > 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 [...] > > 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 Regards, Brad
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 -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
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 Sun, Aug 29, 2021 at 2:09 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > 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 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. Best, Brad diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index d3b509e175e47..14751c7ccd1f4 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -240,11 +240,6 @@ config GPIO_EIC_SPRD help 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 - config GPIO_EM tristate "Emma Mobile GPIO" depends on (ARCH_EMEV2 || COMPILE_TEST) && OF_GPIO diff --git a/arch/arm64/boot/dts/pensando/elba-asic-common.dtsi b/arch/arm64/boot/dts/pensando/elba-asic-common.dtsi index a59405dc5676d..17dfb0c91f84c 100644 --- a/arch/arm64/boot/dts/pensando/elba-asic-common.dtsi +++ b/arch/arm64/boot/dts/pensando/elba-asic-common.dtsi @@ -66,8 +66,8 @@ &spi0 { num-cs = <4>; - cs-gpios = <&spics 0 GPIO_ACTIVE_LOW>, <&spics 1 GPIO_ACTIVE_LOW>, - <&porta 1 GPIO_ACTIVE_LOW>, <&porta 7 GPIO_ACTIVE_LOW>; + cs-gpios = <0>, <0>, + <&porta 1 GPIO_ACTIVE_LOW>, <&porta 7 GPIO_ACTIVE_LOW>; status = "okay"; spi0_cs0@0 { compatible = "pensando,cpld"; diff --git a/arch/arm64/boot/dts/pensando/elba.dtsi b/arch/arm64/boot/dts/pensando/elba.dtsi index 029dd5f0045f3..3ff4c39815639 100644 --- a/arch/arm64/boot/dts/pensando/elba.dtsi +++ b/arch/arm64/boot/dts/pensando/elba.dtsi @@ -127,6 +127,7 @@ spi0: spi@2800 { compatible = "pensando,elba-spi"; reg = <0x0 0x2800 0x0 0x100>; + pensando,spics = <&mssoc 0x2468 0>; clocks = <&ahb_clk>; interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>; #address-cells = <1>; @@ -257,13 +258,6 @@ reg = <0x0 0x307c2080 0x0 0x4>; }; - spics: spics@307c2468 { - compatible = "pensando,elba-spics"; - reg = <0x0 0x307c2468 0x0 0x4>; - gpio-controller; - #gpio-cells = <2>; - }; - pcie@307c2480 { compatible = "pensando,pcie"; reg = <0x0 0x307c2480 0x0 0x4 /* MS CFG_WDT */ diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c index 3379720cfcb8..64644bae8923 100644 --- a/drivers/spi/spi-dw-mmio.c +++ b/drivers/spi/spi-dw-mmio.c @@ -16,6 +16,7 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/of_platform.h> +#include <linux/gpio.h> #include <linux/acpi.h> #include <linux/property.h> #include <linux/regmap.h> @@ -53,6 +54,24 @@ struct dw_spi_mscc { void __iomem *spi_mst; /* Not sparx5 */ }; +struct dw_spi_elba { + struct regmap *regmap; + unsigned int reg; + unsigned int ctl; +}; + +/* + * ctl: 1 | 0 + * cs: 1 0 | 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 ELBA_SPICS_SHIFT(ctl, cs) (4 * (ctl) + 2 * (cs)) +#define ELBA_SPICS_MASK(ctl, cs) (0x3 << ELBA_SPICS_SHIFT(ctl, cs)) +#define ELBA_SPICS_SET(ctl, cs, val) \ + ((((val) << 1) | 0x1) << ELBA_SPICS_SHIFT(ctl, cs)) + /* * The Designware SPI controller (referred to as master in the documentation) * automatically deasserts chip select when the tx fifo is empty. The chip @@ -237,6 +256,74 @@ static int dw_spi_canaan_k210_init(struct platform_device *pdev, return 0; } +static void elba_spics_set_cs(struct dw_spi_elba *dwselba, int cs, int enable) +{ + regmap_update_bits(dwselba->regmap, dwselba->reg, + ELBA_SPICS_MASK(dwselba->ctl, cs), + ELBA_SPICS_SET(dwselba->ctl, cs, enable)); +} + +static void dw_spi_elba_set_cs(struct spi_device *spi, bool enable) +{ + struct dw_spi *dws = spi_master_get_devdata(spi->master); + struct dw_spi_mmio *dwsmmio = container_of(dws, struct dw_spi_mmio, dws); + struct dw_spi_elba *dwselba = dwsmmio->priv; + u8 cs = spi->chip_select; + + if (cs < 2) { + /* overridden native chip-select */ + elba_spics_set_cs(dwselba, spi->chip_select, enable); + } + + /* + * The DW SPI controller needs a native CS bit selected to start + * the serial engine, and we have fewer native CSs than we need, so + * use CS0 always. + */ + spi->chip_select = 0; + dw_spi_set_cs(spi, enable); + spi->chip_select = cs; +} + +static int dw_spi_elba_init(struct platform_device *pdev, + struct dw_spi_mmio *dwsmmio) +{ + struct of_phandle_args args; + struct dw_spi_elba *dwselba; + struct regmap *regmap; + int rc; + + rc = of_parse_phandle_with_fixed_args(pdev->dev.of_node, + "pensando,spics", 2, 0, &args); + if (rc) { + dev_err(&pdev->dev, "could not find pensando,spics\n"); + return rc; + } + + regmap = syscon_node_to_regmap(args.np); + if (IS_ERR(regmap)) { + dev_err(&pdev->dev, "could not map pensando,spics\n"); + return PTR_ERR(regmap); + } + + dwselba = devm_kzalloc(&pdev->dev, sizeof(*dwselba), GFP_KERNEL); + if (!dwselba) + return -ENOMEM; + + dwselba->regmap = regmap; + dwselba->reg = args.args[0]; + dwselba->ctl = args.args[1]; + + /* deassert cs */ + elba_spics_set_cs(dwselba, 0, 1); + elba_spics_set_cs(dwselba, 1, 1); + + dwsmmio->priv = dwselba; + 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 +438,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}, { /* end of table */} }; MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
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 -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index e3607ec4c2e8..d99bc82aa8fa 100644 --- 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 + 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 diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index c58a90a3c3b1..c5c7acad371b 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -54,6 +54,7 @@ obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o obj-$(CONFIG_GPIO_DLN2) += gpio-dln2.o obj-$(CONFIG_GPIO_DWAPB) += gpio-dwapb.o obj-$(CONFIG_GPIO_EIC_SPRD) += gpio-eic-sprd.o +obj-$(CONFIG_GPIO_ELBA_SPICS) += gpio-elba-spics.o obj-$(CONFIG_GPIO_EM) += gpio-em.o obj-$(CONFIG_GPIO_EP93XX) += gpio-ep93xx.o obj-$(CONFIG_GPIO_EXAR) += gpio-exar.o diff --git a/drivers/gpio/gpio-elba-spics.c b/drivers/gpio/gpio-elba-spics.c new file mode 100644 index 000000000000..a845525cf2a3 --- /dev/null +++ b/drivers/gpio/gpio-elba-spics.c @@ -0,0 +1,120 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Pensando Elba ASIC SPI chip select driver + * + * Copyright (c) 2020-2021, Pensando Systems Inc. + */ + +#include <linux/err.h> +#include <linux/gpio.h> +#include <linux/module.h> +#include <linux/io.h> +#include <linux/init.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> +#include <linux/types.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)) + +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; +} + +static void elba_spics_set_value(struct gpio_chip *chip, + unsigned int pin, int value) +{ + struct elba_spics_priv *p = gpiochip_get_data(chip); + unsigned long flags; + u32 tmp; + + /* select chip select from register */ + spin_lock_irqsave(&p->lock, flags); + tmp = readl_relaxed(p->base); + tmp = (tmp & ~SPICS_MASK(pin)) | SPICS_SET(pin, value); + writel_relaxed(tmp, p->base); + spin_unlock_irqrestore(&p->lock, flags); +} + +static int elba_spics_direction_input(struct gpio_chip *chip, unsigned int pin) +{ + return -ENXIO; +} + +static int elba_spics_direction_output(struct gpio_chip *chip, + unsigned int pin, int value) +{ + elba_spics_set_value(chip, pin, value); + return 0; +} + +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"); + return 0; +} + +static const struct of_device_id ebla_spics_of_match[] = { + { .compatible = "pensando,elba-spics" }, + {} +}; + +static struct platform_driver elba_spics_driver = { + .probe = elba_spics_probe, + .driver = { + .name = "pensando-elba-spics", + .of_match_table = ebla_spics_of_match, + }, +}; +module_platform_driver(elba_spics_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("Elba SPI chip-select driver");
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