Attention is currently required from: Matt DeVillier, Tim Wawrzynczak, Sridhar Siricilla, Michael Niewöhner, Lean Sheng Tan, Patrick Rudolph, EricR Lai.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61650 )
Change subject: soc/intel/skylake: Add function to clear PMCON status bits
......................................................................
Patch Set 3:
(1 comment)
File src/soc/intel/skylake/pmutil.c:
https://review.coreboot.org/c/coreboot/+/61650/comment/9d945c96_560feca3
PS3, Line 269: void pmc_clear_pmcon_sts(void)
> > > > > this function is all the same for all platforms using common code, so why exactly do we have to copy-pasta it once again instead of finally moving it to soc/intel/common?
> > > >
> > > > There are few SOCs which eventually uses the IA PMC common code but the SOC EDS does have exact register details. I've tried creating the common code which eventually need to resolve the register definitions from SoC layer as GEN_PMCON_A offset is not same across all SoC. But due to documentation limitation issue, I just avoided pursuing that path.
> > >
> > > Well, when the GEN_PMCON_A offset differs, it can be defined in the SoC headers, but the code can still be common code. Same applies to the bit MS4V, which is the only bit offset needed to be known, right?
> >
> > Denverton
>
> Oh. Yay. Denverton. *sigh* I've had *some* "fun" with denverton, too, in this regard...
>
And on DNV, it's not the PCI MMIO space instead it's the PCI config space offset hence the access method is not exactly same with other SOCs.
>
> > Denverton uses PMC IA common code but GEN_PMCON_A is not there is soc/pm.h or soc/pmc.h. I ran into issue due to not having macro resolved by all SOC layer.
>
> ```
> $ grep -r PMCON_A src/soc/intel/
> src/soc/intel/denverton_ns/include/soc/pmc.h:#define PMC_GEN_PMCON_A 0xA0
It's in PCI config space hence one need to do PCI config read where other PMCON_A belongs to PCI MMIO. It's too much of guarding one need. Hence, I dropped the making common effort.
> ```
>
> Hmmm? 😊 You just have to rename it
>
> > I think there is one more SOC compliant about the same.
>
> Probably xeon_sp, or apl/glk...
>
> > Moving things common without any guard might need to have all SoC implements the required macro isn't it?
>
> Correct. Probably only APL/GLK has to be guarded, though, or... does it? PMCON_A seems to be PMCON_1 there.
yes, both are same but just EDS naming convention difference.
>
> >
> > >
> > > >
> > > > > btw. this probably should be done in soc_pmc_init instead of finalize.
> > > >
> > > > The reason this is being done as part of finalize is to ensure that prior booting to OS, those bits are cleared.
> > > >
> > > > Note: FSP can even request a global reset during FSP notify (which is post soc_pmc_init) hence, after global reset, this corresponding bit would have set. And, we might need to clear it prior to boot.
> > >
> > > Got it, thanks! Add a short comment for this, please.
I will, thanks,
> > >
> > > > There is no harm if you would like to call this more than once.
> > >
> > > Please don't 😄
> >
> > Although EDS says, writing 0 in this bit doesn't take any effect as it's RW/1C. But my comment to ensure we should remove the redundant to keep only one copy.
--
To view, visit https://review.coreboot.org/c/coreboot/+/61650
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie786e6ba2daf88accb5d70be33de0abe593f8c53
Gerrit-Change-Number: 61650
Gerrit-PatchSet: 3
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Comment-Date: Thu, 10 Feb 2022 21:05:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61451 )
Change subject: soc/intel/cannonlake: Use SBI msg to disable HECI1
......................................................................
Patch Set 8:
(1 comment)
Patchset:
PS8:
> presumably because heci1_disable() is called from SMM, but the call to disable via PMC is guarded with !ENV_SMM
Disabling via PMC doesn't need to execute in SMM mode, the SBI msg need to run in SBI mode.
Do you see any error with default selection aka SBI msg in SMM mode. I mean, if you have debug time and I don't have board handy.
--
To view, visit https://review.coreboot.org/c/coreboot/+/61451
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6882b619506d1bf4131f68c2c9a32ef4f7d6f6d7
Gerrit-Change-Number: 61451
Gerrit-PatchSet: 8
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Wonkyu Kim
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Comment-Date: Thu, 10 Feb 2022 21:01:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-MessageType: comment
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61451 )
Change subject: soc/intel/cannonlake: Use SBI msg to disable HECI1
......................................................................
Patch Set 8:
(1 comment)
Patchset:
PS8:
> > > this appears to cause a hang during finalization on google/hatch (akemi variant); reverting it allows normal booting
> >
> > Interesting, On CML platform, we had validated both more actually.
> >
> > Can you please select SOC_INTEL_COMMON_BLOCK_HECI1_DISABLE_USING_PMC_IPC instead HECI_SMM and see if any luck from SoC Kconfig.
>
> using SOC_INTEL_COMMON_BLOCK_HECI1_DISABLE_USING_PMC_IPC instead of HECI_DISABLE_USING_SMM fixes booting on hatch, but HECI1 is not disabled per lspci
I don't have the device handy but I will try to verify the code flow tomorrow.
--
To view, visit https://review.coreboot.org/c/coreboot/+/61451
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6882b619506d1bf4131f68c2c9a32ef4f7d6f6d7
Gerrit-Change-Number: 61451
Gerrit-PatchSet: 8
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Wonkyu Kim
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Comment-Date: Thu, 10 Feb 2022 20:58:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-MessageType: comment
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61451 )
Change subject: soc/intel/cannonlake: Use SBI msg to disable HECI1
......................................................................
Patch Set 8:
(1 comment)
Patchset:
PS8:
> > > this appears to cause a hang during finalization on google/hatch (akemi variant); reverting it a […]
presumably because heci1_disable() is called from SMM, but the call to disable via PMC is guarded with !ENV_SMM
--
To view, visit https://review.coreboot.org/c/coreboot/+/61451
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6882b619506d1bf4131f68c2c9a32ef4f7d6f6d7
Gerrit-Change-Number: 61451
Gerrit-PatchSet: 8
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Wonkyu Kim
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Comment-Date: Thu, 10 Feb 2022 20:58:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Subrata Banik, Matt DeVillier, Tim Wawrzynczak, Sridhar Siricilla, Lean Sheng Tan, Patrick Rudolph, EricR Lai.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61650 )
Change subject: soc/intel/skylake: Add function to clear PMCON status bits
......................................................................
Patch Set 3:
(2 comments)
File src/soc/intel/skylake/pmc.c:
https://review.coreboot.org/c/coreboot/+/61650/comment/19985283_1c47fe0c
PS3, Line 99: pci_or_config32(dev, GEN_PMCON_A, 0);
> > Sorry, not sure what you mean. […]
Understood, then this one should be dropped
File src/soc/intel/skylake/pmutil.c:
https://review.coreboot.org/c/coreboot/+/61650/comment/e2c11ac9_fc7f9ead
PS3, Line 269: void pmc_clear_pmcon_sts(void)
> > > > this function is all the same for all platforms using common code, so why exactly do we have to copy-pasta it once again instead of finally moving it to soc/intel/common?
> > >
> > > There are few SOCs which eventually uses the IA PMC common code but the SOC EDS does have exact register details. I've tried creating the common code which eventually need to resolve the register definitions from SoC layer as GEN_PMCON_A offset is not same across all SoC. But due to documentation limitation issue, I just avoided pursuing that path.
> >
> > Well, when the GEN_PMCON_A offset differs, it can be defined in the SoC headers, but the code can still be common code. Same applies to the bit MS4V, which is the only bit offset needed to be known, right?
>
> Denverton
Oh. Yay. Denverton. *sigh* I've had *some* "fun" with denverton, too, in this regard...
> Denverton uses PMC IA common code but GEN_PMCON_A is not there is soc/pm.h or soc/pmc.h. I ran into issue due to not having macro resolved by all SOC layer.
```
$ grep -r PMCON_A src/soc/intel/
src/soc/intel/denverton_ns/include/soc/pmc.h:#define PMC_GEN_PMCON_A 0xA0
```
Hmmm? 😊 You just have to rename it
> I think there is one more SOC compliant about the same.
Probably xeon_sp, or apl/glk...
> Moving things common without any guard might need to have all SoC implements the required macro isn't it?
Correct. Probably only APL/GLK has to be guarded, though, or... does it? PMCON_A seems to be PMCON_1 there.
>
> >
> > >
> > > > btw. this probably should be done in soc_pmc_init instead of finalize.
> > >
> > > The reason this is being done as part of finalize is to ensure that prior booting to OS, those bits are cleared.
> > >
> > > Note: FSP can even request a global reset during FSP notify (which is post soc_pmc_init) hence, after global reset, this corresponding bit would have set. And, we might need to clear it prior to boot.
> >
> > Got it, thanks! Add a short comment for this, please.
> >
> > > There is no harm if you would like to call this more than once.
> >
> > Please don't 😄
>
> Although EDS says, writing 0 in this bit doesn't take any effect as it's RW/1C. But my comment to ensure we should remove the redundant to keep only one copy.
--
To view, visit https://review.coreboot.org/c/coreboot/+/61650
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie786e6ba2daf88accb5d70be33de0abe593f8c53
Gerrit-Change-Number: 61650
Gerrit-PatchSet: 3
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Comment-Date: Thu, 10 Feb 2022 20:57:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61451 )
Change subject: soc/intel/cannonlake: Use SBI msg to disable HECI1
......................................................................
Patch Set 8:
(1 comment)
Patchset:
PS8:
> > this appears to cause a hang during finalization on google/hatch (akemi variant); reverting it allows normal booting
>
> Interesting, On CML platform, we had validated both more actually.
>
> Can you please select SOC_INTEL_COMMON_BLOCK_HECI1_DISABLE_USING_PMC_IPC instead HECI_SMM and see if any luck from SoC Kconfig.
using SOC_INTEL_COMMON_BLOCK_HECI1_DISABLE_USING_PMC_IPC instead of HECI_DISABLE_USING_SMM fixes booting on hatch, but HECI1 is not disabled per lspci
--
To view, visit https://review.coreboot.org/c/coreboot/+/61451
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6882b619506d1bf4131f68c2c9a32ef4f7d6f6d7
Gerrit-Change-Number: 61451
Gerrit-PatchSet: 8
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Wonkyu Kim
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Comment-Date: Thu, 10 Feb 2022 20:55:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Marshall Dawson, Felix Held.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54923 )
Change subject: soc/amd/common/block: Add irq_override flags
......................................................................
Patch Set 1:
(1 comment)
File src/soc/amd/common/block/include/amdblocks/chip.h:
https://review.coreboot.org/c/coreboot/+/54923/comment/50718dff_1cbe49d2
PS1, Line 45: uint8_t flags;
> wouldn't it be a good idea to use an enum here?
They are flags that are `|` together. I could add a `trigger` and `polarity` property instead, though that's a bigger refactor.
--
To view, visit https://review.coreboot.org/c/coreboot/+/54923
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: If6f44d4ef4b7bb7398e173f42269e4fa833b545c
Gerrit-Change-Number: 54923
Gerrit-PatchSet: 1
Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Thu, 10 Feb 2022 20:52:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-MessageType: comment
Attention is currently required from: Matt DeVillier, Tim Wawrzynczak, Sridhar Siricilla, Michael Niewöhner, Lean Sheng Tan, Patrick Rudolph, EricR Lai.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61650 )
Change subject: soc/intel/skylake: Add function to clear PMCON status bits
......................................................................
Patch Set 3:
(1 comment)
File src/soc/intel/skylake/pmc.c:
https://review.coreboot.org/c/coreboot/+/61650/comment/876cbe20_22d948ef
PS3, Line 99: pci_or_config32(dev, GEN_PMCON_A, 0);
> Sorry, not sure what you mean. pmc_soc_init runs way before finalize, so this one here clears bit 18 and it will be clear already in finalize. Correct me if I'm wrong, please
What I meant is like assume you have cleared the all rst bit as part of pmc.c file but FSP asked for a global reset at later stage, then we don't have a code in finalize.c which does the cleaning part again. Hence, I have requested to delay this clearing at the end of the boot, when we know, no one can issue global reset post that stage.
--
To view, visit https://review.coreboot.org/c/coreboot/+/61650
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie786e6ba2daf88accb5d70be33de0abe593f8c53
Gerrit-Change-Number: 61650
Gerrit-PatchSet: 3
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Comment-Date: Thu, 10 Feb 2022 20:42:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment
Attention is currently required from: Matt DeVillier, Tim Wawrzynczak, Sridhar Siricilla, Michael Niewöhner, Lean Sheng Tan, Patrick Rudolph, EricR Lai.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61650 )
Change subject: soc/intel/skylake: Add function to clear PMCON status bits
......................................................................
Patch Set 3:
(1 comment)
File src/soc/intel/skylake/pmutil.c:
https://review.coreboot.org/c/coreboot/+/61650/comment/3756084a_e87bd572
PS3, Line 269: void pmc_clear_pmcon_sts(void)
> > > this function is all the same for all platforms using common code, so why exactly do we have to copy-pasta it once again instead of finally moving it to soc/intel/common?
> >
> > There are few SOCs which eventually uses the IA PMC common code but the SOC EDS does have exact register details. I've tried creating the common code which eventually need to resolve the register definitions from SoC layer as GEN_PMCON_A offset is not same across all SoC. But due to documentation limitation issue, I just avoided pursuing that path.
>
> Well, when the GEN_PMCON_A offset differs, it can be defined in the SoC headers, but the code can still be common code. Same applies to the bit MS4V, which is the only bit offset needed to be known, right?
Denverton uses PMC IA common code but GEN_PMCON_A is not there is soc/pm.h or soc/pmc.h. I ran into issue due to not having macro resolved by all SOC layer. I think there is one more SOC compliant about the same. Moving things common without any guard might need to have all SoC implements the required macro isn't it?
>
> >
> > > btw. this probably should be done in soc_pmc_init instead of finalize.
> >
> > The reason this is being done as part of finalize is to ensure that prior booting to OS, those bits are cleared.
> >
> > Note: FSP can even request a global reset during FSP notify (which is post soc_pmc_init) hence, after global reset, this corresponding bit would have set. And, we might need to clear it prior to boot.
>
> Got it, thanks! Add a short comment for this, please.
>
> > There is no harm if you would like to call this more than once.
>
> Please don't 😄
Although EDS says, writing 0 in this bit doesn't take any effect as it's RW/1C. But my comment to ensure we should remove the redundant to keep only one copy.
--
To view, visit https://review.coreboot.org/c/coreboot/+/61650
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie786e6ba2daf88accb5d70be33de0abe593f8c53
Gerrit-Change-Number: 61650
Gerrit-PatchSet: 3
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Comment-Date: Thu, 10 Feb 2022 20:40:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment
Attention is currently required from: Subrata Banik, Matt DeVillier, Tim Wawrzynczak, Sridhar Siricilla, Lean Sheng Tan, Patrick Rudolph, EricR Lai.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61650 )
Change subject: soc/intel/skylake: Add function to clear PMCON status bits
......................................................................
Patch Set 3: -Code-Review
(2 comments)
File src/soc/intel/skylake/pmc.c:
https://review.coreboot.org/c/coreboot/+/61650/comment/8f4ab33d_aefb0784
PS3, Line 99: pci_or_config32(dev, GEN_PMCON_A, 0);
> > well... […]
Sorry, not sure what you mean. pmc_soc_init runs way before finalize, so this one here clears bit 18 and it will be clear already in finalize. Correct me if I'm wrong, please
File src/soc/intel/skylake/pmutil.c:
https://review.coreboot.org/c/coreboot/+/61650/comment/78c770ff_6e676718
PS3, Line 269: void pmc_clear_pmcon_sts(void)
> > this function is all the same for all platforms using common code, so why exactly do we have to copy-pasta it once again instead of finally moving it to soc/intel/common?
>
> There are few SOCs which eventually uses the IA PMC common code but the SOC EDS does have exact register details. I've tried creating the common code which eventually need to resolve the register definitions from SoC layer as GEN_PMCON_A offset is not same across all SoC. But due to documentation limitation issue, I just avoided pursuing that path.
Well, when the GEN_PMCON_A offset differs, it can be defined in the SoC headers, but the code can still be common code. Same applies to the bit MS4V, which is the only bit offset needed to be known, right?
>
> > btw. this probably should be done in soc_pmc_init instead of finalize.
>
> The reason this is being done as part of finalize is to ensure that prior booting to OS, those bits are cleared.
>
> Note: FSP can even request a global reset during FSP notify (which is post soc_pmc_init) hence, after global reset, this corresponding bit would have set. And, we might need to clear it prior to boot.
Got it, thanks! Add a short comment for this, please.
> There is no harm if you would like to call this more than once.
Please don't 😄
--
To view, visit https://review.coreboot.org/c/coreboot/+/61650
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie786e6ba2daf88accb5d70be33de0abe593f8c53
Gerrit-Change-Number: 61650
Gerrit-PatchSet: 3
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Comment-Date: Thu, 10 Feb 2022 20:32:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment