Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't en MAX98357A in dt ......................................................................
mainboard/google/puff: Don't en MAX98357A in dt
Puff doesn't have this packed. Don't enable MAX98357A in the devicetree overrides so that the correct SSDT is generated.
BRANCH=none BUG=b:146519004 TEST=./util/abuild/abuild -p none -t google/hatch -x -a
Change-Id: I8e437d9cd5c4bf66d39e290cc9153e08e32a3389 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/variants/puff/overridetree.cb 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/37822/1
diff --git a/src/mainboard/google/hatch/variants/puff/overridetree.cb b/src/mainboard/google/hatch/variants/puff/overridetree.cb index d5e2e5a..1112495 100644 --- a/src/mainboard/google/hatch/variants/puff/overridetree.cb +++ b/src/mainboard/google/hatch/variants/puff/overridetree.cb @@ -95,6 +95,7 @@ end end #I2C #4 device pci 1e.3 off end # GSPI #1 + device pci 1f.3 off end # no max98357a packed. end
end
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37822
to look at the new patch set (#2).
Change subject: mainboard/google/puff: Don't en MAX98357A in dt ......................................................................
mainboard/google/puff: Don't en MAX98357A in dt
Puff doesn't have this packed. Don't enable MAX98357A in the devicetree overrides so that the correct SSDT is generated.
We have to do a little bit of magic here to deal with a devicetree compiler limitation when overriding. The issue is that the node override wont override the specified children in the base and so we must force the child to be detected as non-present.
BRANCH=none BUG=b:146519004 TEST=./util/abuild/abuild -p none -t google/hatch -x -a
Change-Id: I8e437d9cd5c4bf66d39e290cc9153e08e32a3389 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/variants/puff/overridetree.cb 1 file changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/37822/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't en MAX98357A in dt ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37822/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/37822/2/src/mainboard/google/hatch/... PS2, Line 102: # the children and so we must force the childs driver to 'childs' may be misspelled - perhaps 'children'?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't en MAX98357A in dt ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37822/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/37822/2/src/mainboard/google/hatch/... PS2, Line 102: # the children and so we must force the childs driver to
'childs' may be misspelled - perhaps 'children'?
?
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37822
to look at the new patch set (#3).
Change subject: mainboard/google/puff: Don't en MAX98357A in dt ......................................................................
mainboard/google/puff: Don't en MAX98357A in dt
Puff doesn't have this packed. Don't enable MAX98357A in the devicetree overrides so that the correct SSDT is generated.
We have to do a little bit of magic here to deal with a devicetree compiler limitation when overriding. The issue is that the node override wont override the specified children in the base and so we must force the child to be detected as non-present.
BRANCH=none BUG=b:146519004 TEST=./util/abuild/abuild -p none -t google/hatch -x -a
Change-Id: I8e437d9cd5c4bf66d39e290cc9153e08e32a3389 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/variants/puff/overridetree.cb 2 files changed, 10 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/37822/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't en MAX98357A in dt ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... PS3, Line 151: # the children and so we must force the childs driver to 'childs' may be misspelled - perhaps 'children'?
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't en MAX98357A in dt ......................................................................
Patch Set 3:
(6 comments)
https://review.coreboot.org/c/coreboot/+/37822/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37822/3//COMMIT_MSG@7 PS3, Line 7: en enable
https://review.coreboot.org/c/coreboot/+/37822/3//COMMIT_MSG@9 PS3, Line 9: doesn't have this packed does not have these speaker amps, so don't ...
https://review.coreboot.org/c/coreboot/+/37822/3//COMMIT_MSG@14 PS3, Line 14: base base -> baseboard (?)
https://review.coreboot.org/c/coreboot/+/37822/3//COMMIT_MSG@14 PS3, Line 14: wont won't
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... PS3, Line 6: DRIVERS_I2C_DA7219 What about this one? AFAICT, this is only enabled in kohaku's dt.
Perhaps it is cleaner to move both of these to the non-baseboards that need them.
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... PS3, Line 147: device pci 1f.3 on perhaps it would be cleaner to move this out of the baseboard and into the other variants?
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't en MAX98357A in dt ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... PS3, Line 6: DRIVERS_I2C_DA7219
What about this one? […]
Or maybe just leave these enabled? Does it really matter if they are enabled in KConfig but not used via dt?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't en MAX98357A in dt ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/37822/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37822/3//COMMIT_MSG@7 PS3, Line 7: en
enable
I just did this to keep the short msg short, however done.
https://review.coreboot.org/c/coreboot/+/37822/3//COMMIT_MSG@9 PS3, Line 9: doesn't have this packed
does not have these speaker amps, so don't ...
Done
https://review.coreboot.org/c/coreboot/+/37822/3//COMMIT_MSG@14 PS3, Line 14: wont
won't
Done
https://review.coreboot.org/c/coreboot/+/37822/3//COMMIT_MSG@14 PS3, Line 14: base
base -> baseboard (?)
Done
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... PS3, Line 6: DRIVERS_I2C_DA7219
Or maybe just leave these enabled? Does it really matter if they are enabled in KConfig but not use […]
I assume you are referring to the line before?
It MAX98.. allegedly does get it's fill_ssdt fn ptr called if any devicetree bits are in the baseboard dt config because devicetree override doesn't properly override children.
If we want to go that route I short revive the earlier patch that moves it completely out of the baseboard however that is a little more invasive.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37822
to look at the new patch set (#4).
Change subject: mainboard/google/puff: Don't enable MAX98357A in dt ......................................................................
mainboard/google/puff: Don't enable MAX98357A in dt
Puff not have these speaker amps, so don't enable MAX98357A in the devicetree overrides so that the correct SSDT is generated.
We have to do a little bit of magic here to deal with a devicetree compiler limitation when overriding. The issue is that the node override won't override the specified children in the baseboard and so we must force the child to be detected as non-present.
BRANCH=none BUG=b:146519004 TEST=./util/abuild/abuild -p none -t google/hatch -x -a
Change-Id: I8e437d9cd5c4bf66d39e290cc9153e08e32a3389 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/variants/puff/overridetree.cb 2 files changed, 10 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/37822/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't enable MAX98357A in dt ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37822/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/37822/4/src/mainboard/google/hatch/... PS4, Line 184: # the children and so we must force the childs driver to 'childs' may be misspelled - perhaps 'children'?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't enable MAX98357A in dt ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... PS3, Line 6: DRIVERS_I2C_DA7219
I assume you are referring to the line before? […]
Sorry I think your *also* talking about the da7219 here as well.. umm that was confusing, well that isn't a concern here as you rightly pointed out that isn't enumerated in the baseboard dt and so is out of scope of this patch.
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't enable MAX98357A in dt ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... PS3, Line 6: DRIVERS_I2C_DA7219
Sorry I think your *also* talking about the da7219 here as well.. […]
ok... iiuc, we need to remove the select for PUFF b/c there is no clean way to completely remove it from puff's dt?
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37822
to look at the new patch set (#5).
Change subject: mainboard/google/puff: Don't enable MAX98357A in dt ......................................................................
mainboard/google/puff: Don't enable MAX98357A in dt
Puff not have these speaker amps, so don't enable MAX98357A in the devicetree overrides so that the correct SSDT is generated.
We have to do a little bit of magic here to deal with a devicetree compiler limitation when overriding. The issue is that the node override won't override the specified children in the baseboard and so we must force the child to be detected as non-present.
BRANCH=none BUG=b:146519004 TEST=./util/abuild/abuild -p none -t google/hatch -x -a
Change-Id: I8e437d9cd5c4bf66d39e290cc9153e08e32a3389 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/variants/puff/overridetree.cb 2 files changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/37822/5
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't enable MAX98357A in dt ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... PS3, Line 6: DRIVERS_I2C_DA7219
ok... […]
Yes, follow up as discussed offline. That is more invasive and so we keep this commit scoped to resolving the immediate issue with puff specifically.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't enable MAX98357A in dt ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... PS3, Line 147: device pci 1f.3 on
perhaps it would be cleaner to move this out of the baseboard and into the other variants?
I agree.
https://review.coreboot.org/c/coreboot/+/37822/3/src/mainboard/google/hatch/... PS3, Line 151: # the children and so we must force the childs driver to
'childs' may be misspelled - perhaps 'children'?
Ack
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't enable MAX98357A in dt ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37822/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37822/5//COMMIT_MSG@9 PS5, Line 9: not does not
https://review.coreboot.org/c/coreboot/+/37822/5//COMMIT_MSG@10 PS5, Line 10: devicetree overrides so that the correct SSDT is generated. so don't enable the MAX98357A driver in Kconfig.
https://review.coreboot.org/c/coreboot/+/37822/5//COMMIT_MSG@12 PS5, Line 12: We have to do a little bit of magic here to deal with a devicetree This paragraph doesn't apply now; but you could still say that we are disabling the driver to work around not being able to disable the dt node via override.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37822
to look at the new patch set (#6).
Change subject: mainboard/google/puff: Don't enable MAX98357A in dt ......................................................................
mainboard/google/puff: Don't enable MAX98357A in dt
Puff does not have these speaker amps, so don't enable the MAX98357A driver in Kconfig so that the correct SSDT is generated.
BRANCH=none BUG=b:146519004 TEST=./util/abuild/abuild -p none -t google/hatch -x -a
Change-Id: I8e437d9cd5c4bf66d39e290cc9153e08e32a3389 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/variants/puff/overridetree.cb 2 files changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/37822/6
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't enable MAX98357A in dt ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37822/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37822/5//COMMIT_MSG@9 PS5, Line 9: not
does not
Done
https://review.coreboot.org/c/coreboot/+/37822/5//COMMIT_MSG@10 PS5, Line 10: devicetree overrides so that the correct SSDT is generated.
so don't enable the MAX98357A driver in Kconfig.
Done
https://review.coreboot.org/c/coreboot/+/37822/5//COMMIT_MSG@12 PS5, Line 12: We have to do a little bit of magic here to deal with a devicetree
This paragraph doesn't apply now; but you could still say that we are disabling the driver to work a […]
Done
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't enable MAX98357A in dt ......................................................................
Patch Set 6: Code-Review+2
Hello build bot (Jenkins), Daniel Kurtz,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37822
to look at the new patch set (#7).
Change subject: mainboard/google/puff: Don't enable MAX98357A in dt ......................................................................
mainboard/google/puff: Don't enable MAX98357A in dt
Puff not have these speaker amps, so don't enable MAX98357A in the devicetree overrides so that the correct SSDT is generated.
We have to do a little bit of magic here to deal with a devicetree compiler limitation when overriding. The issue is that the node override won't override the specified children in the baseboard and so we must force the child to be detected as non-present.
BRANCH=none BUG=b:146519004 TEST=./util/abuild/abuild -p none -t google/hatch -x -a
Change-Id: I8e437d9cd5c4bf66d39e290cc9153e08e32a3389 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/variants/puff/overridetree.cb 2 files changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/37822/7
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't enable MAX98357A in dt ......................................................................
Patch Set 7:
(1 comment)
CB:37909 undoes what is done in this CL. Can we just drop this?
https://review.coreboot.org/c/coreboot/+/37822/7/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:
https://review.coreboot.org/c/coreboot/+/37822/7/src/mainboard/google/hatch/... PS7, Line 5: if !BOARD_GOOGLE_PUFF This should not be required. The linker drops unused sections.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't enable MAX98357A in dt ......................................................................
Patch Set 7:
(1 comment)
Patch Set 7:
(1 comment)
CB:37909 undoes what is done in this CL. Can we just drop this?
This fixes an immediate issue while the follow up is the proposed 'proper fix' which is more invasive. As I explained to Dan already, here we fix the issue while in the follow up we propose the more long term maintainable strategy on detailing with the current assumptions in baseboard.
https://review.coreboot.org/c/coreboot/+/37822/7/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:
https://review.coreboot.org/c/coreboot/+/37822/7/src/mainboard/google/hatch/... PS7, Line 5: if !BOARD_GOOGLE_PUFF
This should not be required. The linker drops unused sections.
That's incorrect, and if you see the history of Dan asking me to change the commit message from before may give insight.
The issue here is that the baseboard is defining a devicetree chip node and this is why it gets instantiated. The devil is in the detail here
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't enable MAX98357A in dt ......................................................................
Patch Set 7:
(1 comment)
Patch Set 7:
(1 comment)
Patch Set 7:
(1 comment)
CB:37909 undoes what is done in this CL. Can we just drop this?
This fixes an immediate issue while the follow up is the proposed 'proper fix' which is more invasive. As I explained to Dan already, here we fix the issue while in the follow up we propose the more long term maintainable strategy on detailing with the current assumptions in baseboard.
Understood. But, both changes are going to effectively land at the same time and since you already have a solution in CB:37909 which is right long term fix, having this separate short-lived change seems unnecessary.
https://review.coreboot.org/c/coreboot/+/37822/7/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:
https://review.coreboot.org/c/coreboot/+/37822/7/src/mainboard/google/hatch/... PS7, Line 5: if !BOARD_GOOGLE_PUFF
That's incorrect, and if you see the history of Dan asking me to change the commit message from before may give insight.
I was referring to your entire patch series. I agree that the devicetree chip node needs to be removed from baseboard/devicetree.cb. But once that is done, there is no need to get rid of this driver selection from this Kconfig file. For puff variant, this driver would go unused and hence won't cause any side-effects for Puff.
To summarize, I think the changes that are really required are: 1. Get rid of max98537a device node from baseboard/devicetree.cb 2. Add max98537a device node to each variant overridetree.cb except Puff
Rest can still remain the same.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't enable MAX98357A in dt ......................................................................
Patch Set 7:
Patch Set 7:
(1 comment)
Patch Set 7:
(1 comment)
Patch Set 7:
(1 comment)
CB:37909 undoes what is done in this CL. Can we just drop this?
This fixes an immediate issue while the follow up is the proposed 'proper fix' which is more invasive. As I explained to Dan already, here we fix the issue while in the follow up we propose the more long term maintainable strategy on detailing with the current assumptions in baseboard.
Understood. But, both changes are going to effectively land at the same time and since you already have a solution in CB:37909 which is right long term fix, having this separate short-lived change seems unnecessary.
More than happy to drop the short term fix for the long term refactor that fixes the issue provided you are happy with the invasiveness. I am just trying to get the issues fixes while at the same time put effort into making things cleaner for the long term.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't enable MAX98357A in dt ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37822/7/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:
https://review.coreboot.org/c/coreboot/+/37822/7/src/mainboard/google/hatch/... PS7, Line 5: if !BOARD_GOOGLE_PUFF
That's incorrect, and if you see the history of Dan asking me to change the commit message from be […]
I can go in that direction and lean on the linker more.
Edward O'Callaghan has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37822 )
Change subject: mainboard/google/puff: Don't enable MAX98357A in dt ......................................................................
Abandoned
no longer needed
-
build bot (Jenkins) (Code Review) -
Daniel Kurtz (Code Review)
-
Edward O'Callaghan (Code Review)
-
Furquan Shaikh (Code Review)