Change in coreboot[master]: soc/intel/cnl: GNA

[Felix Singer (Code Review)]

Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44739 )

Change subject: soc/intel/cnl: GNA
......................................................................

soc/intel/cnl: GNA

Change-Id: I921e29bfc77c8096519b52fec8bb6b2193006f99
Signed-off-by: Felix Singer 
---
M src/soc/intel/cannonlake/fsp_params.c
M src/soc/intel/cannonlake/include/soc/pci_devs.h
2 files changed, 7 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/44739/1

diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c
index 51ed2a8..95da672 100644
--- a/src/soc/intel/cannonlake/fsp_params.c
+++ b/src/soc/intel/cannonlake/fsp_params.c
@@ -643,6 +643,9 @@
    	params->PeiGraphicsPeimInit = 1;
    else
    	params->PeiGraphicsPeimInit = 0;
+
+	dev = pcidev_path_on_root(SA_DEVFN_GNA);
+	params->GnaEnable = is_dev_enabled(dev);
 }

/* Mainboard GPIO Configuration */
diff --git a/src/soc/intel/cannonlake/include/soc/pci_devs.h b/src/soc/intel/cannonlake/include/soc/pci_devs.h
index c9c51ca..fd43bbd 100644
--- a/src/soc/intel/cannonlake/include/soc/pci_devs.h
+++ b/src/soc/intel/cannonlake/include/soc/pci_devs.h
@@ -34,6 +34,10 @@
 #define  SA_DEVFN_IPU		PCI_DEVFN(SA_DEV_SLOT_IPU, 0)
 #define  SA_DEV_IPU		PCI_DEV(0, SA_DEV_SLOT_IPU, 0)

+#define SA_DEV_SLOT_GNA		0x08
+#define  SA_DEVFN_GNA		PCI_DEVFN(SA_DEV_SLOT_GNA, 0)
+#define  SA_DEV_GNA		PCI_DEV(0, SA_DEV_SLOT_GNA, 0)
+
 /* PCH Devices */
 #define PCH_DEV_SLOT_THERMAL	0x12
 #define  PCH_DEVFN_THERMAL	_PCH_DEVFN(THERMAL, 0)

Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling GNA device

[Felix Singer (Code Review)]

Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44739 )

Change subject: soc/intel/cnl: Allow enabling/disabling GNA device
......................................................................

Set Ready For Review

Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling GNA device

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44739 )

Change subject: soc/intel/cnl: Allow enabling/disabling GNA device
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling GNA device

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44739 )

Change subject: soc/intel/cnl: Allow enabling/disabling GNA device
......................................................................

Patch Set 3:

Sorry for being picky, but is this really a mainboard option?
Is there anything a mainboard design has to provide to make
it work? Or is it rather a user option if they want to make use
of that device?

Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling GNA device

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44739 )

Change subject: soc/intel/cnl: Allow enabling/disabling GNA device
......................................................................

Patch Set 3: Code-Review+1
> Patch Set 3:
>
> Sorry for being picky, but is this really a mainboard option?
> Is there anything a mainboard design has to provide to make
> it work? Or is it rather a user option if they want to make use
> of that device?

I'm inclined to agree here. I wasn't able to find out that much about the device but AFAIU this is dependent on board desin. Let's make that a Kconfig option instead.

Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling GNA device

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44739 )

Change subject: soc/intel/cnl: Allow enabling/disabling GNA device
......................................................................

Patch Set 3:
> Patch Set 3: Code-Review+1
> > Patch Set 3:
> >
> > Sorry for being picky, but is this really a mainboard option?
> > Is there anything a mainboard design has to provide to make
> > it work? Or is it rather a user option if they want to make use
> > of that device?
>
> I'm inclined to agree here. I wasn't able to find out that much about the device but AFAIU this is dependent on board desin. Let's make that a Kconfig option instead.

Erm. Well... I meant "this is NOT dependent on board design"

Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling GNA device

[Felix Singer (Code Review)]

Hello build bot (Jenkins), Nico Huber, Angel Pons, Michael Niewöhner, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/44739

to look at the new patch set (#12).

Change subject: soc/intel/cnl: Allow enabling/disabling GNA device
......................................................................

soc/intel/cnl: Allow enabling/disabling GNA device

Add GNA device definitions to pci_devs.h and allow enabling / disabling
the GNA device depending on the devicetree configuration.

GNA is the short term for "Gaussian mixture model and neural network
acceleration unit".

Change-Id: I921e29bfc77c8096519b52fec8bb6b2193006f99
Signed-off-by: Felix Singer 
---
M src/soc/intel/cannonlake/fsp_params.c
M src/soc/intel/cannonlake/include/soc/pci_devs.h
2 files changed, 7 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/44739/12

Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling GNA device

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44739 )

Change subject: soc/intel/cnl: Allow enabling/disabling GNA device
......................................................................

Patch Set 13:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44739/13/src/soc/intel/cannonlake/f... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/44739/13/src/soc/intel/cannonlake/f... 
PS13, Line 529: 	params->GnaEnable = is_dev_enabled(dev);
Do all affected boards mention this in their devicetree yet? If in doubt,
we can also wait for soc devtrees.

Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling GNA device

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44739 )

Change subject: soc/intel/cnl: Allow enabling/disabling GNA device
......................................................................

Patch Set 13:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44739/13/src/soc/intel/cannonlake/f... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/44739/13/src/soc/intel/cannonlake/f... 
PS13, Line 529: GnaEnable
same as chap; do we want to have that in the devtree? I think the gna device is user-specific, iow gets enabled when the user needs it, not when the soc just supports it

Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling GNA device

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44739 )

Change subject: soc/intel/cnl: Allow enabling/disabling GNA device
......................................................................

Patch Set 13:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44739/13/src/soc/intel/cannonlake/f... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/44739/13/src/soc/intel/cannonlake/f... 
PS13, Line 529: GnaEnable
> same as chap; do we want to have that in the devtree? I think the gna device is user-specific, iow g […]

I agree. This is nothing a developer can or should have to decide. I some-
times dream about having implicit NVRAM options to override such things
(i.e. the devicetree would only provide a default and NVRAM could override),
but we are not there yet ;)

Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling GNA device

[Michael Niewöhner (Code Review)]

Attention is currently required from:  Felix Singer, Nico Huber.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44739 )

Change subject: soc/intel/cnl: Allow enabling/disabling GNA device
......................................................................

Patch Set 13:

(1 comment)

File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/44739/comment/c0623e92_9fe3aa86 
PS13, Line 529: GnaEnable
> I agree. This is nothing a developer can or should have to decide. I some- […]

also see CB:44740

[XS] Change in coreboot[master]: soc/intel/cnl: Allow enabling/disabling GNA device

[Martin L Roth (Code Review)]

Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/44739?usp=email )

Change subject: soc/intel/cnl: Allow enabling/disabling GNA device
......................................................................

Abandoned

This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.