To dump a particular register, echo reg_addr > <debugfs_root>/ieee80211/phyX/ath6kl/reg_addr,
to dump the entire register set, echo 0 > <debugfs_root>/ieee80211/phyX/ath6kl/reg_addr, and
register values will be available at <debugfs_root>/ieee80211/phyX/ath6kl/reg_dump.
Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
drivers/net/wireless/ath/ath6kl/core.h | 1 +
drivers/net/wireless/ath/ath6kl/debug.c | 191 +++++++++++++++++++++++++++++++
2 files changed, 192 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath6kl/core.h b/drivers/net/wireless/ath/ath6kl/core.h
index 61d3142..4f9670b 100644
--- a/drivers/net/wireless/ath/ath6kl/core.h
+++ b/drivers/net/wireless/ath/ath6kl/core.h
@@ -480,6 +480,7 @@ struct ath6kl {
} debug;
#endif /* CONFIG_ATH6KL_DEBUG */
+ unsigned int dbgfs_diag_reg;
};
static inline void *ath6kl_priv(struct net_device *dev)
diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
index 9608692..6ecfc70 100644
--- a/drivers/net/wireless/ath/ath6kl/debug.c
+++ b/drivers/net/wireless/ath/ath6kl/debug.c
@@ -54,6 +54,28 @@ int ath6kl_printk(const char *level, const char *fmt, ...)
}
#ifdef CONFIG_ATH6KL_DEBUG
+
+#define REG_OUTPUT_LEN_PER_LINE 25
+#define ATH6KL_REG_TYPE_MAX 8
+#define REGTYPE_STR_LEN 100
+
+struct ath6kl_diag_reg_info {
+ u32 reg_start;
+ u32 reg_end;
+ char *reg_info;
+};
+
+static const struct ath6kl_diag_reg_info diag_reg[ATH6KL_REG_TYPE_MAX] = {
+ { 0x20000, 0x200fc, "General DMA and Rx registers" },
+ { 0x28000, 0x28900, "MAC PCU register & keycache" },
+ { 0x20800, 0x20a40, "QCU" },
+ { 0x21000, 0x212f0, "DCU" },
+ { 0x4000, 0x42e4, "RTC" },
+ { 0x540000, 0x540000 + (256 * 1024), "RAM" },
+ { 0x29800, 0x2B210, "Base Band" },
+ { 0x1C000, 0x1C748, "Analog" },
+};
+
void ath6kl_dump_registers(struct ath6kl_device *dev,
struct ath6kl_irq_proc_registers *irq_proc_reg,
struct ath6kl_irq_enable_reg *irq_enable_reg)
@@ -533,6 +555,171 @@ static const struct file_operations fops_credit_dist_stats = {
.llseek = default_llseek,
};
+static unsigned long ath6kl_get_num_reg(void)
+{
+ int i;
+ unsigned long n_reg = 0;
+
+ for (i = 0; i < ATH6KL_REG_TYPE_MAX; i++)
+ n_reg = n_reg +
+ (diag_reg[i].reg_end - diag_reg[i].reg_start) / 4 + 1;
+
+ return n_reg;
+}
+
+static bool ath6kl_dbg_is_diag_reg_valid(u32 reg_addr)
+{
+ int i;
+
+ for (i = 0; i < ATH6KL_REG_TYPE_MAX; i++) {
+ if (reg_addr >= diag_reg[i].reg_start &&
+ reg_addr <= diag_reg[i].reg_end)
+ return true;
+ }
+
+ ath6kl_info("Invalid addr\n");
+ return false;
+}
+
+static ssize_t read_file_reg_read(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct ath6kl *ar = file->private_data;
+ u8 buf[50];
+ unsigned int len = 0;
+
+ if (ar->dbgfs_diag_reg)
+ len += scnprintf(buf + len, sizeof(buf) - len, "0x%u\n",
+ ar->dbgfs_diag_reg);
+ else
+ len += scnprintf(buf + len, sizeof(buf) - len,
+ "All diag registers\n");
+
+ return simple_read_from_buffer(user_buf, count, ppos, buf, len);
+}
+
+static ssize_t write_file_reg_read(struct file *file,
+ const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct ath6kl *ar = file->private_data;
+ u8 buf[50];
+ unsigned int len;
+ unsigned long reg_addr;
+
+ len = min(count, sizeof(buf) - 1);
+ if (copy_from_user(buf, user_buf, len))
+ return -EFAULT;
+
+ buf[len] = '\0';
+
+ if (strict_strtoul(buf, 0, ®_addr))
+ return -EINVAL;
+
+ if ((reg_addr % 4) != 0)
+ return -EINVAL;
+
+ if (reg_addr && !ath6kl_dbg_is_diag_reg_valid(reg_addr))
+ return -EINVAL;
+
+ ar->dbgfs_diag_reg = reg_addr;
+
+ return count;
+}
+
+static const struct file_operations fops_diag_reg_read = {
+ .read = read_file_reg_read,
+ .write = write_file_reg_read,
+ .open = ath6kl_debugfs_open,
+ .owner = THIS_MODULE,
+ .llseek = default_llseek,
+};
+
+static int open_file_reg_dump(struct inode *inode, struct file *file)
+{
+ struct ath6kl *ar = inode->i_private;
+ u8 *buf;
+ unsigned long int reg_len;
+ unsigned int len = 0, n_reg;
+ __le32 reg_val;
+ u32 addr;
+ int i, status;
+
+ /* Dump all the registers if no register is specified */
+ if (!ar->dbgfs_diag_reg)
+ n_reg = ath6kl_get_num_reg();
+ else
+ n_reg = 1;
+
+ reg_len = n_reg * REG_OUTPUT_LEN_PER_LINE;
+ if (n_reg > 1)
+ reg_len += REGTYPE_STR_LEN;
+
+ buf = vmalloc(reg_len);
+ if (!buf)
+ return -ENOMEM;
+
+ if (n_reg == 1) {
+ addr = ar->dbgfs_diag_reg;
+
+ status = ath6kl_diag_read32(ar,
+ cpu_to_le32(TARG_VTOP(ar->target_type, addr)),
+ ®_val);
+ if (status)
+ goto fail_reg_read;
+
+ len += scnprintf(buf + len, reg_len - len,
+ "0x%06x 0x%08x\n", addr, le32_to_cpu(reg_val));
+ } else {
+ for (i = 0; i < ATH6KL_REG_TYPE_MAX; i++) {
+ len += scnprintf(buf + len, reg_len - len,
+ "%s\n", diag_reg[i].reg_info);
+ for (addr = diag_reg[i].reg_start;
+ addr <= diag_reg[i].reg_end; addr += 4) {
+ status = ath6kl_diag_read32(ar,
+ cpu_to_le32(TARG_VTOP(ar->target_type,
+ addr)), ®_val);
+ if (status)
+ goto fail_reg_read;
+
+ len += scnprintf(buf + len, reg_len - len,
+ "0x%06x 0x%08x\n",
+ addr, le32_to_cpu(reg_val));
+ }
+ }
+ }
+
+ file->private_data = buf;
+
+ return 0;
+
+fail_reg_read:
+ ath6kl_warn("Unable to read memory:%u\n", addr);
+ vfree(buf);
+ return -EIO;
+}
+
+static ssize_t read_file_reg_dump(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ u8 *buf = file->private_data;
+ return simple_read_from_buffer(user_buf, count, ppos, buf, strlen(buf));
+}
+
+static int release_file_reg_dump(struct inode *inode, struct file *file)
+{
+ vfree(file->private_data);
+ return 0;
+}
+
+static const struct file_operations fops_reg_dump = {
+ .open = open_file_reg_dump,
+ .read = read_file_reg_dump,
+ .release = release_file_reg_dump,
+ .owner = THIS_MODULE,
+ .llseek = default_llseek,
+};
+
int ath6kl_debug_init(struct ath6kl *ar)
{
ar->debug.fwlog_buf.buf = vmalloc(ATH6KL_FWLOG_SIZE);
@@ -566,6 +753,10 @@ int ath6kl_debug_init(struct ath6kl *ar)
debugfs_create_file("credit_dist_stats", S_IRUSR, ar->debugfs_phy, ar,
&fops_credit_dist_stats);
+ debugfs_create_file("reg_addr", S_IRUSR, ar->debugfs_phy, ar,
+ &fops_diag_reg_read);
+ debugfs_create_file("reg_dump", S_IRUSR, ar->debugfs_phy, ar,
+ &fops_reg_dump);
debugfs_create_file("fwlog", S_IRUSR, ar->debugfs_phy, ar,
&fops_fwlog);
--
1.7.0.4
On Tue, Aug 30, 2011 at 08:46:59PM +0530, Vasanthakumar Thiagarajan wrote:
> To dump a particular register, echo reg_addr > <debugfs_root>/ieee80211/phyX/ath6kl/reg_addr,
> to dump the entire register set, echo 0 > <debugfs_root>/ieee80211/phyX/ath6kl/reg_addr, and
> register values will be available at <debugfs_root>/ieee80211/phyX/ath6kl/reg_dump.
>
I'm fixing endian issues with chip register read/write. Please drop
this patch. I'll rebase it with my endian patches.
Vasanth
On 08/30/2011 06:16 PM, Vasanthakumar Thiagarajan wrote:
> To dump a particular register, echo reg_addr > <debugfs_root>/ieee80211/phyX/ath6kl/reg_addr,
> to dump the entire register set, echo 0 > <debugfs_root>/ieee80211/phyX/ath6kl/reg_addr, and
> register values will be available at <debugfs_root>/ieee80211/phyX/ath6kl/reg_dump.
I like this :) The full dump just takes quite long, 22 seconds on my x86
box, and cat dumps everything only in the end. It would be nice to
improve that somehow by sending results in smaller pieces, but no need
to worry about that now.
I just saw your message about sending v2, but I had already started
reviewing this and didn't want to stop.
First, I saw few sparse warnings:
drivers/net/wireless/ath/ath6kl/debug.c:666:33: warning: incorrect type
in argument 2 (different base types)
drivers/net/wireless/ath/ath6kl/debug.c:666:33: expected unsigned int
[unsigned] [usertype] address
drivers/net/wireless/ath/ath6kl/debug.c:666:33: got restricted __le32
[usertype] <noident>
drivers/net/wireless/ath/ath6kl/debug.c:667:34: warning: incorrect type
in argument 3 (different base types)
drivers/net/wireless/ath/ath6kl/debug.c:667:34: expected unsigned int
[usertype] *value
drivers/net/wireless/ath/ath6kl/debug.c:667:34: got restricted __le32
*<noident>
drivers/net/wireless/ath/ath6kl/debug.c:680:41: warning: incorrect type
in argument 2 (different base types)
drivers/net/wireless/ath/ath6kl/debug.c:680:41: expected unsigned int
[unsigned] [usertype] address
drivers/net/wireless/ath/ath6kl/debug.c:680:41: got restricted __le32
[usertype] <noident>
drivers/net/wireless/ath/ath6kl/debug.c:681:50: warning: incorrect type
in argument 3 (different base types)
drivers/net/wireless/ath/ath6kl/debug.c:681:50: expected unsigned int
[usertype] *value
drivers/net/wireless/ath/ath6kl/debug.c:681:50: got restricted __le32
*<noident>
> --- a/drivers/net/wireless/ath/ath6kl/core.h
> +++ b/drivers/net/wireless/ath/ath6kl/core.h
> @@ -480,6 +480,7 @@ struct ath6kl {
> } debug;
> #endif /* CONFIG_ATH6KL_DEBUG */
>
> + unsigned int dbgfs_diag_reg;
> };
Please move this inside the debug struct above. I'm trying to categorise
struct ath6kl a bit to make it easier to understand.
> static inline void *ath6kl_priv(struct net_device *dev)
> diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
> index 9608692..6ecfc70 100644
> --- a/drivers/net/wireless/ath/ath6kl/debug.c
> +++ b/drivers/net/wireless/ath/ath6kl/debug.c
> @@ -54,6 +54,28 @@ int ath6kl_printk(const char *level, const char *fmt, ...)
> }
>
> #ifdef CONFIG_ATH6KL_DEBUG
> +
> +#define REG_OUTPUT_LEN_PER_LINE 25
> +#define ATH6KL_REG_TYPE_MAX 8
> +#define REGTYPE_STR_LEN 100
please indent values 8 and 100 similarly like you did with 25.
Also ATH6KL_REG_TYPE_MAX could be replaced with ARRAY_SIZE(), right?
> +
> +struct ath6kl_diag_reg_info {
> + u32 reg_start;
> + u32 reg_end;
> + char *reg_info;
const char *?
> +};
> +
> +static const struct ath6kl_diag_reg_info diag_reg[ATH6KL_REG_TYPE_MAX] = {
> + { 0x20000, 0x200fc, "General DMA and Rx registers" },
> + { 0x28000, 0x28900, "MAC PCU register & keycache" },
> + { 0x20800, 0x20a40, "QCU" },
> + { 0x21000, 0x212f0, "DCU" },
> + { 0x4000, 0x42e4, "RTC" },
> + { 0x540000, 0x540000 + (256 * 1024), "RAM" },
> + { 0x29800, 0x2B210, "Base Band" },
> + { 0x1C000, 0x1C748, "Analog" },
> +};
> +
> void ath6kl_dump_registers(struct ath6kl_device *dev,
> struct ath6kl_irq_proc_registers *irq_proc_reg,
> struct ath6kl_irq_enable_reg *irq_enable_reg)
> @@ -533,6 +555,171 @@ static const struct file_operations fops_credit_dist_stats = {
> .llseek = default_llseek,
> };
>
> +static unsigned long ath6kl_get_num_reg(void)
> +{
> + int i;
> + unsigned long n_reg = 0;
> +
> + for (i = 0; i < ATH6KL_REG_TYPE_MAX; i++)
> + n_reg = n_reg +
> + (diag_reg[i].reg_end - diag_reg[i].reg_start) / 4 + 1;
> +
> + return n_reg;
> +}
> +
> +static bool ath6kl_dbg_is_diag_reg_valid(u32 reg_addr)
> +{
> + int i;
> +
> + for (i = 0; i < ATH6KL_REG_TYPE_MAX; i++) {
> + if (reg_addr >= diag_reg[i].reg_start &&
> + reg_addr <= diag_reg[i].reg_end)
> + return true;
> + }
> +
> + ath6kl_info("Invalid addr\n");
ath6kl_warn() and a better warning message, please.
> + if (n_reg == 1) {
> + addr = ar->dbgfs_diag_reg;
> +
> + status = ath6kl_diag_read32(ar,
> + cpu_to_le32(TARG_VTOP(ar->target_type, addr)),
> + ®_val);
> + if (status)
> + goto fail_reg_read;
> +
> + len += scnprintf(buf + len, reg_len - len,
> + "0x%06x 0x%08x\n", addr, le32_to_cpu(reg_val));
> + } else {
You could add 'goto out;' at the end of the if block and then you don't
wouldn't need to use else block at all. That would make indentation more
readable.
> +static ssize_t read_file_reg_dump(struct file *file, char __user *user_buf,
> + size_t count, loff_t *ppos)
> +{
> + u8 *buf = file->private_data;
> + return simple_read_from_buffer(user_buf, count, ppos, buf, strlen(buf));
Does simple_read_from_buffer() handle the case when buf doesn't fit to
user_buf in one go? I guess it does but too busy to check that right now.
> +static const struct file_operations fops_reg_dump = {
> + .open = open_file_reg_dump,
> + .read = read_file_reg_dump,
> + .release = release_file_reg_dump,
The naming could follow more the style I'm trying to push, for example
something like this:
ath6kl_reg_dump_open()
ath6kl_reg_dump_read()
ath6kl_reg_dump_release()
> + .owner = THIS_MODULE,
> + .llseek = default_llseek,
> +};
> +
> int ath6kl_debug_init(struct ath6kl *ar)
> {
> ar->debug.fwlog_buf.buf = vmalloc(ATH6KL_FWLOG_SIZE);
> @@ -566,6 +753,10 @@ int ath6kl_debug_init(struct ath6kl *ar)
>
> debugfs_create_file("credit_dist_stats", S_IRUSR, ar->debugfs_phy, ar,
> &fops_credit_dist_stats);
> + debugfs_create_file("reg_addr", S_IRUSR, ar->debugfs_phy, ar,
> + &fops_diag_reg_read);
You need write mode for reg_addr. I had missed the same in my fwlog
patches :)
Kalle
On Wed, Aug 31, 2011 at 11:24:05AM +0300, Kalle Valo wrote:
> On 08/30/2011 06:16 PM, Vasanthakumar Thiagarajan wrote:
> > To dump a particular register, echo reg_addr > <debugfs_root>/ieee80211/phyX/ath6kl/reg_addr,
> > to dump the entire register set, echo 0 > <debugfs_root>/ieee80211/phyX/ath6kl/reg_addr, and
> > register values will be available at <debugfs_root>/ieee80211/phyX/ath6kl/reg_dump.
>
> I like this :) The full dump just takes quite long, 22 seconds on my x86
> box, and cat dumps everything only in the end. It would be nice to
> improve that somehow by sending results in smaller pieces, but no need
> to worry about that now.
Yeah, it takes significant time (but not as much as 22:)), anyway,
may be we need to give an option to dump specific range of register
like BB,MAC,DCU...This can be a follow up patch.
>
> First, I saw few sparse warnings:
>
> drivers/net/wireless/ath/ath6kl/debug.c:666:33: warning: incorrect type
> in argument 2 (different base types)
> drivers/net/wireless/ath/ath6kl/debug.c:666:33: expected unsigned int
> [unsigned] [usertype] address
> drivers/net/wireless/ath/ath6kl/debug.c:666:33: got restricted __le32
> [usertype] <noident>
> drivers/net/wireless/ath/ath6kl/debug.c:667:34: warning: incorrect type
> in argument 3 (different base types)
> drivers/net/wireless/ath/ath6kl/debug.c:667:34: expected unsigned int
> [usertype] *value
> drivers/net/wireless/ath/ath6kl/debug.c:667:34: got restricted __le32
> *<noident>
> drivers/net/wireless/ath/ath6kl/debug.c:680:41: warning: incorrect type
> in argument 2 (different base types)
> drivers/net/wireless/ath/ath6kl/debug.c:680:41: expected unsigned int
> [unsigned] [usertype] address
> drivers/net/wireless/ath/ath6kl/debug.c:680:41: got restricted __le32
> [usertype] <noident>
> drivers/net/wireless/ath/ath6kl/debug.c:681:50: warning: incorrect type
> in argument 3 (different base types)
> drivers/net/wireless/ath/ath6kl/debug.c:681:50: expected unsigned int
> [usertype] *value
> drivers/net/wireless/ath/ath6kl/debug.c:681:50: got restricted __le32
> *<noident>
Yeah, i saw sparse warnings lately. My endian patches fix this
warning.
>
> > --- a/drivers/net/wireless/ath/ath6kl/core.h
> > +++ b/drivers/net/wireless/ath/ath6kl/core.h
> > @@ -480,6 +480,7 @@ struct ath6kl {
> > } debug;
> > #endif /* CONFIG_ATH6KL_DEBUG */
> >
> > + unsigned int dbgfs_diag_reg;
> > };
>
> Please move this inside the debug struct above. I'm trying to categorise
> struct ath6kl a bit to make it easier to understand.
Sure, before your patch, I did not think having #ifdef just for a
variable is worth. It makes sense now.
>
> > static inline void *ath6kl_priv(struct net_device *dev)
> > diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
> > index 9608692..6ecfc70 100644
> > --- a/drivers/net/wireless/ath/ath6kl/debug.c
> > +++ b/drivers/net/wireless/ath/ath6kl/debug.c
> > @@ -54,6 +54,28 @@ int ath6kl_printk(const char *level, const char *fmt, ...)
> > }
> >
> > #ifdef CONFIG_ATH6KL_DEBUG
> > +
> > +#define REG_OUTPUT_LEN_PER_LINE 25
> > +#define ATH6KL_REG_TYPE_MAX 8
> > +#define REGTYPE_STR_LEN 100
>
> please indent values 8 and 100 similarly like you did with 25.
>
> Also ATH6KL_REG_TYPE_MAX could be replaced with ARRAY_SIZE(), right?
right, thanks.
>
> > +
> > +struct ath6kl_diag_reg_info {
> > + u32 reg_start;
> > + u32 reg_end;
> > + char *reg_info;
>
> const char *?
Sure.
> > +
> > +static bool ath6kl_dbg_is_diag_reg_valid(u32 reg_addr)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ATH6KL_REG_TYPE_MAX; i++) {
> > + if (reg_addr >= diag_reg[i].reg_start &&
> > + reg_addr <= diag_reg[i].reg_end)
> > + return true;
> > + }
> > +
> > + ath6kl_info("Invalid addr\n");
>
> ath6kl_warn() and a better warning message, please.
Makes sense, thanks.
>
> > + if (n_reg == 1) {
> > + addr = ar->dbgfs_diag_reg;
> > +
> > + status = ath6kl_diag_read32(ar,
> > + cpu_to_le32(TARG_VTOP(ar->target_type, addr)),
> > + ®_val);
> > + if (status)
> > + goto fail_reg_read;
> > +
> > + len += scnprintf(buf + len, reg_len - len,
> > + "0x%06x 0x%08x\n", addr, le32_to_cpu(reg_val));
> > + } else {
>
> You could add 'goto out;' at the end of the if block and then you don't
> wouldn't need to use else block at all. That would make indentation more
> readable.
Ok.
>
> > +static ssize_t read_file_reg_dump(struct file *file, char __user *user_buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + u8 *buf = file->private_data;
> > + return simple_read_from_buffer(user_buf, count, ppos, buf, strlen(buf));
>
> Does simple_read_from_buffer() handle the case when buf doesn't fit to
> user_buf in one go? I guess it does but too busy to check that right now.
I think it is taken care by simple_read_from_buffer. After all it is
going to read only count number of bytes for which user_buf memory should
be available.
>
> > +static const struct file_operations fops_reg_dump = {
> > + .open = open_file_reg_dump,
> > + .read = read_file_reg_dump,
> > + .release = release_file_reg_dump,
>
> The naming could follow more the style I'm trying to push, for example
> something like this:
>
> ath6kl_reg_dump_open()
> ath6kl_reg_dump_read()
> ath6kl_reg_dump_release()
Ok, thanks.
>
> > + .owner = THIS_MODULE,
> > + .llseek = default_llseek,
> > +};
> > +
> > int ath6kl_debug_init(struct ath6kl *ar)
> > {
> > ar->debug.fwlog_buf.buf = vmalloc(ATH6KL_FWLOG_SIZE);
> > @@ -566,6 +753,10 @@ int ath6kl_debug_init(struct ath6kl *ar)
> >
> > debugfs_create_file("credit_dist_stats", S_IRUSR, ar->debugfs_phy, ar,
> > &fops_credit_dist_stats);
> > + debugfs_create_file("reg_addr", S_IRUSR, ar->debugfs_phy, ar,
> > + &fops_diag_reg_read);
>
> You need write mode for reg_addr. I had missed the same in my fwlog
> patches :)
Right, i'm aware write mode support as well, working on it. Thanks
Vasanth