Re: [PATCH 10/11] spi: add more debug prints in s3c64xx
On Thu, Jun 04, 2015 at 12:52:19PM +0200, Michal Suchanek wrote: > There is no code interdependency with the other patches so if it's > preferred I can send patches like this separately. Yes, that's better practice. signature.asc Description: Digital signature
Re: [PATCH 10/11] spi: add more debug prints in s3c64xx
On 4 June 2015 at 12:26, Mark Brown wrote: > On Thu, Jun 04, 2015 at 11:33:37AM +0200, Michal Suchanek wrote: >> On 4 June 2015 at 11:16, Mark Brown wrote: > >> > Also for this patch (which just adds some trace) there isn't any clear >> > reason for it to be sent as part of the series at all, it doesn't help >> > deliver the functionality and doesn't depend on the rest of the series. > >> I used this patch to add missing information to dmesg output so I >> could diagnose the SPI failures so I think it is relevant. > > The fact that you used this to debug problems is not relevant to any fix > you might have developed for those problems; the fix can be explained > without needing to know how you got there. Similarly the debugging is > hopefully useful in general without needing to know which particular > problem prompted it's creation. Yes, this patch should be meaningful on its own. The ultimate purpose of this patch series is to make the SPI NOR flash chip usable on boards of which I have 1 sample. If the results on boards other than mine happen to be different the debug prints will be useful for users of this flash chip. There is no code interdependency with the other patches so if it's preferred I can send patches like this separately. > >> To print a clock name you AFAICT need this header. I think this is an >> abstraction problem in the clock framework. Fixing the abstraction >> problem with clock framework would only grow the list of recipients >> which is already so long you complain about it. > > This is a bit of a warning sign that the series isn't very focused. > It's also just not good practice, sending patches with obvious problems > (especially obvious problems that aren't clearly flagged up as something > temporary) will frustrate reviewers and can often lead to other issues > being obscured. As has been pointed out public interface already exists for getting the clock name so the issue here is cosmetic. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/11] spi: add more debug prints in s3c64xx
On Thu, Jun 04, 2015 at 11:33:37AM +0200, Michal Suchanek wrote: > On 4 June 2015 at 11:16, Mark Brown wrote: > > Also for this patch (which just adds some trace) there isn't any clear > > reason for it to be sent as part of the series at all, it doesn't help > > deliver the functionality and doesn't depend on the rest of the series. > I used this patch to add missing information to dmesg output so I > could diagnose the SPI failures so I think it is relevant. The fact that you used this to debug problems is not relevant to any fix you might have developed for those problems; the fix can be explained without needing to know how you got there. Similarly the debugging is hopefully useful in general without needing to know which particular problem prompted it's creation. > To print a clock name you AFAICT need this header. I think this is an > abstraction problem in the clock framework. Fixing the abstraction > problem with clock framework would only grow the list of recipients > which is already so long you complain about it. This is a bit of a warning sign that the series isn't very focused. It's also just not good practice, sending patches with obvious problems (especially obvious problems that aren't clearly flagged up as something temporary) will frustrate reviewers and can often lead to other issues being obscured. signature.asc Description: Digital signature
Re: [PATCH 10/11] spi: add more debug prints in s3c64xx
On Thu, Jun 04, 2015 at 11:30:15AM +0200, Geert Uytterhoeven wrote: > On Thu, Jun 4, 2015 at 11:16 AM, Mark Brown wrote: > >> --- a/drivers/spi/spi-s3c64xx.c > >> +++ b/drivers/spi/spi-s3c64xx.c > >> @@ -18,6 +18,7 @@ > >> #include > >> #include > >> #include > >> +#include > > Whatever you're doing here this indicates that there's a very big > > abstraction problem going on. > I guess it's needed for __clk_get_name(), which can be avoided by using > the "%pC" printf format specifier instead. Right, or by adding an externally usable API - the point is that using internal functions in a client driver is a big red flag. signature.asc Description: Digital signature
Re: [PATCH 10/11] spi: add more debug prints in s3c64xx
Hello,
On 4 June 2015 at 11:16, Mark Brown wrote:
> On Wed, Jun 03, 2015 at 09:26:42PM -, Michal Suchanek wrote:
>> The SPI NOR transfers mysteriously fail so add more debug prints about
>> SPI transactions.
>
> Please try to only send patches to relevant people - the list of
> recipients for this is so large that it only barely fits on a single
> screen in my mail client.
>
> Also for this patch (which just adds some trace) there isn't any clear
> reason for it to be sent as part of the series at all, it doesn't help
> deliver the functionality and doesn't depend on the rest of the series.
I used this patch to add missing information to dmesg output so I
could diagnose the SPI failures so I think it is relevant.
The failure is reported by the s3c64xx but the root cause is pl330 not
finishing a DMA transfer.
It is non-obvious that this is the issue. The information that a call
to pl330 failed is only available in the s3c64xx driver and not SPI
core so I think there is no reasonable way to debug this issue other
than adding prints in s3c64xx. This issue is not completely resolved
or even well tested (I have only single board to test with) so these
prints remain relevant in my view.
>
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -18,6 +18,7 @@
>> #include
>> #include
>> #include
>> +#include
>
> Whatever you're doing here this indicates that there's a very big
> abstraction problem going on.
I wanted to print a clock name in case the information was relevant
for the issue at hand.
To print a clock name you AFAICT need this header. I think this is an
abstraction problem in the clock framework. Fixing the abstraction
problem with clock framework would only grow the list of recipients
which is already so long you complain about it.
It turns out that in this case the clock was not at fault so I did not
need the clock name in the end.
>
>> + pr_debug("%s %s %s waiting for %ims transferring %zubytes@%iHz",
>> + __func__, sdd->pdev ? dev_name(>pdev->dev) : NULL,
>> + dev_name(>master->dev),
>> + ms, xfer->len, sdd->cur_speed);
>
> I'd say dev_dbg() but more generally this is just tracing things that
> seem to be already covered by the trace points already present in the
> core, the same goes for most of the rest of it. If there's things
> missing from the existing trace it seems better to add to it.
There is master and slave device involved but it's certainly possible
to use dev_dbg on one of them and pass the other as string.
Thanks
Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/11] spi: add more debug prints in s3c64xx
On Thu, Jun 4, 2015 at 11:16 AM, Mark Brown wrote:
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -18,6 +18,7 @@
>> #include
>> #include
>> #include
>> +#include
>
> Whatever you're doing here this indicates that there's a very big
> abstraction problem going on.
I guess it's needed for __clk_get_name(), which can be avoided by using
the "%pC" printf format specifier instead.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/11] spi: add more debug prints in s3c64xx
On Wed, Jun 03, 2015 at 09:26:42PM -, Michal Suchanek wrote:
> The SPI NOR transfers mysteriously fail so add more debug prints about
> SPI transactions.
Please try to only send patches to relevant people - the list of
recipients for this is so large that it only barely fits on a single
screen in my mail client.
Also for this patch (which just adds some trace) there isn't any clear
reason for it to be sent as part of the series at all, it doesn't help
deliver the functionality and doesn't depend on the rest of the series.
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -18,6 +18,7 @@
> #include
> #include
> #include
> +#include
Whatever you're doing here this indicates that there's a very big
abstraction problem going on.
> + pr_debug("%s %s %s waiting for %ims transferring %zubytes@%iHz",
> + __func__, sdd->pdev ? dev_name(>pdev->dev) : NULL,
> + dev_name(>master->dev),
> + ms, xfer->len, sdd->cur_speed);
I'd say dev_dbg() but more generally this is just tracing things that
seem to be already covered by the trace points already present in the
core, the same goes for most of the rest of it. If there's things
missing from the existing trace it seems better to add to it.
signature.asc
Description: Digital signature
Re: [PATCH 10/11] spi: add more debug prints in s3c64xx
On Wed, Jun 03, 2015 at 09:26:42PM -, Michal Suchanek wrote: The SPI NOR transfers mysteriously fail so add more debug prints about SPI transactions. Please try to only send patches to relevant people - the list of recipients for this is so large that it only barely fits on a single screen in my mail client. Also for this patch (which just adds some trace) there isn't any clear reason for it to be sent as part of the series at all, it doesn't help deliver the functionality and doesn't depend on the rest of the series. --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -18,6 +18,7 @@ #include linux/interrupt.h #include linux/delay.h #include linux/clk.h +#include linux/clk-provider.h Whatever you're doing here this indicates that there's a very big abstraction problem going on. + pr_debug(%s %s %s waiting for %ims transferring %zubytes@%iHz, + __func__, sdd-pdev ? dev_name(sdd-pdev-dev) : NULL, + dev_name(sdd-master-dev), + ms, xfer-len, sdd-cur_speed); I'd say dev_dbg() but more generally this is just tracing things that seem to be already covered by the trace points already present in the core, the same goes for most of the rest of it. If there's things missing from the existing trace it seems better to add to it. signature.asc Description: Digital signature
Re: [PATCH 10/11] spi: add more debug prints in s3c64xx
On Thu, Jun 4, 2015 at 11:16 AM, Mark Brown broo...@kernel.org wrote:
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -18,6 +18,7 @@
#include linux/interrupt.h
#include linux/delay.h
#include linux/clk.h
+#include linux/clk-provider.h
Whatever you're doing here this indicates that there's a very big
abstraction problem going on.
I guess it's needed for __clk_get_name(), which can be avoided by using
the %pC printf format specifier instead.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/11] spi: add more debug prints in s3c64xx
On Thu, Jun 04, 2015 at 12:52:19PM +0200, Michal Suchanek wrote: There is no code interdependency with the other patches so if it's preferred I can send patches like this separately. Yes, that's better practice. signature.asc Description: Digital signature
Re: [PATCH 10/11] spi: add more debug prints in s3c64xx
On Thu, Jun 04, 2015 at 11:33:37AM +0200, Michal Suchanek wrote: On 4 June 2015 at 11:16, Mark Brown broo...@kernel.org wrote: Also for this patch (which just adds some trace) there isn't any clear reason for it to be sent as part of the series at all, it doesn't help deliver the functionality and doesn't depend on the rest of the series. I used this patch to add missing information to dmesg output so I could diagnose the SPI failures so I think it is relevant. The fact that you used this to debug problems is not relevant to any fix you might have developed for those problems; the fix can be explained without needing to know how you got there. Similarly the debugging is hopefully useful in general without needing to know which particular problem prompted it's creation. To print a clock name you AFAICT need this header. I think this is an abstraction problem in the clock framework. Fixing the abstraction problem with clock framework would only grow the list of recipients which is already so long you complain about it. This is a bit of a warning sign that the series isn't very focused. It's also just not good practice, sending patches with obvious problems (especially obvious problems that aren't clearly flagged up as something temporary) will frustrate reviewers and can often lead to other issues being obscured. signature.asc Description: Digital signature
Re: [PATCH 10/11] spi: add more debug prints in s3c64xx
On 4 June 2015 at 12:26, Mark Brown broo...@kernel.org wrote: On Thu, Jun 04, 2015 at 11:33:37AM +0200, Michal Suchanek wrote: On 4 June 2015 at 11:16, Mark Brown broo...@kernel.org wrote: Also for this patch (which just adds some trace) there isn't any clear reason for it to be sent as part of the series at all, it doesn't help deliver the functionality and doesn't depend on the rest of the series. I used this patch to add missing information to dmesg output so I could diagnose the SPI failures so I think it is relevant. The fact that you used this to debug problems is not relevant to any fix you might have developed for those problems; the fix can be explained without needing to know how you got there. Similarly the debugging is hopefully useful in general without needing to know which particular problem prompted it's creation. Yes, this patch should be meaningful on its own. The ultimate purpose of this patch series is to make the SPI NOR flash chip usable on boards of which I have 1 sample. If the results on boards other than mine happen to be different the debug prints will be useful for users of this flash chip. There is no code interdependency with the other patches so if it's preferred I can send patches like this separately. To print a clock name you AFAICT need this header. I think this is an abstraction problem in the clock framework. Fixing the abstraction problem with clock framework would only grow the list of recipients which is already so long you complain about it. This is a bit of a warning sign that the series isn't very focused. It's also just not good practice, sending patches with obvious problems (especially obvious problems that aren't clearly flagged up as something temporary) will frustrate reviewers and can often lead to other issues being obscured. As has been pointed out public interface already exists for getting the clock name so the issue here is cosmetic. Thanks Michal -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/11] spi: add more debug prints in s3c64xx
Hello, On 4 June 2015 at 11:16, Mark Brown broo...@kernel.org wrote: On Wed, Jun 03, 2015 at 09:26:42PM -, Michal Suchanek wrote: The SPI NOR transfers mysteriously fail so add more debug prints about SPI transactions. Please try to only send patches to relevant people - the list of recipients for this is so large that it only barely fits on a single screen in my mail client. Also for this patch (which just adds some trace) there isn't any clear reason for it to be sent as part of the series at all, it doesn't help deliver the functionality and doesn't depend on the rest of the series. I used this patch to add missing information to dmesg output so I could diagnose the SPI failures so I think it is relevant. The failure is reported by the s3c64xx but the root cause is pl330 not finishing a DMA transfer. It is non-obvious that this is the issue. The information that a call to pl330 failed is only available in the s3c64xx driver and not SPI core so I think there is no reasonable way to debug this issue other than adding prints in s3c64xx. This issue is not completely resolved or even well tested (I have only single board to test with) so these prints remain relevant in my view. --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -18,6 +18,7 @@ #include linux/interrupt.h #include linux/delay.h #include linux/clk.h +#include linux/clk-provider.h Whatever you're doing here this indicates that there's a very big abstraction problem going on. I wanted to print a clock name in case the information was relevant for the issue at hand. To print a clock name you AFAICT need this header. I think this is an abstraction problem in the clock framework. Fixing the abstraction problem with clock framework would only grow the list of recipients which is already so long you complain about it. It turns out that in this case the clock was not at fault so I did not need the clock name in the end. + pr_debug(%s %s %s waiting for %ims transferring %zubytes@%iHz, + __func__, sdd-pdev ? dev_name(sdd-pdev-dev) : NULL, + dev_name(sdd-master-dev), + ms, xfer-len, sdd-cur_speed); I'd say dev_dbg() but more generally this is just tracing things that seem to be already covered by the trace points already present in the core, the same goes for most of the rest of it. If there's things missing from the existing trace it seems better to add to it. There is master and slave device involved but it's certainly possible to use dev_dbg on one of them and pass the other as string. Thanks Michal -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/11] spi: add more debug prints in s3c64xx
On Thu, Jun 04, 2015 at 11:30:15AM +0200, Geert Uytterhoeven wrote: On Thu, Jun 4, 2015 at 11:16 AM, Mark Brown broo...@kernel.org wrote: --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -18,6 +18,7 @@ #include linux/interrupt.h #include linux/delay.h #include linux/clk.h +#include linux/clk-provider.h Whatever you're doing here this indicates that there's a very big abstraction problem going on. I guess it's needed for __clk_get_name(), which can be avoided by using the %pC printf format specifier instead. Right, or by adding an externally usable API - the point is that using internal functions in a client driver is a big red flag. signature.asc Description: Digital signature
Re: [PATCH 10/11] spi: add more debug prints in s3c64xx
On Wednesday, June 03, 2015 at 11:26:42 PM, Michal Suchanek wrote: > The SPI NOR transfers mysteriously fail so add more debug prints about > SPI transactions. > > Signed-off-by: Michal Suchanek dev_dbg() and friends would certainly be nicer here. Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/11] spi: add more debug prints in s3c64xx
On Wednesday, June 03, 2015 at 11:26:42 PM, Michal Suchanek wrote: The SPI NOR transfers mysteriously fail so add more debug prints about SPI transactions. Signed-off-by: Michal Suchanek hramr...@gmail.com dev_dbg() and friends would certainly be nicer here. Best regards, Marek Vasut -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
