Date:	Tue, 10 Feb 2015 13:37:35 +0000
From:	Mark Rutland <mark.rutland@....com>
To:	Brent Wang <wangbintian@...il.com>
Cc:	Bintian Wang <bintian.wang@...wei.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Catalin Marinas <Catalin.Marinas@....com>,
	Will Deacon <Will.Deacon@....com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	Pawel Moll <Pawel.Moll@....com>,
	"ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>,
	"galak@...eaurora.org" <galak@...eaurora.org>,
	"khilman@...aro.org" <khilman@...aro.org>,
	"mturquette@...aro.org" <mturquette@...aro.org>,
	"rob.herring@...aro.org" <rob.herring@...aro.org>,
	"zhangfei.gao@...aro.org" <zhangfei.gao@...aro.org>,
	"haojian.zhuang@...aro.org" <haojian.zhuang@...aro.org>,
	"xuwei5@...ilicon.com" <xuwei5@...ilicon.com>,
	"jh80.chung@...sung.com" <jh80.chung@...sung.com>,
	"olof@...om.net" <olof@...om.net>,
	"yanhaifeng@...il.com" <yanhaifeng@...il.com>,
	"sboyd@...eaurora.org" <sboyd@...eaurora.org>,
	"xuejiancheng@...wei.com" <xuejiancheng@...wei.com>,
	"sledge.yanwei@...wei.com" <sledge.yanwei@...wei.com>,
	"tomeu.vizoso@...labora.com" <tomeu.vizoso@...labora.com>,
	"linux@....linux.org.uk" <linux@....linux.org.uk>,
	"guodong.xu@...aro.org" <guodong.xu@...aro.org>,
	"xuyiping@...ilicon.com" <xuyiping@...ilicon.com>,
	"wangbinghui@...ilicon.com" <wangbinghui@...ilicon.com>,
	"zhenwei.wang@...ilicon.com" <zhenwei.wang@...ilicon.com>,
	"victor.lixin@...ilicon.com" <victor.lixin@...ilicon.com>,
	"puck.chen@...ilicon.com" <puck.chen@...ilicon.com>,
	"dan.zhao@...ilicon.com" <dan.zhao@...ilicon.com>,
	"huxinwei@...wei.com" <huxinwei@...wei.com>,
	"z.liuxinliang@...wei.com" <z.liuxinliang@...wei.com>,
	"heyunlei@...wei.com" <heyunlei@...wei.com>,
	"kong.kongxinwei@...ilicon.com" <kong.kongxinwei@...ilicon.com>,
	"btw@...l.itp.ac.cn" <btw@...l.itp.ac.cn>,
	"w.f@...wei.com" <w.f@...wei.com>,
	"liguozhu@...ilicon.com" <liguozhu@...ilicon.com>
Subject: Re: [PATCH 3/3] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

On Fri, Feb 06, 2015 at 03:37:52PM +0000, Brent Wang wrote:
> Hello Mark,
> 
> 2015-02-06 18:44 GMT+08:00 Mark Rutland <mark.rutland@....com>:
> > On Fri, Feb 06, 2015 at 08:42:22AM +0000, Brent Wang wrote:
> >> Hello Mark,
> >>
> >> 2015-02-06 3:30 GMT+08:00 Mark Rutland <mark.rutland@....com>:
> >> > On Thu, Feb 05, 2015 at 09:24:37AM +0000, Bintian Wang wrote:
> >> >> Add initial dtsi file to support Hisilicon Hi6220 SoC with
> >> >> support of Octal core CPUs in two clusters and each cluster
> >> >> has quard Cortex-A53.
> >> >>
> >> >> We now use the "spin-table" method for SMP, and it will be
> >> >> changed to PSCI later.
> >> >
> >> > If that's the case, please don't place the enable-method and related
> >> > properties in this file. Get your bootloader to add the appropriate
> >> > properties for its boot protocol.
> >> >
> >> > When is PSCI likely to appear?
> >> PSCI is under development.
> >
> > Sure. Do you have an estimate as to when it will appear?
> Another team will do the job, I can not give my estimation now.

Ok.

> > What are you using for your PSCI implementation? The ARM Trusted
> > Firmware?
> Yes, ATF.
> >
> > How are you testing it?
> I think cpu hotplug can test it.
> 
> >
> >> > Are we certain of the split between components the PSCI implementation
> >> > must touch and those the kernel must touch?
> >> >
> >> >> Also add dts file to support HiKey development board which
> >> >> based on Hi6220 SoC and document the devicetree bindings.
> >> >>
> >> >> These dts files will be changed later and more nodes will be
> >> >> added to describe other devices.
> >> >
> >> > How is this going to be changed other than the addition of nodes?
> >> >
> >> > Will this DTB continue to work in future? Or do you intend to make
> >> > changes that will break support?
> >> My original idea is: use spin-table for SMP now, when firmware is OK to
> >> support PSCI, we submit another patch to replace the spin-table with PSCI.
> >
> > For any users who have not updated their FW, this will break booting.
> >
> > This is why I suggest having hte bootloader/FW fill this in as it should
> > know what enable-method the FW supports.
> >
> >> If DTB should continue to work in the future, we really need to remove
> >> the spin-table
> >> from current dts file, how about just enable one core now?
> >>
> >> Which one do you favor or any other suggestion?
> >
> > If spin-table is just for testing while you await PSCI, drop spin-table
> > from the dts for now.
> So, just booting one core may be the right choice now, right?

Without an enable-method for secondary CPUs, that will be all that's
possible. If your FW/bootlaoder injects the appropriate enable-method,
then you could gain spin-table based SMP bringup while awaiting PSCI,
without there being a DTB compatibility issue.

[...]

> >> >> +             pm_ctrl: pm_ctrl {
> >> >> +                     compatible = "hisilicon,pmctrl", "syscon";
> >> >> +                     #address-cells = <1>;
> >> >> +                     #size-cells = <1>;
> >> >> +                     reg = <0x0 0xf7032000 0x0 0x1000>;
> >> >> +                     ranges = <0 0x0 0xf7032000 0x1000>;
> >> >> +
> >> >> +                     clock_power: clock3@0 {
> >> >> +                             compatible = "hisilicon,hi6220-clock-power";
> >> >> +                             reg = <0 0x1000>;
> >> >> +                             #clock-cells = <1>;
> >> >> +                     };
> >> >> +             };
> >> >
> >> > I really doesn't see the point in having a sub-device that covers the
> >> > entirely of the register space of the containing node, and that being
> >> > the case I am extremely concerned that the containers are marked as
> >> > syscon compatible.
> >> The SoC clocks are designed and placed under different system controllers,
> >> so I define corresponding nodes under different controllers for clock operation.
> >
> > What I'm concerned wit hhere is that the pm_ctrl node and the clock3@0
> > sub-node have the _exact_ same register space.
> >
> > Given this should mean that the clock3@0 node owns that register space,
> > having the container node export this as syscon does not make sense. And
> > the split between pm_ctrl and clock3@) doesn't seem to make sense given
> > they cover the same space.
> I understand your worry and will find the max offset of those clocks
> under this controller.
> 
> >
> > As I asked before, why is pm_ctrl marked as a syscon, and what's the
> > point of the separate sub-node?
> There is no big difference between pm_ctrl and other controller,  they
> are all designed as
> the base address to control some functions of other modules (certainly
> include some clock gates).

Are they just different instances of the same IP block, or are there
fundamental differences between them?

> Maybe only one node is enough, not one node plus one sub-node ?

At least in the case above, I cannot see a reason to have more than a
single node without a child.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists