| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150310173115.GS5264@atomide.com>
Date: Tue, 10 Mar 2015 10:31:15 -0700
From: Tony Lindgren <tony@...mide.com>
To: Nishanth Menon <nm@...com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
Linux-OMAP <linux-omap@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
Lokesh Vutla <lokeshvutla@...com>
Subject: Re: [PATCH 1/2] pinctrl: bindings: pinctrl: Add support for TI's
IODelay configuration
* Nishanth Menon <nm@...com> [150310 10:25]:
> On 03/10/2015 10:33 AM, Tony Lindgren wrote:
> > * Linus Walleij <linus.walleij@...aro.org> [150310 03:39]:
> >> On Wed, Mar 4, 2015 at 1:00 AM, Nishanth Menon <nm@...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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists