mbox

[GIT,PULL,for,3.17] pull request for hisilicon soc

Message ID 53D1F2B0.1020603@hisilicon.com
State New
Headers show

Pull-request

git://github.com/hisilicon/linux-hisi.git tags/hix5hd2-for-3.17

Message

Wei Xu July 25, 2014, 6:01 a.m. UTC
Hi Olof, Hi Kevin, Hi Arnd,

Resend it again and add arm@kernel.org into the CC list.
Please help to merge pull request for hisilicon soc. 

Best Regards,
Wei


The following changes since commit f753b5db43e3819e02354a638ea31e332e08ac3a:

  ARM: hisi: store reboot reg in global variable (2014-07-03 19:29:15 +0800)

are available in the git repository at:

  git://github.com/hisilicon/linux-hisi.git tags/hix5hd2-for-3.17

for you to fetch changes up to 5354cf5f6026b5d16caf0001886c06ab9ca42dff:

  ARM: hisi: enable L2 driver (2014-07-04 14:07:17 +0800)

----------------------------------------------------------------
enable hisilicon terminal SoC based on 3.16-rc1

----------------------------------------------------------------
Haifeng Yan (3):
      ARM: debug: Rename Hi3716 to HI5XHD2
      ARM: hisi: enable hix5hd2 SoC
      ARM: dts: Add hix5hd2-dkb dts file.

Haojian Zhuang (4):
      ARM: debug: add HiP04 debug uart
      ARM: hisi: add ARCH_HISI
      ARM: config: add ARCH_HIX5HD2 in hi3xxx_defconfig
      ARM: hisi: enable L2 driver

 arch/arm/Kconfig.debug            |  22 +++++++++++++-----
 arch/arm/Makefile                 |   2 +-
 arch/arm/boot/dts/Makefile        |   1 +
 arch/arm/boot/dts/hix5hd2-dkb.dts |  52 ++++++++++++++++++++++++++++++++++++++++++
 arch/arm/boot/dts/hix5hd2.dtsi    | 171 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 arch/arm/configs/hi3xxx_defconfig |   2 ++
 arch/arm/mach-hisi/Kconfig        |  27 +++++++++++++++++++---
 arch/arm/mach-hisi/Makefile       |   2 +-
 arch/arm/mach-hisi/core.h         |   5 ++++
 arch/arm/mach-hisi/headsmp.S      |  16 +++++++++++++
 arch/arm/mach-hisi/hisilicon.c    |  20 ++++++++++++++++
 arch/arm/mach-hisi/hotplug.c      |  58 ++++++++++++++++++++++++++++++++++++++++++++++
 arch/arm/mach-hisi/platsmp.c      |  50 +++++++++++++++++++++++++++++++++++++---
 13 files changed, 414 insertions(+), 14 deletions(-)
 create mode 100644 arch/arm/boot/dts/hix5hd2-dkb.dts
 create mode 100644 arch/arm/boot/dts/hix5hd2.dtsi
 create mode 100644 arch/arm/mach-hisi/headsmp.S

Comments

Arnd Bergmann July 26, 2014, 3:30 p.m. UTC | #1
On Friday 25 July 2014 14:01:20 xuwei wrote:
> Hi Olof, Hi Kevin, Hi Arnd,
> 
> Resend it again and add arm@kernel.org into the CC list.
> Please help to merge pull request for hisilicon soc. 
> 

I pulled this at first and was planning to complain about
individual problems, but then decided to undo the pull, after
the mistakes outweigh the good parts in this pull request:
> 
> The following changes since commit f753b5db43e3819e02354a638ea31e332e08ac3a:
> 
>   ARM: hisi: store reboot reg in global variable (2014-07-03 19:29:15 +0800)

We have not actually merge commit f753b5db43e3819e yet, and you effectively
left it out of the pull request, but it is of course present in the branch.

This commit also seems completely wrong and should be reverted: There
is no need to avoid parsing DT in the restart function, and instead of
adding another callback, the code should be changed to move the restart
handling into a separate driver in drivers/power/reset/.

> are available in the git repository at:
> 
>   git://github.com/hisilicon/linux-hisi.git tags/hix5hd2-for-3.17
> 
> for you to fetch changes up to 5354cf5f6026b5d16caf0001886c06ab9ca42dff:
> 
>   ARM: hisi: enable L2 driver (2014-07-04 14:07:17 +0800)
> 
> ----------------------------------------------------------------
> enable hisilicon terminal SoC based on 3.16-rc1
> 

This description doesn't seem to reflect the contents of the
branch, and is much too short. I actually starting writing
a proper changeset text to help you but dropped that now.

> ----------------------------------------------------------------
> Haifeng Yan (3):
>       ARM: debug: Rename Hi3716 to HI5XHD2

This one had me confused for a while, because it seems like
you are breaking support for hi3xxx, but instead it's just
a different name for the same chip if I see this right.

An easier approach would actually be to remove DEBUG_HI3716_UART
completely: the setting is exactly the same for
HI3716, HI3620 and HIX5HD2, so you can simply keep the name
for the oldest chip here and change the help text to reflect
which products it works on.

>       ARM: hisi: enable hix5hd2 SoC

Here you introduce a device node with compatible="hisilicon,cpuctrl",
which is a far too generic name, as it seems to imply that this is
the only "CPU control" part that ever came out of hisilicon. The
binding should be documented in
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
and reviewed on the mailing list. From the use of the code, it
seems that this should really be a reset controller with a
device driver in drivers/reset, but that is not clear without
knowing more about the hardware.

Also, it would be nice to use the CPU_METHOD_OF_DECLARE() macro here.

Finally, it's not clear why you need a new Kconfig symbol. It seems
that all code you have is compiled independent of these, except for
the dtb files and the DEBUG_LL setting.

>       ARM: dts: Add hix5hd2-dkb dts file.

This is ok.

> Haojian Zhuang (4):
>       ARM: debug: add HiP04 debug uart

This patch seem correct, but it is completely useless
because support for HiP04 is not available. You effectively
just add dead code.

>       ARM: hisi: add ARCH_HISI

This is ok, except for the "hisilicon,cpuctrl" node mentioned
above.

>       ARM: config: add ARCH_HIX5HD2 in hi3xxx_defconfig

I'd prefer to see the defconfig changes in a separate pull
request. Also, please enable all drivers you need in
multi_v7_defconfig.

>       ARM: hisi: enable L2 driver

This one should also be done differently. Instead of adding
an .init_machine function, you should set the l2x0 aux value
in the machine descriptor directly.

If you can fix up the problems I mention above quickly, I can
consider a new pull request.

What happened to HiP04 support? I thought that would arrive
in time for 3.17.

	Arnd
Haojian Zhuang July 27, 2014, 1:57 a.m. UTC | #2
On 26 July 2014 23:30, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 25 July 2014 14:01:20 xuwei wrote:
>> Hi Olof, Hi Kevin, Hi Arnd,
>>
>> Resend it again and add arm@kernel.org into the CC list.
>> Please help to merge pull request for hisilicon soc.
>>
>
> I pulled this at first and was planning to complain about
> individual problems, but then decided to undo the pull, after
> the mistakes outweigh the good parts in this pull request:
>>
>> The following changes since commit f753b5db43e3819e02354a638ea31e332e08ac3a:
>>
>>   ARM: hisi: store reboot reg in global variable (2014-07-03 19:29:15 +0800)
>
> We have not actually merge commit f753b5db43e3819e yet, and you effectively
> left it out of the pull request, but it is of course present in the branch.
>
> This commit also seems completely wrong and should be reverted: There
> is no need to avoid parsing DT in the restart function, and instead of
> adding another callback, the code should be changed to move the restart
> handling into a separate driver in drivers/power/reset/.
>
>> are available in the git repository at:
>>
>>   git://github.com/hisilicon/linux-hisi.git tags/hix5hd2-for-3.17
>>
>> for you to fetch changes up to 5354cf5f6026b5d16caf0001886c06ab9ca42dff:
>>
>>   ARM: hisi: enable L2 driver (2014-07-04 14:07:17 +0800)
>>
>> ----------------------------------------------------------------
>> enable hisilicon terminal SoC based on 3.16-rc1
>>
>
> This description doesn't seem to reflect the contents of the
> branch, and is much too short. I actually starting writing
> a proper changeset text to help you but dropped that now.
>
>> ----------------------------------------------------------------
>> Haifeng Yan (3):
>>       ARM: debug: Rename Hi3716 to HI5XHD2
>
> This one had me confused for a while, because it seems like
> you are breaking support for hi3xxx, but instead it's just
> a different name for the same chip if I see this right.
>
> An easier approach would actually be to remove DEBUG_HI3716_UART
> completely: the setting is exactly the same for
> HI3716, HI3620 and HIX5HD2, so you can simply keep the name
> for the oldest chip here and change the help text to reflect
> which products it works on.

The physical address of hi3xxx uart is different from x5hd2.

Since hi3620 & hix5hd2 could be built into one image. If I don't use the
DEBUG_HIX5HD2_UART to mark, I can't distinguish the right UART
physical address only by ARCH_HI3xxx or ARCH_HIX5HD2.

>
>>       ARM: hisi: enable hix5hd2 SoC
>
> Here you introduce a device node with compatible="hisilicon,cpuctrl",
> which is a far too generic name, as it seems to imply that this is
> the only "CPU control" part that ever came out of hisilicon. The
> binding should be documented in
> Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
> and reviewed on the mailing list. From the use of the code, it
> seems that this should really be a reset controller with a
> device driver in drivers/reset, but that is not clear without
> knowing more about the hardware.
>
> Also, it would be nice to use the CPU_METHOD_OF_DECLARE() macro here.

OK. I'll update it.

>
> Finally, it's not clear why you need a new Kconfig symbol. It seems
> that all code you have is compiled independent of these, except for
> the dtb files and the DEBUG_LL setting.
>

Actually it's nearly same except for headsmp.S.

Hisilicon guys think that hix5hd2 belongs to another group. They don't
want to totally share their code base with hi3xxx.

>>       ARM: dts: Add hix5hd2-dkb dts file.
>
> This is ok.
>
>> Haojian Zhuang (4):
>>       ARM: debug: add HiP04 debug uart
>
> This patch seem correct, but it is completely useless
> because support for HiP04 is not available. You effectively
> just add dead code.
>

OK. I can remove this from this patch set.

>>       ARM: hisi: add ARCH_HISI
>
> This is ok, except for the "hisilicon,cpuctrl" node mentioned
> above.
>
>>       ARM: config: add ARCH_HIX5HD2 in hi3xxx_defconfig
>
> I'd prefer to see the defconfig changes in a separate pull
> request. Also, please enable all drivers you need in
> multi_v7_defconfig.
>

OK

>>       ARM: hisi: enable L2 driver
>
> This one should also be done differently. Instead of adding
> an .init_machine function, you should set the l2x0 aux value
> in the machine descriptor directly.
>
OK

> If you can fix up the problems I mention above quickly, I can
> consider a new pull request.
>

I'll try.

> What happened to HiP04 support? I thought that would arrive
> in time for 3.17.
>

I just sent v14 in this week. I already updated gic & mcpm according
to comments. But I haven't gotten any comments & Ack yet. So I don't
know whether we could send the pull request of HiP04 in 3.17.

