Message-Id: <1237239724.3106.68.camel@achroite>
Date:	Mon, 16 Mar 2009 21:42:04 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	ram.vepa@...erion.com
Cc:	Netdev <netdev@...r.kernel.org>, David Miller <davem@...emloft.net>
Subject: Re: [net-2.6 PATCH 6/10] Neterion: New driver: Hardware init &
	configuration

On Sat, 2009-03-14 at 00:21 -0800, Ramkrishna Vepa wrote:
> This patch takes care of Initialization and configuration steps of
> Neterion Inc's X3100 Series 10GbE PCIe I/O Virtualized Server Adapter.
> - Device Initialization.
> - Verification and setting of device config parameters.
> - Allocation of Tx FIFO and Rx Ring descriptors (DTR).
> - APIs to get various type of hw stats
> - APIs to configure RTS (Receive Traffic Steering)
> 
> Signed-off-by: Sivakumar Subramani <sivakumar.subramani@...erion.com>
> Signed-off-by: Rastapur Santosh <santosh.rastapur@...erion.com>
> Signed-off-by: Ramkrishna Vepa <ram.vepa@...erion.com>
> ---
> diff -urpN patch_5/drivers/net/vxge/vxge-config.c patch_6/drivers/net/vxge/vxge-config.c
> --- patch_5/drivers/net/vxge/vxge-config.c	1969-12-31 16:00:00.000000000 -0800
> +++ patch_6/drivers/net/vxge/vxge-config.c	2009-03-13 00:11:27.000000000 -0700
> @@ -0,0 +1,7576 @@
> +/******************************************************************************
> + * This software may be used and distributed according to the terms of
> + * the GNU General Public License (GPL), incorporated herein by reference.
> + * Drivers based on or derived from this code fall under the GPL and must
> + * retain the authorship, copyright and license notice.  This file is not
> + * a complete program and may only be used when the entire operating
> + * system is licensed under the GPL.
> + * See the file COPYING in this distribution for more information.
> + ******************************************************************************/
> +/*******************************************************************************
> + * vxge-config.c: Driver for Neterion Inc's X3100 Series 10GbE PCIe I/O
> + *                Virtualized Server Adapter.
> + * Copyright(c) 2002-2009 Neterion Inc.
> + ******************************************************************************/
> +#include <linux/version.h>
> +#include <linux/netdevice.h>
> +#include <linux/etherdevice.h>
> +#include <linux/pci.h>
> +#include <linux/delay.h>
> +
> +#include "vxge-traffic.h"
> +#include "vxge-config.h"
> +
> +/*

You need to start a comment with "/**" to make it a kernel-doc comment.

> + * __hw_channel_allocate - Allocate memory for channel
> + * @devh: Handle to the device object
> + * @vph: Handle to Virtual Path
> + * @type: Type of channel
> + * @length: Lengths of arrays
> + * @alloc_work_array: Flag to specify if work array is required
> + * @alloc_free_array: Flag to specify if free array is required
> + * @alloc_reserve_array: Flag to specify if reserve array is required
> + * @per_dtr_space: driver requested per dtr space to be allocated in priv
> + * @userdata: User data to be passed back in the callback
> + *
> + * This function allocates required memory for the channel and various arrays
> + * in the channel
> + */
> +struct __hw_channel*
> +__hw_channel_allocate(

Drivers in the kernel tree are not necessarily compiled as modules, so
their external symbols must be globally unique.  You should use a
driver- specific prefix, presumably "vxge" (possibly with "__" in front
of that).

[...]
> +/*
> + * __hw_channel_free - Free memory allocated for channel
> + * @channel: channel to be freed
> + *
> + * This function deallocates memory from the channel and various arrays
> + * in the channel
> + */
> +void
> +__hw_channel_free(
> +	struct __hw_channel *channel)
> +{
> +	u32	vp_id = 0;
> +	struct __hw_device  *hldev;
> +
> +	vxge_assert(channel != NULL);
> +
> +	hldev = (struct __hw_device  *)channel->devh;
> +
> +	vp_id = ((struct __hw_vpath_handle *)channel->vph)->vpath->vp_id;
> +
> +	if (channel->work_arr) {
> +		vfree(channel->work_arr);
> +		channel->work_arr = NULL;

Why bother clearing members of *channel just before you free it?

[...]
> +/*
> + * __hw_channel_initialize - Initialize a channel
> + * @channel: channel to be initialized
> + *
> + * This function initializes a channel by properly setting the
> + * various references
> + */
> +enum vxge_hw_status
> +__hw_channel_initialize(
> +	struct __hw_channel *channel)
> +{
> +	u32 i;
> +	struct __hw_virtualpath *vpath;
> +
> +	vxge_assert(channel != NULL);
> +
> +	vpath = (struct __hw_virtualpath *)((struct __hw_vpath_handle *) \
> +						channel->vph)->vpath;

Why are you using backslashes in multi-line statements?  This is only
necessary in macro definitions.

[...]
> +/*
> + * __hw_device_pci_caps_list_process
> + * @hldev: HW device handle.
> + *
> + * Process PCI capabilities and initialize the offsets
> + */
> +void
> +__hw_device_pci_caps_list_process(struct __hw_device *hldev)

This is redundant with pci_find_capability().

[...]
> +/*
> + * __hw_device_pci_e_init
> + * @hldev: HW device handle.
> + *
> + * Initialize certain PCI/PCI-X configuration registers
> + * with recommended values. Save config space for future hw resets.
> + */
> +void
> +__hw_device_pci_e_init(struct __hw_device *hldev)#
> +{
> +	int i;
> +	u16 cmd = 0;
> +	u16 device_id;
> +	u8  revision;
> +
> +	vxge_assert(hldev != NULL);
> +
> +	/* Store PCI device ID and revision for future references where in we
> +	 * decide Xena revision using PCI sub system ID */
> +	pci_read_config_word(hldev->pdev,
> +			vxge_offsetof(struct vxge_hw_pci_config_le, device_id),
> +			&device_id);
> +
> +	pci_read_config_byte(hldev->pdev, vxge_offsetof(
> +				struct vxge_hw_pci_config_le, revision),
> +				&revision);

This information is in *hldev->pdev already.

> +	/* save original PCI config space to restore it on device_terminate() */
> +	for (i = 0; i < VXGE_HW_PCI_CONFIG_SPACE_SIZE/4; i++) {
> +		pci_read_config_dword(hldev->pdev, i * 4,
> +				(u32 *)&hldev->pci_config_space_bios  +  i);
> +	}

Since you only change two bits in the command register, maybe you should
just save those bits.  Or not bother.

> +	__hw_device_pci_caps_list_process(hldev);
> +
> +	/* Set the PErr Repconse bit and SERR in PCI command register. */
> +	pci_read_config_word(hldev->pdev,
> +			vxge_offsetof(struct vxge_hw_pci_config_le, command),
> +			&cmd);
> +	cmd |= 0x140;
> +	pci_write_config_word(hldev->pdev,
> +			vxge_offsetof(struct vxge_hw_pci_config_le, command),
> +			cmd);
> +
> +	/* save PCI config space for future resets */
> +	for (i = 0; i < VXGE_HW_PCI_CONFIG_SPACE_SIZE/4; i++) {
> +		pci_read_config_dword(hldev->pdev, i * 4,
> +					(u32 *)&hldev->pci_config_space  +  i);
> +	}

You should use pci_save_state() and pci_restore_state() around resets
instead.

> +	return;
> +}
> +
> +/*
> + * __hw_device_register_poll
> + * @reg: register to poll for
> + * @op: 0 - bit reset, 1 - bit set
> + * @mask: mask for logical "and" condition based on %op
> + * @max_millis: maximum time to try to poll in milliseconds
> + *
> + * Will poll certain register for specified amount of time.
> + * Will poll until masked bit is not cleared.
> + */
> +enum vxge_hw_status
> +__hw_device_register_poll(u64		*reg,
> +			u32		op,
> +			u64		mask,
> +			u32		max_millis)
> +{
> +	u64 val64;
> +	u32 i = 0;
> +	enum vxge_hw_status ret = VXGE_HW_FAIL;
> +
> +	vxge_os_udelay(10);
> +
> +	do {
> +		val64 = readq(reg);
> +		if (op == 0 && !(val64 & mask))
> +			return VXGE_HW_OK;
> +		else if (op == 1 && (val64 & mask) == mask)
> +			return VXGE_HW_OK;
> +		vxge_os_udelay(100);

udelay(100)

> +	} while (++i <= 9);

You need to reset i to 0 or 1 after this, since you've waited 910 us not
9 ms.

> +	do {
> +		val64 = readq(reg);
> +		if (op == 0 && !(val64 & mask))
> +			return VXGE_HW_OK;
> +		else if (op == 1 && (val64 & mask) == mask)
> +			return VXGE_HW_OK;
> +		vxge_os_udelay(1000);

mdelay(1)

> +	} while (++i < max_millis);

This should probably be a while() not a do...while().

> +	return ret;
> +}
> +
> +/*
> + * __hw_device_toc_get
> + * @bar0: Address of BAR0 in PCI config
> + *
> + * This routine sets the sapper and reads the toc pointer and returns the

Based on the function call below I'm guessing "sapper" should be
"swapper".

> + * memory mapped address of the toc
> + */
> +struct vxge_hw_toc_reg *
> +__hw_device_toc_get(u8 *bar0)
> +{
> +	u64 val64;
> +	struct vxge_hw_toc_reg *toc = NULL;
> +	enum vxge_hw_status status;
> +	struct vxge_hw_legacy_reg *legacy_reg =
> +		(struct vxge_hw_legacy_reg *)bar0;
> +
> +	status = __hw_legacy_swapper_set(legacy_reg);
> +	if (status != VXGE_HW_OK)
> +		goto exit;
> +
> +	val64 =	readq(&legacy_reg->toc_first_pointer);
> +
> +	toc = (struct vxge_hw_toc_reg *)(bar0+val64);
> +
> +exit:
> +
> +	return toc;
> +}
> +
> +/*
> + * __hw_device_reg_addr_get
> + * @hldev: HW Device object.
> + *
> + * This routine sets the swapper and reads the toc pointer and initializes the
> + * register location pointers in the device object. It waits until the ric is
> + * completed initializing registers.
> + */
> +enum vxge_hw_status
> +__hw_device_reg_addr_get(struct __hw_device *hldev)
> +{
> +	u64 val64;
> +	u32 i;
> +	enum vxge_hw_status status = VXGE_HW_OK;
> +
> +	vxge_assert(hldev);
> +
> +	hldev->legacy_reg = (struct vxge_hw_legacy_reg *)hldev->bar0;
> +
> +	hldev->toc_reg = __hw_device_toc_get(hldev->bar0);
> +	if (hldev->toc_reg  == NULL) {
> +		status = VXGE_HW_FAIL;
> +		goto exit;
> +	}
> +
> +	val64 = readq(&hldev->toc_reg->toc_common_pointer);
> +
> +	hldev->common_reg =
> +		(struct vxge_hw_common_reg *)(hldev->bar0  +  val64);
> +
> +	val64 = readq(&hldev->toc_reg->toc_memrepair_pointer);
> +
> +	hldev->memrepair_reg =
> +		(struct vxge_hw_memrepair_reg *)(hldev->bar0  +  val64);
> +
> +	for (i = 0; i < VXGE_HW_TITAN_PCICFGMGMT_REG_SPACES; i++) {
> +
> +		val64 = readq(&hldev->toc_reg->toc_pcicfgmgmt_pointer[i]);
> +
> +		hldev->pcicfgmgmt_reg[i] = (struct vxge_hw_pcicfgmgmt_reg *) \
> +						(hldev->bar0  +  val64);
> +
> +	}

This function

might have 

too much

vertical whitespace.

[...]
> +/**
> + * __hw_device_get_vpd_data - Getting vpd_data.
> + *
> + *   @hldev: HW device handle.
> + *
> + *   Getting  product name and serial number from vpd capabilites structure
> + *
> + */
> +void
> +__hw_device_get_vpd_data(struct __hw_device *hldev)
> +{
> +	u8   *vpd_data;
> +	u16  data;
> +	u32  data32;
> +	u32  index = 0, i, count, fail = 0;
> +	u32  addr_offset;
> +	u32  data_offset;
> +	u32  max_count = hldev->config.device_poll_millis * 10;
> +
> +	vxge_assert(hldev);
> +
> +	addr_offset = hldev->pci_caps.vpd_cap_offset  +
> +		vxge_offsetof(struct vxge_hw_vpid_capability_le, vpd_address);
> +
> +	data_offset = hldev->pci_caps.vpd_cap_offset  +
> +		vxge_offsetof(struct vxge_hw_vpid_capability_le, vpd_data);
> +	strcpy((char *) hldev->vpd_data.product_name,
> +		"10 Gigabit Ethernet Adapter");
> +
> +	strcpy((char *) hldev->vpd_data.serial_num, "not available");
> +
> +	if ((hldev->host_type != VXGE_HW_NO_MR_NO_SR_NORMAL_FUNCTION) ||
> +			(hldev->func_id != 0)) {
> +		strcpy((char *) hldev->vpd_data.serial_num,
> +				"not supported");
> +		strcpy((char *) hldev->vpd_data.product_name,
> +				"not supported");
> +		return;
> +	}
> +	vpd_data = (u8 *) vmalloc(VXGE_HW_VPD_BUFFER_SIZE  +  16);
> +	if (vpd_data == 0)
> +		return;
> +	for (index = 0; index < VXGE_HW_VPD_BUFFER_SIZE; index += 4) {

Use pci_read_vpd() instead of this loop.

[...]
> +/*
> + * vxge_hw_wrr_rebalance - Rebalance the RX_WRR and KDFC_WRR calandars.
> + * @devh: HW device handle.
> + *
> + * Rebalance the RX_WRR and KDFC_WRR calandars.
> + *
> + */
> +enum vxge_hw_status vxge_hw_wrr_rebalance(struct __hw_device *hldev)
> +{
> +	u64 val64;
> +	u32 calender[176];
[...]

Why 176?  This value should have at least a comment, but preferably a
name or explicit formula.

Also should this variable be called "calendar"?

This is as far as I got.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists