| Message ID | 1398716258-8420-10-git-send-email-tharvey@gateworks.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
Hi Tim, On 04/28/2014 01:17 PM, Tim Harvey wrote: > > Add new function that can take an array of iomux configs, an index, and > a stride to allow a multi-dimentional array of pinmux values to be used > to define pinmux values per cpu-type. > > This takes a different approach to previously proposed solutions which used > multiple arrays of pad lists. The goal is to eliminate having these multiple > arrays such as 'mx6q_uart1_pads' and 'mx6dl_uart1_pads' which are almost > identical copies of each other except for the MX6Q/MX6DL prefix on the PAD. > I don't think a blind reference to the previously proposed solution helps in understanding this patch, and an example would be more helpful. > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > --- > v2: > - moved macros for declaring and using structs for array variant > - removed non-related whitespace cleanup > --- > arch/arm/imx-common/iomux-v3.c | 19 +++++++++++++++---- > arch/arm/include/asm/imx-common/iomux-v3.h | 15 +++++++++++++++ > 2 files changed, 30 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/imx-common/iomux-v3.c b/arch/arm/imx-common/iomux-v3.c > index b59b802..d3e1e30 100644 > --- a/arch/arm/imx-common/iomux-v3.c > +++ b/arch/arm/imx-common/iomux-v3.c > @@ -46,12 +46,23 @@ void imx_iomux_v3_setup_pad(iomux_v3_cfg_t pad) > #endif > } > It's odd that git generated removal and re-insertion of this bit: > -void imx_iomux_v3_setup_multiple_pads(iomux_v3_cfg_t const *pad_list, > - unsigned count) I think 'start_offs' would be clearer than 'list'. The function name ..._array() also doesn't really capture what's going on here. Naming is hard though, and I'm not coming up with something else. Perhaps 'sparse', 'skip', or alternate? > +/* configures a list of pads within an array of lists */ > +void imx_iomux_v3_setup_multiple_pads_array(iomux_v3_cfg_t const *pad_list, > + unsigned count, unsigned list, > + unsigned stride) > { > iomux_v3_cfg_t const *p = pad_list; > int i; > > - for (i = 0; i < count; i++) > - imx_iomux_v3_setup_pad(*p++); > + p += list; > + for (i = 0; i < count; i++) { > + imx_iomux_v3_setup_pad(*p); > + p += stride; > + } > +} > + > +void imx_iomux_v3_setup_multiple_pads(iomux_v3_cfg_t const *pad_list, > + unsigned count) > +{ > + imx_iomux_v3_setup_multiple_pads_array(pad_list, count, 0, 1); > } > diff --git a/arch/arm/include/asm/imx-common/iomux-v3.h b/arch/arm/include/asm/imx-common/iomux-v3.h > index dec11a1..2f7a1cb 100644 > --- a/arch/arm/include/asm/imx-common/iomux-v3.h > +++ b/arch/arm/include/asm/imx-common/iomux-v3.h > @@ -167,7 +167,22 @@ typedef u64 iomux_v3_cfg_t; > #define GPIO_PORTF (5 << GPIO_PORT_SHIFT) > > void imx_iomux_v3_setup_pad(iomux_v3_cfg_t pad); > +void imx_iomux_v3_setup_multiple_pads_array(iomux_v3_cfg_t const *pad_list, > + unsigned count, unsigned list, > + unsigned stride); Ditto about function and parameter naming. > void imx_iomux_v3_setup_multiple_pads(iomux_v3_cfg_t const *pad_list, > unsigned count); > It doesn't get any simpler than this: > +/* macros for declaring and using pinmux array */ > +#define IOMUX_PADS(x) (MX6Q_##x), (MX6DL_##x) In a similar vein to my comment about Patch 8, I do wonder if a minor extension of this will allow use with a single-variant board though. We have some custom designs that really only function with one variant (usually i.MX6DQ) and it seems wrong to have the other variant included. > +#define SETUP_IOMUX_PAD(def) \ > +if (is_cpu_type(MXC_CPU_MX6Q)) { \ > + imx_iomux_v3_setup_pad(MX6Q_##def); \ > +} else { \ > + imx_iomux_v3_setup_pad(MX6DL_##def); \ > +} > +#define SETUP_IOMUX_PADS(x) \ > + imx_iomux_v3_setup_multiple_pads_array(x, \ > + ARRAY_SIZE(x)/2, is_cpu_type(MXC_CPU_MX6Q) ? 0 : 1, 2) > + > #endif /* __MACH_IOMUX_V3_H__*/ > Please don't mis-interpret my comments as any form of Nack. This patch moves the ball forward, and the approach of building two lists into one prevents duplication of tables quite nicely. Regards, Eric
On Tue, Apr 29, 2014 at 8:22 AM, Eric Nelson <eric.nelson@boundarydevices.com> wrote: > Hi Tim, > Hi Eric, > > On 04/28/2014 01:17 PM, Tim Harvey wrote: >> >> >> Add new function that can take an array of iomux configs, an index, and >> a stride to allow a multi-dimentional array of pinmux values to be used >> to define pinmux values per cpu-type. >> >> This takes a different approach to previously proposed solutions which >> used >> multiple arrays of pad lists. The goal is to eliminate having these >> multiple >> arrays such as 'mx6q_uart1_pads' and 'mx6dl_uart1_pads' which are almost >> identical copies of each other except for the MX6Q/MX6DL prefix on the >> PAD. >> > I don't think a blind reference to the previously proposed solution > helps in understanding this patch, and an example would be more helpful. agreed - will remove > <snip> > >> -void imx_iomux_v3_setup_multiple_pads(iomux_v3_cfg_t const *pad_list, >> - unsigned count) > > > I think 'start_offs' would be clearer than 'list'. agreed - will change to 'start_offs' > > The function name ..._array() also doesn't really capture what's > going on here. Naming is hard though, and I'm not coming up > with something else. > > Perhaps 'sparse', 'skip', or alternate? ya, I'm not sure anything else is more explanatory when we are doing something like this. Its bad enough that its likely difficult for someone to understand their first time through that we are doing this to eliminate multiple structs. > <snip> > >> +/* macros for declaring and using pinmux array */ >> +#define IOMUX_PADS(x) (MX6Q_##x), (MX6DL_##x) > > > In a similar vein to my comment about Patch 8, I do wonder if a > minor extension of this will allow use with a single-variant > board though. for a single-variant one would just use the original IOMUX_PAD/imx_iomux_v3_setup_pad/imx_iomux_v3_setup_pad right? > > We have some custom designs that really only function with > one variant (usually i.MX6DQ) and it seems wrong to have > the other variant included. > > >> +#define SETUP_IOMUX_PAD(def) \ >> +if (is_cpu_type(MXC_CPU_MX6Q)) { \ >> + imx_iomux_v3_setup_pad(MX6Q_##def); \ >> +} else { \ >> + imx_iomux_v3_setup_pad(MX6DL_##def); \ >> +} >> +#define SETUP_IOMUX_PADS(x) \ >> + imx_iomux_v3_setup_multiple_pads_array(x, \ >> + ARRAY_SIZE(x)/2, is_cpu_type(MXC_CPU_MX6Q) ? 0 : 1, 2) >> + >> #endif /* __MACH_IOMUX_V3_H__*/ >> > > Please don't mis-interpret my comments as any form of Nack. > > This patch moves the ball forward, and the approach of building > two lists into one prevents duplication of tables quite nicely. > > Regards, > > > Eric > Thanks for the review! Tim
On 06/05/14 07:35, Tim Harvey wrote: > On Tue, Apr 29, 2014 at 8:22 AM, Eric Nelson > <eric.nelson@boundarydevices.com> wrote: <snip> >> The function name ..._array() also doesn't really capture what's >> going on here. Naming is hard though, and I'm not coming up >> with something else. >> >> Perhaps 'sparse', 'skip', or alternate? > ya, I'm not sure anything else is more explanatory when we are doing > something like this. Its bad enough that its likely difficult for > someone to understand their first time through that we are doing this > to eliminate multiple structs. Come to think of it, I don't think we need an _array() function at all. The list selection and stride size are IOMUX_PADS implementation details. It's not something we should expose to the function user. is_cpu_type() and ifdef(CONFIG_MX6QDL) can be used to decide the list and stride values inside imx_iomux_v3_setup_multiple_pads(), and then this function could be used for both single and multi cpu type situations. > > <snip> >>> +/* macros for declaring and using pinmux array */ >>> +#define IOMUX_PADS(x) (MX6Q_##x), (MX6DL_##x) >> >> In a similar vein to my comment about Patch 8, I do wonder if a >> minor extension of this will allow use with a single-variant >> board though. > for a single-variant one would just use the original > IOMUX_PAD/imx_iomux_v3_setup_pad/imx_iomux_v3_setup_pad right? They can, but then we don't get to use the same code for both situations. If we define two versions of IOMUX_PADS: one for multi cpu type, and one for single cpu type, then the pinmux arrays for both situations will be syntactically similar. When combined with my other suggestion, it will be very easy to take a U-Boot configured for one CPU type, and reconfigure it to support both CPU types. > >> We have some custom designs that really only function with >> one variant (usually i.MX6DQ) and it seems wrong to have >> the other variant included. >> >> >>> +#define SETUP_IOMUX_PAD(def) \ >>> +if (is_cpu_type(MXC_CPU_MX6Q)) { \ >>> + imx_iomux_v3_setup_pad(MX6Q_##def); \ >>> +} else { \ >>> + imx_iomux_v3_setup_pad(MX6DL_##def); \ >>> +} >>> +#define SETUP_IOMUX_PADS(x) \ >>> + imx_iomux_v3_setup_multiple_pads_array(x, \ >>> + ARRAY_SIZE(x)/2, is_cpu_type(MXC_CPU_MX6Q) ? 0 : 1, 2) >>> + >>> #endif /* __MACH_IOMUX_V3_H__*/ >>> >> Please don't mis-interpret my comments as any form of Nack. >> >> This patch moves the ball forward, and the approach of building >> two lists into one prevents duplication of tables quite nicely. >> >> Regards, >> >> >> Eric >> > Thanks for the review! > > Tim > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot -- Regards, Nikita Kiryanov
On Wed, May 7, 2014 at 9:59 AM, Nikita Kiryanov <nikita@compulab.co.il> wrote: > > On 06/05/14 07:35, Tim Harvey wrote: >> >> On Tue, Apr 29, 2014 at 8:22 AM, Eric Nelson >> <eric.nelson@boundarydevices.com> wrote: > > <snip> > >>> The function name ..._array() also doesn't really capture what's >>> going on here. Naming is hard though, and I'm not coming up >>> with something else. >>> >>> Perhaps 'sparse', 'skip', or alternate? >> >> ya, I'm not sure anything else is more explanatory when we are doing >> something like this. Its bad enough that its likely difficult for >> someone to understand their first time through that we are doing this >> to eliminate multiple structs. > > > Come to think of it, I don't think we need an _array() function at all. The > list selection and > stride size are IOMUX_PADS implementation details. It's not something we > should expose to > the function user. is_cpu_type() and ifdef(CONFIG_MX6QDL) can be used to > decide the > list and stride values inside imx_iomux_v3_setup_multiple_pads(), and then > this function > could be used for both single and multi cpu type situations. > > >> >> <snip> >>>> >>>> +/* macros for declaring and using pinmux array */ >>>> +#define IOMUX_PADS(x) (MX6Q_##x), (MX6DL_##x) >>> >>> >>> In a similar vein to my comment about Patch 8, I do wonder if a >>> minor extension of this will allow use with a single-variant >>> board though. >> >> for a single-variant one would just use the original >> IOMUX_PAD/imx_iomux_v3_setup_pad/imx_iomux_v3_setup_pad right? > > > They can, but then we don't get to use the same code for both > situations. > If we define two versions of IOMUX_PADS: one for multi cpu type, > and one for single cpu type, then the pinmux arrays for both > situations will be syntactically similar. > When combined with my other suggestion, it will be very easy to > take a U-Boot configured for one CPU type, and reconfigure it to > support both CPU types. > Nikita, Excellent idea - I've merged that idea into my new patchset that I will post shortly. Tim
diff --git a/arch/arm/imx-common/iomux-v3.c b/arch/arm/imx-common/iomux-v3.c index b59b802..d3e1e30 100644 --- a/arch/arm/imx-common/iomux-v3.c +++ b/arch/arm/imx-common/iomux-v3.c @@ -46,12 +46,23 @@ void imx_iomux_v3_setup_pad(iomux_v3_cfg_t pad) #endif } -void imx_iomux_v3_setup_multiple_pads(iomux_v3_cfg_t const *pad_list, - unsigned count) +/* configures a list of pads within an array of lists */ +void imx_iomux_v3_setup_multiple_pads_array(iomux_v3_cfg_t const *pad_list, + unsigned count, unsigned list, + unsigned stride) { iomux_v3_cfg_t const *p = pad_list; int i; - for (i = 0; i < count; i++) - imx_iomux_v3_setup_pad(*p++); + p += list; + for (i = 0; i < count; i++) { + imx_iomux_v3_setup_pad(*p); + p += stride; + } +} + +void imx_iomux_v3_setup_multiple_pads(iomux_v3_cfg_t const *pad_list, + unsigned count) +{ + imx_iomux_v3_setup_multiple_pads_array(pad_list, count, 0, 1); } diff --git a/arch/arm/include/asm/imx-common/iomux-v3.h b/arch/arm/include/asm/imx-common/iomux-v3.h index dec11a1..2f7a1cb 100644 --- a/arch/arm/include/asm/imx-common/iomux-v3.h +++ b/arch/arm/include/asm/imx-common/iomux-v3.h @@ -167,7 +167,22 @@ typedef u64 iomux_v3_cfg_t; #define GPIO_PORTF (5 << GPIO_PORT_SHIFT) void imx_iomux_v3_setup_pad(iomux_v3_cfg_t pad); +void imx_iomux_v3_setup_multiple_pads_array(iomux_v3_cfg_t const *pad_list, + unsigned count, unsigned list, + unsigned stride); void imx_iomux_v3_setup_multiple_pads(iomux_v3_cfg_t const *pad_list, unsigned count); +/* macros for declaring and using pinmux array */ +#define IOMUX_PADS(x) (MX6Q_##x), (MX6DL_##x) +#define SETUP_IOMUX_PAD(def) \ +if (is_cpu_type(MXC_CPU_MX6Q)) { \ + imx_iomux_v3_setup_pad(MX6Q_##def); \ +} else { \ + imx_iomux_v3_setup_pad(MX6DL_##def); \ +} +#define SETUP_IOMUX_PADS(x) \ + imx_iomux_v3_setup_multiple_pads_array(x, \ + ARRAY_SIZE(x)/2, is_cpu_type(MXC_CPU_MX6Q) ? 0 : 1, 2) + #endif /* __MACH_IOMUX_V3_H__*/
Add new function that can take an array of iomux configs, an index, and a stride to allow a multi-dimentional array of pinmux values to be used to define pinmux values per cpu-type. This takes a different approach to previously proposed solutions which used multiple arrays of pad lists. The goal is to eliminate having these multiple arrays such as 'mx6q_uart1_pads' and 'mx6dl_uart1_pads' which are almost identical copies of each other except for the MX6Q/MX6DL prefix on the PAD. Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- v2: - moved macros for declaring and using structs for array variant - removed non-related whitespace cleanup --- arch/arm/imx-common/iomux-v3.c | 19 +++++++++++++++---- arch/arm/include/asm/imx-common/iomux-v3.h | 15 +++++++++++++++ 2 files changed, 30 insertions(+), 4 deletions(-)