Change in ...coreboot[master]: intel/apollolake: Add IPU to disable_dev function

[Werner Zeh (Code Review)]

Werner Zeh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30992

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

intel/apollolake: Add IPU to disable_dev function

The SoC has an Image Processing Unit which is located on PCI 00:03.0.
There is a corresponding parameter for FSP which treats enable/disable
of this functionality (IpuEn). Add this device to the disable_dev()
function of the chip so that if this device is disabled in devicetree
the matching FSP parameter will be disabled as well.

Change-Id: I75444bf483de32ba641f76ca50e9744fdce2e726
Signed-off-by: Werner Zeh 
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/include/soc/pci_devs.h
2 files changed, 7 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/30992/1

diff --git a/src/soc/intel/apollolake/chip.c b/src/soc/intel/apollolake/chip.c
index b38265f..102ba34 100644
--- a/src/soc/intel/apollolake/chip.c
+++ b/src/soc/intel/apollolake/chip.c
@@ -482,6 +482,9 @@
    case PCH_DEVFN_SMBUS:
    	silconfig->SmbusEnable = 0;
    	break;
+	case SA_DEVFN_IPU:
+		silconfig->IpuEn = 0;
+		break;
    default:
    	printk(BIOS_WARNING, "PCI:%02x.%01x: Could not disable the device\n",
    		PCI_SLOT(dev->path.pci.devfn),
diff --git a/src/soc/intel/apollolake/include/soc/pci_devs.h b/src/soc/intel/apollolake/include/soc/pci_devs.h
index ad726f8..363301d 100644
--- a/src/soc/intel/apollolake/include/soc/pci_devs.h
+++ b/src/soc/intel/apollolake/include/soc/pci_devs.h
@@ -45,6 +45,10 @@
 #define  SA_DEVFN_IGD		_SA_DEVFN(IGD)
 #define  SA_DEV_IGD		_SA_DEV(IGD)

+#define SA_DEV_SLOT_IPU		0x03
+#define  SA_DEVFN_IPU		_SA_DEVFN(IPU)
+#define  SA_DEV_IPU		_SA_DEV(IPU)
+
 /* PCH Devices */

#define PCH_DEV_SLOT_NPK	0x00

[Werner Zeh (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

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

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

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

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

intel/apollolake: Add IPU to disable_dev function

The SoC has an Image Processing Unit which is located on PCI 00:03.0.
There is a corresponding parameter for FSP which treats enable/disable
of this functionality (IpuEn). Add this device to the disable_dev()
function of the chip so that if this device is disabled in devicetree
the matching FSP parameter will be disabled as well.
As this parameter is only valid for Apollo Lake, use the config switch
CONFIG_SOC_INTEL_GLK to disable this code if compiled not for Apollo
Lake. As this issue is regarding a missing structure member, this check
needs to be done on preprocessor level and not at runtime.

Change-Id: I75444bf483de32ba641f76ca50e9744fdce2e726
Signed-off-by: Werner Zeh 
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/include/soc/pci_devs.h
2 files changed, 10 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/30992/2

[Mario Scheithauer (Code Review)]

Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30992 )

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

Patch Set 2: Code-Review+1

[Mario Scheithauer (Code Review)]

Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30992 )

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

Patch Set 3: Code-Review+2

[Alex Thiessen (Code Review)]

Alex Thiessen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30992 )

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/30992/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/30992/3//COMMIT_MSG@10 
PS3, Line 10: treats enable/disable
handles enabling/disabling

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30992 )

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/30992/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/30992/3//COMMIT_MSG@10 
PS3, Line 10: treats enable/disable
> handles enabling/disabling

Ack

[Werner Zeh (Code Review)]

Hello Patrick Rudolph, Mario Scheithauer, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

intel/apollolake: Add IPU to disable_dev function

The SoC has an Image Processing Unit which is located on PCI 00:03.0.
There is a corresponding parameter for FSP which handles
enabling/disabling of this functionality (IpuEn). Add this device to
the disable_dev() function of the chip so that if this device is
disabled in devicetree the matching FSP parameter will be disabled as
well. As this parameter is only valid for Apollo Lake, use the config
switch CONFIG_SOC_INTEL_GLK to disable this code if compiled not for
Apollo Lake. As this issue is regarding a missing structure member,
this check needs to be done on preprocessor level and not at runtime.

Change-Id: I75444bf483de32ba641f76ca50e9744fdce2e726
Signed-off-by: Werner Zeh 
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/include/soc/pci_devs.h
2 files changed, 10 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/30992/4

[Alex Thiessen (Code Review)]

Alex Thiessen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30992 )

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

Patch Set 4: Code-Review+1

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30992 )

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

Patch Set 6: Code-Review+1

(1 comment)

https://review.coreboot.org/#/c/30992/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/30992/6//COMMIT_MSG@18 
PS6, Line 18: 
Tested how?

[Werner Zeh (Code Review)]

Hello Alex Thiessen, Patrick Rudolph, Mario Scheithauer, Paul Menzel, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

intel/apollolake: Add IPU to disable_dev function

The SoC has an Image Processing Unit which is located on PCI 00:03.0.
There is a corresponding parameter for FSP which handles
enabling/disabling of this functionality (IpuEn). Add this device to
the disable_dev() function of the chip so that if this device is
disabled in devicetree the matching FSP parameter will be disabled as
well. As this parameter is only valid for Apollo Lake, use the config
switch CONFIG_SOC_INTEL_GLK to disable this code if compiled not for
Apollo Lake. As this issue is regarding a missing structure member,
this check needs to be done on preprocessor level and not at runtime.

Test=Verified this function on mc_apl2.

Change-Id: I75444bf483de32ba641f76ca50e9744fdce2e726
Signed-off-by: Werner Zeh 
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/include/soc/pci_devs.h
2 files changed, 10 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/30992/7

[Nico Huber (Code Review)]

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

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

Patch Set 7: Code-Review+2

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30992 )

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/30992/7/src/soc/intel/apollolake/chip.c 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/#/c/30992/7/src/soc/intel/apollolake/chip.c@487 
PS7, Line 487: silconfig->IpuEn = 0;
i'm not sure if any APL design is using MIPI camera and internal IPU if yes then this patch might impact. if you want to make something specific for mainboard alone then i believe adding things in chip.h and devicetree.cb to disable should be good.

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30992 )

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/30992/7/src/soc/intel/apollolake/chip.c 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/#/c/30992/7/src/soc/intel/apollolake/chip.c@487 
PS7, Line 487: silconfig->IpuEn = 0;
> i'm not sure if any APL design is using MIPI camera and internal IPU if yes then this patch might im […]

If a design is using MIPI camera and/or IPU, then it will not disable PCI device 00:03.0 as it needs it. In this case this code path will not be reached. It will only be reached if devicetree disables PCI dev 00:03.0. And in this case I doubt that the usage of IPU is by intention.
So what we get here is just disabling IPU in FSP just for the case where the device is switched off in devicetree.

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30992 )

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

Patch Set 7: Code-Review+2

(1 comment)

https://review.coreboot.org/#/c/30992/7/src/soc/intel/apollolake/chip.c 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/#/c/30992/7/src/soc/intel/apollolake/chip.c@487 
PS7, Line 487: silconfig->IpuEn = 0;
> If a design is using MIPI camera and/or IPU, then it will not disable PCI device 00:03. […]

i thought this function is UPD assignment, bt after seeing function name its disable_dev.
make sense

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30992 )

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

Patch Set 7: -Code-Review

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30992 )

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/30992/7/src/soc/intel/apollolake/chip.c 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/#/c/30992/7/src/soc/intel/apollolake/chip.c@485 
PS7, Line 485: #if !IS_ENABLED(CONFIG_SOC_INTEL_GLK)
then why you need this check ?

[Nico Huber (Code Review)]

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

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/30992/7/src/soc/intel/apollolake/chip.c 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/#/c/30992/7/src/soc/intel/apollolake/chip.c@485 
PS7, Line 485: #if !IS_ENABLED(CONFIG_SOC_INTEL_GLK)
> then why you need this check ?

As the commit message tries to explain, the UPD header files of
APL and GLK are incompatible.

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30992 )

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/30992/7/src/soc/intel/apollolake/chip.c 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/#/c/30992/7/src/soc/intel/apollolake/chip.c@485 
PS7, Line 485: #if !IS_ENABLED(CONFIG_SOC_INTEL_GLK)
> As the commit message tries to explain, the UPD header files of […]

yes, but I assuming you wish to do the same for GLK as well then there should be an else case as well right ?

[Nico Huber (Code Review)]

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

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/30992/7/src/soc/intel/apollolake/chip.c 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/#/c/30992/7/src/soc/intel/apollolake/chip.c@485 
PS7, Line 485: #if !IS_ENABLED(CONFIG_SOC_INTEL_GLK)
> yes, but I assuming you wish to do the same for GLK as well then there should be an else case as wel […]

Well, both IpuEn and SaIpuEnable are marked as reserved in
the GLK header file. So there seems to be nothing that we
could put into an else path?

Also, now that I looked at it again, what is the difference
between IpuEn and SaIpuEnable?

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30992 )

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/30992/7/src/soc/intel/apollolake/chip.c 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/#/c/30992/7/src/soc/intel/apollolake/chip.c@485 
PS7, Line 485: #if !IS_ENABLED(CONFIG_SOC_INTEL_GLK)
> Well, both IpuEn and SaIpuEnable are marked as reserved in […]

can you please add Hannah to get some inside from APL and GLK

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/30992 )

Change subject: intel/apollolake: Add IPU to disable_dev function
......................................................................

intel/apollolake: Add IPU to disable_dev function

The SoC has an Image Processing Unit which is located on PCI 00:03.0.
There is a corresponding parameter for FSP which handles
enabling/disabling of this functionality (IpuEn). Add this device to
the disable_dev() function of the chip so that if this device is
disabled in devicetree the matching FSP parameter will be disabled as
well. As this parameter is only valid for Apollo Lake, use the config
switch CONFIG_SOC_INTEL_GLK to disable this code if compiled not for
Apollo Lake. As this issue is regarding a missing structure member,
this check needs to be done on preprocessor level and not at runtime.

Test=Verified this function on mc_apl2.

Change-Id: I75444bf483de32ba641f76ca50e9744fdce2e726
Signed-off-by: Werner Zeh 
Reviewed-on: https://review.coreboot.org/c/30992
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/include/soc/pci_devs.h
2 files changed, 10 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/src/soc/intel/apollolake/chip.c b/src/soc/intel/apollolake/chip.c
index 9912080..afbb45c 100644
--- a/src/soc/intel/apollolake/chip.c
+++ b/src/soc/intel/apollolake/chip.c
@@ -2,7 +2,7 @@
  * This file is part of the coreboot project.
  *
  * Copyright (C) 2015 - 2017 Intel Corp.
- * Copyright (C) 2017 - 2018 Siemens AG
+ * Copyright (C) 2017 - 2019 Siemens AG
  * (Written by Alexandru Gagniuc  for Intel Corp.)
  * (Written by Andrey Petrov  for Intel Corp.)
  *
@@ -522,6 +522,11 @@
    case PCH_DEVFN_SMBUS:
    	silconfig->SmbusEnable = 0;
    	break;
+#if !IS_ENABLED(CONFIG_SOC_INTEL_GLK)
+	case SA_DEVFN_IPU:
+		silconfig->IpuEn = 0;
+		break;
+#endif
    default:
    	printk(BIOS_WARNING, "PCI:%02x.%01x: Could not disable the device\n",
    		PCI_SLOT(dev->path.pci.devfn),
diff --git a/src/soc/intel/apollolake/include/soc/pci_devs.h b/src/soc/intel/apollolake/include/soc/pci_devs.h
index 5773a01..2edf6f4 100644
--- a/src/soc/intel/apollolake/include/soc/pci_devs.h
+++ b/src/soc/intel/apollolake/include/soc/pci_devs.h
@@ -44,6 +44,10 @@
 #define  SA_DEVFN_IGD		_SA_DEVFN(IGD)
 #define  SA_DEV_IGD		_SA_DEV(IGD)

+#define SA_DEV_SLOT_IPU		0x03
+#define  SA_DEVFN_IPU		_SA_DEVFN(IPU)
+#define  SA_DEV_IPU		_SA_DEV(IPU)
+
 /* PCH Devices */

#define PCH_DEV_SLOT_NPK	0x00