diff mbox

[v2,1/5] ARM: Broadcom: Unconditionally build arch/arm/mach-bcm

Message ID 20130726151223.045835540@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Domenico Andreoli July 26, 2013, 2:56 p.m. UTC
From: Domenico Andreoli <domenico.andreoli@linux.com>

arch/arm/mach-bcm contains a plurality of Broadcom SoCs, each configured
separately. As a matter of flexibility and maintenance, it needs to be
always included in the build.

Cc: Christian Daudt <csd@broadcom.com>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Domenico Andreoli <domenico.andreoli@linux.com>
---
 arch/arm/Makefile |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jason Cooper July 26, 2013, 3:29 p.m. UTC | #1
On Fri, Jul 26, 2013 at 04:56:40PM +0200, Domenico Andreoli wrote:
> From: Domenico Andreoli <domenico.andreoli@linux.com>
> 
> arch/arm/mach-bcm contains a plurality of Broadcom SoCs, each configured
> separately. As a matter of flexibility and maintenance, it needs to be
> always included in the build.

So if I'm building mach-kirkwood, I _have_ to build Broadcom?  What is
the *specific* problem you're encountering that this solves?

Notice:

$ git grep '^machine-y'
$

thx,

Jason.

> Cc: Christian Daudt <csd@broadcom.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Domenico Andreoli <domenico.andreoli@linux.com>
> ---
>  arch/arm/Makefile |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: b/arch/arm/Makefile
> ===================================================================
> --- a/arch/arm/Makefile
> +++ b/arch/arm/Makefile
> @@ -145,7 +145,7 @@ textofs-$(CONFIG_ARCH_MSM8960) := 0x0020
>  # Machine directory name.  This list is sorted alphanumerically
>  # by CONFIG_* macro name.
>  machine-$(CONFIG_ARCH_AT91)		+= at91
> -machine-$(CONFIG_ARCH_BCM)		+= bcm
> +machine-y				+= bcm
>  machine-$(CONFIG_ARCH_BCM2835)		+= bcm2835
>  machine-$(CONFIG_ARCH_CLPS711X)		+= clps711x
>  machine-$(CONFIG_ARCH_CNS3XXX)		+= cns3xxx
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Christian Daudt July 26, 2013, 3:55 p.m. UTC | #2
[resend in plain text]
On 13-07-26 08:29 AM, Jason Cooper wrote:
> On Fri, Jul 26, 2013 at 04:56:40PM +0200, Domenico Andreoli wrote:
>> From: Domenico Andreoli<domenico.andreoli@linux.com>
>>
>> arch/arm/mach-bcm contains a plurality of Broadcom SoCs, each configured
>> separately. As a matter of flexibility and maintenance, it needs to be
>> always included in the build.
> So if I'm building mach-kirkwood, I _have_ to build Broadcom?  What is
> the *specific* problem you're encountering that this solves?
  No it won't, as the Makefile inside mach-bcm will only pull in files 
based on ARCH_ settings. This move is so that a number different 
Broadcom SoCs can co-exist inside the mach-bcm directory.
>> Cc: Christian Daudt<csd@broadcom.com>
>> Cc:linux-arm-kernel@lists.infradead.org
>> Signed-off-by: Domenico Andreoli<domenico.andreoli@linux.com>
>> ---
>>   arch/arm/Makefile |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Index: b/arch/arm/Makefile
>> ===================================================================
>> --- a/arch/arm/Makefile
>> +++ b/arch/arm/Makefile
>> @@ -145,7 +145,7 @@ textofs-$(CONFIG_ARCH_MSM8960) := 0x0020
>>   # Machine directory name.  This list is sorted alphanumerically
>>   # by CONFIG_* macro name.
>>   machine-$(CONFIG_ARCH_AT91)		+= at91
>> -machine-$(CONFIG_ARCH_BCM)		+= bcm
>> +machine-y				+= bcm
>>   machine-$(CONFIG_ARCH_BCM2835)		+= bcm2835
>>   machine-$(CONFIG_ARCH_CLPS711X)		+= clps711x
>>   machine-$(CONFIG_ARCH_CNS3XXX)		+= cns3xxx
>>
>>
This patch is already contained in Hauke's patchset from yesterday ( 
http://article.gmane.org/gmane.linux.ports.arm.kernel/254986) so you'll 
need to sync with him on it. In case yours goes in first:
Acked-by: Christian Daudt <csd@broadcom.com>

  Thanks,
    csd
