[PATCH] [RFC] v3: CS5536 cleanup

[Carl-Daniel Hailfinger]

Clean up Geode companion chip CS5536 code and improve VPCI hiding
debug message. This also eliminates a few dev_find_pci_device().

I'm not happy with the new code yet. hide_vpci() should have a
variant which takes a struct device *.

Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>

Index: LinuxBIOSv3-cs5536cleanup/southbridge/amd/cs5536/cs5536.c
===================================================================
--- LinuxBIOSv3-cs5536cleanup/southbridge/amd/cs5536/cs5536.c	(Revision 676)
+++ LinuxBIOSv3-cs5536cleanup/southbridge/amd/cs5536/cs5536.c	(Arbeitskopie)
@@ -175,8 +175,7 @@
 static void enable_ide_nand_flash_header(void)
 {
 	/* Tell VSA to use FLASH PCI header. Not IDE header. */
-	outl(0x80007A40, 0xCF8);
-	outl(0xDEADBEEF, 0xCFC);
+	hide_vpci(0x800079C4);
 }
 
 #define RTC_CENTURY	0x32
@@ -240,16 +239,13 @@
  *
  * @param sb Southbridge config structure.
  */
-static void uarts_init(struct southbridge_amd_cs5536_dts_config *sb)
+static void uarts_init(struct southbridge_amd_cs5536_dts_config *sb,
+			struct device *dev)
 {
 	struct msr msr;
 	u16 addr = 0;
 	u32 gpio_addr;
-	struct device *dev;
 
-	dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
-			      PCI_DEVICE_ID_AMD_CS5536_ISA, 0);
-
 	gpio_addr = pci_read_config32(dev, PCI_BASE_ADDRESS_1);
 	gpio_addr &= ~1;	/* Clear I/O bit */
 	printk(BIOS_DEBUG, "GPIO_ADDR: %08X\n", gpio_addr);
@@ -418,7 +414,7 @@
 {
 	u32 *bar;
 	struct msr msr;
-	struct device *dev;
+	struct device *dev, *otg_dev;
 
 	dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
 			      PCI_DEVICE_ID_AMD_CS5536_EHCI, 0);
@@ -440,9 +436,9 @@
 		*(bar + HCCPARAMS) = 0x00005012;
 	}
 
-	dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
+	otg_dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
 			      PCI_DEVICE_ID_AMD_CS5536_OTG, 0);
-	if (dev) {
+	if (otg_dev) {
 		bar = (u32 *) pci_read_config32(dev, PCI_BASE_ADDRESS_0);
 
 		*(bar + UOCMUX) &= PUEN_SET;
@@ -472,9 +468,7 @@
 			*(bar + UDCDEVCTL) |= UDC_SD_SET;
 		}
 
-		dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
-				      PCI_DEVICE_ID_AMD_CS5536_OTG, 0);
-		if (dev) {
+		if (otg_dev) {
 			bar = (u32 *)pci_read_config32(dev, PCI_BASE_ADDRESS_0);
 			*(bar + UOCCTL) |= PADEN_SET;
 			*(bar + UOCCAP) |= APU_SET;
@@ -482,15 +476,8 @@
 	}
 
 	/* Disable virtual PCI UDC and OTG headers. */
-	dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
-			      PCI_DEVICE_ID_AMD_CS5536_UDC, 0);
-	if (dev)
-		pci_write_config32(dev, 0x7C, 0xDEADBEEF);
-
-	dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
-			      PCI_DEVICE_ID_AMD_CS5536_OTG, 0);
-	if (dev)
-		pci_write_config32(dev, 0x7C, 0xDEADBEEF);
+	hide_vpci(0x80007e00); /* UDC */
+	hide_vpci(0x80007f00); /* OTG */
 }
 
 /** 
@@ -603,9 +590,19 @@
 
 static void hide_vpci(u32 vpci_devid)
 {
-	/* Hide unwanted virtual PCI device. */
-	printk(BIOS_DEBUG, "Hiding VPCI device: 0x%08X\n",
-		vpci_devid);
+	/* Hide unwanted virtual PCI device.
+	 * bits  0 ->  1 zero
+	 * bits  2 ->  7 target dword within the target function 
+	 *               (zero if we're disabling entire pci devices)
+	 * bits  8 -> 10 target function of the device
+	 * bits 11 -> 15 target pci device
+	 * bits 16 -> 23 pci bus
+	 * bits 24 -> 30 reserved and set to zero
+	 * bit  31       triggers the config cycle
+	 */
+	printk(BIOS_DEBUG, "Hiding VPCI device: 0x%08X (%02x:%02x.%01x)\n",
+		vpci_devid, (vpci_devid >> 16) & 0xff,
+		(vpci_devid >> 11) & 0x1f, (vpci_devid >> 8) & 0x7);
 	outl(vpci_devid + 0x7C, 0xCF8);
 	outl(0xDEADBEEF, 0xCFC);
 }
@@ -630,7 +627,7 @@
 
 	setup_i8259();
 	lpc_init(sb);
-	uarts_init(sb);
+	uarts_init(sb, dev);
 
 	if (sb->enable_gpio_int_route) {
 		printk(BIOS_SPEW, "cs5536: call vr_write\n");

[Carl-Daniel Hailfinger]

Hi Ron,

can you review this?

- Clean up Geode companion chip CS5536 code.
- Improve VPCI hiding debug message and add doxygen comments.
- Eliminate a few redundant dev_find_pci_device() calls.

This should be an equivalence transformation.

Build tested on norwich, db800, alix.1c, alix.2c3, dbe62.
No new breakage on dbe61.

Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>

Index: LinuxBIOSv3-cs5536cleanup/southbridge/amd/cs5536/cs5536.c
===================================================================
--- LinuxBIOSv3-cs5536cleanup/southbridge/amd/cs5536/cs5536.c	(Revision 677)
+++ LinuxBIOSv3-cs5536cleanup/southbridge/amd/cs5536/cs5536.c	(Arbeitskopie)
@@ -89,6 +89,28 @@
 };
 
 /**
+ * Hide unwanted virtual PCI device.
+ *
+ * @param vpci_devid The bus location of the device to be hidden.
+ * bits  0 ->  1 zero
+ * bits  2 ->  7 target dword within the target function 
+ *               (zero if we're disabling entire pci devices)
+ * bits  8 -> 10 target function of the device
+ * bits 11 -> 15 target pci device
+ * bits 16 -> 23 pci bus
+ * bits 24 -> 30 reserved and set to zero
+ * bit  31       triggers the config cycle
+ */
+static void hide_vpci(u32 vpci_devid)
+{
+	printk(BIOS_DEBUG, "Hiding VPCI device: 0x%08X (%02x:%02x.%01x)\n",
+		vpci_devid, (vpci_devid >> 16) & 0xff,
+		(vpci_devid >> 11) & 0x1f, (vpci_devid >> 8) & 0x7);
+	outl(vpci_devid + 0x7C, 0xCF8);
+	outl(0xDEADBEEF, 0xCFC);
+}
+
+/**
  * Power button setup.
  *
  * Setup GPIO24, it is the external signal for CS5536 vsb_work_aux which
@@ -175,8 +197,7 @@
 static void enable_ide_nand_flash_header(void)
 {
 	/* Tell VSA to use FLASH PCI header. Not IDE header. */
-	outl(0x80007A40, 0xCF8);
-	outl(0xDEADBEEF, 0xCFC);
+	hide_vpci(0x800079C4);
 }
 
 #define RTC_CENTURY	0x32
@@ -240,16 +261,13 @@
  *
  * @param sb Southbridge config structure.
  */
-static void uarts_init(struct southbridge_amd_cs5536_dts_config *sb)
+static void uarts_init(struct southbridge_amd_cs5536_dts_config *sb,
+			struct device *dev)
 {
 	struct msr msr;
 	u16 addr = 0;
 	u32 gpio_addr;
-	struct device *dev;
 
-	dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
-			      PCI_DEVICE_ID_AMD_CS5536_ISA, 0);
-
 	gpio_addr = pci_read_config32(dev, PCI_BASE_ADDRESS_1);
 	gpio_addr &= ~1;	/* Clear I/O bit */
 	printk(BIOS_DEBUG, "GPIO_ADDR: %08X\n", gpio_addr);
@@ -418,11 +436,11 @@
 {
 	u32 *bar;
 	struct msr msr;
-	struct device *dev;
+	struct device *ehci_dev, *otg_dev, *udc_dev;
 
-	dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
+	ehci_dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
 			      PCI_DEVICE_ID_AMD_CS5536_EHCI, 0);
-	if (dev) {
+	if (ehci_dev) {
 		/* Serial short detect enable */
 		msr = rdmsr(USB2_SB_GLD_MSR_CONF);
 		msr.hi |= USB2_UPPER_SSDEN_SET;
@@ -431,7 +449,7 @@
 		/* Write to clear diag register. */
 		wrmsr(USB2_SB_GLD_MSR_DIAG, rdmsr(USB2_SB_GLD_MSR_DIAG));
 
-		bar = (u32 *) pci_read_config32(dev, PCI_BASE_ADDRESS_0);
+		bar = (u32 *) pci_read_config32(ehci_dev, PCI_BASE_ADDRESS_0);
 
 		/* Make HCCPARAMS writable. */
 		*(bar + IPREG04) |= USB_HCCPW_SET;
@@ -440,10 +458,10 @@
 		*(bar + HCCPARAMS) = 0x00005012;
 	}
 
-	dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
+	otg_dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
 			      PCI_DEVICE_ID_AMD_CS5536_OTG, 0);
-	if (dev) {
-		bar = (u32 *) pci_read_config32(dev, PCI_BASE_ADDRESS_0);
+	if (otg_dev) {
+		bar = (u32 *) pci_read_config32(otg_dev, PCI_BASE_ADDRESS_0);
 
 		*(bar + UOCMUX) &= PUEN_SET;
 
@@ -458,6 +476,8 @@
 			*(bar + UOCCAP) |= sb->enable_USBP4_overcurrent;
 	}
 
+	udc_dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
+				  PCI_DEVICE_ID_AMD_CS5536_UDC, 0);
 	/* PBz#6466: If the UOC(OTG) device, port 4, is configured as a
 	 * device, then perform the following sequence:
 	 *  - Set SD bit in DEVCTRL udc register
@@ -465,32 +485,24 @@
 	 *  - Set APU bit in uoc register
 	 */
 	if (sb->enable_USBP4_device) {
-		dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
-				      PCI_DEVICE_ID_AMD_CS5536_UDC, 0);
-		if (dev) {
-			bar = (u32 *)pci_read_config32(dev, PCI_BASE_ADDRESS_0);
+		if (udc_dev) {
+			bar = (u32 *)pci_read_config32(udc_dev, PCI_BASE_ADDRESS_0);
 			*(bar + UDCDEVCTL) |= UDC_SD_SET;
 		}
 
-		dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
-				      PCI_DEVICE_ID_AMD_CS5536_OTG, 0);
-		if (dev) {
-			bar = (u32 *)pci_read_config32(dev, PCI_BASE_ADDRESS_0);
+		if (otg_dev) {
+			bar = (u32 *)pci_read_config32(otg_dev, PCI_BASE_ADDRESS_0);
 			*(bar + UOCCTL) |= PADEN_SET;
 			*(bar + UOCCAP) |= APU_SET;
 		}
 	}
 
 	/* Disable virtual PCI UDC and OTG headers. */
-	dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
-			      PCI_DEVICE_ID_AMD_CS5536_UDC, 0);
-	if (dev)
-		pci_write_config32(dev, 0x7C, 0xDEADBEEF);
+	if (udc_dev)
+		pci_write_config32(udc_dev, 0x7C, 0xDEADBEEF);
 
-	dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
-			      PCI_DEVICE_ID_AMD_CS5536_OTG, 0);
-	if (dev)
-		pci_write_config32(dev, 0x7C, 0xDEADBEEF);
+	if (otg_dev)
+		pci_write_config32(otg_dev, 0x7C, 0xDEADBEEF);
 }
 
 /** 
@@ -605,16 +617,6 @@
 	pci_write_config8(dev, IDE_CFG, ide_cfg);
 }
 
-
-static void hide_vpci(u32 vpci_devid)
-{
-	/* Hide unwanted virtual PCI device. */
-	printk(BIOS_DEBUG, "Hiding VPCI device: 0x%08X\n",
-		vpci_devid);
-	outl(vpci_devid + 0x7C, 0xCF8);
-	outl(0xDEADBEEF, 0xCFC);
-}
-
 /**
  * TODO.
  *
@@ -635,7 +637,7 @@
 
 	setup_i8259();
 	lpc_init(sb);
-	uarts_init(sb);
+	uarts_init(sb, dev);
 
 	if (sb->enable_gpio_int_route) {
 		printk(BIOS_SPEW, "cs5536: call vr_write\n");

[ron minnich]

will test tonight or tomorrow night. Am on short trip.

ron

[Carl-Daniel Hailfinger]

Hi Ron,

I assume this slipped through the cracks. Had we committed this earlier,
you would have been able to scratch part of the additional code comments
in r688.

Regards,
Carl-Daniel

On 08.05.2008 02:51, ron minnich wrote:
> On 08.05.2008 02:12, Carl-Daniel Hailfinger wrote:
> > Hi Ron,
> >
> > can you review this?
> >
> > - Clean up Geode companion chip CS5536 code.
> > - Improve VPCI hiding debug message and add doxygen comments.
> > - Eliminate a few redundant dev_find_pci_device() calls.
> >
> > This should be an equivalence transformation.
> >
> > Build tested on norwich, db800, alix.1c, alix.2c3, dbe62.
> > No new breakage on dbe61.
> >
> > Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>
> > will test tonight or tomorrow night. Am on short trip.
>
> ron

[ron minnich]

So, I went to apply and test this patch, but I guess, on reflection,
I'm pretty uncomfortable with it. Why? Because of this:

+	hide_vpci(0x800079C4);

This is even worse than putting stuff like this in the code
        dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
                              PCI_DEVICE_ID_AMD_CS5536_UDC, 0);
        if (dev)
                pci_write_config32(dev, 0x7C, 0xDEADBEEF);

because at least the latter form doesn't hard code pcidevfn.

I think you are right. hide_vpci should take a dev pointer. So let's
try to rework with that change to hide_vpci and we can then bring this
patch in.

hide_vpci then becomes pretty simple in fact ...

thanks

ron

[Carl-Daniel Hailfinger]

On 04.06.2008 16:53, ron minnich wrote:
> So, I went to apply and test this patch, but I guess, on reflection,
> I'm pretty uncomfortable with it. Why? Because of this:
>
> +	hide_vpci(0x800079C4);
>
> This is even worse than putting stuff like this in the code
>         dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
>                               PCI_DEVICE_ID_AMD_CS5536_UDC, 0);
>         if (dev)
>                 pci_write_config32(dev, 0x7C, 0xDEADBEEF);
>
> because at least the latter form doesn't hard code pcidevfn.
>
> I think you are right. hide_vpci should take a dev pointer. So let's
> try to rework with that change to hide_vpci and we can then bring this
> patch in.
>
> hide_vpci then becomes pretty simple in fact ...

Please note that my patch does only change those occurences where

pci_write_config32(dev, 0x7C, 0xDEADBEEF);

was unusable. The rest of the changes is just better commenting,
avoidance of variable reuse and so on.

My original patch had the pci_write_config32 -> hide_vpci transformation
(bad), the updated patch (dated 08.05.2008 02:12) didn't, that way I can
send a followup patch which introduces hide_vpci_dev and cleans up the
rest once you ack this one.

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

New iteration.

- Clean up Geode companion chip CS5536 code.
- Eliminate a few redundant dev_find_pci_device() calls.
- Fix a compile warning intruduced in r689.

This should be an equivalence transformation.

Build tested on norwich, db800, alix.1c, alix.2c3, dbe62, dbe61.

Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>

Index: LinuxBIOSv3-tmp3/southbridge/amd/cs5536/cs5536.c
===================================================================
--- LinuxBIOSv3-tmp3/southbridge/amd/cs5536/cs5536.c	(Revision 691)
+++ LinuxBIOSv3-tmp3/southbridge/amd/cs5536/cs5536.c	(Arbeitskopie)
@@ -240,16 +240,13 @@
  *
  * @param sb Southbridge config structure.
  */
-static void uarts_init(struct southbridge_amd_cs5536_dts_config *sb)
+static void uarts_init(struct southbridge_amd_cs5536_dts_config *sb,
+			struct device *dev)
 {
 	struct msr msr;
 	u16 addr = 0;
 	u32 gpio_addr;
-	struct device *dev;
 
-	dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
-			      PCI_DEVICE_ID_AMD_CS5536_ISA, 0);
-
 	gpio_addr = pci_read_config32(dev, PCI_BASE_ADDRESS_1);
 	gpio_addr &= ~1;	/* Clear I/O bit */
 	printk(BIOS_DEBUG, "GPIO_ADDR: %08X\n", gpio_addr);
@@ -419,11 +416,11 @@
 {
 	u32 *bar;
 	struct msr msr;
-	struct device *dev;
+	struct device *ehci_dev, *otg_dev, *udc_dev;
 
-	dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
+	ehci_dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
 			      PCI_DEVICE_ID_AMD_CS5536_EHCI, 0);
-	if (dev) {
+	if (ehci_dev) {
 		/* Serial short detect enable */
 		msr = rdmsr(USB2_SB_GLD_MSR_CONF);
 		msr.hi |= USB2_UPPER_SSDEN_SET;
@@ -432,7 +429,7 @@
 		/* Write to clear diag register. */
 		wrmsr(USB2_SB_GLD_MSR_DIAG, rdmsr(USB2_SB_GLD_MSR_DIAG));
 
-		bar = (u32 *) pci_read_config32(dev, PCI_BASE_ADDRESS_0);
+		bar = (u32 *) pci_read_config32(ehci_dev, PCI_BASE_ADDRESS_0);
 
 		/* Make HCCPARAMS writable. */
 		*(bar + IPREG04) |= USB_HCCPW_SET;
@@ -441,10 +438,10 @@
 		*(bar + HCCPARAMS) = 0x00005012;
 	}
 
-	dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
+	otg_dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
 			      PCI_DEVICE_ID_AMD_CS5536_OTG, 0);
-	if (dev) {
-		bar = (u32 *) pci_read_config32(dev, PCI_BASE_ADDRESS_0);
+	if (otg_dev) {
+		bar = (u32 *) pci_read_config32(otg_dev, PCI_BASE_ADDRESS_0);
 
 		printk(BIOS_DEBUG, "UOCMUX is %x\n", *(bar + UOCMUX));
 
@@ -470,6 +467,8 @@
 
 	}
 
+	udc_dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
+				  PCI_DEVICE_ID_AMD_CS5536_UDC, 0);
 	/* PBz#6466: If the UOC(OTG) device, port 4, is configured as a
 	 * device, then perform the following sequence:
 	 *  - Set SD bit in DEVCTRL udc register
@@ -477,24 +476,19 @@
 	 *  - Set APU bit in uoc register
 	 */
 	if (sb->enable_USBP4_device) {
-		dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
-				      PCI_DEVICE_ID_AMD_CS5536_UDC, 0);
-		if (dev) {
-			bar = (u32 *)pci_read_config32(dev, PCI_BASE_ADDRESS_0);
+		if (udc_dev) {
+			bar = (u32 *)pci_read_config32(udc_dev, PCI_BASE_ADDRESS_0);
 			*(bar + UDCDEVCTL) |= UDC_SD_SET;
 		}
 
-		dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
-				      PCI_DEVICE_ID_AMD_CS5536_OTG, 0);
-		if (dev) {
-			bar = (u32 *)pci_read_config32(dev, PCI_BASE_ADDRESS_0);
+		if (otg_dev) {
+			bar = (u32 *)pci_read_config32(otg_dev, PCI_BASE_ADDRESS_0);
 			*(bar + UOCCTL) |= PADEN_SET;
 			*(bar + UOCCAP) |= APU_SET;
+			printk(BIOS_DEBUG, "UOCCTL is %x\n", *(bar + UOCCTL));
 		}
 	}
 
-	printk(BIOS_DEBUG, "UOCCTL is %x\n", *(bar + UOCCTL));
-
 	/* Disable virtual PCI UDC and OTG headers.  The kernel never
 	 * sees a header for this device.  It used to provide an OS
 	 * visible device, but that was defeatured.  There are still
@@ -506,15 +500,11 @@
 	 * device ID PCI_DEVICE_ID_AMD_CS5536_OTG, but it is hidden
 	 * when 0xDEADBEEF is written to config space register 0x7C.
 	 */
