[coreboot] [Fwd: Re: [patch][v2] cs5536 usb port4 configuration - new v3 patch]

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Sat Jul 12 01:52:03 CEST 2008


Hi Marc,

please ignore my other mail regarding your equivalent v2 patch. I had
not seen this mail yet and assumed divergence.
Overall, I like the current style a little bit more, but the current
style also has these ugly divisions by sizeof(u32) which I dislike.

If anybody else acks this patch, feel free to add
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
to your v2 and v3 patch.

The only nitpick I have is a slight whitespace issue below.


On 12.07.2008 01:27, Marc Jones wrote:

> Correctly access MMIO for cs5536 USB port4 configuration. 
>
> Signed-off-by: Marc Jones <marc.jones at amd.com>
>
> Index: coreboot-v3/southbridge/amd/cs5536/cs5536.c
> ===================================================================
> --- coreboot-v3.orig/southbridge/amd/cs5536/cs5536.c	2008-06-05 15:37:05.000000000 -0600
> +++ coreboot-v3/southbridge/amd/cs5536/cs5536.c	2008-06-05 16:12:54.000000000 -0600
> @@ -395,19 +395,18 @@
>  	}
>  }
>  
> -/* the /sizeof(u32) is to convert byte offsets into u32 offsets */
> -#define HCCPARAMS		(0x08/sizeof(u32))
> -#define IPREG04			(0xA0/sizeof(u32))
> -#define USB_HCCPW_SET		(1 << 1)
> +#define HCCPARAMS		0x08
> +#define IPREG04			0xA0
> +#define USB_HCCPW_SET	(1 << 1)
>   

A few of the #define above look a bit unaligned.

>  #define UOCCAP			0x00
>  #define APU_SET			(1 << 15)
> -#define UOCMUX			(0x04/sizeof(u32))
> +#define UOCMUX			0x04
>  #define PMUX_HOST		0x02
>  #define PMUX_DEVICE		0x03
>  #define PUEN_SET		(1 << 2)
> -#define UDCDEVCTL		(0x404/sizeof(u32))
> +#define UDCDEVCTL		0x404
>  #define UDC_SD_SET		(1 << 10)
> -#define UOCCTL			(0x0C/sizeof(u32))
> +#define UOCCTL			0x0C
>  #define PADEN_SET		(1 << 7)
>  
>  /**
> @@ -417,7 +416,7 @@
>   */
>  static void enable_USB_port4(struct southbridge_amd_cs5536_dts_config *sb)
>  {
> -	u32 *bar;
> +	u8 *bar;
>  	struct msr msr;
>  	struct device *dev;
>  
> @@ -432,41 +431,42 @@
>  		/* 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 = (u8 *) pci_read_config32(dev, PCI_BASE_ADDRESS_0);
>  
>  		/* Make HCCPARAMS writable. */
> -		*(bar + IPREG04) |= USB_HCCPW_SET;
> +		writel(readl(bar + IPREG04) | USB_HCCPW_SET, bar + IPREG04);
>  
>  		/* EECP=50h, IST=01h, ASPC=1 */
> -		*(bar + HCCPARAMS) = 0x00005012;
> +		writel(0x00005012, bar + HCCPARAMS);
>  	}
>  
>  	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);
> +		bar = (u8 *) pci_read_config32(dev, PCI_BASE_ADDRESS_0);
>  
> -		printk(BIOS_DEBUG, "UOCMUX is %x\n", *(bar + UOCMUX));
> +		printk(BIOS_DEBUG, "UOCMUX is %x\n", readl(bar + UOCMUX));
>  
> -		*(bar + UOCMUX) &= PUEN_SET;
> +		writel(readl(bar + UOCMUX) & PUEN_SET, bar + UOCMUX);
>  
>  		/* Host or Device? */
>  		if (sb->enable_USBP4_device)
> -			*(bar + UOCMUX) |= PMUX_DEVICE;
> +			writel(readl(bar + UOCMUX) | PMUX_DEVICE, bar + UOCMUX);
>  		else
> -			*(bar + UOCMUX) |= PMUX_HOST;
> +			writel(readl(bar + UOCMUX) | PMUX_HOST, bar + UOCMUX);
>  
>  		/* Overcurrent configuration */
> -		printk(BIOS_DEBUG, "UOCCAP is %x\n", *(bar + UOCCAP));
> +		printk(BIOS_DEBUG, "UOCCAP is %x\n", readl(bar + UOCCAP));
>  		if (sb->enable_USBP4_overcurrent)
> -			*(bar + UOCCAP) |= sb->enable_USBP4_overcurrent;
> +			writel(readl(bar + UOCCAP)
> +			       | sb->enable_USBP4_overcurrent, bar + UOCCAP);
> +
>  		/* power control. see comment in the dts for these bits */
>  		if (sb->pph) {
> -			*(bar + UOCCAP) &= ~0xff;
> -			*(bar + UOCCAP) |= sb->pph;
> +			writel((readl(bar + UOCCAP) & ~0xff) | sb->pph, bar + UOCCAP);
>  		}
> -		printk(BIOS_DEBUG, "UOCCAP is %x\n", *(bar + UOCCAP));
> -		printk(BIOS_DEBUG, "UOCMUX is %x\n", *(bar + UOCMUX));
> +		printk(BIOS_DEBUG, "UOCCAP is %x\n", readl(bar + UOCCAP));
> +		printk(BIOS_DEBUG, "UOCMUX is %x\n", readl(bar + UOCMUX));
>  
>  	}
>  
> @@ -480,20 +480,20 @@
>  		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);
> -			*(bar + UDCDEVCTL) |= UDC_SD_SET;
> +			bar = (u8 *)pci_read_config32(dev, PCI_BASE_ADDRESS_0);
> +			writel(readl(bar + UDCDEVCTL) | UDC_SD_SET, bar + UDCDEVCTL);
>  		}
>  
>  		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);
> -			*(bar + UOCCTL) |= PADEN_SET;
> -			*(bar + UOCCAP) |= APU_SET;
> +			bar = (u8 *)pci_read_config32(dev, PCI_BASE_ADDRESS_0);
> +			writel(readl(bar + UOCCTL) | PADEN_SET, bar + UOCCTL);
> +			writel(readl(bar + UOCCAP) | APU_SET, bar + UOCCAP);
>  		}
>  	}
>  
> -	printk(BIOS_DEBUG, "UOCCTL is %x\n", *(bar + UOCCTL));
> +	printk(BIOS_DEBUG, "UOCCTL is %x\n", readl(bar + UOCCTL));
>  
>  	/* Disable virtual PCI UDC and OTG headers.  The kernel never
>  	 * sees a header for this device.  It used to provide an OS
>   
>   

Regards,
Carl-Daniel

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





More information about the coreboot mailing list