Olof Johansson July 26, 2013, 5:24 p.m. UTC | #3
Hi,

On Fri, Jul 26, 2013 at 7:56 AM, Domenico Andreoli
<domenico.andreoli@linux.com> wrote:
> From: Domenico Andreoli <domenico.andreoli@linux.com>
>
> arch/arm/mach-bcm contains a plurality of Broadcom SoCs, each configured
> separately. As a matter of flexibility and maintenance, it needs to be
> always included in the build.
>
> Cc: Christian Daudt <csd@broadcom.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Domenico Andreoli <domenico.andreoli@linux.com>
> ---
>  arch/arm/Makefile |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: b/arch/arm/Makefile
> ===================================================================
> --- a/arch/arm/Makefile
> +++ b/arch/arm/Makefile
> @@ -145,7 +145,7 @@ textofs-$(CONFIG_ARCH_MSM8960) := 0x0020
>  # Machine directory name.  This list is sorted alphanumerically
>  # by CONFIG_* macro name.
>  machine-$(CONFIG_ARCH_AT91)            += at91
> -machine-$(CONFIG_ARCH_BCM)             += bcm
> +machine-y                              += bcm
>  machine-$(CONFIG_ARCH_BCM2835)         += bcm2835
>  machine-$(CONFIG_ARCH_CLPS711X)                += clps711x
>  machine-$(CONFIG_ARCH_CNS3XXX)         += cns3xxx

It's hard to tell what you're trying to do here, since the current
in-tree code works just fine and you're alluding to it having
problems. :)

Are you looking to add new selection of per-SoC config options? If so,
I propose keeping ARCH_BCM as a common, but silent, option that is
just selected by the other SoCs.  See how OMAP does it where
OMAP2/3/4/5 all select ARCH_OMAP2PLUS for comparison.


-Olof
Domenico Andreoli July 26, 2013, 9:59 p.m. UTC | #4
On Fri, Jul 26, 2013 at 11:29:18AM -0400, Jason Cooper wrote:
> On Fri, Jul 26, 2013 at 04:56:40PM +0200, Domenico Andreoli wrote:
> > From: Domenico Andreoli <domenico.andreoli@linux.com>
> > 
> > arch/arm/mach-bcm contains a plurality of Broadcom SoCs, each configured
> > separately. As a matter of flexibility and maintenance, it needs to be
> > always included in the build.
> 
> So if I'm building mach-kirkwood, I _have_ to build Broadcom?  What is
> the *specific* problem you're encountering that this solves?

