| Message ID | 1425487205-5477-2-git-send-email-kernel@martin.sperl.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/04/2015 09:40 AM, kernel@martin.sperl.org wrote: > From: Martin Sperl <kernel@martin.sperl.org> > > Change the device tree to use cs-gpios for the spi bus master > and standard gpio operation instead of relying on the HW with > just 2 chip_selects using ALT0. > > This reassigns the existing CS pins 7(=CS1) and 8(=CS0) > as output instead of ALT0 (=SPI HW block controlled) > and adds them in the list of cs-gpios for the spi-bus. These pins aren't used by anything on the board, but are rather part of the expansion header. I wonder if we wouldn't be better off removing any configuration of the pins from the DT. After all, we can't guarantee how the user has connected them. The "default" usage, a/k/a the expansion header signal naming, isn't any guarantee. Rather, the user should specify what they want to use the pin as; as a GPIO input, GPIO output, or an SPI chip-select. > diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi > gpioout: gpioout { > - brcm,pins = <6>; > + brcm,pins = <6 7 8>; > brcm,function = <1>; /* GPIO out */ > }; > > alt0: alt0 { > - brcm,pins = <0 1 2 3 4 5 7 8 9 10 11 14 15 40 45>; > + brcm,pins = <0 1 2 3 4 5 9 10 11 14 15 40 45>; > brcm,function = <4>; /* alt0 */ > }; While the existing DT already has this issue, note that this forces these pins to be driven as outputs. What if the user has hooked up an external device that drives these signals, and wants to use the pins as GPIO inputs? > diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi > #address-cells = <1>; > #size-cells = <0>; > status = "disabled"; > + cs-gpios = <&gpio 8 0>, <&gpio 7 0>; > }; This shouldn't be in the SoC .dtsi file. It's quite possible for someone to use other GPIOs as SPI CS. It's board or even use-case specific whether those are the correct values. I would argue that we should not put any cs-gpios into any in-kernel DT file, since there's no on-board usage of SPI on the RPi boards. For SPI to be useful, the user has to add a DT node to represent the SPI device itself anyway, so adding some properties to the controller to define which GPIOs to use for SPI CS can be done then too. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On 07.03.2015, at 06:47, Stephen Warren <swarren@wwwdotorg.org> wrote: > > These pins aren't used by anything on the board, but are rather part of > the expansion header. I wonder if we wouldn't be better off removing any > configuration of the pins from the DT. After all, we can't guarantee how > the user has connected them. The "default" usage, a/k/a the expansion > header signal naming, isn't any guarantee. > > Rather, the user should specify what they want to use the pin as; as a > GPIO input, GPIO output, or an SPI chip-select. ... > While the existing DT already has this issue, note that this forces > these pins to be driven as outputs. What if the user has hooked up an > external device that drives these signals, and wants to use the pins as > GPIO inputs? Actually if you look at the Documentation on page 102 you will find that those pins are pulled high by default (if it is HW or firmware I am not sure) - so a hat designer needs to take this into consideration anyway. The only difference is that it is pulled high and not driven high/low. Also with the "new" models there is the Firmware that will read those "hat-descriptors" from the eeprom and configure the GPIO "modes" and pullups based on this information. But then it means in principle that this is a more general issue that just became apparent now. > This shouldn't be in the SoC .dtsi file. It's quite possible for someone > to use other GPIOs as SPI CS. It's board or even use-case specific > whether those are the correct values. > > I would argue that we should not put any cs-gpios into any in-kernel DT > file, since there's no on-board usage of SPI on the RPi boards. but then: why not just make it optional and NOT configuring it at all and keep it "outside". > For SPI to be useful, the user has to add a DT node to represent the SPI > device itself anyway, so adding some properties to the controller to > define which GPIOs to use for SPI CS can be done then too. From what I have seen (and I am not an expert) is that with the foundation device trees each "device" (spi/i2c/...) has a separate Gpio section, which gets referenced inside the spi/i2c/... block. As far as I understood with this setup the GPIOs ALT only gets set up when the driver itself loads (probably while parsing the DT for the device) So this is maybe the way forward for the whole default-dt? For SPI it would look like this: &gpio { spi0_pins: spi0_pins { brcm,pins = <7 8 9 10 11>; brcm,function = <4>; /* alt0 */ }; ... } &spi0 { ... pinctrl-0 = <&spi0_pins>; ... } And if you keep spi0 disabled in the dtsi files then the ALT modes should not be set. Obviously we could also split the gpio-block into "normal SPI" and "CS" pins, which would allow changing the "defaults" also in the dts that gets build. So how should we proceed? Martin-- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/11/2015 09:21 AM, Martin Sperl wrote: >> On 07.03.2015, at 06:47, Stephen Warren <swarren@wwwdotorg.org> wrote: >> >> These pins aren't used by anything on the board, but are rather part of >> the expansion header. I wonder if we wouldn't be better off removing any >> configuration of the pins from the DT. After all, we can't guarantee how >> the user has connected them. The "default" usage, a/k/a the expansion >> header signal naming, isn't any guarantee. >> >> Rather, the user should specify what they want to use the pin as; as a >> GPIO input, GPIO output, or an SPI chip-select. > ... >> While the existing DT already has this issue, note that this forces >> these pins to be driven as outputs. What if the user has hooked up an >> external device that drives these signals, and wants to use the pins as >> GPIO inputs? > > Actually if you look at the Documentation on page 102 you will find that > those pins are pulled high by default (if it is HW or firmware I am not > sure) - so a hat designer needs to take this into consideration anyway. > The only difference is that it is pulled high and not driven high/low. Assuming that table shows HW defaults as opposed to the pin's capabilities for pull-up-vs-down, then yes. Still, there is typically quite a difference in strength between a pull-up and a drive-up. > Also with the "new" models there is the Firmware that will read those > "hat-descriptors" from the eeprom and configure the GPIO "modes" and > pullups based on this information. Yes, my thinking is we should probably rely on that, or an explicit user modification of the DT, to configure the pinmux, rather than making assumptions. > But then it means in principle that this is a more general issue > that just became apparent now. Yes. >> This shouldn't be in the SoC .dtsi file. It's quite possible for someone >> to use other GPIOs as SPI CS. It's board or even use-case specific >> whether those are the correct values. >> >> I would argue that we should not put any cs-gpios into any in-kernel DT >> file, since there's no on-board usage of SPI on the RPi boards. > > but then: why not just make it optional and NOT configuring it at all > and keep it "outside". Sorry, I don't quite understand that comment; outside what? >> For SPI to be useful, the user has to add a DT node to represent the SPI >> device itself anyway, so adding some properties to the controller to >> define which GPIOs to use for SPI CS can be done then too. > > From what I have seen (and I am not an expert) is that with the > foundation device trees each "device" (spi/i2c/...) has a separate > Gpio section, which gets referenced inside the spi/i2c/... block. > > As far as I understood with this setup the GPIOs ALT only gets set > up when the driver itself loads (probably while parsing the DT > for the device) Yes, that is certainly possible. > So this is maybe the way forward for the whole default-dt? > > For SPI it would look like this: > &gpio { > spi0_pins: spi0_pins { > brcm,pins = <7 8 9 10 11>; > brcm,function = <4>; /* alt0 */ > }; > ... > } > > &spi0 { > ... > pinctrl-0 = <&spi0_pins>; > ... > } > > And if you keep spi0 disabled in the dtsi files then the ALT > modes should not be set. Yes, so long as it's disabled by default that would be OK. However, I wonder why we don't just rely on the firmware to set up the pinmux, since as you mentioned it does it now? > Obviously we could also split the gpio-block into > "normal SPI" and "CS" pins, which would allow changing the > "defaults" also in the dts that gets build. > > So how should we proceed? If we do put any default CS GPIO setup in the kernel DT, we should indeed put it into a separate node (pinctrl state) so that the user can override it easily without any interactions with any other pins/... -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On 17.03.2015, at 04:18, Stephen Warren <swarren@wwwdotorg.org> wrote: >> But then it means in principle that this is a more general issue >> that just became apparent now. > > Yes. > ... >> So this is maybe the way forward for the whole default-dt? >> >> For SPI it would look like this: >> &gpio { >> spi0_pins: spi0_pins { >> brcm,pins = <7 8 9 10 11>; >> brcm,function = <4>; /* alt0 */ >> }; >> ... >> } >> >> &spi0 { >> ... >> pinctrl-0 = <&spi0_pins>; >> ... >> } >> >> And if you keep spi0 disabled in the dtsi files then the ALT >> modes should not be set. > > Yes, so long as it's disabled by default that would be OK. However, I > wonder why we don't just rely on the firmware to set up the pinmux, > since as you mentioned it does it now? ... >> Obviously we could also split the gpio-block into >> "normal SPI" and "CS" pins, which would allow changing the >> "defaults" also in the dts that gets build. >> >> So how should we proceed? > > If we do put any default CS GPIO setup in the kernel DT, we should > indeed put it into a separate node (pinctrl state) so that the user can > override it easily without any interactions with any other pins/... So I will create a patch to separate the spi portions out as mentioned above. I will actually create 2 gpio-sections: one for the SPI transfer pins (GPIO 9, 10, 11) and one for the chipselects (8, 7), which can get overridden either as output or as ALT0 in a customized device tree. SPI by default shall be disabled. Martin -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Martin, Am 04.03.2015 um 17:40 schrieb kernel@martin.sperl.org: > From: Martin Sperl <kernel@martin.sperl.org> > > Change the device tree to use cs-gpios for the spi bus master > and standard gpio operation instead of relying on the HW with > just 2 chip_selects using ALT0. > > This reassigns the existing CS pins 7(=CS1) and 8(=CS0) > as output instead of ALT0 (=SPI HW block controlled) > and adds them in the list of cs-gpios for the spi-bus. > > Signed-off-by: Martin Sperl <kernel@martin.sperl.org> > > --- > arch/arm/boot/dts/bcm2835-rpi.dtsi | 4 ++-- > arch/arm/boot/dts/bcm2835.dtsi | 1 + > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi > index c706448..f0e36ab 100644 > --- a/arch/arm/boot/dts/bcm2835-rpi.dtsi > +++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi > @@ -20,12 +20,12 @@ > pinctrl-names = "default"; > > gpioout: gpioout { > - brcm,pins = <6>; > + brcm,pins = <6 7 8>; > brcm,function = <1>; /* GPIO out */ > }; > > alt0: alt0 { > - brcm,pins = <0 1 2 3 4 5 7 8 9 10 11 14 15 40 45>; > + brcm,pins = <0 1 2 3 4 5 9 10 11 14 15 40 45>; > brcm,function = <4>; /* alt0 */ > }; just a note for your next version. I think this patch won't apply against for-rpi-next. It misses that one: https://git.kernel.org/cgit/linux/kernel/git/rpi/linux-rpi.git/commit/?h=for-rpi-next&id=72a6dbe0ea91e2a9c4357a3b0afbbd7b1f00dc8a Regards Stefan -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi index c706448..f0e36ab 100644 --- a/arch/arm/boot/dts/bcm2835-rpi.dtsi +++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi @@ -20,12 +20,12 @@ pinctrl-names = "default"; gpioout: gpioout { - brcm,pins = <6>; + brcm,pins = <6 7 8>; brcm,function = <1>; /* GPIO out */ }; alt0: alt0 { - brcm,pins = <0 1 2 3 4 5 7 8 9 10 11 14 15 40 45>; + brcm,pins = <0 1 2 3 4 5 9 10 11 14 15 40 45>; brcm,function = <4>; /* alt0 */ }; diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi index 3342cb1..74d08e1 100644 --- a/arch/arm/boot/dts/bcm2835.dtsi +++ b/arch/arm/boot/dts/bcm2835.dtsi @@ -110,6 +110,7 @@ #address-cells = <1>; #size-cells = <0>; status = "disabled"; + cs-gpios = <&gpio 8 0>, <&gpio 7 0>; }; i2c0: i2c@20205000 {