Change in ...coreboot[master]: sb/intel/common/firmware: Don't call GbE binary `firmware`

[Nico Huber (Code Review)]

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31108

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

sb/intel/common/firmware: Don't call GbE binary `firmware`

Unless things changed considerably, this file doesn't contain any
firmware. It is merely replacing a configuration EEPROM for the MAC
address etc. So don't call it firmware.

Change-Id: Ife6190639e7f05da2cb6eddeb1b0db0e8ffc8e6e
Signed-off-by: Nico Huber 
---
M src/southbridge/intel/common/firmware/Kconfig
1 file changed, 5 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/31108/1

diff --git a/src/southbridge/intel/common/firmware/Kconfig b/src/southbridge/intel/common/firmware/Kconfig
index ba54fcd..31a3df3 100644
--- a/src/southbridge/intel/common/firmware/Kconfig
+++ b/src/southbridge/intel/common/firmware/Kconfig
@@ -111,15 +111,15 @@
    def_bool n

config HAVE_GBE_BIN
-	bool "Add gigabit ethernet firmware"
+	bool "Add gigabit ethernet configuration"
    depends on HAVE_IFD_BIN && MAINBOARD_USES_IFD_GBE_REGION
    help
-	  The integrated gigabit ethernet controller needs a firmware file.
-	  Select this if you are going to use the PCH integrated controller
-	  and have the firmware.
+	  The integrated gigabit ethernet controller needs a configuration
+	  file. Select this if you are going to use the PCH integrated
+	  controller and want to add that file.

config GBE_BIN_PATH
-	string "Path to gigabit ethernet firmware"
+	string "Path to gigabit ethernet configuration"
    depends on HAVE_GBE_BIN
    default "3rdparty/blobs/mainboard/$(MAINBOARDDIR)/gbe.bin"

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31108 )

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1: Code-Review+2

[Felix Held (Code Review)]

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

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1: Code-Review+2

[Tristan Corrick (Code Review)]

Tristan Corrick has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31108 )

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1: Code-Review+2

[Subrata Banik (Code Review)]

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

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1: Code-Review-1

(1 comment)

https://review.coreboot.org/#/c/31108/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31108/1//COMMIT_MSG@8 
PS1, Line 8: 
i don't know from where you get this details about calling it gbe configuration. if you really keen not calling it "firmware" then use gbe "binary file".

I don't think its correct word to use configuration. do you have background what kind of configuration this binary has ?

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31108 )

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

https://review.coreboot.org/#/c/31108/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31108/1//COMMIT_MSG@8 
PS1, Line 8:
> i don't know from where you get this details about calling it gbe configuration. […]

The GbE binary holds the MAC address the integrated Ethernet uses. I would say that is more of a "configuration" (holds config values) than a "firmware" (which usually implies runnable code). I am afraid I don't understand your point.

[Subrata Banik (Code Review)]

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

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31108/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31108/1//COMMIT_MSG@8 
PS1, Line 8:
> The GbE binary holds the MAC address the integrated Ethernet uses. […]

can you please point me to the document where you get those details about referring as Gbe "configuration"

This is what I could see at Intel document, not sure if you have a better version ?

SPI programming guide tells me the purpose of this Gbe region that you have modified in this CL:

GbE – Location for Integrated LAN firmware and MAC address

[Subrata Banik (Code Review)]

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

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1: Code-Review-2

As per Intel document this region holds "Lan FW + MAC address".

do you think " Don't call GbE binary `firmware`" is justified here ?

[Subrata Banik (Code Review)]

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

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1: Code-Review-1

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31108 )

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1: Code-Review+2

[Subrata Banik (Code Review)]

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

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1: Code-Review-2

Giving -2 to make sure we have closure on document rather just merging this CL.
i don't see any single place at Intel any document tells me its configuration region.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31108 )

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review-2
>
> Giving -2 to make sure we have closure on document rather just merging this CL.
> i don't see any single place at Intel any document tells me its configuration region.

Regarding the aforementioned documents, care to give some links or names for them, please?

[Nico Huber (Code Review)]

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

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1:

(1 comment)
> As per Intel document this region holds "Lan FW + MAC address".
>
> do you think " Don't call GbE binary `firmware`" is justified here ?

Yes, pretty much justified.

As per Intel document, this region is described as follows:

The NVM image contains both static and dynamic data. The
    static data is the basic platform configuration, and
    includes OEM specific configuration bits as well as the
    unique Printed Circuit Board Assembly (PBA). The dynamic
    data holds the product’s Ethernet Individual Address (IA)
    and Checksum. This file can be created using a text editor.

Doesn't sound anything like firmware to me. That's from
"Intel ® 82579 Gigabit Ethernet PHY Datasheet v2.1" btw.

https://review.coreboot.org/#/c/31108/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31108/1//COMMIT_MSG@8 
PS1, Line 8:
> can you please point me to the document where you get those details about referring as Gbe "configur […]

Well, quoting undisclosed documentation isn't helpful. Also, I don't
care what a random document calls it. I'm sure even for Intel insiders
the binary situation can be confusing.

So let's name it what it is, please. Not what somebody else named it.

[Subrata Banik (Code Review)]

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

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1:
> (1 comment)
> > As per Intel document this region holds "Lan FW + MAC address".
> >
> > do you think " Don't call GbE binary `firmware`" is justified
>
> here ?
>
> Yes, pretty much justified.
>
> As per Intel document, this region is described as follows:
>
> The NVM image contains both static and dynamic data. The
> static data is the basic platform configuration, and
> includes OEM specific configuration bits as well as the
> unique Printed Circuit Board Assembly (PBA). The dynamic
> data holds the product’s Ethernet Individual Address (IA)
> and Checksum. This file can be created using a text editor.
>
> Doesn't sound anything like firmware to me. That's from
> "Intel ® 82579 Gigabit Ethernet PHY Datasheet v2.1" btw.

Its beyond what it sounds to you. 
Please give a line from document that tell you its not FW region.

I'm referring to doc 550696.

this Kconfig meant to pack GBE region using IFD tool and i could see all over its been refer as GBE FW. Hence there could be 2 things.

1. All document is wrong where it refers as gbe fw should be gbe configuration 
2. get clarification till the moment hold off this CL

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31108 )

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1:
> Patch Set 1:
>
> I'm referring to doc 550696.

Where can I find it?

[Subrata Banik (Code Review)]

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

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1:
> > Patch Set 1:
> >
> > I'm referring to doc 550696.
>
> Where can I find it?

if you have partner account with Intel, you can find the doc in CDI/IBP.

@Nico, just to be clear, i'm not against this CL. Its good that you find something that is challenging exist believe and documentation. So, lets take it gracefully rather rushing.

if you know someone at intel who can get those things clarification, i have no problem, you can get right comment and clarification and share here. Else i can volunteer here to get required details on this. please give me few working days
> > I'm sure even for Intel insiders the binary situation can be confusing.

I think you have more details than us, this is good sign. please continue.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31108 )

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1:
> Patch Set 1:
> > > Patch Set 1:
> > >
> > > I'm referring to doc 550696.
> >
> > Where can I find it?
>
> if you have partner account with Intel, you can find the doc in CDI/IBP.

Well, I do not think I have such an account, so no doc for me. Bummer.

[Nico Huber (Code Review)]

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

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1:
> if you know someone at intel who can get those things clarification

I don't. The people most experienced with Intel hardware and firmware
that I know are not working at Intel. The scores on this change pretty
much reflect that.
> i have no problem, you can get right comment and clarification and
> share here. Else i can volunteer here to get required details on this.
> please give me few working days

Please do so. With your -2, it is your obligation to give "concrete
recommendations for what could be changed[...]". Also, if somebody at
Intel tells you that it is firmware, please be prepared to justify
that.

[Subrata Banik (Code Review)]

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

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1:
> > if you know someone at intel who can get those things
>
> clarification
>
> I don't. The people most experienced with Intel hardware and
> firmware
> that I know are not working at Intel. The scores on this change
> pretty
> much reflect that.
> > i have no problem, you can get right comment and clarification
>
> and
> > share here. Else i can volunteer here to get required details on
>
> this.
> > please give me few working days
>
> Please do so. With your -2, it is your obligation to give "concrete
> recommendations for what could be changed[...]". Also, if somebody
> at
> Intel tells you that it is firmware, please be prepared to justify
> that.

Sure

[Paul Menzel (Code Review)]

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

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1: Code-Review+1

[Subrata Banik (Code Review)]

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

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 1: Code-Review+2

@Nico: We have confirmed that this is a "config" file and going forward we will try to update SPI programming guide with right naming convention for Gbe region.

Appreciate your work and please continue the same.

[Subrata Banik (Code Review)]

Subrata Banik has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31108 )

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

sb/intel/common/firmware: Don't call GbE binary `firmware`

Unless things changed considerably, this file doesn't contain any
firmware. It is merely replacing a configuration EEPROM for the MAC
address etc. So don't call it firmware.

Change-Id: Ife6190639e7f05da2cb6eddeb1b0db0e8ffc8e6e
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/31108
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Georgi 
Reviewed-by: Felix Held 
Reviewed-by: Tristan Corrick 
Reviewed-by: Angel Pons 
Reviewed-by: Arthur Heymans 
Reviewed-by: Paul Menzel 
Reviewed-by: Subrata Banik 
---
M src/southbridge/intel/common/firmware/Kconfig
1 file changed, 5 insertions(+), 5 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved
  Paul Menzel: Looks good to me, but someone else must approve
  Felix Held: Looks good to me, approved
  Arthur Heymans: Looks good to me, approved
  Subrata Banik: Looks good to me, approved
  Tristan Corrick: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/southbridge/intel/common/firmware/Kconfig b/src/southbridge/intel/common/firmware/Kconfig
index ba54fcd..31a3df3 100644
--- a/src/southbridge/intel/common/firmware/Kconfig
+++ b/src/southbridge/intel/common/firmware/Kconfig
@@ -111,15 +111,15 @@
    def_bool n

config HAVE_GBE_BIN
-	bool "Add gigabit ethernet firmware"
+	bool "Add gigabit ethernet configuration"
    depends on HAVE_IFD_BIN && MAINBOARD_USES_IFD_GBE_REGION
    help
-	  The integrated gigabit ethernet controller needs a firmware file.
-	  Select this if you are going to use the PCH integrated controller
-	  and have the firmware.
+	  The integrated gigabit ethernet controller needs a configuration
+	  file. Select this if you are going to use the PCH integrated
+	  controller and want to add that file.

config GBE_BIN_PATH
-	string "Path to gigabit ethernet firmware"
+	string "Path to gigabit ethernet configuration"
    depends on HAVE_GBE_BIN
    default "3rdparty/blobs/mainboard/$(MAINBOARDDIR)/gbe.bin"

[Nico Huber (Code Review)]

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

Change subject: sb/intel/common/firmware: Don't call GbE binary `firmware`
......................................................................

Patch Set 2:
> @Nico: We have confirmed that this is a "config" file and going forward we will try to update SPI programming guide with right naming convention for Gbe region.

That's good to hear. Thank you.