diff mbox

[RFC,V2,10/12] arm64: dts: msm8994 issolate non standard bootloader/LK entries

Message ID 1475375919-618-11-git-send-email-jmcnicol@redhat.com (mailing list archive)
State Not Applicable, archived
Delegated to: Andy Gross
Headers show

Commit Message

Jeremy McNicoll Oct. 2, 2016, 2:38 a.m. UTC
These non standard DT entries need to be cast aside as to not
pollute the main device tree bindings.  Without these essential
DT items the bootloader/LK will not pass control over to the kernel
and thus never boot.

Signed-off-by: Jeremy McNicoll <jeremymc@redhat.com>
---
 .../arm64/boot/dts/qcom/msm8994-angler-rev-101.dts |  1 -
 arch/arm64/boot/dts/qcom/msm8994.dtsi              |  3 +--
 .../boot/dts/qcom/nexus6p_bootloader_bits.dtsi     | 24 ++++++++++++++++++++++
 3 files changed, 25 insertions(+), 3 deletions(-)
 create mode 100644 arch/arm64/boot/dts/qcom/nexus6p_bootloader_bits.dtsi

Comments

Rob Herring Oct. 12, 2016, 12:41 a.m. UTC | #1
On Sat, Oct 1, 2016 at 9:38 PM, Jeremy McNicoll <jmcnicol@redhat.com> wrote:
> These non standard DT entries need to be cast aside as to not
> pollute the main device tree bindings.  Without these essential
> DT items the bootloader/LK will not pass control over to the kernel
> and thus never boot.

I discussed this with Stephen recently. I'm okay with leaving these on
boards that have no chance of getting updated bootloaders to use the
compatible string instead. Having to use dtbTool is far worse than a
couple of extra properties IMO. I reserve the right to complain if new
stuff continues to use these though.

> Signed-off-by: Jeremy McNicoll <jeremymc@redhat.com>
> ---
>  .../arm64/boot/dts/qcom/msm8994-angler-rev-101.dts |  1 -
>  arch/arm64/boot/dts/qcom/msm8994.dtsi              |  3 +--
>  .../boot/dts/qcom/nexus6p_bootloader_bits.dtsi     | 24 ++++++++++++++++++++++

Just put this into the board file rather than yet another include.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Oct. 12, 2016, 10:39 a.m. UTC | #2
On Tuesday, October 11, 2016 7:41:22 PM CEST Rob Herring wrote:
> On Sat, Oct 1, 2016 at 9:38 PM, Jeremy McNicoll <jmcnicol@redhat.com> wrote:
> > These non standard DT entries need to be cast aside as to not
> > pollute the main device tree bindings.  Without these essential
> > DT items the bootloader/LK will not pass control over to the kernel
> > and thus never boot.
> 
> I discussed this with Stephen recently. I'm okay with leaving these on
> boards that have no chance of getting updated bootloaders to use the
> compatible string instead. Having to use dtbTool is far worse than a
> couple of extra properties IMO. I reserve the right to complain if new
> stuff continues to use these though.
> 
> > Signed-off-by: Jeremy McNicoll <jeremymc@redhat.com>
> > ---
> >  .../arm64/boot/dts/qcom/msm8994-angler-rev-101.dts |  1 -
> >  arch/arm64/boot/dts/qcom/msm8994.dtsi              |  3 +--
> >  .../boot/dts/qcom/nexus6p_bootloader_bits.dtsi     | 24 ++++++++++++++++++++++
> 
> Just put this into the board file rather than yet another include.

The suggestion that I had was to have two .dts files: the normal
one without these properties, and another .dts file including the
first but adding these three for compatibility with the legacy
bootloaders.

That way we could have a 'clean' .dtb file once the bootloaders
get fixed, and can name the other one appropriately to discourage
copying the method for new machines.

Having the bootloader files included from the main .dts files
would serve no purpose.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Oct. 12, 2016, 12:11 p.m. UTC | #3
On Wed, Oct 12, 2016 at 5:39 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday, October 11, 2016 7:41:22 PM CEST Rob Herring wrote:
>> On Sat, Oct 1, 2016 at 9:38 PM, Jeremy McNicoll <jmcnicol@redhat.com> wrote:
>> > These non standard DT entries need to be cast aside as to not
>> > pollute the main device tree bindings.  Without these essential
>> > DT items the bootloader/LK will not pass control over to the kernel
>> > and thus never boot.
>>
>> I discussed this with Stephen recently. I'm okay with leaving these on
>> boards that have no chance of getting updated bootloaders to use the
>> compatible string instead. Having to use dtbTool is far worse than a
>> couple of extra properties IMO. I reserve the right to complain if new
>> stuff continues to use these though.
>>
>> > Signed-off-by: Jeremy McNicoll <jeremymc@redhat.com>
>> > ---
>> >  .../arm64/boot/dts/qcom/msm8994-angler-rev-101.dts |  1 -
>> >  arch/arm64/boot/dts/qcom/msm8994.dtsi              |  3 +--
>> >  .../boot/dts/qcom/nexus6p_bootloader_bits.dtsi     | 24 ++++++++++++++++++++++
>>
>> Just put this into the board file rather than yet another include.
>
> The suggestion that I had was to have two .dts files: the normal
> one without these properties, and another .dts file including the
> first but adding these three for compatibility with the legacy
> bootloaders.
>
> That way we could have a 'clean' .dtb file once the bootloaders
> get fixed, and can name the other one appropriately to discourage
> copying the method for new machines.

Yes, that makes sense. Though my understanding is things like the
Nexus 6P and 5X will never get fixed bootloaders. The DB410c and other
dev boards are hopefully another story.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeremy McNicoll Oct. 13, 2016, 12:59 a.m. UTC | #4
On 2016-10-12 3:39 AM, Arnd Bergmann wrote:
> On Tuesday, October 11, 2016 7:41:22 PM CEST Rob Herring wrote:
>> On Sat, Oct 1, 2016 at 9:38 PM, Jeremy McNicoll <jmcnicol@redhat.com> wrote:
>>> These non standard DT entries need to be cast aside as to not
>>> pollute the main device tree bindings.  Without these essential
>>> DT items the bootloader/LK will not pass control over to the kernel
>>> and thus never boot.
>>
>> I discussed this with Stephen recently. I'm okay with leaving these on
>> boards that have no chance of getting updated bootloaders to use the
>> compatible string instead. Having to use dtbTool is far worse than a
>> couple of extra properties IMO. I reserve the right to complain if new
>> stuff continues to use these though.
>>
>>> Signed-off-by: Jeremy McNicoll <jeremymc@redhat.com>
>>> ---
>>>  .../arm64/boot/dts/qcom/msm8994-angler-rev-101.dts |  1 -
>>>  arch/arm64/boot/dts/qcom/msm8994.dtsi              |  3 +--
>>>  .../boot/dts/qcom/nexus6p_bootloader_bits.dtsi     | 24 ++++++++++++++++++++++
>>
>> Just put this into the board file rather than yet another include.
>
> The suggestion that I had was to have two .dts files: the normal
> one without these properties, and another .dts file including the
> first but adding these three for compatibility with the legacy
> bootloaders.
>

So I did it backwards from what you had suggested?
Based on my discussion with, (cant seem to recall) my understanding
was that we simply wanted to have these 3 bootloader specific entries
in another file.

> That way we could have a 'clean' .dtb file once the bootloaders
> get fixed, and can name the other one appropriately to discourage
> copying the method for new machines.
>

Did you miss the part about Ebola or Bubonic plague ?

> Having the bootloader files included from the main .dts files
> would serve no purpose.
>

Well, not really.  What about?

$ git revert <commit that introduced nexus6p_bootloader_bits.dtsi>

-jeremy

> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Oct. 19, 2016, 2:56 p.m. UTC | #5
On Wednesday, October 12, 2016 5:59:41 PM CEST Jeremy McNicoll wrote:
> On 2016-10-12 3:39 AM, Arnd Bergmann wrote:
> > On Tuesday, October 11, 2016 7:41:22 PM CEST Rob Herring wrote:
> >> On Sat, Oct 1, 2016 at 9:38 PM, Jeremy McNicoll <jmcnicol@redhat.com> wrote:
> >>> These non standard DT entries need to be cast aside as to not
> >>> pollute the main device tree bindings.  Without these essential
> >>> DT items the bootloader/LK will not pass control over to the kernel
> >>> and thus never boot.
> >>
> >> I discussed this with Stephen recently. I'm okay with leaving these on
> >> boards that have no chance of getting updated bootloaders to use the
> >> compatible string instead. Having to use dtbTool is far worse than a
> >> couple of extra properties IMO. I reserve the right to complain if new
> >> stuff continues to use these though.
> >>
> >>> Signed-off-by: Jeremy McNicoll <jeremymc@redhat.com>
> >>> ---
> >>>  .../arm64/boot/dts/qcom/msm8994-angler-rev-101.dts |  1 -
> >>>  arch/arm64/boot/dts/qcom/msm8994.dtsi              |  3 +--
> >>>  .../boot/dts/qcom/nexus6p_bootloader_bits.dtsi     | 24 ++++++++++++++++++++++
> >>
> >> Just put this into the board file rather than yet another include.
> >
> > The suggestion that I had was to have two .dts files: the normal
> > one without these properties, and another .dts file including the
> > first but adding these three for compatibility with the legacy
> > bootloaders.
> >
> 
(sorry for the late reply, I thought I had replied already but
couldn't find that in the archives when I saw I still had this
reply open)

> So I did it backwards from what you had suggested?
> Based on my discussion with, (cant seem to recall) my understanding
> was that we simply wanted to have these 3 bootloader specific entries
> in another file.

Right

What I would like to see here is two separate .dtb files, one
with the hack and one without it, so we have a migration path
for the machines that eventually get a boot loader with proper
DT support.

> > That way we could have a 'clean' .dtb file once the bootloaders
> > get fixed, and can name the other one appropriately to discourage
> > copying the method for new machines.
> >
> 
> Did you miss the part about Ebola or Bubonic plague ?

I did, which means others are likely to miss it as well ;-)

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Gross Oct. 19, 2016, 9:46 p.m. UTC | #6
On Wed, Oct 19, 2016 at 04:56:20PM +0200, Arnd Bergmann wrote:
> On Wednesday, October 12, 2016 5:59:41 PM CEST Jeremy McNicoll wrote:
> > On 2016-10-12 3:39 AM, Arnd Bergmann wrote:
> > > On Tuesday, October 11, 2016 7:41:22 PM CEST Rob Herring wrote:
> > >> On Sat, Oct 1, 2016 at 9:38 PM, Jeremy McNicoll <jmcnicol@redhat.com> wrote:
> > >>> These non standard DT entries need to be cast aside as to not
> > >>> pollute the main device tree bindings.  Without these essential
> > >>> DT items the bootloader/LK will not pass control over to the kernel
> > >>> and thus never boot.
> > >>
> > >> I discussed this with Stephen recently. I'm okay with leaving these on
> > >> boards that have no chance of getting updated bootloaders to use the
> > >> compatible string instead. Having to use dtbTool is far worse than a
> > >> couple of extra properties IMO. I reserve the right to complain if new
> > >> stuff continues to use these though.
> > >>
> > >>> Signed-off-by: Jeremy McNicoll <jeremymc@redhat.com>
> > >>> ---
> > >>>  .../arm64/boot/dts/qcom/msm8994-angler-rev-101.dts |  1 -
> > >>>  arch/arm64/boot/dts/qcom/msm8994.dtsi              |  3 +--
> > >>>  .../boot/dts/qcom/nexus6p_bootloader_bits.dtsi     | 24 ++++++++++++++++++++++
> > >>
> > >> Just put this into the board file rather than yet another include.
> > >
> > > The suggestion that I had was to have two .dts files: the normal
> > > one without these properties, and another .dts file including the
> > > first but adding these three for compatibility with the legacy
> > > bootloaders.
> > >
> > 
> (sorry for the late reply, I thought I had replied already but
> couldn't find that in the archives when I saw I still had this
> reply open)
> 
> > So I did it backwards from what you had suggested?
> > Based on my discussion with, (cant seem to recall) my understanding
> > was that we simply wanted to have these 3 bootloader specific entries
> > in another file.
> 
> Right
> 
> What I would like to see here is two separate .dtb files, one
> with the hack and one without it, so we have a migration path
> for the machines that eventually get a boot loader with proper
> DT support.

So my main beef with this is that it is kind of onerous.  The machines that
require this will never get a bootloader change.  So we'll be adding 2 dtb
targets and only ever use one.

It's much simpler in my opinion to just add the msm-id to the files that need it
right now..... comment it with something like 'this is because of the Qualcomm
braindead bootloader requirements' and move on.

If there was any hope of a new bootloader for non-bleeding edge boards, I'd
wholeheartedly agree with you Arnd.  But there isn't, and there won't be.


Regards,

Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Oct. 20, 2016, 1:07 a.m. UTC | #7
On Wed, Oct 19, 2016 at 4:46 PM, Andy Gross <andy.gross@linaro.org> wrote:
> On Wed, Oct 19, 2016 at 04:56:20PM +0200, Arnd Bergmann wrote:
>> On Wednesday, October 12, 2016 5:59:41 PM CEST Jeremy McNicoll wrote:
>> > On 2016-10-12 3:39 AM, Arnd Bergmann wrote:
>> > > On Tuesday, October 11, 2016 7:41:22 PM CEST Rob Herring wrote:
>> > >> On Sat, Oct 1, 2016 at 9:38 PM, Jeremy McNicoll <jmcnicol@redhat.com> wrote:
>> > >>> These non standard DT entries need to be cast aside as to not
>> > >>> pollute the main device tree bindings.  Without these essential
>> > >>> DT items the bootloader/LK will not pass control over to the kernel
>> > >>> and thus never boot.
>> > >>
>> > >> I discussed this with Stephen recently. I'm okay with leaving these on
>> > >> boards that have no chance of getting updated bootloaders to use the
>> > >> compatible string instead. Having to use dtbTool is far worse than a
>> > >> couple of extra properties IMO. I reserve the right to complain if new
>> > >> stuff continues to use these though.
>> > >>
>> > >>> Signed-off-by: Jeremy McNicoll <jeremymc@redhat.com>
>> > >>> ---
>> > >>>  .../arm64/boot/dts/qcom/msm8994-angler-rev-101.dts |  1 -
>> > >>>  arch/arm64/boot/dts/qcom/msm8994.dtsi              |  3 +--
>> > >>>  .../boot/dts/qcom/nexus6p_bootloader_bits.dtsi     | 24 ++++++++++++++++++++++
>> > >>
>> > >> Just put this into the board file rather than yet another include.
>> > >
>> > > The suggestion that I had was to have two .dts files: the normal
>> > > one without these properties, and another .dts file including the
>> > > first but adding these three for compatibility with the legacy
>> > > bootloaders.
>> > >
>> >
>> (sorry for the late reply, I thought I had replied already but
>> couldn't find that in the archives when I saw I still had this
>> reply open)
>>
>> > So I did it backwards from what you had suggested?
>> > Based on my discussion with, (cant seem to recall) my understanding
>> > was that we simply wanted to have these 3 bootloader specific entries
>> > in another file.
>>
>> Right
>>
>> What I would like to see here is two separate .dtb files, one
>> with the hack and one without it, so we have a migration path
>> for the machines that eventually get a boot loader with proper
>> DT support.
>
> So my main beef with this is that it is kind of onerous.  The machines that
> require this will never get a bootloader change.  So we'll be adding 2 dtb
> targets and only ever use one.
>
> It's much simpler in my opinion to just add the msm-id to the files that need it
> right now..... comment it with something like 'this is because of the Qualcomm
> braindead bootloader requirements' and move on.
>
> If there was any hope of a new bootloader for non-bleeding edge boards, I'd
> wholeheartedly agree with you Arnd.  But there isn't, and there won't be.