>         Arnd
Arnd Bergmann July 28, 2014, 11:35 a.m. UTC | #3
On Sunday 27 July 2014 09:57:22 Haojian Zhuang wrote:
> On 26 July 2014 23:30, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Friday 25 July 2014 14:01:20 xuwei wrote:
> >
> >> ----------------------------------------------------------------
> >> Haifeng Yan (3):
> >>       ARM: debug: Rename Hi3716 to HI5XHD2
> >
> > This one had me confused for a while, because it seems like
> > you are breaking support for hi3xxx, but instead it's just
> > a different name for the same chip if I see this right.
> >
> > An easier approach would actually be to remove DEBUG_HI3716_UART
> > completely: the setting is exactly the same for
> > HI3716, HI3620 and HIX5HD2, so you can simply keep the name
> > for the oldest chip here and change the help text to reflect
> > which products it works on.
> 
> The physical address of hi3xxx uart is different from x5hd2.
> 
> Since hi3620 & hix5hd2 could be built into one image. If I don't use the
> DEBUG_HIX5HD2_UART to mark, I can't distinguish the right UART
> physical address only by ARCH_HI3xxx or ARCH_HIX5HD2.

Ah, you are right, I misread the source code. I saw that the
virtual address is the same for both but didn't notice that
the physical address is not.

So this patch is ok after all, please just clarify in the changelog
that it is actually the same chip.

> > Finally, it's not clear why you need a new Kconfig symbol. It seems
> > that all code you have is compiled independent of these, except for
> > the dtb files and the DEBUG_LL setting.
> >
> 
> Actually it's nearly same except for headsmp.S.
> 
> Hisilicon guys think that hix5hd2 belongs to another group. They don't
> want to totally share their code base with hi3xxx.

This doesn't seem like a technical reason to me at all. I would
much prefer if you and Xu Wei as maintainers were able to describe
(in the changelog) the underlying technical reasons for decisions
coming from management if it makes sense, or otherwise explain to
them that we don't want those patches upstream if it doesn't make
sense.

It's not a show-stopper this way, and I'd still pull the branches
with this, but you should be aware that it does not help your
reputation.

> > What happened to HiP04 support? I thought that would arrive
> > in time for 3.17.
> >
> 
> I just sent v14 in this week. I already updated gic & mcpm according
> to comments. But I haven't gotten any comments & Ack yet. So I don't
> know whether we could send the pull request of HiP04 in 3.17.

I don't see anything beyond v10 in my email, and that had a few
outstanding comments but otherwise looked almost ready to me.
Can you find out what happened?

	Arnd
Jason Cooper July 28, 2014, 1:05 p.m. UTC | #4
Arnd, Marc,

On Mon, Jul 28, 2014 at 01:35:50PM +0200, Arnd Bergmann wrote:
> On Sunday 27 July 2014 09:57:22 Haojian Zhuang wrote:
> > On 26 July 2014 23:30, Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Friday 25 July 2014 14:01:20 xuwei wrote:
> > >
> > >> ----------------------------------------------------------------
> > >> Haifeng Yan (3):
> > >>       ARM: debug: Rename Hi3716 to HI5XHD2
> > >
> > > This one had me confused for a while, because it seems like
> > > you are breaking support for hi3xxx, but instead it's just
> > > a different name for the same chip if I see this right.
> > >
> > > An easier approach would actually be to remove DEBUG_HI3716_UART
> > > completely: the setting is exactly the same for
> > > HI3716, HI3620 and HIX5HD2, so you can simply keep the name
> > > for the oldest chip here and change the help text to reflect
> > > which products it works on.
> > 
> > The physical address of hi3xxx uart is different from x5hd2.
> > 
> > Since hi3620 & hix5hd2 could be built into one image. If I don't use the
> > DEBUG_HIX5HD2_UART to mark, I can't distinguish the right UART
> > physical address only by ARCH_HI3xxx or ARCH_HIX5HD2.
> 
> Ah, you are right, I misread the source code. I saw that the
> virtual address is the same for both but didn't notice that
> the physical address is not.
> 
> So this patch is ok after all, please just clarify in the changelog
> that it is actually the same chip.
> 
> > > Finally, it's not clear why you need a new Kconfig symbol. It seems
> > > that all code you have is compiled independent of these, except for
> > > the dtb files and the DEBUG_LL setting.
> > >
> > 
> > Actually it's nearly same except for headsmp.S.
> > 
> > Hisilicon guys think that hix5hd2 belongs to another group. They don't
> > want to totally share their code base with hi3xxx.
> 
> This doesn't seem like a technical reason to me at all. I would
> much prefer if you and Xu Wei as maintainers were able to describe
> (in the changelog) the underlying technical reasons for decisions
> coming from management if it makes sense, or otherwise explain to
> them that we don't want those patches upstream if it doesn't make
> sense.
> 
> It's not a show-stopper this way, and I'd still pull the branches
> with this, but you should be aware that it does not help your
> reputation.
> 
> > > What happened to HiP04 support? I thought that would arrive
> > > in time for 3.17.
> > >
> > 
> > I just sent v14 in this week. I already updated gic & mcpm according
> > to comments. But I haven't gotten any comments & Ack yet. So I don't
> > know whether we could send the pull request of HiP04 in 3.17.
> 
> I don't see anything beyond v10 in my email, and that had a few
> outstanding comments but otherwise looked almost ready to me.
> Can you find out what happened?

