Thread (2 messages) 2 messages, 2 authors, 2011-10-21

Re: [PATCH v15 03/10] USB/ppc4xx: Add Synopsys DWC OTG Core Interface Layer (CIL)

From: Olof Johansson <hidden>
Date: 2011-10-21 17:43:38

Hi,

Some hit-and-run comments below.


-Olof

On Fri, Oct 14, 2011 at 03:08:49PM -0700, tmarri@apm.com wrote:
quoted hunk ↗ jump to hunk
diff --git a/drivers/usb/dwc/cil.h b/drivers/usb/dwc/cil.h
new file mode 100644
index 0000000..8c29678
--- /dev/null
+++ b/drivers/usb/dwc/cil.h
@@ -0,0 +1,1174 @@
+/*
+ * DesignWare HS OTG controller driver
+ * Copyright (C) 2006 Synopsys, Inc.
+ * Portions Copyright (C) 2010 Applied Micro Circuits Corporation.
+ *
+ * This program is free software: you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License version 2 for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see http://www.gnu.org/licenses
+ * or write to the Free Software Foundation, Inc., 51 Franklin Street,
+ * Suite 500, Boston, MA 02110-1335 USA.
+ *
+ * Based on Synopsys driver version 2.60a
+ * Modified by Mark Miesfeld <mmiesfeld@apm.com>
+ * Modified by Stefan Roese <sr@denx.de>, DENX Software Engineering
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING BUT NOT LIMITED TO THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL SYNOPSYS, INC. BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+ * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ */
+
+#if !defined(__DWC_CIL_H__)
+#define __DWC_CIL_H__
+#include <linux/io.h>
+#include <linux/usb/ch9.h>
+#include <linux/usb/gadget.h>
+#include <linux/interrupt.h>
+#include <linux/dmapool.h>
+#include <linux/spinlock.h>
+#include <linux/usb/otg.h>
+
+#include "regs.h"
+
+#ifdef CONFIG_DWC_DEBUG
+#define DEBUG
+#endif
+
+/**
+ * Reads the content of a register.
+ */
+static inline u32 dwc_reg_read(ulong reg , u32 offset)
+{
+
+#ifdef CONFIG_DWC_OTG_REG_LE
+	return in_le32((unsigned __iomem *)(reg + offset));
+#else
+	return in_be32((unsigned __iomem *)(reg + offset));
+#endif
+}
+
+static inline void dwc_reg_write(ulong reg, u32 offset, const u32 value)
+{
+#ifdef CONFIG_DWC_OTG_REG_LE
+	out_le32((unsigned __iomem *)(reg + offset), value);
+#else
+	out_be32((unsigned __iomem *)(reg + offset), value);
+#endif
+};
NACK. This is powerpc-specific code.

Also, your function has to take the register as a void __iomem *  type instead
of casting it here.

Finally, please try to abstract away the endian ness a bit further, instead of
duplicating each implementation here twice.
+/*
+ * Debugging support vanishes in non-debug builds.
+ */
+/* Display CIL Debug messages */
+#define dwc_dbg_cil		(0x2)
+
+/* Display CIL Verbose debug messages */
+#define dwc_dbg_cilv		(0x20)
+
+/* Display PCD (Device) debug messages */
+#define dwc_dbg_pcd		(0x4)
+
+/* Display PCD (Device) Verbose debug  messages */
+#define dwc_dbg_pcdv		(0x40)
+
+/* Display Host debug messages */
+#define dwc_dbg_hcd		(0x8)
+
+/* Display Verbose Host debug messages */
+#define dwc_dbg_hcdv		(0x80)
+
+/* Display enqueued URBs in host mode. */
+#define dwc_dbg_hcd_urb		(0x800)
+
+/* Display "special purpose" debug messages */
+#define dwc_dbg_sp		(0x400)
+
+/* Display all debug messages */
+#define dwc_dbg_any		(0xFF)
+
+/* All debug messages off */
+#define dwc_dbg_off		0
+
+/* Prefix string for DWC_DEBUG print macros. */
+#define usb_dwc "dwc_otg: "
These don't seem to have to do with CIL, so should go in their own debug header
perhaps?
+/*
+ * Added-sr: 2007-07-26
Irrellevant in the upstream context.
+	/* Internal DWC HCD Flags */
+	union dwc_otg_hcd_internal_flags {
+		u32 d32;
+		struct {
+			unsigned port_connect_status_change:1;
+			unsigned port_connect_status:1;
+			unsigned port_reset_change:1;
+			unsigned port_enable_change:1;
+			unsigned port_suspend_change:1;
+			unsigned port_over_current_change:1;
+			unsigned reserved:27;
+		} b;
+	} flags;
Please don't use structs/unions for register definitions.
+static int dwc_otg_handle_otg_intr(struct core_if *core_if)
+{
This function looks a bit long and indentation is deep. Please refactor into helpers.


-Olof
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help