Re: [PATCH 02/11] soc/fsl: Introduce DPAA BMan device management driver
On Fri, 2015-07-10 at 15:57 -0500, Pledge Roy-R01356 wrote:
> >
> > On Fri, 2015-07-10 at 13:36 +0200, Paul Bolle wrote:
> > > On do, 2015-07-09 at 16:21 -0400, Roy Pledge wrote:
> > > > +#ifdef CONFIG_FSL_DPA_CHECKING
> > > > +#define DPA_ASSERT(x) \
> > > > + do { \
> > > > + if (!(x)) { \
> > > > + pr_crit("ASSERT: (%s:%d) %s\n", __FILE__,
> > > > __LINE__, \
> > > > + __stringify_1(x)); \
> > > > + dump_stack(); \
> > > > + panic("assertion failure"); \
> > >
> > > Not my call, but why panic() here?
> >
> > I'm pretty sure I've complained about this before (as well as all the
> > BUG_ONs).
> >
> Is the concern here just the call to panic()? I'm happy to change what
> happens when an issue is detected but the DPA_ASSERT() calls are very
> useful when testing changes to the driver and when bringing up the drivers
> on new silicon variants.
Use WARN_ON() or a variant thereof.
-Scott
--
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 02/11] soc/fsl: Introduce DPAA BMan device management driver
>
> On Fri, 2015-07-10 at 13:36 +0200, Paul Bolle wrote:
> > On do, 2015-07-09 at 16:21 -0400, Roy Pledge wrote:
> > > +#ifdef CONFIG_FSL_DPA_CHECKING
> > > +#define DPA_ASSERT(x) \
> > > + do { \
> > > + if (!(x)) { \
> > > + pr_crit("ASSERT: (%s:%d) %s\n", __FILE__, __LINE__, \
> > > + __stringify_1(x)); \
> > > + dump_stack(); \
> > > + panic("assertion failure"); \
> >
> > Not my call, but why panic() here?
>
> I'm pretty sure I've complained about this before (as well as all the
> BUG_ONs).
>
Is the concern here just the call to panic()? I'm happy to change what happens
when an issue is detected but the DPA_ASSERT() calls are very useful when
testing changes to the driver and when bringing up the drivers on new silicon
variants.
Re: [PATCH 02/11] soc/fsl: Introduce DPAA BMan device management driver
On Fri, 2015-07-10 at 13:29 -0500, Pledge Roy-R01356 wrote: > > return in_be32((void *)bm + offset); > > > ^ > > > [...]/drivers/soc/fsl/qbman/bman.c: In function ‘__bm_out’: > > > [...]/drivers/soc/fsl/qbman/bman.c:172:2: error: implicit declaration > > > of function ‘out_be32’ [-Werror=implicit-function-declaration] > > > out_be32((void *)bm + offset, val); > > > > These PPCisms will need to be fixed. LS1043A is an ARM chip with DPAA > > 1.0. > > LS1043 (ARM, Little Endian) support is still work in progress. The patches > for that are being tested now but are based on the SDK version of the > driver and will need to be massaged in order to get them applied here. Our > plan of record is to add upstream support for this at a later time. If you're already reworking this for upstream acceptance, why not fix the more obvious PPCisms now? -Scott -- 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 02/11] soc/fsl: Introduce DPAA BMan device management driver
> return in_be32((void *)bm + offset);
> > ^
> > [...]/drivers/soc/fsl/qbman/bman.c: In function ‘__bm_out’:
> > [...]/drivers/soc/fsl/qbman/bman.c:172:2: error: implicit declaration
> > of function ‘out_be32’ [-Werror=implicit-function-declaration]
> > out_be32((void *)bm + offset, val);
>
> These PPCisms will need to be fixed. LS1043A is an ARM chip with DPAA 1.0.
LS1043 (ARM, Little Endian) support is still work in progress. The patches for
that are being tested now but are based on the SDK version of the driver and
will need to be massaged in order to get them applied here. Our plan of record
is to add upstream support for this at a later time.
>
> > ^
> > [...]/drivers/soc/fsl/qbman/bman.c: In function ‘of_fsl_bman_probe’:
> > [...]/drivers/soc/fsl/qbman/bman.c:463:17: error: ‘NO_IRQ’ undeclared
> > (first use in this function)
> > if (err_irq == NO_IRQ) {
>
> This isn't even a PPCism. It's just wrong. Compare to zero instead.
Yeah - I recall fixing this when doing the ARM port but I guess I didn't make
the same change in this version. I will address this in the next version.
>
> -Scott
Re: [PATCH 02/11] soc/fsl: Introduce DPAA BMan device management driver
On Fri, 2015-07-10 at 13:36 +0200, Paul Bolle wrote:
> On do, 2015-07-09 at 16:21 -0400, Roy Pledge wrote:
> > +#ifdef CONFIG_FSL_DPA_CHECKING
> > +#define DPA_ASSERT(x) \
> > + do { \
> > + if (!(x)) { \
> > + pr_crit("ASSERT: (%s:%d) %s\n", __FILE__, __LINE__, \
> > + __stringify_1(x)); \
> > + dump_stack(); \
> > + panic("assertion failure"); \
>
> Not my call, but why panic() here?
I'm pretty sure I've complained about this before (as well as all the
BUG_ONs).
-Scott
--
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 02/11] soc/fsl: Introduce DPAA BMan device management driver
On Fri, 2015-07-10 at 10:38 +0200, Paul Bolle wrote:
> On do, 2015-07-09 at 16:21 -0400, Roy Pledge wrote:
> > --- /dev/null
> > +++ b/drivers/soc/fsl/qbman/Kconfig
> > @@ -0,0 +1,33 @@
> > +menuconfig FSL_DPA
> > + bool "Freescale DPAA support"
> > + depends on FSL_SOC || COMPILE_TEST
>
> Are you sure about COMPILE_TEST?
>
> > + default n
> > + help
> > + FSL Data-Path Acceleration Architecture drivers
> > +
> > + These are not the actual Ethernet driver(s)
> > +
> > +if FSL_DPA
>
> [...]
>
> > +config FSL_BMAN
> > + tristate "BMan device management"
> > + default n
> > + help
> > + FSL DPAA BMan driver
> > +
> > +endif # FSL_DPA
>
> Because with FSL_BMAN set to 'm' testing things with
> make -C ../../../.. M=$PWD bman.ko
>
> will not actually compile on x86_64:
>
> make: Entering directory '[...]'
> CC [M] [...]/drivers/soc/fsl/qbman/bman.o
> In file included from [...]/drivers/soc/fsl/qbman/bman_priv.h:33:0,
> from [...]/drivers/soc/fsl/qbman/bman.c:31:
> [...]/drivers/soc/fsl/qbman/dpaa_sys.h: In function ‘mfatb’:
> [...]/drivers/soc/fsl/qbman/dpaa_sys.h:134:8: error: implicit declaration
> of function ‘mfspr’ [-Werror=implicit-function-declaration]
>hi = mfspr(SPRN_ATBU);
> ^
> [...]/drivers/soc/fsl/qbman/dpaa_sys.h:134:14: error: ‘SPRN_ATBU’
> undeclared (first use in this function)
>hi = mfspr(SPRN_ATBU);
> ^
> [...]/drivers/soc/fsl/qbman/dpaa_sys.h:134:14: note: each undeclared
> identifier is reported only once for each function it appears in
> [...]/drivers/soc/fsl/qbman/dpaa_sys.h:135:14: error: ‘SPRN_ATBL’
> undeclared (first use in this function)
>lo = mfspr(SPRN_ATBL);
> ^
> [...]/drivers/soc/fsl/qbman/bman.c: In function ‘__bm_in’:
> [...]/drivers/soc/fsl/qbman/bman.c:168:9: error: implicit declaration of
> function ‘in_be32’ [-Werror=implicit-function-declaration]
> return in_be32((void *)bm + offset);
> ^
> [...]/drivers/soc/fsl/qbman/bman.c: In function ‘__bm_out’:
> [...]/drivers/soc/fsl/qbman/bman.c:172:2: error: implicit declaration of
> function ‘out_be32’ [-Werror=implicit-function-declaration]
> out_be32((void *)bm + offset, val);
These PPCisms will need to be fixed. LS1043A is an ARM chip with DPAA 1.0.
> ^
> [...]/drivers/soc/fsl/qbman/bman.c: In function ‘of_fsl_bman_probe’:
> [...]/drivers/soc/fsl/qbman/bman.c:463:17: error: ‘NO_IRQ’ undeclared
> (first use in this function)
> if (err_irq == NO_IRQ) {
This isn't even a PPCism. It's just wrong. Compare to zero instead.
-Scott
--
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 02/11] soc/fsl: Introduce DPAA BMan device management driver
On do, 2015-07-09 at 16:21 -0400, Roy Pledge wrote:
> --- /dev/null
> +++ b/drivers/soc/fsl/qbman/Kconfig
> +menuconfig FSL_DPA
> + bool "Freescale DPAA support"
> + depends on FSL_SOC || COMPILE_TEST
(I already commented on COMPILE_TEST in a separate mail.)
> + default n
> + help
> + FSL Data-Path Acceleration Architecture drivers
> +
> + These are not the actual Ethernet driver(s)
> +
> +if FSL_DPA
> +
> +config FSL_DPA_CHECKING
> + bool "additional driver checking"
> + default n
> + help
> + Compiles in additional checks to sanity-check the drivers and
> + any use of it by other code. Not recommended for performance
Only recommended for people that are certain none of the 100+ asserts
will ever trigger. See below.
> +config FSL_DPA_CAN_WAIT
> + bool
> + default y
> +
> +config FSL_DPA_CAN_WAIT_SYNC
> + bool
> + default y
Both these aren't actually used in this patch. The first patch that uses
them is 3/11.
Besides, the way these two symbols are implemented makes them function
as aliases for FSL_DPA. So why are they needed?
> +config FSL_BMAN
> + tristate "BMan device management"
> + default n
> + help
> + FSL DPAA BMan driver
(Will readers know what BMan means?)
> +endif # FSL_DPA
> --- /dev/null
> +++ b/drivers/soc/fsl/qbman/Makefile
> +obj-$(CONFIG_FSL_BMAN) += bman.o
> --- /dev/null
> +++ b/drivers/soc/fsl/qbman/bman.c
> +int bm_pool_set(u32 bpid, const u32 *thresholds)
> +{
> + if (!bm)
> + return -ENODEV;
> + bm_set_pool(bm, bpid, thresholds[0], thresholds[1],
> + thresholds[2], thresholds[3]);
> + return 0;
> +}
> +EXPORT_SYMBOL(bm_pool_set);
Nit: the first (caller of this function and) user of this export is
added in 3/11.
I couldn't find a MODULE_LICENSE() in bman.c. So building this as a
module and loading that module will generate a warning and taint the
kernel.
> --- /dev/null
> +++ b/drivers/soc/fsl/qbman/dpaa_sys.h
> +#ifdef CONFIG_FSL_DPA_CHECKING
> +#define DPA_ASSERT(x) \
> + do { \
> + if (!(x)) { \
> + pr_crit("ASSERT: (%s:%d) %s\n", __FILE__, __LINE__, \
> + __stringify_1(x)); \
> + dump_stack(); \
> + panic("assertion failure"); \
Not my call, but why panic() here?
> + } \
> + } while (0)
> +#else
> +#define DPA_ASSERT(x)
> +#endif
Thanks,
Paul Bolle
--
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 02/11] soc/fsl: Introduce DPAA BMan device management driver
On do, 2015-07-09 at 16:21 -0400, Roy Pledge wrote:
> --- /dev/null
> +++ b/drivers/soc/fsl/qbman/Kconfig
> @@ -0,0 +1,33 @@
> +menuconfig FSL_DPA
> + bool "Freescale DPAA support"
> + depends on FSL_SOC || COMPILE_TEST
Are you sure about COMPILE_TEST?
> + default n
> + help
> + FSL Data-Path Acceleration Architecture drivers
> +
> + These are not the actual Ethernet driver(s)
> +
> +if FSL_DPA
[...]
> +config FSL_BMAN
> + tristate "BMan device management"
> + default n
> + help
> + FSL DPAA BMan driver
> +
> +endif # FSL_DPA
Because with FSL_BMAN set to 'm' testing things with
make -C ../../../.. M=$PWD bman.ko
will not actually compile on x86_64:
make: Entering directory '[...]'
CC [M] [...]/drivers/soc/fsl/qbman/bman.o
In file included from [...]/drivers/soc/fsl/qbman/bman_priv.h:33:0,
from [...]/drivers/soc/fsl/qbman/bman.c:31:
[...]/drivers/soc/fsl/qbman/dpaa_sys.h: In function ‘mfatb’:
[...]/drivers/soc/fsl/qbman/dpaa_sys.h:134:8: error: implicit declaration of
function ‘mfspr’ [-Werror=implicit-function-declaration]
hi = mfspr(SPRN_ATBU);
^
[...]/drivers/soc/fsl/qbman/dpaa_sys.h:134:14: error: ‘SPRN_ATBU’ undeclared
(first use in this function)
hi = mfspr(SPRN_ATBU);
^
[...]/drivers/soc/fsl/qbman/dpaa_sys.h:134:14: note: each undeclared identifier
is reported only once for each function it appears in
[...]/drivers/soc/fsl/qbman/dpaa_sys.h:135:14: error: ‘SPRN_ATBL’ undeclared
(first use in this function)
lo = mfspr(SPRN_ATBL);
^
[...]/drivers/soc/fsl/qbman/bman.c: In function ‘__bm_in’:
[...]/drivers/soc/fsl/qbman/bman.c:168:9: error: implicit declaration of
function ‘in_be32’ [-Werror=implicit-function-declaration]
return in_be32((void *)bm + offset);
^
[...]/drivers/soc/fsl/qbman/bman.c: In function ‘__bm_out’:
[...]/drivers/soc/fsl/qbman/bman.c:172:2: error: implicit declaration of
function ‘out_be32’ [-Werror=implicit-function-declaration]
out_be32((void *)bm + offset, val);
^
[...]/drivers/soc/fsl/qbman/bman.c: In function ‘of_fsl_bman_probe’:
[...]/drivers/soc/fsl/qbman/bman.c:463:17: error: ‘NO_IRQ’ undeclared (first
use in this function)
if (err_irq == NO_IRQ) {
^
cc1: some warnings being treated as errors
scripts/Makefile.build:264: recipe for target
'[...]/drivers/soc/fsl/qbman/bman.o' failed
make[1]: *** [[...]/drivers/soc/fsl/qbman/bman.o] Error 1
Makefile:1568: recipe for target 'bman.ko' failed
make: *** [bman.ko] Error 2
make: Leaving directory '[...]'
Thanks,
Paul Bolle
--
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/
