Hi Jonas,
On 07/13/2011 09:36 PM, Jonas Gorski wrote:
> Hi,
>
> some minor things I saw:
>
> On 9 July 2011 13:05, Hauke Mehrtens <hauke@xxxxxxxxxx> wrote:
>> This patch adds support for using bcma on a Broadcom SoC as the system
>> bus. An SoC like the bcm4716 could register this bus and use it to
>> searches for the bcma cores and register the devices on this bus.
>>
>> Signed-off-by: Hauke Mehrtens <hauke@xxxxxxxxxx>
>> ---
>> drivers/bcma/Kconfig | 5 +
>> drivers/bcma/Makefile | 1 +
>> drivers/bcma/host_soc.c | 178 +++++++++++++++++++++++++++++++++++++++++
>> drivers/bcma/main.c | 1 +
>> drivers/bcma/scan.c | 24 +++++-
>> include/linux/bcma/bcma.h | 4 +
>> include/linux/bcma/bcma_soc.h | 16 ++++
>> 7 files changed, 227 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/bcma/host_soc.c
>> create mode 100644 include/linux/bcma/bcma_soc.h
>>
>> diff --git a/drivers/bcma/Kconfig b/drivers/bcma/Kconfig
>> index 353781b..8d82f42 100644
>> --- a/drivers/bcma/Kconfig
>> +++ b/drivers/bcma/Kconfig
>> @@ -22,6 +22,11 @@ config BCMA_HOST_PCI
>> bool "Support for BCMA on PCI-host bus"
>> depends on BCMA_HOST_PCI_POSSIBLE
>>
>> +config BCMA_HOST_SOC
>> + bool
>> + depends on BCMA && MIPS
>> + default n
>
> Default default is already "n", so this line is superfluous.
Will remove this.
>
>> +
>> config BCMA_DEBUG
>> bool "BCMA debugging"
>> depends on BCMA
>> diff --git a/drivers/bcma/Makefile b/drivers/bcma/Makefile
>> index 0d56245..42d61dd 100644
>> --- a/drivers/bcma/Makefile
>> +++ b/drivers/bcma/Makefile
>> @@ -2,6 +2,7 @@ bcma-y += main.o scan.o core.o
>> bcma-y += driver_chipcommon.o driver_chipcommon_pmu.o
>> bcma-y += driver_pci.o
>> bcma-$(CONFIG_BCMA_HOST_PCI) += host_pci.o
>> +bcma-$(CONFIG_BCMA_HOST_SOC) += host_soc.o
>> obj-$(CONFIG_BCMA) += bcma.o
>>
>> ccflags-$(CONFIG_BCMA_DEBUG) := -DDEBUG
>> diff --git a/drivers/bcma/host_soc.c b/drivers/bcma/host_soc.c
>> new file mode 100644
>> index 0000000..a6fe724
>> --- /dev/null
>> +++ b/drivers/bcma/host_soc.c
>> @@ -0,0 +1,178 @@
>> +/*
>> + * Broadcom specific AMBA
>> + * System on Chip (SoC) Host
>> + *
>> + * Licensed under the GNU/GPL. See COPYING for details.
>> + */
>> +
>> +#include "bcma_private.h"
>> +#include "scan.h"
>> +#include <linux/bcma/bcma.h>
>> +#include <linux/bcma/bcma_soc.h>
>> +
>> +static u8 bcma_host_soc_read8(struct bcma_device *core, u16 offset)
>> +{
>> + return readb(core->io_addr + offset);
>> +}
>> +
>> +static u16 bcma_host_soc_read16(struct bcma_device *core, u16 offset)
>> +{
>> + return readw(core->io_addr + offset);
>> +}
>> +
>> +static u32 bcma_host_soc_read32(struct bcma_device *core, u16 offset)
>> +{
>> + return readl(core->io_addr + offset);
>> +}
>> +
>> +static void bcma_host_soc_write8(struct bcma_device *core, u16 offset,
>> + u8 value)
>> +{
>> + writeb(value, core->io_addr + offset);
>> +}
>> +
>> +static void bcma_host_soc_write16(struct bcma_device *core, u16 offset,
>> + u16 value)
>> +{
>> + writew(value, core->io_addr + offset);
>> +}
>> +
>> +static void bcma_host_soc_write32(struct bcma_device *core, u16 offset,
>> + u32 value)
>> +{
>> + writel(value, core->io_addr + offset);
>> +}
>> +
>> +#ifdef CONFIG_BCMA_BLOCKIO
>> +static void bcma_host_soc_block_read(struct bcma_device *core, void *buffer,
>> + size_t count, u16 offset, u8 reg_width)
>> +{
>> + void __iomem *addr = core->io_addr + offset;
>> +
>> + switch (reg_width) {
>> + case sizeof(u8): {
>> + u8 *buf = buffer;
>> +
>> + while (count) {
>> + *buf = __raw_readb(addr);
>> + buf++;
>> + count--;
>> + }
>> + break;
>> + }
>> + case sizeof(u16): {
>> + __le16 *buf = buffer;
>> +
>> + WARN_ON(count & 1);
>> + while (count) {
>> + *buf = (__force __le16)__raw_readw(addr);
>> + buf++;
>> + count -= 2;
>> + }
>> + break;
>> + }
>> + case sizeof(u32): {
>> + __le32 *buf = buffer;
>> +
>> + WARN_ON(count & 3);
>> + while (count) {
>> + *buf = (__force __le32)__raw_readl(addr);
>> + buf++;
>> + count -= 4;
>> + }
>> + break;
>> + }
>> + default:
>> + WARN_ON(1);
>> + }
>> +}
>> +
>> +static void bcma_host_soc_block_write(struct bcma_device *core,
>> + const void *buffer,
>> + size_t count, u16 offset, u8 reg_width)
>> +{
>> + void __iomem *addr = core->io_addr + offset;
>> +
>> + switch (reg_width) {
>> + case sizeof(u8): {
>> + const u8 *buf = buffer;
>> +
>> + while (count) {
>> + __raw_writeb(*buf, addr);
>> + buf++;
>> + count--;
>> + }
>> + break;
>> + }
>> + case sizeof(u16): {
>> + const __le16 *buf = buffer;
>> +
>> + WARN_ON(count & 1);
>> + while (count) {
>> + __raw_writew((__force u16)(*buf), addr);
>> + buf++;
>> + count -= 2;
>> + }
>> + break;
>> + }
>> + case sizeof(u32): {
>> + const __le32 *buf = buffer;
>> +
>> + WARN_ON(count & 3);
>> + while (count) {
>> + __raw_writel((__force u32)(*buf), addr);
>> + buf++;
>> + count -= 4;
>> + }
>> + break;
>> + }
>> + default:
>> + WARN_ON(1);
>> + }
>> +}
>> +#endif /* CONFIG_BCMA_BLOCKIO */
>> +
>> +static u32 bcma_host_soc_aread32(struct bcma_device *core, u16 offset)
>> +{
>> + return readl(core->io_wrap + offset);
>> +}
>> +
>> +static void bcma_host_soc_awrite32(struct bcma_device *core, u16 offset,
>> + u32 value)
>> +{
>> + writel(value, core->io_wrap + offset);
>> +}
>> +
>> +const struct bcma_host_ops bcma_host_soc_ops = {
>> + .read8 = bcma_host_soc_read8,
>> + .read16 = bcma_host_soc_read16,
>> + .read32 = bcma_host_soc_read32,
>> + .write8 = bcma_host_soc_write8,
>> + .write16 = bcma_host_soc_write16,
>> + .write32 = bcma_host_soc_write32,
>> +#ifdef CONFIG_BCMA_BLOCKIO
>> + .block_read = bcma_host_soc_block_read,
>> + .block_write = bcma_host_soc_block_write,
>> +#endif
>> + .aread32 = bcma_host_soc_aread32,
>> + .awrite32 = bcma_host_soc_awrite32,
>> +};
>> +
>> +int __init bcma_host_soc_register(struct bcma_soc *soc)
>> +{
>> + struct bcma_bus *bus = &soc->bus;
>> +
>> + /* iomap only first core. We have to read some register on this core
>> + * to scan the bus.
>> + */
>> + bus->mmio = ioremap(BCMA_ADDR_BASE, BCMA_CORE_SIZE * 1);
>> + if (!bus->mmio)
>> + return -ENOMEM;
>> +
>> + /* Host specific */
>> + bus->hosttype = BCMA_HOSTTYPE_SOC;
>> + bus->ops = &bcma_host_soc_ops;
>> +
>> + /* Register */
>> + return bcma_bus_early_register(bus, &soc->core_cc, &soc->core_mips);
>> +}
>> diff --git a/drivers/bcma/main.c b/drivers/bcma/main.c
>> index e6c308c..2ca5eeb 100644
>> --- a/drivers/bcma/main.c
>> +++ b/drivers/bcma/main.c
>> @@ -92,6 +92,7 @@ static int bcma_register_cores(struct bcma_bus *bus)
>> break;
>> case BCMA_HOSTTYPE_NONE:
>> case BCMA_HOSTTYPE_SDIO:
>> + case BCMA_HOSTTYPE_SOC:
>> break;
>> }
>>
>> diff --git a/drivers/bcma/scan.c b/drivers/bcma/scan.c
>> index bf9f806..202edc8 100644
>> --- a/drivers/bcma/scan.c
>> +++ b/drivers/bcma/scan.c
>> @@ -337,6 +337,14 @@ static int bcma_get_next_core(struct bcma_bus *bus, u32 __iomem **eromptr,
>> }
>> }
>> }
>> + if (bus->hosttype == BCMA_HOSTTYPE_SOC) {
>> + core->io_addr = ioremap(core->addr, BCMA_CORE_SIZE);
>> + if (!core->io_addr)
>> + return -ENOMEM;
>> + core->io_wrap = ioremap(core->wrap, BCMA_CORE_SIZE);
>> + if (!core->io_wrap)
>> + return -ENOMEM;
>
> Shouldn't you unmap core->io_addr if remapping io_wrap fails?
Ok I will add iounmap() on the error path and when the core is freed.
>
>> + }
>> return 0;
>> }
>>
>> @@ -369,7 +377,13 @@ int bcma_bus_scan(struct bcma_bus *bus)
>> bcma_init_bus(bus);
>>
>> erombase = bcma_scan_read32(bus, 0, BCMA_CC_EROM);
>> - eromptr = bus->mmio;
>> + if (bus->hosttype == BCMA_HOSTTYPE_SOC) {
>> + eromptr = ioremap(erombase, BCMA_CORE_SIZE);
>> + if (!eromptr)
>> + return -ENOMEM;
>> + } else
>> + eromptr = bus->mmio;
>
> Documentation/CodingStyle says use braces in both branches if one needs them.
Will fix this.
>
>> +
>> eromend = eromptr + BCMA_CORE_SIZE / sizeof(u32);
>>
>> bcma_scan_switch_core(bus, erombase);
>> @@ -417,7 +431,13 @@ int __init bcma_bus_scan_early(struct bcma_bus *bus,
>> int err, core_num = 0;
>>
>> erombase = bcma_scan_read32(bus, 0, BCMA_CC_EROM);
>> - eromptr = bus->mmio;
>> + if (bus->hosttype == BCMA_HOSTTYPE_SOC) {
>> + eromptr = ioremap(erombase, BCMA_CORE_SIZE);
>> + if (!eromptr)
>> + return -ENOMEM;
>> + } else
>> + eromptr = bus->mmio;
>
> Ditto.
>
>> +
>> eromend = eromptr + BCMA_CORE_SIZE / sizeof(u32);
>>
>> bcma_scan_switch_core(bus, erombase);
>> diff --git a/include/linux/bcma/bcma.h b/include/linux/bcma/bcma.h
>> index 6bd7b7f..73fda1c 100644
>> --- a/include/linux/bcma/bcma.h
>> +++ b/include/linux/bcma/bcma.h
>> @@ -16,6 +16,7 @@ enum bcma_hosttype {
>> BCMA_HOSTTYPE_NONE,
>> BCMA_HOSTTYPE_PCI,
>> BCMA_HOSTTYPE_SDIO,
>> + BCMA_HOSTTYPE_SOC,
>> };
>>
>> struct bcma_chipinfo {
>> @@ -124,6 +125,9 @@ struct bcma_device {
>> u32 addr;
>> u32 wrap;
>>
>> + void __iomem *io_addr;
>> + void __iomem *io_wrap;
>> +
>> void *drvdata;
>> struct list_head list;
>> };
>> diff --git a/include/linux/bcma/bcma_soc.h b/include/linux/bcma/bcma_soc.h
>> new file mode 100644
>> index 0000000..4203c55
>> --- /dev/null
>> +++ b/include/linux/bcma/bcma_soc.h
>> @@ -0,0 +1,16 @@
>> +#ifndef LINUX_BCMA_SOC_H_
>> +#define LINUX_BCMA_SOC_H_
>> +
>> +#include <linux/bcma/bcma.h>
>> +
>> +struct bcma_soc {
>> + struct bcma_bus bus;
>> + struct bcma_device core_cc;
>> + struct bcma_device core_mips;
>> +};
>> +
>> +int __init bcma_host_soc_register(struct bcma_soc *soc);
>> +
>> +int bcma_bus_register(struct bcma_bus *bus);
>> +
>> +#endif /* LINUX_BCMA_SOC_H_ */
>> --
>> 1.7.4.1
