Change in coreboot[master]: sb/intel/bd82x6x: Add legacy mode support to SATA

[Felix Singer (Code Review)]

Hello Nico Huber,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: sb/intel/bd82x6x: Add legacy mode support to SATA
......................................................................

sb/intel/bd82x6x: Add legacy mode support to SATA

Legacy mode is supposed to help with IDE controller drivers that don't
know Intel's "native" IDE interface. We extend the `sata_mode` NVRAM
variable to provide the following choices:

* 0 "AHCI"        - AHCI interface
  * 1 "Compatible"  - Intel's "native" interface
  * 2 "Legacy"      - Legacy interface

Change-Id: I0e7a4befa02772f620602fa2a92c3583895d4d1c
Signed-off-by: Nico Huber 
---
M src/southbridge/intel/bd82x6x/sata.c
1 file changed, 7 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/39828/1

diff --git a/src/southbridge/intel/bd82x6x/sata.c b/src/southbridge/intel/bd82x6x/sata.c
index ba3630a..0f7a042 100644
--- a/src/southbridge/intel/bd82x6x/sata.c
+++ b/src/southbridge/intel/bd82x6x/sata.c
@@ -138,10 +138,13 @@
    	reg16 &= ~PCI_COMMAND_MEMORY;
    	pci_write_config16(dev, PCI_COMMAND, reg16);

-		/* Native mode capable on both primary and secondary (0xa)
-		 * or'ed with enabled (0x50) = 0xf
-		 */
-		pci_write_config8(dev, 0x09, 0x8f);
+		if (sata_mode == 1) {
+			/* Native mode on both primary and secondary. */
+			pci_or_config8(dev, 0x09, 0x05);
+		} else {
+			/* Legacy mode on both primary and secondary. */
+			pci_update_config8(dev, 0x09, ~0x05, 0x00);
+		}

/* Set Interrupt Line */
    	/* Interrupt Pin is set by D31IP.PIP */

[Paul Menzel (Code Review)]

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

Change subject: sb/intel/bd82x6x: Add legacy mode support to SATA
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
File src/southbridge/intel/bd82x6x/sata.c:

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
PS2, Line 146: 0x00
Excuse my ignorance, but doesn’t or’ing with 0x0 mean the value stay unchanged?

| 0 1
    or | 0 0
     ⇒ | 0 1

[Angel Pons (Code Review)]

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

Change subject: sb/intel/bd82x6x: Add legacy mode support to SATA
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
File src/southbridge/intel/bd82x6x/sata.c:

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
PS2, Line 146: 0x00
> Excuse my ignorance, but doesn’t or’ing with 0x0 mean the value stay unchanged? […]

Yes, but there's no `pci_and_config8` function.

[Patrick Rudolph (Code Review)]

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

Change subject: sb/intel/bd82x6x: Add legacy mode support to SATA
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
File src/southbridge/intel/bd82x6x/sata.c:

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
PS2, Line 143: 			pci_or_config8(dev, 0x09, 0x05);
is that register documented?
if so could you please use define for that

[Paul Menzel (Code Review)]

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

Change subject: sb/intel/bd82x6x: Add legacy mode support to SATA
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
File src/southbridge/intel/bd82x6x/sata.c:

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
PS2, Line 146: 0x00
> Yes, but there's no `pci_and_config8` function.

I do not understand. Isn’t it a no-op here?

[Nico Huber (Code Review)]

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

Change subject: sb/intel/bd82x6x: Add legacy mode support to SATA
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
File src/southbridge/intel/bd82x6x/sata.c:

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
PS2, Line 146: 0x00
> I do not understand. […]

If we were calling pci_or_config8(), but we aren't. I don't know where
the confusion comes from, maybe read the whole line again and don't focus
too much on the 0?

[Paul Menzel (Code Review)]

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

Change subject: sb/intel/bd82x6x: Add legacy mode support to SATA
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
File src/southbridge/intel/bd82x6x/sata.c:

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
PS2, Line 146: 0x00
> If we were calling pci_or_config8(), but we aren't. I don't know where […]

From the definition.

```
#if ENV_PCI_SIMPLE_DEVICE
static __always_inline
void pci_update_config8(pci_devfn_t dev, u16 reg, u8 mask, u8 or)
#else
static __always_inline
void pci_update_config8(const struct device *dev, u16 reg, u8 mask, u8 or)
#endif
{
        u8 reg8;

reg8 = pci_read_config8(dev, reg);
        reg8 &= mask;
        reg8 |= or;
        pci_write_config8(dev, reg, reg8);
}
```

[Paul Menzel (Code Review)]

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

Change subject: sb/intel/bd82x6x: Add legacy mode support to SATA
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
File src/southbridge/intel/bd82x6x/sata.c:

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
PS2, Line 146: 0x00
> From the definition. […]

Ok, it sets the values “not covered by the mask” to 0. From the function name, I’d would have expected those to stay unchanged.

[Angel Pons (Code Review)]

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

Change subject: sb/intel/bd82x6x: Add legacy mode support to SATA
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
File src/southbridge/intel/bd82x6x/sata.c:

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
PS2, Line 143: 			pci_or_config8(dev, 0x09, 0x05);
> is that register documented? […]

I can put names to all the registers in a separate patch. Would that be good?

[Felix Singer (Code Review)]

Hello build bot (Jenkins), Nico Huber, Paul Menzel, Angel Pons, Arthur Heymans, Patrick Rudolph,

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

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

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

Change subject: sb/intel/bd82x6x: Add legacy mode support to SATA
......................................................................

sb/intel/bd82x6x: Add legacy mode support to SATA

Legacy mode is supposed to help with IDE controller drivers that don't
know Intel's "native" IDE interface. We extend the `sata_mode` NVRAM
variable to provide the following choices:

* 0 "AHCI"        - AHCI interface
  * 1 "Compatible"  - Intel's "native" interface
  * 2 "Legacy"      - Legacy interface

Change-Id: I0e7a4befa02772f620602fa2a92c3583895d4d1c
Signed-off-by: Nico Huber 
---
M src/southbridge/intel/bd82x6x/sata.c
1 file changed, 60 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/39828/3

Change in coreboot[master]: sb/intel/bd82x6x/sata: Add legacy mode support

[Felix Singer (Code Review)]

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

Change subject: sb/intel/bd82x6x/sata: Add legacy mode support
......................................................................

Patch Set 9:

This change is ready for review.

Change in coreboot[master]: sb/intel/bd82x6x/sata: Add legacy mode support

[Nico Huber (Code Review)]

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

Change subject: sb/intel/bd82x6x/sata: Add legacy mode support
......................................................................

Patch Set 9:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
File src/southbridge/intel/bd82x6x/sata.c:

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
PS2, Line 143: 			pci_or_config8(dev, 0x09, 0x05);
> I can put names to all the registers in a separate patch. […]

Angel, feel free to, but please coordinate with Felix. PCI registers below 0x40
are standard registers. This one is PCI_CLASS_PROG and already defined.

For the bits I'd suggest
    #define PI_PRIMARY_MODE_NATIVE          (1 << 0)
    #define PI_SECONDARY_MODE_NATIVE        (1 << 2)

But I have to say, 0x05 seems more readable to me, fits a single line and one
has to look it up in the datasheet anyway to be sure.

Change in coreboot[master]: sb/intel/bd82x6x/sata: Add legacy mode support

[Angel Pons (Code Review)]

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

Change subject: sb/intel/bd82x6x/sata: Add legacy mode support
......................................................................

Patch Set 9: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
File src/southbridge/intel/bd82x6x/sata.c:

https://review.coreboot.org/c/coreboot/+/39828/2/src/southbridge/intel/bd82x... 
PS2, Line 143: 			pci_or_config8(dev, 0x09, 0x05);
> Angel, feel free to, but please coordinate with Felix. PCI registers below 0x40 […]

Agreed, won't block this change though.

Change in coreboot[master]: sb/intel/bd82x6x/sata: Add legacy mode support

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39828 )

Change subject: sb/intel/bd82x6x/sata: Add legacy mode support
......................................................................

sb/intel/bd82x6x/sata: Add legacy mode support

Legacy mode is supposed to help with IDE controller drivers that don't
know Intel's "native" IDE interface. We extend the `sata_mode` NVRAM
variable to provide the following choices:

* 0 "AHCI"        - AHCI interface
  * 1 "Compatible"  - Intel's "native" interface
  * 2 "Legacy"      - Legacy interface

