| Message ID | 1309406028-2924-2-git-send-email-b32955@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thursday 30 June 2011, Huang Shijie wrote: > add GPMI-NFC support for imx23 and imx28. > > Signed-off-by: Huang Shijie <b32955@freescale.com> This needs a better changelog, please at least spell out what GPMI-NFC means. I'm probably not the only one who gets confused into reading this as 'near field communication' instead of 'nand flash controller'. Could you rename this all into gpmi-nand or gpmi-flash instead, to avoid confusion when you add support for near field communication? > +#define RES_MEM(soc, _id, _s, _n) \ > + { \ > + .start = soc ##_## _id ## _BASE_ADDR, \ > + .end = soc ##_## _id ## _BASE_ADDR + (_s) - 1,\ > + .name = (_n), \ > + .flags = IORESOURCE_MEM, \ > + } > + > +#define RES_IRQ(soc, _id, _n) \ > + { \ > + .start = soc ##_INT_## _id, \ > + .end = soc ##_INT_## _id, \ > + .name = (_n), \ > + .flags = IORESOURCE_IRQ, \ > + } > + > +#define RES_DMA(soc, _i_s, _i_e, _n) \ > + { \ > + .start = soc ##_## _i_s, \ > + .end = soc ##_## _i_e, \ > + .name = (_n), \ > + .flags = IORESOURCE_DMA, \ > + } I know that you didn't start this pattern, but I find these macros extremely annoying. It obscures the use of the macros with the string concatenation and the macro names are way too generic for something platform specific. If people think it's a good idea to have these, please submit a patch to add macros (without the string concatenation) into include/linux/ioport.h. Until then, better spell out the resources. > +/** > + * struct gpmi_nfc_platform_data - GPMI NFC driver platform data. > + * > + * This structure communicates platform-specific information to the GPMI NFC > + * driver that can't be expressed as resources. > + * > + * @platform_init: A pointer to a function the driver will call to > + * initialize the platform (e.g., set up the pin mux). > + * @platform_exit: A pointer to a function the driver will call to > + * exit the platform (e.g., free pins). > + * @min_prop_delay_in_ns: Minimum propagation delay of GPMI signals to and > + * from the NAND Flash device, in nanoseconds. > + * @max_prop_delay_in_ns: Maximum propagation delay of GPMI signals to and > + * from the NAND Flash device, in nanoseconds. > + * @max_chip_count: The maximum number of chips for which the driver > + * should configure the hardware. This value most > + * likely reflects the number of pins that are > + * connected to a NAND Flash device. If this is > + * greater than the SoC hardware can support, the > + * driver will print a message and fail to initialize. > + * @partitions: An optional pointer to an array of partition > + * descriptions. > + * @partition_count: The number of elements in the partitions array. > + */ > +struct gpmi_nfc_platform_data { > + /* SoC hardware information. */ > + int (*platform_init)(void); > + void (*platform_exit)(void); > + > + /* NAND Flash information. */ > + unsigned int min_prop_delay_in_ns; > + unsigned int max_prop_delay_in_ns; > + unsigned int max_chip_count; > + > + /* Medium information. */ > + struct mtd_partition *partitions; > + unsigned partition_count; > +}; When adding new infrastructure, always keep in mind how you want it to look after the device tree conversion. The partitions and min/max_* are easily covered with that, but the init/exit function pointers are somewhat problematic. Fortunately, you don't really require these functions for this driver. The _exit function is completely unused, so just get rid of it. The init function is used only to set up iomux, so the logical replacement is a pointer to the iomux data, and calling mxs_iomux_setup_multiple_pads directly from the driver. Arnd
Hi, Arnd Bergmann writes: > On Thursday 30 June 2011, Huang Shijie wrote: > > add GPMI-NFC support for imx23 and imx28. > > > > Signed-off-by: Huang Shijie <b32955@freescale.com> > [...] > > +struct gpmi_nfc_platform_data { > > + /* SoC hardware information. */ > > + int (*platform_init)(void); > > + void (*platform_exit)(void); > > + > > + /* NAND Flash information. */ > > + unsigned int min_prop_delay_in_ns; > > + unsigned int max_prop_delay_in_ns; > > + unsigned int max_chip_count; > > + > > + /* Medium information. */ > > + struct mtd_partition *partitions; > > + unsigned partition_count; > > +}; > > When adding new infrastructure, always keep in mind how you want it to look > after the device tree conversion. The partitions and min/max_* are easily covered > with that, but the init/exit function pointers are somewhat problematic. > > Fortunately, you don't really require these functions for this driver. The _exit > function is completely unused, so just get rid of it. > > The init function is used only to set up iomux, so the logical replacement is > a pointer to the iomux data, and calling mxs_iomux_setup_multiple_pads > directly from the driver. > NO! I strongly object that. mxs_iomux_setup_multiple_pads() is a platform specific function and has no business inside a device driver that should be platform agnostic. Consider the same controller being part of a different SoC that requires you to call mxs_iomux_v2_setup_multiple_pads() instead. You would then have to check for the SoC type inside the driver to find the right function to call which is quite obscene. I would rather live with the iomux statically configured in the platform init code than having direct calls into platform specific code from device drivers. Lothar Waßmann
On Thursday 30 June 2011 16:58:38 Lothar Waßmann wrote: > > > > When adding new infrastructure, always keep in mind how you want it to look > > after the device tree conversion. The partitions and min/max_* are easily covered > > with that, but the init/exit function pointers are somewhat problematic. > > > > Fortunately, you don't really require these functions for this driver. The _exit > > function is completely unused, so just get rid of it. > > > > The init function is used only to set up iomux, so the logical replacement is > > a pointer to the iomux data, and calling mxs_iomux_setup_multiple_pads > > directly from the driver. > > > NO! I strongly object that. mxs_iomux_setup_multiple_pads() is a > platform specific function and has no business inside a device driver > that should be platform agnostic. > Consider the same controller being part of a different SoC that > requires you to call mxs_iomux_v2_setup_multiple_pads() instead. You > would then have to check for the SoC type inside the driver to find > the right function to call which is quite obscene. Won't this problem just go away as soon as we get the mxs converted to the generic pinmux subsystem that Linus Walleij is doing? Obviously, you would not have to check the soc type really, you would have to instead check for different versions of the gpmi, which you most likely have to anyway because reusing the same hardware block in a new chip usually comes with other changes as well. Note how the driver already does this with code like + if (GPMI_IS_MX23(this) || GPMI_IS_MX28(this)) + nfc = &gpmi_nfc_hal_mxs; If you really want to call out obsceneties, how about the fact that this driver comes with an 805 line patch to add a HAL for a single chip! Such abstractions should not be introduced as long as there is only a single instance of the hardware. > I would rather live with the iomux statically configured in the > platform init code than having direct calls into platform specific > code from device drivers. That would certainly work until we have the pinmux subsystem to take care of it. Arnd
Hi, Arnd Bergmann writes: > On Thursday 30 June 2011 16:58:38 Lothar Waßmann wrote: > > > > > > When adding new infrastructure, always keep in mind how you want it to look > > > after the device tree conversion. The partitions and min/max_* are easily covered > > > with that, but the init/exit function pointers are somewhat problematic. > > > > > > Fortunately, you don't really require these functions for this driver. The _exit > > > function is completely unused, so just get rid of it. > > > > > > The init function is used only to set up iomux, so the logical replacement is > > > a pointer to the iomux data, and calling mxs_iomux_setup_multiple_pads > > > directly from the driver. > > > > > NO! I strongly object that. mxs_iomux_setup_multiple_pads() is a > > platform specific function and has no business inside a device driver > > that should be platform agnostic. > > Consider the same controller being part of a different SoC that > > requires you to call mxs_iomux_v2_setup_multiple_pads() instead. You > > would then have to check for the SoC type inside the driver to find > > the right function to call which is quite obscene. > > Won't this problem just go away as soon as we get the mxs converted to the > generic pinmux subsystem that Linus Walleij is doing? > > Obviously, you would not have to check the soc type really, you would have > to instead check for different versions of the gpmi, which you most likely > have to anyway because reusing the same hardware block in a new chip usually > comes with other changes as well. > The pin muxing is SoC specific, not GPMI specific. So the SoC version should be checked, not something else that may be loosely linked to it. > Note how the driver already does this with code like > > + if (GPMI_IS_MX23(this) || GPMI_IS_MX28(this)) > + nfc = &gpmi_nfc_hal_mxs; > > If you really want to call out obsceneties, how about the fact that this > driver comes with an 805 line patch to add a HAL for a single chip! > > Such abstractions should not be introduced as long as there is only > a single instance of the hardware. > I had already commented on this in the first round of patches from Huang in March this year. Lothar Waßmann
Hi Arnd, > If you really want to call out obsceneties, how about the fact that this > driver comes with an 805 line patch to add a HAL for a single chip! > > Such abstractions should not be introduced as long as there is only > a single instance of the hardware. If I understood correctly, most if not all upcoming i.MX will have the GPMI (mx50, mx6). Huang, do you already have a draft for the mx50-hal? Regards, Wolfram
Hi: > Hi Arnd, > >> If you really want to call out obsceneties, how about the fact that this >> driver comes with an 805 line patch to add a HAL for a single chip! >> >> Such abstractions should not be introduced as long as there is only >> a single instance of the hardware. > If I understood correctly, most if not all upcoming i.MX will have the GPMI > (mx50, mx6). Huang, do you already have a draft for the mx50-hal? > I have finished the code for mx50's GPMI. And I am coding for the MX6's GPMI recently. I need a separate mx50-hal (or mx60-hal) to make the code tidy. The MX50 and mx60 support ONFI NAND and TOGGLE nand(which mx23/mx28 do not support), they need a long code to initialize the TIMING register. What's more, the READ/WRITE functions are different from the mx23/mx28. Frankly speaking, I can merge the mxs-hal.c file to the gpmi-nfc.c, but don't you think it too messy? Best Regards Huang Shijie > Regards, > > Wolfram >
On Fri, Jul 01, 2011 at 03:53:13PM +0800, Huang Shijie wrote: > Hi: > >Hi Arnd, > > > >>If you really want to call out obsceneties, how about the fact that this > >>driver comes with an 805 line patch to add a HAL for a single chip! > >> > >>Such abstractions should not be introduced as long as there is only > >>a single instance of the hardware. > >If I understood correctly, most if not all upcoming i.MX will have the GPMI > >(mx50, mx6). Huang, do you already have a draft for the mx50-hal? > > > I have finished the code for mx50's GPMI. > And I am coding for the MX6's GPMI recently. > > I need a separate mx50-hal (or mx60-hal) to make the code tidy. > The MX50 and mx60 support ONFI NAND and TOGGLE nand(which mx23/mx28 > do not support), > they need a long code to initialize the TIMING register. What's > more, the READ/WRITE functions > are different from the mx23/mx28. > > Frankly speaking, I can merge the mxs-hal.c file to the gpmi-nfc.c, > but don't you think it too > messy? Is it possible to post the mx50 code (as RFC with a note saying that it is not ready yet and is not intended to be merged) so we can see better?
On Friday 01 July 2011 09:53:13 Huang Shijie wrote: > >> If you really want to call out obsceneties, how about the fact that this > >> driver comes with an 805 line patch to add a HAL for a single chip! > >> > >> Such abstractions should not be introduced as long as there is only > >> a single instance of the hardware. > > If I understood correctly, most if not all upcoming i.MX will have the GPMI > > (mx50, mx6). Huang, do you already have a draft for the mx50-hal? > > > I have finished the code for mx50's GPMI. > And I am coding for the MX6's GPMI recently. Ok. > I need a separate mx50-hal (or mx60-hal) to make the code tidy. > The MX50 and mx60 support ONFI NAND and TOGGLE nand(which mx23/mx28 do > not support), > they need a long code to initialize the TIMING register. What's more, > the READ/WRITE functions > are different from the mx23/mx28. > > Frankly speaking, I can merge the mxs-hal.c file to the gpmi-nfc.c, but > don't you think it too > messy? If you already have the code for the other SoCs, it's fine to have an abstraction. From reading your patches that were specifically targetted at mxs, that was not clear. After a very brief look at the driver, my impression is that your layering is upside-down though: You have a "main" driver that registers to all devices and then pulls in hardware specific bits. What you should have instead is a "library" driver for the common code that provides all the common functionality as exported functions, and individual drivers for each SoC type that each register a platform_driver for one device id and use the functions from the common module. I don't know how hard it would be to retrofit that model, and I'm not asking you to do it, unless other people feel strongly about it. However, I think it would make your life easier when maintaining the driver in the future. There are multiple advantages to that: * You would not need a HAL. For your information, that term causes strong negative associations with many Linux developers. To us, the kernel drivers themselves are the hardware abstraction, you don't need another one. * It's more easy to extend to multiple levels of libraries. E.g. you could have one module for stuff that is common between i.MX5 and i.MX6 but not i.MX3, and make the later drivers use both libraries. * It gets rid of the need for #ifdef in the common driver module. * It becomes very easy to override one function just for a specific instance of the hardware, while everything else uses a common version from the library. Arnd
Hello, On Thu, Jun 30, 2011 at 03:55:19PM +0200, Arnd Bergmann wrote: > On Thursday 30 June 2011, Huang Shijie wrote: > > add GPMI-NFC support for imx23 and imx28. > > > > Signed-off-by: Huang Shijie <b32955@freescale.com> > > > +/** > > + * struct gpmi_nfc_platform_data - GPMI NFC driver platform data. > > + * > > + * This structure communicates platform-specific information to the GPMI NFC > > + * driver that can't be expressed as resources. > > + * > > + * @platform_init: A pointer to a function the driver will call to > > + * initialize the platform (e.g., set up the pin mux). > > + * @platform_exit: A pointer to a function the driver will call to > > + * exit the platform (e.g., free pins). > > + * @min_prop_delay_in_ns: Minimum propagation delay of GPMI signals to and > > + * from the NAND Flash device, in nanoseconds. > > + * @max_prop_delay_in_ns: Maximum propagation delay of GPMI signals to and > > + * from the NAND Flash device, in nanoseconds. > > + * @max_chip_count: The maximum number of chips for which the driver > > + * should configure the hardware. This value most > > + * likely reflects the number of pins that are > > + * connected to a NAND Flash device. If this is > > + * greater than the SoC hardware can support, the > > + * driver will print a message and fail to initialize. > > + * @partitions: An optional pointer to an array of partition > > + * descriptions. > > + * @partition_count: The number of elements in the partitions array. > > + */ > > +struct gpmi_nfc_platform_data { > > + /* SoC hardware information. */ > > + int (*platform_init)(void); > > + void (*platform_exit)(void); > > + > > + /* NAND Flash information. */ > > + unsigned int min_prop_delay_in_ns; > > + unsigned int max_prop_delay_in_ns; > > + unsigned int max_chip_count; > > + > > + /* Medium information. */ > > + struct mtd_partition *partitions; > > + unsigned partition_count; > > +}; > > When adding new infrastructure, always keep in mind how you want it to look > after the device tree conversion. The partitions and min/max_* are easily covered > with that, but the init/exit function pointers are somewhat problematic. > > Fortunately, you don't really require these functions for this driver. The _exit > function is completely unused, so just get rid of it. > > The init function is used only to set up iomux, so the logical replacement is > a pointer to the iomux data, and calling mxs_iomux_setup_multiple_pads > directly from the driver. Why not put the iomux stuff into the per-machine table and get rid of the init callback, too? Best regards Uwe
? 2011?07?08? 15:31, Uwe Kleine-König ??: > Hello, > > On Thu, Jun 30, 2011 at 03:55:19PM +0200, Arnd Bergmann wrote: >> On Thursday 30 June 2011, Huang Shijie wrote: >>> add GPMI-NFC support for imx23 and imx28. >>> >>> Signed-off-by: Huang Shijie<b32955@freescale.com> >>> +/** >>> + * struct gpmi_nfc_platform_data - GPMI NFC driver platform data. >>> + * >>> + * This structure communicates platform-specific information to the GPMI NFC >>> + * driver that can't be expressed as resources. >>> + * >>> + * @platform_init: A pointer to a function the driver will call to >>> + * initialize the platform (e.g., set up the pin mux). >>> + * @platform_exit: A pointer to a function the driver will call to >>> + * exit the platform (e.g., free pins). >>> + * @min_prop_delay_in_ns: Minimum propagation delay of GPMI signals to and >>> + * from the NAND Flash device, in nanoseconds. >>> + * @max_prop_delay_in_ns: Maximum propagation delay of GPMI signals to and >>> + * from the NAND Flash device, in nanoseconds. >>> + * @max_chip_count: The maximum number of chips for which the driver >>> + * should configure the hardware. This value most >>> + * likely reflects the number of pins that are >>> + * connected to a NAND Flash device. If this is >>> + * greater than the SoC hardware can support, the >>> + * driver will print a message and fail to initialize. >>> + * @partitions: An optional pointer to an array of partition >>> + * descriptions. >>> + * @partition_count: The number of elements in the partitions array. >>> + */ >>> +struct gpmi_nfc_platform_data { >>> + /* SoC hardware information. */ >>> + int (*platform_init)(void); >>> + void (*platform_exit)(void); >>> + >>> + /* NAND Flash information. */ >>> + unsigned int min_prop_delay_in_ns; >>> + unsigned int max_prop_delay_in_ns; >>> + unsigned int max_chip_count; >>> + >>> + /* Medium information. */ >>> + struct mtd_partition *partitions; >>> + unsigned partition_count; >>> +}; >> When adding new infrastructure, always keep in mind how you want it to look >> after the device tree conversion. The partitions and min/max_* are easily covered >> with that, but the init/exit function pointers are somewhat problematic. >> >> Fortunately, you don't really require these functions for this driver. The _exit >> function is completely unused, so just get rid of it. >> >> The init function is used only to set up iomux, so the logical replacement is >> a pointer to the iomux data, and calling mxs_iomux_setup_multiple_pads >> directly from the driver. > Why not put the iomux stuff into the per-machine table and get rid of > the init callback, too? The mmc (ssp) has pin conflict with gpmi on both mx23evk and mx28evk. So, it's better to initialize the pin when the driver(GPMI or MMC) is enabled. I think the init callback is good solution. Best Regards Huang Shijie > Best regards > Uwe >
On Friday 08 July 2011 09:31:19 Uwe Kleine-König wrote: > > The init function is used only to set up iomux, so the logical replacement is > > a pointer to the iomux data, and calling mxs_iomux_setup_multiple_pads > > directly from the driver. > Why not put the iomux stuff into the per-machine table and get rid of > the init callback, too? Sounds good to me. Arnd
Hello, On Fri, Jul 08, 2011 at 03:40:46PM +0800, Huang Shijie wrote: > ? 2011?07?08? 15:31, Uwe Kleine-König ??: > >Hello, > > > >On Thu, Jun 30, 2011 at 03:55:19PM +0200, Arnd Bergmann wrote: > >>On Thursday 30 June 2011, Huang Shijie wrote: > >>>add GPMI-NFC support for imx23 and imx28. > >>> > >>>Signed-off-by: Huang Shijie<b32955@freescale.com> > >>>+/** > >>>+ * struct gpmi_nfc_platform_data - GPMI NFC driver platform data. > >>>+ * > >>>+ * This structure communicates platform-specific information to the GPMI NFC > >>>+ * driver that can't be expressed as resources. > >>>+ * > >>>+ * @platform_init: A pointer to a function the driver will call to > >>>+ * initialize the platform (e.g., set up the pin mux). > >>>+ * @platform_exit: A pointer to a function the driver will call to > >>>+ * exit the platform (e.g., free pins). > >>>+ * @min_prop_delay_in_ns: Minimum propagation delay of GPMI signals to and > >>>+ * from the NAND Flash device, in nanoseconds. > >>>+ * @max_prop_delay_in_ns: Maximum propagation delay of GPMI signals to and > >>>+ * from the NAND Flash device, in nanoseconds. > >>>+ * @max_chip_count: The maximum number of chips for which the driver > >>>+ * should configure the hardware. This value most > >>>+ * likely reflects the number of pins that are > >>>+ * connected to a NAND Flash device. If this is > >>>+ * greater than the SoC hardware can support, the > >>>+ * driver will print a message and fail to initialize. > >>>+ * @partitions: An optional pointer to an array of partition > >>>+ * descriptions. > >>>+ * @partition_count: The number of elements in the partitions array. > >>>+ */ > >>>+struct gpmi_nfc_platform_data { > >>>+ /* SoC hardware information. */ > >>>+ int (*platform_init)(void); > >>>+ void (*platform_exit)(void); > >>>+ > >>>+ /* NAND Flash information. */ > >>>+ unsigned int min_prop_delay_in_ns; > >>>+ unsigned int max_prop_delay_in_ns; > >>>+ unsigned int max_chip_count; > >>>+ > >>>+ /* Medium information. */ > >>>+ struct mtd_partition *partitions; > >>>+ unsigned partition_count; > >>>+}; > >>When adding new infrastructure, always keep in mind how you want it to look > >>after the device tree conversion. The partitions and min/max_* are easily covered > >>with that, but the init/exit function pointers are somewhat problematic. > >> > >>Fortunately, you don't really require these functions for this driver. The _exit > >>function is completely unused, so just get rid of it. > >> > >>The init function is used only to set up iomux, so the logical replacement is > >>a pointer to the iomux data, and calling mxs_iomux_setup_multiple_pads > >>directly from the driver. > >Why not put the iomux stuff into the per-machine table and get rid of > >the init callback, too? > > The mmc (ssp) has pin conflict with gpmi on both mx23evk and mx28evk. > So, it's better to initialize the pin when the driver(GPMI or MMC) > is enabled. What do you do to prevent userspace from trying to use both devices? I guess you need to configure the hardware somehow to switch between the two using a jumper? Isn't it possible to detect the hardware setting and setup the muxer accordingly? IMHO an per-device init-callback is the wrong approach to solve a pin conflict. Best regards Uwe
On Fri, Jul 08, 2011 at 12:24:22PM +0200, Lothar Waßmann wrote: > Uwe Kleine-König writes: > > Hello Huang, > > > > On Fri, Jul 08, 2011 at 05:27:11PM +0800, Huang Shijie wrote: > > > >>>>The init function is used only to set up iomux, so the logical replacement is > > > >>>>a pointer to the iomux data, and calling mxs_iomux_setup_multiple_pads > > > >>>>directly from the driver. > > > >>>Why not put the iomux stuff into the per-machine table and get rid of > > > >>>the init callback, too? > > > >>The mmc (ssp) has pin conflict with gpmi on both mx23evk and mx28evk. > > > >>So, it's better to initialize the pin when the driver(GPMI or MMC) > > > >>is enabled. > > > >What do you do to prevent userspace from trying to use both devices? > > > The board can not support the two devices at the same time. > > > So the user can only use one device with the board. > > > > > > >I guess you need to configure the hardware somehow to switch between the > > > >two using a jumper? Isn't it possible to detect the hardware setting and > > > >setup the muxer accordingly? > > > > > > > >IMHO an per-device init-callback is the wrong approach to solve a pin > > > >conflict. > > > Do you have any good solution about this? > > Put the pinmux corresponding to the one device that currently works in > > the pinmux list!? > > > #define 'that currently works' > > For a dedicated system that may not be a problem. But for development > kits and modular systems that allow peripheral modules to be plugged > in there is no 'one device that currently works'. Yeah, I know that problem. Back when I worked for a company selling development boards I solved it with clks. Not pretty but more convenient than kernel parameters or #ifdefs. The upside of doing it with clks is that if $customer tries to use both conflicting devices you get an error message instead of breaking device1 when device2 is opened/probed. IMHO this last scenario must not happen, so I strongly object to setup the pinmuxing in an .init callback. Best regards Uwe
Hi Uwe: > On Fri, Jul 08, 2011 at 12:24:22PM +0200, Lothar Waßmann wrote: >> Uwe Kleine-König writes: >>> Hello Huang, >>> >>> On Fri, Jul 08, 2011 at 05:27:11PM +0800, Huang Shijie wrote: >>>>>>>> The init function is used only to set up iomux, so the logical replacement is >>>>>>>> a pointer to the iomux data, and calling mxs_iomux_setup_multiple_pads >>>>>>>> directly from the driver. >>>>>>> Why not put the iomux stuff into the per-machine table and get rid of >>>>>>> the init callback, too? >>>>>> The mmc (ssp) has pin conflict with gpmi on both mx23evk and mx28evk. >>>>>> So, it's better to initialize the pin when the driver(GPMI or MMC) >>>>>> is enabled. >>>>> What do you do to prevent userspace from trying to use both devices? >>>> The board can not support the two devices at the same time. >>>> So the user can only use one device with the board. >>>> >>>>> I guess you need to configure the hardware somehow to switch between the >>>>> two using a jumper? Isn't it possible to detect the hardware setting and >>>>> setup the muxer accordingly? >>>>> >>>>> IMHO an per-device init-callback is the wrong approach to solve a pin >>>>> conflict. >>>> Do you have any good solution about this? >>> Put the pinmux corresponding to the one device that currently works in >>> the pinmux list!? >>> >> #define 'that currently works' >> >> For a dedicated system that may not be a problem. But for development >> kits and modular systems that allow peripheral modules to be plugged >> in there is no 'one device that currently works'. > Yeah, I know that problem. Back when I worked for a company selling > development boards I solved it with clks. Not pretty but more convenient Could you give me some more details about how did you solve it with clks? I am confused about it. thanks Best Regards Huang Shijie > than kernel parameters or #ifdefs. > The upside of doing it with clks is that if $customer tries to use both > conflicting devices you get an error message instead of breaking device1 > when device2 is opened/probed. > IMHO this last scenario must not happen, so I strongly object to setup > the pinmuxing in an .init callback. > > Best regards > Uwe >
Uwe Kleine-König writes: > On Fri, Jul 08, 2011 at 12:24:22PM +0200, Lothar Waßmann wrote: > > Uwe Kleine-König writes: > > > Hello Huang, > > > > > > On Fri, Jul 08, 2011 at 05:27:11PM +0800, Huang Shijie wrote: > > > > >>>>The init function is used only to set up iomux, so the logical replacement is > > > > >>>>a pointer to the iomux data, and calling mxs_iomux_setup_multiple_pads > > > > >>>>directly from the driver. > > > > >>>Why not put the iomux stuff into the per-machine table and get rid of > > > > >>>the init callback, too? > > > > >>The mmc (ssp) has pin conflict with gpmi on both mx23evk and mx28evk. > > > > >>So, it's better to initialize the pin when the driver(GPMI or MMC) > > > > >>is enabled. > > > > >What do you do to prevent userspace from trying to use both devices? > > > > The board can not support the two devices at the same time. > > > > So the user can only use one device with the board. > > > > > > > > >I guess you need to configure the hardware somehow to switch between the > > > > >two using a jumper? Isn't it possible to detect the hardware setting and > > > > >setup the muxer accordingly? > > > > > > > > > >IMHO an per-device init-callback is the wrong approach to solve a pin > > > > >conflict. > > > > Do you have any good solution about this? > > > Put the pinmux corresponding to the one device that currently works in > > > the pinmux list!? > > > > > #define 'that currently works' > > > > For a dedicated system that may not be a problem. But for development > > kits and modular systems that allow peripheral modules to be plugged > > in there is no 'one device that currently works'. > Yeah, I know that problem. Back when I worked for a company selling > development boards I solved it with clks. Not pretty but more convenient > than kernel parameters or #ifdefs. > The upside of doing it with clks is that if $customer tries to use both > conflicting devices you get an error message instead of breaking device1 > when device2 is opened/probed. > This could be prevented by a reservation mechanism for the iomux like for GPIOs. Lothar Waßmann
diff --git a/arch/arm/mach-mxs/clock-mx23.c b/arch/arm/mach-mxs/clock-mx23.c index 0163b6d..e190c53 100644 --- a/arch/arm/mach-mxs/clock-mx23.c +++ b/arch/arm/mach-mxs/clock-mx23.c @@ -456,6 +456,7 @@ static struct clk_lookup lookups[] = { _REGISTER_CLOCK("mxs-pwm.3", NULL, pwm_clk) _REGISTER_CLOCK("mxs-pwm.4", NULL, pwm_clk) _REGISTER_CLOCK("imx23-fb", NULL, lcdif_clk) + _REGISTER_CLOCK("imx23-gpmi-nfc", NULL, gpmi_clk) }; static int clk_misc_init(void) diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c index 5dcc59d..0397f65 100644 --- a/arch/arm/mach-mxs/clock-mx28.c +++ b/arch/arm/mach-mxs/clock-mx28.c @@ -614,6 +614,7 @@ static struct clk_lookup lookups[] = { _REGISTER_CLOCK("duart", NULL, uart_clk) _REGISTER_CLOCK("imx28-fec.0", NULL, fec_clk) _REGISTER_CLOCK("imx28-fec.1", NULL, fec_clk) + _REGISTER_CLOCK("imx28-gpmi-nfc", NULL, gpmi_clk) _REGISTER_CLOCK("mxs-auart.0", NULL, uart_clk) _REGISTER_CLOCK("mxs-auart.1", NULL, uart_clk) _REGISTER_CLOCK("mxs-auart.2", NULL, uart_clk) diff --git a/arch/arm/mach-mxs/devices-mx23.h b/arch/arm/mach-mxs/devices-mx23.h index c6f345f..a9c495d 100644 --- a/arch/arm/mach-mxs/devices-mx23.h +++ b/arch/arm/mach-mxs/devices-mx23.h @@ -21,6 +21,10 @@ extern const struct mxs_auart_data mx23_auart_data[] __initconst; #define mx23_add_auart0() mx23_add_auart(0) #define mx23_add_auart1() mx23_add_auart(1) +extern const struct mxs_gpmi_nfc_data mx23_gpmi_nfc_data __initconst; +#define mx23_add_gpmi_nfc(pdata) \ + mxs_add_gpmi_nfc(pdata, &mx23_gpmi_nfc_data) + extern const struct mxs_mxs_mmc_data mx23_mxs_mmc_data[] __initconst; #define mx23_add_mxs_mmc(id, pdata) \ mxs_add_mxs_mmc(&mx23_mxs_mmc_data[id], pdata) diff --git a/arch/arm/mach-mxs/devices-mx28.h b/arch/arm/mach-mxs/devices-mx28.h index 79b9452..bbc8f0c 100644 --- a/arch/arm/mach-mxs/devices-mx28.h +++ b/arch/arm/mach-mxs/devices-mx28.h @@ -34,6 +34,10 @@ extern const struct mxs_flexcan_data mx28_flexcan_data[] __initconst; #define mx28_add_flexcan0(pdata) mx28_add_flexcan(0, pdata) #define mx28_add_flexcan1(pdata) mx28_add_flexcan(1, pdata) +extern const struct mxs_gpmi_nfc_data mx28_gpmi_nfc_data __initconst; +#define mx28_add_gpmi_nfc(pdata) \ + mxs_add_gpmi_nfc(pdata, &mx28_gpmi_nfc_data) + extern const struct mxs_mxs_i2c_data mx28_mxs_i2c_data[] __initconst; #define mx28_add_mxs_i2c(id) mxs_add_mxs_i2c(&mx28_mxs_i2c_data[id]) diff --git a/arch/arm/mach-mxs/devices/Kconfig b/arch/arm/mach-mxs/devices/Kconfig index acf9eea..b42a14b 100644 --- a/arch/arm/mach-mxs/devices/Kconfig +++ b/arch/arm/mach-mxs/devices/Kconfig @@ -12,6 +12,9 @@ config MXS_HAVE_PLATFORM_FLEXCAN select HAVE_CAN_FLEXCAN if CAN bool +config MXS_HAVE_PLATFORM_GPMI_NFC + bool + config MXS_HAVE_PLATFORM_MXS_I2C bool diff --git a/arch/arm/mach-mxs/devices/Makefile b/arch/arm/mach-mxs/devices/Makefile index 351915c..972abdc 100644 --- a/arch/arm/mach-mxs/devices/Makefile +++ b/arch/arm/mach-mxs/devices/Makefile @@ -3,6 +3,7 @@ obj-$(CONFIG_MXS_HAVE_PLATFORM_AUART) += platform-auart.o obj-y += platform-dma.o obj-$(CONFIG_MXS_HAVE_PLATFORM_FEC) += platform-fec.o obj-$(CONFIG_MXS_HAVE_PLATFORM_FLEXCAN) += platform-flexcan.o +obj-$(CONFIG_MXS_HAVE_PLATFORM_GPMI_NFC) += platform-gpmi-nfc.o obj-$(CONFIG_MXS_HAVE_PLATFORM_MXS_I2C) += platform-mxs-i2c.o obj-$(CONFIG_MXS_HAVE_PLATFORM_MXS_MMC) += platform-mxs-mmc.o obj-$(CONFIG_MXS_HAVE_PLATFORM_MXS_PWM) += platform-mxs-pwm.o diff --git a/arch/arm/mach-mxs/devices/platform-gpmi-nfc.c b/arch/arm/mach-mxs/devices/platform-gpmi-nfc.c new file mode 100644 index 0000000..ae88672 --- /dev/null +++ b/arch/arm/mach-mxs/devices/platform-gpmi-nfc.c @@ -0,0 +1,90 @@ +/* + * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ +#include <asm/sizes.h> +#include <mach/mx23.h> +#include <mach/mx28.h> +#include <mach/devices-common.h> + +#define RES_MEM(soc, _id, _s, _n) \ + { \ + .start = soc ##_## _id ## _BASE_ADDR, \ + .end = soc ##_## _id ## _BASE_ADDR + (_s) - 1,\ + .name = (_n), \ + .flags = IORESOURCE_MEM, \ + } + +#define RES_IRQ(soc, _id, _n) \ + { \ + .start = soc ##_INT_## _id, \ + .end = soc ##_INT_## _id, \ + .name = (_n), \ + .flags = IORESOURCE_IRQ, \ + } + +#define RES_DMA(soc, _i_s, _i_e, _n) \ + { \ + .start = soc ##_## _i_s, \ + .end = soc ##_## _i_e, \ + .name = (_n), \ + .flags = IORESOURCE_DMA, \ + } + +#ifdef CONFIG_SOC_IMX23 +const struct mxs_gpmi_nfc_data mx23_gpmi_nfc_data __initconst = { + .devid = "imx23-gpmi-nfc", + .res = { + /* GPMI */ + RES_MEM(MX23, GPMI, SZ_8K, GPMI_NFC_GPMI_REGS_ADDR_RES_NAME), + RES_IRQ(MX23, GPMI_ATTENTION, GPMI_NFC_GPMI_INTERRUPT_RES_NAME), + /* BCH */ + RES_MEM(MX23, BCH, SZ_8K, GPMI_NFC_BCH_REGS_ADDR_RES_NAME), + RES_IRQ(MX23, BCH, GPMI_NFC_BCH_INTERRUPT_RES_NAME), + /* DMA */ + RES_DMA(MX23, DMA_GPMI0, DMA_GPMI3, + GPMI_NFC_DMA_CHANNELS_RES_NAME), + RES_IRQ(MX23, GPMI_DMA, GPMI_NFC_DMA_INTERRUPT_RES_NAME), + }, +}; +#endif + +#ifdef CONFIG_SOC_IMX28 +const struct mxs_gpmi_nfc_data mx28_gpmi_nfc_data __initconst = { + .devid = "imx28-gpmi-nfc", + .res = { + /* GPMI */ + RES_MEM(MX28, GPMI, SZ_8K, GPMI_NFC_GPMI_REGS_ADDR_RES_NAME), + RES_IRQ(MX28, GPMI, GPMI_NFC_GPMI_INTERRUPT_RES_NAME), + /* BCH */ + RES_MEM(MX28, BCH, SZ_8K, GPMI_NFC_BCH_REGS_ADDR_RES_NAME), + RES_IRQ(MX28, BCH, GPMI_NFC_BCH_INTERRUPT_RES_NAME), + /* DMA */ + RES_DMA(MX28, DMA_GPMI0, DMA_GPMI7, + GPMI_NFC_DMA_CHANNELS_RES_NAME), + RES_IRQ(MX28, GPMI_DMA, GPMI_NFC_DMA_INTERRUPT_RES_NAME), + }, +}; +#endif + +struct platform_device *__init +mxs_add_gpmi_nfc(const struct gpmi_nfc_platform_data *pdata, + const struct mxs_gpmi_nfc_data *data) +{ + return mxs_add_platform_device_dmamask(data->devid, -1, + data->res, RES_SIZE, + pdata, sizeof(*pdata), DMA_BIT_MASK(32)); +} diff --git a/arch/arm/mach-mxs/include/mach/devices-common.h b/arch/arm/mach-mxs/include/mach/devices-common.h index 812d7a8..e032120 100644 --- a/arch/arm/mach-mxs/include/mach/devices-common.h +++ b/arch/arm/mach-mxs/include/mach/devices-common.h @@ -66,6 +66,16 @@ struct platform_device *__init mxs_add_flexcan( const struct mxs_flexcan_data *data, const struct flexcan_platform_data *pdata); +/* gpmi-nfc */ +#include <mach/gpmi-nfc.h> +struct mxs_gpmi_nfc_data { + const char *devid; + const struct resource res[RES_SIZE]; +}; +struct platform_device *__init +mxs_add_gpmi_nfc(const struct gpmi_nfc_platform_data *pdata, + const struct mxs_gpmi_nfc_data *data); + /* i2c */ struct mxs_mxs_i2c_data { int id; diff --git a/arch/arm/mach-mxs/include/mach/gpmi-nfc.h b/arch/arm/mach-mxs/include/mach/gpmi-nfc.h new file mode 100644 index 0000000..eda8192 --- /dev/null +++ b/arch/arm/mach-mxs/include/mach/gpmi-nfc.h @@ -0,0 +1,71 @@ +/* + * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#ifndef __MACH_MXS_GPMI_NFC_H__ +#define __MACH_MXS_GPMI_NFC_H__ + +/* The size of the resource is fixed. */ +#define RES_SIZE 6 + +/* Resource names for the GPMI NFC driver. */ +#define GPMI_NFC_GPMI_REGS_ADDR_RES_NAME "GPMI NFC GPMI Registers" +#define GPMI_NFC_GPMI_INTERRUPT_RES_NAME "GPMI NFC GPMI Interrupt" +#define GPMI_NFC_BCH_REGS_ADDR_RES_NAME "GPMI NFC BCH Registers" +#define GPMI_NFC_BCH_INTERRUPT_RES_NAME "GPMI NFC BCH Interrupt" +#define GPMI_NFC_DMA_CHANNELS_RES_NAME "GPMI NFC DMA Channels" +#define GPMI_NFC_DMA_INTERRUPT_RES_NAME "GPMI NFC DMA Interrupt" + +/** + * struct gpmi_nfc_platform_data - GPMI NFC driver platform data. + * + * This structure communicates platform-specific information to the GPMI NFC + * driver that can't be expressed as resources. + * + * @platform_init: A pointer to a function the driver will call to + * initialize the platform (e.g., set up the pin mux). + * @platform_exit: A pointer to a function the driver will call to + * exit the platform (e.g., free pins). + * @min_prop_delay_in_ns: Minimum propagation delay of GPMI signals to and + * from the NAND Flash device, in nanoseconds. + * @max_prop_delay_in_ns: Maximum propagation delay of GPMI signals to and + * from the NAND Flash device, in nanoseconds. + * @max_chip_count: The maximum number of chips for which the driver + * should configure the hardware. This value most + * likely reflects the number of pins that are + * connected to a NAND Flash device. If this is + * greater than the SoC hardware can support, the + * driver will print a message and fail to initialize. + * @partitions: An optional pointer to an array of partition + * descriptions. + * @partition_count: The number of elements in the partitions array. + */ +struct gpmi_nfc_platform_data { + /* SoC hardware information. */ + int (*platform_init)(void); + void (*platform_exit)(void); + + /* NAND Flash information. */ + unsigned int min_prop_delay_in_ns; + unsigned int max_prop_delay_in_ns; + unsigned int max_chip_count; + + /* Medium information. */ + struct mtd_partition *partitions; + unsigned partition_count; +}; +#endif
add GPMI-NFC support for imx23 and imx28. Signed-off-by: Huang Shijie <b32955@freescale.com> --- arch/arm/mach-mxs/clock-mx23.c | 1 + arch/arm/mach-mxs/clock-mx28.c | 1 + arch/arm/mach-mxs/devices-mx23.h | 4 + arch/arm/mach-mxs/devices-mx28.h | 4 + arch/arm/mach-mxs/devices/Kconfig | 3 + arch/arm/mach-mxs/devices/Makefile | 1 + arch/arm/mach-mxs/devices/platform-gpmi-nfc.c | 90 +++++++++++++++++++++++ arch/arm/mach-mxs/include/mach/devices-common.h | 10 +++ arch/arm/mach-mxs/include/mach/gpmi-nfc.h | 71 ++++++++++++++++++ 9 files changed, 185 insertions(+), 0 deletions(-) create mode 100644 arch/arm/mach-mxs/devices/platform-gpmi-nfc.c create mode 100644 arch/arm/mach-mxs/include/mach/gpmi-nfc.h