Re: [PATCH 00/12] Thumb-2 kernel support

Re: [PATCH 00/12] Thumb-2 kernel support

[Andrew Morton]

On Fri, 06 Jun 2008 18:33:00 +0100
Catalin Marinas <catalin.marinas@arm.com> wrote:

> This series of patches allows the kernel to be compiled to Thumb-2 mode
> (on ARMv7 CPUs). The patches have been posted a few times on the list
> and the comments were implemented (hopefully I haven't missed any).
> 
> If/when there are no more comments, I'd like the series to be merged
> into the -mm tree if Andrew agrees. It was tested and generated against
> the 2.6.26-rc2-mm1 kernel.
> 
> It is also available via Git at git://linux-arm.org/linux-2.6 for-akpm.
> 
> Thanks.
> 
> 
> Catalin Marinas (12):
>       Thumb-2: Add Thumb-2 support to the build files
>       Thumb-2: Handle the exceptions in Thumb mode on noMMU kernels
>       Thumb-2: Implement the unified support for the RealView boards
>       Thumb-2: Implement the unified support for the Integrator boards
>       Thumb-2: Implement the unified boot code
>       Thumb-2: Implement the unified VFP support
>       Thumb-2: Implement unified locking support
>       Thumb-2: Implement the unified arch/arm/lib functions
>       Thumb-2: Implement the unified arch/arm/mm support
>       Thumb-2: Implementation of the unified start-up and exceptions code
>       Thumb-2: Add macros for the unified assembler syntax
>       Thumb-2: Add the ENDPROC declarations to the .S files

OK, I added this tree to -mm.  Nobody is likely to test or review it
there, but at least I might pick up on any merge or build issues.

If you're hankering for a 2.6.27 merge of this material it might be
more approprite to add it to linux-next.

Re: [PATCH 00/12] Thumb-2 kernel support

[Catalin Marinas]

On Mon, 2008-06-30 at 12:51 -0700, Andrew Morton wrote:
> On Fri, 06 Jun 2008 18:33:00 +0100
> Catalin Marinas <catalin.marinas@arm.com> wrote:
> 
> > This series of patches allows the kernel to be compiled to Thumb-2 mode
> > (on ARMv7 CPUs). The patches have been posted a few times on the list
> > and the comments were implemented (hopefully I haven't missed any).
> > 
> > If/when there are no more comments, I'd like the series to be merged
> > into the -mm tree if Andrew agrees. It was tested and generated against
> > the 2.6.26-rc2-mm1 kernel.
[...]
> OK, I added this tree to -mm.  Nobody is likely to test or review it
> there, but at least I might pick up on any merge or build issues.

Thanks.

> If you're hankering for a 2.6.27 merge of this material it might be
> more appropriate to add it to linux-next.

I aim for the post 2.6.27 merge window (i.e. patches available in
2.6.28).

But isn't linux-next targeted to subsystem maintainers only (i.e. get
them merged via RMK's trees)? Or an "Acked-by: RMK" would be enough.

-- 
Catalin

Re: [PATCH 00/12] Thumb-2 kernel support

[Andrew Morton]

On Tue, 01 Jul 2008 09:32:11 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:

> On Mon, 2008-06-30 at 12:51 -0700, Andrew Morton wrote:
> > On Fri, 06 Jun 2008 18:33:00 +0100
> > Catalin Marinas <catalin.marinas@arm.com> wrote:
> > 
> > > This series of patches allows the kernel to be compiled to Thumb-2 mode
> > > (on ARMv7 CPUs). The patches have been posted a few times on the list
> > > and the comments were implemented (hopefully I haven't missed any).
> > > 
> > > If/when there are no more comments, I'd like the series to be merged
> > > into the -mm tree if Andrew agrees. It was tested and generated against
> > > the 2.6.26-rc2-mm1 kernel.
> [...]
> > OK, I added this tree to -mm.  Nobody is likely to test or review it
> > there, but at least I might pick up on any merge or build issues.
> 
> Thanks.
> 
> > If you're hankering for a 2.6.27 merge of this material it might be
> > more appropriate to add it to linux-next.
> 
> I aim for the ___post 2.6.27 merge window (i.e. patches available in
> 2.6.28).

OK, then in that case it shouldn't go into linux-next until around
2.6.27-rc1 time.


> But isn't linux-next targeted to subsystem maintainers only (i.e. get
> them merged via RMK's trees)? Or an "Acked-by: RMK" would be enough.

An ack is always nice.  But assuming there aren't any roadblocks
looming, getting it into linux-next under the assumption that the code
will be ready to go by the time 2.6.28 opens would make sense.

Re: [PATCH 00/12] Thumb-2 kernel support

[Catalin Marinas]

Hi Andrew,

On Tue, 2008-07-01 at 01:45 -0700, Andrew Morton wrote:
> On Tue, 01 Jul 2008 09:32:11 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:
> 
> > On Mon, 2008-06-30 at 12:51 -0700, Andrew Morton wrote:
> > > On Fri, 06 Jun 2008 18:33:00 +0100
> > > Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > 
> > > > This series of patches allows the kernel to be compiled to Thumb-2 mode
> > > > (on ARMv7 CPUs). The patches have been posted a few times on the list
> > > > and the comments were implemented (hopefully I haven't missed any).
> > > > 
> > > > If/when there are no more comments, I'd like the series to be merged
> > > > into the -mm tree if Andrew agrees. It was tested and generated against
> > > > the 2.6.26-rc2-mm1 kernel.
> > [...]
> > > OK, I added this tree to -mm.  Nobody is likely to test or review it
> > > there, but at least I might pick up on any merge or build issues.

Were the Thumb-2 patches merged in any of the -mm tree releases? I now
updated the series to the 2.6.26-rc8-mm1 kernel if you still consider
merging them (git://linux-arm.org/linux-2.6 for-akpm).

> > > If you're hankering for a 2.6.27 merge of this material it might be
> > > more appropriate to add it to linux-next.
> > 
> > I aim for the ___post 2.6.27 merge window (i.e. patches available in
> > 2.6.28).
> 
> OK, then in that case it shouldn't go into linux-next until around
> 2.6.27-rc1 time.

Russell (and others in the ARM community), are you OK with this set of
patches being merged into 2.6.28 mainline (i.e. at the next merging
window)? If yes, do you acknowledge the patches? In the meantime, I can
rebase them on top of linux-next to check for possible merge conflicts
(or even ask for them to be pulled into linux-next).

Thanks.

-- 
Catalin

Re: [PATCH 00/12] Thumb-2 kernel support

[Andrew Morton]

On Wed, 23 Jul 2008 17:05:55 +0100
Catalin Marinas <catalin.marinas@arm.com> wrote:

> Hi Andrew,
> 
> On Tue, 2008-07-01 at 01:45 -0700, Andrew Morton wrote:
> > On Tue, 01 Jul 2008 09:32:11 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:
> > 
> > > On Mon, 2008-06-30 at 12:51 -0700, Andrew Morton wrote:
> > > > On Fri, 06 Jun 2008 18:33:00 +0100
> > > > Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > > 
> > > > > This series of patches allows the kernel to be compiled to Thumb-2 mode
> > > > > (on ARMv7 CPUs). The patches have been posted a few times on the list
> > > > > and the comments were implemented (hopefully I haven't missed any).
> > > > > 
> > > > > If/when there are no more comments, I'd like the series to be merged
> > > > > into the -mm tree if Andrew agrees. It was tested and generated against
> > > > > the 2.6.26-rc2-mm1 kernel.
> > > [...]
> > > > OK, I added this tree to -mm.  Nobody is likely to test or review it
> > > > there, but at least I might pick up on any merge or build issues.
> 
> Were the Thumb-2 patches merged in any of the -mm tree releases? I now
> updated the series to the 2.6.26-rc8-mm1 kernel if you still consider
> merging them (___git://linux-arm.org/linux-2.6 for-akpm).

I pull it regularly but always get rejects and never got around to
looking into fixing them.  Probably the git-thumb tree is based on an
ARM git tree so I'd need to generate the diff reletive to that tree. 
But I just haven't got onto it, sorry.

> > > > If you're hankering for a 2.6.27 merge of this material it might be
> > > > more appropriate to add it to linux-next.
> > > 
> > > I aim for the ___post 2.6.27 merge window (i.e. patches available in
> > > 2.6.28).
> > 
> > OK, then in that case it shouldn't go into linux-next until around
> > 2.6.27-rc1 time.
> 
> Russell (and others in the ARM community), are you OK with this set of
> patches being merged into 2.6.28 mainline (i.e. at the next merging
> window)? If yes, do you acknowledge the patches? In the meantime, I can
> rebase them on top of linux-next to check for possible merge conflicts
> (or even ask for them to be pulled into linux-next).

yup, we should maintain the tree in linux-next if it's for 2.6.28.

Re: [PATCH 00/12] Thumb-2 kernel support

[Catalin Marinas]

On Wed, 2008-07-23 at 13:01 -0700, Andrew Morton wrote:
> On Wed, 23 Jul 2008 17:05:55 +0100
> Catalin Marinas <catalin.marinas@arm.com> wrote:
> > Were the Thumb-2 patches merged in any of the -mm tree releases? I now
> > updated the series to the 2.6.26-rc8-mm1 kernel if you still consider
> > merging them (___git://linux-arm.org/linux-2.6 for-akpm).
> 
> I pull it regularly but always get rejects and never got around to
> looking into fixing them.  Probably the git-thumb tree is based on an
> ARM git tree so I'd need to generate the diff reletive to that tree. 
> But I just haven't got onto it, sorry.

I only based the for-akpm branch on your latest -mm tree release but
since you are rebasing your tree with every release, the base of my tree
(currently 2.6.26-rc8-mm1) is probably not an ancestor of your
development branch, hence the merging errors.

To minimise the conflicts, what commit or tag id do you base your
current development branch on?

> > Russell (and others in the ARM community), are you OK with this set of
> > patches being merged into 2.6.28 mainline (i.e. at the next merging
> > window)? If yes, do you acknowledge the patches? In the meantime, I can
> > rebase them on top of linux-next to check for possible merge conflicts
> > (or even ask for them to be pulled into linux-next).
> 
> yup, we should maintain the tree in linux-next if it's for 2.6.28.

I'm waiting for Russell to ack the patches first.

Thanks.

-- 
Catalin

Re: [PATCH 00/12] Thumb-2 kernel support

[Andrew Morton]

On Wed, 23 Jul 2008 22:10:22 +0100
Catalin Marinas <catalin.marinas@arm.com> wrote:

> On Wed, 2008-07-23 at 13:01 -0700, Andrew Morton wrote:
> > On Wed, 23 Jul 2008 17:05:55 +0100
> > Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > Were the Thumb-2 patches merged in any of the -mm tree releases? I now
> > > updated the series to the 2.6.26-rc8-mm1 kernel if you still consider
> > > merging them (___git://linux-arm.org/linux-2.6 for-akpm).
> > 
> > I pull it regularly but always get rejects and never got around to
> > looking into fixing them.  Probably the git-thumb tree is based on an
> > ARM git tree so I'd need to generate the diff reletive to that tree. 
> > But I just haven't got onto it, sorry.
> 
> I only based the for-akpm branch on your latest -mm tree release but
> since you are rebasing your tree with every release, the base of my tree
> (currently 2.6.26-rc8-mm1) is probably not an ancestor of your
> development branch, hence the merging errors.
> 
> To minimise the conflicts, what commit or tag id do you base your
> current development branch on?

err, it's not that simple.  I just maintain plain old patches against
linux-next.

Most git trees are based on current Linus mainline.

> > > Russell (and others in the ARM community), are you OK with this set of
> > > patches being merged into 2.6.28 mainline (i.e. at the next merging
> > > window)? If yes, do you acknowledge the patches? In the meantime, I can
> > > rebase them on top of linux-next to check for possible merge conflicts
> > > (or even ask for them to be pulled into linux-next).
> > 
> > yup, we should maintain the tree in linux-next if it's for 2.6.28.
> 
> I'm waiting for Russell to ack the patches first.
> 

Well.  Rather than doing things sequentially we could go parallel. 
Russell could say "I'll look at them before 2.6.28 but please put them
into linux-next meanwhile".

Re: [PATCH 00/12] Thumb-2 kernel support

[Catalin Marinas]

On Wed, 2008-07-23 at 15:10 -0700, Andrew Morton wrote:
> On Wed, 23 Jul 2008 22:10:22 +0100
> Catalin Marinas <catalin.marinas@arm.com> wrote:
> > To minimise the conflicts, what commit or tag id do you base your
> > current development branch on?
> 
> err, it's not that simple.  I just maintain plain old patches against
> linux-next.
> 
> Most git trees are based on current Linus mainline.

OK, it makes sense. I rebased the for-akpm branch onto Linus' latest git
tree.

-- 
Catalin

Re: [PATCH 00/12] Thumb-2 kernel support

[Russell King - ARM Linux]

On Wed, Jul 23, 2008 at 03:10:45PM -0700, Andrew Morton wrote:
> Well.  Rather than doing things sequentially we could go parallel. 
> Russell could say "I'll look at them before 2.6.28 but please put them
> into linux-next meanwhile".

There have been some valid but (iirc) non-vocal objections to the
Thumb-2 support from a few people.  The biggest one is all the
mess associated with supporting this "unified" assembler stuff.

My view is that I really don't like these patches; they make the
code very unreadable and more prone to errors.  They're going to
cause us lots of problems in the future.  Every git merge which
touches any ARM file containing assembly is going to have to be
_very_ carefully reviewed, and it's not always possible to catch
all of those.

The only suggestion I have to improving the situation is to recommend
that someone rethinks their wizzy new assembly language format - which
IMHO is currently only fit for "write once, read or modify never"
assembly code.

Re: [PATCH 00/12] Thumb-2 kernel support

[Catalin Marinas]

On Sun, 2008-07-27 at 12:08 +0100, Russell King - ARM Linux wrote:
> On Wed, Jul 23, 2008 at 03:10:45PM -0700, Andrew Morton wrote:
> > Well.  Rather than doing things sequentially we could go parallel. 
> > Russell could say "I'll look at them before 2.6.28 but please put them
> > into linux-next meanwhile".
> 
> There have been some valid but (iirc) non-vocal objections to the
> Thumb-2 support from a few people.  The biggest one is all the
> mess associated with supporting this "unified" assembler stuff.

My first implementation of these patches (last year) created a separate
arch/ directory and a lot of people objected to this recommending to
merge it into the existing arch/arm. Once I posted the re-worked
implementation there were no big .

I agree that the syntax isn't as readable as before but this is mainly
because we need to support older binutils via conditional compilation
(the unified assembly syntax itself isn't as bad). IIRC, some people
were OK with this as long as, at some point, the conditional compilation
can be removed once most of the binutils in use support it (I think this
syntax is already 2 years old).

If you don't like the unified assembly syntax at all, I can't help much
(not even argue whether it makes sense or not, I wasn't involved in
designing it and it's pretty late to change it now anyway). What I can
do is try to make the Thumb-2 patches cleaner following comments I get
on the list (and assuming that I get comments more often than twice a
year :-)).

IMHO, I don't think the assembly syntax for an architecture is a valid
reason to reject patches. But if you really want to keep arch/arm clean,
that's fine with me and I can duplicate parts of it into arch/arm-t2
(with many files compiled directly from arch/arm as I don't want to
duplicate the arch/arm/mach-* stuff). This would only support ARMv7
onwards where Thumb-2 is mandated and use fairly recent binutils. Since
you don't like the unified assembly syntax you probably won't like to
maintain arch/arm-t2 either and this is fine as well, I can do it (the
code base would be relatively small anyway, it's mainly the assembly
files and some inline assembly in headers).

> My view is that I really don't like these patches; they make the
> code very unreadable and more prone to errors.  They're going to
> cause us lots of problems in the future.  Every git merge which
> touches any ARM file containing assembly is going to have to be
> _very_ carefully reviewed, and it's not always possible to catch
> all of those.
> 
> The only suggestion I have to improving the situation is to recommend
> that someone rethinks their wizzy new assembly language format - which
> IMHO is currently only fit for "write once, read or modify never"
> assembly code.

There were some public discussions in the past with Richard Earnshaw
(gcc/binutils) here in ARM regarding the assembly syntax but I don't
think it can be changed in a fully backwards compatible way while
supporting a slightly different instruction set.

I would really like some clear statement from you to know where to
focus my attention (clean up the current patches even more or create
separate arch/arm-t2). AFAICT, you don't really like these patches
merged, in which case I'd have to focus on the second option.

Thanks.

-- 
Catalin

Re: [PATCH 00/12] Thumb-2 kernel support

[Catalin Marinas]

On Mon, 2008-07-28 at 12:45 +0100, Catalin Marinas wrote:
> On Sun, 2008-07-27 at 12:08 +0100, Russell King - ARM Linux wrote:
> > On Wed, Jul 23, 2008 at 03:10:45PM -0700, Andrew Morton wrote:
> > > Well.  Rather than doing things sequentially we could go parallel. 
> > > Russell could say "I'll look at them before 2.6.28 but please put them
> > > into linux-next meanwhile".
> > 
> > There have been some valid but (iirc) non-vocal objections to the
> > Thumb-2 support from a few people.  The biggest one is all the
> > mess associated with supporting this "unified" assembler stuff.
> 
> My first implementation of these patches (last year) created a separate
> arch/ directory and a lot of people objected to this recommending to
> merge it into the existing arch/arm. Once I posted the re-worked
> implementation there were no big .

It looks like I didn't finish the above phrase - I meant no big
objections to the conditional compilation or someone stating clearly
that this is unacceptable.

-- 
Catalin

Re: [PATCH 00/12] Thumb-2 kernel support

[Russell King - ARM Linux]

On Mon, Jul 28, 2008 at 12:45:11PM +0100, Catalin Marinas wrote:
> On Sun, 2008-07-27 at 12:08 +0100, Russell King - ARM Linux wrote:
> > On Wed, Jul 23, 2008 at 03:10:45PM -0700, Andrew Morton wrote:
> > > Well.  Rather than doing things sequentially we could go parallel. 
> > > Russell could say "I'll look at them before 2.6.28 but please put them
> > > into linux-next meanwhile".
> > 
> > There have been some valid but (iirc) non-vocal objections to the
> > Thumb-2 support from a few people.  The biggest one is all the
> > mess associated with supporting this "unified" assembler stuff.
> 
> My first implementation of these patches (last year) created a separate
> arch/ directory and a lot of people objected to this recommending to
> merge it into the existing arch/arm. Once I posted the re-worked
> implementation there were no big .

I'm not surprised.  People don't want to duplicate their machine support
across two arch/arm for the same family of SoCs.

> I agree that the syntax isn't as readable as before but this is mainly
> because we need to support older binutils via conditional compilation
> (the unified assembly syntax itself isn't as bad). IIRC, some people
> were OK with this as long as, at some point, the conditional compilation
> can be removed once most of the binutils in use support it (I think this
> syntax is already 2 years old).

How is that realistically achievable given the issues that I've raised
over the impossibility of review and the issues with gcc not knowing
how many instructions an asm() statement generates?

> If you don't like the unified assembly syntax at all, I can't help much
> (not even argue whether it makes sense or not, I wasn't involved in
> designing it and it's pretty late to change it now anyway).

If neither of us can "help much", then T2 won't be going in, and ARM
Ltd is going to have to accept that T2 isn't going to be supported by
mainline Linux.

However, there is another solution to this: ARM Ltd could write a
static checker to ensure that the 'itxxxx' instructions match the
conditions in the following instructions, resulting in disagreements
between them being spotted at build time, rather than having some
data corrupting bug lurking around.

I totally disagree with getting rid of those conditional suffixes
even for T2 only code - doing so will make my objection even stronger
against T2.  Rather than getting rid of the conditional suffixes, use
them as a continuing existing check that the 'itxxxx' instructions are
correct.

Then we can pick up on things like merge errors, bad patches, wrong
'itxxxx' covering the following instructions, etc.

I think I've proven my point well - at least one of those 'itxxxx'
instructions in your patch is wrong, but not obviously wrong from
reading the patches.  That's only one which looked at how the
resulting code actually turns out.  I didn't thoroughly check the
others, so there could be other issues lurking in the missing
patch context lines.

> IMHO, I don't think the assembly syntax for an architecture is a valid
> reason to reject patches.

My objection is primerily on the readability of the code in the ARM
kernel, and the viability of continued maintainence of it - which is
an extremely important consideration and one which trumps *everything*
else - and it's that basis which I'm rejecting the patches.

That objection is directly caused by this crappy assembly syntax.  So
you can turn it into something political by saying that it's the
assembly syntax I'm objecting to.  My objection is _far_ more than
that.

> > My view is that I really don't like these patches; they make the
> > code very unreadable and more prone to errors.  They're going to
> > cause us lots of problems in the future.  Every git merge which
> > touches any ARM file containing assembly is going to have to be
> > _very_ carefully reviewed, and it's not always possible to catch
> > all of those.
> > 
> > The only suggestion I have to improving the situation is to recommend
> > that someone rethinks their wizzy new assembly language format - which
> > IMHO is currently only fit for "write once, read or modify never"
> > assembly code.
> 
> There were some public discussions in the past with Richard Earnshaw
> (gcc/binutils) here in ARM regarding the assembly syntax but I don't
> think it can be changed in a fully backwards compatible way while
> supporting a slightly different instruction set.

Were there?  That's the first I've heard of them!  The use of "public"
here is actually rather annoying.  They may have been "public" from the
point of view of being on some random mailing list on the Internet, but
that doesn't mean that everyone concerned knows that they're going on.

And no one person can have a finger in every pie, or be subscribed to
every mailing list on the planet and be able to track what's going on.

I don't consider the discussions here as "public".  They're "public" in
the sense of addressing this community, but if we were to talk about
changing the glibc<->kernel interface here only, and then claim to the
glibc people "but the discussions were public" they'd laugh at us.

> ???I would really like some clear statement from you to know where to
> focus my attention (clean up the current patches even more or create
> separate arch/arm-t2). AFAICT, you don't really like these patches
> merged, in which case I'd have to focus on the second option.

I've put my point of view forward in the responses to the patches.
I'm not sure what "clear statement" you're really after.

Also, I'm not entirely sure why there's a body of people who grumble
about this stuff, but don't post their concerns...

Maybe if that ARM Linux developers conference had gone ahead, this
could have been discussed there in a broader manner and some way
forward could have been reached.

Re: [PATCH 00/12] Thumb-2 kernel support

[Russell King - ARM Linux]

On Mon, Jul 28, 2008 at 01:40:55PM +0100, Catalin Marinas wrote:
> On Mon, 2008-07-28 at 12:45 +0100, Catalin Marinas wrote:
> > On Sun, 2008-07-27 at 12:08 +0100, Russell King - ARM Linux wrote:
> > > On Wed, Jul 23, 2008 at 03:10:45PM -0700, Andrew Morton wrote:
> > > > Well.  Rather than doing things sequentially we could go parallel. 
> > > > Russell could say "I'll look at them before 2.6.28 but please put them
> > > > into linux-next meanwhile".
> > > 
> > > There have been some valid but (iirc) non-vocal objections to the
> > > Thumb-2 support from a few people.  The biggest one is all the
> > > mess associated with supporting this "unified" assembler stuff.
> > 
> > My first implementation of these patches (last year) created a separate
> > arch/ directory and a lot of people objected to this recommending to
> > merge it into the existing arch/arm. Once I posted the re-worked
> > implementation there were no big .
> 
> It looks like I didn't finish the above phrase - I meant no big
> objections to the conditional compilation or someone stating clearly
> that this is unacceptable.

And that's partly what I find rather frustrating.  I _know_ that people
have issues with it, but they haven't raised them.

Re: [PATCH 00/12] Thumb-2 kernel support

[Catalin Marinas]

(the e-mails are getting pretty long but I think this is useful as we've
never discussed properly about this in the past)

On Mon, 2008-07-28 at 13:50 +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 28, 2008 at 12:45:11PM +0100, Catalin Marinas wrote:
> > On Sun, 2008-07-27 at 12:08 +0100, Russell King - ARM Linux wrote:
> > > There have been some valid but (iirc) non-vocal objections to the
> > > Thumb-2 support from a few people.  The biggest one is all the
> > > mess associated with supporting this "unified" assembler stuff.
> > 
> > My first implementation of these patches (last year) created a separate
> > arch/ directory and a lot of people objected to this recommending to
> > merge it into the existing arch/arm. Once I posted the re-worked
> > implementation there were no big .
> 
> I'm not surprised.  People don't want to duplicate their machine support
> across two arch/arm for the same family of SoCs.

IIRC, I left most of the machine support under arch/arm/, only some
Makefile/Kconfig files needed to be duplicated.

> > I agree that the syntax isn't as readable as before but this is mainly
> > because we need to support older binutils via conditional compilation
> > (the unified assembly syntax itself isn't as bad). IIRC, some people
> > were OK with this as long as, at some point, the conditional compilation
> > can be removed once most of the binutils in use support it (I think this
> > syntax is already 2 years old).
> 
> How is that realistically achievable given the issues that I've raised
> over the impossibility of review and the issues with gcc not knowing
> how many instructions an asm() statement generates?

What is this information used for? Size? AFAIK, gcc currently counts the
number of new lines or semicolons but if it needs this to guess the size
of the asm block it is wrong anyway as Thumb-2 instructions have
variable size.

> > If you don't like the unified assembly syntax at all, I can't help much
> > (not even argue whether it makes sense or not, I wasn't involved in
> > designing it and it's pretty late to change it now anyway).
> 
> If neither of us can "help much", then T2 won't be going in, and ARM
> Ltd is going to have to accept that T2 isn't going to be supported by
> mainline Linux.

The unified assembly can't be changed (unless there is a really good
reason like proving it inconsistent on the ARM/Thumb-2 instruction set
rather than Linux-related) - it is already implemented in gcc/binutils,
ARM compiler and some other proprietary toolchains and the cost of
changing this would be quite high. The main decision was taken when
breaking the compatibility with the old syntax and that's why in
new-syntax files you add ".syntax unified".

If you look at Thumb-2 as a new instruction set (though it's pretty
close to ARM, especially with the unified syntax), I don't see why this
couldn't go into mainline as arch/arm-t2/. If we want to go this route,
I don't think it should be your decision *exclusively* but rather a
combination of yours and others upstream.

> However, there is another solution to this: ARM Ltd could write a
> static checker to ensure that the 'itxxxx' instructions match the
> conditions in the following instructions, resulting in disagreements
> between them being spotted at build time, rather than having some
> data corrupting bug lurking around.
> 
> I totally disagree with getting rid of those conditional suffixes
> even for T2 only code - doing so will make my objection even stronger
> against T2.  Rather than getting rid of the conditional suffixes, use
> them as a continuing existing check that the 'itxxxx' instructions are
> correct.

You might have read some (initial) draft documents but the unified
syntax enforces the IT instruction to match the subsequent conditional
suffixes. Have a look at my patches to see that everything conditional
instruction has a flag matching the corresponding IT flag. With -mthumb,
gas even complaints if a suffix is missing or incompatible with a
previous IT instruction. It doesn't do this for ARM as it ignores the IT
instruction completely (i.e. only takes the conditional suffixes into
account). Since the IT instruction in ARM isn't mandatory, there is no
be any reason to enforce it (if it exists, it could check it but you
easily find the problem by just compiling to Thumb-2).

I can point you to the right documentation if interested.

> I think I've proven my point well - at least one of those 'itxxxx'
> instructions in your patch is wrong, but not obviously wrong from
> reading the patches.  That's only one which looked at how the
> resulting code actually turns out.  I didn't thoroughly check the
> others, so there could be other issues lurking in the missing
> patch context lines.

I'll reply separately to those e-mails but the current behaviour is that
the matching of IT and conditional suffixes is always checked in Thumb-2
mode and the IT is ignored in ARM mode where, as before, only the
conditional suffixes are used.

> > IMHO, I don't think the assembly syntax for an architecture is a valid
> > reason to reject patches.
> 
> My objection is primerily on the readability of the code in the ARM
> kernel, and the viability of continued maintainence of it - which is
> an extremely important consideration and one which trumps *everything*
> else - and it's that basis which I'm rejecting the patches.

There is no easy way around. We could duplicate code in the existing
arch/arm by creating things like head-t2.S or entry-t2.S but we still
have to cope with inline assembly in header files plus the amount of
duplication in arch/arm/lib.

> That objection is directly caused by this crappy assembly syntax.  So
> you can turn it into something political by saying that it's the
> assembly syntax I'm objecting to.  My objection is _far_ more than
> that.

It looks to me like you are objecting to both the *unified syntax* and
the *Thumb-2 instruction set* in general (while missing the big
picture). The old syntax *cannot* fully accommodate the new Thumb-2
instruction set. Rather than having a completely separate assembly
syntax, ARM decided to slightly modify the existing one to accommodate
both and make life *easier* for developers (on new CPUs) but with the
risk of breaking backwards compatibility.

The main additions are the IT Thumb-2 instruction (not used in ARM
assembly) and a way to explicitly state whether you want a specific
instruction to be 32bit or 16bit wide in Thumb-2 (always 32 bit on ARM)
using the ".w" suffix.

If the syntax would have been completely different, no-one would have
asked for this Linux port to be merged into arch/arm/ (as it happened
when I first posted the patches).

I will restate - the Thumb-2 patches make the current assembly code less
readable not only because of the unified syntax (this only adds the W
macro and IT instruction) but also the limitations of the Thumb-2
instruction set where I had to add conditional compilation. You can
argue that ARM Ltd should redesign the Thumb-2 instruction set to be
identical to ARM.

> > There were some public discussions in the past with Richard Earnshaw
> > (gcc/binutils) here in ARM regarding the assembly syntax but I don't
> > think it can be changed in a fully backwards compatible way while
> > supporting a slightly different instruction set.
> 
> Were there?  That's the first I've heard of them!  The use of "public"
> here is actually rather annoying.  They may have been "public" from the
> point of view of being on some random mailing list on the Internet, but
> that doesn't mean that everyone concerned knows that they're going on.

OK, sorry for the confusion. I cc'ed Richard to a reply to Nicolas and
arm-linux-kernel but Richard decided to reply off-line only to Nicolas
and me:

http://article.gmane.org/gmane.linux.ports.arm.kernel/38947

Anyway, the main summary is a few paragraphs above.

> > ???I would really like some clear statement from you to know where to
> > focus my attention (clean up the current patches even more or create
> > separate arch/arm-t2). AFAICT, you don't really like these patches
> > merged, in which case I'd have to focus on the second option.
> 
> I've put my point of view forward in the responses to the patches.
> I'm not sure what "clear statement" you're really after.

The comments are useful and I'll reply to them separately.

But since you are objecting to the unified assembly syntax in general
which *cannot* be changed, my understanding is that you don't want this
set of patches under arch/arm/, no matter how much I try to adapt the
Thumb-2 patches. Is my understanding correct?

> Maybe if that ARM Linux developers conference had gone ahead, this
> could have been discussed there in a broader manner and some way
> forward could have been reached.

Well, it is pretty difficult to get a lot of people from around the
world together and since some of the main developers (including you)
couldn't make the specific date, it was cancelled. If you want, we can
have a few-hours conference call via phone/skype/gnome meeting/irc etc.
with ARM Linux developers joining in.

-- 
Catalin

Re: [PATCH 00/12] Thumb-2 kernel support

[Nicolas Pitre]

On Mon, 28 Jul 2008, Russell King - ARM Linux wrote:

> On Mon, Jul 28, 2008 at 01:40:55PM +0100, Catalin Marinas wrote:
> > On Mon, 2008-07-28 at 12:45 +0100, Catalin Marinas wrote:
> > > On Sun, 2008-07-27 at 12:08 +0100, Russell King - ARM Linux wrote:
> > > > On Wed, Jul 23, 2008 at 03:10:45PM -0700, Andrew Morton wrote:
> > > > > Well.  Rather than doing things sequentially we could go parallel. 
> > > > > Russell could say "I'll look at them before 2.6.28 but please put them
> > > > > into linux-next meanwhile".
> > > > 
> > > > There have been some valid but (iirc) non-vocal objections to the
> > > > Thumb-2 support from a few people.  The biggest one is all the
> > > > mess associated with supporting this "unified" assembler stuff.
> > > 
> > > My first implementation of these patches (last year) created a separate
> > > arch/ directory and a lot of people objected to this recommending to
> > > merge it into the existing arch/arm. Once I posted the re-worked
> > > implementation there were no big .
> > 
> > It looks like I didn't finish the above phrase - I meant no big
> > objections to the conditional compilation or someone stating clearly
> > that this is unacceptable.
> 
> And that's partly what I find rather frustrating.  I _know_ that people
> have issues with it, but they haven't raised them.

It happens that I just returned from vacations so couldn't speak up 
before now.

I do have issues with this new and improved unified assembly 
crap^H^H^H^Hsyntax.  And crappy features makes for crappy patches.  
With some additional rounds of review and refinements we might possibly 
make T2 support somewhat tolerable, but I don't think the latest T2 
patch series is there yet.  I'll provide relevant comments in the 
existing threads.


Nicolas

Re: [PATCH 00/12] Thumb-2 kernel support

[Nicolas Pitre]

On Mon, 28 Jul 2008, Russell King - ARM Linux wrote:

> On Mon, Jul 28, 2008 at 12:45:11PM +0100, Catalin Marinas wrote:
> > ???I would really like some clear statement from you to know where 
> > to focus my attention (clean up the current patches even more or 
> > create separate arch/arm-t2). AFAICT, you don't really like these 
> > patches merged, in which case I'd have to focus on the second 
> > option.
> 
> I've put my point of view forward in the responses to the patches.
> I'm not sure what "clear statement" you're really after.

For what my personal opinion is worth, simply considering a arch/arm-t2 
solution should be completely out of the question.  T2 support is 
possibly not too far from being somewhat tolerable given your latest 
patch series plus all suggestions to improve it further.

So my biggest issue with this whole T2 unified assembly 
crap^H^H^H^Hsyntax has to do with the fact that it is apparently too 
late to change it.  One of the best assets for the ARM architecture IMHO 
was its really neat assembly syntax and now it feels like ARM just 
screwed it.

IMHO again, this was a big mistake from ARM Ltd not to consult people 
from the Open Source community who do write ARM assembly code when this 
unified syntax was elaborated.  for one I certainly wasn't consulted at 
all. That could have provided ARM Ltd with inputs from folks whose first 
concern is long term code maintainability.  Sure, many ARM Ltd partners 
just don't have such concern as they usually do write-once-and-forget 
assembly code and they're probably just happy with the current T2 
syntax, or they simply don't care.  This is not my case.

In other words, and IMHO again, this whole new combined syntax just 
sucks.  That is my personal statement and I don't know how to make it 
more clear.  Now, since we're stuck with it for the foreseeable future, 
let's find a way to best accommodate it which does NOT include a 
separate architecture tree.


Nicolas