Re: [PATCH][WAS:bcmai,axi] bcma: add Broadcom specific AMBA bus driver

[Arnd Bergmann]

Hi RafaÅ,

I haven't looked in detail, but since I'll be travelling for the next
two weeks, here is a really quick review. Overall, the bus driver
looks really nice, except for a few things that I guess should be
easy to resolve.

On Thursday 05 May 2011, RafaÅ MiÅecki wrote:
> Cc: Greg KH <greg@xxxxxxxxx>
> Cc: Michael BÃsch <mb@xxxxxxxxx>
> Cc: Larry Finger <Larry.Finger@xxxxxxxxxxxx>
> Cc: George Kashperko <george@xxxxxxxxxxx>
> Cc: Arend van Spriel <arend@xxxxxxxxxxxx>
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: Russell King <rmk@xxxxxxxxxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Cc: Andy Botting <andy@xxxxxxxxxxxxxxx>
> Cc: linuxdriverproject <devel@xxxxxxxxxxxxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>
> Signed-off-by: RafaÅ MiÅecki <zajec5@xxxxxxxxx>

This really needs a changelog. You've probably written all of
it before, just explain above the Cc what bcma is, where it's
used, why you use a bus_type. This will be the place where
people look when they want to find out what it is, so try
to make a good description.

> new file mode 100644
> index 0000000..b52cd2b
> --- /dev/null
> +++ b/drivers/bcma/README
> @@ -0,0 +1,18 @@
> +Broadcom introduced new bus as replacement for older SSB. It is based on AMBA,
> +however from programming point of view there is nothing AMBA specific we use.
> +
> +Standard AMBA drivers are platform specific, have hardcoded addresses and use
> +AMBA standard fields like CID and PID.
> +
> +In case of Broadcom's cards every device consists of:
> +1) Broadcom specific AMBA device. It is put on AMBA bus, but can not be treated
> +   as standard AMBA device. Reading it's CID or PID can cause machine lockup.
> +2) AMBA standard devices called ports or wrappers. They have CIDs (AMBA_CID)
> +   and PIDs (0x103BB369), but we do not use that info for anything. One of that
> +   devices is used for managing Broadcom specific core.
> +
> +Addresses of AMBA devices are not hardcoded in driver and have to be read from
> +EPROM.
> +
> +In this situation we decided to introduce separated bus with devices identified
> +by Broadcom specific fields, read from EPROM.

This would be a good start for the changelog. You don't actually need the
readme in the code directory, it's better to put the information somewhere
in the Documentation/ directory.

> diff --git a/drivers/bcma/TODO b/drivers/bcma/TODO
> new file mode 100644
> index 0000000..45eadc9
> --- /dev/null
> +++ b/drivers/bcma/TODO
> @@ -0,0 +1,3 @@
> +- Interrupts
> +- Defines for PCI core driver
> +- Convert bcma_bus->cores into linked list

The last item doesn't make sense to me. Since you are using the regular
driver model, you can simply iterate over all child devices of any
dev.

> +static void bcma_core_disable(struct bcma_device *core, u32 flags)
> +{
> +	if (bcma_aread32(core, BCMA_RESET_CTL) & BCMA_RESET_CTL_RESET)
> +		return;
> +
> +	bcma_awrite32(core, BCMA_IOCTL, flags);
> +	bcma_aread32(core, BCMA_IOCTL);
> +	udelay(10);
> +
> +	bcma_awrite32(core, BCMA_RESET_CTL, BCMA_RESET_CTL_RESET);
> +	udelay(1);
> +}
> +
> +int bcma_core_enable(struct bcma_device *core, u32 flags)
> +{
> +	bcma_core_disable(core, flags);
> +
> +	bcma_awrite32(core, BCMA_IOCTL, (BCMA_IOCTL_CLK | BCMA_IOCTL_FGC | flags));
> +	bcma_aread32(core, BCMA_IOCTL);
> +
> +	bcma_awrite32(core, BCMA_RESET_CTL, 0);
> +	udelay(1);
> +
> +	bcma_awrite32(core, BCMA_IOCTL, (BCMA_IOCTL_CLK | flags));
> +	bcma_aread32(core, BCMA_IOCTL);
> +	udelay(1);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(bcma_core_enable);

I don't know if we discussed this before. Normally, you should not need such
udelay() calls, at least if you have the correct I/O barriers in place.

> +#include "bcma_private.h"
> +#include <linux/bcma/bcma.h>
> +
> +MODULE_DESCRIPTION("Broadcom's specific AMBA driver");
> +MODULE_LICENSE("GPL");
> +
> +static int bcma_bus_match(struct device *dev, struct device_driver *drv);
> +static int bcma_device_probe(struct device *dev);
> +static int bcma_device_remove(struct device *dev);

Try to reorder your functions so you don't need any forward declarations.

> +const char *bcma_device_name(u16 coreid)
> +{
> +	switch (coreid) {
> +	case BCMA_CORE_OOB_ROUTER:
> +		return "OOB Router";
> +	case BCMA_CORE_INVALID:
> +		return "Invalid";
> +	case BCMA_CORE_CHIPCOMMON:
> +		return "ChipCommon";
> +	case BCMA_CORE_ILINE20:
> +		return "ILine 20";

It's better to make that a data structure than a switch() statement,
both from readability and efficiency aspects.

> +
> +/* 1) It is not allowed to put struct device statically in bcma_device
> + * 2) We can not just use pointer to struct device because we use container_of
> + * 3) We do not have pointer to struct bcma_device in struct device
> + * Solution: use such a dummy wrapper
> + */
> +struct __bcma_dev_wrapper {
> +	struct device dev;
> +	struct bcma_device *core;
> +};
> +
> +struct bcma_device {
> +	struct bcma_bus *bus;
> +	struct bcma_device_id id;
> +
> +	struct device *dev;
> +
> +	u8 core_index;
> +
> +	u32 addr;
> +	u32 wrap;
> +
> +	void *drvdata;
> +};

Something went wrong here, maybe you misunderstood the API, or I
misunderstood what you are trying to do. When you define your own bus
type, the private device (struct bcma_device) should definitely contain
a struct device as a member, and you allocate that structure dynamically
when probing the bus. I don't see any reason for that wrapper.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/