-	dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
-			      PCI_DEVICE_ID_AMD_CS5536_UDC, 0);
-	if (dev)
-		pci_write_config32(dev, 0x7C, 0xDEADBEEF);
+	if (udc_dev)
+		pci_write_config32(udc_dev, 0x7C, 0xDEADBEEF);
 
-	dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
-			      PCI_DEVICE_ID_AMD_CS5536_OTG, 0);
-	if (dev)
-		pci_write_config32(dev, 0x7C, 0xDEADBEEF);
+	if (otg_dev)
+		pci_write_config32(otg_dev, 0x7C, 0xDEADBEEF);
 }
 
 /** 
@@ -659,7 +649,7 @@
 
 	setup_i8259();
 	lpc_init(sb);
-	uarts_init(sb);
+	uarts_init(sb, dev);
 
 	if (sb->enable_gpio_int_route) {
 		printk(BIOS_SPEW, "cs5536: call vr_write\n");

 


Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

[PATCH] [RFC] v3: CS5536 cleanup, and v3 design

[Peter Stuge]

On Thu, Jul 03, 2008 at 04:47:57PM +0200, Carl-Daniel Hailfinger wrote:
> New iteration.
>
> - Clean up Geode companion chip CS5536 code.
> - Eliminate a few redundant dev_find_pci_device() calls.
> - Fix a compile warning intruduced in r689.
>
> This should be an equivalence transformation.
>
> Build tested on norwich, db800, alix.1c, alix.2c3, dbe62, dbe61.
>
> Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>

Acked-by: Peter Stuge <[e-mail]>
> @@ -659,7 +649,7 @@
>
> setup_i8259();
>   lpc_init(sb);
> -	uarts_init(sb);
> +	uarts_init(sb, dev);
>
> if (sb->enable_gpio_int_route) {
>       printk(BIOS_SPEW, "cs5536: call vr_write\n");

More generally to the v3 design - should we try to create finer
granularity hooks?

I'm thinking per PCI-device in each component rather than per
component?


//Peter

[PATCH] [RFC] v3: CS5536 cleanup, and v3 design

[ron minnich]

On Thu, Jul 3, 2008 at 10:53 AM, Peter Stuge <[e-mail]> wrote:
> I'm thinking per PCI-device in each component rather than per
> component?

We've had this discussion before. It's a tough call: finer granularity
vs. having lots of fiddly files. That said, your idea is attractive. I
am very concerned that we maintain readability for non-experts and
newbies. Would this make it more readable in your opinion? If so, it
makes sense.

BTW, this is a great patch. And it's tested :-)

ron

[PATCH] [RFC] v3: CS5536 cleanup, and v3 design

[Peter Stuge]

On Thu, Jul 03, 2008 at 11:00:49AM -0700, ron minnich wrote:
> > I'm thinking per PCI-device in each component rather than per
> > component?
>
> We've had this discussion before. It's a tough call: finer granularity
> vs. having lots of fiddly files.

Not neccessarily more files though - primarily more functions.
> That said, your idea is attractive. I am very concerned that we
> maintain readability for non-experts and newbies.

Good, I agree.
> Would this make it more readable in your opinion? If so, it makes
> sense.

Actually yes, I think so, if there is correlation between what is
in the component dts, and what is in the component source.

Maybe some components should be several files, but not neccessarily
as a rule. I've compared with Linux kernel drivers before and will
again. Drivers can be just one .c, but can also be a bunch of them
when that makes sense. This is not top priority of course, right now
I would prefer to keep it at one file per component.


//Peter

[PATCH] [RFC] v3: CS5536 cleanup, and v3 design

[Carl-Daniel Hailfinger]

On 03.07.2008 20:00, ron minnich wrote:
> On Thu, Jul 3, 2008 at 10:53 AM, Peter Stuge <[e-mail]> wrote:
> > I'm thinking per PCI-device in each component rather than per
> > component?
>
> We've had this discussion before. It's a tough call: finer granularity
> vs. having lots of fiddly files. That said, your idea is attractive. I
> am very concerned that we maintain readability for non-experts and
> newbies. Would this make it more readable in your opinion? If so, it
> makes sense.
>
> BTW, this is a great patch. And it's tested :-)

Thanks! Can I have an Ack from you as well?

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

[PATCH] [RFC] v3: CS5536 cleanup, and v3 design

[ron minnich]

On Thu, Jul 3, 2008 at 11:17 AM, Carl-Daniel Hailfinger
> Thanks! Can I have an Ack from you as well?

It just so happens I have a special price on Acks today.

Acked-by: Ronald G. Minnich <[e-mail]>

ron

[PATCH] [RFC] v3: CS5536 cleanup, and v3 design

[Carl-Daniel Hailfinger]

On 03.07.2008 21:34, ron minnich wrote:
> On Thu, Jul 3, 2008 at 11:17 AM, Carl-Daniel Hailfinger
> > Thanks! Can I have an Ack from you as well?
>
> It just so happens I have a special price on Acks today.

OK, I'll buy you a pizza next time we meet.
> Acked-by: Ronald G. Minnich <[e-mail]>

Thanks, r692.


Regards,
Carl-Daniel

[PATCH] [RFC] v3: CS5536 cleanup, and v3 design

[Carl-Daniel Hailfinger]

On 03.07.2008 19:53, Peter Stuge wrote:
> On Thu, Jul 03, 2008 at 04:47:57PM +0200, Carl-Daniel Hailfinger wrote:
> > New iteration.
> >
> > - Clean up Geode companion chip CS5536 code.
> > - Eliminate a few redundant dev_find_pci_device() calls.
> > - Fix a compile warning intruduced in r689.
> >
> > This should be an equivalence transformation.
> >
> > Build tested on norwich, db800, alix.1c, alix.2c3, dbe62, dbe61.
> >
> > Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>
>
> Acked-by: Peter Stuge <[e-mail]>

Thanks!
> > @@ -659,7 +649,7 @@
> >
> > setup_i8259();
> >   lpc_init(sb);
> > -	uarts_init(sb);
> > +	uarts_init(sb, dev);
> >
> > if (sb->enable_gpio_int_route) {
> >       printk(BIOS_SPEW, "cs5536: call vr_write\n");
>
> More generally to the v3 design - should we try to create finer
> granularity hooks?
>
> I'm thinking per PCI-device in each component rather than per
> component?

Absolutely. The ultimate goal is to kill dev_find_pci_device completely.
We have a really nice device model and we should not work around it with
v2-style code. The cleanup I did is structured in a way that allows easy
migration to per-PCI-device hooks.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

v3 hooks per-logical-device instead of -component

[Peter Stuge]

On Thu, Jul 03, 2008 at 08:03:36PM +0200, Carl-Daniel Hailfinger wrote:
> easy migration to per-PCI-device hooks.

And s,-PCI,, works too! :)


//Peter

[PATCH] [RFC] v3: CS5536 cleanup, and v3 design

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> Absolutely. The ultimate goal is to kill dev_find_pci_device completely.
> We have a really nice device model and we should not work around it with
> v2-style code.

This will not always be possible, because devices have interactions.

Example:

function setup_link_between_a_and_b:
- Set bit X in device A
- Set bit Y in device B
- Poll bit Z in device A until a link between A and B is valid

Now, you have to put that function either in device A or device B.

Stefan

-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: [phone] • Fax: [phone]
Email: [e-mail]  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866

[PATCH] [RFC] v3: CS5536 cleanup, and v3 design

[Carl-Daniel Hailfinger]

On 03.07.2008 23:58, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
> > Absolutely. The ultimate goal is to kill dev_find_pci_device completely.
> > We have a really nice device model and we should not work around it with
> > v2-style code.
>
> This will not always be possible, because devices have interactions.
>
> Example:
>
> function setup_link_between_a_and_b:
> - Set bit X in device A
> - Set bit Y in device B
> - Poll bit Z in device A until a link between A and B is valid
>
> Now, you have to put that function either in device A or device B.

OK, that one is indeed valid. However, I'd like to keep the number of
dev_find_*_device calls as low as possible without compromising code
readability.
For example, if you want to establish a link between two devices,
calling the function with device pointers for both devices would make
the code readable and stuff would still work.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

[PATCH] [RFC] v3: CS5536 cleanup, and v3 design

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> OK, that one is indeed valid. However, I'd like to keep the number of
> dev_find_*_device calls as low as possible without compromising code
> readability.
> For example, if you want to establish a link between two devices,
> calling the function with device pointers for both devices would make
> the code readable and stuff would still work.

Hmm... and we can describe those links between devices in the dts.. I 
think we tried that for HT once.

So we'd have a construct of "devices" and a construct of "links" which 
are connections between devices.

links would have enable, disable, reset(, probe?) as methods?


-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: [phone] • Fax: [phone]
Email: [e-mail]  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866

[PATCH] [RFC] v3: CS5536 cleanup, and v3 design

[Carl-Daniel Hailfinger]

On 04.07.2008 00:21, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
> > OK, that one is indeed valid. However, I'd like to keep the number of
> > dev_find_*_device calls as low as possible without compromising code
> > readability.
> > For example, if you want to establish a link between two devices,
> > calling the function with device pointers for both devices would make
> > the code readable and stuff would still work.
>
> Hmm... and we can describe those links between devices in the dts.. I
> think we tried that for HT once.
>
> So we'd have a construct of "devices" and a construct of "links" which
> are connections between devices.
>
> links would have enable, disable, reset(, probe?) as methods?

Great idea!

Funny side note: I spent the last 9 months working on link/device model
representation with dependencies for my diploma thesis. This will be an
exciting way to cross-check the stuff I developed in a totally different
field.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

[PATCH] [RFC] v3: CS5536 cleanup, and v3 design

[Peter Stuge]

On Thu, Jul 03, 2008 at 11:58:59PM +0200, Stefan Reinauer wrote:
> > Absolutely. The ultimate goal is to kill dev_find_pci_device
> > completely.
>
> This will not always be possible, because devices have interactions.
>
> Example:
>
> function setup_link_between_a_and_b:
> - Set bit X in device A
> - Set bit Y in device B
> - Poll bit Z in device A until a link between A and B is valid
>
> Now, you have to put that function either in device A or device B.

It probably goes outside both A and B. I think that's what you intend
with the links.


//Peter

[Carl-Daniel Hailfinger]

- Improve VPCI hiding debug message and add doxygen comments.
- Replace a hand-crafted open-coded VPCI hiding sequence.

Build tested on all relevant targets.

Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>

Index: LinuxBIOSv3-cs5536cleanup/southbridge/amd/cs5536/cs5536.c
===================================================================
--- LinuxBIOSv3-cs5536cleanup/southbridge/amd/cs5536/cs5536.c	(Revision 692)
+++ LinuxBIOSv3-cs5536cleanup/southbridge/amd/cs5536/cs5536.c	(Arbeitskopie)
@@ -89,6 +89,28 @@
 };
 
 /**
+ * Hide unwanted virtual PCI device.
+ *
+ * @param vpci_devid The bus location of the device to be hidden.
+ * bits  0 ->  1 zero
+ * bits  2 ->  7 target dword within the target function 
+ *               (zero if we're disabling entire pci devices)
+ * bits  8 -> 10 target function of the device
+ * bits 11 -> 15 target pci device
+ * bits 16 -> 23 pci bus
+ * bits 24 -> 30 reserved and set to zero
+ * bit  31       triggers the config cycle
+ */
+static void hide_vpci(u32 vpci_devid)
+{
+	printk(BIOS_DEBUG, "Hiding VPCI device: 0x%08X (%02x:%02x.%01x)\n",
+		vpci_devid, (vpci_devid >> 16) & 0xff,
+		(vpci_devid >> 11) & 0x1f, (vpci_devid >> 8) & 0x7);
+	outl(vpci_devid + 0x7C, 0xCF8);
+	outl(0xDEADBEEF, 0xCFC);
+}
+
+/**
  * Power button setup.
  *
  * Setup GPIO24, it is the external signal for CS5536 vsb_work_aux which
@@ -175,8 +197,7 @@
 static void enable_ide_nand_flash_header(void)
 {
 	/* Tell VSA to use FLASH PCI header. Not IDE header. */
