Message-ID: <1440461499.25477.23.camel@freescale.com>
Subject: Re: [v2 02/11] soc/fsl: Introduce DPAA BMan device management driver
From: Scott Wood <scottwood@freescale.com>
To: <Roy.Pledge@freescale.com>
CC: <linuxppc-dev@lists.ozlabs.org>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Date: Mon, 24 Aug 2015 19:11:39 -0500
In-Reply-To: <1439410497-19039-3-git-send-email-Roy.Pledge@freescale.com>
References: <1439410497-19039-1-git-send-email-Roy.Pledge@freescale.com>
	 <1439410497-19039-3-git-send-email-Roy.Pledge@freescale.com>
Content-Type: text/plain; charset="UTF-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Aug 2015 00:13:03.3845
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR03MB1488
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 33045
Lines: 936

On Wed, 2015-08-12 at 16:14 -0400, Roy Pledge wrote:
> From: Geoff Thorpe <Geoff.Thorpe@freescale.com>
> 
> This driver enables the Freescale DPAA 1.0 Buffer Manager block. BMan
> is a hardware buffer pool manager that allows accelerators
> connected to the SoC datapath to acquire and release buffers during
> data processing.
> 
> Signed-off-by: Geoff Thorpe <Geoff.Thorpe@freescale.com>
> Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com>
> Signed-off-by: Roy Pledge <Roy.Pledge@freescale.com>
> ---
>  drivers/soc/Kconfig               |    1 +
>  drivers/soc/Makefile              |    1 +
>  drivers/soc/fsl/Kconfig           |    5 +
>  drivers/soc/fsl/Makefile          |    3 +
>  drivers/soc/fsl/qbman/Kconfig     |   25 ++
>  drivers/soc/fsl/qbman/Makefile    |    1 +
>  drivers/soc/fsl/qbman/bman.c      |  553 
> +++++++++++++++++++++++++++++++++++++
>  drivers/soc/fsl/qbman/bman_priv.h |   53 ++++
>  drivers/soc/fsl/qbman/dpaa_sys.h  |   55 ++++
>  9 files changed, 697 insertions(+)
>  create mode 100644 drivers/soc/fsl/Kconfig
>  create mode 100644 drivers/soc/fsl/Makefile
>  create mode 100644 drivers/soc/fsl/qbman/Kconfig
>  create mode 100644 drivers/soc/fsl/qbman/Makefile
>  create mode 100644 drivers/soc/fsl/qbman/bman.c
>  create mode 100644 drivers/soc/fsl/qbman/bman_priv.h
>  create mode 100644 drivers/soc/fsl/qbman/dpaa_sys.h
> 
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index 96ddecb..4e3c8f4 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -1,6 +1,7 @@
>  menu "SOC (System On Chip) specific Drivers"
>  
>  source "drivers/soc/mediatek/Kconfig"
> +source "drivers/soc/fsl/Kconfig"
>  source "drivers/soc/qcom/Kconfig"
>  source "drivers/soc/sunxi/Kconfig"
>  source "drivers/soc/ti/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 7dc7c0d..7adcd97 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -3,6 +3,7 @@
>  #
>  
>  obj-$(CONFIG_ARCH_MEDIATEK)  += mediatek/
> +obj-$(CONFIG_FSL_SOC)                += fsl/
>  obj-$(CONFIG_ARCH_QCOM)              += qcom/
>  obj-$(CONFIG_ARCH_SUNXI)     += sunxi/
>  obj-$(CONFIG_ARCH_TEGRA)     += tegra/
> diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig
> new file mode 100644
> index 0000000..daa9c0d
> --- /dev/null
> +++ b/drivers/soc/fsl/Kconfig
> @@ -0,0 +1,5 @@
> +menu "Freescale SOC (System On Chip) specific Drivers"
> +
> +source "drivers/soc/fsl/qbman/Kconfig"
> +
> +endmenu
> diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile
> new file mode 100644
> index 0000000..19e74bb
> --- /dev/null
> +++ b/drivers/soc/fsl/Makefile
> @@ -0,0 +1,3 @@
> +# Common
> +obj-$(CONFIG_FSL_DPA)                                += qbman/
> +
> diff --git a/drivers/soc/fsl/qbman/Kconfig b/drivers/soc/fsl/qbman/Kconfig
> new file mode 100644
> index 0000000..be4ae01
> --- /dev/null
> +++ b/drivers/soc/fsl/qbman/Kconfig
> @@ -0,0 +1,25 @@
> +menuconfig FSL_DPA
> +     bool "Freescale DPAA support"
> +     depends on FSL_SOC || COMPILE_TEST
> +     default n

Drop the COMPILE_TEST -- this driver still has PPCisms that will break the 
build elsewhere.

> +     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
> +
> +config FSL_BMAN
> +     tristate "BMan device management"
> +     default n
> +     help
> +             FSL DPAA BMan driver

Please describe here what BMan is and when it should be enabled.  Why isn't 
it always enabled when DPA is enabled?

> +endif # FSL_DPA
> diff --git a/drivers/soc/fsl/qbman/Makefile b/drivers/soc/fsl/qbman/Makefile
> new file mode 100644
> index 0000000..02014d9
> --- /dev/null
> +++ b/drivers/soc/fsl/qbman/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_FSL_BMAN)                               += bman.o
> diff --git a/drivers/soc/fsl/qbman/bman.c b/drivers/soc/fsl/qbman/bman.c
> new file mode 100644
> index 0000000..9a500ce
> --- /dev/null
> +++ b/drivers/soc/fsl/qbman/bman.c
> @@ -0,0 +1,553 @@
> +/* Copyright (c) 2009 - 2015 Freescale Semiconductor, Inc.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are 
> met:
> + *     * Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + *     * Neither the name of Freescale Semiconductor nor the
> + *    names of its contributors may be used to endorse or promote products
> + *    derived from this software without specific prior written permission.
> + *
> + * ALTERNATIVELY, this software may be distributed under the terms of the
> + * GNU General Public License ("GPL") as published by the Free Software
> + * Foundation, either version 2 of that License or (at your option) any
> + * later version.
> + *
> + * THIS SOFTWARE IS PROVIDED BY Freescale Semiconductor ``AS IS'' AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE 
> IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL Freescale Semiconductor BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL 
> DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR 
> SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED 
> AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR 
> TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE 
> OF THIS
> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "bman_priv.h"
> +
> +/* Last updated for v00.79 of the BG */

Don't make obscure references to ancient internal documents.

> +
> +struct bman;
> +
> +/* Register offsets */
> +#define REG_POOL_SWDET(n)    (0x0000 + ((n) * 0x04))
> +#define REG_POOL_HWDET(n)    (0x0100 + ((n) * 0x04))
> +#define REG_POOL_SWDXT(n)    (0x0200 + ((n) * 0x04))
> +#define REG_POOL_HWDXT(n)    (0x0300 + ((n) * 0x04))
> +#define REG_POOL_CONTENT(n)  (0x0600 + ((n) * 0x04))
> +#define REG_FBPR_FPC         0x0800
> +#define REG_ECSR             0x0a00
> +#define REG_ECIR             0x0a04
> +#define REG_EADR             0x0a08
> +#define REG_EDATA(n)         (0x0a10 + ((n) * 0x04))
> +#define REG_SBEC(n)          (0x0a80 + ((n) * 0x04))
> +#define REG_IP_REV_1         0x0bf8
> +#define REG_IP_REV_2         0x0bfc
> +#define REG_FBPR_BARE                0x0c00
> +#define REG_FBPR_BAR         0x0c04
> +#define REG_FBPR_AR          0x0c10
> +#define REG_SRCIDR           0x0d04
> +#define REG_LIODNR           0x0d08
> +#define REG_ERR_ISR          0x0e00  /* + "enum bm_isr_reg" */
> +
> +/* Used by all error interrupt registers except 'inhibit' */
> +#define BM_EIRQ_IVCI 0x00000010      /* Invalid Command Verb */
> +#define BM_EIRQ_FLWI 0x00000008      /* FBPR Low Watermark */
> +#define BM_EIRQ_MBEI 0x00000004      /* Multi-bit ECC Error */
> +#define BM_EIRQ_SBEI 0x00000002      /* Single-bit ECC Error */
> +#define BM_EIRQ_BSCN 0x00000001      /* pool State Change Notification */
> +
> +/* BMAN_ECIR valid error bit */
> +#define PORTAL_ECSR_ERR      (BM_EIRQ_IVCI)
> +
> +union bman_ecir {
> +     u32 ecir_raw;
> +     struct {
> +             u32 __reserved1:4;
> +             u32 portal_num:4;
> +             u32 __reserved2:12;
> +             u32 numb:4;
> +             u32 __reserved3:2;
> +             u32 pid:6;
> +     } __packed info;
> +};

I've seen GCC generate some awful code when bitfields are combined with 
packed.  Why are either necessary here?


> +
> +union bman_eadr {
> +     u32 eadr_raw;
> +     struct {
> +             u32 __reserved1:5;
> +             u32 memid:3;
> +             u32 __reserved2:14;
> +             u32 eadr:10;
> +     } __packed info;
> +};
> +
> +struct bman_hwerr_txt {
> +     u32 mask;
> +     const char *txt;
> +};
> +
> +#define BMAN_HWE_TXT(a, b) { .mask = BM_EIRQ_##a, .txt = b }
> +
> +static const struct bman_hwerr_txt bman_hwerr_txts[] = {
> +     BMAN_HWE_TXT(IVCI, "Invalid Command Verb"),
> +     BMAN_HWE_TXT(FLWI, "FBPR Low Watermark"),
> +     BMAN_HWE_TXT(MBEI, "Multi-bit ECC Error"),
> +     BMAN_HWE_TXT(SBEI, "Single-bit ECC Error"),
> +     BMAN_HWE_TXT(BSCN, "Pool State Change Notification"),
> +};
> +#define BMAN_HWE_COUNT (sizeof(bman_hwerr_txts)/sizeof(struct 
> bman_hwerr_txt))

Use ARRAY_SIZE().

> +struct bman_error_info_mdata {
> +     u16 addr_mask;
> +     u16 bits;
> +     const char *txt;
> +};
> +
> +#define BMAN_ERR_MDATA(a, b, c) { .addr_mask = a, .bits = b, .txt = c}
> +static const struct bman_error_info_mdata error_mdata[] = {
> +     BMAN_ERR_MDATA(0x03FF, 192, "Stockpile memory"),
> +     BMAN_ERR_MDATA(0x00FF, 256, "SW portal ring memory port 1"),
> +     BMAN_ERR_MDATA(0x00FF, 256, "SW portal ring memory port 2"),
> +};

Is all this detail necessary (and complete/up-to-date), versus printing raw 

values and having the person investigating the issue look up the error 
values?  Is there anything a normal user, who wouldn't be expected to look 
such things up, can do themselves in response to seeing the string output?

> +#define BMAN_ERR_MDATA_COUNT \
> +     (sizeof(error_mdata)/sizeof(struct bman_error_info_mdata))
> +
> +/* Add this in Kconfig */
> +#define BMAN_ERRS_TO_UNENABLE (BM_EIRQ_FLWI)

"unenable"?

Why should it be added to Kconfig?

> +
> +/**
> + * bm_err_isr_<reg>_<verb> - Manipulate global interrupt registers
> + * @v: for accessors that write values, this is the 32-bit value
> + *
> + * Manipulates BMAN_ERR_ISR, BMAN_ERR_IER, BMAN_ERR_ISDR, BMAN_ERR_IIR. All
> + * manipulations except bm_err_isr_[un]inhibit() use 32-bit masks composed 
> of
> + * the BM_EIRQ_*** definitions. Note that "bm_err_isr_enable_write" means
> + * "write the enable register" rather than "enable the write register"!
> + */
> +#define bm_err_isr_status_read(bm)   \
> +             __bm_err_isr_read(bm, bm_isr_status)
> +#define bm_err_isr_status_clear(bm, m)       \
> +             __bm_err_isr_write(bm, bm_isr_status, m)
> +#define bm_err_isr_enable_read(bm)   \
> +             __bm_err_isr_read(bm, bm_isr_enable)
> +#define bm_err_isr_enable_write(bm, v)       \
> +             __bm_err_isr_write(bm, bm_isr_enable, v)
> +#define bm_err_isr_disable_read(bm)  \
> +             __bm_err_isr_read(bm, bm_isr_disable)
> +#define bm_err_isr_disable_write(bm, v)      \
> +             __bm_err_isr_write(bm, bm_isr_disable, v)
> +#define bm_err_isr_inhibit(bm)               \
> +             __bm_err_isr_write(bm, bm_isr_inhibit, 1)
> +#define bm_err_isr_uninhibit(bm)     \
> +             __bm_err_isr_write(bm, bm_isr_inhibit, 0)

Do these wrappers really contribute much to readability, especially given 
that you need a comment to disambiguate one of them?

> +/*
> + * TODO: unimplemented registers
> + *
> + * BMAN_POOLk_SDCNT, BMAN_POOLk_HDCNT, BMAN_FULT,
> + * BMAN_VLDPL, BMAN_EECC, BMAN_SBET, BMAN_EINJ
> + */
> +

Why are they TODO?  What do you plan to do with them?

> +/* Encapsulate "struct bman *" as a cast of the register space address. */
> +
> +static struct bman *bm_create(void *regs)
> +{
> +     return (struct bman *)regs;
> +}

NACK

If you want the regs to be a struct, then actually define the members of the 
struct, use __iomem, don't hide the cast in a vaguely named function that 
isn't actually creating anything, and call it something like "struct 
bman_regs" so that it's clear that it's a register struct rather than a 
driver state struct.


> +
> +static inline u32 __bm_in(struct bman *bm, u32 offset)
> +{
> +     return in_be32((void *)bm + offset);
> +}
> +static inline void __bm_out(struct bman *bm, u32 offset, u32 val)
> +{
> +     out_be32((void *)bm + offset, val);
> +}
> +#define bm_in(reg)           __bm_in(bm, REG_##reg)
> +#define bm_out(reg, val)     __bm_out(bm, REG_##reg, val)
> +
> +static u32 __bm_err_isr_read(struct bman *bm, enum bm_isr_reg n)
> +{
> +     return __bm_in(bm, REG_ERR_ISR + (n << 2));
> +}
> +
> +static void __bm_err_isr_write(struct bman *bm, enum bm_isr_reg n, u32 val)
> +{
> +     __bm_out(bm, REG_ERR_ISR + (n << 2), val);
> +}
> +
> +static void bm_get_version(struct bman *bm, u16 *id, u8 *major, u8 *minor)
> +{
> +     u32 v = bm_in(IP_REV_1);
> +     *id = (v >> 16);
> +     *major = (v >> 8) & 0xff;
> +     *minor = v & 0xff;
> +}
> +
> +static u32 __generate_thresh(u32 val, int roundup)
> +{
> +     u32 e = 0;      /* co-efficient, exponent */
> +     int oddbit = 0;
> +
> +     while (val > 0xff) {
> +             oddbit = val & 1;
> +             val >>= 1;
> +             e++;
> +             if (roundup && oddbit)
> +                     val++;
> +     }
> +     DPA_ASSERT(e < 0x10);
> +     return val | (e << 8);
> +}
> +
> +static void bm_set_pool(struct bman *bm, u8 pool, u32 swdet, u32 swdxt,
> +                     u32 hwdet, u32 hwdxt)
> +{
> +     DPA_ASSERT(pool < bman_pool_max);
> +
> +     bm_out(POOL_SWDET(pool), __generate_thresh(swdet, 0));
> +     bm_out(POOL_SWDXT(pool), __generate_thresh(swdxt, 1));
> +     bm_out(POOL_HWDET(pool), __generate_thresh(hwdet, 0));
> +     bm_out(POOL_HWDXT(pool), __generate_thresh(hwdxt, 1));
> +}
> +
> +static void bm_set_memory(struct bman *bm, u64 ba, int prio, u32 size)
> +{
> +     u32 exp = ilog2(size);
> +     /* choke if size isn't within range */
> +     DPA_ASSERT((size >= 4096) && (size <= 1073741824) &&
> +                     is_power_of_2(size));
> +     /* choke if '[e]ba' has lower-alignment than 'size' */
> +     DPA_ASSERT(!(ba & (size - 1)));
> +     bm_out(FBPR_BARE, upper_32_bits(ba));
> +     bm_out(FBPR_BAR, lower_32_bits(ba));
> +     bm_out(FBPR_AR, (prio ? 0x40000000 : 0) | (exp - 1));
> +}
> +
> +/*****************/
> +/* Config driver */
> +/*****************/
> +
> +/* We support only one of these. */
> +static struct bman *bm;

Please don't have both a file-scope "bm" and parameter "bm"s in the same file.

> +
> +/* And this state belongs to 'bm' */
> +static dma_addr_t fbpr_a;
> +static size_t fbpr_sz;

So why aren't they in "struct bman"?

> +static int bman_fbpr(struct reserved_mem *rmem)
> +{
> +     fbpr_a = rmem->base;
> +     fbpr_sz = rmem->size;
> +
> +     WARN_ON(!(fbpr_a && fbpr_sz));
> +
> +     return 0;
> +}
> +RESERVEDMEM_OF_DECLARE(bman_fbpr, "fsl,bman-fbpr", bman_fbpr);
> +
> +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);

Why does there need to be both bm_pool_set() and bm_set_pool() that do the 
some thing with a different calling convention?

> +
> +static void log_edata_bits(u32 bit_count)
> +{
> +     u32 i, j, mask = 0xffffffff;
> +
> +     pr_warn("ErrInt, EDATA:\n");
> +     i = bit_count/32;
> +     if (bit_count%32) {
> +             i++;
> +             mask = ~(mask << bit_count%32);
> +     }
> +     j = 16-i;
> +     pr_warn("  0x%08x\n", bm_in(EDATA(j)) & mask);
> +     j++;
> +     for (; j < 16; j++)
> +             pr_warn("  0x%08x\n", bm_in(EDATA(j)));
> +}

Put spaces around binary operators.

Where does the 16 come from?  I see 8 EDATA registers, and no possibility for 
a non-multiple-of-32 number of bits.  I also see the manual saying that "bit 0
from the memory is always in bit 0 of BMAN_EDATA0" so why is this code trying 
to right-justify it?

Why not just keep it simple and print all the EDATA registers?

> +
> +static void log_additional_error_info(u32 isr_val, u32 ecsr_val)
> +{
> +     union bman_ecir ecir_val;
> +     union bman_eadr eadr_val;
> +
> +     ecir_val.ecir_raw = bm_in(ECIR);
> +     /* Is portal info valid */
> +     if (ecsr_val & PORTAL_ECSR_ERR) {
> +             pr_warn("ErrInt: SWP id %d, numb %d, pid %d\n",
> +                     ecir_val.info.portal_num, ecir_val.info.numb,
> +                     ecir_val.info.pid);
> +     }
> +     if (ecsr_val & (BM_EIRQ_SBEI|BM_EIRQ_MBEI)) {
> +             eadr_val.eadr_raw = bm_in(EADR);
> +             pr_warn("ErrInt: EADR Memory: %s, 0x%x\n",
> +                     error_mdata[eadr_val.info.memid].txt,
> +                     error_mdata[eadr_val.info.memid].addr_mask
> +                             & eadr_val.info.eadr);
> +             log_edata_bits(error_mdata[eadr_val.info.memid].bits);
> +     }
> +}
> +
> +/* BMan interrupt handler */
> +static irqreturn_t bman_isr(int irq, void *ptr)
> +{
> +     u32 isr_val, ier_val, ecsr_val, isr_mask, i;
> +
> +     ier_val = bm_err_isr_enable_read(bm);
> +     isr_val = bm_err_isr_status_read(bm);
> +     ecsr_val = bm_in(ECSR);
> +     isr_mask = isr_val & ier_val;
> +
> +     if (!isr_mask)
> +             return IRQ_NONE;
> +
> +     for (i = 0; i < BMAN_HWE_COUNT; i++) {
> +             if (bman_hwerr_txts[i].mask & isr_mask) {
> +                     pr_warn("ErrInt: %s\n", bman_hwerr_txts[i].txt);

Use ptr to get to a struct device so you can use dev_err() here.

> +                     if (bman_hwerr_txts[i].mask & ecsr_val) {
> +                             log_additional_error_info(isr_mask, ecsr_val);
> +                             /* Re-arm error capture registers */
> +                             bm_out(ECSR, ecsr_val);
> +                     }
> +                     if (bman_hwerr_txts[i].mask & BMAN_ERRS_TO_UNENABLE) {
> +                             pr_devel("Un-enabling error 0x%x\n",
> +                                     bman_hwerr_txts[i].mask);
> +                             ier_val &= ~bman_hwerr_txts[i].mask;
> +                             bm_err_isr_enable_write(bm, ier_val);
> +                     }
> +             }
> +     }
> +     bm_err_isr_status_clear(bm, isr_val);
> +
> +     return IRQ_HANDLED;
> +}
> +
> +u32 bm_pool_free_buffers(u32 bpid)
> +{
> +     return bm_in(POOL_CONTENT(bpid));
> +}
> +
> +static ssize_t show_fbpr_fpc(struct device *dev,
> +     struct device_attribute *dev_attr, char *buf)
> +{
> +     return snprintf(buf, PAGE_SIZE, "%u\n", bm_in(FBPR_FPC));
> +};
> +
> +static ssize_t show_pool_count(struct device *dev,
> +     struct device_attribute *dev_attr, char *buf)
> +{
> +     u32 data;
> +     int i;
> +
> +     if (kstrtoint(dev_attr->attr.name, 10, &i))
> +             return -EINVAL;
> +     data = bm_in(POOL_CONTENT(i));
> +     return snprintf(buf, PAGE_SIZE, "%d\n", data);
> +};
> +
> +static ssize_t show_err_isr(struct device *dev,
> +     struct device_attribute *dev_attr, char *buf)
> +{
> +     return snprintf(buf, PAGE_SIZE, "0x%08x\n", bm_in(ERR_ISR));
> +};
> +
> +static ssize_t show_sbec(struct device *dev,
> +     struct device_attribute *dev_attr, char *buf)
> +{
> +     int i;
> +
> +     if (sscanf(dev_attr->attr.name, "sbec_%d", &i) != 1)
> +             return -EINVAL;
> +     return snprintf(buf, PAGE_SIZE, "%u\n", bm_in(SBEC(i)));
> +};
> +
> +static DEVICE_ATTR(err_isr, S_IRUSR, show_err_isr, NULL);
> +static DEVICE_ATTR(fbpr_fpc, S_IRUSR, show_fbpr_fpc, NULL);
> +
> +/* Didn't use DEVICE_ATTR as 64 of this would be required.
> + * Initialize them when needed. */
> +static char *name_attrs_pool_count; /* "xx" + null-terminator */
> +static struct device_attribute *dev_attr_buffer_pool_count;
> +
> +static DEVICE_ATTR(sbec_0, S_IRUSR, show_sbec, NULL);
> +static DEVICE_ATTR(sbec_1, S_IRUSR, show_sbec, NULL);
> +
> +static struct attribute *bman_dev_attributes[] = {
> +     &dev_attr_fbpr_fpc.attr,
> +     &dev_attr_err_isr.attr,
> +     NULL
> +};
> +
> +static struct attribute *bman_dev_ecr_attributes[] = {
> +     &dev_attr_sbec_0.attr,
> +     &dev_attr_sbec_1.attr,
> +     NULL
> +};
> +
> +static struct attribute **bman_dev_pool_count_attributes;
> +
> +/* root level */
> +static const struct attribute_group bman_dev_attr_grp = {
> +     .name = NULL,
> +     .attrs = bman_dev_attributes
> +};
> +static const struct attribute_group bman_dev_ecr_grp = {
> +     .name = "error_capture",
> +     .attrs = bman_dev_ecr_attributes
> +};
> +static struct attribute_group bman_dev_pool_countent_grp = {
> +     .name = "pool_count",
> +};

What do these sysfs attributes get used for, and where are they documented?

> +static int of_fsl_bman_remove(struct platform_device *ofdev)
> +{
> +     sysfs_remove_group(&ofdev->dev.kobj, &bman_dev_attr_grp);
> +     return 0;
> +};

This does not seem sufficient.  At the least, you're creating more than one 
sysfs group in probe*().  How does this stop references to memory that was 
allocated using devm?  Keep in mind that userspace can unbind a device from 
its driver, and there's nothing you can do to block it (unlike module 
removal).  The remove() function has to be able to completely halt access to 
the device before returning, and any memory that needs to survive past 
remove() can't be allocated with devm.

If unbinding is not viable for this driver (which does not seem completely 
unreasonable given that it is subsystem infrastructure rather than a 
standalone device), then at least WARN in remove() (and no point doing 
anything else) to make it obvious that this is unsupported.

> +
> +static int of_fsl_bman_probe(struct platform_device *ofdev)
> +{
> +     int ret, err_irq, i;
> +     struct device *dev = &ofdev->dev;
> +     struct device_node *node = dev->of_node;
> +     struct resource res;
> +     u32 __iomem *regs;
> +     u16 id;
> +     u8 major, minor;
> +
> +     if (!of_device_is_available(node))
> +             return -ENODEV;

The base OF platform code will check this for you.

> +
> +     ret = of_address_to_resource(node, 0, &res);
> +     if (ret) {
> +             dev_err(dev, "Can't get %s property 'reg'\n", node->full_name);
> +             return ret;
> +     }
> +     regs = devm_ioremap(dev, res.start, res.end - res.start + 1);
> +     if (!regs)
> +             return -ENXIO;
> +
> +     bm = bm_create(regs);
> +
> +     bm_get_version(bm, &id, &major, &minor);
> +     dev_info(dev, "Bman ver:%04x,%02x,%02x\n", id, major, minor);
> +     if ((major == 1) && (minor == 0))
> +             bman_pool_max = 64;
> +     else if ((major == 2) && (minor == 0))
> +             bman_pool_max = 8;
> +     else if ((major == 2) && (minor == 1))
> +             bman_pool_max = 64;
> +     else
> +             dev_warn(dev, "unknown Bman version, default to rev1.0\n");

Why is it safe to default to rev 1.0 rather than refusing to bind to an 
unknown version?

Also, unnecessary parentheses.

> +
> +
> +     bm_set_memory(bm, fbpr_a, 0, fbpr_sz);
> +
> +     err_irq = of_irq_to_resource(node, 0, NULL);
> +     if (err_irq == NO_IRQ) {
> +             dev_info(dev, "Can't get %s property 'interrupts'\n",
> +                      node->full_name);
> +             return -ENODEV;
> +     }
> +     ret = devm_request_irq(dev, err_irq, bman_isr, IRQF_SHARED, "bman-err",
> +                            node);
> +     if (ret)  {
> +             dev_err(dev, "devm_request_irq() failed %d for '%s'\n",
> +                     ret, node->full_name);
> +             return ret;
> +     }
> +     /* Disable Buffer Pool State Change */
> +     bm_err_isr_disable_write(bm, BM_EIRQ_BSCN);
> +     /* Write-to-clear any stale bits, (eg. starvation being asserted prior
> +      * to resource allocation during driver init). */
> +     bm_err_isr_status_clear(bm, 0xffffffff);
> +     /* Enable Error Interrupts */
> +     bm_err_isr_enable_write(bm, 0xffffffff);
> +
> +     ret = sysfs_create_group(&dev->kobj, &bman_dev_attr_grp);
> +     if (ret)
> +             goto done;
> +     ret = sysfs_create_group(&dev->kobj, &bman_dev_ecr_grp);
> +     if (ret)
> +             goto del_group_0;
> +
> +     name_attrs_pool_count = devm_kmalloc(dev,
> +             sizeof(char) * bman_pool_max * 3, GFP_KERNEL);
> +     if (!name_attrs_pool_count)
> +             goto del_group_1;
> +
> +     dev_attr_buffer_pool_count = devm_kmalloc(dev,
> +             sizeof(struct device_attribute) * bman_pool_max, GFP_KERNEL);
> +     if (!dev_attr_buffer_pool_count)
> +             goto del_group_1;
> +
> +     bman_dev_pool_count_attributes = devm_kmalloc(dev,
> +             sizeof(struct attribute *) * (bman_pool_max + 1), GFP_KERNEL);
> +     if (!bman_dev_pool_count_attributes)
> +             goto del_group_1;
> +
> +     for (i = 0; i < bman_pool_max; i++) {
> +             ret = scnprintf((name_attrs_pool_count + i * 3), 3, "%d", i);
> +             if (!ret)
> +                     goto del_group_1;
> +             dev_attr_buffer_pool_count[i].attr.name =
> +                     (name_attrs_pool_count + i * 3);
> +             dev_attr_buffer_pool_count[i].attr.mode = S_IRUSR;
> +             dev_attr_buffer_pool_count[i].show = show_pool_count;
> +             bman_dev_pool_count_attributes[i] =
> +                     &dev_attr_buffer_pool_count[i].attr;
> +     }
> +     bman_dev_pool_count_attributes[bman_pool_max] = NULL;
> +
> +     bman_dev_pool_countent_grp.attrs = bman_dev_pool_count_attributes;
> +
> +     ret = sysfs_create_group(&dev->kobj, &bman_dev_pool_countent_grp);
> +     if (ret)
> +             goto del_group_1;
> +
> +     goto done;
> +
> +del_group_1:
> +     sysfs_remove_group(&dev->kobj, &bman_dev_ecr_grp);
> +del_group_0:
> +     sysfs_remove_group(&dev->kobj, &bman_dev_attr_grp);
> +done:
> +     if (ret)
> +             dev_err(dev, "Cannot create dev attributes ret=%d\n", ret);
> +
> +     return ret;
> +};
> +
> +static const struct of_device_id of_fsl_bman_ids[] = {
> +     {
> +             .compatible = "fsl,bman",
> +     },
> +     {}
> +};
> +MODULE_DEVICE_TABLE(of, of_fsl_bman_ids);
> +
> +static struct platform_driver of_fsl_bman_driver = {
> +     .driver = {
> +             .name = KBUILD_MODNAME,
> +             .of_match_table = of_fsl_bman_ids,
> +     },
> +     .probe = of_fsl_bman_probe,
> +     .remove = of_fsl_bman_remove,
> +};
> +
> +module_platform_driver(of_fsl_bman_driver);
> diff --git a/drivers/soc/fsl/qbman/bman_priv.h 
> b/drivers/soc/fsl/qbman/bman_priv.h
> new file mode 100644
> index 0000000..702e469
> --- /dev/null
> +++ b/drivers/soc/fsl/qbman/bman_priv.h
> @@ -0,0 +1,53 @@
> +/* Copyright 2008 - 2015 Freescale Semiconductor, Inc.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are 
> met:
> + *     * Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + *     * Neither the name of Freescale Semiconductor nor the
> + *    names of its contributors may be used to endorse or promote products
> + *    derived from this software without specific prior written permission.
> + *
> + * ALTERNATIVELY, this software may be distributed under the terms of the
> + * GNU General Public License ("GPL") as published by the Free Software
> + * Foundation, either version 2 of that License or (at your option) any
> + * later version.
> + *
> + * THIS SOFTWARE IS PROVIDED BY Freescale Semiconductor ``AS IS'' AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE 
> IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL Freescale Semiconductor BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL 
> DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR 
> SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED 
> AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR 
> TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE 
> OF THIS
> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include "dpaa_sys.h"
> +
> +/* used by CCSR and portal interrupt code */
> +enum bm_isr_reg {
> +     bm_isr_status = 0,
> +     bm_isr_enable = 1,
> +     bm_isr_disable = 2,
> +     bm_isr_inhibit = 3
> +};

struct bm_isr_regs {
        u32 status;
        u32 enable;
        u32 disable;
        u32 inhibit;
};

...and then you won't need __bm_err_isr_read/__bm_err_isr_write because it'll 
just be a straightforward in_be32/out_be32() (or an wrapper that handles 
endianness, if ls1043a ended up making this little-endian).

Or better, define all the registers in a struct rather than just these, and 
more of the I/O wrappers go away.

> +
> +/* Set depletion thresholds associated with a buffer pool. Requires that 
> the
> + * operating system have access to BMan CCSR (ie. compiled in support and
> + * run-time access courtesy of the device-tree). */

/* 
 * Please use standard
 * Linux multi-line
 * comment style,
 * here and elsewhere.
 */


> +int bm_pool_set(u32 bpid, const u32 *thresholds);
> +#define BM_POOL_THRESH_SW_ENTER 0
> +#define BM_POOL_THRESH_SW_EXIT       1
> +#define BM_POOL_THRESH_HW_ENTER 2
> +#define BM_POOL_THRESH_HW_EXIT       3
> +
> +/* Read the free buffer count for a given buffer */
> +u32 bm_pool_free_buffers(u32 bpid);
> diff --git a/drivers/soc/fsl/qbman/dpaa_sys.h 
> b/drivers/soc/fsl/qbman/dpaa_sys.h
> new file mode 100644
> index 0000000..cdaf3f7
> --- /dev/null
> +++ b/drivers/soc/fsl/qbman/dpaa_sys.h
> @@ -0,0 +1,55 @@
> +/* Copyright 2008 - 2015 Freescale Semiconductor, Inc.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are 
> met:
> + *     * Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + *     * Neither the name of Freescale Semiconductor nor the
> + *    names of its contributors may be used to endorse or promote products
> + *    derived from this software without specific prior written permission.
> + *
> + * ALTERNATIVELY, this software may be distributed under the terms of the
> + * GNU General Public License ("GPL") as published by the Free Software
> + * Foundation, either version 2 of that License or (at your option) any
> + * later version.
> + *
> + * THIS SOFTWARE IS PROVIDED BY Freescale Semiconductor ``AS IS'' AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE 
> IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL Freescale Semiconductor BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL 
> DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR 
> SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED 
> AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR 
> TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE 
> OF THIS
> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef __DPAA_SYS_H
> +#define __DPAA_SYS_H
> +
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/platform_device.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"); \
> +             } \

Again, do not panic here.  Use one of the WARN macros.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/