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 <redacted>
---
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
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).
Have you documented this?
Other than that this is a nice and complete driver.
Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 [off-list ref] wrote:
quoted
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 <redacted>
(...)
quoted
+#include <linux/gpio.h>
Use this in new drivers:
#include <linux/gpio/driver.h>
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.
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
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Aug 23, 2021 at 3:06 AM Brad Larson [off-list ref] 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Sun, Aug 29, 2021 at 2:09 PM Linus Walleij [off-list ref] wrote:
On Mon, Aug 23, 2021 at 3:06 AM Brad Larson [off-list ref] wrote:
quoted
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
On Tue, Oct 12, 2021 at 4:52 PM Linus Walleij [off-list ref] wrote:
On Mon, Oct 4, 2021 at 6:46 PM Brad Larson [off-list ref] wrote:
quoted
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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).
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
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.
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
+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
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.
Hi Elliott,
On Thu, Mar 4, 2021 at 12:44 PM Elliott, Robert (Servers)
[off-list ref] wrote:
[...]
quoted
+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
Fixed the typo and using SoC which is more accurate.
Regards,
Brad
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Krzysztof Kozlowski <hidden> Date: 2021-03-05 11:26:48
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 <redacted>
---
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
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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
--
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
+ 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.
...
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.
quoted
+ 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.
quoted
+#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.
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.
quoted
quoted
+ 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
...
quoted
quoted
+#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?
...
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.
quoted
quoted
quoted
+ 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
...
quoted
quoted
quoted
+#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>
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().
quoted
quoted
quoted
+ if (IS_ERR(p->base)) {
quoted
+ dev_err(&pdev->dev, "failed to remap I/O memory\n");
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.
quoted
quoted
quoted
+ depends on ARCH_PENSANDO_ELBA_SOC
+ help
+ Say yes here to support the Pensndo Elba SoC SPI chip-select driver
Pensando
quoted
quoted
quoted
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
--
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Geert,
On Mon, Aug 23, 2021 at 12:50 AM Geert Uytterhoeven
[off-list ref] wrote:
Hi Brad,
On Mon, Aug 23, 2021 at 3:14 AM Brad Larson [off-list ref] wrote:
quoted
On Mon, Mar 29, 2021 at 3:40 AM Andy Shevchenko
[...]
quoted
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
Hi Brad,
On Mon, Aug 23, 2021 at 6:31 PM Brad Larson [off-list ref] wrote:
quoted hunk
On Mon, Aug 23, 2021 at 12:50 AM Geert Uytterhoeven
[off-list ref] wrote:
quoted
On Mon, Aug 23, 2021 at 3:14 AM Brad Larson [off-list ref] wrote:
quoted
On Mon, Mar 29, 2021 at 3:40 AM Andy Shevchenko
[...]
quoted
quoted
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
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Aug 23, 2021 at 1:11 PM Geert Uytterhoeven [off-list ref] wrote:
Hi Brad,
On Mon, Aug 23, 2021 at 6:31 PM Brad Larson [off-list ref] wrote:
quoted
On Mon, Aug 23, 2021 at 12:50 AM Geert Uytterhoeven
[off-list ref] wrote:
quoted
On Mon, Aug 23, 2021 at 3:14 AM Brad Larson [off-list ref] wrote:
quoted
On Mon, Mar 29, 2021 at 3:40 AM Andy Shevchenko
[...]
quoted
quoted
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
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Brad,
On Mon, Oct 4, 2021 at 7:14 PM Brad Larson [off-list ref] wrote:
On Mon, Aug 23, 2021 at 1:11 PM Geert Uytterhoeven [off-list ref] wrote:
quoted
On Mon, Aug 23, 2021 at 6:31 PM Brad Larson [off-list ref] wrote:
quoted
On Mon, Aug 23, 2021 at 12:50 AM Geert Uytterhoeven
[off-list ref] wrote:
quoted
On Mon, Aug 23, 2021 at 3:14 AM Brad Larson [off-list ref] wrote:
quoted
On Mon, Mar 29, 2021 at 3:40 AM Andy Shevchenko
[...]
quoted
quoted
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
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
+ 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).
...
quoted
+#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
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 <redacted>
---
drivers/spi/spi-dw-mmio.c | 35 ++++++++++++++++++++++++++++++++++-
1 file changed, 34 insertions(+), 1 deletion(-)
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).
@@ -237,6 +237,38 @@ static int dw_spi_canaan_k210_init(struct platform_device *pdev,return0;}
+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
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 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 [off-list ref] wrote:
Hello Brad.
Thanks for the patch. See my comments below.
On Wed, Mar 03, 2021 at 07:41:36PM -0800, Brad Larson wrote:
quoted
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
quoted
+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 {
quoted
+ dw_writel(dws, DW_SPI_SER, 0);
+ } /* See [1] */
quoted
+}
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.
quoted
+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,
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Mar 4, 2021 at 4:42 AM Brad Larson [off-list ref] 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 <redacted>
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Mar 4, 2021 at 12:48 AM Linus Walleij [off-list ref] wrote:
On Thu, Mar 4, 2021 at 4:42 AM Brad Larson [off-list ref] wrote:
quoted
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 <redacted>
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Mar 4, 2021 at 4:41 AM Brad Larson [off-list ref] wrote:
Add ARCH_PENSANDO configuration option for Pensando SoC
based platforms.
Signed-off-by: Brad Larson <redacted>
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
@@ -6,6 +6,7 @@ Required properties: For TI 66AK2G SoC - "ti,k2g-qspi", "cdns,qspi-nor". For TI AM654 SoC - "ti,am654-ospi", "cdns,qspi-nor". For Intel LGM SoC - "intel,lgm-qspi", "cdns,qspi-nor".+ For Pensando SoC - "pensando,cdns-qspi". - reg : Contains two entries, each of which is a tuple consisting of a physical address and length. The first entry is the address and length of the controller register set. The second entry is the
On Wed, Mar 03, 2021 at 07:41:40PM -0800, Brad Larson wrote:
Add Pensando common and Elba SoC specific device nodes
and corresponding binding documentation.
This also needs to be split up into sub-patches seeing these are
unrelated changes like device bindings update, new platform DT file.
Note text-based bindings are deprecated in favor of the DT schemas.
Also note dts needs to pass dtbs_check validation. So all new HW/DT
nodes need to be reflected in the DT-schemas. See [1] for details.
[1] Documentation/devicetree/writing-schema.rst
@@ -6,6 +6,7 @@ Required properties: For TI 66AK2G SoC - "ti,k2g-qspi", "cdns,qspi-nor". For TI AM654 SoC - "ti,am654-ospi", "cdns,qspi-nor". For Intel LGM SoC - "intel,lgm-qspi", "cdns,qspi-nor".
+ For Pensando SoC - "pensando,cdns-qspi".
What about converting this file to DT-schema and adding new HW
bindings in there?
quoted hunk
- reg : Contains two entries, each of which is a tuple consisting of a
physical address and length. The first entry is the address and
length of the controller register set. The second entry is the
Oh, you've got four peripheral SPI devices connected with only two native CS
available. Hmm, then I don't really know a better way, but just to forget about
the native DW APB CS functionality and activate the direct driving of
all the CS-pins at the moment of the DW APB SPI controller probe
procedure. Then indeed you'll need a custom CS function defined in the DW APB
SPI driver to handle that.
So that GPIO-controller is just a single register which provides a way
to toggle the DW APB SPI CS-mode together with their output value.
If so and seeing there are a few more tiny spaces of config
registers added to eMMC, PCI, etc DT node, I suppose all of them
belong to some bigger config space of the SoC. Thus I'd suggest to at
least implement them as part of a System Controller DT node. Then use
that device service to switch on/off corresponding functionality.
See [2] and the rest of added to the kernel DTS files with
syscon-nodes for example.
[2] Documentation/devicetree/bindings/mfd/syscon.yaml
-Sergey
Oh, you've got four peripheral SPI devices connected with only two native CS
available. Hmm, then I don't really know a better way, but just to forget about
the native DW APB CS functionality and activate the direct driving of
all the CS-pins at the moment of the DW APB SPI controller probe
procedure. Then indeed you'll need a custom CS function defined in the DW APB
SPI driver to handle that.
Yes, with an Elba SoC specific gpio driver.
So that GPIO-controller is just a single register which provides a way
to toggle the DW APB SPI CS-mode together with their output value.
If so and seeing there are a few more tiny spaces of config
registers added to eMMC, PCI, etc DT node, I suppose all of them
belong to some bigger config space of the SoC. Thus I'd suggest to at
least implement them as part of a System Controller DT node. Then use
that device service to switch on/off corresponding functionality.
See [2] and the rest of added to the kernel DTS files with
syscon-nodes for example.
[2] Documentation/devicetree/bindings/mfd/syscon.yaml
To us it was more understandable to implement a standard gpio driver
for the spi chip-selects.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Sergey,
On Thu, Mar 4, 2021 at 12:03 AM Serge Semin [off-list ref] wrote:
On Wed, Mar 03, 2021 at 07:41:40PM -0800, Brad Larson wrote:
quoted
Add Pensando common and Elba SoC specific device nodes
and corresponding binding documentation.
This also needs to be split up into sub-patches seeing these are
unrelated changes like device bindings update, new platform DT file.
Note text-based bindings are deprecated in favor of the DT schemas.
Also note dts needs to pass dtbs_check validation. So all new HW/DT
nodes need to be reflected in the DT-schemas. See [1] for details.
[1] Documentation/devicetree/writing-schema.rst
Yes, patchset v2 was a first cut at organizing into sub-patches and in
v2 I used DT schemas for new files. I will need to add additional new
sub-patches per review comments for v3 of the patchset.
Alas it's not enough. New HW compatible strings shall be defined in the
binding schema.
Based upon the next-20210818 version of cdns,sdhci.yaml below is the
proposed change. In terms of defining new HW compatible strings is an
added example sufficient for pensando,elba-emmc? There is no
additional definition for socionext,uniphier-sd4hc other than the
example in this file.
@@ -6,6 +6,7 @@ Required properties: For TI 66AK2G SoC - "ti,k2g-qspi", "cdns,qspi-nor". For TI AM654 SoC - "ti,am654-ospi", "cdns,qspi-nor". For Intel LGM SoC - "intel,lgm-qspi", "cdns,qspi-nor".
quoted
+ For Pensando SoC - "pensando,cdns-qspi".
What about converting this file to DT-schema and adding new HW
bindings in there?
The file cadence-quadspi.txt has been converted to cdns,qspi-nor.yaml
in next-20210818. This would be the updated change where
pensando,cdns-qspi is now pensando,elba-qspi to be more specific.
Oh, you've got four peripheral SPI devices connected with only two native CS
available. Hmm, then I don't really know a better way, but just to forget about
the native DW APB CS functionality and activate the direct driving of
all the CS-pins at the moment of the DW APB SPI controller probe
procedure. Then indeed you'll need a custom CS function defined in the DW APB
SPI driver to handle that.
Right, confusion was created by leaving in code implying that the two
native CS are supported. CS0 is used just to start the serial engine.
The existing dw_spi_set_cs() function works fine resulting in this
implementation.
static void dw_spi_elba_set_cs(struct spi_device *spi, bool enable)
{
spi->chip_select = 0;
dw_spi_set_cs(spi, enable);
}
So that GPIO-controller is just a single register which provides a way
to toggle the DW APB SPI CS-mode together with their output value.
If so and seeing there are a few more tiny spaces of config
registers added to eMMC, PCI, etc DT node, I suppose all of them
belong to some bigger config space of the SoC. Thus I'd suggest to at
least implement them as part of a System Controller DT node. Then use
that device service to switch on/off corresponding functionality.
See [2] and the rest of added to the kernel DTS files with
syscon-nodes for example.
[2] Documentation/devicetree/bindings/mfd/syscon.yaml
-Sergey
I've looked at the syscon documentation, other drivers that use it and
tried the below proposed example with variations. The result is Elba
works ok for its four SPI devices but the host has a machine check
which must be due to a pcie access error. From another thread on this
topic here is the recommended change to using syscon.
Rob, please see here having a small sized reg-space one more time.
Having so many small-sized registers scattered around the dts file
makes me thinking that most of them likely belong to some bigger
block like "System Controller". If so then there must be a main node
compatible with "syscon" device, which phandle would be referenced in
the particular device nodes. Like this:
\ {
soc {
syscon: syscon@307c0000 {
compatible = "pensando,elba-sys-con", "syscon", "simple-mfd";
reg = <0x0 0x307c0000 0x0 0x10000>;
spics: spics@307c2468 {
compatible = "pensando,elba-spics";
gpio-controller;
#gpio-cells = <2>;
};
};
pcie@307c2480 {
compatible = "pensando,pcie";
reg = <0x0 0x20000000 0x0 0x00380000>; /* PXB Base */
syscon = <&syscon>;
};
/* etc */
};
};
On Thu, Mar 4, 2021 at 4:42 AM Brad Larson [off-list ref] wrote:
Add Pensando common and Elba SoC specific device nodes
and corresponding binding documentation.
Signed-off-by: Brad Larson <redacted>
(...)
.../bindings/gpio/pensando,elba-spics.txt | 24 ++
Please use YAML schema for this.
See Documentation/devicetree/writing-schema.rst
for instructions, you need to install some python pip packages
to test your schema.
Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Mar 4, 2021 at 12:52 AM Linus Walleij [off-list ref] wrote:
On Thu, Mar 4, 2021 at 4:42 AM Brad Larson [off-list ref] wrote:
quoted
Add Pensando common and Elba SoC specific device nodes
and corresponding binding documentation.
Signed-off-by: Brad Larson <redacted>
(...)
quoted
.../bindings/gpio/pensando,elba-spics.txt | 24 ++
Please use YAML schema for this.
In patchset v2 changed to YAML schema and passed dt_binding_check and
dtbs_check.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
@@ -0,0 +1,24 @@+Pensando Elba SPI Chip Select Driver++The Pensando Elba ASIC provides four SPI bus chip selects++Required properties:+- compatible: Should be "pensando,elba-spics"+- reg: Address range of spics controller+- gpio-controller: Marks the device node as gpio controller+- #gpio-cells: Must be 2
You need to document what each of the cells are for. In your
example, the second cell is always zero, is that intentional?
These are in the wrong order, the most generic one (cdns,sd4hc) always
comes last.
If you add the string in the example, it also has to be an option in the
actual binding, otherwise neither the example nor your dtb would
be valid.
You also wouldn't find a controller that is compatible with both the uniphier
variant and the elba variant, unless your 'elba' SoC is strictly derived from
Socionext's Uniphier products and inherits all the quirks in its sdhci
implementation that were not already part of Cadence's IP block.
@@ -6,6 +6,7 @@ Required properties: For TI 66AK2G SoC - "ti,k2g-qspi", "cdns,qspi-nor". For TI AM654 SoC - "ti,am654-ospi", "cdns,qspi-nor". For Intel LGM SoC - "intel,lgm-qspi", "cdns,qspi-nor".+ For Pensando SoC - "pensando,cdns-qspi".
This does not look specific enough: There is no guarantee that this
is the only time Pensando uses any Cadenci qspi block. If the company
is not yet out of business, you should be prepared for future products
and have the name of the chip in there as well.
These don't seem to have a binding document, which needs to be added
first. What is a Pensando "cpld"? Is it possible that there will be multiple
versions of it that need to be uniquely identified?
+
+ /* Common UIO device for MSI drivers */
+ uio_penmsi {
+ compatible = "pensando,uio_penmsi";
+ name = "uio_penmsi";
+ };
Missing binding again. Since you name this a UIO device, I assume this
is actually tied to a particular Linux device driver and exported to user
space. The information in the dts should however not assume a particular
OS implementation but describe the platform.
Is this for PCI MSI? If so, I would recommend just using the GICv3 that you
also have, and leave this device completely unused.
In either case, please leave out the device node until a binding has
been agreed and a matching kernel driver was reviewed (if any)
+
+ /*
+ * Until we know the interrupt domain following this, we
+ * are forced to use this is the place where interrupts from
+ * PCI converge. In the ideal case, we use one domain higher,
+ * where the PCI-ness has been shed.
+ */
+ pxc0_intr: intc@20102200 {
+ compatible = "pensando,soc-ictlr-csrintr";
+ interrupt-controller;
+ reg = <0x0 0x20102200 0x0 0x4>;
+ #interrupt-cells = <3>;
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "pxc0_intr";
+ };
Leave this one out as well, this has to be reviewed in combination with the
PCI driver.
This does not follow the PCI host bridge binding. Leave it out for now,
and bring it back once you have a proper PCI driver.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mmc/cdns,sdhci.example.dt.yaml: mmc@5a000000: compatible: ['socionext,uniphier-sd4hc', 'cdns,sd4hc', 'pensando,elba-emmc'] is too long
From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mmc/cdns,sdhci.example.dt.yaml: mmc@5a000000: compatible: Additional items are not allowed ('pensando,elba-emmc' was unexpected)
From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
See https://patchwork.ozlabs.org/patch/1447072
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
@@ -6,6 +6,7 @@ Required properties: For TI 66AK2G SoC - "ti,k2g-qspi", "cdns,qspi-nor". For TI AM654 SoC - "ti,am654-ospi", "cdns,qspi-nor". For Intel LGM SoC - "intel,lgm-qspi", "cdns,qspi-nor".+ For Pensando SoC - "pensando,cdns-qspi". - reg : Contains two entries, each of which is a tuple consisting of a physical address and length. The first entry is the address and length of the controller register set. The second entry is the
You have SPDX header. No boiler plate.
OK, I stopped...
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
@@ -250,6 +250,21 @@ config MMC_SDHCI_CADENCEIfunsure,sayN.+configMMC_SDHCI_CADENCE_ELBA+tristate"SDHCI support for the Pensando/Cadence SD/SDIO/eMMC controller"+depends onARCH_PENSANDO_ELBA_SOC+depends onMMC_SDHCI+depends onOF+depends onMMC_SDHCI_CADENCE+depends onMMC_SDHCI_PLTFM+selectMMC_SDHCI_IO_ACCESSORS+help+ThisselectsthePensando/CadenceSD/SDIO/eMMCcontroller.++Ifyouhaveacontrollerwiththisinterface,sayYorMhere.++Ifunsure,sayN.+configMMC_SDHCI_CNS3XXXtristate"SDHCI support on the Cavium Networks CNS3xxx SoC"depends onARCH_CNS3XXX||COMPILE_TEST