Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42685 )
Change subject: soc/amd/common: Drop ACPIMMIO bank for SMBus device PCI config
......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42685/6/src/soc/amd/picasso/uart.c
File src/soc/amd/picasso/uart.c:
https://review.coreboot.org/c/coreboot/+/42685/6/src/soc/amd/picasso/uart.c…
PS6, Line 60: Help
> If psp verstage is the first code we can execute, we will want to be able to configure the UART and […]
D14F0 seems to only be for 1.8MHz and configuring legacy UART IO.
It's also confusing that we have PICASSO_UART_48MZ. https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/t…
As for the PSP, all console writes are done using system calls, so the PSP owns the console UART.
--
To view, visit https://review.coreboot.org/c/coreboot/+/42685
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5c8ce8de0a6ab0ed41e7e8a5980d0f0510aaa993
Gerrit-Change-Number: 42685
Gerrit-PatchSet: 6
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Comment-Date: Thu, 25 Jun 2020 19:52:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org>
Comment-In-Reply-To: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-MessageType: comment
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42685 )
Change subject: soc/amd/common: Drop ACPIMMIO bank for SMBus device PCI config
......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42685/6//COMMIT_MSG
Commit Message:
https://review.coreboot.org/c/coreboot/+/42685/6//COMMIT_MSG@7
PS6, Line 7: Drop ACPIMMIO ban
> Why do you want to drop it?
Why have it if PCI config write does the same thing, with less confusion? If PSP did map this particular ACPIMMIO bank this could have had some use.
https://review.coreboot.org/c/coreboot/+/42685/6//COMMIT_MSG@10
PS6, Line 10: PCI config write
> Sure: D14F0x0FC
No PCI config access from psp-verstage?
https://review.coreboot.org/c/coreboot/+/42685/6/src/soc/amd/picasso/uart.c
File src/soc/amd/picasso/uart.c:
https://review.coreboot.org/c/coreboot/+/42685/6/src/soc/amd/picasso/uart.c…
PS6, Line 60: Help
> > 0=use 48MHz as UART baud rate clock. 1=enable UART legacy mode to use 1.843MHz as baud rate clock. […]
If psp verstage is the first code we can execute, we will want to be able to configure the UART and baudrate. As long as there are no other relevant UART configuration bits inside D14F0, sm_pci_read/write() can be dropped..
But if there _are_ relevant UART enable bits there... PSP kernel should really provide either PCI configuration access or a service call to map this SM_PCI bank.
--
To view, visit https://review.coreboot.org/c/coreboot/+/42685
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5c8ce8de0a6ab0ed41e7e8a5980d0f0510aaa993
Gerrit-Change-Number: 42685
Gerrit-PatchSet: 6
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Comment-Date: Thu, 25 Jun 2020 19:44:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org>
Gerrit-MessageType: comment
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42708 )
Change subject: AGESA fam14: Use AMD_ACPIMMIO_GPIO_BASE_100
......................................................................
Patch Set 4:
(1 comment)
> Patch Set 3: Code-Review+1
>
> Could you please add a comment in src/soc/amd/common/block/include/amdblocks/acpimmio_map.h describing this only applies to certain families? That macro is not correct for all chipsets.
https://review.coreboot.org/c/coreboot/+/42708/4/src/soc/amd/common/block/i…
File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/42708/4/src/soc/amd/common/block/i…
PS4, Line 36: /* Family 14h or before */
> It's 100% of the discrete controller hubs, plus Kabini (Family 16h Models 00h-0Fh). […]
I think we could have done without end-of-line comment here, followup moves the banks' descriptions to this file. How about we interleave the full explanation about this GPIO_BASE_100 there?
--
To view, visit https://review.coreboot.org/c/coreboot/+/42708
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I29fbc82fffc69b864adb4ddbda1425db98e2e48a
Gerrit-Change-Number: 42708
Gerrit-PatchSet: 4
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-CC: Martin Roth <martinroth(a)google.com>
Gerrit-Comment-Date: Thu, 25 Jun 2020 19:18:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-MessageType: comment