-	outl(0x80007A40, 0xCF8);
-	outl(0xDEADBEEF, 0xCFC);
+	hide_vpci(0x800079C4);
 }
 
 #define RTC_CENTURY	0x32
@@ -619,16 +640,6 @@
 	pci_write_config32(dev, IDE_CFG, ide_cfg);
 }
 
-
-static void hide_vpci(u32 vpci_devid)
-{
-	/* Hide unwanted virtual PCI device. */
-	printk(BIOS_DEBUG, "Hiding VPCI device: 0x%08X\n",
-		vpci_devid);
-	outl(vpci_devid + 0x7C, 0xCF8);
-	outl(0xDEADBEEF, 0xCFC);
-}
-
 /**
  * TODO.
  *



-- 
http://www.hailfinger.org/

[ron minnich]

build tested. Let's get some run tests back before we commit.

ron

[Carl-Daniel Hailfinger]

On 04.07.2008 01:32, Carl-Daniel Hailfinger wrote:
> - Improve VPCI hiding debug message and add doxygen comments.
> - Replace a hand-crafted open-coded VPCI hiding sequence.
>
> Build tested on all relevant targets.
>
> Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>

And as attachment for Ron.


Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/


Index: LinuxBIOSv3-cs5536cleanup/southbridge/amd/cs5536/cs5536.c
===================================================================
--- LinuxBIOSv3-cs5536cleanup/southbridge/amd/cs5536/cs5536.c	(Revision 692)
+++ LinuxBIOSv3-cs5536cleanup/southbridge/amd/cs5536/cs5536.c	(Arbeitskopie)
@@ -89,6 +89,28 @@
 };
 
 /**
+ * Hide unwanted virtual PCI device.
+ *
+ * @param vpci_devid The bus location of the device to be hidden.
+ * bits  0 ->  1 zero
+ * bits  2 ->  7 target dword within the target function 
+ *               (zero if we're disabling entire pci devices)
+ * bits  8 -> 10 target function of the device
+ * bits 11 -> 15 target pci device
+ * bits 16 -> 23 pci bus
+ * bits 24 -> 30 reserved and set to zero
+ * bit  31       triggers the config cycle
+ */
+static void hide_vpci(u32 vpci_devid)
+{
+	printk(BIOS_DEBUG, "Hiding VPCI device: 0x%08X (%02x:%02x.%01x)\n",
+		vpci_devid, (vpci_devid >> 16) & 0xff,
+		(vpci_devid >> 11) & 0x1f, (vpci_devid >> 8) & 0x7);
+	outl(vpci_devid + 0x7C, 0xCF8);
+	outl(0xDEADBEEF, 0xCFC);
+}
+
+/**
  * Power button setup.
  *
  * Setup GPIO24, it is the external signal for CS5536 vsb_work_aux which
@@ -175,8 +197,7 @@
 static void enable_ide_nand_flash_header(void)
 {
 	/* Tell VSA to use FLASH PCI header. Not IDE header. */
-	outl(0x80007A40, 0xCF8);
-	outl(0xDEADBEEF, 0xCFC);
+	hide_vpci(0x800079C4);
 }
 
 #define RTC_CENTURY	0x32
@@ -619,16 +640,6 @@
 	pci_write_config32(dev, IDE_CFG, ide_cfg);
 }
 
-
-static void hide_vpci(u32 vpci_devid)
-{
-	/* Hide unwanted virtual PCI device. */
-	printk(BIOS_DEBUG, "Hiding VPCI device: 0x%08X\n",
-		vpci_devid);
-	outl(vpci_devid + 0x7C, 0xCF8);
-	outl(0xDEADBEEF, 0xCFC);
-}
-
 /**
  * TODO.
  *

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> On 04.07.2008 01:32, Carl-Daniel Hailfinger wrote:
> > - Improve VPCI hiding debug message and add doxygen comments.
> > - Replace a hand-crafted open-coded VPCI hiding sequence.
> >
> > Build tested on all relevant targets.
> >
> > Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>
>
> And as attachment for Ron.

I suggest using pci_write_config32 to register 0x7C, because that is
exactly what this does.


-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: [phone] • Fax: [phone]
Email: [e-mail]  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866

[Carl-Daniel Hailfinger]

On 10.07.2008 16:48, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
> > On 04.07.2008 01:32, Carl-Daniel Hailfinger wrote:
> > > - Improve VPCI hiding debug message and add doxygen comments.
> > > - Replace a hand-crafted open-coded VPCI hiding sequence.
> > >
> > > Build tested on all relevant targets.
> > >
> > > Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>
> >
> > And as attachment for Ron.
>
> I suggest using pci_write_config32 to register 0x7C, because that is
> exactly what this does.

The one call I converted is the only one which does not use register
0x7C. All other calls will be using a to-be written function which takes
a struct device * and uses pci_write_config32. Besides that, as long as
we offer the unwanted_vpci array in the dts, the function in my patch is
essential.

If there are workable alternatives, I'd like to hear about them.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

[Carl-Daniel Hailfinger]

Ping?

Regards,
Carl-Daniel

On 10.07.2008 20:31, Carl-Daniel Hailfinger wrote:
> On 10.07.2008 16:48, Stefan Reinauer wrote:
> > Carl-Daniel Hailfinger wrote:
> > > On 04.07.2008 01:32, Carl-Daniel Hailfinger wrote:
> > > > - Improve VPCI hiding debug message and add doxygen comments.
> > > > - Replace a hand-crafted open-coded VPCI hiding sequence.
> > > >
> > > > Build tested on all relevant targets.
> > > >
> > > > Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>
> > >
> > > And as attachment for Ron.
> >
> > I suggest using pci_write_config32 to register 0x7C, because that is
> > exactly what this does.
>
> The one call I converted is the only one which does not use register
> 0x7C. All other calls will be using a to-be written function which takes
> a struct device * and uses pci_write_config32. Besides that, as long as
> we offer the unwanted_vpci array in the dts, the function in my patch is
> essential.
>
> If there are workable alternatives, I'd like to hear about them.
>
> Regards,
> Carl-Daniel

Index: LinuxBIOSv3-cs5536cleanup/southbridge/amd/cs5536/cs5536.c
===================================================================
--- LinuxBIOSv3-cs5536cleanup/southbridge/amd/cs5536/cs5536.c	(Revision 692)
+++ LinuxBIOSv3-cs5536cleanup/southbridge/amd/cs5536/cs5536.c	(Arbeitskopie)
@@ -89,6 +89,28 @@
 };
 
 /**
+ * Hide unwanted virtual PCI device.
+ *
+ * @param vpci_devid The bus location of the device to be hidden.
+ * bits  0 ->  1 zero
+ * bits  2 ->  7 target dword within the target function 
+ *               (zero if we're disabling entire pci devices)
+ * bits  8 -> 10 target function of the device
+ * bits 11 -> 15 target pci device
+ * bits 16 -> 23 pci bus
+ * bits 24 -> 30 reserved and set to zero
+ * bit  31       triggers the config cycle
+ */
+static void hide_vpci(u32 vpci_devid)
+{
+	printk(BIOS_DEBUG, "Hiding VPCI device: 0x%08X (%02x:%02x.%01x)\n",
+		vpci_devid, (vpci_devid >> 16) & 0xff,
+		(vpci_devid >> 11) & 0x1f, (vpci_devid >> 8) & 0x7);
+	outl(vpci_devid + 0x7C, 0xCF8);
+	outl(0xDEADBEEF, 0xCFC);
+}
+
+/**
  * Power button setup.
  *
  * Setup GPIO24, it is the external signal for CS5536 vsb_work_aux which
@@ -175,8 +197,7 @@
 static void enable_ide_nand_flash_header(void)
 {
 	/* Tell VSA to use FLASH PCI header. Not IDE header. */
-	outl(0x80007A40, 0xCF8);
-	outl(0xDEADBEEF, 0xCFC);
+	hide_vpci(0x800079C4);
 }
 
 #define RTC_CENTURY	0x32
@@ -619,16 +640,6 @@
 	pci_write_config32(dev, IDE_CFG, ide_cfg);
 }
 
-
-static void hide_vpci(u32 vpci_devid)
-{
-	/* Hide unwanted virtual PCI device. */
-	printk(BIOS_DEBUG, "Hiding VPCI device: 0x%08X\n",
-		vpci_devid);
-	outl(vpci_devid + 0x7C, 0xCF8);
-	outl(0xDEADBEEF, 0xCFC);
-}
-
 /**
  * TODO.
  *

[ron minnich]

Acked-by: Ronald G. Minnich <[e-mail]>

recommend you get ward to test it as well on his alix.

ron

[Carl-Daniel Hailfinger]

On 01.08.2008 20:30, ron minnich wrote:
> Acked-by: Ronald G. Minnich <[e-mail]>

Thanks, r787.

Regards,
Carl-Daniel