Add verbose debug for register accesses. This enables easier debugging
by following where and how hardware is stimulated, and how it answers.
Signed-off-by: Robert Jarzmik <[email protected]>
---
drivers/mtd/nand/pxa3xx_nand.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 1259cc558ce9..ed44bddcc43f 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -127,11 +127,23 @@
#define EXT_CMD_TYPE_MONO 0 /* Monolithic read/write */
/* macros for registers read/write */
-#define nand_writel(info, off, val) \
- writel_relaxed((val), (info)->mmio_base + (off))
-
-#define nand_readl(info, off) \
- readl_relaxed((info)->mmio_base + (off))
+#define nand_writel(info, off, val) \
+ do { \
+ dev_vdbg(&info->pdev->dev, \
+ "%s():%d nand_writel(0x%x, %s)\n", \
+ __func__, __LINE__, (val), #off); \
+ writel_relaxed((val), (info)->mmio_base + (off)); \
+ } while (0)
+
+#define nand_readl(info, off) \
+ ({ \
+ unsigned int _v; \
+ _v = readl_relaxed((info)->mmio_base + (off)); \
+ dev_vdbg(&info->pdev->dev, \
+ "%s():%d nand_readl(%s): 0x%x\n", \
+ __func__, __LINE__, #off, _v); \
+ _v; \
+ })
/* error code and state */
enum {
--
2.1.4
When the nand is first probe, and upon the first command start, the
status bits should be cleared before the interrupts are unmasked.
The bug is tricky : if the bootloader left a status bit set, the
unmasking of interrupts does trigger the interrupt handler before the
first command is issued, blocking the good behavior of the nand.
The same would happen if in pxa3xx_nand code flow a status bit is left,
and then a command is started.
Signed-off-by: Robert Jarzmik <[email protected]>
---
drivers/mtd/nand/pxa3xx_nand.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index ed44bddcc43f..edfe329cc9db 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -451,8 +451,8 @@ static void pxa3xx_nand_start(struct pxa3xx_nand_info *info)
ndcr |= NDCR_ND_RUN;
/* clear status bits and run */
- nand_writel(info, NDCR, 0);
nand_writel(info, NDSR, NDSR_MASK);
+ nand_writel(info, NDCR, 0);
nand_writel(info, NDCR, ndcr);
}
--
2.1.4
The cases of READID detection are broken on pxa3xx. The reason is that
in the early stages of nand probing, ie. at pxa3xx_nand_scan(), we
always have :
- info->use_dma = 0 (regardless of dma support yet)
- info->chunk_size = 0 (not yet detected)
The READID issued by pxa3xx_nand_scan() will therefore end up in
handle_data_pio(), and do_bytes will be 0, leading to not reading the
nand id, and blocking detection.
This doesn't happen if "keep_config" is used, which is probably the most
tested case.
Signed-off-by: Robert Jarzmik <[email protected]>
Fixes: 70ed85232a93 ("mtd: nand: pxa3xx: Introduce multiple page I/O
support")
---
drivers/mtd/nand/pxa3xx_nand.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index edfe329cc9db..b0737aec7caf 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -1482,6 +1482,7 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
int i, ret, num;
uint16_t ecc_strength, ecc_step;
+ info->chunk_size = 512;
if (pdata->keep_config && !pxa3xx_nand_detect_config(info))
goto KEEP_CONFIG;
--
2.1.4
On 11 Aug 09:57 PM, Robert Jarzmik wrote:
> The cases of READID detection are broken on pxa3xx. The reason is that
> in the early stages of nand probing, ie. at pxa3xx_nand_scan(), we
> always have :
> - info->use_dma = 0 (regardless of dma support yet)
> - info->chunk_size = 0 (not yet detected)
>
> The READID issued by pxa3xx_nand_scan() will therefore end up in
> handle_data_pio(), and do_bytes will be 0, leading to not reading the
> nand id, and blocking detection.
>
> This doesn't happen if "keep_config" is used, which is probably the most
> tested case.
>
> Signed-off-by: Robert Jarzmik <[email protected]>
> Fixes: 70ed85232a93 ("mtd: nand: pxa3xx: Introduce multiple page I/O
> support")
To be fair, Antoine submitted this a while ago:
http://lists.infradead.org/pipermail/linux-mtd/2015-April/058739.html
Not sure which one takes precedence in such a case (and yours
has a proper Fixes tag).
> ---
> drivers/mtd/nand/pxa3xx_nand.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index edfe329cc9db..b0737aec7caf 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -1482,6 +1482,7 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
> int i, ret, num;
> uint16_t ecc_strength, ecc_step;
>
> + info->chunk_size = 512;
> if (pdata->keep_config && !pxa3xx_nand_detect_config(info))
> goto KEEP_CONFIG;
>
> --
> 2.1.4
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
Ezequiel Garcia, VanguardiaSur
http://www.vanguardiasur.com.ar
Ezequiel Garcia <[email protected]> writes:
> On 11 Aug 09:57 PM, Robert Jarzmik wrote:
>> The cases of READID detection are broken on pxa3xx. The reason is that
>> in the early stages of nand probing, ie. at pxa3xx_nand_scan(), we
>> always have :
>> - info->use_dma = 0 (regardless of dma support yet)
>> - info->chunk_size = 0 (not yet detected)
>>
>> The READID issued by pxa3xx_nand_scan() will therefore end up in
>> handle_data_pio(), and do_bytes will be 0, leading to not reading the
>> nand id, and blocking detection.
>>
>> This doesn't happen if "keep_config" is used, which is probably the most
>> tested case.
>>
>> Signed-off-by: Robert Jarzmik <[email protected]>
>> Fixes: 70ed85232a93 ("mtd: nand: pxa3xx: Introduce multiple page I/O
>> support")
>
> To be fair, Antoine submitted this a while ago:
>
> http://lists.infradead.org/pipermail/linux-mtd/2015-April/058739.html
>
> Not sure which one takes precedence in such a case (and yours
> has a proper Fixes tag).
His has precedence. How is it that a fix patch is not yet merge since April ?
Is it because it's part of a still in review serie ?
Cheers.
--
Robert
On 16 August 2015 at 19:22, Robert Jarzmik <[email protected]> wrote:
> Ezequiel Garcia <[email protected]> writes:
>
>> On 11 Aug 09:57 PM, Robert Jarzmik wrote:
>>> The cases of READID detection are broken on pxa3xx. The reason is that
>>> in the early stages of nand probing, ie. at pxa3xx_nand_scan(), we
>>> always have :
>>> - info->use_dma = 0 (regardless of dma support yet)
>>> - info->chunk_size = 0 (not yet detected)
>>>
>>> The READID issued by pxa3xx_nand_scan() will therefore end up in
>>> handle_data_pio(), and do_bytes will be 0, leading to not reading the
>>> nand id, and blocking detection.
>>>
>>> This doesn't happen if "keep_config" is used, which is probably the most
>>> tested case.
>>>
>>> Signed-off-by: Robert Jarzmik <[email protected]>
>>> Fixes: 70ed85232a93 ("mtd: nand: pxa3xx: Introduce multiple page I/O
>>> support")
>>
>> To be fair, Antoine submitted this a while ago:
>>
>> http://lists.infradead.org/pipermail/linux-mtd/2015-April/058739.html
>>
>> Not sure which one takes precedence in such a case (and yours
>> has a proper Fixes tag).
> His has precedence. How is it that a fix patch is not yet merge since April ?
> Is it because it's part of a still in review serie ?
>
Exactly. And because I thought it was needed for Berlin support,
so it wasn't a regression.
--
Ezequiel García, VanguardiaSur
http://www.vanguardiasur.com.ar
Ezequiel Garcia <[email protected]> writes:
> On 16 August 2015 at 19:22, Robert Jarzmik <[email protected]> wrote:
>> Ezequiel Garcia <[email protected]> writes:
>>
>>> On 11 Aug 09:57 PM, Robert Jarzmik wrote:
>>>> The cases of READID detection are broken on pxa3xx. The reason is that
>>>> in the early stages of nand probing, ie. at pxa3xx_nand_scan(), we
>>>> always have :
>>>> - info->use_dma = 0 (regardless of dma support yet)
>>>> - info->chunk_size = 0 (not yet detected)
>>>>
>>>> The READID issued by pxa3xx_nand_scan() will therefore end up in
>>>> handle_data_pio(), and do_bytes will be 0, leading to not reading the
>>>> nand id, and blocking detection.
>>>>
>>>> This doesn't happen if "keep_config" is used, which is probably the most
>>>> tested case.
>>>>
>>>> Signed-off-by: Robert Jarzmik <[email protected]>
>>>> Fixes: 70ed85232a93 ("mtd: nand: pxa3xx: Introduce multiple page I/O
>>>> support")
>>>
>>> To be fair, Antoine submitted this a while ago:
>>>
>>> http://lists.infradead.org/pipermail/linux-mtd/2015-April/058739.html
>>>
>>> Not sure which one takes precedence in such a case (and yours
>>> has a proper Fixes tag).
>> His has precedence. How is it that a fix patch is not yet merge since April ?
>> Is it because it's part of a still in review serie ?
Antoine, could you resubmit this single patch with this as trailer please :
Acked-by: Robert Jarzmik <robert.jarzmik@free>
Fixes: 70ed85232a93 ("mtd: nand: pxa3xx: Introduce multiple page I/O
support")
This should get in Brian's fixes tree as soon as possible.
Thanks.
--
Robert
On 11 Aug 09:57 PM, Robert Jarzmik wrote:
> Add verbose debug for register accesses. This enables easier debugging
> by following where and how hardware is stimulated, and how it answers.
>
I really don't see why we want this patch. It's probably an useful hack to
use in some cases, but can't see why we would want it in mainline.
Feel free to prove me wrong.
> Signed-off-by: Robert Jarzmik <[email protected]>
> ---
> drivers/mtd/nand/pxa3xx_nand.c | 22 +++++++++++++++++-----
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index 1259cc558ce9..ed44bddcc43f 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -127,11 +127,23 @@
> #define EXT_CMD_TYPE_MONO 0 /* Monolithic read/write */
>
> /* macros for registers read/write */
> -#define nand_writel(info, off, val) \
> - writel_relaxed((val), (info)->mmio_base + (off))
> -
> -#define nand_readl(info, off) \
> - readl_relaxed((info)->mmio_base + (off))
> +#define nand_writel(info, off, val) \
> + do { \
> + dev_vdbg(&info->pdev->dev, \
> + "%s():%d nand_writel(0x%x, %s)\n", \
> + __func__, __LINE__, (val), #off); \
> + writel_relaxed((val), (info)->mmio_base + (off)); \
> + } while (0)
> +
> +#define nand_readl(info, off) \
> + ({ \
> + unsigned int _v; \
> + _v = readl_relaxed((info)->mmio_base + (off)); \
> + dev_vdbg(&info->pdev->dev, \
> + "%s():%d nand_readl(%s): 0x%x\n", \
> + __func__, __LINE__, #off, _v); \
> + _v; \
> + })
>
> /* error code and state */
> enum {
> --
> 2.1.4
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
Ezequiel Garcia, VanguardiaSur
http://www.vanguardiasur.com.ar
Hi,
On Mon, Aug 17, 2015 at 09:03:38PM +0200, Robert Jarzmik wrote:
> Ezequiel Garcia <[email protected]> writes:
>
> > On 16 August 2015 at 19:22, Robert Jarzmik <[email protected]> wrote:
> >> Ezequiel Garcia <[email protected]> writes:
> >>
> >>> On 11 Aug 09:57 PM, Robert Jarzmik wrote:
> >>>> The cases of READID detection are broken on pxa3xx. The reason is that
> >>>> in the early stages of nand probing, ie. at pxa3xx_nand_scan(), we
> >>>> always have :
> >>>> - info->use_dma = 0 (regardless of dma support yet)
> >>>> - info->chunk_size = 0 (not yet detected)
> >>>>
> >>>> The READID issued by pxa3xx_nand_scan() will therefore end up in
> >>>> handle_data_pio(), and do_bytes will be 0, leading to not reading the
> >>>> nand id, and blocking detection.
> >>>>
> >>>> This doesn't happen if "keep_config" is used, which is probably the most
> >>>> tested case.
> >>>>
> >>>> Signed-off-by: Robert Jarzmik <[email protected]>
> >>>> Fixes: 70ed85232a93 ("mtd: nand: pxa3xx: Introduce multiple page I/O
> >>>> support")
> >>>
> >>> To be fair, Antoine submitted this a while ago:
> >>>
> >>> http://lists.infradead.org/pipermail/linux-mtd/2015-April/058739.html
> >>>
> >>> Not sure which one takes precedence in such a case (and yours
> >>> has a proper Fixes tag).
> >> His has precedence. How is it that a fix patch is not yet merge since April ?
> >> Is it because it's part of a still in review serie ?
>
> Antoine, could you resubmit this single patch with this as trailer please :
> Acked-by: Robert Jarzmik <robert.jarzmik@free>
> Fixes: 70ed85232a93 ("mtd: nand: pxa3xx: Introduce multiple page I/O
> support")
I just resend it.
Antoine
--
Antoine T?nart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Ezequiel Garcia <[email protected]> writes:
> On 11 Aug 09:57 PM, Robert Jarzmik wrote:
>> Add verbose debug for register accesses. This enables easier debugging
>> by following where and how hardware is stimulated, and how it answers.
>>
>
> I really don't see why we want this patch. It's probably an useful hack to
> use in some cases, but can't see why we would want it in mainline.
>
> Feel free to prove me wrong.
Why not.
Imagine that there is a bug in the driver, such as the one with the status bits
clearing. You can't reproduce it, and the person who has the hardware to
reproduce it is not skilled enough to do any debug.
What are the tools and what will you ask this tester to do in order to debug and
solve his problem, as a good driver maintainer ?
Amongst the drivers I wrote in the past, this approach was the most reliable to
capture traces to debug remotely.
Cheers.
--
Robert
Robert Jarzmik <[email protected]> writes:
> Ezequiel Garcia <[email protected]> writes:
>
>> On 11 Aug 09:57 PM, Robert Jarzmik wrote:
>>> Add verbose debug for register accesses. This enables easier debugging
>>> by following where and how hardware is stimulated, and how it answers.
>>>
>>
>> I really don't see why we want this patch. It's probably an useful hack to
>> use in some cases, but can't see why we would want it in mainline.
>>
>> Feel free to prove me wrong.
> Why not.
>
> Imagine that there is a bug in the driver, such as the one with the status bits
> clearing. You can't reproduce it, and the person who has the hardware to
> reproduce it is not skilled enough to do any debug.
>
> What are the tools and what will you ask this tester to do in order to debug and
> solve his problem, as a good driver maintainer ?
>
> Amongst the drivers I wrote in the past, this approach was the most reliable to
> capture traces to debug remotely.
Ezequiel, do you agree with my point ?
Cheers.
--
Robert
On 23 August 2015 at 16:09, Robert Jarzmik <[email protected]> wrote:
> Robert Jarzmik <[email protected]> writes:
>
>> Ezequiel Garcia <[email protected]> writes:
>>
>>> On 11 Aug 09:57 PM, Robert Jarzmik wrote:
>>>> Add verbose debug for register accesses. This enables easier debugging
>>>> by following where and how hardware is stimulated, and how it answers.
>>>>
>>>
>>> I really don't see why we want this patch. It's probably an useful hack to
>>> use in some cases, but can't see why we would want it in mainline.
>>>
>>> Feel free to prove me wrong.
>> Why not.
>>
>> Imagine that there is a bug in the driver, such as the one with the status bits
>> clearing. You can't reproduce it, and the person who has the hardware to
>> reproduce it is not skilled enough to do any debug.
>>
So let's assume this person is not skilled enough to debug it, but it is skilled
enough to rebuild a kernel (to activate the verbose debug).
Then this person can also apply a simple patch (this patch), which seems
a pretty trivial step to do.
Since you might also need some other debug traces, I'm not convinced about
pushing this particular group to mainline.
>> What are the tools and what will you ask this tester to do in order to debug and
>> solve his problem, as a good driver maintainer ?
>>
>> Amongst the drivers I wrote in the past, this approach was the most reliable to
>> capture traces to debug remotely.
>
> Ezequiel, do you agree with my point ?
>
I agree that the hack is useful for debugging purposes, it's just that I don't
see why we want it in mainline - where we usually avoid clutter.
Also, your argument applies to all the other drivers, but that doesn't mean
we will patch them all.
Anyway, this is just my opinion. If Brian thinks differently, I have no problem
with it.
Thanks!
--
Ezequiel García, VanguardiaSur
http://www.vanguardiasur.com.ar
Garbage collecting some old patches before vacation...
On Mon, Aug 24, 2015 at 10:46:18AM -0300, Ezequiel Garcia wrote:
> I agree that the hack is useful for debugging purposes, it's just that I don't
> see why we want it in mainline - where we usually avoid clutter.
I'm not sure this is really that important of clutter. It's in a nicely
hidden abstraction (the local nand_{read,write}l() helpers), and it
doesn't add any compile time cost for most users.
> Also, your argument applies to all the other drivers, but that doesn't mean
> we will patch them all.
>
> Anyway, this is just my opinion. If Brian thinks differently, I have no problem
> with it.
I don't have very strong opinions on this. It's kind of annoying to have
this sort of stuff duplicated for every driver, if it's really needed.
But I'll admit this kind of infrastructure is sometimes useful.
Anecdote: I recently found the regmap trace event infrastructure pretty
nice for debugging some other drivers. This would only require you to
have tracing enabled, and then no recompiles are necessary at all. Just
cmdline changes.
So, I could go with this patch, if Robert still desires it. Or you could
convert to using regmap for MMIO :)
Brian
Brian Norris <[email protected]> writes:
> I don't have very strong opinions on this. It's kind of annoying to have
> this sort of stuff duplicated for every driver, if it's really needed.
> But I'll admit this kind of infrastructure is sometimes useful.
>
> Anecdote: I recently found the regmap trace event infrastructure pretty
> nice for debugging some other drivers. This would only require you to
> have tracing enabled, and then no recompiles are necessary at all. Just
> cmdline changes.
>
> So, I could go with this patch, if Robert still desires it. Or you could
> convert to using regmap for MMIO :)
I'm as you, I don't feel strong opinion about it, I'd like to have a debug
tracing tool, be that this patch or regmap MMIO.
If we all agree on a path I could even make the final patch, whichever solution
is chosen.
Cheers.
--
Robert