| Message ID | 1351106281-31288-1-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/24/2012 09:18 PM, Thomas Petazzoni wrote: > Note: this patch is a *RFC*, it is not intended for merging, only to > get a discussion started. The code is horrible, makes terrible > assumptions and so on. > > On many platforms, most of the pinmux initialization is done in the > bootloader, and therefore persists when we boot the Linux kernel. This > prevents us from making sure that the pinmux configuration in the > board device trees is correct. Thomas, how you make sure something you don't know about? The bootloader sets these pinmux settings for a reason and if the DT doesn't tell the kernel it should make no assumptions at all. > One idea to make sure our device trees are correct in terms of > pinmuxing is to set the state of each pin to an unavailable function > during the initialization of the pinctrl driver. This way, only pins > that are explicitly configured through proper device tree attributes > will actually be functional. > > Remaining questions to solve: > > * Is this useful? IMHO it isn't - but maybe I am missing the point here. What is it that you don't like in the bootloaders choice of configuring pinmux? > * Maybe some pins should be excluded for this, especially if the > console serial port pins are muxed. On Armada XP, it's not the > case: UART0 RX/UART0 TX pins are not part of MPPs, so we can clear > all pins. But on other mvebu platforms, it may be the case. That's why you don't touch pins that you don't know about. The mvebu pinctrl is written to overwrite pinctrl settings only if someone requests a special function. Usually this is done by a developer that knows about the board. Sebastian
Sebastian, On Wed, 24 Oct 2012 21:38:23 +0200, Sebastian Hesselbarth wrote: > how you make sure something you don't know about? The bootloader sets > these pinmux settings for a reason and if the DT doesn't tell the kernel > it should make no assumptions at all. Apparently, the general feeling of the kernel people is that the kernel should reduce as much as possible its dependency on the hardware configuration set up by the bootloader. Therefore, even though your bootloader does a certain pinmux configuration, your kernel should make its own pinmux configuration and be fully independent from the one done by the bootloader. If we want to achieve this, then it seemed like a good idea to put all pins to an unknown state at boot time. > IMHO it isn't - but maybe I am missing the point here. What is it that > you don't like in the bootloaders choice of configuring pinmux? Just because the kernel should not depend on hardware configuration done by the bootloader, except for very core things like DRAM timings and al. At least, that's my understanding of where the ARM kernel people are trying to go. But I might have misunderstood, in which case of course, the entire purpose of this patch disappears. Best regards, Thomas
On Wed, Oct 24, 2012 at 09:18:01PM +0200, Thomas Petazzoni wrote: > Note: this patch is a *RFC*, it is not intended for merging, only to > get a discussion started. The code is horrible, makes terrible > assumptions and so on. > > On many platforms, most of the pinmux initialization is done in the > bootloader, and therefore persists when we boot the Linux kernel. This > prevents us from making sure that the pinmux configuration in the > board device trees is correct. You can get a lot of information from /debug. eg, # cat /debug/pinctrl/f1010000.pinctrl/pinconf-groups Pin config settings per pin group Format: group (name): configs 0 (mpp0):current: spi(cs), available = [ gpio(io) nand(io2) ] 1 (mpp1):current: spi(mosi), available = [ gpo(o) nand(io3) ] 2 (mpp2):current: spi(sck), available = [ gpo(o) nand(io4) ] 3 (mpp3):current: spi(miso), available = [ gpo(o) nand(io5) ] 4 (mpp4):current: sata1(act), available = [ gpio(io) nand(io6) uart0(rxd) lcd(hsync) ] 5 (mpp5):current: sata0(act), available = [ gpo(o) nand(io7) uart0(txd) lcd(vsync) ] 6 (mpp6):current: spi(mosi), available = [ sysrst(out) ] 7 (mpp7):current: gpo(o), available = [ spi(cs) lcd(pwm) ] 8 (mpp8):current: twsi0(sda), available = [ gpio(io) uart0(rts) uart1(rts) mii-1(rxerr) sata1(prsnt) mii(col) ] 9 (mpp9):current: twsi0(sck), available = [ gpio(io) uart0(cts) uart1(cts) sata0(prsnt) mii(crs) ] 10 (mpp10):current: uart0(txd), available = [ gpo(o) spi(sck) sata1(act) ] 11 (mpp11):current: uart0(rxd), available = [ gpio(io) spi(miso) sata0(act) ] 12 (mpp12):current: gpo(o), available = [ sdio(clk) audio(spdifo) spi(mosi) twsi1(sda) ] 13 (mpp13):current: uart1(txd), available = [ gpio(io) sdio(cmd) audio(rmclk) lcd(pwm) ] 14 (mpp14):current: uart1(rxd), available = [ gpio(io) sdio(d0) sata1(prsnt) audio(spdifi) audio-1(sdi) mii(col) ] 15 (mpp15):current: sata0(act), available = [ gpio(io) sdio(d1) uart0(rts) uart1(txd) spi(cs) ] 16 (mpp16):current: gpio(io), available = [ sdio(d2) uart0(cts) uart1(rxd) sata1(act) lcd(extclk) mii(crs) ] 17 (mpp17):current: gpio(io), available = [ sdio(d3) sata0(prsnt) sata1(act) twsi1(sck) ] 18 (mpp18):current: gpo(o), available = [ nand(io0) pex(clkreq) ] 19 (mpp19):current: gpo(o), available = [ nand(io1) ] 20 (mpp20):current: sata1(act), available = [ gpio(io) ts(mp0) tdm(tx0ql) ge1(txd0) audio(spdifi) lcd(d0) ] 21 (mpp21):current: sata0(act), available = [ gpio(io) ts(mp1) tdm(rx0ql) ge1(txd1) audio(spdifo) lcd(d1) ] 22 (mpp22):current: sata1(prsnt), available = [ gpio(io) ts(mp2) tdm(tx2ql) ge1(txd2) audio(rmclk) lcd(d2) ] 23 (mpp23):current: sata0(prsnt), available = [ gpio(io) ts(mp3) tdm(rx2ql) ge1(txd3) audio(bclk) lcd(d3) ] 24 (mpp24):current: gpio(io), available = [ ts(mp4) tdm(spi-cs0) ge1(rxd0) audio(sdo) lcd(d4) ] 25 (mpp25):current: gpio(io), available = [ ts(mp5) tdm(spi-sck) ge1(rxd1) audio(lrclk) lcd(d5) ] 26 (mpp26):current: gpio(io), available = [ ts(mp6) tdm(spi-miso) ge1(rxd2) audio(mclk) lcd(d6) ] 27 (mpp27):current: gpio(io), available = [ ts(mp7) tdm(spi-mosi) ge1(rxd3) audio(sdi) lcd(d7) ] 28 (mpp28):current: gpio(io), available = [ ts(mp8) tdm(int) ge1(col) audio(extclk) lcd(d8) ] 29 (mpp29):current: gpio(io), available = [ ts(mp9) tdm(rst) ge1(txclk) lcd(d9) ] 30 (mpp30):current: gpio(io), available = [ ts(mp10) tdm(pclk) ge1(rxctl) lcd(d10) ] 31 (mpp31):current: gpio(io), available = [ ts(mp11) tdm(fs) ge1(rxclk) lcd(d11) ] 32 (mpp32):current: gpio(io), available = [ ts(mp12) tdm(drx) ge1(txclko) lcd(d12) ] 33 (mpp33):current: gpo(o), available = [ tdm(dtx) ge1(txctl) lcd(d13) ] 34 (mpp34):current: gpio(io), available = [ tdm(spi-cs1) ge1(txen) sata1(act) lcd(d14) ] 35 (mpp35):current: gpio(io), available = [ tdm(tx0ql) ge1(rxerr) sata0(act) lcd(d15) mii(rxerr) ] 36 (mpp36):current: gpio(io), available = [ ts(mp0) tdm(spi-cs1) audio(spdifi) twsi1(sda) ] 37 (mpp37):current: gpio(io), available = [ ts(mp1) tdm(tx2ql) audio(spdifo) twsi1(sck) ] 38 (mpp38):current: gpio(io), available = [ ts(mp2) tdm(rx2ql) audio(rmclk) lcd(d18) ] 39 (mpp39):current: gpio(io), available = [ ts(mp3) tdm(spi-cs0) audio(bclk) lcd(d19) ] 40 (mpp40):current: gpio(io), available = [ ts(mp4) tdm(spi-sck) audio(sdo) lcd(d20) ] 41 (mpp41):current: gpio(io), available = [ ts(mp5) tdm(spi-miso) audio(lrclk) lcd(d21) ] 42 (mpp42):current: gpio(io), available = [ ts(mp6) tdm(spi-mosi) audio(mclk) lcd(d22) ] 43 (mpp43):current: gpio(io), available = [ ts(mp7) tdm(int) audio(sdi) lcd(d23) ] 44 (mpp44):current: gpio(io), available = [ ts(mp8) tdm(rst) audio(extclk) lcd(clk) ] 45 (mpp45):current: gpio(io), available = [ ts(mp9) tdm(pclk) lcd(e) ] 46 (mpp46):current: gpio(io), available = [ ts(mp10) tdm(fs) lcd(hsync) ] 47 (mpp47):current: gpio(io), available = [ ts(mp11) tdm(drx) lcd(vsync) ] 48 (mpp48):current: gpio(io), available = [ ts(mp12) tdm(dtx) lcd(d16) ] 49 (mpp49):current: gpo(o), available = [ tdm(rx0ql) pex(clkreq) lcd(d17) ] shows the current configuration of each pin. # cat /debug/pinctrl/f1010000.pinctrl/pinmux-pins Pinmux settings per pin Format: pin (name): mux_owner gpio_owner hog? pin 0 (PIN0): f1010000.pinctrl (GPIO UNCLAIMED) (HOG) function spi group mpp0 pin 1 (PIN1): f1010000.pinctrl (GPIO UNCLAIMED) (HOG) function spi group mpp1 pin 2 (PIN2): f1010000.pinctrl (GPIO UNCLAIMED) (HOG) function spi group mpp2 pin 3 (PIN3): f1010000.pinctrl (GPIO UNCLAIMED) (HOG) function spi group mpp3 pin 4 (PIN4): f1010000.pinctrl (GPIO UNCLAIMED) (HOG) function sata1 group mpp4 pin 5 (PIN5): f1010000.pinctrl (GPIO UNCLAIMED) (HOG) function sata0 group mpp5 pin 6 (PIN6): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 7 (PIN7): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 8 (PIN8): f1010000.pinctrl (GPIO UNCLAIMED) (HOG) function twsi0 group mpp8 pin 9 (PIN9): f1010000.pinctrl (GPIO UNCLAIMED) (HOG) function twsi0 group mpp9 pin 10 (PIN10): f1010000.pinctrl (GPIO UNCLAIMED) (HOG) function uart0 group mpp10 pin 11 (PIN11): f1010000.pinctrl (GPIO UNCLAIMED) (HOG) function uart0 group mpp11 pin 12 (PIN12): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 13 (PIN13): f1010000.pinctrl (GPIO UNCLAIMED) (HOG) function uart1 group mpp13 pin 14 (PIN14): f1010000.pinctrl (GPIO UNCLAIMED) (HOG) function uart1 group mpp14 pin 15 (PIN15): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 16 (PIN16): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 17 (PIN17): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 18 (PIN18): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 19 (PIN19): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 20 (PIN20): f1010000.pinctrl (GPIO UNCLAIMED) (HOG) function sata1 group mpp20 pin 21 (PIN21): f1010000.pinctrl (GPIO UNCLAIMED) (HOG) function sata0 group mpp21 pin 22 (PIN22): f1010000.pinctrl (GPIO UNCLAIMED) (HOG) function sata1 group mpp22 pin 23 (PIN23): f1010000.pinctrl (GPIO UNCLAIMED) (HOG) function sata0 group mpp23 pin 24 (PIN24): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 25 (PIN25): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 26 (PIN26): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 27 (PIN27): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 28 (PIN28): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 29 (PIN29): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 30 (PIN30): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 31 (PIN31): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 32 (PIN32): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 33 (PIN33): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 34 (PIN34): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 35 (PIN35): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 36 (PIN36): f1010000.pinctrl (GPIO UNCLAIMED) (HOG) function gpio group mpp36 pin 37 (PIN37): f1010000.pinctrl mvebu-gpio:37 (HOG) function gpio group mpp37 pin 38 (PIN38): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 39 (PIN39): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 40 (PIN40): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 41 (PIN41): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 42 (PIN42): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 43 (PIN43): f1010000.pinctrl mvebu-gpio:43 (HOG) function gpio group mpp43 pin 44 (PIN44): f1010000.pinctrl (GPIO UNCLAIMED) (HOG) function gpio group mpp44 pin 45 (PIN45): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 46 (PIN46): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 47 (PIN47): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 48 (PIN48): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 49 (PIN49): (MUX UNCLAIMED) (GPIO UNCLAIMED) This shows how pins have been configured via DT. If you compare the two, you can see that pin 6 has probably been set by uboot, but not by DT. Andrew
Dear Andrew Lunn, On Wed, 24 Oct 2012 22:15:45 +0200, Andrew Lunn wrote: > On Wed, Oct 24, 2012 at 09:18:01PM +0200, Thomas Petazzoni wrote: > > Note: this patch is a *RFC*, it is not intended for merging, only to > > get a discussion started. The code is horrible, makes terrible > > assumptions and so on. > > > > On many platforms, most of the pinmux initialization is done in the > > bootloader, and therefore persists when we boot the Linux kernel. This > > prevents us from making sure that the pinmux configuration in the > > board device trees is correct. > > You can get a lot of information from /debug. eg, > # cat /debug/pinctrl/f1010000.pinctrl/pinconf-groups Yes, I was using that one... > # cat /debug/pinctrl/f1010000.pinctrl/pinmux-pins > Pinmux settings per pin > Format: pin (name): mux_owner gpio_owner hog? ... but not that one. > If you compare the two, you can see that pin 6 has probably been set by > uboot, but not by DT. Indeed, by correlating the two files, you can get a good view of which pins are configured even though no driver has claimed them. I don't think it's as clear as having a non-functional device due to the pin not being muxed at all, but if it is thought as being sufficient, then fair enough. Best regards, Thomas
On Wed, Oct 24, 2012 at 9:18 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > On many platforms, most of the pinmux initialization is done in the > bootloader, and therefore persists when we boot the Linux kernel. This > prevents us from making sure that the pinmux configuration in the > board device trees is correct. > > One idea to make sure our device trees are correct in terms of > pinmuxing is to set the state of each pin to an unavailable function > during the initialization of the pinctrl driver. This way, only pins > that are explicitly configured through proper device tree attributes > will actually be functional. Now I don't know which kernel senior being it was that told me never to screw around with the defaults from the boot loader if not really needed. It is better if the driver reads the hardware to figure out what state it's in and move on from there. There may be cases where it's still needed, such as to save power, if pins (in this case) are set up such that they waste power unless the default setting is overridden. So I think it'd be nice if the actual reasons for doing this movement to a known state were known? Else the design pattern should be to discover the current state from the actual hardware registers. Depending on complexity the above may be a bit utopic... Yours, Linus Walleij
On Wed, Oct 24, 2012 at 10:21 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > On Wed, 24 Oct 2012 22:15:45 +0200, Andrew Lunn wrote: >> If you compare the two, you can see that pin 6 has probably been set by >> uboot, but not by DT. > > Indeed, by correlating the two files, you can get a good view of which > pins are configured even though no driver has claimed them. If it is useful, please consider introducing new debugfs files, that thing is supposed to be helpful. For example: if a <debugfs>/pinctrl/pinctrl-foo/pins-probe-state caching and providing the power-on-state of all pins is useful, then just add that mechanism to the pinctrl core I'd say. Yours, Linus Walleij
On Thu, Oct 25, 2012 at 08:46:38AM +0200, Linus Walleij wrote: > Now I don't know which kernel senior being it was that told me > never to screw around with the defaults from the boot loader > if not really needed. It is better if the driver reads the hardware > to figure out what state it's in and move on from there. This depends on the platform quite a bit - there's two schools of thought on what bootloaders should do, and typically platforms decide on a per platform basis which they'll use: 1. The bootloader does enough to load the kernel and nothing more, the kernel should ignore anything the bootloader did. Some people prefer this as it places minimal reliance on the bootloader which may be of uncertain quality and minimises the need to upgrade the bootloader which is often highly risky. 2. The bootloader does all the pin setup and so on. This provides a place for board specific stuff and avoids the kernel having to know about lots of device variants. For example AIUI OMAP tends towards the second view (partly due to number of device variants) but for example Samsung platforms typically use the first method (their pinmux is pretty simple).
On 10/24/2012 01:18 PM, Thomas Petazzoni wrote: > Note: this patch is a *RFC*, it is not intended for merging, only to > get a discussion started. The code is horrible, makes terrible > assumptions and so on. > > On many platforms, most of the pinmux initialization is done in the > bootloader, and therefore persists when we boot the Linux kernel. This > prevents us from making sure that the pinmux configuration in the > board device trees is correct. Well, it's easy enough to determine that the kernel's configuration is correct as far as it goes, since it's applied, and if everything still works, the pinmux configuration is presumably correct. The issue is more that the kernel's configuration may be missing some entries, and hence relying on the bootloader having already set up some pins/groups. > One idea to make sure our device trees are correct in terms of > pinmuxing is to set the state of each pin to an unavailable function > during the initialization of the pinctrl driver. This way, only pins > that are explicitly configured through proper device tree attributes > will actually be functional. On Tegra at least, and I imagine many SoCs, there is not an "unavailable" function for each pin/group. Hence, this would be impossible to implement. For pins/groups where there is a mux function that is reserved or actually does do nothing, the value of that mux function is potentially different per pin/group. At least I suppose we could force tristate on for all pingroups, so no output signals would work. > Remaining questions to solve: > > * Is this useful? I can certainly see the use of this, as an explicitly enabled option used for testing at least. On Tegra, we've certainly had issues where the kernel accidentally relies on the bootloader having set things up, so when switching to a different lighter-weight bootloader, things stop working. That was more w.r.t. clocks, but it could easily happen for pinmux too. > * How to figure out what function number is a good function number to > set all pins to at startup? It could be passed by the SoC-specific > pinctrl drivers. Yes, this would have to be either implemented by individual drivers, or use data passed in by the individual drivers. > * Maybe some pins should be excluded for this, especially if the > console serial port pins are muxed. On Armada XP, it's not the > case: UART0 RX/UART0 TX pins are not part of MPPs, so we can clear > all pins. But on other mvebu platforms, it may be the case. I don't think excluding the serial port pins would be useful. Presumably, if you enable this option, you're doing so explicitly to test the pinmux setup after you've tested that the kernel otherwise works correctly. As such, if the serial port stops working when you flip on this option, it's a pretty good clue that the serial port pinmux setup isn't correct. Presumably, you might also enable earlyprintk over the UART when debugging with this new option. > * If this sounds like an interesting thing, should we keep it only at > the mvebu driver level, or make it something more generically > available in the pinctrl subsystem? Making this generic is probably complex for reasons I outlined above. I'd tend towards a common kernel Kconfig or cmdline option to trigger this, but have each driver use that option as it sees fit in its own custom code. I imagine that will reduce the overall amount of code it takes to implement this. If using a cmdline option, the pinmux core should parse it, and set up some variable or function for the drivers to query.
diff --git a/drivers/pinctrl/pinctrl-mvebu.c b/drivers/pinctrl/pinctrl-mvebu.c index 8e6266c..32a9cbe 100644 --- a/drivers/pinctrl/pinctrl-mvebu.c +++ b/drivers/pinctrl/pinctrl-mvebu.c @@ -739,6 +739,9 @@ int __devinit mvebu_pinctrl_probe(struct platform_device *pdev) dev_info(&pdev->dev, "registered pinctrl driver\n"); + for (n = 0; n < pctl->num_groups; n++) + mvebu_pinconf_group_set(pctl->pctldev, n, 7); + /* register gpio ranges */ for (n = 0; n < soc->ngpioranges; n++) pinctrl_add_gpio_range(pctl->pctldev, &soc->gpioranges[n]);
Note: this patch is a *RFC*, it is not intended for merging, only to get a discussion started. The code is horrible, makes terrible assumptions and so on. On many platforms, most of the pinmux initialization is done in the bootloader, and therefore persists when we boot the Linux kernel. This prevents us from making sure that the pinmux configuration in the board device trees is correct. One idea to make sure our device trees are correct in terms of pinmuxing is to set the state of each pin to an unavailable function during the initialization of the pinctrl driver. This way, only pins that are explicitly configured through proper device tree attributes will actually be functional. Remaining questions to solve: * Is this useful? * How to figure out what function number is a good function number to set all pins to at startup? It could be passed by the SoC-specific pinctrl drivers. * Maybe some pins should be excluded for this, especially if the console serial port pins are muxed. On Armada XP, it's not the case: UART0 RX/UART0 TX pins are not part of MPPs, so we can clear all pins. But on other mvebu platforms, it may be the case. * If this sounds like an interesting thing, should we keep it only at the mvebu driver level, or make it something more generically available in the pinctrl subsystem? Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- drivers/pinctrl/pinctrl-mvebu.c | 3 +++ 1 file changed, 3 insertions(+)