| Message ID | 1490368934-12494-4-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Horman |
| Headers | show |
On Fri, Mar 24, 2017 at 4:22 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: > Add dt-bindings for Renesas r7s72100 pin controller header file. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > +/* > + * Pin is bi-directional. > + * An alternate function that needs both input/output functionalities shall > + * be configured as bidirectional. > + * Eg. SDA/SCL pins of an I2c interface. > + */ > +#define BI_DIR (1 << 3) Any specific reason why this should not simply be added to include/linux/pinctrl/pinconf-generic.h as PIN_CONFIG_BIDIRECTIONAL and parsed in drivers/pinctrl/pinconf-generic.c from the (new) DT property "bidirectional" simply? > +/* > + * Flags used to ask software to drive the pin I/O direction overriding the > + * alternate function configuration. > + * Some alternate functions require software to force I/O direction of a pin, > + * overriding the designated one. > + * Refer to the HW manual to know when this flag shall be used. > + */ > +#define SWIO_IN (1 << 4) > +#define SWIO_OUT (1 << 5) What is wrong in doing this with generic pin config using PIN_CONFIG_INPUT_ENABLE and PIN_CONFIG_OUTPUT (ignoring the argument)? In the device tree use input-enable and add a new output-enable (with unspecified value) with proper description and DT bindings? And if you think these have no general applicability, by the end of the day they are *still* pin config, not magic flags we can choose to toss in with the muxing, so you can do what the Qualcomm driver does and add custom pin configurations extending the generic pin config, see drivers/pinctrl/qcom/pinctrl-spmi-gpio.c qcom,pull-up-strength etc. Yours, Linus Walleij
On Wednesday, March 29, 2017, Linus Walleij wrote: > On Fri, Mar 24, 2017 at 4:22 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> > wrote: > > > Add dt-bindings for Renesas r7s72100 pin controller header file. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > +/* > > + * Pin is bi-directional. > > + * An alternate function that needs both input/output functionalities > > +shall > > + * be configured as bidirectional. > > + * Eg. SDA/SCL pins of an I2c interface. > > + */ > > +#define BI_DIR (1 << 3) > > Any specific reason why this should not simply be added to > include/linux/pinctrl/pinconf-generic.h > as PIN_CONFIG_BIDIRECTIONAL and parsed in drivers/pinctrl/pinconf- > generic.c from the (new) DT property "bidirectional" simply? I see your point. It would cut down from every driver out there inventing some new property or config instead of everyone just sharing a fixed set. Maybe someone else out there will end up having a need for a "bidirectional" option. > > +/* > > + * Flags used to ask software to drive the pin I/O direction > > +overriding the > > + * alternate function configuration. > > + * Some alternate functions require software to force I/O direction > > +of a pin, > > + * overriding the designated one. > > + * Refer to the HW manual to know when this flag shall be used. > > + */ > > +#define SWIO_IN (1 << 4) > > +#define SWIO_OUT (1 << 5) > > What is wrong in doing this with generic pin config using > PIN_CONFIG_INPUT_ENABLE and PIN_CONFIG_OUTPUT (ignoring the argument)? > > In the device tree use input-enable and add a new output-enable (with > unspecified value) with proper description and DT bindings? Again, that's probably fine. It seems we are still doing the same thing which is using the DT to pass extra config information to the driver. And, we can do whatever we want with that info. > And if you think these have no general applicability, by the end of the > day they are *still* pin config, not magic flags we can choose to toss in > with the muxing, so you can do what the Qualcomm driver does and add > custom pin configurations extending the generic pin config, see > drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > qcom,pull-up-strength etc. But, it seems that when you set a config option, it applies to everything in "pins"? I2C Example: (seem OK) /* P1_6 = RIIC3SCL (bi dir) */ /* P1_7 = RIIC3SDA (bi dir) */ i2c3_pins: i2c3 { pins = <PIN(1, 6) | FUNC_1>, <PIN(1, 7) | FUNC_1>; bidirectional; }; But, what do we do for Ethernet? All the pins are "normal" except just the MDIO pin needs to be bidirectional. That's the part I'm confused by. How do we flag that just the ET_MDIO needs "bidirectional"? /* Ethernet */ ether_pins: ether { /* Ethernet on Ports 1,2,3,5 */ pins = <PIN(1, 14) | FUNC_4)>, /* P1_14 = ET_COL */ <PIN(5, 9) | FUNC_2)>, /* P5_9 = ET_MDC */ <PIN(3, 3) | FUNC_2)>, /* P3_3 = ET_MDIO !!!! (bi dir) !!!!!! */ <PIN(3, 4) | FUNC_2)>, /* P3_4 = ET_RXCLK */ <PIN(3, 5) | FUNC_2)>, /* P3_5 = ET_RXER */ <PIN(3, 6) | FUNC_2)>, /* P3_6 = ET_RXDV */ <PIN(2, 0) | FUNC_2)>, /* P2_0 = ET_TXCLK */ <PIN(2, 1) | FUNC_2)>, /* P2_1 = ET_TXER */ <PIN(2, 2) | FUNC_2)>, /* P2_2 = ET_TXEN */ <PIN(2, 3) | FUNC_2)>, /* P2_3 = ET_CRS */ <PIN(2, 4) | FUNC_2)>, /* P2_4 = ET_TXD0 */ <PIN(2, 5) | FUNC_2)>, /* P2_5 = ET_TXD1 */ <PIN(2, 6) | FUNC_2)>, /* P2_6 = ET_TXD2 */ <PIN(2, 7) | FUNC_2)>, /* P2_7 = ET_TXD3 */ <PIN(2, 8) | FUNC_2)>, /* P2_8 = ET_RXD0 */ <PIN(2, 9) | FUNC_2)>, /* P2_9 = ET_RXD1 */ <PIN(2, 10) | FUNC_2)>, /* P2_10 = ET_RXD2 */ <PIN(2, 11) | FUNC_2)>; /* P2_11 = ET_RXD3 */ }; Chris
Hi Chris, On Wed, Mar 29, 2017 at 4:55 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Wednesday, March 29, 2017, Linus Walleij wrote: >> On Fri, Mar 24, 2017 at 4:22 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: >> > +/* >> > + * Flags used to ask software to drive the pin I/O direction >> > +overriding the >> > + * alternate function configuration. >> > + * Some alternate functions require software to force I/O direction >> > +of a pin, >> > + * overriding the designated one. >> > + * Refer to the HW manual to know when this flag shall be used. >> > + */ >> > +#define SWIO_IN (1 << 4) >> > +#define SWIO_OUT (1 << 5) >> >> What is wrong in doing this with generic pin config using >> PIN_CONFIG_INPUT_ENABLE and PIN_CONFIG_OUTPUT (ignoring the argument)? >> >> In the device tree use input-enable and add a new output-enable (with >> unspecified value) with proper description and DT bindings? > > Again, that's probably fine. It seems we are still doing the same thing > which is using the DT to pass extra config information to the driver. > And, we can do whatever we want with that info. > > >> And if you think these have no general applicability, by the end of the >> day they are *still* pin config, not magic flags we can choose to toss in >> with the muxing, so you can do what the Qualcomm driver does and add >> custom pin configurations extending the generic pin config, see >> drivers/pinctrl/qcom/pinctrl-spmi-gpio.c >> qcom,pull-up-strength etc. > > But, it seems that when you set a config option, it applies to everything > in "pins"? > > I2C Example: (seem OK) > /* P1_6 = RIIC3SCL (bi dir) */ > /* P1_7 = RIIC3SDA (bi dir) */ > i2c3_pins: i2c3 { > pins = <PIN(1, 6) | FUNC_1>, > <PIN(1, 7) | FUNC_1>; > bidirectional; > }; Correct. > But, what do we do for Ethernet? All the pins are "normal" except just > the MDIO pin needs to be bidirectional. > That's the part I'm confused by. > How do we flag that just the ET_MDIO needs "bidirectional"? You add subnodes, cfr. arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts: avb_pins: avb { mux { groups = "avb_link", "avb_phy_int", "avb_mdc", "avb_mii"; function = "avb"; }; pins_mdc { groups = "avb_mdc"; drive-strength = <24>; }; pins_mii_tx { pins = "PIN_AVB_TX_CTL", "PIN_AVB_TXC", "PIN_AVB_TD0", "PIN_AVB_TD1", "PIN_AVB_TD2", "PIN_AVB_TD3"; drive-strength = <12>; }; }; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, On Wednesday, March 29, 2017, Geert Uytterhoeven wrote: > > But, what do we do for Ethernet? All the pins are "normal" except just > > the MDIO pin needs to be bidirectional. > > That's the part I'm confused by. > > How do we flag that just the ET_MDIO needs "bidirectional"? > > You add subnodes, cfr. arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts: > > avb_pins: avb { > mux { > groups = "avb_link", "avb_phy_int", "avb_mdc", > "avb_mii"; > function = "avb"; > }; > > pins_mdc { > groups = "avb_mdc"; > drive-strength = <24>; > }; > > pins_mii_tx { > pins = "PIN_AVB_TX_CTL", "PIN_AVB_TXC", > "PIN_AVB_TD0", > "PIN_AVB_TD1", "PIN_AVB_TD2", > "PIN_AVB_TD3"; > drive-strength = <12>; > }; > }; Oh, so there is a way! Thank you. So, for clarity: /* Ethernet */ ether_pins: ether { /* Ethernet on Ports 1,2,3,5 */ mux { pins = <PIN(1, 14) | FUNC_4)>, /* P1_14 = ET_COL */ <PIN(5, 9) | FUNC_2)>, /* P5_9 = ET_MDC */ <PIN(3, 4) | FUNC_2)>, /* P3_4 = ET_RXCLK */ <PIN(3, 5) | FUNC_2)>, /* P3_5 = ET_RXER */ <PIN(3, 6) | FUNC_2)>, /* P3_6 = ET_RXDV */ <PIN(2, 0) | FUNC_2)>, /* P2_0 = ET_TXCLK */ <PIN(2, 1) | FUNC_2)>, /* P2_1 = ET_TXER */ <PIN(2, 2) | FUNC_2)>, /* P2_2 = ET_TXEN */ <PIN(2, 3) | FUNC_2)>, /* P2_3 = ET_CRS */ <PIN(2, 4) | FUNC_2)>, /* P2_4 = ET_TXD0 */ <PIN(2, 5) | FUNC_2)>, /* P2_5 = ET_TXD1 */ <PIN(2, 6) | FUNC_2)>, /* P2_6 = ET_TXD2 */ <PIN(2, 7) | FUNC_2)>, /* P2_7 = ET_TXD3 */ <PIN(2, 8) | FUNC_2)>, /* P2_8 = ET_RXD0 */ <PIN(2, 9) | FUNC_2)>, /* P2_9 = ET_RXD1 */ <PIN(2, 10) | FUNC_2)>, /* P2_10 = ET_RXD2 */ <PIN(2, 11) | FUNC_2)>; /* P2_11 = ET_RXD3 */ }; pins_bidir { pins = <PIN(3, 3) | FUNC_2)>, /* P3_3 = ET_MDIO */ bidirectional; }; }; Chris
On Wednesday, March 29, 2017, Chris Brandt wrote: > On Wednesday, March 29, 2017, Geert Uytterhoeven wrote: > > > But, what do we do for Ethernet? All the pins are "normal" except > > > just the MDIO pin needs to be bidirectional. > > > That's the part I'm confused by. > > > How do we flag that just the ET_MDIO needs "bidirectional"? > > > > You add subnodes, cfr. arch/arm64/boot/dts/renesas/r8a7795-salvator- > x.dts: > > > > avb_pins: avb { > > mux { > > groups = "avb_link", "avb_phy_int", "avb_mdc", > > "avb_mii"; > > function = "avb"; > > }; > > > > pins_mdc { > > groups = "avb_mdc"; > > drive-strength = <24>; > > }; > > > > pins_mii_tx { > > pins = "PIN_AVB_TX_CTL", "PIN_AVB_TXC", > > "PIN_AVB_TD0", > > "PIN_AVB_TD1", "PIN_AVB_TD2", > > "PIN_AVB_TD3"; > > drive-strength = <12>; > > }; > > }; > > Oh, so there is a way! > > Thank you. > > > So, for clarity: Actually, Linus's request was to use "pinmux", not "pins". So, it should be: /* P1_6 = RIIC3SCL (bi dir) */ /* P1_7 = RIIC3SDA (bi dir) */ i2c3_pins: i2c3 { pinmux = <PIN(1, 6) | FUNC_1>, <PIN(1, 7) | FUNC_1>; bidirectional; }; /* Ethernet */ ether_pins: ether { /* Ethernet on Ports 1,2,3,5 */ mux { pinmux = <PIN(1, 14) | FUNC_4)>, /* P1_14 = ET_COL */ <PIN(5, 9) | FUNC_2)>, /* P5_9 = ET_MDC */ <PIN(3, 4) | FUNC_2)>, /* P3_4 = ET_RXCLK */ <PIN(3, 5) | FUNC_2)>, /* P3_5 = ET_RXER */ <PIN(3, 6) | FUNC_2)>, /* P3_6 = ET_RXDV */ <PIN(2, 0) | FUNC_2)>, /* P2_0 = ET_TXCLK */ <PIN(2, 1) | FUNC_2)>, /* P2_1 = ET_TXER */ <PIN(2, 2) | FUNC_2)>, /* P2_2 = ET_TXEN */ <PIN(2, 3) | FUNC_2)>, /* P2_3 = ET_CRS */ <PIN(2, 4) | FUNC_2)>, /* P2_4 = ET_TXD0 */ <PIN(2, 5) | FUNC_2)>, /* P2_5 = ET_TXD1 */ <PIN(2, 6) | FUNC_2)>, /* P2_6 = ET_TXD2 */ <PIN(2, 7) | FUNC_2)>, /* P2_7 = ET_TXD3 */ <PIN(2, 8) | FUNC_2)>, /* P2_8 = ET_RXD0 */ <PIN(2, 9) | FUNC_2)>, /* P2_9 = ET_RXD1 */ <PIN(2, 10) | FUNC_2)>, /* P2_10 = ET_RXD2 */ <PIN(2, 11) | FUNC_2)>; /* P2_11 = ET_RXD3 */ }; pins_bidir { pinmux = <PIN(3, 3) | FUNC_2)>, /* P3_3 = ET_MDIO */ bidirectional; }; }; NOTE: "FUNC_2" can just be "2" as per Geert's request. Chris
Hi Linus,
another reply to your email, please don't feel assaulted :)
On Wed, Mar 29, 2017 at 03:22:23PM +0200, Linus Walleij wrote:
> On Fri, Mar 24, 2017 at 4:22 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
>
> > Add dt-bindings for Renesas r7s72100 pin controller header file.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> > +/*
> > + * Pin is bi-directional.
> > + * An alternate function that needs both input/output functionalities shall
> > + * be configured as bidirectional.
> > + * Eg. SDA/SCL pins of an I2c interface.
> > + */
> > +#define BI_DIR (1 << 3)
>
> Any specific reason why this should not simply be added to
> include/linux/pinctrl/pinconf-generic.h
> as PIN_CONFIG_BIDIRECTIONAL and parsed in
> drivers/pinctrl/pinconf-generic.c
> from the (new) DT property "bidirectional" simply?
>
I can try to give you a few reasons why I don't see those flags fit in
the pin configuration flags definition.
*) those flags are used during pin multiplexing procedure only and that
procedure has a specific order to be respected:
You can have a look here:
https://www.spinics.net/lists/linux-renesas-soc/msg12793.html
In "rza1_alternate_function_conf()" function, we need to set bidir
before setting every other register.
The same applies some lines below:, the PIPC, PMC and PM register set order
has to be respected, and depends on those BIDIR and SWIO_* parameters.
This implies those configuration cannot be applied after pin muxing,
certainly not in pin_config[_group]_set() whose invocation time
is independent from pin_mux_set()'s one.
One way forward would be, every time we mux a pin, look for a pinconf group
that includes the pin we're muxing. That would happen for each pin,
for no added benefits imo.
*) as Geert already pointed out, we may need dedicated subnodes to
specify those pin configuration flags, not only because of what Chris
said, but because pinconf_generic_dt_subnode_to_map() wants "pins" or
"groups" to be there in the subnode, and in our pin multiplexing
sub-nodes we only have "pinmux" property (say: we cannot specify
pin_conf flags in the same sub-node where we describe pin
multiplexing, we always need a dedicated sub-node).
Chris and Geert gave some examples in their replies on how that would
like, and how it makes the bindings a little more complex.
*) those flags, according to Chris, won't be used in RZ/A2, and
reasonably not in any other RZ device. Do we want to add them to the
generic bindings, being them so specific to this hardware platform?
One thing I suggest considering is to get rid of those flags, at
least in bindings, and introduce 3 variants for each pin multiplexing
function identifier.
Say:
include/dt-bindings/pinctrl/r7s72100-pinctrl.h:
#define MUX_1 (1 << 16)
#define MUX_1_BIDIR (1 << 16 | 1 << 24)
#define MUX_1_SWIO_IN (1 << 16 | 2 << 24)
#define MUX_1_SWIO_OUT (1 << 16 | 3 << 24)
...
#define MUX_8 (8 << 16)
#define MUX_8_BIDIR (8 << 16 | 1 << 24)
....
this way we get rid of those extra flag values and squeeze pin id
and mux function id in a single integer, part of the "pinmux"
arguments list.
i2c_pins {
pinmux = <(PIN(1, 6) | MUX_1_BIDIR)>,
<(PIN(1, 7) | MUX_2_BIDIR)>;
};
The driver will parse the bits from [31:24] to find out if it needs
to enable some special multiplexing property, and performs
multiplexing as it is doing right now.
> > +/*
> > + * Flags used to ask software to drive the pin I/O direction overriding the
> > + * alternate function configuration.
> > + * Some alternate functions require software to force I/O direction of a pin,
> > + * overriding the designated one.
> > + * Refer to the HW manual to know when this flag shall be used.
> > + */
> > +#define SWIO_IN (1 << 4)
> > +#define SWIO_OUT (1 << 5)
>
> What is wrong in doing this with generic pin config using
> PIN_CONFIG_INPUT_ENABLE and PIN_CONFIG_OUTPUT
> (ignoring the argument)?
>
> In the device tree use input-enable and add a new output-enable
> (with unspecified value) with proper description and DT bindings?
>
> And if you think these have no general applicability, by the end
> of the day they are *still* pin config, not magic flags we can choose to
> toss in with the muxing, so you can do what the Qualcomm driver
> does and add custom pin configurations extending the generic
> pin config, see drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> qcom,pull-up-strength etc.
>
I see, but that custom pin configuration flag can be applied
independently from pin muxing procedure and it can be applied to pins
while they're configured in GPIO mode.
Our "flags" are not of that nature, and only apply to some register
setting during pinmux, as I hopefully tried to clarify above.
Thanks for time and patience in this long email thread.
j
> Yours,
> Linus Walleij
On Wed, Mar 29, 2017 at 4:55 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Wednesday, March 29, 2017, Linus Walleij wrote: >> On Fri, Mar 24, 2017 at 4:22 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> >> wrote: >> >> > Add dt-bindings for Renesas r7s72100 pin controller header file. >> > >> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >> >> > +/* >> > + * Pin is bi-directional. >> > + * An alternate function that needs both input/output functionalities >> > +shall >> > + * be configured as bidirectional. >> > + * Eg. SDA/SCL pins of an I2c interface. >> > + */ >> > +#define BI_DIR (1 << 3) >> >> Any specific reason why this should not simply be added to >> include/linux/pinctrl/pinconf-generic.h >> as PIN_CONFIG_BIDIRECTIONAL and parsed in drivers/pinctrl/pinconf- >> generic.c from the (new) DT property "bidirectional" simply? > > I see your point. It would cut down from every driver out there > inventing some new property or config instead of everyone just sharing > a fixed set. > Maybe someone else out there will end up having a need for a > "bidirectional" option. I was thinking about that one. It is a bit weird electrically, like what kind of electronics is really bidirectional? It seems like a fancy name for open drain/open source, what we call "single ended" configuration. (See docs in Documentation/gpio/driver.txt) It would be great if you could check if that is what they mean, actually. > But, what do we do for Ethernet? All the pins are "normal" except just > the MDIO pin needs to be bidirectional. I see Geert clarified what we could do here. Yours, Linus Walleij
On Wed, Mar 29, 2017 at 5:56 PM, jacopo <jacopo@jmondi.org> wrote: > I can try to give you a few reasons why I don't see those flags fit in > the pin configuration flags definition. > > *) those flags are used during pin multiplexing procedure only and that > procedure has a specific order to be respected: > > You can have a look here: > https://www.spinics.net/lists/linux-renesas-soc/msg12793.html > In "rza1_alternate_function_conf()" function, we need to set bidir > before setting every other register. > The same applies some lines below:, the PIPC, PMC and PM register set order > has to be respected, and depends on those BIDIR and SWIO_* parameters. > This implies those configuration cannot be applied after pin muxing, > certainly not in pin_config[_group]_set() whose invocation time > is independent from pin_mux_set()'s one. But now you are mixing up syntax and semantics. You are describing what steps are necessary on this hardware to apply a certain setting. That is fine, if you didn't need any specific semantics there, you could be using pinctrl-single which will just hammer stuff into registers, one register per pin. You have a pin controller driver exactly beacause your hardware has semantics. How this is described in the device tree or a board file is a different thing from how you write your driver. I understand why i makes for easier code, but does it make for more generic and understandable device trees for people maintaining several systems? I don't think so. > One way forward would be, every time we mux a pin, look for a pinconf group > that includes the pin we're muxing. That would happen for each pin, > for no added benefits imo. The benefit is clearly abstraction, standardization and readability of the device tree. I, as developer, understand what is going on electrically, having seen this on other systems, and being able to access generic documentation on generic pin config properties. > *) as Geert already pointed out, we may need dedicated subnodes to > specify those pin configuration flags, not only because of what Chris > said, but because pinconf_generic_dt_subnode_to_map() wants "pins" or > "groups" to be there in the subnode, and in our pin multiplexing > sub-nodes we only have "pinmux" property (say: we cannot specify > pin_conf flags in the same sub-node where we describe pin > multiplexing, we always need a dedicated sub-node). > Chris and Geert gave some examples in their replies on how that would > like, and how it makes the bindings a little more complex. Very little more complex, and actually it could be argued that this is exactly why subnodes exist: to be able to have different pin config on pins. I think it is very readable. > *) those flags, according to Chris, won't be used in RZ/A2, and > reasonably not in any other RZ device. Do we want to add them to the > generic bindings, being them so specific to this hardware platform? I have seen so much stuff that people say is "necessarily different" for their platform. It turns our that silicon IO and solid state physics isn't that much different between systems. The same things invariably pop up in several chips. Hell this was what people said about this whole subsystem from the beginning: pin control is so necessarily different that there is no point in trying to create an abstraction for it. If I had listened to that kind of advice we wouldn't be where we are today. And that said, I have already pointed out that two of them already exist in the pin control subsystem (PIN_CONFIG*). Because other SoCs are doing similar things. > One thing I suggest considering is to get rid of those flags, at > least in bindings, and introduce 3 variants for each pin multiplexing > function identifier. > > Say: > include/dt-bindings/pinctrl/r7s72100-pinctrl.h: > #define MUX_1 (1 << 16) > #define MUX_1_BIDIR (1 << 16 | 1 << 24) > #define MUX_1_SWIO_IN (1 << 16 | 2 << 24) > #define MUX_1_SWIO_OUT (1 << 16 | 3 << 24) > ... > #define MUX_8 (8 << 16) > #define MUX_8_BIDIR (8 << 16 | 1 << 24) > .... I understand they can be made more beautiful, but in my view that is putting make up on a pig, I want generic pin config for these things. >> What is wrong in doing this with generic pin config using >> PIN_CONFIG_INPUT_ENABLE and PIN_CONFIG_OUTPUT >> (ignoring the argument)? >> >> In the device tree use input-enable and add a new output-enable >> (with unspecified value) with proper description and DT bindings? >> >> And if you think these have no general applicability, by the end >> of the day they are *still* pin config, not magic flags we can choose to >> toss in with the muxing, so you can do what the Qualcomm driver >> does and add custom pin configurations extending the generic >> pin config, see drivers/pinctrl/qcom/pinctrl-spmi-gpio.c >> qcom,pull-up-strength etc. >> > > I see, but that custom pin configuration flag can be applied > independently from pin muxing procedure and it can be applied to pins > while they're configured in GPIO mode. See "GPIO mode pitfalls" in Documentation/pinctrl.txt I've been over this hardware lingo so many times already. It is not about "GPIO" at all, it is about pin configuration. The generic pin config was not invented for GPIO, it was just recently that we started to provide pin config back-ends for GPIO. Only Intel do that so far. > Our "flags" are not of that nature, and only apply to some register > setting during pinmux, as I hopefully tried to clarify above. And that is a driver semantic. Or even a subsystem semantic. No big deal, accumulate such writes in the driver and apply it all when you have it all available no matter if pin multiplexing or pin config happens first? Surely this is just a hardware pecularity, then it warrants some special driver code. If you definately feel you must get a call from the pin control core setting up muxing and config at the same time we need to think of a way to augment the pin control core if necessary? The fact that Linux pin control subsystem semantics you don't like does not affect the relevant device tree bindings. Yours, Linus Walleij
On Thursday, March 30, 2017, Linus Walleij wrote: > >> > +/* > >> > + * Pin is bi-directional. > >> > + * An alternate function that needs both input/output > >> > +functionalities shall > >> > + * be configured as bidirectional. > >> > + * Eg. SDA/SCL pins of an I2c interface. > >> > + */ > >> > +#define BI_DIR (1 << 3) > >> > >> Any specific reason why this should not simply be added to > >> include/linux/pinctrl/pinconf-generic.h > >> as PIN_CONFIG_BIDIRECTIONAL and parsed in drivers/pinctrl/pinconf- > >> generic.c from the (new) DT property "bidirectional" simply? > > > > I see your point. It would cut down from every driver out there > > inventing some new property or config instead of everyone just sharing > > a fixed set. > > Maybe someone else out there will end up having a need for a > > "bidirectional" option. > > I was thinking about that one. It is a bit weird electrically, like what > kind of electronics is really bidirectional? > > It seems like a fancy name for open drain/open source, what we call > "single ended" configuration. (See docs in > Documentation/gpio/driver.txt) > > It would be great if you could check if that is what they mean, actually. Here is the definition of the register in the hardware manual: --- 54.3.13 Port Bidirection Control Register (PBDCn) This register enables or disables the input buffer while the output buffer is enabled. When the input buffer is enabled while the output buffer is enabled, the bidirectional mode is entered, allowing the level of the Pn_m pin to always be read via the PPRn.PPRnm bit. --- But...what they don't really tell you is that any peripheral that needs to TX and RX data over a pin needs this set. In the hardware manual, there is a pretty picture (Figure 54.1 Logical Diagram of Port Control) that shows a detailed logic diagram of the interface between the peripheral bus and the pin pad. As far as I can tell, the "function mux" portion of this controller only knows how to set a pin as direction=in or direction=out. So, in the case of I2C where each the SCL and SDA pins needs to be driven and read...it can't do that. Therefore, there is an additional register to manually enable the input buffer and let the mux enable the output buffer. So while yes, I2C is open-drain, this is also the case for the data pins for the SDHI. And the MDIO pin from Ethernet. It really has to do with the fact that this pin controller wasn't designed to enable both the input and output buffers at the same time. The situation is similar for our SWIO_IN and SWIO_OUT flags. For example, to use the SSI sound interface, you have to set the TX signals to "input" (SWIO_IN). Makes sense, right?? I could argue that all of these "FLAGS" settings should have been incorporated in the HW logic of the pin controller...but for whatever reason, the they had to make them separate registers and make SW do it. So, I could argue that these registers settings are really part of pin muxing, not really user config....and hence belong in the "pinmux" property. How about this compromise: Instead of BI_DIR, we use "TYPE_I2C", "TYPE_SDDATA", "TYPE_MDIO", "TYPE_LVDS", etc... to describe the 'special' ones. That way it can go back under "pinmux". Like Jacopo said, these register settings are really supposed to be set when you are selecting the pin-mux option. Of course behind the scenes, it's really: #define TYPE_I2C BI_DIR #define TYPE_SDDATA BI_DIR #define TYPE_SDCMD BI_DIR #define TYPE_LVDS SWIO_OUT Examples: i2c3_pins: i2c3 { pinmux = <PIN(1, 6) | FUNC_1 | TYPE_I2C>, <PIN(1, 7) | FUNC_1 | TYPE_I2C>; }; /* SHDI ch1 on CN1 */ sdhi1_pins: sdhi1 { /* SHDI ch1 on Port 3 */ pinmux = <PIN(3, 8) | FUNC_7>, /* SDHI1 CD */ <PIN(3, 9) FUNC_7)>, /* SDHI1 WP */ <PIN(3, 10) FUNC_7 | TYPE_SDDATA)>, /* SDHI1 DAT1 */ <PIN(3, 11) FUNC_7 | TYPE_SDDATA)>, /* SDHI1 DAT0 */ <PIN(3, 12) FUNC_7)>, /* SDHI1 CLK */ <PIN(3, 13) FUNC_7 | TYPE_SDCMD)>, /* SDHI1 CMD */ <PIN(3, 14) FUNC_7 | TYPE_SDDATA)>, /* SDHI1 DAT3 */ <PIN(3, 15) FUNC_7 | TYPE_SDDATA)>; /* SDHI1 DAT2 */ }; # Honestly, I have no idea where this pin controller came from. I have not seen it in any other Renesas part (Mitsubishi/Hitachi/NEC). Chris
diff --git a/include/dt-bindings/pinctrl/r7s72100-pinctrl.h b/include/dt-bindings/pinctrl/r7s72100-pinctrl.h new file mode 100644 index 0000000..170338b --- /dev/null +++ b/include/dt-bindings/pinctrl/r7s72100-pinctrl.h @@ -0,0 +1,36 @@ +/* + * Defines macros and constants for Renesas RZ/A1 pin controller pin + * muxing functions. + */ +#ifndef __DT_BINDINGS_PINCTRL_RENESAS_RZA1_H +#define __DT_BINDINGS_PINCTRL_RENESAS_RZA1_H + +#define RZA1_PINS_PER_PORT 16 + +/* Create the pin index from its bank and position numbers */ +#define PIN(b, p) ((b) * RZA1_PINS_PER_PORT + (p)) + +/* + * Flags to apply to alternate function configuration + * All of the following are mutually exclusive. + */ + +/* + * Pin is bi-directional. + * An alternate function that needs both input/output functionalities shall + * be configured as bidirectional. + * Eg. SDA/SCL pins of an I2c interface. + */ +#define BI_DIR (1 << 3) + +/* + * Flags used to ask software to drive the pin I/O direction overriding the + * alternate function configuration. + * Some alternate functions require software to force I/O direction of a pin, + * overriding the designated one. + * Refer to the HW manual to know when this flag shall be used. + */ +#define SWIO_IN (1 << 4) +#define SWIO_OUT (1 << 5) + +#endif
Add dt-bindings for Renesas r7s72100 pin controller header file. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- include/dt-bindings/pinctrl/r7s72100-pinctrl.h | 36 ++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 include/dt-bindings/pinctrl/r7s72100-pinctrl.h