[PATCH] sdhci: sdhci_dumpregs: allow private information to be dumped on error

[PATCH] sdhci: sdhci_dumpregs: allow private information to be dumped on error

[Philip Rakity]

Our controller has private registers that are useful to see on error.
define a new host->ops to allow these registers to be printed

Signed-off-by: Philip Rakity <prakity@marvell.com>
---
 drivers/mmc/host/sdhci.c |    2 ++
 drivers/mmc/host/sdhci.h |    1 +
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 782c0ee..fba6e28 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -85,6 +85,8 @@ static void sdhci_dumpregs(struct sdhci_host *host)
 		printk(KERN_DEBUG DRIVER_NAME ": ADMA Err: 0x%08x | ADMA Ptr: 0x%08x\n",
 		       readl(host->ioaddr + SDHCI_ADMA_ERROR),
 		       readl(host->ioaddr + SDHCI_ADMA_ADDRESS));
+	if (host->ops->print_private_info)
+		host->ops->print_private_info(host);
 
 	printk(KERN_DEBUG DRIVER_NAME ": ===========================================\n");
 }
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index b7b8a3b..2e11ff9 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -215,6 +215,7 @@ struct sdhci_ops {
 	void (*platform_send_init_74_clocks)(struct sdhci_host *host,
 					     u8 power_mode);
 	unsigned int    (*get_ro)(struct sdhci_host *host);
+	void		(*print_private_info)(struct sdhci_host *host);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
-- 
1.6.0.4

Re: [PATCH] sdhci: sdhci_dumpregs: allow private information to be dumped on error

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

Hi Philip,

On Tue, Oct 19, 2010 at 11:27:22AM -0700, Philip Rakity wrote:
> Our controller has private registers that are useful to see on error.
> define a new host->ops to allow these registers to be printed
> 
> Signed-off-by: Philip Rakity <prakity@marvell.com>

I wonder if we really need such a thing in the mainline kernel? It is probably
used for debugging the driver?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] sdhci: sdhci_dumpregs: allow private information to be dumped on error

[Chris Ball]

Hi,

On Tue, Oct 19, 2010 at 09:47:24PM +0200, Wolfram Sang wrote:
> On Tue, Oct 19, 2010 at 11:27:22AM -0700, Philip Rakity wrote:
> > Our controller has private registers that are useful to see on error.
> > define a new host->ops to allow these registers to be printed
> > 
> > Signed-off-by: Philip Rakity <prakity@marvell.com>
> 
> I wonder if we really need such a thing in the mainline kernel? It is probably
> used for debugging the driver?

There's another question:  why merge a patch that adds a hook that isn't
used by anything currently in the kernel?  Should we wait until this is
accompanied by a print_private_info() implementation in a mainline
driver, rather than adding it proactively?

Thanks,

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] sdhci: sdhci_dumpregs: allow private information to be dumped on error

[Philip Rakity]

sdhci_dumpregs is used to debug the hardware/driver so I do not see an issue here.  The register info is not useful to a normal linux user.   We can fine tune the timing on the controller so knowing the settings is useful if there are CRC errors.

The reason to add this -- rather than wait -- is that I did not imagine we were the only folks who had private information that might be useful to display.  

Philip

On Oct 19, 2010, at 1:04 PM, Chris Ball wrote:

> Hi,
> 
> On Tue, Oct 19, 2010 at 09:47:24PM +0200, Wolfram Sang wrote:
>> On Tue, Oct 19, 2010 at 11:27:22AM -0700, Philip Rakity wrote:
>>> Our controller has private registers that are useful to see on error.
>>> define a new host->ops to allow these registers to be printed
>>> 
>>> Signed-off-by: Philip Rakity <prakity@marvell.com>
>> 
>> I wonder if we really need such a thing in the mainline kernel? It is probably
>> used for debugging the driver?
> 
> There's another question:  why merge a patch that adds a hook that isn't
> used by anything currently in the kernel?  Should we wait until this is
> accompanied by a print_private_info() implementation in a mainline
> driver, rather than adding it proactively?
> 
> Thanks,
> 
> -- 
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child

Re: [PATCH] sdhci: sdhci_dumpregs: allow private information to be dumped on error

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 480 bytes --]

Hi Philip,

> sdhci_dumpregs is used to debug the hardware/driver so I do not see an issue
> here.  The register info is not useful to a normal linux user.   We can fine
> tune the timing on the controller so knowing the settings is useful if there
> are CRC errors.

I'd prefer to keep such changes local because they are not needed for a wider
audience. (And adding something to dump_regs is trivial enough.)

Nevertheless, thanks for the suggestion!

Best regards,

   Wolfram

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]