I'd like Marc to review the newest version of the changes to the GIC and
Ack before I'll pull them in.  He was on vacation up until today.
You'll be able to pull in a tag on irqchip/gic and base off of that if
needed.

Conversely, if you were to move the HiP04 to a separate file, like I
asked, (using Mark's suggestion of function pointers, which you've
done), I could probably Ack it to go through arm-soc.  There are simply
too many changes to irq-gic.c this time around...

thx,

Jason.
Haojian Zhuang July 28, 2014, 1:48 p.m. UTC | #5
On 28 July 2014 19:35, Arnd Bergmann <arnd@arndb.de> wrote:
> On Sunday 27 July 2014 09:57:22 Haojian Zhuang wrote:
>>
>> I just sent v14 in this week. I already updated gic & mcpm according
>> to comments. But I haven't gotten any comments & Ack yet. So I don't
>> know whether we could send the pull request of HiP04 in 3.17.
>
> I don't see anything beyond v10 in my email, and that had a few
> outstanding comments but otherwise looked almost ready to me.
> Can you find out what happened?
>
>         Arnd

I'll send v15 patch set since the code base of HiP04 relies on HIX5HD2.
It's only refreshed because of code base updated.

Best Regards
Haojian
Marc Zyngier July 28, 2014, 1:54 p.m. UTC | #6
Hi Jason,

On 28/07/14 14:05, Jason Cooper wrote:
> Arnd, Marc,
> 
> On Mon, Jul 28, 2014 at 01:35:50PM +0200, Arnd Bergmann wrote:
>> On Sunday 27 July 2014 09:57:22 Haojian Zhuang wrote:
>>> On 26 July 2014 23:30, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> On Friday 25 July 2014 14:01:20 xuwei wrote:
>>>>
>>>>> ----------------------------------------------------------------
>>>>> Haifeng Yan (3):
>>>>>       ARM: debug: Rename Hi3716 to HI5XHD2
>>>>
>>>> This one had me confused for a while, because it seems like
>>>> you are breaking support for hi3xxx, but instead it's just
>>>> a different name for the same chip if I see this right.
>>>>
>>>> An easier approach would actually be to remove DEBUG_HI3716_UART
>>>> completely: the setting is exactly the same for
>>>> HI3716, HI3620 and HIX5HD2, so you can simply keep the name
>>>> for the oldest chip here and change the help text to reflect
>>>> which products it works on.
>>>
>>> The physical address of hi3xxx uart is different from x5hd2.
>>>
>>> Since hi3620 & hix5hd2 could be built into one image. If I don't use the
>>> DEBUG_HIX5HD2_UART to mark, I can't distinguish the right UART
>>> physical address only by ARCH_HI3xxx or ARCH_HIX5HD2.
>>
>> Ah, you are right, I misread the source code. I saw that the
>> virtual address is the same for both but didn't notice that
>> the physical address is not.
>>
>> So this patch is ok after all, please just clarify in the changelog
>> that it is actually the same chip.
>>
>>>> Finally, it's not clear why you need a new Kconfig symbol. It seems
>>>> that all code you have is compiled independent of these, except for
>>>> the dtb files and the DEBUG_LL setting.
>>>>
>>>
>>> Actually it's nearly same except for headsmp.S.
>>>
>>> Hisilicon guys think that hix5hd2 belongs to another group. They don't
>>> want to totally share their code base with hi3xxx.
>>
>> This doesn't seem like a technical reason to me at all. I would
>> much prefer if you and Xu Wei as maintainers were able to describe
>> (in the changelog) the underlying technical reasons for decisions
>> coming from management if it makes sense, or otherwise explain to
>> them that we don't want those patches upstream if it doesn't make
>> sense.
>>
>> It's not a show-stopper this way, and I'd still pull the branches
>> with this, but you should be aware that it does not help your
>> reputation.
>>
>>>> What happened to HiP04 support? I thought that would arrive
>>>> in time for 3.17.
>>>>
>>>
>>> I just sent v14 in this week. I already updated gic & mcpm according
>>> to comments. But I haven't gotten any comments & Ack yet. So I don't
>>> know whether we could send the pull request of HiP04 in 3.17.
>>
>> I don't see anything beyond v10 in my email, and that had a few
>> outstanding comments but otherwise looked almost ready to me.
>> Can you find out what happened?
> 
> I'd like Marc to review the newest version of the changes to the GIC and
> Ack before I'll pull them in.  He was on vacation up until today.
> You'll be able to pull in a tag on irqchip/gic and base off of that if
> needed.

