| Message ID | BANLkTimPN=GauwY7Rwh3xUEkV3=q3wfzAQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Thu, Apr 28 2011, zhangfei gao wrote: > Subject: [PATCH 2/2] sdhci-pxa: add call back interface to share sdhci-pxa > > Call back functions is added to support mmp2, pxa910, and pxa955 > > Signed-off-by: Zhangfei Gao <zhangfei.gao@marvell.com> > Signed-off-by: RaymondWu <xywu@marvell.com> > Signed-off-by: Jun Nie <njun@marvell.com> This patchset looks good to me. Philip, any objections? Thanks, - Chris.
All other platform specific code is in the host/ directory. This moves it to arch/arm If that is the direction the group wants to go in --> then the patch is fine provided the mmc group can review the patches. Otherwise they are handled by the arm maintainer. On May 12, 2011, at 3:25 PM, Chris Ball wrote: > Hi, > > On Thu, Apr 28 2011, zhangfei gao wrote: >> Subject: [PATCH 2/2] sdhci-pxa: add call back interface to share sdhci-pxa >> >> Call back functions is added to support mmp2, pxa910, and pxa955 >> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@marvell.com> >> Signed-off-by: RaymondWu <xywu@marvell.com> >> Signed-off-by: Jun Nie <njun@marvell.com> > > This patchset looks good to me. Philip, any objections? > > Thanks, > > - Chris. > -- > Chris Ball <cjb@laptop.org> <http://printf.net/> > One Laptop Per Child -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, May 12 2011, Philip Rakity wrote: > All other platform specific code is in the host/ directory. > > This moves it to arch/arm > > If that is the direction the group wants to go in --> then the patch > is fine provided the mmc group can review the patches. Otherwise they > are handled by the arm maintainer. Thanks. Wolfram, do you have any ideas on what the best design is for these SoC-specific changes to sdhci-pxa? - Chris.
On Fri, May 13, 2011 at 9:47 PM, Chris Ball <cjb@laptop.org> wrote: > Hi, > > On Thu, May 12 2011, Philip Rakity wrote: >> All other platform specific code is in the host/ directory. >> >> This moves it to arch/arm >> >> If that is the direction the group wants to go in --> then the patch >> is fine provided the mmc group can review the patches. Otherwise they >> are handled by the arm maintainer. > > Thanks. Wolfram, do you have any ideas on what the best design is for > these SoC-specific changes to sdhci-pxa? > > - Chris. The code in arch/arm is 1. Accessing private register, take pxa910 and mmp2 we want to support as example, there are several private registers differece, though they are same ip, with same issues and quirk. 2. Handle platform difference, for example, mmp2 used in two different platform, one use wp pin, the other does not. Thanks > -- > Chris Ball <cjb@laptop.org> <http://printf.net/> > One Laptop Per Child > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On May 13, 2011, at 10:11 PM, zhangfei gao wrote: > On Fri, May 13, 2011 at 9:47 PM, Chris Ball <cjb@laptop.org> wrote: >> Hi, >> >> On Thu, May 12 2011, Philip Rakity wrote: >>> All other platform specific code is in the host/ directory. >>> >>> This moves it to arch/arm >>> >>> If that is the direction the group wants to go in --> then the patch >>> is fine provided the mmc group can review the patches. Otherwise they >>> are handled by the arm maintainer. >> >> Thanks. Wolfram, do you have any ideas on what the best design is for >> these SoC-specific changes to sdhci-pxa? >> >> - Chris. > > The code in arch/arm is > 1. Accessing private register, take pxa910 and mmp2 we want to support > as example, there are several private registers differece, though they > are same ip, with same issues and quirk. > 2. Handle platform difference, for example, mmp2 used in two different > platform, one use wp pin, the other does not. The situation is a little more complicated. pxa168 and pxa910 share (almost) the same IP The are both based on SD 2.0 controller spec with extensions. The pxa910 controller has fixes to the pxa168 controller. They share the same private registers that allow support for clock gating and timing adjustments. mmp2 is based on SD 3.0 spec. The private register space is different. mmc/host/Kconfig takes no account of these differences. mmp2 and pxa168/910 cannot co-exist. What is currently submitted does not work. One cannot compile mmp2 and pxa910 nor would they work if one could. Mark Brown and I submitted patches to fix this. We added code to the host/ directory that took into account these differences. It provided a common interface layer that then used platform specific code in the host/ directory to handle the different behavior. Arng Bergmann provided advice and reviewed the patches to allow explicit selection of the pxa familty controller and board. Based on this comments we submitted a patch to allow selection if the appropriate SoC. These are two approaches. > > Thanks >> -- >> Chris Ball <cjb@laptop.org> <http://printf.net/> >> One Laptop Per Child >> -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
resend On May 14, 2011, at 10:01 AM, Philip Rakity wrote: > > On May 13, 2011, at 10:11 PM, zhangfei gao wrote: > >> On Fri, May 13, 2011 at 9:47 PM, Chris Ball <cjb@laptop.org> wrote: >>> Hi, >>> >>> On Thu, May 12 2011, Philip Rakity wrote: >>>> All other platform specific code is in the host/ directory. >>>> >>>> This moves it to arch/arm >>>> >>>> If that is the direction the group wants to go in --> then the patch >>>> is fine provided the mmc group can review the patches. Otherwise they >>>> are handled by the arm maintainer. >>> >>> Thanks. Wolfram, do you have any ideas on what the best design is for >>> these SoC-specific changes to sdhci-pxa? >>> >>> - Chris. >> >> The code in arch/arm is >> 1. Accessing private register, take pxa910 and mmp2 we want to support >> as example, there are several private registers differece, though they >> are same ip, with same issues and quirk. >> 2. Handle platform difference, for example, mmp2 used in two different >> platform, one use wp pin, the other does not. > > The situation is a little more complicated. > > pxa168 and pxa910 share (almost) the same IP The are both based on SD 2.0 > controller spec with extensions. The pxa910 controller has fixes to the > pxa168 controller. They share the same private registers that allow support > for clock gating and timing adjustments. > > mmp2 is based on SD 3.0 spec. The private register space is different. > > mmc/host/Kconfig takes no account of these differences. mmp2 and pxa168/910 > cannot co-exist. What is currently submitted does not work. One cannot > compile mmp2 and pxa910 nor would they work if one could. > > Mark Brown and I submitted patches to fix this. We added code to the host/ directory > that took into account these differences. It provided a common interface layer > that then used platform specific code in the host/ directory to handle the different > behavior. > > Arng Bergmann provided advice and reviewed the patches to allow explicit selection of the > pxa familty controller and board. Based on this comments we submitted a patch > to allow selection if the appropriate SoC. > > These are two approaches. > >> >> Thanks >>> -- >>> Chris Ball <cjb@laptop.org> <http://printf.net/> >>> One Laptop Per Child >>> > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, May 14, 2011 at 1:01 PM, Philip Rakity <prakity@marvell.com> wrote: > > On May 13, 2011, at 10:11 PM, zhangfei gao wrote: > >> On Fri, May 13, 2011 at 9:47 PM, Chris Ball <cjb@laptop.org> wrote: >>> Hi, >>> >>> On Thu, May 12 2011, Philip Rakity wrote: >>>> All other platform specific code is in the host/ directory. >>>> >>>> This moves it to arch/arm >>>> >>>> If that is the direction the group wants to go in --> then the patch >>>> is fine provided the mmc group can review the patches. Otherwise they >>>> are handled by the arm maintainer. >>> >>> Thanks. Wolfram, do you have any ideas on what the best design is for >>> these SoC-specific changes to sdhci-pxa? >>> >>> - Chris. >> >> The code in arch/arm is >> 1. Accessing private register, take pxa910 and mmp2 we want to support >> as example, there are several private registers differece, though they >> are same ip, with same issues and quirk. >> 2. Handle platform difference, for example, mmp2 used in two different >> platform, one use wp pin, the other does not. > > The situation is a little more complicated. The interface is used for long time among mmp2, pxa910 and mmp3. Also could be used for new controller with minor register difference but same ip, without adding new specific driver. > > pxa168 and pxa910 share (almost) the same IP The are both based on SD 2.0 > controller spec with extensions. The pxa910 controller has fixes to the > pxa168 controller. They share the same private registers that allow support > for clock gating and timing adjustments. > > mmp2 is based on SD 3.0 spec. The private register space is different. > > mmc/host/Kconfig takes no account of these differences. mmp2 and pxa168/910 > cannot co-exist. What is currently submitted does not work. One cannot > compile mmp2 and pxa910 nor would they work if one could. > > Mark Brown and I submitted patches to fix this. We added code to the host/ directory > that took into account these differences. It provided a common interface layer > that then used platform specific code in the host/ directory to handle the different > behavior. > > Arng Bergmann provided advice and reviewed the patches to allow explicit selection of the > pxa familty controller and board. Based on this comments we submitted a patch > to allow selection if the appropriate SoC. > > These are two approaches. > >> >> Thanks >>> -- >>> Chris Ball <cjb@laptop.org> <http://printf.net/> >>> One Laptop Per Child >>> > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On May 15, 2011, at 11:26 PM, zhangfei gao wrote: > On Sat, May 14, 2011 at 1:01 PM, Philip Rakity <prakity@marvell.com> wrote: >> >> On May 13, 2011, at 10:11 PM, zhangfei gao wrote: >> >>> On Fri, May 13, 2011 at 9:47 PM, Chris Ball <cjb@laptop.org> wrote: >>>> Hi, >>>> >>>> On Thu, May 12 2011, Philip Rakity wrote: >>>>> All other platform specific code is in the host/ directory. >>>>> >>>>> This moves it to arch/arm >>>>> >>>>> If that is the direction the group wants to go in --> then the patch >>>>> is fine provided the mmc group can review the patches. Otherwise they >>>>> are handled by the arm maintainer. >>>> >>>> Thanks. Wolfram, do you have any ideas on what the best design is for >>>> these SoC-specific changes to sdhci-pxa? >>>> >>>> - Chris. >>> >>> The code in arch/arm is >>> 1. Accessing private register, take pxa910 and mmp2 we want to support >>> as example, there are several private registers differece, though they >>> are same ip, with same issues and quirk. >>> 2. Handle platform difference, for example, mmp2 used in two different >>> platform, one use wp pin, the other does not. >> >> The situation is a little more complicated. > > The interface is used for long time among mmp2, pxa910 and mmp3. > Also could be used for new controller with minor register difference > but same ip, without adding new specific driver. applies to both approaches. The mmp2 specific code can be applied to other marvell platforms that share the same controller. Just change Kconfig and the Makefile to use the mmp2 code. The name of the file is not the important thing. It is what it does. The pxa168 and pxa910 code are a little less sharable due to io accessors and some other quirks. For other following the discussion we are probably talking about 100 liens of code. The philosophical location of where the code belongs for host specific drivers is to me more important. Is it in arch/arm and not directly visible to the mmc list or is it like all other platform drivers in drivers/mmc/host. > >> >> pxa168 and pxa910 share (almost) the same IP The are both based on SD 2.0 >> controller spec with extensions. The pxa910 controller has fixes to the >> pxa168 controller. They share the same private registers that allow support >> for clock gating and timing adjustments. >> >> mmp2 is based on SD 3.0 spec. The private register space is different. >> >> mmc/host/Kconfig takes no account of these differences. mmp2 and pxa168/910 >> cannot co-exist. What is currently submitted does not work. One cannot >> compile mmp2 and pxa910 nor would they work if one could. >> >> Mark Brown and I submitted patches to fix this. We added code to the host/ directory >> that took into account these differences. It provided a common interface layer >> that then used platform specific code in the host/ directory to handle the different >> behavior. >> >> Arng Bergmann provided advice and reviewed the patches to allow explicit selection of the >> pxa familty controller and board. Based on this comments we submitted a patch >> to allow selection if the appropriate SoC. >> >> These are two approaches. >> >>> >>> Thanks >>>> -- >>>> Chris Ball <cjb@laptop.org> <http://printf.net/> >>>> One Laptop Per Child >>>> >> >> -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 16, 2011 at 9:51 AM, Philip Rakity <prakity@marvell.com> wrote: > > On May 15, 2011, at 11:26 PM, zhangfei gao wrote: > >> On Sat, May 14, 2011 at 1:01 PM, Philip Rakity <prakity@marvell.com> wrote: >>> >>> On May 13, 2011, at 10:11 PM, zhangfei gao wrote: >>> >>>> On Fri, May 13, 2011 at 9:47 PM, Chris Ball <cjb@laptop.org> wrote: >>>>> Hi, >>>>> >>>>> On Thu, May 12 2011, Philip Rakity wrote: >>>>>> All other platform specific code is in the host/ directory. >>>>>> >>>>>> This moves it to arch/arm >>>>>> >>>>>> If that is the direction the group wants to go in --> then the patch >>>>>> is fine provided the mmc group can review the patches. Otherwise they >>>>>> are handled by the arm maintainer. >>>>> >>>>> Thanks. Wolfram, do you have any ideas on what the best design is for >>>>> these SoC-specific changes to sdhci-pxa? >>>>> >>>>> - Chris. >>>> >>>> The code in arch/arm is >>>> 1. Accessing private register, take pxa910 and mmp2 we want to support >>>> as example, there are several private registers differece, though they >>>> are same ip, with same issues and quirk. >>>> 2. Handle platform difference, for example, mmp2 used in two different >>>> platform, one use wp pin, the other does not. >>> >>> The situation is a little more complicated. >> >> The interface is used for long time among mmp2, pxa910 and mmp3. >> Also could be used for new controller with minor register difference >> but same ip, without adding new specific driver. > > applies to both approaches. The mmp2 specific code can be applied to other marvell > platforms that share the same controller. Just change Kconfig and the Makefile to use > the mmp2 code. The name of the file is not the important thing. It is what it does. > > The pxa168 and pxa910 code are a little less sharable due to io accessors and some > other quirks. > > For other following the discussion we are probably talking about 100 liens of code. > > The philosophical location of where the code belongs for host specific drivers is > to me more important. > > Is it in arch/arm and not directly visible to the mmc list or is it like all other platform drivers > in drivers/mmc/host. The code handle several register difference are located at arch/arm/mach-mmp/mmp2.c is for mmp2 arch/arm/mach-mmp/pxa910.c is for pxa910 serious, arch/arm/mach-mmp/mmp3.c is for mmp3 serious, considering there may still registers changing. The board difference are directly put in board.c For example ttc_dkb do not use wp pin, so get_ro is provided. > > > >> >>> >>> pxa168 and pxa910 share (almost) the same IP The are both based on SD 2.0 >>> controller spec with extensions. The pxa910 controller has fixes to the >>> pxa168 controller. They share the same private registers that allow support >>> for clock gating and timing adjustments. >>> >>> mmp2 is based on SD 3.0 spec. The private register space is different. >>> >>> mmc/host/Kconfig takes no account of these differences. mmp2 and pxa168/910 >>> cannot co-exist. What is currently submitted does not work. One cannot >>> compile mmp2 and pxa910 nor would they work if one could. >>> >>> Mark Brown and I submitted patches to fix this. We added code to the host/ directory >>> that took into account these differences. It provided a common interface layer >>> that then used platform specific code in the host/ directory to handle the different >>> behavior. >>> >>> Arng Bergmann provided advice and reviewed the patches to allow explicit selection of the >>> pxa familty controller and board. Based on this comments we submitted a patch >>> to allow selection if the appropriate SoC. >>> >>> These are two approaches. >>> >>>> >>>> Thanks >>>>> -- >>>>> Chris Ball <cjb@laptop.org> <http://printf.net/> >>>>> One Laptop Per Child >>>>> >>> >>> > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On May 16, 2011, at 7:02 PM, zhangfei gao wrote: > On Mon, May 16, 2011 at 9:51 AM, Philip Rakity <prakity@marvell.com> wrote: >> >> On May 15, 2011, at 11:26 PM, zhangfei gao wrote: >> >>> On Sat, May 14, 2011 at 1:01 PM, Philip Rakity <prakity@marvell.com> wrote: >>>> >>>> On May 13, 2011, at 10:11 PM, zhangfei gao wrote: >>>> >>>>> On Fri, May 13, 2011 at 9:47 PM, Chris Ball <cjb@laptop.org> wrote: >>>>>> Hi, >>>>>> >>>>>> On Thu, May 12 2011, Philip Rakity wrote: >>>>>>> All other platform specific code is in the host/ directory. >>>>>>> >>>>>>> This moves it to arch/arm >>>>>>> >>>>>>> If that is the direction the group wants to go in --> then the patch >>>>>>> is fine provided the mmc group can review the patches. Otherwise they >>>>>>> are handled by the arm maintainer. >>>>>> >>>>>> Thanks. Wolfram, do you have any ideas on what the best design is for >>>>>> these SoC-specific changes to sdhci-pxa? >>>>>> >>>>>> - Chris. >>>>> >>>>> The code in arch/arm is >>>>> 1. Accessing private register, take pxa910 and mmp2 we want to support >>>>> as example, there are several private registers differece, though they >>>>> are same ip, with same issues and quirk. >>>>> 2. Handle platform difference, for example, mmp2 used in two different >>>>> platform, one use wp pin, the other does not. >>>> >>>> The situation is a little more complicated. >>> >>> The interface is used for long time among mmp2, pxa910 and mmp3. >>> Also could be used for new controller with minor register difference >>> but same ip, without adding new specific driver. >> >> applies to both approaches. The mmp2 specific code can be applied to other marvell >> platforms that share the same controller. Just change Kconfig and the Makefile to use >> the mmp2 code. The name of the file is not the important thing. It is what it does. >> >> The pxa168 and pxa910 code are a little less sharable due to io accessors and some >> other quirks. >> >> For other following the discussion we are probably talking about 100 liens of code. >> >> The philosophical location of where the code belongs for host specific drivers is >> to me more important. >> >> Is it in arch/arm and not directly visible to the mmc list or is it like all other platform drivers >> in drivers/mmc/host. > > The code handle several register difference are located at > arch/arm/mach-mmp/mmp2.c is for mmp2 > arch/arm/mach-mmp/pxa910.c is for pxa910 serious, > arch/arm/mach-mmp/mmp3.c is for mmp3 serious, considering there may > still registers changing. > > The board difference are directly put in board.c > For example ttc_dkb do not use wp pin, so get_ro is provided. embedding the code in these chip files stops code sharing. For example, mmp3 has same controller as mmp2. > >> >> >> >>> >>>> >>>> pxa168 and pxa910 share (almost) the same IP The are both based on SD 2.0 >>>> controller spec with extensions. The pxa910 controller has fixes to the >>>> pxa168 controller. They share the same private registers that allow support >>>> for clock gating and timing adjustments. >>>> >>>> mmp2 is based on SD 3.0 spec. The private register space is different. >>>> >>>> mmc/host/Kconfig takes no account of these differences. mmp2 and pxa168/910 >>>> cannot co-exist. What is currently submitted does not work. One cannot >>>> compile mmp2 and pxa910 nor would they work if one could. >>>> >>>> Mark Brown and I submitted patches to fix this. We added code to the host/ directory >>>> that took into account these differences. It provided a common interface layer >>>> that then used platform specific code in the host/ directory to handle the different >>>> behavior. >>>> >>>> Arng Bergmann provided advice and reviewed the patches to allow explicit selection of the >>>> pxa familty controller and board. Based on this comments we submitted a patch >>>> to allow selection if the appropriate SoC. >>>> >>>> These are two approaches. >>>> >>>>> >>>>> Thanks >>>>>> -- >>>>>> Chris Ball <cjb@laptop.org> <http://printf.net/> >>>>>> One Laptop Per Child >>>>>> >>>> >>>> >> >> -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 17, 2011 at 12:27 AM, Philip Rakity <prakity@marvell.com> wrote: > > On May 16, 2011, at 7:02 PM, zhangfei gao wrote: > >> On Mon, May 16, 2011 at 9:51 AM, Philip Rakity <prakity@marvell.com> wrote: >>> >>> On May 15, 2011, at 11:26 PM, zhangfei gao wrote: >>> >>>> On Sat, May 14, 2011 at 1:01 PM, Philip Rakity <prakity@marvell.com> wrote: >>>>> >>>>> On May 13, 2011, at 10:11 PM, zhangfei gao wrote: >>>>> >>>>>> On Fri, May 13, 2011 at 9:47 PM, Chris Ball <cjb@laptop.org> wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On Thu, May 12 2011, Philip Rakity wrote: >>>>>>>> All other platform specific code is in the host/ directory. >>>>>>>> >>>>>>>> This moves it to arch/arm >>>>>>>> >>>>>>>> If that is the direction the group wants to go in --> then the patch >>>>>>>> is fine provided the mmc group can review the patches. Otherwise they >>>>>>>> are handled by the arm maintainer. >>>>>>> >>>>>>> Thanks. Wolfram, do you have any ideas on what the best design is for >>>>>>> these SoC-specific changes to sdhci-pxa? >>>>>>> >>>>>>> - Chris. >>>>>> >>>>>> The code in arch/arm is >>>>>> 1. Accessing private register, take pxa910 and mmp2 we want to support >>>>>> as example, there are several private registers differece, though they >>>>>> are same ip, with same issues and quirk. >>>>>> 2. Handle platform difference, for example, mmp2 used in two different >>>>>> platform, one use wp pin, the other does not. >>>>> >>>>> The situation is a little more complicated. >>>> >>>> The interface is used for long time among mmp2, pxa910 and mmp3. >>>> Also could be used for new controller with minor register difference >>>> but same ip, without adding new specific driver. >>> >>> applies to both approaches. The mmp2 specific code can be applied to other marvell >>> platforms that share the same controller. Just change Kconfig and the Makefile to use >>> the mmp2 code. The name of the file is not the important thing. It is what it does. >>> >>> The pxa168 and pxa910 code are a little less sharable due to io accessors and some >>> other quirks. >>> >>> For other following the discussion we are probably talking about 100 liens of code. >>> >>> The philosophical location of where the code belongs for host specific drivers is >>> to me more important. >>> >>> Is it in arch/arm and not directly visible to the mmc list or is it like all other platform drivers >>> in drivers/mmc/host. >> >> The code handle several register difference are located at >> arch/arm/mach-mmp/mmp2.c is for mmp2 >> arch/arm/mach-mmp/pxa910.c is for pxa910 serious, >> arch/arm/mach-mmp/mmp3.c is for mmp3 serious, considering there may >> still registers changing. >> >> The board difference are directly put in board.c >> For example ttc_dkb do not use wp pin, so get_ro is provided. > > embedding the code in these chip files stops code sharing. For example, > mmp3 has same controller as mmp2. If register are totally same, they can share. If there is minor difference with chip upgrading, they can put in different file. >> >>> >>> >>> >>>> >>>>> >>>>> pxa168 and pxa910 share (almost) the same IP The are both based on SD 2.0 >>>>> controller spec with extensions. The pxa910 controller has fixes to the >>>>> pxa168 controller. They share the same private registers that allow support >>>>> for clock gating and timing adjustments. >>>>> >>>>> mmp2 is based on SD 3.0 spec. The private register space is different. >>>>> >>>>> mmc/host/Kconfig takes no account of these differences. mmp2 and pxa168/910 >>>>> cannot co-exist. What is currently submitted does not work. One cannot >>>>> compile mmp2 and pxa910 nor would they work if one could. >>>>> >>>>> Mark Brown and I submitted patches to fix this. We added code to the host/ directory >>>>> that took into account these differences. It provided a common interface layer >>>>> that then used platform specific code in the host/ directory to handle the different >>>>> behavior. >>>>> >>>>> Arng Bergmann provided advice and reviewed the patches to allow explicit selection of the >>>>> pxa familty controller and board. Based on this comments we submitted a patch >>>>> to allow selection if the appropriate SoC. >>>>> >>>>> These are two approaches. >>>>> >>>>>> >>>>>> Thanks >>>>>>> -- >>>>>>> Chris Ball <cjb@laptop.org> <http://printf.net/> >>>>>>> One Laptop Per Child >>>>>>> >>>>> >>>>> >>> >>> > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 17 May 2011, zhangfei gao wrote: > >> The code handle several register difference are located at > >> arch/arm/mach-mmp/mmp2.c is for mmp2 > >> arch/arm/mach-mmp/pxa910.c is for pxa910 serious, > >> arch/arm/mach-mmp/mmp3.c is for mmp3 serious, considering there may > >> still registers changing. > >> > >> The board difference are directly put in board.c > >> For example ttc_dkb do not use wp pin, so get_ro is provided. > > > > embedding the code in these chip files stops code sharing. For example, > > mmp3 has same controller as mmp2. > > If register are totally same, they can share. > If there is minor difference with chip upgrading, they can put in > different file. When we talked about similar problems in drivers during the Linaro Developer Summit, the broad consensus was to move code from arch/arm into individual device drivers, and I think this should be done for this driver too. My recommendation here would be to have no callbacks in the platform data at all, but instead have the code for all variants in the sdhci-pxa driver itself. You can still have a structure to describe the differences using function pointers, but it's better if that is part of the driver itself. In the platform data, provide a way to identify the variant of the controller (e.g. using an enum) and point to the base addresses as required. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 18, 2011 at 4:38 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 17 May 2011, zhangfei gao wrote: >> >> The code handle several register difference are located at >> >> arch/arm/mach-mmp/mmp2.c is for mmp2 >> >> arch/arm/mach-mmp/pxa910.c is for pxa910 serious, >> >> arch/arm/mach-mmp/mmp3.c is for mmp3 serious, considering there may >> >> still registers changing. >> >> >> >> The board difference are directly put in board.c >> >> For example ttc_dkb do not use wp pin, so get_ro is provided. >> > >> > embedding the code in these chip files stops code sharing. For example, >> > mmp3 has same controller as mmp2. >> >> If register are totally same, they can share. >> If there is minor difference with chip upgrading, they can put in >> different file. > > When we talked about similar problems in drivers during the Linaro Developer > Summit, the broad consensus was to move code from arch/arm into individual > device drivers, and I think this should be done for this driver too. > > My recommendation here would be to have no callbacks in the platform > data at all, but instead have the code for all variants in the > sdhci-pxa driver itself. You can still have a structure to describe > the differences using function pointers, but it's better if that is > part of the driver itself. In the platform data, provide a way > to identify the variant of the controller (e.g. using an enum) > and point to the base addresses as required. > > Arnd > Hi, Arnd Thanks for your suggestion. To make sdhci-pxa easy to maintain, we still prefer put platform specific difference under platform code. "Identify controller from platform data" looks to me is same as using #ifdef. The method is used in our another driver before. More and more workaround comes when more and more platform need to support, making the driver bigger and bigger and not easy to maintain. Nobody remember what's the workaround purpose several years later. So the result is we have to re-write the driver again to make it simple :( Currently, the ip is shared among mmp2, pxa910, pxa168 etc, pxa910 maintainer does not care what workaround is used on pxa168. When mmp3 wants to reuse the ip, it's easier to not care what's the history at all. So we still prefer keep driver as simple as possible, while specific platform self-maintain specific workaround , which is not aware to other platform. Thanks > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 19 May 2011, zhangfei gao wrote: > To make sdhci-pxa easy to maintain, we still prefer put platform > specific difference under platform code. > > "Identify controller from platform data" looks to me is same as using #ifdef. > The method is used in our another driver before. I meant run-time code, not compile-time #ifdef. In the long run, we want to have a kernel that is able to work on many different systems, and that means the drivers should not impose hardcoded limitations. > More and more workaround comes when more and more platform need to > support, making the driver bigger and bigger and not easy to maintain. > Nobody remember what's the workaround purpose several years later. > So the result is we have to re-write the driver again to make it simple :( > > Currently, the ip is shared among mmp2, pxa910, pxa168 etc, pxa910 > maintainer does not care what workaround is used on pxa168. > When mmp3 wants to reuse the ip, it's easier to not care what's the > history at all. > > So we still prefer keep driver as simple as possible, while specific > platform self-maintain specific workaround , which is not aware to > other platform. There are a lot of different ways to do the same thing, but splitting out code into the subarchitecture is not a good one. Please have a look at how Shawn Guo has reworked the sdhci-pltfm drivers to have common part as a library and then multiple users of that. You can probably do the same with the different versions of the sdhci-pxa driver. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 19 May 2011, zhangfei gao wrote: > On Wed, May 18, 2011 at 4:38 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tuesday 17 May 2011, zhangfei gao wrote: > >> >> The code handle several register difference are located at > >> >> arch/arm/mach-mmp/mmp2.c is for mmp2 > >> >> arch/arm/mach-mmp/pxa910.c is for pxa910 serious, > >> >> arch/arm/mach-mmp/mmp3.c is for mmp3 serious, considering there may > >> >> still registers changing. > >> >> > >> >> The board difference are directly put in board.c > >> >> For example ttc_dkb do not use wp pin, so get_ro is provided. > >> > > >> > embedding the code in these chip files stops code sharing. For example, > >> > mmp3 has same controller as mmp2. > >> > >> If register are totally same, they can share. > >> If there is minor difference with chip upgrading, they can put in > >> different file. > > > > When we talked about similar problems in drivers during the Linaro Developer > > Summit, the broad consensus was to move code from arch/arm into individual > > device drivers, and I think this should be done for this driver too. > > > > My recommendation here would be to have no callbacks in the platform > > data at all, but instead have the code for all variants in the > > sdhci-pxa driver itself. You can still have a structure to describe > > the differences using function pointers, but it's better if that is > > part of the driver itself. In the platform data, provide a way > > to identify the variant of the controller (e.g. using an enum) > > and point to the base addresses as required. > > > > Arnd > > > Hi, Arnd > > Thanks for your suggestion. > > To make sdhci-pxa easy to maintain, we still prefer put platform > specific difference under platform code. That's fine to split out platform differences. However please put those differences, maybe in different files, but alongside the driver itself in drivers/mmc/host/sdhci-pxa-pxa910.c, sdhci-pxa-mmp2.c, etc. We don't want to see driver specific stuff hidden in arch/arm/mach-pxa/ or the like. > More and more workaround comes when more and more platform need to > support, making the driver bigger and bigger and not easy to maintain. > Nobody remember what's the workaround purpose several years later. You really really should spend more time documenting those workarounds in the source directly in that case. Anyone not familiar with the issues should be able to understand why a given workaround is there, whether it is today or in several years. And this is why we want driver stuff go in the driver directory where more people specifically interested in MMC/SD are looking. Nicolas
On Fri, May 20, 2011 at 2:24 AM, Nicolas Pitre <nico@fluxnic.net> wrote: > On Thu, 19 May 2011, zhangfei gao wrote: > >> On Wed, May 18, 2011 at 4:38 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Tuesday 17 May 2011, zhangfei gao wrote: >> >> >> The code handle several register difference are located at >> >> >> arch/arm/mach-mmp/mmp2.c is for mmp2 >> >> >> arch/arm/mach-mmp/pxa910.c is for pxa910 serious, >> >> >> arch/arm/mach-mmp/mmp3.c is for mmp3 serious, considering there may >> >> >> still registers changing. >> >> >> >> >> >> The board difference are directly put in board.c >> >> >> For example ttc_dkb do not use wp pin, so get_ro is provided. >> >> > >> >> > embedding the code in these chip files stops code sharing. For example, >> >> > mmp3 has same controller as mmp2. >> >> >> >> If register are totally same, they can share. >> >> If there is minor difference with chip upgrading, they can put in >> >> different file. >> > >> > When we talked about similar problems in drivers during the Linaro Developer >> > Summit, the broad consensus was to move code from arch/arm into individual >> > device drivers, and I think this should be done for this driver too. >> > >> > My recommendation here would be to have no callbacks in the platform >> > data at all, but instead have the code for all variants in the >> > sdhci-pxa driver itself. You can still have a structure to describe >> > the differences using function pointers, but it's better if that is >> > part of the driver itself. In the platform data, provide a way >> > to identify the variant of the controller (e.g. using an enum) >> > and point to the base addresses as required. >> > >> > Arnd >> > >> Hi, Arnd >> >> Thanks for your suggestion. >> >> To make sdhci-pxa easy to maintain, we still prefer put platform >> specific difference under platform code. > > That's fine to split out platform differences. However please put those > differences, maybe in different files, but alongside the driver itself > in drivers/mmc/host/sdhci-pxa-pxa910.c, sdhci-pxa-mmp2.c, etc. We don't > want to see driver specific stuff hidden in arch/arm/mach-pxa/ or the > like. > >> More and more workaround comes when more and more platform need to >> support, making the driver bigger and bigger and not easy to maintain. >> Nobody remember what's the workaround purpose several years later. > > You really really should spend more time documenting those workarounds > in the source directly in that case. Anyone not familiar with the > issues should be able to understand why a given workaround is there, > whether it is today or in several years. And this is why we want driver > stuff go in the driver directory where more people specifically > interested in MMC/SD are looking. Thanks for explanation, will update accordingly. > > > Nicolas > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 19, 2011 at 9:04 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Thursday 19 May 2011, zhangfei gao wrote: >> To make sdhci-pxa easy to maintain, we still prefer put platform >> specific difference under platform code. >> >> "Identify controller from platform data" looks to me is same as using #ifdef. >> The method is used in our another driver before. > > I meant run-time code, not compile-time #ifdef. In the long run, we > want to have a kernel that is able to work on many different systems, > and that means the drivers should not impose hardcoded limitations. > >> More and more workaround comes when more and more platform need to >> support, making the driver bigger and bigger and not easy to maintain. >> Nobody remember what's the workaround purpose several years later. >> So the result is we have to re-write the driver again to make it simple :( >> >> Currently, the ip is shared among mmp2, pxa910, pxa168 etc, pxa910 >> maintainer does not care what workaround is used on pxa168. >> When mmp3 wants to reuse the ip, it's easier to not care what's the >> history at all. >> >> So we still prefer keep driver as simple as possible, while specific >> platform self-maintain specific workaround , which is not aware to >> other platform. > > There are a lot of different ways to do the same thing, but splitting > out code into the subarchitecture is not a good one. Please have a look > at how Shawn Guo has reworked the sdhci-pltfm drivers to have common > part as a library and then multiple users of that. You can probably > do the same with the different versions of the sdhci-pxa driver. > > Arnd > Hi, Arnd When we start to share sdhci-pxa, we find another sdhci-platfm occurs. While in Shawn's patch, driver self-register is recommended. If this is new trend, it maybe a better choice to use separate driver for each device based on updated sdhci-platfm.c by Shawn. Will update the sdhci-mmp2.c for mmp2 only based on Shawn's modification. Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 23 May 2011, zhangfei gao wrote: > When we start to share sdhci-pxa, we find another sdhci-platfm occurs. > While in Shawn's patch, driver self-register is recommended. > If this is new trend, it maybe a better choice to use separate driver > for each device based on updated sdhci-platfm.c by Shawn. > Will update the sdhci-mmp2.c for mmp2 only based on Shawn's modification. Having each driver register its own platform_driver sounds like the best solution to me. Whether you do that on top of sdhci-pltfm.c or model sdhci-pxa.c to be similar to sdhci-pltfm.c is a different question. I don't care much either way, so just do whichever works best for you. Maybe Shawn or Chris can recommend what they think would work better. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/plat-pxa/include/plat/sdhci.h b/arch/arm/plat-pxa/include/plat/sdhci.h index 1ab332e..269dbe6 100644 --- a/arch/arm/plat-pxa/include/plat/sdhci.h +++ b/arch/arm/plat-pxa/include/plat/sdhci.h @@ -13,9 +13,14 @@ #ifndef __PLAT_PXA_SDHCI_H #define __PLAT_PXA_SDHCI_H +#include <linux/mmc/sdhci.h> +#include <linux/platform_device.h> + /* pxa specific flag */ /* Require clock free running */ #define PXA_FLAG_DISABLE_CLOCK_GATING (1<<0) +/* card alwayes wired to host, like on-chip emmc */ +#define PXA_FLAG_CARD_PERMANENT (1<<1) /* Board design supports 8-bit data on SD/SDIO BUS */ #define PXA_FLAG_SD_8_BIT_CAPABLE_SLOT (1<<2) @@ -23,13 +28,54 @@ /* * struct pxa_sdhci_platdata() - Platform device data for PXA SDHCI * @max_speed: the maximum speed supported + * @host_caps: Standard MMC host capabilities bit field. * @quirks: quirks of specific device * @flags: flags for platform requirement + * @clk_delay_cycles: + * mmp2: each step is roughly 100ps, 5bits width + * pxa910: each step is 1ns, 4bits width + * @clk_delay_enable: enable clk_delay or not, used on pxa910 + * @clk_delay_sel: select clk_delay, used on pxa910 + * 0: choose feedback clk + * 1: choose feedback clk + delay value + * 2: choose internal clk + * @ext_cd_gpio: gpio pin used for external CD line + * @ext_cd_gpio_invert: invert values for external CD gpio line + * @soc_set_timing: set timing for specific soc + * @ext_cd_init: Initialize external card detect subsystem + * @ext_cd_cleanup: Cleanup external card detect subsystem + * @soc_set_ops: overwrite host ops with soc specific ops */ +struct sdhci_pxa; struct sdhci_pxa_platdata { unsigned int max_speed; + unsigned int host_caps; unsigned int quirks; unsigned int flags; + unsigned int clk_delay_cycles; + unsigned int clk_delay_sel; + unsigned int ext_cd_gpio; + bool ext_cd_gpio_invert; + bool clk_delay_enable; + + void (*soc_set_timing)(struct sdhci_host *host, + struct sdhci_pxa_platdata *pdata); + int (*ext_cd_init)(void (*notify_func)(struct platform_device *dev, + int state), void *data); + int (*ext_cd_cleanup)(void (*notify_func)(struct platform_device *dev, + int state), void *data); + void (*soc_set_ops)(struct sdhci_pxa *pxa); +}; + +struct sdhci_pxa { + struct sdhci_host *host; + struct sdhci_pxa_platdata *pdata; + struct clk *clk; + struct resource *res; + struct sdhci_ops *ops; + + u8 clk_enable; + u8 power_mode; }; #endif /* __PLAT_PXA_SDHCI_H */ diff --git a/drivers/mmc/host/sdhci-pxa.c b/drivers/mmc/host/sdhci-pxa.c index 5a61208..4a1b7f7 100644 --- a/drivers/mmc/host/sdhci-pxa.c +++ b/drivers/mmc/host/sdhci-pxa.c @@ -24,32 +24,22 @@ #include <linux/clk.h> #include <linux/io.h> #include <linux/err.h> +#include <linux/slab.h> #include <plat/sdhci.h> #include "sdhci.h" #define DRIVER_NAME "sdhci-pxa" -#define SD_FIFO_PARAM 0x104 -#define DIS_PAD_SD_CLK_GATE 0x400 - -struct sdhci_pxa { - struct sdhci_host *host; - struct sdhci_pxa_platdata *pdata; - struct clk *clk; - struct resource *res; - - u8 clk_enable; -}; - /*****************************************************************************\ * * * SDHCI core callbacks * * * \*****************************************************************************/ + static void set_clock(struct sdhci_host *host, unsigned int clock) { struct sdhci_pxa *pxa = sdhci_priv(host); - u32 tmp = 0; + struct sdhci_pxa_platdata *pdata = pxa->pdata; if (clock == 0) { if (pxa->clk_enable) { @@ -57,21 +47,31 @@ static void set_clock(struct sdhci_host *host, unsigned int clock) pxa->clk_enable = 0; } } else { - if (0 == pxa->clk_enable) { - if (pxa->pdata->flags & PXA_FLAG_DISABLE_CLOCK_GATING) { - tmp = readl(host->ioaddr + SD_FIFO_PARAM); - tmp |= DIS_PAD_SD_CLK_GATE; - writel(tmp, host->ioaddr + SD_FIFO_PARAM); - } - clk_enable(pxa->clk); - pxa->clk_enable = 1; - } + if (pdata && pdata->soc_set_timing) + pdata->soc_set_timing(host, pdata); + clk_enable(pxa->clk); + pxa->clk_enable = 1; } } -static struct sdhci_ops sdhci_pxa_ops = { - .set_clock = set_clock, -}; +static void sdhci_pxa_notify_change(struct platform_device *dev, int state) +{ + struct sdhci_host *host = platform_get_drvdata(dev); + unsigned long flags; + + if (host) { + spin_lock_irqsave(&host->lock, flags); + if (state) { + dev_dbg(&dev->dev, "card inserted.\n"); + host->flags &= ~SDHCI_DEVICE_DEAD; + } else { + dev_dbg(&dev->dev, "card removed.\n"); + host->flags |= SDHCI_DEVICE_DEAD; + } + tasklet_schedule(&host->card_tasklet); + spin_unlock_irqrestore(&host->lock, flags); + } +} /*****************************************************************************\