| Message ID | 20181108112647.7205-2-vigneshr@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AM654: Add pinmux support | expand |
On 16:56-20181108, Vignesh R wrote: > From: Tero Kristo <t-kristo@ti.com> > > The dt-bindings header for TI K3-AM6 SoCs define a set of macros for > defining pinmux configs in human readable form, instead of raw-coded > hex values. > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> > Signed-off-by: Vignesh R <vigneshr@ti.com> > --- > MAINTAINERS | 1 + > include/dt-bindings/pinctrl/k3-am6.h | 49 ++++++++++++++++++++++++++++ > 2 files changed, 50 insertions(+) > create mode 100644 include/dt-bindings/pinctrl/k3-am6.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index fb58c64dda49..7fd59955fd21 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2204,6 +2204,7 @@ S: Supported > F: Documentation/devicetree/bindings/arm/ti/k3.txt > F: arch/arm64/boot/dts/ti/Makefile > F: arch/arm64/boot/dts/ti/k3-* > +F: include/dt-bindings/pinctrl/k3-am6.h > > ARM/TEXAS INSTRUMENT KEYSTONE ARCHITECTURE > M: Santosh Shilimkar <ssantosh@kernel.org> > diff --git a/include/dt-bindings/pinctrl/k3-am6.h b/include/dt-bindings/pinctrl/k3-am6.h > new file mode 100644 > index 000000000000..42e22ee57600 > --- /dev/null > +++ b/include/dt-bindings/pinctrl/k3-am6.h Are we thinking of creating headers for every single SoC? Is it possible for us to reduce the number of headers we'd need? > @@ -0,0 +1,49 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * This header provides constants for TI K3-AM6 pinctrl bindings. > + * > + * Copyright (C) 2018 Texas Instruments Please fix this: Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ > + */ > +#ifndef _DT_BINDINGS_PINCTRL_TI_K3_AM6_H > +#define _DT_BINDINGS_PINCTRL_TI_K3_AM6_H > + > +/* K3 mux mode options for each pin. See TRM for options */ > +#define MUX_MODE0 0 > +#define MUX_MODE1 1 > +#define MUX_MODE2 2 > +#define MUX_MODE3 3 > +#define MUX_MODE4 4 > +#define MUX_MODE5 5 > +#define MUX_MODE6 6 > +#define MUX_MODE7 7 > +#define MUX_MODE15 15 > + bit 15? > +#define PULL_DISABLE (1 << 16) PULLUDEN -> this bit enables the Pull > +#define PULL_UP (1 << 17) This is a pull direction selection bit. so, should these be: #define PULLUDEN_SHIFT (16) #define PULLTYPESEL_SHIFT (17) #define RXACTIVE_SHIFT (18) #define PULL_DISABLE (1 << PULLUDEN_SHIFT) #define PULL_ENABLE (0 << PULLUDEN_SHIFT) #define PULL_UP (1 << PULLTYPESEL_SHIFT | PULL_ENABLE) #define PULL_DOWN (0 << PULLTYPESEL_SHIFT | PULL_ENABLE) > +#define INPUT_EN (1 << 18) #define INPUT_EN (1 << RXACTIVE_SHIFT) These names dont seem to match up to the http://www.ti.com/lit/pdf/spruid7 This might be readable when people look up trm and try to generate the pinmux setup. > +#define SLEWCTRL_200MHZ 0 > +#define SLEWCTRL_150MHZ (1 << 19) > +#define SLEWCTRL_100MHZ (2 << 19) > +#define SLEWCTRL_50MHZ (3 << 19) > +#define TX_DIS (1 << 21) What does this mean? Transmit disable? or output disable? > +#define ISO_OVR (1 << 22) > +#define ISO_BYPASS (1 << 23) > +#define DS_EN (1 << 24) > +#define DS_INPUT (1 << 25) Seems to be DSOUT_DIS not the same as input -> in deepsleep, there is no input - this bit simply means that you are driving an output or not during deep sleep? > +#define DS_FORCE_OUT_HIGH (1 << 26) you need a FORCE_OUT_LOW as well to be readable. > +#define DS_PULL_UP_DOWN_EN 0 > +#define DS_PULL_UP_DOWN_DIS (1 << 27) > +#define DS_PULL_UP_SEL (1 << 28) > +#define WAKEUP_ENABLE (1 << 29) I do have a similar set of confusion here as well. > + > +#define PIN_OUTPUT (PULL_DISABLE) > +#define PIN_OUTPUT_PULLUP (PULL_UP) > +#define PIN_OUTPUT_PULLDOWN 0 > +#define PIN_INPUT (INPUT_EN | PULL_DISABLE) > +#define PIN_INPUT_PULLUP (INPUT_EN | PULL_UP) > +#define PIN_INPUT_PULLDOWN (INPUT_EN) Not exactly sure of the approach taken here -> it seems to be a mix of implicit macro -> there needs to be some level of consistency and guidance to dts folks as to which macros are to be used and what not. will be nice if the number of macros are minimal and is clearly documented as to what macros are expected to be used in dts. > + > +#define AM65X_IOPAD(pa, val) (((pa) & 0x1fff)) (val) > +#define AM65X_WKUP_IOPAD(pa, val) (((pa) & 0x1fff)) (val) > + > +#endif > -- > 2.19.1 >
On 08/11/18 7:15 PM, Nishanth Menon wrote: > On 16:56-20181108, Vignesh R wrote: >> From: Tero Kristo <t-kristo@ti.com> >> >> The dt-bindings header for TI K3-AM6 SoCs define a set of macros for >> defining pinmux configs in human readable form, instead of raw-coded >> hex values. >> >> Signed-off-by: Tero Kristo <t-kristo@ti.com> >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> >> Signed-off-by: Vignesh R <vigneshr@ti.com> >> --- >> MAINTAINERS | 1 + >> include/dt-bindings/pinctrl/k3-am6.h | 49 ++++++++++++++++++++++++++++ >> 2 files changed, 50 insertions(+) >> create mode 100644 include/dt-bindings/pinctrl/k3-am6.h >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index fb58c64dda49..7fd59955fd21 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -2204,6 +2204,7 @@ S: Supported >> F: Documentation/devicetree/bindings/arm/ti/k3.txt >> F: arch/arm64/boot/dts/ti/Makefile >> F: arch/arm64/boot/dts/ti/k3-* >> +F: include/dt-bindings/pinctrl/k3-am6.h >> >> ARM/TEXAS INSTRUMENT KEYSTONE ARCHITECTURE >> M: Santosh Shilimkar <ssantosh@kernel.org> >> diff --git a/include/dt-bindings/pinctrl/k3-am6.h b/include/dt-bindings/pinctrl/k3-am6.h >> new file mode 100644 >> index 000000000000..42e22ee57600 >> --- /dev/null >> +++ b/include/dt-bindings/pinctrl/k3-am6.h > > Are we thinking of creating headers for every single SoC? We would need one file per SoC family (i.e atleast one for K3 family as a whole) as pinctrl register layout is significantly different than other TI SoCs. I can rename the file to include/dt-bindings/pinctrl/k3.h to indicate its intended to be common for all K3 SoCs, if you prefer. > > Is it possible for us to reduce the number of headers we'd need? > >> @@ -0,0 +1,49 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * This header provides constants for TI K3-AM6 pinctrl bindings. >> + * >> + * Copyright (C) 2018 Texas Instruments > > Please fix this: > Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ > Done. >> + */ >> +#ifndef _DT_BINDINGS_PINCTRL_TI_K3_AM6_H >> +#define _DT_BINDINGS_PINCTRL_TI_K3_AM6_H >> + >> +/* K3 mux mode options for each pin. See TRM for options */ >> +#define MUX_MODE0 0 >> +#define MUX_MODE1 1 >> +#define MUX_MODE2 2 >> +#define MUX_MODE3 3 >> +#define MUX_MODE4 4 >> +#define MUX_MODE5 5 >> +#define MUX_MODE6 6 >> +#define MUX_MODE7 7 >> +#define MUX_MODE15 15 >> + > bit 15? > Nope its 0xF. Anyway, dropping these macros as suggested by Tony on Patch 2/2 >> +#define PULL_DISABLE (1 << 16) > PULLUDEN -> this bit enables the Pull >> +#define PULL_UP (1 << 17) > This is a pull direction selection bit. > > so, should these be: > #define PULLUDEN_SHIFT (16) > #define PULLTYPESEL_SHIFT (17) > #define RXACTIVE_SHIFT (18) > > #define PULL_DISABLE (1 << PULLUDEN_SHIFT) > #define PULL_ENABLE (0 << PULLUDEN_SHIFT) > > #define PULL_UP (1 << PULLTYPESEL_SHIFT | PULL_ENABLE) > #define PULL_DOWN (0 << PULLTYPESEL_SHIFT | PULL_ENABLE) > >> +#define INPUT_EN (1 << 18) > #define INPUT_EN (1 << RXACTIVE_SHIFT) > > These names dont seem to match up to the http://www.ti.com/lit/pdf/spruid7 > > This might be readable when people look up trm and try to generate the > pinmux setup. > Ok, will update as you suggested above. >> +#define SLEWCTRL_200MHZ 0 >> +#define SLEWCTRL_150MHZ (1 << 19) >> +#define SLEWCTRL_100MHZ (2 << 19) >> +#define SLEWCTRL_50MHZ (3 << 19) > >> +#define TX_DIS (1 << 21) > What does this mean? Transmit disable? or output disable? > >> +#define ISO_OVR (1 << 22) >> +#define ISO_BYPASS (1 << 23) >> +#define DS_EN (1 << 24) >> +#define DS_INPUT (1 << 25) > Seems to be DSOUT_DIS not the same as input -> in deepsleep, > there is no input - this bit simply means that you are driving an output > or not during deep sleep? > >> +#define DS_FORCE_OUT_HIGH (1 << 26) > you need a FORCE_OUT_LOW as well to be readable. > >> +#define DS_PULL_UP_DOWN_EN 0 >> +#define DS_PULL_UP_DOWN_DIS (1 << 27) >> +#define DS_PULL_UP_SEL (1 << 28) >> +#define WAKEUP_ENABLE (1 << 29) > I do have a similar set of confusion here as well. Dropping all the above from v2 as we wont be needing them ATM. >> + >> +#define PIN_OUTPUT (PULL_DISABLE) >> +#define PIN_OUTPUT_PULLUP (PULL_UP) >> +#define PIN_OUTPUT_PULLDOWN 0 >> +#define PIN_INPUT (INPUT_EN | PULL_DISABLE) >> +#define PIN_INPUT_PULLUP (INPUT_EN | PULL_UP) >> +#define PIN_INPUT_PULLDOWN (INPUT_EN) > > > Not exactly sure of the approach taken here -> it seems to be a mix of > implicit macro -> there needs to be some level of consistency and > guidance to dts folks as to which macros are to be used and what not. > Will fix that. > will be nice if the number of macros are minimal and is clearly > documented as to what macros are expected to be used in dts. > I will come up with minimal set needed for now and add a comment as to what macros are intended to be used in DT. >> + >> +#define AM65X_IOPAD(pa, val) (((pa) & 0x1fff)) (val) >> +#define AM65X_WKUP_IOPAD(pa, val) (((pa) & 0x1fff)) (val) >> + >> +#endif >> -- >> 2.19.1 >> >
On 14:20-20181109, Vignesh R wrote: > >> +++ b/include/dt-bindings/pinctrl/k3-am6.h > > > > Are we thinking of creating headers for every single SoC? > > We would need one file per SoC family (i.e atleast one for K3 family as > a whole) as pinctrl register layout is significantly different than Yes, I am aware of the same. > other TI SoCs. I can rename the file to include/dt-bindings/pinctrl/k3.h > to indicate its intended to be common for all K3 SoCs, if you prefer. Yes, that would be the preference. There are two types of IO muxing IPs (the equivalent of padconf from omap days) that I am aware of. However these are all within the same 32bit definitions and I know there has been significant effort spend by designers based on software team feedbacks to maintain consistency as much as possible within technological constraints. So macros could be used to differentiate within the same header. The deltas do exist, however they can be differentiated easily. You should have access to the alternative solution, and even if you cannot publish it, you should plan the header to scale easily when the time is appropriate as a trivial additional patch. For the rest.. Will wait for v2.
diff --git a/MAINTAINERS b/MAINTAINERS index fb58c64dda49..7fd59955fd21 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2204,6 +2204,7 @@ S: Supported F: Documentation/devicetree/bindings/arm/ti/k3.txt F: arch/arm64/boot/dts/ti/Makefile F: arch/arm64/boot/dts/ti/k3-* +F: include/dt-bindings/pinctrl/k3-am6.h ARM/TEXAS INSTRUMENT KEYSTONE ARCHITECTURE M: Santosh Shilimkar <ssantosh@kernel.org> diff --git a/include/dt-bindings/pinctrl/k3-am6.h b/include/dt-bindings/pinctrl/k3-am6.h new file mode 100644 index 000000000000..42e22ee57600 --- /dev/null +++ b/include/dt-bindings/pinctrl/k3-am6.h @@ -0,0 +1,49 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * This header provides constants for TI K3-AM6 pinctrl bindings. + * + * Copyright (C) 2018 Texas Instruments + */ +#ifndef _DT_BINDINGS_PINCTRL_TI_K3_AM6_H +#define _DT_BINDINGS_PINCTRL_TI_K3_AM6_H + +/* K3 mux mode options for each pin. See TRM for options */ +#define MUX_MODE0 0 +#define MUX_MODE1 1 +#define MUX_MODE2 2 +#define MUX_MODE3 3 +#define MUX_MODE4 4 +#define MUX_MODE5 5 +#define MUX_MODE6 6 +#define MUX_MODE7 7 +#define MUX_MODE15 15 + +#define PULL_DISABLE (1 << 16) +#define PULL_UP (1 << 17) +#define INPUT_EN (1 << 18) +#define SLEWCTRL_200MHZ 0 +#define SLEWCTRL_150MHZ (1 << 19) +#define SLEWCTRL_100MHZ (2 << 19) +#define SLEWCTRL_50MHZ (3 << 19) +#define TX_DIS (1 << 21) +#define ISO_OVR (1 << 22) +#define ISO_BYPASS (1 << 23) +#define DS_EN (1 << 24) +#define DS_INPUT (1 << 25) +#define DS_FORCE_OUT_HIGH (1 << 26) +#define DS_PULL_UP_DOWN_EN 0 +#define DS_PULL_UP_DOWN_DIS (1 << 27) +#define DS_PULL_UP_SEL (1 << 28) +#define WAKEUP_ENABLE (1 << 29) + +#define PIN_OUTPUT (PULL_DISABLE) +#define PIN_OUTPUT_PULLUP (PULL_UP) +#define PIN_OUTPUT_PULLDOWN 0 +#define PIN_INPUT (INPUT_EN | PULL_DISABLE) +#define PIN_INPUT_PULLUP (INPUT_EN | PULL_UP) +#define PIN_INPUT_PULLDOWN (INPUT_EN) + +#define AM65X_IOPAD(pa, val) (((pa) & 0x1fff)) (val) +#define AM65X_WKUP_IOPAD(pa, val) (((pa) & 0x1fff)) (val) + +#endif