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 <[EMAIL PROTECTED]>
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 <[EMAIL PROTECTED]>
>
> 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/
--
coreboot mailing list
[email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