Change-Id: I0e7a4befa02772f620602fa2a92c3583895d4d1c
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39828
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M src/southbridge/intel/bd82x6x/sata.c
1 file changed, 70 insertions(+), 7 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/src/southbridge/intel/bd82x6x/sata.c b/src/southbridge/intel/bd82x6x/sata.c
index c1b61f2..6c6e5be 100644
--- a/src/southbridge/intel/bd82x6x/sata.c
+++ b/src/southbridge/intel/bd82x6x/sata.c
@@ -28,6 +28,65 @@
    pci_write_config32(dev, SATA_SIRD, value);
 }

+static void sata_read_resources(struct device *dev)
+{
+	struct resource *res;
+
+	pci_dev_read_resources(dev);
+
+	/* Assign fixed resources for IDE legacy mode */
+
+	u8 sata_mode = 0;
+	get_option(&sata_mode, "sata_mode");
+	if (sata_mode != 2)
+		return;
+
+	res = find_resource(dev, PCI_BASE_ADDRESS_0);
+	if (res) {
+		res->base = 0x1f0;
+		res->size = 8;
+		res->flags = IORESOURCE_IO | IORESOURCE_ASSIGNED | IORESOURCE_FIXED;
+	}
+
+	res = find_resource(dev, PCI_BASE_ADDRESS_1);
+	if (res) {
+		res->base = 0x3f4;
+		res->size = 4;
+		res->flags = IORESOURCE_IO | IORESOURCE_ASSIGNED | IORESOURCE_FIXED;
+	}
+
+	res = find_resource(dev, PCI_BASE_ADDRESS_2);
+	if (res) {
+		res->base = 0x170;
+		res->size = 8;
+		res->flags = IORESOURCE_IO | IORESOURCE_ASSIGNED | IORESOURCE_FIXED;
+	}
+
+	res = find_resource(dev, PCI_BASE_ADDRESS_3);
+	if (res) {
+		res->base = 0x374;
+		res->size = 4;
+		res->flags = IORESOURCE_IO | IORESOURCE_ASSIGNED | IORESOURCE_FIXED;
+	}
+}
+
+static void sata_set_resources(struct device *dev)
+{
+	/* work around bug in pci_dev_set_resources(), it bails out on FIXED */
+	u8 sata_mode = 0;
+	get_option(&sata_mode, "sata_mode");
+	if (sata_mode == 2) {
+		unsigned int i;
+		for (i = PCI_BASE_ADDRESS_0; i <= PCI_BASE_ADDRESS_3; i += 4) {
+			struct resource *const res = find_resource(dev, i);
+			if (res)
+				res->flags &= ~IORESOURCE_FIXED;
+		}
+	}
+
+	pci_dev_set_resources(dev);
+}
+
 static void sata_init(struct device *dev)
 {
    u32 reg32;
@@ -112,17 +171,21 @@
    	write32(abar + 0xa0, reg32);
    } else {
            /* IDE */
-		printk(BIOS_DEBUG, "SATA: Controller in plain mode.\n");

/* Without AHCI BAR no memory decoding */
    	reg16 = pci_read_config16(dev, PCI_COMMAND);
    	reg16 &= ~PCI_COMMAND_MEMORY;
    	pci_write_config16(dev, PCI_COMMAND, reg16);

-		/* Native mode capable on both primary and secondary (0xa)
-		 * or'ed with enabled (0x50) = 0xf
-		 */
-		pci_write_config8(dev, 0x09, 0x8f);
+		if (sata_mode == 1) {
+			/* Native mode on both primary and secondary. */
+			pci_or_config8(dev, 0x09, 0x05);
+			printk(BIOS_DEBUG, "SATA: Controller in IDE compat mode.\n");
+		} else {
+			/* Legacy mode on both primary and secondary. */
+			pci_update_config8(dev, 0x09, ~0x05, 0x00);
+			printk(BIOS_DEBUG, "SATA: Controller in IDE legacy mode.\n");
+		}

/* Set timings */
    	pci_write_config16(dev, IDE_TIM_PRI, IDE_DECODE_ENABLE |
@@ -234,8 +297,8 @@
 };

static struct device_operations sata_ops = {
-	.read_resources		= pci_dev_read_resources,
-	.set_resources		= pci_dev_set_resources,
+	.read_resources		= sata_read_resources,
+	.set_resources		= sata_set_resources,
    .enable_resources	= pci_dev_enable_resources,
    .acpi_fill_ssdt		= sata_fill_ssdt,
    .init			= sata_init,