Makes sense to me for things like Nexus phones here. What about DB410
for example? Is there hope for a fix there? My bootloader is only a
couple of months old and needs the properties still.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Gross Oct. 20, 2016, 3:17 a.m. UTC | #8
On Wed, Oct 19, 2016 at 08:07:25PM -0500, Rob Herring wrote:
> On Wed, Oct 19, 2016 at 4:46 PM, Andy Gross <andy.gross@linaro.org> wrote:
> > On Wed, Oct 19, 2016 at 04:56:20PM +0200, Arnd Bergmann wrote:
> >> On Wednesday, October 12, 2016 5:59:41 PM CEST Jeremy McNicoll wrote:
> >> > On 2016-10-12 3:39 AM, Arnd Bergmann wrote:
> >> > > On Tuesday, October 11, 2016 7:41:22 PM CEST Rob Herring wrote:
> >> > >> On Sat, Oct 1, 2016 at 9:38 PM, Jeremy McNicoll <jmcnicol@redhat.com> wrote:
> >> > >>> These non standard DT entries need to be cast aside as to not
> >> > >>> pollute the main device tree bindings.  Without these essential
> >> > >>> DT items the bootloader/LK will not pass control over to the kernel
> >> > >>> and thus never boot.
> >> > >>
> >> > >> I discussed this with Stephen recently. I'm okay with leaving these on
> >> > >> boards that have no chance of getting updated bootloaders to use the
> >> > >> compatible string instead. Having to use dtbTool is far worse than a
> >> > >> couple of extra properties IMO. I reserve the right to complain if new
> >> > >> stuff continues to use these though.
> >> > >>
> >> > >>> Signed-off-by: Jeremy McNicoll <jeremymc@redhat.com>
> >> > >>> ---
> >> > >>>  .../arm64/boot/dts/qcom/msm8994-angler-rev-101.dts |  1 -
> >> > >>>  arch/arm64/boot/dts/qcom/msm8994.dtsi              |  3 +--
> >> > >>>  .../boot/dts/qcom/nexus6p_bootloader_bits.dtsi     | 24 ++++++++++++++++++++++
> >> > >>
> >> > >> Just put this into the board file rather than yet another include.
> >> > >
> >> > > The suggestion that I had was to have two .dts files: the normal
> >> > > one without these properties, and another .dts file including the
> >> > > first but adding these three for compatibility with the legacy
> >> > > bootloaders.
> >> > >
> >> >
> >> (sorry for the late reply, I thought I had replied already but
> >> couldn't find that in the archives when I saw I still had this
> >> reply open)
> >>
> >> > So I did it backwards from what you had suggested?
> >> > Based on my discussion with, (cant seem to recall) my understanding
> >> > was that we simply wanted to have these 3 bootloader specific entries
> >> > in another file.
> >>
> >> Right
> >>
> >> What I would like to see here is two separate .dtb files, one
> >> with the hack and one without it, so we have a migration path
> >> for the machines that eventually get a boot loader with proper
> >> DT support.
> >
> > So my main beef with this is that it is kind of onerous.  The machines that
> > require this will never get a bootloader change.  So we'll be adding 2 dtb
> > targets and only ever use one.
> >
> > It's much simpler in my opinion to just add the msm-id to the files that need it
> > right now..... comment it with something like 'this is because of the Qualcomm
> > braindead bootloader requirements' and move on.
> >
> > If there was any hope of a new bootloader for non-bleeding edge boards, I'd
> > wholeheartedly agree with you Arnd.  But there isn't, and there won't be.
> 
> Makes sense to me for things like Nexus phones here. What about DB410
> for example? Is there hope for a fix there? My bootloader is only a
> couple of months old and needs the properties still.

There won't be a fix for the 410c.  We barely got them to respin to use PSCI.
Stephen can correct me if I am wrong on this.

If this is fixed, it would be 8996+.  If..........

So this means introducing the msm-id's for the boards that currently require it,
and for the boards that will require it in the future.  And this would stay in
effect until the bootloader is able to parse the compatible strings or figure
this out without the msm-ids.

Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Andersson Oct. 21, 2016, 7:44 p.m. UTC | #9
On Wed 19 Oct 20:17 PDT 2016, Andy Gross wrote:

> On Wed, Oct 19, 2016 at 08:07:25PM -0500, Rob Herring wrote:
[..]
> > Makes sense to me for things like Nexus phones here. What about DB410
> > for example? Is there hope for a fix there? My bootloader is only a
> > couple of months old and needs the properties still.
> 
> There won't be a fix for the 410c.  We barely got them to respin to use PSCI.
> Stephen can correct me if I am wrong on this.
> 
> If this is fixed, it would be 8996+.  If..........
> 
> So this means introducing the msm-id's for the boards that currently require it,
> and for the boards that will require it in the future.  And this would stay in
> effect until the bootloader is able to parse the compatible strings or figure
> this out without the msm-ids.
> 

But if the bootloader at any point in the future would support picking a
dtb by compatible strings instead of {msm,board,pmic}-id we wouldn't we
just be back to the ridiculous compatible strings that tipped over into
acceptance to these ids in the first place.

Or do we expect the boot loader to do a deep scan of the dtb to match on
multiple nodes from the tree?

Am I missing something here?

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd Oct. 21, 2016, 8:04 p.m. UTC | #10
On 10/21, Bjorn Andersson wrote:
> On Wed 19 Oct 20:17 PDT 2016, Andy Gross wrote:
> 
> > On Wed, Oct 19, 2016 at 08:07:25PM -0500, Rob Herring wrote:
> [..]
> > > Makes sense to me for things like Nexus phones here. What about DB410
> > > for example? Is there hope for a fix there? My bootloader is only a
> > > couple of months old and needs the properties still.
> > 
> > There won't be a fix for the 410c.  We barely got them to respin to use PSCI.
> > Stephen can correct me if I am wrong on this.
> > 
> > If this is fixed, it would be 8996+.  If..........
> > 
> > So this means introducing the msm-id's for the boards that currently require it,
> > and for the boards that will require it in the future.  And this would stay in
> > effect until the bootloader is able to parse the compatible strings or figure
> > this out without the msm-ids.
> > 
> 
> But if the bootloader at any point in the future would support picking a
> dtb by compatible strings instead of {msm,board,pmic}-id we wouldn't we
> just be back to the ridiculous compatible strings that tipped over into
> acceptance to these ids in the first place.
> 
> Or do we expect the boot loader to do a deep scan of the dtb to match on
> multiple nodes from the tree?

I'm pushing the bootloader team to do the deep scan of the dtb to
match up board compatible and pmic compatible strings so that we
don't have to keep these numbers around. Basically put what
dtbtool is doing into the bootloader so we don't have to post
process the dtb anymore. We're currently discussing how to
implement it and how to move the internal codebase to the new
scheme.

At least for 96boards I think we can update the lk bootloaders on
there to adopt this code. For other platforms like nexus though I
don't see a way we can update those bootloaders, and those
bootloaders require these properties exist in the dtbs, so we
should just throw the numbers into the dts files there and be
done with post processing. For bootloaders that require the QCDT
header, we'll have to keep running dtbtool there to generate the
header. Having the ids in the dts file or not doesn't really
matter there.
Arnd Bergmann Oct. 21, 2016, 8:25 p.m. UTC | #11
On Friday, October 21, 2016 1:04:09 PM CEST Stephen Boyd wrote:
> On 10/21, Bjorn Andersson wrote:
> > On Wed 19 Oct 20:17 PDT 2016, Andy Gross wrote:
> > 
> > > On Wed, Oct 19, 2016 at 08:07:25PM -0500, Rob Herring wrote:
> > [..]
> > > > Makes sense to me for things like Nexus phones here. What about DB410
> > > > for example? Is there hope for a fix there? My bootloader is only a
> > > > couple of months old and needs the properties still.
> > > 
> > > There won't be a fix for the 410c.  We barely got them to respin to use PSCI.
> > > Stephen can correct me if I am wrong on this.
> > > 
> > > If this is fixed, it would be 8996+.  If..........
> > > 
> > > So this means introducing the msm-id's for the boards that currently require it,
> > > and for the boards that will require it in the future.  And this would stay in
> > > effect until the bootloader is able to parse the compatible strings or figure
> > > this out without the msm-ids.
> > > 
> > 
> > But if the bootloader at any point in the future would support picking a
> > dtb by compatible strings instead of {msm,board,pmic}-id we wouldn't we
> > just be back to the ridiculous compatible strings that tipped over into
> > acceptance to these ids in the first place.
> > 
> > Or do we expect the boot loader to do a deep scan of the dtb to match on
> > multiple nodes from the tree?
> 
> I'm pushing the bootloader team to do the deep scan of the dtb to
> match up board compatible and pmic compatible strings so that we
> don't have to keep these numbers around. Basically put what
> dtbtool is doing into the bootloader so we don't have to post
> process the dtb anymore. We're currently discussing how to
> implement it and how to move the internal codebase to the new
> scheme.
> 
> At least for 96boards I think we can update the lk bootloaders on
> there to adopt this code. For other platforms like nexus though I
> don't see a way we can update those bootloaders, and those
> bootloaders require these properties exist in the dtbs, so we
> should just throw the numbers into the dts files there and be
> done with post processing. For bootloaders that require the QCDT
> header, we'll have to keep running dtbtool there to generate the
> header. Having the ids in the dts file or not doesn't really
> matter there.

I think part of the problem here is the way that the bootloader
expects multiple dtbs to be appended to the kernel binary, and
then pick one of them based on its contents. That doesn't really
change at all when changing the parser from looking at nonstandard
properties to looking at the compatible strings.

It still breaks the last-resort workaround for broken bootloaders
that we have in the form of appending the DT to the kernel
with CONFIG_ARM_APPENDED_DTB.

I think a better long-term strategy would be to make the bootloader
load the dtb separately from the kernel and finding the right file
using some information outside of the dtb. Ideally this is done
by storing all files on a file system that can also be mounted
to /boot, but there are probably other options that work equally well.

	Arnd 
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Andersson Oct. 21, 2016, 9:52 p.m. UTC | #12
On Fri 21 Oct 13:25 PDT 2016, Arnd Bergmann wrote:

> On Friday, October 21, 2016 1:04:09 PM CEST Stephen Boyd wrote:
> > On 10/21, Bjorn Andersson wrote:
> > > On Wed 19 Oct 20:17 PDT 2016, Andy Gross wrote:
> > > 
> > > > On Wed, Oct 19, 2016 at 08:07:25PM -0500, Rob Herring wrote:
> > > [..]
> > > > > Makes sense to me for things like Nexus phones here. What about DB410
> > > > > for example? Is there hope for a fix there? My bootloader is only a
> > > > > couple of months old and needs the properties still.
> > > > 
> > > > There won't be a fix for the 410c.  We barely got them to respin to use PSCI.
> > > > Stephen can correct me if I am wrong on this.
> > > > 
> > > > If this is fixed, it would be 8996+.  If..........
> > > > 
> > > > So this means introducing the msm-id's for the boards that currently require it,
> > > > and for the boards that will require it in the future.  And this would stay in
> > > > effect until the bootloader is able to parse the compatible strings or figure
> > > > this out without the msm-ids.
> > > > 
> > > 
> > > But if the bootloader at any point in the future would support picking a
> > > dtb by compatible strings instead of {msm,board,pmic}-id we wouldn't we
> > > just be back to the ridiculous compatible strings that tipped over into
> > > acceptance to these ids in the first place.
> > > 
> > > Or do we expect the boot loader to do a deep scan of the dtb to match on
> > > multiple nodes from the tree?
> > 
> > I'm pushing the bootloader team to do the deep scan of the dtb to
> > match up board compatible and pmic compatible strings so that we
> > don't have to keep these numbers around. Basically put what
> > dtbtool is doing into the bootloader so we don't have to post
> > process the dtb anymore. We're currently discussing how to
> > implement it and how to move the internal codebase to the new
> > scheme.
> > 
> > At least for 96boards I think we can update the lk bootloaders on
> > there to adopt this code. For other platforms like nexus though I
> > don't see a way we can update those bootloaders, and those
> > bootloaders require these properties exist in the dtbs, so we
> > should just throw the numbers into the dts files there and be
> > done with post processing. For bootloaders that require the QCDT
> > header, we'll have to keep running dtbtool there to generate the
> > header. Having the ids in the dts file or not doesn't really
> > matter there.
> 
> I think part of the problem here is the way that the bootloader
> expects multiple dtbs to be appended to the kernel binary, and
> then pick one of them based on its contents. That doesn't really
> change at all when changing the parser from looking at nonstandard
> properties to looking at the compatible strings.
> 

This is unrelated to appending images to the zImage. Qualcomm already
supports (their original approach) of storing the multiple DTBs in a
QCDT blob; which contains a header of {msm,board,pmic}-id and offsets to
each blob.

So in this case it's not the boot that parses the dtb, but rather the
tool packaging the factory images.

> It still breaks the last-resort workaround for broken bootloaders
> that we have in the form of appending the DT to the kernel
> with CONFIG_ARM_APPENDED_DTB.
> 

Yes, this is a likely reason to why Qualcomm didn't take that route,
until it showed up in Nexus 6 (and 5?).

> I think a better long-term strategy would be to make the bootloader
> load the dtb separately from the kernel and finding the right file
> using some information outside of the dtb. Ideally this is done
> by storing all files on a file system that can also be mounted
> to /boot, but there are probably other options that work equally well.
> 

This scheme exists so that you can have a single software image running
on multiple revisions of a SoC and PMIC, something that is essential for
both production and upgrade schemes.

No matter how you package your multiple dtbs there needs to be some sort
of identifier associated that the boot can use to pick the appropriate
image from the bunch.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Andersson Oct. 21, 2016, 10:42 p.m. UTC | #13
On Fri 21 Oct 13:04 PDT 2016, Stephen Boyd wrote:

> On 10/21, Bjorn Andersson wrote:
> > On Wed 19 Oct 20:17 PDT 2016, Andy Gross wrote:
> > 
> > > On Wed, Oct 19, 2016 at 08:07:25PM -0500, Rob Herring wrote:
> > [..]
> > > > Makes sense to me for things like Nexus phones here. What about DB410
> > > > for example? Is there hope for a fix there? My bootloader is only a
> > > > couple of months old and needs the properties still.
> > > 
> > > There won't be a fix for the 410c.  We barely got them to respin to use PSCI.
> > > Stephen can correct me if I am wrong on this.
> > > 
> > > If this is fixed, it would be 8996+.  If..........
> > > 
> > > So this means introducing the msm-id's for the boards that currently require it,
> > > and for the boards that will require it in the future.  And this would stay in
> > > effect until the bootloader is able to parse the compatible strings or figure
> > > this out without the msm-ids.
> > > 
> > 
> > But if the bootloader at any point in the future would support picking a
> > dtb by compatible strings instead of {msm,board,pmic}-id we wouldn't we
> > just be back to the ridiculous compatible strings that tipped over into
> > acceptance to these ids in the first place.
> > 
> > Or do we expect the boot loader to do a deep scan of the dtb to match on
> > multiple nodes from the tree?
> 
> I'm pushing the bootloader team to do the deep scan of the dtb to
> match up board compatible and pmic compatible strings so that we
> don't have to keep these numbers around. Basically put what
> dtbtool is doing into the bootloader so we don't have to post
> process the dtb anymore. We're currently discussing how to
> implement it and how to move the internal codebase to the new
> scheme.
> 

Based on the variations described in your "Document qcom board
compatible format" patch you would need to scan the DTB for:

* SoC, Platform type and Version
  All part of /compatible, so that's simple

* Memory size
  Look at second cell of /memory/reg ? Or just reject this variable?

* PMIC
  Find the qcom,spmi-pmic-arb and iterate over each child and match the
  compatible of with some predefined list. We need to add all version
  variations of these in the compatibles to make this work as well.

* Main storage technology
  Look for an active node compatible with qcom,ufshc and if not found
  fall back to expecting this was a eMMC only DTB?

* Display panel
  Find the qcom,mdss compatible, follow the qcom,mdp5 compatible child
  to extract ports/port@0 to get the of_graph handle to some connector
  node, then in ../port@1 we can find a phandle to the panel which we
  can find and then match against a predefined set of compatibles.

> At least for 96boards I think we can update the lk bootloaders on
> there to adopt this code. For other platforms like nexus though I
> don't see a way we can update those bootloaders, and those
> bootloaders require these properties exist in the dtbs, so we
> should just throw the numbers into the dts files there and be
> done with post processing. For bootloaders that require the QCDT
> header, we'll have to keep running dtbtool there to generate the
> header. Having the ids in the dts file or not doesn't really
> matter there.
> 

At the time we introduced the Xperia Z1 (Honami) our boot loader only
supported QCDT, so I experimented with a version of dtbTool that kept a
compatible to id table mapping hardcoded (very much like your existing
dtbTool). And as expected it turns into an unmaintainable mess to track
this information on the side.


I'm sorry, but to me that just sounds like a lot of work to find an
alternative to the functional and pragmatic solution that exists today,
just for the sake of hiding these non-standard ids in an even more
non-standard way.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd Oct. 22, 2016, 12:07 a.m. UTC | #14
On 10/21, Arnd Bergmann wrote:
> On Friday, October 21, 2016 1:04:09 PM CEST Stephen Boyd wrote:
> > I'm pushing the bootloader team to do the deep scan of the dtb to
> > match up board compatible and pmic compatible strings so that we
> > don't have to keep these numbers around. Basically put what
> > dtbtool is doing into the bootloader so we don't have to post
> > process the dtb anymore. We're currently discussing how to
> > implement it and how to move the internal codebase to the new
> > scheme.
> > 
> > At least for 96boards I think we can update the lk bootloaders on
> > there to adopt this code. For other platforms like nexus though I
> > don't see a way we can update those bootloaders, and those
> > bootloaders require these properties exist in the dtbs, so we
> > should just throw the numbers into the dts files there and be
> > done with post processing. For bootloaders that require the QCDT
> > header, we'll have to keep running dtbtool there to generate the
> > header. Having the ids in the dts file or not doesn't really
> > matter there.
> 
> I think part of the problem here is the way that the bootloader
> expects multiple dtbs to be appended to the kernel binary, and
> then pick one of them based on its contents. That doesn't really
> change at all when changing the parser from looking at nonstandard
> properties to looking at the compatible strings.
> 
> It still breaks the last-resort workaround for broken bootloaders
> that we have in the form of appending the DT to the kernel
> with CONFIG_ARM_APPENDED_DTB.

That can be "fixed" by having the bootloader use the single
appended DTB regardless of the properties existing or not. That's
a few lines of code to count the number of appended blobs and
then special case there being one.

> 
> I think a better long-term strategy would be to make the bootloader
> load the dtb separately from the kernel and finding the right file
> using some information outside of the dtb.

Can you please explain why that's a better long term strategy? So
far I've had a hard time selling this internally so I could use
the help to come up with a laundry list of reasons why this is a
better design than what we have today.

> Ideally this is done
> by storing all files on a file system that can also be mounted
> to /boot, but there are probably other options that work equally well.
> 

Some bootloaders *cough* LK *cough* aren't always able to read
filesystems. All they can do is read raw data from partitions.
That's probably why nobody has thought about reading files from
some place like /boot (which doesn't even exist in android)
because these bootloaders don't have filesystem support.

If the bootloader supports filesystems, then it would work to
encode the qcom,{msm-id,board-id,pmic-id} properties into some
long filename and then have the bootloader pick the right file
based on that. Fuzzy matching would be interesting, but it should
still work.
Stephen Boyd Oct. 22, 2016, 12:36 a.m. UTC | #15
On 10/21, Bjorn Andersson wrote:
> On Fri 21 Oct 13:04 PDT 2016, Stephen Boyd wrote:
> 
> > On 10/21, Bjorn Andersson wrote:
> > > On Wed 19 Oct 20:17 PDT 2016, Andy Gross wrote:
> > > 
> > > > On Wed, Oct 19, 2016 at 08:07:25PM -0500, Rob Herring wrote:
> > > [..]
> > > > > Makes sense to me for things like Nexus phones here. What about DB410
> > > > > for example? Is there hope for a fix there? My bootloader is only a
> > > > > couple of months old and needs the properties still.
> > > > 
> > > > There won't be a fix for the 410c.  We barely got them to respin to use PSCI.
> > > > Stephen can correct me if I am wrong on this.
> > > > 
> > > > If this is fixed, it would be 8996+.  If..........
> > > > 
> > > > So this means introducing the msm-id's for the boards that currently require it,
> > > > and for the boards that will require it in the future.  And this would stay in
> > > > effect until the bootloader is able to parse the compatible strings or figure
> > > > this out without the msm-ids.
> > > > 
> > > 
> > > But if the bootloader at any point in the future would support picking a
> > > dtb by compatible strings instead of {msm,board,pmic}-id we wouldn't we
> > > just be back to the ridiculous compatible strings that tipped over into
> > > acceptance to these ids in the first place.
> > > 
> > > Or do we expect the boot loader to do a deep scan of the dtb to match on
> > > multiple nodes from the tree?
> > 
> > I'm pushing the bootloader team to do the deep scan of the dtb to
> > match up board compatible and pmic compatible strings so that we
> > don't have to keep these numbers around. Basically put what
> > dtbtool is doing into the bootloader so we don't have to post
> > process the dtb anymore. We're currently discussing how to
> > implement it and how to move the internal codebase to the new
> > scheme.
> > 
> 
> Based on the variations described in your "Document qcom board
> compatible format" patch you would need to scan the DTB for:

I think you're looking at an outdated doc?

> 
> * SoC, Platform type and Version
>   All part of /compatible, so that's simple
> 
> * Memory size
>   Look at second cell of /memory/reg ? Or just reject this variable?

We dropped this one. I don't know if we'll have users so we
punted.

> 
> * PMIC
>   Find the qcom,spmi-pmic-arb and iterate over each child and match the
>   compatible of with some predefined list. We need to add all version
>   variations of these in the compatibles to make this work as well.

We find it based on aliases. usid0,2,4,6 are found from the
aliases node and then we look at the pmic compatible string. I've
been trying to keep people honest there and have them put the
actual compatible string for the pmic in the pmic node.

> 
> * Main storage technology
>   Look for an active node compatible with qcom,ufshc and if not found
>   fall back to expecting this was a eMMC only DTB?
> 
> * Display panel
>   Find the qcom,mdss compatible, follow the qcom,mdp5 compatible child
>   to extract ports/port@0 to get the of_graph handle to some connector
>   node, then in ../port@1 we can find a phandle to the panel which we
>   can find and then match against a predefined set of compatibles.

We punted on these ones too. I don't know if we care. The panel
sounds especially painful.

> 
> > At least for 96boards I think we can update the lk bootloaders on
> > there to adopt this code. For other platforms like nexus though I
> > don't see a way we can update those bootloaders, and those
> > bootloaders require these properties exist in the dtbs, so we
> > should just throw the numbers into the dts files there and be
> > done with post processing. For bootloaders that require the QCDT
> > header, we'll have to keep running dtbtool there to generate the
> > header. Having the ids in the dts file or not doesn't really
> > matter there.
> > 
> 
> At the time we introduced the Xperia Z1 (Honami) our boot loader only
> supported QCDT, so I experimented with a version of dtbTool that kept a
> compatible to id table mapping hardcoded (very much like your existing
> dtbTool). And as expected it turns into an unmaintainable mess to track
> this information on the side.
> 

I think msm-id and pmic-ids are fairly simple. The problems
really come from the slight board variants and how to handle
those. The scheme we have today with dtbtool attempts to keep
things simple but it may need to be more complicated on Honami. I
don't really have a problem continuing to update the tool with
new SoC and PMIC models, but I don't see much point in it now
(see below).

> 
> I'm sorry, but to me that just sounds like a lot of work to find an
> alternative to the functional and pragmatic solution that exists today,
> just for the sake of hiding these non-standard ids in an even more
> non-standard way.
> 

I agree. This has all been a lot of effort to figure out some
alternative to having the numbers in the dts files. I won't argue
that the numbers are pretty, or that they don't duplicate
information that's already there in some other form of board/pmic
compatible strings, but the truth is we have bootloaders that are
looking inside the dtbs for these magic numbers now. The approach
that Nexus devices take has been pushed as the only method of dtb
picking inside the company so QCDT support is going away soon.

So going through the work to post process and then add in the
properties that could have been added in the dts in the first
place seems like a big waste of time. I'd rather we just leave
the numbers there and everything just works. We do have the
possibility of two different dtbs aliasing on the qcom,board-id
property, but I'd rather just let that problem happen and rely on
users to only append dtbs with different ids to their kernel.
Bjorn Andersson Oct. 22, 2016, 1:14 a.m. UTC | #16
On Fri 21 Oct 17:07 PDT 2016, Stephen Boyd wrote:

> On 10/21, Arnd Bergmann wrote:
> > On Friday, October 21, 2016 1:04:09 PM CEST Stephen Boyd wrote:
> > > I'm pushing the bootloader team to do the deep scan of the dtb to
> > > match up board compatible and pmic compatible strings so that we
> > > don't have to keep these numbers around. Basically put what
> > > dtbtool is doing into the bootloader so we don't have to post
> > > process the dtb anymore. We're currently discussing how to
> > > implement it and how to move the internal codebase to the new
> > > scheme.
> > > 
> > > At least for 96boards I think we can update the lk bootloaders on
> > > there to adopt this code. For other platforms like nexus though I
> > > don't see a way we can update those bootloaders, and those
> > > bootloaders require these properties exist in the dtbs, so we
> > > should just throw the numbers into the dts files there and be
> > > done with post processing. For bootloaders that require the QCDT
> > > header, we'll have to keep running dtbtool there to generate the
> > > header. Having the ids in the dts file or not doesn't really
> > > matter there.
> > 
> > I think part of the problem here is the way that the bootloader
> > expects multiple dtbs to be appended to the kernel binary, and
> > then pick one of them based on its contents. That doesn't really
> > change at all when changing the parser from looking at nonstandard
> > properties to looking at the compatible strings.
> > 
> > It still breaks the last-resort workaround for broken bootloaders
> > that we have in the form of appending the DT to the kernel
> > with CONFIG_ARM_APPENDED_DTB.
> 
> That can be "fixed" by having the bootloader use the single
> appended DTB regardless of the properties existing or not. That's
> a few lines of code to count the number of appended blobs and
> then special case there being one.
> 

This is already the case on at least 8974; if the ids are present they
must be the right ones, otherwise it will just pick the dtb that's
there.

[..]
> > Ideally this is done
> > by storing all files on a file system that can also be mounted
> > to /boot, but there are probably other options that work equally well.
> > 
> 
> Some bootloaders *cough* LK *cough* aren't always able to read
> filesystems. All they can do is read raw data from partitions.
> That's probably why nobody has thought about reading files from
> some place like /boot (which doesn't even exist in android)
> because these bootloaders don't have filesystem support.
> 

Well, your version of LK has ext2 support - just not in the code path
that loads the kernel. But that's still only a different way to
represent what we today have in QCDT, it doesn't change anything related
to these ids.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm64/boot/dts/qcom/msm8994-angler-rev-101.dts b/arch/arm64/boot/dts/qcom/msm8994-angler-rev-101.dts
index 5cff29f..a0fb9c3 100644
--- a/arch/arm64/boot/dts/qcom/msm8994-angler-rev-101.dts
+++ b/arch/arm64/boot/dts/qcom/msm8994-angler-rev-101.dts
@@ -18,7 +18,6 @@ 
 / {
 	model = "HUAWEI MSM8994 ANGLER rev-1.01";
 	compatible = "qcom,msm8994";
-	qcom,board-id= <8026 0>;
 };
 
 / {
diff --git a/arch/arm64/boot/dts/qcom/msm8994.dtsi b/arch/arm64/boot/dts/qcom/msm8994.dtsi
index c95cb73..7fdda23 100644
--- a/arch/arm64/boot/dts/qcom/msm8994.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8994.dtsi
@@ -15,12 +15,11 @@ 
 
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/clock/qcom,gcc-msm8994.h>
+#include "nexus6p_bootloader_bits.dtsi"
 
 / {
 	model = "Qualcomm Technologies, Inc. MSM 8994";
 	compatible = "qcom,msm8994";
-	qcom,msm-id = <207 0x0>;
-	qcom,pmic-id = <0x10009 0x1000A 0x0 0x0>;
 	interrupt-parent = <&intc>;
 
 	#address-cells = <2>;
diff --git a/arch/arm64/boot/dts/qcom/nexus6p_bootloader_bits.dtsi b/arch/arm64/boot/dts/qcom/nexus6p_bootloader_bits.dtsi
new file mode 100644
index 0000000..c40618d
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/nexus6p_bootloader_bits.dtsi
@@ -0,0 +1,24 @@ 
+/*
+ * Copyright (c) 2016, Red Hat Inc.
+ * Author: Jeremy McNicoll
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+/* WARNING:  This file and associated DTS nodes are non-standard
+	and their use should be avoided.  If this code is referenced
+	in any way you put yourself at great risk of catching Bubonic
+	plague. */
+
+/ {
+	qcom,board-id = <8026 0>;
+	qcom,msm-id = <257 0>;
+	qcom,pmic-id = <0x10009 0x1000A 0x0 0x0>;
+};