| Message ID | 1425427237-11511-2-git-send-email-nm@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 4, 2015 at 1:00 AM, Nishanth Menon <nm@ti.com> wrote: > +Configuration definition follows similar model as the pinctrl-single: > +The groups of pin configuration are defined under "pinctrl-single,pins" > + > +&dra7_iodelay_core { > + mmc2_iodelay_3v3_conf: mmc2_iodelay_3v3_conf { > + pinctrl-single,pins = < > + 0x18c (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A19_IN */ > + 0x1a4 (A_DELAY(265) | G_DELAY(360)) /* CFG_GPMC_A20_IN */ > + 0x1b0 (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A21_IN */ > + 0x1bc (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A22_IN */ > + 0x1c8 (A_DELAY(287) | G_DELAY(420)) /* CFG_GPMC_A23_IN */ > + 0x1d4 (A_DELAY(144) | G_DELAY(240)) /* CFG_GPMC_A24_IN */ > + 0x1e0 (A_DELAY(0) | G_DELAY(0)) /* CFG_GPMC_A25_IN */ > + 0x1ec (A_DELAY(120) | G_DELAY(0)) /* CFG_GPMC_A26_IN */ > + 0x1f8 (A_DELAY(120) | G_DELAY(180)) /* CFG_GPMC_A27_IN */ > + 0x360 (A_DELAY(0) | G_DELAY(0)) /* CFG_GPMC_CS1_IN */ > + >; > + }; > +}; But wait. The promise when we merged pinctrl-single was that this driver was to be used when the system had a one-register-per-pin layout and it was easy to do device trees based on that. We were very reluctant to accept that even though we didn't even have the generic pin control bindings in place, the argument being that the driver should know the detailed register layout, it should not be described in the device tree. We eventually caved in and accepted it as an exception. Since this pin controller is not using pinctrl-single it does not enjoy that privilege and you need to explain why this pin controller cannot use the generic bindings like so many other pin controllers have since started to do. ("It is in the same SoC" is not an acceptable argument.) What is wrong with this: Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt We can add generic delay bindings to the list, it even seems like a good idea to do so, as it is likely something that will come up in other hardware and will be needed for ACPI etc going forward. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/10/2015 05:39 AM, Linus Walleij wrote: > On Wed, Mar 4, 2015 at 1:00 AM, Nishanth Menon <nm@ti.com> wrote: > >> +Configuration definition follows similar model as the pinctrl-single: >> +The groups of pin configuration are defined under "pinctrl-single,pins" >> + >> +&dra7_iodelay_core { >> + mmc2_iodelay_3v3_conf: mmc2_iodelay_3v3_conf { >> + pinctrl-single,pins = < >> + 0x18c (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A19_IN */ >> + 0x1a4 (A_DELAY(265) | G_DELAY(360)) /* CFG_GPMC_A20_IN */ >> + 0x1b0 (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A21_IN */ >> + 0x1bc (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A22_IN */ >> + 0x1c8 (A_DELAY(287) | G_DELAY(420)) /* CFG_GPMC_A23_IN */ >> + 0x1d4 (A_DELAY(144) | G_DELAY(240)) /* CFG_GPMC_A24_IN */ >> + 0x1e0 (A_DELAY(0) | G_DELAY(0)) /* CFG_GPMC_A25_IN */ >> + 0x1ec (A_DELAY(120) | G_DELAY(0)) /* CFG_GPMC_A26_IN */ >> + 0x1f8 (A_DELAY(120) | G_DELAY(180)) /* CFG_GPMC_A27_IN */ >> + 0x360 (A_DELAY(0) | G_DELAY(0)) /* CFG_GPMC_CS1_IN */ >> + >; >> + }; >> +}; > > But wait. > > The promise when we merged pinctrl-single was that this driver was to be used > when the system had a one-register-per-pin layout and it was easy to do device > trees based on that. > > We were very reluctant to accept that even though we didn't even have the > generic pin control bindings in place, the argument being that the driver > should know the detailed register layout, it should not be described in the > device tree. We eventually caved in and accepted it as an exception. > > Since this pin controller is not using pinctrl-single it does not enjoy that > privilege and you need to explain why this pin controller cannot use the > generic bindings like so many other pin controllers have since started > to do. ("It is in the same SoC" is not an acceptable argument.) > > What is wrong with this: > Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > > We can add generic delay bindings to the list, it even seems like > a good idea to do so, as it is likely something that will come up in > other hardware and will be needed for ACPI etc going forward. Let me try to explain how the hardware works in this instance - it is a quirk that we had'nt understood as being mandatory until very recently. Apologies on pointing to TRM. Unfortunately, understanding this kind of needs us to understand the hardware a little deeper. http://www.ti.com/lit/ug/spruhz6/spruhz6.pdf 18.5.2 CTRL_MODULE_CORE registers (page 4040) -> mux starts from CTRL_CORE_PAD_GPMC_AD0 (page 4390) This is the basic support needed for DRA7 family of SoC handled by pinctrl-single. a single register for a single pin -> write the mux mode, internal pull, wakeup capability etc. handled today as part of pinctrl-single compatible "ti,dra7-padconf". This works for almost 95%+ of the pins. few pins need tweaking of the delay parameters to address per die, operational and board(rarely) characteristics. Here is where it gets a little complicated. 18.6.1 IODELAYCONFIG Module Instance Summary (page 4798) - starts at CFG_RMII_MHZ_50_CLK_IN. These registers are programmed per "18.4.6.1.4 Manual IO Timing Modes" (page 4004). Initial sequence of recalibration involves IO isolation - process involving isolating every DRA7 pin from the external world - The only logical place to do this is obviously as part of bootloader. Doing this from kernel can be more than rationally complicated (IO isolation for doing recalibration while a video playback or coprocessor like DSP is active is just plain ridiculous in complexity). The calibrated values then are read for programming these iodelay registers per pin as described in the Section "18.4.6.1.4 Manual IO Timing Modes" (page 4005). In summary: - This does not really control traditional points of pinctrl control such as mux mode, pull direction etc. It is, in short, a tweaking of delay paths from the IP to the external pin. - Most pins do not need iodelay register programming. The ones that do may have upto 3 other registers that may need programming (IN, OUT, OUTEN) - Unlike pinctrl-single where a value is read from dts and programmed straight to the register, programming iodelay involves taking two parameter(a_delay and g_delay) per iodelay register, value for the registers computed based on two other parameters(cdpe, fdpe). Where cdpe and fdpe are computed based on "recalibration sequence" generated values programmed in register fields for ref_count, delay_count and ref_clk_period. - This is also why pinctrl-single wont work here - it is not a copy from dts to register sequence, it is a compute from dts to register sequence. - The recalibration parameters ref_count, delay_count and ref_clk_period are die dependent (hence the need of recalibration sequence). - The parameters a_delay, g_delay are dependent on operational mode/board properties. - This is why such a data in kernel may not scale with multitude of board variations and operational needs of certain pins. - The values themselves come from some automated tool/data sheet with per-board tweaking done under simulation. Considering this was a "configuration of per pin iodelay", i chose pinconf-like model here. Unfortunately, The traditional pinconf is a set of properties for a pin, but it does not give information as to which register those properties belong to. since these registers do not form the same pattern of offsets as padconf register, that iodelay register information will need to be encoded in some form. The traditional pinctrl does not fit this hardware either. it does not control mux, rather given that pinctrl has selected a specific mux mode, it tweaks the delay parameters corresponding to that mux. This is one of those modules that I cant seem to fit neatly in any of the existing frameworks, I am most happy to understand if there are alternatives that can be proposed..
* Linus Walleij <linus.walleij@linaro.org> [150310 03:39]: > On Wed, Mar 4, 2015 at 1:00 AM, Nishanth Menon <nm@ti.com> wrote: > > > +Configuration definition follows similar model as the pinctrl-single: > > +The groups of pin configuration are defined under "pinctrl-single,pins" > > + > > +&dra7_iodelay_core { > > + mmc2_iodelay_3v3_conf: mmc2_iodelay_3v3_conf { > > + pinctrl-single,pins = < > > + 0x18c (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A19_IN */ > > + 0x1a4 (A_DELAY(265) | G_DELAY(360)) /* CFG_GPMC_A20_IN */ > > + 0x1b0 (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A21_IN */ > > + 0x1bc (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A22_IN */ > > + 0x1c8 (A_DELAY(287) | G_DELAY(420)) /* CFG_GPMC_A23_IN */ > > + 0x1d4 (A_DELAY(144) | G_DELAY(240)) /* CFG_GPMC_A24_IN */ > > + 0x1e0 (A_DELAY(0) | G_DELAY(0)) /* CFG_GPMC_A25_IN */ > > + 0x1ec (A_DELAY(120) | G_DELAY(0)) /* CFG_GPMC_A26_IN */ > > + 0x1f8 (A_DELAY(120) | G_DELAY(180)) /* CFG_GPMC_A27_IN */ > > + 0x360 (A_DELAY(0) | G_DELAY(0)) /* CFG_GPMC_CS1_IN */ > > + >; > > + }; > > +}; > > But wait. > > The promise when we merged pinctrl-single was that this driver was to be used > when the system had a one-register-per-pin layout and it was easy to do device > trees based on that. > > We were very reluctant to accept that even though we didn't even have the > generic pin control bindings in place, the argument being that the driver > should know the detailed register layout, it should not be described in the > device tree. We eventually caved in and accepted it as an exception. Hey let's get few things straight here. There's nothing stopping the driver from knowing a detailed register layout with the pinctrl-single binding. This can be very easily done based on the compatible flag and match data as needed and check the values provided. And let's also recap the reasons for the pinctrl-single binding. The the main reason for the pinctrl-single binding is to avoid mapping individual register bits to device tree compatible string properties. Imagine doing that for hundreds of similar registers. Believe me, I tried using device tree properties initially and it just sucked big time. For larger amounts of dts data, it's best to stick to numeric values and phandles in the device tree data and rely on the preprocessor for getting the values right. Finally, we don't want to build databases into the kernel drivers for every SoC. We certainly have plenty fo those already. > Since this pin controller is not using pinctrl-single it does not enjoy that > privilege and you need to explain why this pin controller cannot use the > generic bindings like so many other pin controllers have since started > to do. ("It is in the same SoC" is not an acceptable argument.) > > What is wrong with this: > Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt Nishanth, care to explain your reasons for using pinctrl-single binding here vs the generic binding? Is the amount of string parsing with the data an issue here? > We can add generic delay bindings to the list, it even seems like > a good idea to do so, as it is likely something that will come up in > other hardware and will be needed for ACPI etc going forward. We certainly need to make setting delays (with values) generic, no doubt about that. If the large amount of data is not an issue here, we could maybe set each iodelay register as a separate device? Then the binding could be just along the interrupts-extended type binding: foo = <&bar 0x18c A_DELAY(0) G_DELAY(120)>; Where the 0x18c is the instance offset of the register within the ti,dra7-iodelay compatible controller. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/10/2015 10:33 AM, Tony Lindgren wrote: > * Linus Walleij <linus.walleij@linaro.org> [150310 03:39]: >> On Wed, Mar 4, 2015 at 1:00 AM, Nishanth Menon <nm@ti.com> wrote: >> >>> +Configuration definition follows similar model as the pinctrl-single: >>> +The groups of pin configuration are defined under "pinctrl-single,pins" >>> + >>> +&dra7_iodelay_core { >>> + mmc2_iodelay_3v3_conf: mmc2_iodelay_3v3_conf { >>> + pinctrl-single,pins = < >>> + 0x18c (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A19_IN */ >>> + 0x1a4 (A_DELAY(265) | G_DELAY(360)) /* CFG_GPMC_A20_IN */ >>> + 0x1b0 (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A21_IN */ >>> + 0x1bc (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A22_IN */ >>> + 0x1c8 (A_DELAY(287) | G_DELAY(420)) /* CFG_GPMC_A23_IN */ >>> + 0x1d4 (A_DELAY(144) | G_DELAY(240)) /* CFG_GPMC_A24_IN */ >>> + 0x1e0 (A_DELAY(0) | G_DELAY(0)) /* CFG_GPMC_A25_IN */ >>> + 0x1ec (A_DELAY(120) | G_DELAY(0)) /* CFG_GPMC_A26_IN */ >>> + 0x1f8 (A_DELAY(120) | G_DELAY(180)) /* CFG_GPMC_A27_IN */ >>> + 0x360 (A_DELAY(0) | G_DELAY(0)) /* CFG_GPMC_CS1_IN */ >>> + >; >>> + }; >>> +}; >> >> But wait. >> >> The promise when we merged pinctrl-single was that this driver was to be used >> when the system had a one-register-per-pin layout and it was easy to do device >> trees based on that. >> >> We were very reluctant to accept that even though we didn't even have the >> generic pin control bindings in place, the argument being that the driver >> should know the detailed register layout, it should not be described in the >> device tree. We eventually caved in and accepted it as an exception. > > Hey let's get few things straight here. There's nothing stopping the > driver from knowing a detailed register layout with the pinctrl-single > binding. This can be very easily done based on the compatible flag and > match data as needed and check the values provided. > > And let's also recap the reasons for the pinctrl-single binding. The > the main reason for the pinctrl-single binding is to avoid mapping > individual register bits to device tree compatible string properties. > > Imagine doing that for hundreds of similar registers. Believe me, I tried > using device tree properties initially and it just sucked big time. For > larger amounts of dts data, it's best to stick to numeric values and > phandles in the device tree data and rely on the preprocessor for > getting the values right. > > Finally, we don't want to build databases into the kernel drivers for > every SoC. We certainly have plenty fo those already. > >> Since this pin controller is not using pinctrl-single it does not enjoy that >> privilege and you need to explain why this pin controller cannot use the >> generic bindings like so many other pin controllers have since started >> to do. ("It is in the same SoC" is not an acceptable argument.) >> >> What is wrong with this: >> Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > > Nishanth, care to explain your reasons for using pinctrl-single binding > here vs the generic binding? Is the amount of string parsing with the > data an issue here? Wrong option chosen, I suppose :( - alright, lets discuss the alternative. > >> We can add generic delay bindings to the list, it even seems like >> a good idea to do so, as it is likely something that will come up in >> other hardware and will be needed for ACPI etc going forward. > > We certainly need to make setting delays (with values) generic, no > doubt about that. > > If the large amount of data is not an issue here, we could maybe set > each iodelay register as a separate device? Then the binding could > be just along the interrupts-extended type binding: > > foo = <&bar 0x18c A_DELAY(0) G_DELAY(120)>; > > Where the 0x18c is the instance offset of the register within the > ti,dra7-iodelay compatible controller. if mmc2_pins_default point at pins for mmc pin configuration for control_core (pinctrl-single), are you proposing the following? mmc2_pins_default: mmc2_pins_default { pinctrl-single,pins = < 0x9c (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_a23.mmc2_clk */ 0xb0 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_cs1.mmc2_cmd */ 0xa0 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_a24.mmc2_dat0 */ 0xa4 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_a25.mmc2_dat1 */ 0xa8 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_a26.mmc2_dat2 */ 0xac (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_a27.mmc2_dat3 */ 0x8c (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_a19.mmc2_dat4 */ 0x90 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_a20.mmc2_dat5 */ 0x94 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_a21.mmc2_dat6 */ 0x98 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_a22.mmc2_dat7 */ >; }; &mmc2 { ... pinctrl-1 = &mmc2_pins_default, /* points to mmc control core pins */ <&iodelay 0x18c A_DELAY(0) | G_DELAY(120)>, /* CFG_GPMC_A19_IN */ <&iodelay 0x1a4 A_DELAY(265) | G_DELAY(360)>, /* CFG_GPMC_A20_IN */ <&iodelay 0x1b0 A_DELAY(0) | G_DELAY(120)>, /* CFG_GPMC_A21_IN */ <&iodelay 0x1bc A_DELAY(0) | G_DELAY(120)>, /* CFG_GPMC_A22_IN */ <&iodelay 0x1c8 A_DELAY(287) | G_DELAY(420)>, /* CFG_GPMC_A23_IN */ <&iodelay 0x1d4 A_DELAY(144) | G_DELAY(240)>, /* CFG_GPMC_A24_IN */ <&iodelay 0x1e0 A_DELAY(0) | G_DELAY(0)>, /* CFG_GPMC_A25_IN */ <&iodelay 0x1ec A_DELAY(120) | G_DELAY(0)>, /* CFG_GPMC_A26_IN */ <&iodelay 0x1f8 A_DELAY(120) | G_DELAY(180)>, /* CFG_GPMC_A27_IN */ <&iodelay 0x360 A_DELAY(0) | G_DELAY(0)>; /* CFG_GPMC_CS1_IN */ I have to check if we are capable of parsing that. but if that is the approach chosen, I suppose we might be able to figure something, I suppose..
* Nishanth Menon <nm@ti.com> [150310 10:25]: > On 03/10/2015 10:33 AM, Tony Lindgren wrote: > > * Linus Walleij <linus.walleij@linaro.org> [150310 03:39]: > >> On Wed, Mar 4, 2015 at 1:00 AM, Nishanth Menon <nm@ti.com> wrote: > >> > >>> +Configuration definition follows similar model as the pinctrl-single: > >>> +The groups of pin configuration are defined under "pinctrl-single,pins" > >>> + > >>> +&dra7_iodelay_core { > >>> + mmc2_iodelay_3v3_conf: mmc2_iodelay_3v3_conf { > >>> + pinctrl-single,pins = < > >>> + 0x18c (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A19_IN */ > >>> + 0x1a4 (A_DELAY(265) | G_DELAY(360)) /* CFG_GPMC_A20_IN */ > >>> + 0x1b0 (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A21_IN */ > >>> + 0x1bc (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A22_IN */ > >>> + 0x1c8 (A_DELAY(287) | G_DELAY(420)) /* CFG_GPMC_A23_IN */ > >>> + 0x1d4 (A_DELAY(144) | G_DELAY(240)) /* CFG_GPMC_A24_IN */ > >>> + 0x1e0 (A_DELAY(0) | G_DELAY(0)) /* CFG_GPMC_A25_IN */ > >>> + 0x1ec (A_DELAY(120) | G_DELAY(0)) /* CFG_GPMC_A26_IN */ > >>> + 0x1f8 (A_DELAY(120) | G_DELAY(180)) /* CFG_GPMC_A27_IN */ > >>> + 0x360 (A_DELAY(0) | G_DELAY(0)) /* CFG_GPMC_CS1_IN */ > >>> + >; > >>> + }; > >>> +}; > >> > >> But wait. > >> > >> The promise when we merged pinctrl-single was that this driver was to be used > >> when the system had a one-register-per-pin layout and it was easy to do device > >> trees based on that. > >> > >> We were very reluctant to accept that even though we didn't even have the > >> generic pin control bindings in place, the argument being that the driver > >> should know the detailed register layout, it should not be described in the > >> device tree. We eventually caved in and accepted it as an exception. > > > > Hey let's get few things straight here. There's nothing stopping the > > driver from knowing a detailed register layout with the pinctrl-single > > binding. This can be very easily done based on the compatible flag and > > match data as needed and check the values provided. > > > > And let's also recap the reasons for the pinctrl-single binding. The > > the main reason for the pinctrl-single binding is to avoid mapping > > individual register bits to device tree compatible string properties. > > > > Imagine doing that for hundreds of similar registers. Believe me, I tried > > using device tree properties initially and it just sucked big time. For > > larger amounts of dts data, it's best to stick to numeric values and > > phandles in the device tree data and rely on the preprocessor for > > getting the values right. > > > > Finally, we don't want to build databases into the kernel drivers for > > every SoC. We certainly have plenty fo those already. > > > >> Since this pin controller is not using pinctrl-single it does not enjoy that > >> privilege and you need to explain why this pin controller cannot use the > >> generic bindings like so many other pin controllers have since started > >> to do. ("It is in the same SoC" is not an acceptable argument.) > >> > >> What is wrong with this: > >> Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > > > > Nishanth, care to explain your reasons for using pinctrl-single binding > > here vs the generic binding? Is the amount of string parsing with the > > data an issue here? > > Wrong option chosen, I suppose :( - alright, lets discuss the alternative. Heh well now we know :) > >> We can add generic delay bindings to the list, it even seems like > >> a good idea to do so, as it is likely something that will come up in > >> other hardware and will be needed for ACPI etc going forward. > > > > We certainly need to make setting delays (with values) generic, no > > doubt about that. > > > > If the large amount of data is not an issue here, we could maybe set > > each iodelay register as a separate device? Then the binding could > > be just along the interrupts-extended type binding: > > > > foo = <&bar 0x18c A_DELAY(0) G_DELAY(120)>; > > > > Where the 0x18c is the instance offset of the register within the > > ti,dra7-iodelay compatible controller. > > if mmc2_pins_default point at pins for mmc pin configuration for > control_core (pinctrl-single), are you proposing the following? > > mmc2_pins_default: mmc2_pins_default { > pinctrl-single,pins = < > 0x9c (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* > gpmc_a23.mmc2_clk */ > 0xb0 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* > gpmc_cs1.mmc2_cmd */ > 0xa0 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* > gpmc_a24.mmc2_dat0 */ > 0xa4 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* > gpmc_a25.mmc2_dat1 */ > 0xa8 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* > gpmc_a26.mmc2_dat2 */ > 0xac (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* > gpmc_a27.mmc2_dat3 */ > 0x8c (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* > gpmc_a19.mmc2_dat4 */ > 0x90 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* > gpmc_a20.mmc2_dat5 */ > 0x94 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* > gpmc_a21.mmc2_dat6 */ > 0x98 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* > gpmc_a22.mmc2_dat7 */ > >; > }; Yeah so existing pinctrl-single binding above, with additional iodelay binding below.. > &mmc2 { > ... > pinctrl-1 = > &mmc2_pins_default, /* points to mmc control core pins */ > <&iodelay 0x18c A_DELAY(0) | G_DELAY(120)>, /* CFG_GPMC_A19_IN */ > <&iodelay 0x1a4 A_DELAY(265) | G_DELAY(360)>, /* CFG_GPMC_A20_IN */ > <&iodelay 0x1b0 A_DELAY(0) | G_DELAY(120)>, /* CFG_GPMC_A21_IN */ > <&iodelay 0x1bc A_DELAY(0) | G_DELAY(120)>, /* CFG_GPMC_A22_IN */ > <&iodelay 0x1c8 A_DELAY(287) | G_DELAY(420)>, /* CFG_GPMC_A23_IN */ > <&iodelay 0x1d4 A_DELAY(144) | G_DELAY(240)>, /* CFG_GPMC_A24_IN */ > <&iodelay 0x1e0 A_DELAY(0) | G_DELAY(0)>, /* CFG_GPMC_A25_IN */ > <&iodelay 0x1ec A_DELAY(120) | G_DELAY(0)>, /* CFG_GPMC_A26_IN */ > <&iodelay 0x1f8 A_DELAY(120) | G_DELAY(180)>, /* CFG_GPMC_A27_IN */ > <&iodelay 0x360 A_DELAY(0) | G_DELAY(0)>; /* CFG_GPMC_CS1_IN */ > > I have to check if we are capable of parsing that. but if that is the > approach chosen, I suppose we might be able to figure something, I > suppose.. Yes except I'd make use of some kind of #pinctrl-cells here just like interrupt controller has #interrupt-cells. Then you can have the values seprate and the controller knows what to do with them based on the compatible flag and #pinctrl-cells. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/10/2015 12:31 PM, Tony Lindgren wrote: > * Nishanth Menon <nm@ti.com> [150310 10:25]: >> On 03/10/2015 10:33 AM, Tony Lindgren wrote: >>> * Linus Walleij <linus.walleij@linaro.org> [150310 03:39]: >>>> On Wed, Mar 4, 2015 at 1:00 AM, Nishanth Menon <nm@ti.com> wrote: >>>> >>>>> +Configuration definition follows similar model as the pinctrl-single: >>>>> +The groups of pin configuration are defined under "pinctrl-single,pins" >>>>> + >>>>> +&dra7_iodelay_core { >>>>> + mmc2_iodelay_3v3_conf: mmc2_iodelay_3v3_conf { >>>>> + pinctrl-single,pins = < >>>>> + 0x18c (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A19_IN */ >>>>> + 0x1a4 (A_DELAY(265) | G_DELAY(360)) /* CFG_GPMC_A20_IN */ >>>>> + 0x1b0 (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A21_IN */ >>>>> + 0x1bc (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A22_IN */ >>>>> + 0x1c8 (A_DELAY(287) | G_DELAY(420)) /* CFG_GPMC_A23_IN */ >>>>> + 0x1d4 (A_DELAY(144) | G_DELAY(240)) /* CFG_GPMC_A24_IN */ >>>>> + 0x1e0 (A_DELAY(0) | G_DELAY(0)) /* CFG_GPMC_A25_IN */ >>>>> + 0x1ec (A_DELAY(120) | G_DELAY(0)) /* CFG_GPMC_A26_IN */ >>>>> + 0x1f8 (A_DELAY(120) | G_DELAY(180)) /* CFG_GPMC_A27_IN */ >>>>> + 0x360 (A_DELAY(0) | G_DELAY(0)) /* CFG_GPMC_CS1_IN */ >>>>> + >; >>>>> + }; >>>>> +}; >>>> >>>> But wait. >>>> >>>> The promise when we merged pinctrl-single was that this driver was to be used >>>> when the system had a one-register-per-pin layout and it was easy to do device >>>> trees based on that. >>>> >>>> We were very reluctant to accept that even though we didn't even have the >>>> generic pin control bindings in place, the argument being that the driver >>>> should know the detailed register layout, it should not be described in the >>>> device tree. We eventually caved in and accepted it as an exception. >>> >>> Hey let's get few things straight here. There's nothing stopping the >>> driver from knowing a detailed register layout with the pinctrl-single >>> binding. This can be very easily done based on the compatible flag and >>> match data as needed and check the values provided. >>> >>> And let's also recap the reasons for the pinctrl-single binding. The >>> the main reason for the pinctrl-single binding is to avoid mapping >>> individual register bits to device tree compatible string properties. >>> >>> Imagine doing that for hundreds of similar registers. Believe me, I tried >>> using device tree properties initially and it just sucked big time. For >>> larger amounts of dts data, it's best to stick to numeric values and >>> phandles in the device tree data and rely on the preprocessor for >>> getting the values right. >>> >>> Finally, we don't want to build databases into the kernel drivers for >>> every SoC. We certainly have plenty fo those already. >>> >>>> Since this pin controller is not using pinctrl-single it does not enjoy that >>>> privilege and you need to explain why this pin controller cannot use the >>>> generic bindings like so many other pin controllers have since started >>>> to do. ("It is in the same SoC" is not an acceptable argument.) >>>> >>>> What is wrong with this: >>>> Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt >>> >>> Nishanth, care to explain your reasons for using pinctrl-single binding >>> here vs the generic binding? Is the amount of string parsing with the >>> data an issue here? >> >> Wrong option chosen, I suppose :( - alright, lets discuss the alternative. > > Heh well now we know :) > >>>> We can add generic delay bindings to the list, it even seems like >>>> a good idea to do so, as it is likely something that will come up in >>>> other hardware and will be needed for ACPI etc going forward. >>> >>> We certainly need to make setting delays (with values) generic, no >>> doubt about that. >>> >>> If the large amount of data is not an issue here, we could maybe set >>> each iodelay register as a separate device? Then the binding could >>> be just along the interrupts-extended type binding: >>> >>> foo = <&bar 0x18c A_DELAY(0) G_DELAY(120)>; >>> >>> Where the 0x18c is the instance offset of the register within the >>> ti,dra7-iodelay compatible controller. >> >> if mmc2_pins_default point at pins for mmc pin configuration for >> control_core (pinctrl-single), are you proposing the following? >> >> mmc2_pins_default: mmc2_pins_default { >> pinctrl-single,pins = < >> 0x9c (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >> gpmc_a23.mmc2_clk */ >> 0xb0 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >> gpmc_cs1.mmc2_cmd */ >> 0xa0 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >> gpmc_a24.mmc2_dat0 */ >> 0xa4 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >> gpmc_a25.mmc2_dat1 */ >> 0xa8 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >> gpmc_a26.mmc2_dat2 */ >> 0xac (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >> gpmc_a27.mmc2_dat3 */ >> 0x8c (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >> gpmc_a19.mmc2_dat4 */ >> 0x90 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >> gpmc_a20.mmc2_dat5 */ >> 0x94 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >> gpmc_a21.mmc2_dat6 */ >> 0x98 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >> gpmc_a22.mmc2_dat7 */ >> >; >> }; > > Yeah so existing pinctrl-single binding above, with additional iodelay > binding below.. > >> &mmc2 { >> ... >> pinctrl-1 = >> &mmc2_pins_default, /* points to mmc control core pins */ >> <&iodelay 0x18c A_DELAY(0) | G_DELAY(120)>, /* CFG_GPMC_A19_IN */ >> <&iodelay 0x1a4 A_DELAY(265) | G_DELAY(360)>, /* CFG_GPMC_A20_IN */ >> <&iodelay 0x1b0 A_DELAY(0) | G_DELAY(120)>, /* CFG_GPMC_A21_IN */ >> <&iodelay 0x1bc A_DELAY(0) | G_DELAY(120)>, /* CFG_GPMC_A22_IN */ >> <&iodelay 0x1c8 A_DELAY(287) | G_DELAY(420)>, /* CFG_GPMC_A23_IN */ >> <&iodelay 0x1d4 A_DELAY(144) | G_DELAY(240)>, /* CFG_GPMC_A24_IN */ >> <&iodelay 0x1e0 A_DELAY(0) | G_DELAY(0)>, /* CFG_GPMC_A25_IN */ >> <&iodelay 0x1ec A_DELAY(120) | G_DELAY(0)>, /* CFG_GPMC_A26_IN */ >> <&iodelay 0x1f8 A_DELAY(120) | G_DELAY(180)>, /* CFG_GPMC_A27_IN */ >> <&iodelay 0x360 A_DELAY(0) | G_DELAY(0)>; /* CFG_GPMC_CS1_IN */ >> >> I have to check if we are capable of parsing that. but if that is the >> approach chosen, I suppose we might be able to figure something, I >> suppose.. > > Yes except I'd make use of some kind of #pinctrl-cells here just like > interrupt controller has #interrupt-cells. Then you can have the values > seprate and the controller knows what to do with them based on the > compatible flag and #pinctrl-cells. Something like the following I suppose, where pinctrl-cells is optional? dra7_pmx_core: pinmux@4a003400 { compatible = "ti,dra7-padconf", "pinctrl-single"; reg = <0x4a003400 0x0464>; #address-cells = <1>; #size-cells = <0>; #interrupt-cells = <1>; interrupt-controller; pinctrl-single,register-width = <32>; pinctrl-single,function-mask = <0x3fffffff>; }; dra7_iodelay_core: padconf@4844a000 { compatible = "ti,dra7-iodelay"; reg = <0x4844a000 0x0d1c>; #address-cells = <1>; #size-cells = <0>; #pinctrl-cells = <2>; }; Linus, I hope you are ok with the above?
On 03/10/2015 01:33 PM, Nishanth Menon wrote: > On 03/10/2015 12:31 PM, Tony Lindgren wrote: >> * Nishanth Menon <nm@ti.com> [150310 10:25]: >>> On 03/10/2015 10:33 AM, Tony Lindgren wrote: >>>> * Linus Walleij <linus.walleij@linaro.org> [150310 03:39]: >>>>> On Wed, Mar 4, 2015 at 1:00 AM, Nishanth Menon <nm@ti.com> wrote: >>>>> >>>>>> +Configuration definition follows similar model as the pinctrl-single: >>>>>> +The groups of pin configuration are defined under "pinctrl-single,pins" >>>>>> + >>>>>> +&dra7_iodelay_core { >>>>>> + mmc2_iodelay_3v3_conf: mmc2_iodelay_3v3_conf { >>>>>> + pinctrl-single,pins = < >>>>>> + 0x18c (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A19_IN */ >>>>>> + 0x1a4 (A_DELAY(265) | G_DELAY(360)) /* CFG_GPMC_A20_IN */ >>>>>> + 0x1b0 (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A21_IN */ >>>>>> + 0x1bc (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A22_IN */ >>>>>> + 0x1c8 (A_DELAY(287) | G_DELAY(420)) /* CFG_GPMC_A23_IN */ >>>>>> + 0x1d4 (A_DELAY(144) | G_DELAY(240)) /* CFG_GPMC_A24_IN */ >>>>>> + 0x1e0 (A_DELAY(0) | G_DELAY(0)) /* CFG_GPMC_A25_IN */ >>>>>> + 0x1ec (A_DELAY(120) | G_DELAY(0)) /* CFG_GPMC_A26_IN */ >>>>>> + 0x1f8 (A_DELAY(120) | G_DELAY(180)) /* CFG_GPMC_A27_IN */ >>>>>> + 0x360 (A_DELAY(0) | G_DELAY(0)) /* CFG_GPMC_CS1_IN */ >>>>>> + >; >>>>>> + }; >>>>>> +}; >>>>> >>>>> But wait. >>>>> >>>>> The promise when we merged pinctrl-single was that this driver was to be used >>>>> when the system had a one-register-per-pin layout and it was easy to do device >>>>> trees based on that. >>>>> >>>>> We were very reluctant to accept that even though we didn't even have the >>>>> generic pin control bindings in place, the argument being that the driver >>>>> should know the detailed register layout, it should not be described in the >>>>> device tree. We eventually caved in and accepted it as an exception. >>>> >>>> Hey let's get few things straight here. There's nothing stopping the >>>> driver from knowing a detailed register layout with the pinctrl-single >>>> binding. This can be very easily done based on the compatible flag and >>>> match data as needed and check the values provided. >>>> >>>> And let's also recap the reasons for the pinctrl-single binding. The >>>> the main reason for the pinctrl-single binding is to avoid mapping >>>> individual register bits to device tree compatible string properties. >>>> >>>> Imagine doing that for hundreds of similar registers. Believe me, I tried >>>> using device tree properties initially and it just sucked big time. For >>>> larger amounts of dts data, it's best to stick to numeric values and >>>> phandles in the device tree data and rely on the preprocessor for >>>> getting the values right. >>>> >>>> Finally, we don't want to build databases into the kernel drivers for >>>> every SoC. We certainly have plenty fo those already. >>>> >>>>> Since this pin controller is not using pinctrl-single it does not enjoy that >>>>> privilege and you need to explain why this pin controller cannot use the >>>>> generic bindings like so many other pin controllers have since started >>>>> to do. ("It is in the same SoC" is not an acceptable argument.) >>>>> >>>>> What is wrong with this: >>>>> Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt >>>> >>>> Nishanth, care to explain your reasons for using pinctrl-single binding >>>> here vs the generic binding? Is the amount of string parsing with the >>>> data an issue here? >>> >>> Wrong option chosen, I suppose :( - alright, lets discuss the alternative. >> >> Heh well now we know :) >> >>>>> We can add generic delay bindings to the list, it even seems like >>>>> a good idea to do so, as it is likely something that will come up in >>>>> other hardware and will be needed for ACPI etc going forward. >>>> >>>> We certainly need to make setting delays (with values) generic, no >>>> doubt about that. >>>> >>>> If the large amount of data is not an issue here, we could maybe set >>>> each iodelay register as a separate device? Then the binding could >>>> be just along the interrupts-extended type binding: >>>> >>>> foo = <&bar 0x18c A_DELAY(0) G_DELAY(120)>; >>>> >>>> Where the 0x18c is the instance offset of the register within the >>>> ti,dra7-iodelay compatible controller. >>> >>> if mmc2_pins_default point at pins for mmc pin configuration for >>> control_core (pinctrl-single), are you proposing the following? >>> >>> mmc2_pins_default: mmc2_pins_default { >>> pinctrl-single,pins = < >>> 0x9c (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >>> gpmc_a23.mmc2_clk */ >>> 0xb0 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >>> gpmc_cs1.mmc2_cmd */ >>> 0xa0 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >>> gpmc_a24.mmc2_dat0 */ >>> 0xa4 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >>> gpmc_a25.mmc2_dat1 */ >>> 0xa8 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >>> gpmc_a26.mmc2_dat2 */ >>> 0xac (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >>> gpmc_a27.mmc2_dat3 */ >>> 0x8c (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >>> gpmc_a19.mmc2_dat4 */ >>> 0x90 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >>> gpmc_a20.mmc2_dat5 */ >>> 0x94 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >>> gpmc_a21.mmc2_dat6 */ >>> 0x98 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* >>> gpmc_a22.mmc2_dat7 */ >>> >; >>> }; >> >> Yeah so existing pinctrl-single binding above, with additional iodelay >> binding below.. >> >>> &mmc2 { >>> ... >>> pinctrl-1 = >>> &mmc2_pins_default, /* points to mmc control core pins */ >>> <&iodelay 0x18c A_DELAY(0) | G_DELAY(120)>, /* CFG_GPMC_A19_IN */ >>> <&iodelay 0x1a4 A_DELAY(265) | G_DELAY(360)>, /* CFG_GPMC_A20_IN */ >>> <&iodelay 0x1b0 A_DELAY(0) | G_DELAY(120)>, /* CFG_GPMC_A21_IN */ >>> <&iodelay 0x1bc A_DELAY(0) | G_DELAY(120)>, /* CFG_GPMC_A22_IN */ >>> <&iodelay 0x1c8 A_DELAY(287) | G_DELAY(420)>, /* CFG_GPMC_A23_IN */ >>> <&iodelay 0x1d4 A_DELAY(144) | G_DELAY(240)>, /* CFG_GPMC_A24_IN */ >>> <&iodelay 0x1e0 A_DELAY(0) | G_DELAY(0)>, /* CFG_GPMC_A25_IN */ >>> <&iodelay 0x1ec A_DELAY(120) | G_DELAY(0)>, /* CFG_GPMC_A26_IN */ >>> <&iodelay 0x1f8 A_DELAY(120) | G_DELAY(180)>, /* CFG_GPMC_A27_IN */ >>> <&iodelay 0x360 A_DELAY(0) | G_DELAY(0)>; /* CFG_GPMC_CS1_IN */ >>> >>> I have to check if we are capable of parsing that. but if that is the >>> approach chosen, I suppose we might be able to figure something, I >>> suppose.. >> >> Yes except I'd make use of some kind of #pinctrl-cells here just like >> interrupt controller has #interrupt-cells. Then you can have the values >> seprate and the controller knows what to do with them based on the >> compatible flag and #pinctrl-cells. > > Something like the following I suppose, where pinctrl-cells is optional? > > dra7_pmx_core: pinmux@4a003400 { > compatible = "ti,dra7-padconf", "pinctrl-single"; > reg = <0x4a003400 0x0464>; > #address-cells = <1>; > #size-cells = <0>; > #interrupt-cells = <1>; > interrupt-controller; > pinctrl-single,register-width = <32>; > pinctrl-single,function-mask = <0x3fffffff>; > }; > > dra7_iodelay_core: padconf@4844a000 { > compatible = "ti,dra7-iodelay"; > reg = <0x4844a000 0x0d1c>; > #address-cells = <1>; > #size-cells = <0>; > #pinctrl-cells = <2>; > }; > > Linus, > > I hope you are ok with the above? > a and g delays are in pico seconds parsed by iodelay driver, so, revising proposal (and dropping the macros) to something as follows: pinctrl-1 = &mmc2_pins_default, /* points to mmc control core pins */ <&iodelay IODELAY_OFFSET(0x18c) 0 120>, /* CFG_GPMC_A19_IN */ <&iodelay IODELAY_OFFSET(0x1a4) 265 360>, /* CFG_GPMC_A20_IN */ <&iodelay IODELAY_OFFSET(0x1b0) 0 120>, /* CFG_GPMC_A21_IN */ <&iodelay IODELAY_OFFSET(0x1bc) 0 120>, /* CFG_GPMC_A22_IN */ <&iodelay IODELAY_OFFSET(0x1c8) 287 420>, /* CFG_GPMC_A23_IN */ <&iodelay IODELAY_OFFSET(0x1d4) 144 240>, /* CFG_GPMC_A24_IN */ <&iodelay IODELAY_OFFSET(0x1e0) 0 0>, /* CFG_GPMC_A25_IN */ <&iodelay IODELAY_OFFSET(0x1ec) 120 0>, /* CFG_GPMC_A26_IN */ <&iodelay IODELAY_OFFSET(0x1f8) 120 180>, /* CFG_GPMC_A27_IN */ <&iodelay IODELAY_OFFSET(0x360) 0 0>; /* CFG_GPMC_CS1_IN */ it might just need us to define the right parse path etc.. but should let us scale with other phandle implementations as well. offsets are computed by a macro IODELAY_OFFSET() -> just throwing in an example set of values for illustration here.. but just to get the idea. Will be nice to have some comments before I go down this path. Thanks for helping review this to a reasonable direction.
On Tue, Mar 10, 2015 at 7:33 PM, Nishanth Menon <nm@ti.com> wrote: > On 03/10/2015 12:31 PM, Tony Lindgren wrote: >> Yes except I'd make use of some kind of #pinctrl-cells here just like >> interrupt controller has #interrupt-cells. Then you can have the values >> seprate and the controller knows what to do with them based on the >> compatible flag and #pinctrl-cells. > > Something like the following I suppose, where pinctrl-cells is optional? > > dra7_pmx_core: pinmux@4a003400 { > compatible = "ti,dra7-padconf", "pinctrl-single"; > reg = <0x4a003400 0x0464>; > #address-cells = <1>; > #size-cells = <0>; > #interrupt-cells = <1>; > interrupt-controller; > pinctrl-single,register-width = <32>; > pinctrl-single,function-mask = <0x3fffffff>; > }; > > dra7_iodelay_core: padconf@4844a000 { > compatible = "ti,dra7-iodelay"; > reg = <0x4844a000 0x0d1c>; > #address-cells = <1>; > #size-cells = <0>; > #pinctrl-cells = <2>; > }; > > Linus, > > I hope you are ok with the above? Hm depends on where the documentation hits I guess? Such a generic cell count property has to be to the generic pinctrl-bindings.txt document if I read it right. Overall I guess this will be acceptable but you really need to reuse some more code between this driver and pinctrl-single.c if I read it right. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Linus Walleij <linus.walleij@linaro.org> [150317 18:31]: > On Tue, Mar 10, 2015 at 7:33 PM, Nishanth Menon <nm@ti.com> wrote: > > On 03/10/2015 12:31 PM, Tony Lindgren wrote: > > >> Yes except I'd make use of some kind of #pinctrl-cells here just like > >> interrupt controller has #interrupt-cells. Then you can have the values > >> seprate and the controller knows what to do with them based on the > >> compatible flag and #pinctrl-cells. > > > > Something like the following I suppose, where pinctrl-cells is optional? > > > > dra7_pmx_core: pinmux@4a003400 { > > compatible = "ti,dra7-padconf", "pinctrl-single"; > > reg = <0x4a003400 0x0464>; > > #address-cells = <1>; > > #size-cells = <0>; > > #interrupt-cells = <1>; > > interrupt-controller; > > pinctrl-single,register-width = <32>; > > pinctrl-single,function-mask = <0x3fffffff>; > > }; > > > > dra7_iodelay_core: padconf@4844a000 { > > compatible = "ti,dra7-iodelay"; > > reg = <0x4844a000 0x0d1c>; > > #address-cells = <1>; > > #size-cells = <0>; > > #pinctrl-cells = <2>; > > }; > > > > Linus, > > > > I hope you are ok with the above? > > Hm depends on where the documentation hits I guess? > > Such a generic cell count property has to be to the generic > pinctrl-bindings.txt document if I read it right. Yeah agreed. I suggest discussing the binding and the generic parsing code for it first :) > Overall I guess this will be acceptable but you really need to > reuse some more code between this driver and pinctrl-single.c > if I read it right. It seems with the generic binding the actual driver should be just the hardware specific code hopefully. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 17, 2015 at 06:41:51PM -0700, Tony Lindgren wrote: > Yeah agreed. I suggest discussing the binding and the generic > parsing code for it first :) > > It seems with the generic binding the actual driver should be > just the hardware specific code hopefully. Did this thread go anywhere in the last month? I am certainly looking forward to seeing what the resolution is to this, given for our use the boot loader setup is not appealing at all.
On 04/14/2015 08:29 PM, Lennart Sorensen wrote: > On Tue, Mar 17, 2015 at 06:41:51PM -0700, Tony Lindgren wrote: >> Yeah agreed. I suggest discussing the binding and the generic >> parsing code for it first :) >> >> It seems with the generic binding the actual driver should be >> just the hardware specific code hopefully. > > Did this thread go anywhere in the last month? I am certainly looking > forward to seeing what the resolution is to this, given for our use the > boot loader setup is not appealing at all. > I am yet to post a new revision to this series - few other stuff got in the way. IODelay driver in no way removes the constraint that the SoC architecture has - most of the pins still need to be muxed in bootloader - we cannot escape that. The reasoning for doing the mux in bootloader is independent of the need for iodelay. Reasoning for mux in bootloader is because the mux and pull fields are glitchy - much more than previous generations of TI SoCs and significantly long enough to cause issues depending on the pins being muxed. Reasoning for iodelay is different - it is a hardware block meant to control the timing of signals in a particular signal path to ensure that specification compliance is met. Lets try not to mix the two.
On Wed, Apr 15, 2015 at 11:51:32AM -0500, Nishanth Menon wrote: > I am yet to post a new revision to this series - few other stuff got > in the way. IODelay driver in no way removes the constraint that the > SoC architecture has - most of the pins still need to be muxed in > bootloader - we cannot escape that. The reasoning for doing the mux in > bootloader is independent of the need for iodelay. > > Reasoning for mux in bootloader is because the mux and pull fields are > glitchy - much more than previous generations of TI SoCs and > significantly long enough to cause issues depending on the pins being > muxed. Well if we know glitching is NOT an issue on our boards, then we don't have to do anything in the boot loader other than the basic setup for the serial console and emmc and SD, which has always been necesary. I consider moving the mux setup to the bootloader a terrible design and won't go along with it. We make sure all external devices have reset lines being held while the pinmux is being setup, so glitching is a non issue. > Reasoning for iodelay is different - it is a hardware block meant to > control the timing of signals in a particular signal path to ensure > that specification compliance is met. > > Lets try not to mix the two. Well I was told by multiple people from TI that the reason for moving the pinmux setup to the bootloader was because of the iodelay issue, so you will have to get the message made clear within TI then.
On 04/15/2015 01:44 PM, Lennart Sorensen wrote: > On Wed, Apr 15, 2015 at 11:51:32AM -0500, Nishanth Menon wrote: >> I am yet to post a new revision to this series - few other stuff got >> in the way. IODelay driver in no way removes the constraint that the >> SoC architecture has - most of the pins still need to be muxed in >> bootloader - we cannot escape that. The reasoning for doing the mux in >> bootloader is independent of the need for iodelay. >> >> Reasoning for mux in bootloader is because the mux and pull fields are >> glitchy - much more than previous generations of TI SoCs and >> significantly long enough to cause issues depending on the pins being >> muxed. > > Well if we know glitching is NOT an issue on our boards, then we don't > have to do anything in the boot loader other than the basic setup for > the serial console and emmc and SD, which has always been necesary. > > I consider moving the mux setup to the bootloader a terrible design and > won't go along with it. We make sure all external devices have reset > lines being held while the pinmux is being setup, so glitching is a > non issue. I cannot discuss customer boards on this list - the right forum for TI support is e2e.ti.com or in cases where FAE (Field Applications Engineer) is involved, via appropriate support path. Now, that said, even with personal opinions in place, I have to stick with what the SoC constraints on hand and suggested architecture we have discussed to ensure safe platform operation at least for the platforms we are contributing to. again... muxing in the bootloader IS NOT what this patch is about. If we can stick to the topic in discussion, it is probably more effective. Any improvement suggestions to the code is more than appreciated. >> Reasoning for iodelay is different - it is a hardware block meant to >> control the timing of signals in a particular signal path to ensure >> that specification compliance is met. >> >> Lets try not to mix the two. > > Well I was told by multiple people from TI that the reason for moving > the pinmux setup to the bootloader was because of the iodelay issue, > so you will have to get the message made clear within TI then. > I have passed on this message.
diff --git a/Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt new file mode 100644 index 000000000000..e12f4e5a3f25 --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt @@ -0,0 +1,86 @@ +Texas Instruments I/O Delay module configuration pinctrl definition + +Used in conjunction with Documentation/devicetree/bindings/pinctrl/ti,omap-pinctrl.txt + +Required Properties: +- compatible: Should be: + "ti,dra7-iodelay" - I/O delay configuration for DRA7 +- reg - must be the register address range of IODelay module +- #address-cells = <1>; +- #size-cells = <0>; + +Important note: Use of "ti,dra7-iodelay" compatible definition need to be +carefully evaluated due to the expectation of glitch during configuration. + +Example: + +dra7_iodelay_core: padconf@4844a000 { + compatible = "ti,dra7-iodelay"; + reg = <0x4844a000 0x0d1c>; + #address-cells = <1>; + #size-cells = <0>; +}; + +Configuration definition follows similar model as the pinctrl-single: +The groups of pin configuration are defined under "pinctrl-single,pins" + +&dra7_iodelay_core { + mmc2_iodelay_3v3_conf: mmc2_iodelay_3v3_conf { + pinctrl-single,pins = < + 0x18c (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A19_IN */ + 0x1a4 (A_DELAY(265) | G_DELAY(360)) /* CFG_GPMC_A20_IN */ + 0x1b0 (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A21_IN */ + 0x1bc (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A22_IN */ + 0x1c8 (A_DELAY(287) | G_DELAY(420)) /* CFG_GPMC_A23_IN */ + 0x1d4 (A_DELAY(144) | G_DELAY(240)) /* CFG_GPMC_A24_IN */ + 0x1e0 (A_DELAY(0) | G_DELAY(0)) /* CFG_GPMC_A25_IN */ + 0x1ec (A_DELAY(120) | G_DELAY(0)) /* CFG_GPMC_A26_IN */ + 0x1f8 (A_DELAY(120) | G_DELAY(180)) /* CFG_GPMC_A27_IN */ + 0x360 (A_DELAY(0) | G_DELAY(0)) /* CFG_GPMC_CS1_IN */ + >; + }; +}; + +Usage in conjunction with pinctrl single: + +For a complete description of the pins both the regular muxing as well as the +iodelay configuration is necessary. For example: + +&dra7_pmx_core { + mmc2_pins_default: mmc2_pins_default { + pinctrl-single,pins = < + 0x9c (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_a23.mmc2_clk */ + 0xb0 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_cs1.mmc2_cmd */ + 0xa0 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_a24.mmc2_dat0 */ + 0xa4 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_a25.mmc2_dat1 */ + 0xa8 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_a26.mmc2_dat2 */ + 0xac (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_a27.mmc2_dat3 */ + 0x8c (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_a19.mmc2_dat4 */ + 0x90 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_a20.mmc2_dat5 */ + 0x94 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_a21.mmc2_dat6 */ + 0x98 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /* gpmc_a22.mmc2_dat7 */ + >; + }; +}; + +&dra7_iodelay_core { + mmc2_iodelay_3v3_conf: mmc2_iodelay_3v3_conf { + pinctrl-single,pins = < + 0x18c (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A19_IN */ + 0x1a4 (A_DELAY(265) | G_DELAY(360)) /* CFG_GPMC_A20_IN */ + 0x1b0 (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A21_IN */ + 0x1bc (A_DELAY(0) | G_DELAY(120)) /* CFG_GPMC_A22_IN */ + 0x1c8 (A_DELAY(287) | G_DELAY(420)) /* CFG_GPMC_A23_IN */ + 0x1d4 (A_DELAY(144) | G_DELAY(240)) /* CFG_GPMC_A24_IN */ + 0x1e0 (A_DELAY(0) | G_DELAY(0)) /* CFG_GPMC_A25_IN */ + 0x1ec (A_DELAY(120) | G_DELAY(0)) /* CFG_GPMC_A26_IN */ + 0x1f8 (A_DELAY(120) | G_DELAY(180)) /* CFG_GPMC_A27_IN */ + 0x360 (A_DELAY(0) | G_DELAY(0)) /* CFG_GPMC_CS1_IN */ + >; + }; +}; + +&mmc2 { + pinctrl-names = "default"; + pinctrl-0 = <&mmc2_pins_default &mmc2_iodelay_3v3_conf>; +};
SoCs such as DRA7 family from Texas Instruments also include a highly configurable hardware block called the IOdelay block. This block allows very specific custom fine tuning for electrical characteristics of IO pins. In addition to the regular pin muxing modes supported by the pinctrl-single, additional configuration for this block for specific pins may also be mandatory in certain cases. It is advocated strongly in TI's official documentation considering the existing design of the DRA7 family of processors, which during mux or IODelay reconfiguration, has a potential for a significant glitch which may cause functional impairment to certain hardware. It is hence recommended to do as little of muxing as absolutely necessary without IO isolation (which can only be done in initial stages of bootloader). Even with the above limitation, certain functionality such as MMC may mandate the need of IODelay reconfiguration depending on speed of transfer. Hence, introduce a new binding to facilitate programming the same. Signed-off-by: Nishanth Menon <nm@ti.com> --- .../bindings/pinctrl/ti,iodelay-pinctrl.txt | 86 ++++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt