On Mon, 2015-02-16 at 09:46 -0600, Emil Medve wrote:
From: Geoff Thorpe geoff.tho...@freescale.com
Change-Id: I075944acf740dbaae861104c17a9ff7247dec1be
Signed-off-by: Geoff Thorpe geoff.tho...@freescale.com
Remove Change-Id.
Provide a description of what BMan is.
diff --git a/drivers/soc/freescale/Kconfig b/drivers/soc/freescale/Kconfig
new file mode 100644
index 000..63bf002
--- /dev/null
+++ b/drivers/soc/freescale/Kconfig
@@ -0,0 +1,51 @@
+menuconfig FSL_DPA
+ tristate Freescale DPAA Buffer management
+ depends on HAS_FSL_QBMAN
+ default y
default y is normally a bad idea for drivers (though it can make sense
for options within a driver).
Where is HAS_FSL_QBMAN defined (if the answer is in a later patch,
rearrange it)?
+if FSL_DPA
+
+config FSL_DPA_CHECKING
+ bool additional driver checking
Option texts are usually capitalized.
+ default n
+ ---help---
+ Compiles in additional checks to sanity-check the drivers and any
+ use of it by other code. Not recommended for performance.
+
+config FSL_DPA_CAN_WAIT
+ bool
+ default y
+config FSL_DPA_CAN_WAIT_SYNC
+ bool
+ default y
+
+config FSL_DPA_PIRQ_FAST
+ bool
+ default y
Use consistent whitespace.
Add help texts to these options to document them, even if they're not
user-visible.
+config FSL_BMAN_CONFIG
+ bool BMan device management
+ default y
+ ---help---
+ If this linux image is running natively, you need this option. If this
+ linux image is running as a guest OS under the hypervisor, only one
+ guest OS (the control plane) needs this option.
the hypervisor? I suspect this refers to the Freescale Embedded
Hypervisor (a.k.a. Topaz), rather than KVM or something else.
It would also be nice if the help text explained what the option does,
rather than just when it's needed.
+struct bman;
Where is this struct defined?
+union bman_ecir {
+ u32 ecir_raw;
+ struct {
+ u32 __reserved1:4;
+ u32 portal_num:4;
+ u32 __reserved2:12;
+ u32 numb:4;
+ u32 __reserved3:2;
+ u32 pid:6;
+ } __packed info;
+};
Get rid of __packed. It causes GCC to generate terrible code with
bitfields. For that matter, consider getting rid of the bitfields.
+#define BMAN_HWE_TXT(a, b) { .mask = BM_EIRQ_##a, .txt = b }
+
+static const struct bman_hwerr_txt bman_hwerr_txts[] = {
+ BMAN_HWE_TXT(IVCI, Invalid Command Verb),
+ BMAN_HWE_TXT(FLWI, FBPR Low Watermark),
+ BMAN_HWE_TXT(MBEI, Multi-bit ECC Error),
+ BMAN_HWE_TXT(SBEI, Single-bit ECC Error),
+ BMAN_HWE_TXT(BSCN, Pool State Change Notification),
+};
+#define BMAN_HWE_COUNT (sizeof(bman_hwerr_txts)/sizeof(struct
bman_hwerr_txt))
Use ARRAY_SIZE(), here and elsewhere.
+/* Add this in Kconfig */
+#define BMAN_ERRS_TO_UNENABLE (BM_EIRQ_FLWI)
Add what to kconfig?
+/**
+ * bm_err_isr_reg_verb - Manipulate global interrupt registers
+ * @v: for accessors that write values, this is the 32-bit value
+ *
+ * Manipulates BMAN_ERR_ISR, BMAN_ERR_IER, BMAN_ERR_ISDR, BMAN_ERR_IIR. All
+ * manipulations except bm_err_isr_[un]inhibit() use 32-bit masks composed of
+ * the BM_EIRQ_*** definitions. Note that bm_err_isr_enable_write means
+ * write the enable register rather than enable the write register!
+ */
+#define bm_err_isr_status_read(bm) \
+ __bm_err_isr_read(bm, bm_isr_status)
+#define bm_err_isr_status_clear(bm, m) \
+ __bm_err_isr_write(bm, bm_isr_status, m)
+#define bm_err_isr_enable_read(bm) \
+ __bm_err_isr_read(bm, bm_isr_enable)
+#define bm_err_isr_enable_write(bm, v) \
+ __bm_err_isr_write(bm, bm_isr_enable, v)
+#define bm_err_isr_disable_read(bm) \
+ __bm_err_isr_read(bm, bm_isr_disable)
+#define bm_err_isr_disable_write(bm, v) \
+ __bm_err_isr_write(bm, bm_isr_disable, v)
+#define bm_err_isr_inhibit(bm) \
+ __bm_err_isr_write(bm, bm_isr_inhibit, 1)
+#define bm_err_isr_uninhibit(bm) \
+ __bm_err_isr_write(bm, bm_isr_inhibit, 0)
Is this layer really helpful?
+/*
+ * TODO: unimplemented registers
+ *
+ * BMAN_POOLk_SDCNT, BMAN_POOLk_HDCNT, BMAN_FULT,
+ * BMAN_VLDPL, BMAN_EECC, BMAN_SBET, BMAN_EINJ
+ */
What does it mean for registers to be unimplemented, in a piece of
software? If you mean accessors to those registers, why is that needed
if nothing uses them (yet)?
+/* Encapsulate struct bman * as a cast of the register space address. */
+
+static struct bman *bm_create(void *regs)
+{
+ return (struct bman *)regs;
+}
Unnecessary cast -- and unnecessary encapsulation (especially since you
only use it once). It's also missing __iomem -- have you run sparse on
this?
+static u32 __generate_thresh(u32 val, int roundup)
+{
+ u32 e = 0; /* co-efficient, exponent */
+
