Re: [PATCH 2/9] add support for the TI VLYNQ bus

[Florian Fainelli]

Hi Andrew,

Le Tuesday 02 June 2009 07:08:54 Andrew Morton, vous avez écrit :
> On Mon, 1 Jun 2009 13:58:27 +0200 Florian Fainelli <florian@xxxxxxxxxxx> 
wrote:
> > This patch adds support for the TI VLYNQ high-speed,
> > serial and packetized bus. This bus allows external
> > devices to be connected to the System-on-Chip and
> > appear in the main system memory just like any memory
> > mapped peripheral. It is widely used in TI's networking
> > and mutlimedia SoC, including the AR7 SoC.
> >
> >
> > ...
> >
> > +struct vlynq_regs {
> > +	u32 revision;
> > +	u32 control;
> > +	u32 status;
> > +	u32 int_prio;
> > +	u32 int_status;
> > +	u32 int_pending;
> > +	u32 int_ptr;
> > +	u32 tx_offset;
> > +	struct vlynq_mapping rx_mapping[4];
> > +	u32 chip;
> > +	u32 autonego;
> > +	u32 unused[6];
> > +	u32 int_device[8];
> > +};
> > +
> > +#define vlynq_reg_read(reg) readl(&(reg))
> > +#define vlynq_reg_write(reg, val)  writel(val, &(reg))
>
> grumble.  These just make the code harder to follow.  it'd be better to
> open-code readl() and writel() at the callsites.

Ok

>
> > +static int __vlynq_enable_device(struct vlynq_device *dev);
> > +
> > +#ifdef VLYNQ_DEBUG
> > +static void vlynq_dump_regs(struct vlynq_device *dev)
> > +{
> > +	int i;
> > +
> > +	printk(KERN_DEBUG "VLYNQ local=%p remote=%p\n",
> > +			dev->local, dev->remote);
> > +	for (i = 0; i < 32; i++) {
> > +		printk(KERN_DEBUG "VLYNQ: local %d: %08x\n",
> > +			i + 1, ((u32 *)dev->local)[i]);
> > +		printk(KERN_DEBUG "VLYNQ: remote %d: %08x\n",
> > +			i + 1, ((u32 *)dev->remote)[i]);
> > +	}
> > +}
> > +
> > +static void vlynq_dump_mem(u32 *base, int count)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < (count + 3) / 4; i++) {
> > +		if (i % 4 == 0)
> > +			printk(KERN_DEBUG "\nMEM[0x%04x]:", i * 4);
> > +		printk(KERN_DEBUG " 0x%08x", *(base + i));
> > +	}
> > +	printk(KERN_DEBUG "\n");
> > +}
>
> lib/hexdump.c?

Ok will use it.

>
> > +#endif
> > +
> > +int vlynq_linked(struct vlynq_device *dev)
>
> afacit this didn't need to be a kernel-wide symbol?
>
> Please review the patchset for any unnecessarily-global symbols.
>
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < 100; i++)
> > +		if (vlynq_reg_read(dev->local->status) & VLYNQ_STATUS_LINK)
> > +			return 1;
> > +		else
> > +			cpu_relax();
> > +
> > +	return 0;
> > +}
> > +
> >
> > ...
> >
> > +static void vlynq_remote_ack(unsigned int irq)
> > +{
> > +	struct vlynq_device *dev = get_irq_chip_data(irq);
> > +
> > +	u32 status = vlynq_reg_read(dev->remote->status);
> > +
> > +	if (printk_ratelimit())
> > +		printk(KERN_DEBUG "%s: remote status: 0x%08x\n",
> > +		       dev_name(&dev->dev), status);
> > +	vlynq_reg_write(dev->remote->status, status);
> > +}
>
> This code seems to do rather a lot of printks for production code?

I will use pr_debug instead.

>
> > ...
> >
> > +static int vlynq_device_match(struct device *dev,
> > +			      struct device_driver *drv)
> > +{
> > +	struct vlynq_device *vdev = to_vlynq_device(dev);
> > +	struct vlynq_driver *vdrv = to_vlynq_driver(drv);
> > +	struct vlynq_device_id *ids = vdrv->id_table;
> > +
> > +	while (ids->id) {
> > +		if (ids->id == vdev->dev_id) {
> > +			vdev->divisor = ids->divisor;
> > +			vlynq_set_drvdata(vdev, ids);
> > +			printk(KERN_INFO "Driver found for VLYNQ " \
> > +				"device: %08x\n", vdev->dev_id);
> > +			return 1;
> > +		}
> > +		printk(KERN_DEBUG "Not using the %08x VLYNQ device's driver" \
> > +			" for VLYNQ device: %08x\n", ids->id, vdev->dev_id);
>
> The backslashes here are unneeded and unconventional.
>
> > +		ids++;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int vlynq_device_probe(struct device *dev)
> > +{
> > +	struct vlynq_device *vdev = to_vlynq_device(dev);
> > +	struct vlynq_driver *drv = to_vlynq_driver(dev->driver);
> > +	struct vlynq_device_id *id = vlynq_get_drvdata(vdev);
> > +	int result = -ENODEV;
> > +
> > +	get_device(dev);
> > +	if (drv && drv->probe)
> > +		result = drv->probe(vdev, id);
>
> Can `drv' really be NULL here?
>
> > +	if (result)
> > +		put_device(dev);
> > +	return result;
> > +}
> > +
> > +static int vlynq_device_remove(struct device *dev)
> > +{
> > +	struct vlynq_driver *drv = to_vlynq_driver(dev->driver);
> > +	if (drv && drv->remove)
> > +		drv->remove(to_vlynq_device(dev));
>
> ditto.
>
> > +	put_device(dev);
> > +	return 0;
> > +}
> > +
> > +int __vlynq_register_driver(struct vlynq_driver *driver, struct module
> > *owner) +{
> > +	driver->driver.name = driver->name;
> > +	driver->driver.bus = &vlynq_bus_type;
> > +	return driver_register(&driver->driver);
> > +}
> > +EXPORT_SYMBOL(__vlynq_register_driver);
> > +
> > +void vlynq_unregister_driver(struct vlynq_driver *driver)
> > +{
> > +	driver_unregister(&driver->driver);
> > +}
> > +EXPORT_SYMBOL(vlynq_unregister_driver);
> > +
> > +static int __vlynq_try_remote(struct vlynq_device *dev)
> > +{
> > +	int i;
> > +
> > +	vlynq_reset(dev);
> > +	for (i = dev->dev_id ? vlynq_rdiv2 : vlynq_rdiv8; dev->dev_id ?
> > +			i <= vlynq_rdiv8 : i >= vlynq_rdiv2;
> > +		dev->dev_id ? i++ : i--) {
>
> Wow.
>
> > +		if (!vlynq_linked(dev))
> > +			break;
> > +
> > +		vlynq_reg_write(dev->remote->control,
> > +				(vlynq_reg_read(dev->remote->control) &
> > +				~VLYNQ_CTRL_CLOCK_MASK) |
> > +				VLYNQ_CTRL_CLOCK_INT |
> > +				VLYNQ_CTRL_CLOCK_DIV(i - vlynq_rdiv1));
> > +		vlynq_reg_write(dev->local->control,
> > +				((vlynq_reg_read(dev->local->control)
> > +				& ~(VLYNQ_CTRL_CLOCK_INT |
> > +				VLYNQ_CTRL_CLOCK_MASK)) |
> > +				VLYNQ_CTRL_CLOCK_DIV(i - vlynq_rdiv1)));
> > +
> > +		if (vlynq_linked(dev)) {
> > +			printk(KERN_DEBUG
> > +				"%s: using remote clock divisor %d\n",
> > +				dev_name(&dev->dev), i - vlynq_rdiv1 + 1);
> > +			dev->divisor = i;
> > +			return 0;
> > +		} else {
> > +			vlynq_reset(dev);
> > +		}
> > +	}
> > +
> > +	return -ENODEV;
> > +}
>
> This code could do with a few comments.
>
> See, this reader of your code doesn't know what vlynq_linked() does, so
> I don't have a hope of working out whey this function returns -ENODEV
> if it couldn't find any "linked" devices.  What does this _mean_ in
> terms of the underlying hardware and its configuration?

VLYNQ devices can be daisy-chained that's why we want to know whether they are 
linked or not and return ENODEV in case there is no device.

>
> Dunno.  But it would be nice to be able to work that out from reading
> the code, no?
>
> > +static int __vlynq_try_local(struct vlynq_device *dev)
> > +{
> > +	int i;
> > +
> > +	vlynq_reset(dev);
> > +
> > +	for (i = dev->dev_id ? vlynq_ldiv2 : vlynq_ldiv8; dev->dev_id ?
> > +			i <= vlynq_ldiv8 : i >= vlynq_ldiv2;
> > +		dev->dev_id ? i++ : i--) {
> > +
> > +		vlynq_reg_write(dev->local->control,
> > +				(vlynq_reg_read(dev->local->control) &
> > +				~VLYNQ_CTRL_CLOCK_MASK) |
> > +				VLYNQ_CTRL_CLOCK_INT |
> > +				VLYNQ_CTRL_CLOCK_DIV(i - vlynq_ldiv1));
> > +
> > +		if (vlynq_linked(dev)) {
> > +			printk(KERN_DEBUG
> > +				"%s: using local clock divisor %d\n",
> > +				dev_name(&dev->dev), i - vlynq_ldiv1 + 1);
> > +			dev->divisor = i;
> > +			return 0;
> > +		} else {
> > +			vlynq_reset(dev);
> > +		}
> > +	}
> > +
> > +	return -ENODEV;
> > +}
> > +
> > +static int __vlynq_try_external(struct vlynq_device *dev)
> > +{
> > +	vlynq_reset(dev);
> > +	if (!vlynq_linked(dev))
> > +		return -ENODEV;
> > +
> > +	vlynq_reg_write(dev->remote->control,
> > +			(vlynq_reg_read(dev->remote->control) &
> > +			~VLYNQ_CTRL_CLOCK_INT));
> > +
> > +	vlynq_reg_write(dev->local->control,
> > +			(vlynq_reg_read(dev->local->control) &
> > +			~VLYNQ_CTRL_CLOCK_INT));
> > +
> > +	if (vlynq_linked(dev)) {
> > +		printk(KERN_DEBUG "%s: using external clock\n",
> > +			dev_name(&dev->dev));
> > +			dev->divisor = vlynq_div_external;
> > +		return 0;
> > +	}
> > +
> > +	return -ENODEV;
> > +}
>
> "remote", "local", "external".  What do these terms mean?  Perhaps
> there's a TI datasheet somewhere?

There is an implementation datasheet here : 
http://www.ti.com/litv/pdf/sprue36a which is pretty detailed. "remote" refers 
to the device that is connected to the vlynq bus master and is a separate 
chip from the SoC.

"external" and "local" refer to the clockig methods that can be used to clock 
out remote VLYNQ devices. "external" clocking source is an oscillator shared 
by both the bus master and the remote device, while "local" uses a dedicated 
clocking line between the bus master and the remote device.

>
> > +static int __vlynq_enable_device(struct vlynq_device *dev)
> > +{
> > +	int result;
> > +	struct plat_vlynq_ops *ops = dev->dev.platform_data;
> > +
> > +	result = ops->on(dev);
> > +	if (result)
> > +		return result;
> > +
> > +	switch (dev->divisor) {
> > +	case vlynq_div_external:
> > +	case vlynq_div_auto:
> > +		/* When the device is brought from reset it should have clock
> > +		generation negotiated by hardware.
> > +		Check which device is generating clocks and perform setup
> > +		accordingly */
>
> Preferred comment layout format is
>
> 		/*
> 		 * When the device is brought from reset it should have clock
> 		 * generation negotiated by hardware.  Check which device is
> 		 * generating clocks and perform setup accordingly
> 		 */
>
> > +		if (vlynq_linked(dev) && vlynq_reg_read(dev->remote->control) &
> > +		   VLYNQ_CTRL_CLOCK_INT) {
> > +			if (!__vlynq_try_remote(dev) ||
> > +				!__vlynq_try_local(dev)  ||
> > +				!__vlynq_try_external(dev))
> > +				return 0;
> > +		} else {
> > +			if (!__vlynq_try_external(dev) ||
> > +				!__vlynq_try_local(dev)    ||
> > +				!__vlynq_try_remote(dev))
> > +				return 0;
> > +		}
> > +		break;
> > +	case vlynq_ldiv1: case vlynq_ldiv2: case vlynq_ldiv3: case vlynq_ldiv4:
> > +	case vlynq_ldiv5: case vlynq_ldiv6: case vlynq_ldiv7: case vlynq_ldiv8:
>
> One `case' per line, please.
>
> > +		vlynq_reg_write(dev->local->control,
> > +				VLYNQ_CTRL_CLOCK_INT |
> > +				VLYNQ_CTRL_CLOCK_DIV(dev->divisor -
> > +						     vlynq_ldiv1));
> > +		vlynq_reg_write(dev->remote->control, 0);
> > +		if (vlynq_linked(dev)) {
> > +			printk(KERN_DEBUG
> > +				"%s: using local clock divisor %d\n",
> > +				dev_name(&dev->dev),
> > +				dev->divisor - vlynq_ldiv1 + 1);
> > +			return 0;
> > +		}
> > +		break;
> > +	case vlynq_rdiv1: case vlynq_rdiv2: case vlynq_rdiv3: case vlynq_rdiv4:
> > +	case vlynq_rdiv5: case vlynq_rdiv6: case vlynq_rdiv7: case vlynq_rdiv8:
> > +		vlynq_reg_write(dev->local->control, 0);
> > +		vlynq_reg_write(dev->remote->control,
> > +				VLYNQ_CTRL_CLOCK_INT |
> > +				VLYNQ_CTRL_CLOCK_DIV(dev->divisor -
> > +						     vlynq_rdiv1));
> > +		if (vlynq_linked(dev)) {
> > +			printk(KERN_DEBUG
> > +				"%s: using remote clock divisor %d\n",
> > +				dev_name(&dev->dev),
> > +				dev->divisor - vlynq_rdiv1 + 1);
> > +			return 0;
> > +		}
> > +		break;
> > +	}
> > +
> > +	ops->off(dev);
> > +	return -ENODEV;
> > +}
> > +
> > +int vlynq_enable_device(struct vlynq_device *dev)
> > +{
> > +	struct plat_vlynq_ops *ops = dev->dev.platform_data;
> > +	int result = -ENODEV;
> > +
> > +	result = __vlynq_enable_device(dev);
> > +	if (result)
> > +		return result;
> > +
> > +	result = vlynq_setup_irq(dev);
> > +	if (result)
> > +		ops->off(dev);
>
> It's strange that this function directly calls __vlynq_enable_device(),
> and undoes that via ops->off() if vlynq_setup_irq() failed.
>
> I'd have expected to see a call to ops->off() used as a cancallation
> for ops->on()?

It would be more consistent, you are right.

>
> > +	dev->enabled = !result;
> > +	return result;
> > +}
> > +EXPORT_SYMBOL(vlynq_enable_device);
> > +
> > +
> >
> > ...
> >
> > +struct vlynq_device {
> > +	u32 id, dev_id;
> > +	int local_irq;
> > +	int remote_irq;
> > +	enum vlynq_divisor divisor;
> > +	u32 regs_start, regs_end;
> > +	u32 mem_start, mem_end;
>
> This doesn't look 64-bit-bus friendly.

The bus is not intended to be available on 64-bits architecture, at least I 
did not see any using VLYNQ and since existing users of it are MIPS32 or 
ARM9-based System-on-Chip, TI would have to redesign/extend VLYNQ to make it 
work on 64-bits architecture.

Thanks for your comments, will respin with them addressed.
-- 
Best regards, Florian Fainelli
Email : florian@xxxxxxxxxxx
http://openwrt.org
-------------------------------
--
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/