| Message ID | 1351090434-30499-2-git-send-email-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dear Andrew Lunn, On Wed, 24 Oct 2012 16:53:46 +0200, Andrew Lunn wrote: > + pinctrl-0 = < &pmx_uart0 &pmx_uart1 &pmx_spi > + &pmx_twsi0 &pmx_sata0 &pmx_sata1 > + &pmx_ram_size &pmx_reset_button > + &pmx_USB_copy_button &pmx_board_id>; It would be really better to have those under each device, rather than globally declared here. For some devices such as UARTs, it is not yet possible with the 8250 driver to associate pinctrl pins (but I'm planning to work on that soon). However for the other drivers (SPI, TWSI, SATA, button), it should be possible. > + pinctrl-names = "default"; > + > + pmx_uart0: pmx-uart0 { > + marvell,pins = "mpp10", "mpp11"; > + marvell,function = "uart0"; > + }; > + pmx_uart1: pmx-uart1 { > + marvell,pins = "mpp13", "mpp14"; > + marvell,function = "uart1"; > + }; > + pmx_spi: pmx-spi { > + marvell,pins = "mpp0", "mpp1", "mpp2", "mpp3"; > + marvell,function = "spi"; > + }; > + pmx_twsi0: pmx-twsi0 { > + marvell,pins = "mpp8", "mpp9"; > + marvell,function = "twsi0"; > + }; > + pmx_sata0: pmx-sata0 { > + marvell,pins = "mpp5", "mpp21", "mpp23"; > + marvell,function = "sata0"; > + }; > + pmx_sata1: pmx-sata1 { > + marvell,pins = "mpp4", "mpp20", "mpp22"; > + marvell,function = "sata1"; > + }; All those definitions are not board specific, they are common to the SoC. So they should be in the corresponding .dtsi file. Basically: * The SoC .dtsi file should define all the pinmux groups that are described in the datasheet and are used by boards. I.e, there may be conflicting groups defined, where one group configures pin X with function Y, while another group configures pin X with function Z. * The board .dts file should define the pinmux groups that are really board-specific (buttons, LEDs, etc.), and then for each device, point to the correct pinmux group (either defined in the .dtsi file or in the board file). See for example imx28.dtsi, and then the boards such as imx28-cfa10036.dts, imx28-evk.dts, imx28-m28evk.dts, imx28-tx28.dts. Best regards, Thomas
On 10/24/2012 09:31 PM, Thomas Petazzoni wrote: > Dear Andrew Lunn, > > On Wed, 24 Oct 2012 16:53:46 +0200, Andrew Lunn wrote: > >> + pinctrl-0 =< &pmx_uart0&pmx_uart1&pmx_spi >> + &pmx_twsi0&pmx_sata0&pmx_sata1 >> + &pmx_ram_size&pmx_reset_button >> + &pmx_USB_copy_button&pmx_board_id>; > > It would be really better to have those under each device, rather than > globally declared here. For some devices such as UARTs, it is not yet > possible with the 8250 driver to associate pinctrl pins (but I'm > planning to work on that soon). However for the other drivers (SPI, > TWSI, SATA, button), it should be possible. Thomas, Agree, but for now a global pinhog on pinctrl node itself is the correct way to go here. As soon as there is pinctrl support in the specific device drivers we can move pinmux phandle there. >> + pmx_uart0: pmx-uart0 { >> + marvell,pins = "mpp10", "mpp11"; >> + marvell,function = "uart0"; >> + }; >> ... > > All those definitions are not board specific, they are common to the > SoC. So they should be in the corresponding .dtsi file. > > Basically: > > * The SoC .dtsi file should define all the pinmux groups that are > described in the datasheet and are used by boards. I.e, there may be > conflicting groups defined, where one group configures pin X with > function Y, while another group configures pin X with function Z. Here I disagree. Even quite simple interfaces like uart can have dozens of possible mpp configurations, e.g. rx/tx on up to three different pins each plus rts/cts on various pins plus all possible combinations. Now consider some more complex interface with more than one mpp pin per interface pin. Do you really want to predefine all possible combinations even if it is more likely that in fact only one is used on all boards because they are all based on the same reference design? > * The board .dts file should define the pinmux groups that are really > board-specific (buttons, LEDs, etc.), and then for each device, > point to the correct pinmux group (either defined in the .dtsi file > or in the board file). With respect to mpp the actual configuration _is_ board specific. There are maybe only some pins that are always used if a specific interface is used, e.g. nand pins on dove can only be switched with gpio. Sebastian
Sebastian, On Wed, 24 Oct 2012 21:49:45 +0200, Sebastian Hesselbarth wrote: > Agree, but for now a global pinhog on pinctrl node itself is the correct > way to go here. As soon as there is pinctrl support in the specific > device drivers we can move pinmux phandle there. I agree. > Here I disagree. Even quite simple interfaces like uart can have dozens > of possible mpp configurations, e.g. rx/tx on up to three different pins > each plus rts/cts on various pins plus all possible combinations. > > Now consider some more complex interface with more than one mpp pin per > interface pin. Do you really want to predefine all possible combinations > even if it is more likely that in fact only one is used on all boards > because they are all based on the same reference design? Where did I say that you should define *all* possible configurations? I said: "The SoC .dtsi file should define all the pinmux groups that are described in the datasheet and are used by boards". Read again the "and are used by boards". So I'm clearly not advocating adding *all* possible configurations, because there would be gazillions of them. But I'm in favor of moving the *used* configurations to the .dtsi files. > > * The board .dts file should define the pinmux groups that are really > > board-specific (buttons, LEDs, etc.), and then for each device, > > point to the correct pinmux group (either defined in the .dtsi file > > or in the board file). > > With respect to mpp the actual configuration _is_ board specific. There > are maybe only some pins that are always used if a specific interface is > used, e.g. nand pins on dove can only be switched with gpio. I am not sure to follow you here. There is clearly some MPP pins whose muxing groups can be defined in the SoC level file, and some other MPP pins whose muxing should entirely be done at the board level file. For a UART, the .dtsi file can define a pinmux configuration for RX/TX/RTS/CTS, and the .dts file only needs to reference this configuration. For LEDs, buttons and similar things, the pinmux configuration should be defined in the .dts file, and of course the .dts file will reference this configuration. I think what Shawn Guo did with i.MX 28 is really neat. In the current patches posted by Andrew, for example, the following piece: + pmx_uart1: pmx-uart1 { + marvell,pins = "mpp13", "mpp14"; + marvell,function = "uart1"; + }; is needlessly repeated in kirkwood-ts219-6281.dts, kirkwood-ts219-6282.dts and kirkwood-dnskw.dtsi. This is clearly a pinmux configuration that sets up TXD/RXD of UART1, and it should be in kirkwood.dtsi. Best regards, Thomas
> I think what Shawn Guo did with i.MX 28 is really neat. In the current > patches posted by Andrew, for example, the following piece: > > + pmx_uart1: pmx-uart1 { > + marvell,pins = "mpp13", "mpp14"; > + marvell,function = "uart1"; > + }; > > is needlessly repeated in kirkwood-ts219-6281.dts, > kirkwood-ts219-6282.dts and kirkwood-dnskw.dtsi. This is clearly a > pinmux configuration that sets up TXD/RXD of UART1, and it should be in > kirkwood.dtsi. I did try that, but was getting errors from dtc. Andrew
Andrew, On Wed, 24 Oct 2012 22:04:00 +0200, Andrew Lunn wrote: > > is needlessly repeated in kirkwood-ts219-6281.dts, > > kirkwood-ts219-6282.dts and kirkwood-dnskw.dtsi. This is clearly a > > pinmux configuration that sets up TXD/RXD of UART1, and it should be in > > kirkwood.dtsi. > > I did try that, but was getting errors from dtc. I can probably help figuring out what the problem is. Do you have a patch that exhibits the problem so I can test it here? Thanks, Thomas
On 10/24/2012 10:00 PM, Thomas Petazzoni wrote: > On Wed, 24 Oct 2012 21:49:45 +0200, Sebastian Hesselbarth wrote: >> Now consider some more complex interface with more than one mpp pin per >> interface pin. Do you really want to predefine all possible combinations >> even if it is more likely that in fact only one is used on all boards >> because they are all based on the same reference design? > > Where did I say that you should define *all* possible configurations? > > I said: "The SoC .dtsi file should define all the pinmux groups that are > described in the datasheet and are used by boards". Read again the "and > are used by boards". Ok, then I overread "and used by other boards". Sorry for that. > So I'm clearly not advocating adding *all* possible configurations, > because there would be gazillions of them. But I'm in favor of moving > the *used* configurations to the .dtsi files. Agreed. Sebastian
Sebastian, On Wed, 24 Oct 2012 22:14:10 +0200, Sebastian Hesselbarth wrote: > > Where did I say that you should define *all* possible configurations? > > > > I said: "The SoC .dtsi file should define all the pinmux groups that are > > described in the datasheet and are used by boards". Read again the "and > > are used by boards". > > Ok, then I overread "and used by other boards". Sorry for that. No problem :) > > So I'm clearly not advocating adding *all* possible configurations, > > because there would be gazillions of them. But I'm in favor of moving > > the *used* configurations to the .dtsi files. > > Agreed. Great! Thomas
diff --git a/arch/arm/boot/dts/kirkwood-ts219-6281.dts b/arch/arm/boot/dts/kirkwood-ts219-6281.dts index ccbf327..4d652ce 100644 --- a/arch/arm/boot/dts/kirkwood-ts219-6281.dts +++ b/arch/arm/boot/dts/kirkwood-ts219-6281.dts @@ -3,6 +3,62 @@ /include/ "kirkwood-ts219.dtsi" / { + ocp@f1000000 { + pinctrl: pinctrl@10000 { + compatible = "marvell,88f6281-pinctrl"; + reg = <0x10000 0x20>; + + pinctrl-0 = < &pmx_uart0 &pmx_uart1 &pmx_spi + &pmx_twsi0 &pmx_sata0 &pmx_sata1 + &pmx_ram_size &pmx_reset_button + &pmx_USB_copy_button &pmx_board_id>; + pinctrl-names = "default"; + + pmx_uart0: pmx-uart0 { + marvell,pins = "mpp10", "mpp11"; + marvell,function = "uart0"; + }; + pmx_uart1: pmx-uart1 { + marvell,pins = "mpp13", "mpp14"; + marvell,function = "uart1"; + }; + pmx_spi: pmx-spi { + marvell,pins = "mpp0", "mpp1", "mpp2", "mpp3"; + marvell,function = "spi"; + }; + pmx_twsi0: pmx-twsi0 { + marvell,pins = "mpp8", "mpp9"; + marvell,function = "twsi0"; + }; + pmx_sata0: pmx-sata0 { + marvell,pins = "mpp5", "mpp21", "mpp23"; + marvell,function = "sata0"; + }; + pmx_sata1: pmx-sata1 { + marvell,pins = "mpp4", "mpp20", "mpp22"; + marvell,function = "sata1"; + }; + pmx_ram_size: pmx-ram-size { + /* RAM: 0: 256 MB, 1: 512 MB */ + marvell,pins = "mpp36"; + marvell,function = "gpio"; + }; + pmx_USB_copy_button: pmx-USB-copy-button { + marvell,pins = "mpp15"; + marvell,function = "gpio"; + }; + pmx_reset_button: pmx-reset-button { + marvell,pins = "mpp16"; + marvell,function = "gpio"; + }; + pmx_board_id: pmx-board-id { + /* 0: TS-11x, 1: TS-21x */ + marvell,pins = "mpp44"; + marvell,function = "gpio"; + }; + }; + }; + gpio_keys { compatible = "gpio-keys"; #address-cells = <1>; diff --git a/arch/arm/boot/dts/kirkwood-ts219-6282.dts b/arch/arm/boot/dts/kirkwood-ts219-6282.dts index fbe9932..8c3d720 100644 --- a/arch/arm/boot/dts/kirkwood-ts219-6282.dts +++ b/arch/arm/boot/dts/kirkwood-ts219-6282.dts @@ -3,6 +3,62 @@ /include/ "kirkwood-ts219.dtsi" / { + ocp@f1000000 { + pinctrl: pinctrl@10000 { + compatible = "marvell,88f6282-pinctrl"; + reg = <0x10000 0x20>; + + pinctrl-0 = < &pmx_uart0 &pmx_uart1 &pmx_spi + &pmx_twsi0 &pmx_sata0 &pmx_sata1 + &pmx_ram_size &pmx_reset_button + &pmx_USB_copy_button &pmx_board_id>; + pinctrl-names = "default"; + + pmx_uart0: pmx-uart0 { + marvell,pins = "mpp10", "mpp11"; + marvell,function = "uart0"; + }; + pmx_uart1: pmx-uart1 { + marvell,pins = "mpp13", "mpp14"; + marvell,function = "uart1"; + }; + pmx_spi: pmx-spi { + marvell,pins = "mpp0", "mpp1", "mpp2", "mpp3"; + marvell,function = "spi"; + }; + pmx_twsi0: pmx-twsi0 { + marvell,pins = "mpp8", "mpp9"; + marvell,function = "twsi0"; + }; + pmx_sata0: pmx-sata0 { + marvell,pins = "mpp5", "mpp21", "mpp23"; + marvell,function = "sata0"; + }; + pmx_sata1: pmx-sata1 { + marvell,pins = "mpp4", "mpp20", "mpp22"; + marvell,function = "sata1"; + }; + pmx_ram_size: pmx-ram-size { + /* RAM: 0: 256 MB, 1: 512 MB */ + marvell,pins = "mpp36"; + marvell,function = "gpio"; + }; + pmx_reset_button: pmx-reset-button { + marvell,pins = "mpp37"; + marvell,function = "gpio"; + }; + pmx_USB_copy_button: pmx-USB-copy-button { + marvell,pins = "mpp43"; + marvell,function = "gpio"; + }; + pmx_board_id: pmx-board-id { + /* 0: TS-11x, 1: TS-21x */ + marvell,pins = "mpp44"; + marvell,function = "gpio"; + }; + }; + }; + gpio_keys { compatible = "gpio-keys"; #address-cells = <1>; diff --git a/arch/arm/mach-kirkwood/board-ts219.c b/arch/arm/mach-kirkwood/board-ts219.c index 1750e68..47c8287 100644 --- a/arch/arm/mach-kirkwood/board-ts219.c +++ b/arch/arm/mach-kirkwood/board-ts219.c @@ -26,41 +26,16 @@ #include <asm/mach/arch.h> #include <mach/kirkwood.h> #include "common.h" -#include "mpp.h" #include "tsx1x-common.h" static struct mv643xx_eth_platform_data qnap_ts219_ge00_data = { .phy_addr = MV643XX_ETH_PHY_ADDR(8), }; -static unsigned int qnap_ts219_mpp_config[] __initdata = { - MPP0_SPI_SCn, - MPP1_SPI_MOSI, - MPP2_SPI_SCK, - MPP3_SPI_MISO, - MPP4_SATA1_ACTn, - MPP5_SATA0_ACTn, - MPP8_TW0_SDA, - MPP9_TW0_SCK, - MPP10_UART0_TXD, - MPP11_UART0_RXD, - MPP13_UART1_TXD, /* PIC controller */ - MPP14_UART1_RXD, /* PIC controller */ - MPP15_GPIO, /* USB Copy button (on devices with 88F6281) */ - MPP16_GPIO, /* Reset button (on devices with 88F6281) */ - MPP36_GPIO, /* RAM: 0: 256 MB, 1: 512 MB */ - MPP37_GPIO, /* Reset button (on devices with 88F6282) */ - MPP43_GPIO, /* USB Copy button (on devices with 88F6282) */ - MPP44_GPIO, /* Board ID: 0: TS-11x, 1: TS-21x */ - 0 -}; - void __init qnap_dt_ts219_init(void) { u32 dev, rev; - kirkwood_mpp_conf(qnap_ts219_mpp_config); - kirkwood_pcie_id(&dev, &rev); if (dev == MV88F6282_DEV_ID) qnap_ts219_ge00_data.phy_addr = MV643XX_ETH_PHY_ADDR(0);
Make use of the pinctrl driver for configuring all the pins, instead of using the Orion mpp code. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- arch/arm/boot/dts/kirkwood-ts219-6281.dts | 56 +++++++++++++++++++++++++++++ arch/arm/boot/dts/kirkwood-ts219-6282.dts | 56 +++++++++++++++++++++++++++++ arch/arm/mach-kirkwood/board-ts219.c | 25 ------------- 3 files changed, 112 insertions(+), 25 deletions(-)