| Message ID | E1TAnGn-00086i-Lb@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Sep 09, 2012 at 08:34:13PM +0100, Russell King wrote: > Allow PCI support for Dove to be disabled. Some platforms do not have > anything connected to the PCIe ports, so requiring PCI support to be > built into the kernel just wastes space, and presents a lot more config > options than are necessary. > > However, select USB_ARCH_HAS_EHCI so that we can still have EHCI > support. Hi Russel I have a patch in GregKH usb-next which also ensures USB_ARCH_HAS_EHCI is selected. http://git.kernel.org/?p=linux/kernel/git/gregkh/usb.git;a=commitdiff;h=ba2f9dff6c914b91005f687a9d75c8eac110d323 So we might not need your select as well, although it does not harm. Andrew
On Sun, Sep 09, 2012 at 10:47:05PM +0200, Andrew Lunn wrote: > On Sun, Sep 09, 2012 at 08:34:13PM +0100, Russell King wrote: > > Allow PCI support for Dove to be disabled. Some platforms do not have > > anything connected to the PCIe ports, so requiring PCI support to be > > built into the kernel just wastes space, and presents a lot more config > > options than are necessary. > > > > However, select USB_ARCH_HAS_EHCI so that we can still have EHCI > > support. > > Hi Russel > > I have a patch in GregKH usb-next which also ensures USB_ARCH_HAS_EHCI > is selected. > > http://git.kernel.org/?p=linux/kernel/git/gregkh/usb.git;a=commitdiff;h=ba2f9dff6c914b91005f687a9d75c8eac110d323 > > So we might not need your select as well, although it does not harm. That depends on how this patch is handled. If it gets into mainline via a separate path to GregKH's USB tree, then it must be kept together as otherwise it _will_ cause regressions - namely EHCI becomes impossible to select when PCI=n. That in turn makes the CuBox (and probably other Dove platforms) a rather useless display-only platform...
On Sun, Sep 09, 2012 at 09:55:21PM +0100, Russell King - ARM Linux wrote: > On Sun, Sep 09, 2012 at 10:47:05PM +0200, Andrew Lunn wrote: > > On Sun, Sep 09, 2012 at 08:34:13PM +0100, Russell King wrote: > > > Allow PCI support for Dove to be disabled. Some platforms do not have > > > anything connected to the PCIe ports, so requiring PCI support to be > > > built into the kernel just wastes space, and presents a lot more config > > > options than are necessary. > > > > > > However, select USB_ARCH_HAS_EHCI so that we can still have EHCI > > > support. > > > > Hi Russel > > > > I have a patch in GregKH usb-next which also ensures USB_ARCH_HAS_EHCI > > is selected. > > > > http://git.kernel.org/?p=linux/kernel/git/gregkh/usb.git;a=commitdiff;h=ba2f9dff6c914b91005f687a9d75c8eac110d323 > > > > So we might not need your select as well, although it does not harm. > > That depends on how this patch is handled. If it gets into mainline via > a separate path to GregKH's USB tree, then it must be kept together as > otherwise it _will_ cause regressions - namely EHCI becomes impossible > to select when PCI=n. That in turn makes the CuBox (and probably other > Dove platforms) a rather useless di [message trucated, I need to working on file locking for my email auto-flagging script... :-/] greg just pulled Andrew's patch into his usb tree within the past few days. Russell, would you prefer to submit this to greg's usb tree, or resend without USB_ARCH_HAS_EHCI? thx, Jason.
On Sun, Sep 09, 2012 at 05:01:26PM -0400, Jason Cooper wrote: > On Sun, Sep 09, 2012 at 09:55:21PM +0100, Russell King - ARM Linux wrote: > > On Sun, Sep 09, 2012 at 10:47:05PM +0200, Andrew Lunn wrote: > > > On Sun, Sep 09, 2012 at 08:34:13PM +0100, Russell King wrote: > > > > Allow PCI support for Dove to be disabled. Some platforms do not have > > > > anything connected to the PCIe ports, so requiring PCI support to be > > > > built into the kernel just wastes space, and presents a lot more config > > > > options than are necessary. > > > > > > > > However, select USB_ARCH_HAS_EHCI so that we can still have EHCI > > > > support. > > > > > > Hi Russel > > > > > > I have a patch in GregKH usb-next which also ensures USB_ARCH_HAS_EHCI > > > is selected. > > > > > > http://git.kernel.org/?p=linux/kernel/git/gregkh/usb.git;a=commitdiff;h=ba2f9dff6c914b91005f687a9d75c8eac110d323 > > > > > > So we might not need your select as well, although it does not harm. > > > > That depends on how this patch is handled. If it gets into mainline via > > a separate path to GregKH's USB tree, then it must be kept together as > > otherwise it _will_ cause regressions - namely EHCI becomes impossible > > to select when PCI=n. That in turn makes the CuBox (and probably other > > Dove platforms) a rather useless di > [message trucated, I need to working on file locking for my email > auto-flagging script... :-/] > > greg just pulled Andrew's patch into his usb tree within the past few > days. Russell, would you prefer to submit this to greg's usb tree, or > resend without USB_ARCH_HAS_EHCI? I thought I'd already answered the latter point. As I said, it breaks the patch to remove that select, so it can not be removed. Removing it will create regressions - can I be any more clear about that. So no, I will not remove that select statement because I can't without _breaking_ my Dove builds here. I don't think submitting this patch to Greg's USB tree is also appropriate either, it's clearly ARM specific in nature, or even PCI specific. It isn't a USB patch. Neither is a patch changing a USB setting in arch/arm/Kconfig. If any tree, these patches should be going through arm-soc.
On Mon, Sep 10, 2012 at 01:45:11AM +0100, Russell King - ARM Linux wrote: > On Sun, Sep 09, 2012 at 05:01:26PM -0400, Jason Cooper wrote: > > On Sun, Sep 09, 2012 at 09:55:21PM +0100, Russell King - ARM Linux wrote: > > > On Sun, Sep 09, 2012 at 10:47:05PM +0200, Andrew Lunn wrote: > > > > On Sun, Sep 09, 2012 at 08:34:13PM +0100, Russell King wrote: > > > > > Allow PCI support for Dove to be disabled. Some platforms do not have > > > > > anything connected to the PCIe ports, so requiring PCI support to be > > > > > built into the kernel just wastes space, and presents a lot more config > > > > > options than are necessary. > > > > > > > > > > However, select USB_ARCH_HAS_EHCI so that we can still have EHCI > > > > > support. > > > > > > > > Hi Russel > > > > > > > > I have a patch in GregKH usb-next which also ensures USB_ARCH_HAS_EHCI > > > > is selected. > > > > > > > > http://git.kernel.org/?p=linux/kernel/git/gregkh/usb.git;a=commitdiff;h=ba2f9dff6c914b91005f687a9d75c8eac110d323 > > > > > > > > So we might not need your select as well, although it does not harm. > > > > > > That depends on how this patch is handled. If it gets into mainline via > > > a separate path to GregKH's USB tree, then it must be kept together as > > > otherwise it _will_ cause regressions - namely EHCI becomes impossible > > > to select when PCI=n. That in turn makes the CuBox (and probably other > > > Dove platforms) a rather useless di > > [message trucated, I need to working on file locking for my email > > auto-flagging script... :-/] > > > > greg just pulled Andrew's patch into his usb tree within the past few > > days. Russell, would you prefer to submit this to greg's usb tree, or > > resend without USB_ARCH_HAS_EHCI? > > I thought I'd already answered the latter point. > > As I said, it breaks the patch to remove that select, so it can not be > removed. Removing it will create regressions - can I be any more clear > about that. So no, I will not remove that select statement because I > can't without _breaking_ my Dove builds here. My apologies. I was simply trying to achieve the cleanest solution. Andrew's patch selects USB_ARCH_HAS_EHCI whenever PLAT_ORION is enabled, of which dove is one. Purely from the pov of the code, this makes the select redundant. iiuc, the regression you are refering to would occur iff this patch (without the select) makes it into mainline _before_ Andrew's patch in Greg's tree. As I haven't pulled this patch yet, I can add a dependency in the tag message listing the patch in gregkh's usb tree. Arnd and Olof would heed this and make sure this patch comes after Andrews. This should prevent the regression. If the above is true, then indeed, the select is not necessary. Is there something else I'm missing? > I don't think submitting this patch to Greg's USB tree is also > appropriate either, it's clearly ARM specific in nature, or even PCI > specific. It isn't a USB patch. Agreed, please chalk that up to new maintainer crack-smoking. :-) > Neither is a patch changing a USB setting in arch/arm/Kconfig. If any > tree, these patches should be going through arm-soc. Agreed. thx, Jason.
On Sun, Sep 09, 2012 at 09:44:22PM -0400, Jason Cooper wrote: > iiuc, the regression you are refering to would occur iff this patch > (without the select) makes it into mainline _before_ Andrew's patch in > Greg's tree. As I haven't pulled this patch yet, I can add a dependency > in the tag message listing the patch in gregkh's usb tree. For the regression to occur, you need to land on a commit in the tree where this patch (without the select) is in the history but not the patch in Greg's tree. The commit history remains a series of independent branches even after trees are merged into Linus' tree - there is _no_ linearisation of the history in git. This means when anyone comes to use git bisect, they _can_ land on any commit in any branch, which means if this patch and the other patch take two different routes, and this patch does not have the select, you _will_ hit the regression. Let me illustrate: ---A----B----C----D---...---E----M `--G----H----I----J---...---K-' Let's say A-E is Greg's tree, and D is the commit you are talking about. Let's say G-K is Arnd's tree, and H is my patch without the 'select' statement. Let's also say that '...' in each case includes a significant number of commits. If, through a git bisect (or manual commit selection) you end up trying to build at any commit I to K, you will not be builting a tree with D in, but you will be building a tree with H in. You end up with EHCI being disabled in your configuration. What this also means is that when building Arnd's tree without Greg's tree, you will also end up with EHCI disabled in those configurations (because you also don't have commit D.) See? There is no such thing as "lets make sure tree X gets merged before tree Y because commit A depends on commit B". That "solution" doesn't fix any dependency between the two commits - it merely gives the impression that it does because you're looking only at the result. That kind of solution is broken thinking. > Arnd and > Olof would heed this and make sure this patch comes after Andrews. This > should prevent the regression. As you can see from the above, tree order does not solve these issues. It gives the impression from the final result that it does, but it doesn't. It breaks at least our most powerful regression debug tool - git bisect.
On Sun, 9 Sep 2012, Jason Cooper wrote: > On Mon, Sep 10, 2012 at 01:45:11AM +0100, Russell King - ARM Linux wrote: > > On Sun, Sep 09, 2012 at 05:01:26PM -0400, Jason Cooper wrote: > > > On Sun, Sep 09, 2012 at 09:55:21PM +0100, Russell King - ARM Linux wrote: > > > > On Sun, Sep 09, 2012 at 10:47:05PM +0200, Andrew Lunn wrote: > > > > > On Sun, Sep 09, 2012 at 08:34:13PM +0100, Russell King wrote: > > > > > > Allow PCI support for Dove to be disabled. Some platforms do not have > > > > > > anything connected to the PCIe ports, so requiring PCI support to be > > > > > > built into the kernel just wastes space, and presents a lot more config > > > > > > options than are necessary. > > > > > > > > > > > > However, select USB_ARCH_HAS_EHCI so that we can still have EHCI > > > > > > support. > > > > > > > > > > Hi Russel > > > > > > > > > > I have a patch in GregKH usb-next which also ensures USB_ARCH_HAS_EHCI > > > > > is selected. > > > > > > > > > > http://git.kernel.org/?p=linux/kernel/git/gregkh/usb.git;a=commitdiff;h=ba2f9dff6c914b91005f687a9d75c8eac110d323 > > > > > > > > > > So we might not need your select as well, although it does not harm. > > > > > > > > That depends on how this patch is handled. If it gets into mainline via > > > > a separate path to GregKH's USB tree, then it must be kept together as > > > > otherwise it _will_ cause regressions - namely EHCI becomes impossible > > > > to select when PCI=n. That in turn makes the CuBox (and probably other > > > > Dove platforms) a rather useless di > > > [message trucated, I need to working on file locking for my email > > > auto-flagging script... :-/] > > > > > > greg just pulled Andrew's patch into his usb tree within the past few > > > days. Russell, would you prefer to submit this to greg's usb tree, or > > > resend without USB_ARCH_HAS_EHCI? > > > > I thought I'd already answered the latter point. > > > > As I said, it breaks the patch to remove that select, so it can not be > > removed. Removing it will create regressions - can I be any more clear > > about that. So no, I will not remove that select statement because I > > can't without _breaking_ my Dove builds here. > > My apologies. I was simply trying to achieve the cleanest solution. > Andrew's patch selects USB_ARCH_HAS_EHCI whenever PLAT_ORION is enabled, > of which dove is one. Purely from the pov of the code, this makes the > select redundant. Are we trying to split hairs here? At best, the select will be redundant and that is trivially cleaned up later. At worst, you'll have a trivial git merge conflict to solve (and that happens all the time). So the simplest thing to do is to keep the select there to prevent build issues and remember that perfection is the enemy of the good. Nicolas
On Mon, 10 Sep 2012, Russell King - ARM Linux wrote: > On Sun, Sep 09, 2012 at 09:44:22PM -0400, Jason Cooper wrote: > > iiuc, the regression you are refering to would occur iff this patch > > (without the select) makes it into mainline _before_ Andrew's patch in > > Greg's tree. As I haven't pulled this patch yet, I can add a dependency > > in the tag message listing the patch in gregkh's usb tree. > > For the regression to occur, you need to land on a commit in the tree > where this patch (without the select) is in the history but not the > patch in Greg's tree. > > The commit history remains a series of independent branches even after > trees are merged into Linus' tree - there is _no_ linearisation of the > history in git. @Jason: The dependency tracking done in the arm-soc tree performed by Arnd and Olof is merely a way for you to _base_ your branch on top of other people's branch before they pull it from you. For example, you may decide to build your branch on top of Greg's to get some prerequisite commits for your work. In that case the goal for the arm-soc maintainers is to avoid the situation where a pull of the arm-soc tree into mainline would also pull greg's stuff without his asking for it. Nicolas
On Mon, Sep 10, 2012 at 12:45:10PM -0400, Nicolas Pitre wrote: > So the simplest thing to do is to keep the select there to prevent build > issues and remember that perfection is the enemy of the good. Yes, I have to remind myself of that a lot. To the point I've considered getting a tattoo of it somewhere. :-) You are right. I was primarily trying to drill down on something I didn't understand, which is how leaving out that line could cause a regression. The patch is fine, of course, I'll pull it in in my next round. Anything else in this thread will be purely me attempting to make sure I fully understand git and the merge process wrt to causing regressions. thx, Jason.
On Mon, Sep 10, 2012 at 09:29:42AM +0100, Russell King - ARM Linux wrote: > On Sun, Sep 09, 2012 at 09:44:22PM -0400, Jason Cooper wrote: > > iiuc, the regression you are refering to would occur iff this patch > > (without the select) makes it into mainline _before_ Andrew's patch in > > Greg's tree. As I haven't pulled this patch yet, I can add a dependency > > in the tag message listing the patch in gregkh's usb tree. > > For the regression to occur, you need to land on a commit in the tree > where this patch (without the select) is in the history but not the > patch in Greg's tree. > > The commit history remains a series of independent branches even after > trees are merged into Linus' tree - there is _no_ linearisation of the > history in git. <ding> (light comes on). Thank you! Somewhere in the time I've been working with git I developed an incorrect assumption. While embarrassing, I'm glad I asked for clarification. Anytime I view the merging of branches in my head, I always see gitg's "all branches" view. Since patches are sorted over time, I assumed they became related that way once they were merged. It sounds silly as I type it out loud. :-) It all makes sense now. Thanks for taking the time to explain it so clearly, Jason.
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 27f5b2c..344d40e 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -536,14 +536,15 @@ config ARCH_IXP4XX config ARCH_DOVE bool "Marvell Dove" select CPU_V7 - select PCI select ARCH_REQUIRE_GPIOLIB select GENERIC_CLOCKEVENTS + select MIGHT_HAVE_PCI select NEED_MACH_IO_H select PLAT_ORION select CLKDEV_LOOKUP select NEED_MACH_MEMORY_H select COMMON_CLK + select USB_ARCH_HAS_EHCI help Support for the Marvell Dove SoC 88AP510 diff --git a/arch/arm/mach-dove/Makefile b/arch/arm/mach-dove/Makefile index 7d89269..b5a5773 100644 --- a/arch/arm/mach-dove/Makefile +++ b/arch/arm/mach-dove/Makefile @@ -1,4 +1,5 @@ -obj-y += common.o addr-map.o irq.o pcie.o mpp.o clock.o dump_cp15_regs.o +obj-y += common.o addr-map.o irq.o mpp.o clock.o dump_cp15_regs.o +obj-$(CONFIG_PCI) += pcie.o obj-$(CONFIG_MACH_DOVE_DB) += dove-db-setup.o obj-$(CONFIG_MACH_CM_A510) += cm-a510.o diff --git a/arch/arm/mach-dove/common.h b/arch/arm/mach-dove/common.h index 2ea271d..678ef0b 100644 --- a/arch/arm/mach-dove/common.h +++ b/arch/arm/mach-dove/common.h @@ -29,7 +29,11 @@ void dove_setup_cpu_mbus(void); void dove_ge00_init(struct mv643xx_eth_platform_data *eth_data); void dove_hwmon_init(void); void dove_sata_init(struct mv_sata_platform_data *sata_data); +#ifdef CONFIG_PCI void dove_pcie_init(int init_port0, int init_port1); +#else +static inline void dove_pcie_init(int init_port0, int init_port1) { } +#endif void dove_ehci0_init(void); void dove_ehci1_init(void); void dove_uart0_init(void);
Allow PCI support for Dove to be disabled. Some platforms do not have anything connected to the PCIe ports, so requiring PCI support to be built into the kernel just wastes space, and presents a lot more config options than are necessary. However, select USB_ARCH_HAS_EHCI so that we can still have EHCI support. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- arch/arm/Kconfig | 3 ++- arch/arm/mach-dove/Makefile | 3 ++- arch/arm/mach-dove/common.h | 4 ++++ 3 files changed, 8 insertions(+), 2 deletions(-)