In mach-bcm we (or I, it's not very clear to me) want to have support for
multiple SoCs.

In trying the approach

machine-$(CONFIG_ARCH_BCM)		+= bcm
machine-$(CONFIG_ARCH_BCM4760)		+= bcm

I got linker complains about multiple symbol definitiion in case both the
config options are selected.

The first thought was to use a common option which purpose was only to
include the subdir but then, given my allergy to the tons of config options
with usually not straghtforward purpose, I opted for something more simple.

> 
> Notice:
> 
> $ git grep '^machine-y'
> $
> 
> thx,
> 
> Jason.
> 
> > Cc: Christian Daudt <csd@broadcom.com>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Signed-off-by: Domenico Andreoli <domenico.andreoli@linux.com>
> > ---
> >  arch/arm/Makefile |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > Index: b/arch/arm/Makefile
> > ===================================================================
> > --- a/arch/arm/Makefile
> > +++ b/arch/arm/Makefile
> > @@ -145,7 +145,7 @@ textofs-$(CONFIG_ARCH_MSM8960) := 0x0020
> >  # Machine directory name.  This list is sorted alphanumerically
> >  # by CONFIG_* macro name.
> >  machine-$(CONFIG_ARCH_AT91)		+= at91
> > -machine-$(CONFIG_ARCH_BCM)		+= bcm
> > +machine-y				+= bcm
> >  machine-$(CONFIG_ARCH_BCM2835)		+= bcm2835
> >  machine-$(CONFIG_ARCH_CLPS711X)		+= clps711x
> >  machine-$(CONFIG_ARCH_CNS3XXX)		+= cns3xxx
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Jason Gunthorpe July 26, 2013, 11:11 p.m. UTC | #5
On Fri, Jul 26, 2013 at 11:59:00PM +0200, Domenico Andreoli wrote:

> In mach-bcm we (or I, it's not very clear to me) want to have support for
> multiple SoCs.
> 
> In trying the approach
> 
> machine-$(CONFIG_ARCH_BCM)		+= bcm
> machine-$(CONFIG_ARCH_BCM4760)		+= bcm
> 
> I got linker complains about multiple symbol definitiion in case both the
> config options are selected.

You can't repeat the same dir. Maybe this?

bcm-machine-$(CONFIG_ARCH_BCM) := bmc
bcm-machine-$(CONFIG_ARCH_BCM4760) := bmc
machine-y += $(bcm-machine-y)

Regards,
Jason
Domenico Andreoli July 26, 2013, 11:28 p.m. UTC | #6
On Fri, Jul 26, 2013 at 05:11:08PM -0600, Jason Gunthorpe wrote:
> On Fri, Jul 26, 2013 at 11:59:00PM +0200, Domenico Andreoli wrote:
> 
> > In mach-bcm we (or I, it's not very clear to me) want to have support for
> > multiple SoCs.
> > 
> > In trying the approach
> > 
> > machine-$(CONFIG_ARCH_BCM)		+= bcm
> > machine-$(CONFIG_ARCH_BCM4760)		+= bcm
> > 
> > I got linker complains about multiple symbol definitiion in case both the
> > config options are selected.
> 
> You can't repeat the same dir. Maybe this?
> 
> bcm-machine-$(CONFIG_ARCH_BCM) := bmc
> bcm-machine-$(CONFIG_ARCH_BCM4760) := bmc
> machine-y += $(bcm-machine-y)

nice! I prefer this to the config option used only to descend the dir,
you never know how it is going to be abused.

Christian, would you agree in ditching ARCH_BROADCOM then?

Russel, is it ok?

thanks,
Domenico
Russell King - ARM Linux July 26, 2013, 11:42 p.m. UTC | #7
On Fri, Jul 26, 2013 at 05:11:08PM -0600, Jason Gunthorpe wrote:
> On Fri, Jul 26, 2013 at 11:59:00PM +0200, Domenico Andreoli wrote:
> 
> > In mach-bcm we (or I, it's not very clear to me) want to have support for
> > multiple SoCs.
> > 
> > In trying the approach
> > 
> > machine-$(CONFIG_ARCH_BCM)		+= bcm
> > machine-$(CONFIG_ARCH_BCM4760)		+= bcm
> > 
> > I got linker complains about multiple symbol definitiion in case both the
> > config options are selected.
> 
> You can't repeat the same dir. Maybe this?
> 
> bcm-machine-$(CONFIG_ARCH_BCM) := bmc
> bcm-machine-$(CONFIG_ARCH_BCM4760) := bmc
> machine-y += $(bcm-machine-y)

Thank you for making me look at that file and see how people fail to read
my comments about keeping stuff appropriately sorted.  Really makes me
wonder why I bother.

Anyway, there's a simpler solutions to this:

machdirs := $(patsubst %,arch/arm/mach-%/,$(sort $(machine-y)))
platdirs := $(patsubst %,arch/arm/plat-%/,$(Sort $(plat-y)))

which will remove all duplicates.
Christian Daudt July 26, 2013, 11:55 p.m. UTC | #8
On 13-07-26 04:28 PM, Domenico Andreoli wrote:
> On Fri, Jul 26, 2013 at 05:11:08PM -0600, Jason Gunthorpe wrote:
>> On Fri, Jul 26, 2013 at 11:59:00PM +0200, Domenico Andreoli wrote:
>>
>>> In mach-bcm we (or I, it's not very clear to me) want to have support for
>>> multiple SoCs.
>>>
>>> In trying the approach
>>>
>>> machine-$(CONFIG_ARCH_BCM)		+= bcm
>>> machine-$(CONFIG_ARCH_BCM4760)		+= bcm
>>>
>>> I got linker complains about multiple symbol definitiion in case both the
>>> config options are selected.
>> You can't repeat the same dir. Maybe this?
>>
>> bcm-machine-$(CONFIG_ARCH_BCM) := bmc
>> bcm-machine-$(CONFIG_ARCH_BCM4760) := bmc
>> machine-y += $(bcm-machine-y)
> nice! I prefer this to the config option used only to descend the dir,
> you never know how it is going to be abused.
>
> Christian, would you agree in ditching ARCH_BROADCOM then?
I'm fine with skipping ARCH_BROADCOM
  Thanks,
    csd
Olof Johansson July 27, 2013, 12:01 a.m. UTC | #9
On Fri, Jul 26, 2013 at 4:42 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> Thank you for making me look at that file and see how people fail to read
> my comments about keeping stuff appropriately sorted.  Really makes me
> wonder why I bother.

Grmbl. This one escaped me probably because the comment is at the top,
so I didn't see it when just reviewing the patch. I should obviously
have caught it anyway since the list was so obviously sorted before.
My bad. I'll send a patch in a bit.


-Olof
Russell King - ARM Linux July 27, 2013, 12:04 a.m. UTC | #10
On Fri, Jul 26, 2013 at 05:01:08PM -0700, Olof Johansson wrote:
> On Fri, Jul 26, 2013 at 4:42 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > Thank you for making me look at that file and see how people fail to read
> > my comments about keeping stuff appropriately sorted.  Really makes me
> > wonder why I bother.
> 
> Grmbl. This one escaped me probably because the comment is at the top,
> so I didn't see it when just reviewing the patch. I should obviously
> have caught it anyway since the list was so obviously sorted before.
> My bad. I'll send a patch in a bit.

I've already a patch in my fixes.  It'll appear in -next probably after
the weekend (as next doesn't update over weekends.)

Note that the misordering is not due to these patches, but other previous
patches.
Olof Johansson July 27, 2013, 12:05 a.m. UTC | #11
On Fri, Jul 26, 2013 at 5:04 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Jul 26, 2013 at 05:01:08PM -0700, Olof Johansson wrote:
>> On Fri, Jul 26, 2013 at 4:42 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > Thank you for making me look at that file and see how people fail to read
>> > my comments about keeping stuff appropriately sorted.  Really makes me
>> > wonder why I bother.
>>
>> Grmbl. This one escaped me probably because the comment is at the top,
>> so I didn't see it when just reviewing the patch. I should obviously
>> have caught it anyway since the list was so obviously sorted before.
>> My bad. I'll send a patch in a bit.
>
> I've already a patch in my fixes.  It'll appear in -next probably after
> the weekend (as next doesn't update over weekends.)
>
> Note that the misordering is not due to these patches, but other previous
> patches.

Ok, sounds good.

And yeah, it's pretty obvious based on the order that it has crept in over time.


-Olof
Arnd Bergmann July 27, 2013, 2:38 p.m. UTC | #12
On Saturday 27 July 2013, Russell King - ARM Linux wrote:
> On Fri, Jul 26, 2013 at 05:11:08PM -0600, Jason Gunthorpe wrote:
> > On Fri, Jul 26, 2013 at 11:59:00PM +0200, Domenico Andreoli wrote:
>
> Anyway, there's a simpler solutions to this:
> 
> machdirs := $(patsubst %,arch/arm/mach-%/,$(sort $(machine-y)))
> platdirs := $(patsubst %,arch/arm/plat-%/,$(Sort $(plat-y)))
> 
> which will remove all duplicates.

This does sound like the easiest solution, although I still think
Olof's suggestion of making ARCH_BCM a silent Kconfig symbol would be
just as good if we just have this one instance.

	Arnd
Florian Fainelli Aug. 1, 2013, 9:23 a.m. UTC | #13
Hello,

2013/7/26 Domenico Andreoli <cavokz@gmail.com>:
> On Fri, Jul 26, 2013 at 11:29:18AM -0400, Jason Cooper wrote:
>> On Fri, Jul 26, 2013 at 04:56:40PM +0200, Domenico Andreoli wrote:
>> > From: Domenico Andreoli <domenico.andreoli@linux.com>
>> >
>> > arch/arm/mach-bcm contains a plurality of Broadcom SoCs, each configured
>> > separately. As a matter of flexibility and maintenance, it needs to be
>> > always included in the build.
>>
>> So if I'm building mach-kirkwood, I _have_ to build Broadcom?  What is
>> the *specific* problem you're encountering that this solves?
>
> In mach-bcm we (or I, it's not very clear to me) want to have support for
> multiple SoCs.
>
> In trying the approach
>
> machine-$(CONFIG_ARCH_BCM)              += bcm
> machine-$(CONFIG_ARCH_BCM4760)          += bcm
>
> I got linker complains about multiple symbol definitiion in case both the
> config options are selected.
>
> The first thought was to use a common option which purpose was only to
> include the subdir but then, given my allergy to the tons of config options
> with usually not straghtforward purpose, I opted for something more simple.

I do not understand why are you trying so hard to put your SoC support
in mach-bcm. I was one of the only people to complain that mach-bcm
was both confusing and not generic enough to cover all Broadcom SoCs.
I still think it should have been specified to mach-bcmmobile or
something like mach-bcm28xxx. Back in the days where ARM drivers were
mostly living in arch/arm/*, it *might* have made some sense but now,
I really think that you should go with your own mach-bcm470x
directory.

BCM47060, BCM53xx and BCM28xx all have both different CPU backends and
different on-chip peripherals, which are even connected differently,
put clearly, they share very little but the ARM architecture.
Domenico Andreoli Aug. 1, 2013, 2:15 p.m. UTC | #14
On Thu, Aug 01, 2013 at 10:23:48AM +0100, Florian Fainelli wrote:
> Hello,
> 
> 2013/7/26 Domenico Andreoli <cavokz@gmail.com>:
> > On Fri, Jul 26, 2013 at 11:29:18AM -0400, Jason Cooper wrote:
> >> On Fri, Jul 26, 2013 at 04:56:40PM +0200, Domenico Andreoli wrote:
> >> > From: Domenico Andreoli <domenico.andreoli@linux.com>
> >> >
> >> > arch/arm/mach-bcm contains a plurality of Broadcom SoCs, each configured
> >> > separately. As a matter of flexibility and maintenance, it needs to be
> >> > always included in the build.
> >>
> >> So if I'm building mach-kirkwood, I _have_ to build Broadcom?  What is
> >> the *specific* problem you're encountering that this solves?
> >
> > In mach-bcm we (or I, it's not very clear to me) want to have support for
> > multiple SoCs.
> >
> > In trying the approach
> >
> > machine-$(CONFIG_ARCH_BCM)              += bcm
> > machine-$(CONFIG_ARCH_BCM4760)          += bcm
> >
> > I got linker complains about multiple symbol definitiion in case both the
> > config options are selected.
> >
> > The first thought was to use a common option which purpose was only to
> > include the subdir but then, given my allergy to the tons of config options
> > with usually not straghtforward purpose, I opted for something more simple.
> 
> I do not understand why are you trying so hard to put your SoC support
> in mach-bcm. I was one of the only people to complain that mach-bcm
> was both confusing and not generic enough to cover all Broadcom SoCs.
> I still think it should have been specified to mach-bcmmobile or
> something like mach-bcm28xxx. Back in the days where ARM drivers were
> mostly living in arch/arm/*, it *might* have made some sense but now,
> I really think that you should go with your own mach-bcm470x
> directory.
> 
> BCM47060, BCM53xx and BCM28xx all have both different CPU backends and
> different on-chip peripherals, which are even connected differently,
> put clearly, they share very little but the ARM architecture.

I've already explained my point elsewhere in this thread. It's not technical,
it's social. I don't see any technical disproportion to go either way.

thanks,
Domenico
Domenico Andreoli Aug. 1, 2013, 2:18 p.m. UTC | #15
On Sat, Jul 27, 2013 at 04:38:16PM +0200, Arnd Bergmann wrote:
> On Saturday 27 July 2013, Russell King - ARM Linux wrote:
> > On Fri, Jul 26, 2013 at 05:11:08PM -0600, Jason Gunthorpe wrote:
> > > On Fri, Jul 26, 2013 at 11:59:00PM +0200, Domenico Andreoli wrote:
> >
> > Anyway, there's a simpler solutions to this:
> > 
> > machdirs := $(patsubst %,arch/arm/mach-%/,$(sort $(machine-y)))
> > platdirs := $(patsubst %,arch/arm/plat-%/,$(Sort $(plat-y)))
> > 
> > which will remove all duplicates.
> 
> This does sound like the easiest solution, although I still think
> Olof's suggestion of making ARCH_BCM a silent Kconfig symbol would be
> just as good if we just have this one instance.

I know that time is always tight and there is also this DT ABI discussion
ongoing but do I have any chance to get any ack on this series?

Should I post a new set? It would be only to fix again the build of mach-bcm,
I've nothing else pending.

Thanks,
Domenico
diff mbox

Patch

Index: b/arch/arm/Makefile
===================================================================
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -145,7 +145,7 @@  textofs-$(CONFIG_ARCH_MSM8960) := 0x0020
 # Machine directory name.  This list is sorted alphanumerically
 # by CONFIG_* macro name.
 machine-$(CONFIG_ARCH_AT91)		+= at91
-machine-$(CONFIG_ARCH_BCM)		+= bcm
+machine-y				+= bcm
 machine-$(CONFIG_ARCH_BCM2835)		+= bcm2835
 machine-$(CONFIG_ARCH_CLPS711X)		+= clps711x
 machine-$(CONFIG_ARCH_CNS3XXX)		+= cns3xxx