Which series is the last one? I have a single v14 patch in my Inbox
(01/11, sent on the 23rd), and nothing follows it (and no cover-letter
either).

The archive seems to agree with the state of my Inbox... Did something
go horribly wrong?

Thanks,

	M.
Arnd Bergmann July 28, 2014, 1:58 p.m. UTC | #7
On Monday 28 July 2014 14:54:24 Marc Zyngier wrote:
> 
> Which series is the last one? I have a single v14 patch in my Inbox
> (01/11, sent on the 23rd), and nothing follows it (and no cover-letter
> either).
> 
> The archive seems to agree with the state of my Inbox... Did something
> go horribly wrong?

I see the same thing. I originally thought I didn't get any patches,
but I do in fact have a lone patch 1/11 as well.

	Arnd
Haojian Zhuang July 28, 2014, 2 p.m. UTC | #8
On 28 July 2014 21:58, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 28 July 2014 14:54:24 Marc Zyngier wrote:
>>
>> Which series is the last one? I have a single v14 patch in my Inbox
>> (01/11, sent on the 23rd), and nothing follows it (and no cover-letter
>> either).
>>
>> The archive seems to agree with the state of my Inbox... Did something
>> go horribly wrong?
>
> I see the same thing. I originally thought I didn't get any patches,
> but I do in fact have a lone patch 1/11 as well.
>
>         Arnd

Because I just sent 1/11 without others. Since there's no change on
other patches. Now I'm sending all patches as v15. I'm sorry for
inconvenience.

Regards
Haojian
Haojian Zhuang July 28, 2014, 2:02 p.m. UTC | #9
On 28 July 2014 21:05, Jason Cooper <jason@lakedaemon.net> wrote:
> Arnd, Marc,
>
>
> I'd like Marc to review the newest version of the changes to the GIC and
> Ack before I'll pull them in.  He was on vacation up until today.
> You'll be able to pull in a tag on irqchip/gic and base off of that if
> needed.
>
> Conversely, if you were to move the HiP04 to a separate file, like I
> asked, (using Mark's suggestion of function pointers, which you've
> done), I could probably Ack it to go through arm-soc.  There are simply
> too many changes to irq-gic.c this time around...
>
> thx,
>
> Jason.

Only the 1/11 (irq patch) is based on your git tree because of GICv3.
If you could merge it into your git tree, it would be great.

Best Regards
Haojian