Re: [PATCH v2 4/7] bestcomm: core bestcomm support for Freescale MPC5200

On 10/15/07, Kumar Gala [off-list ref] wrote:

(Comments just on SRAM code)

I think this should be made generic and be utility functionality to
rheap.

CPM, CPM2, QE, L2 SRAM, etc can all use this.  I'd rather we didn't
have 3 ways to do the exact same functionality.  (cpm_dpalloc,
cpm_dpfree, qe_muram_alloc, qe_muram_free)

Fair enough; but not in this patch set.  This series is working
support for bestcomm.  To go to the more generic level of being used
by multiple parts should be done in a separate series.

see other comments inline.

quoted

+

diff --git a/arch/powerpc/sysdev/bestcomm/sram.c b/arch/powerpc/

sysdev/bestcomm/sram.c
new file mode 100644
index 0000000..b3f2ed1

--- /dev/null
+++ b/arch/powerpc/sysdev/bestcomm/sram.c

@@ -0,0 +1,177 @@
+/*
+ * Simple memory allocator for on-board SRAM
+ *
+ *
+ * Maintainer : Sylvain Munaut <tnt@246tNt.com>
+ *
+ * Copyright (C) 2005 Sylvain Munaut <tnt@246tNt.com>
+ *
+ * This file is licensed under the terms of the GNU General Public

License
+ * version 2. This program is licensed "as is" without any
warranty of any
+ * kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
+#include <linux/ioport.h>
+#include <linux/of.h>
+
+#include <asm/io.h>
+#include <asm/mmu.h>
+
+#include "sram.h"
+
+
+/* Struct keeping our 'state' */
+struct bcom_sram *bcom_sram = NULL;

shouldn't be global, so we can support more than one SRAM.

Again; I agree, but I'm not going to make that change in this series.

quoted

+EXPORT_SYMBOL_GPL(bcom_sram);        /* needed for inline functions */
+
+
+/*
======================================================================
== */
+/* Public
API                                                               */
+/*
======================================================================
== */
+/* DO NOT USE in interrupts, if needed in irq handler, we should
use the
+   _irqsave version of the spin_locks */
+
+int bcom_sram_init(struct device_node *sram_node, char *owner)
+{
+     int rv;
+     const u32 *regaddr_p;
+     u64 regaddr64, size64;
+     unsigned int psize;
+
+     /* Create our state struct */
+     if (bcom_sram) {
+             printk(KERN_ERR "%s: bcom_sram_init: "
+                     "Already initialiwed !\n", owner);
+             return -EBUSY;
+     }
+
+     bcom_sram = kmalloc(sizeof(struct bcom_sram), GFP_KERNEL);

should return this handle to the user.

To be done when this driver is changed to support multiple sram regions.

quoted

diff --git a/arch/powerpc/sysdev/bestcomm/sram.h b/arch/powerpc/

sysdev/bestcomm/sram.h
new file mode 100644
index 0000000..b6d6689

--- /dev/null
+++ b/arch/powerpc/sysdev/bestcomm/sram.h

@@ -0,0 +1,54 @@
+/*
+ * Handling of a sram zone for bestcomm
+ *
+ *
+ * Copyright (C) 2007 Sylvain Munaut <tnt@246tNt.com>
+ *
+ * This file is licensed under the terms of the GNU General Public

License
+ * version 2. This program is licensed "as is" without any
warranty of any
+ * kind, whether express or implied.
+ */
+
+#ifndef __BESTCOMM_SRAM_H__
+#define __BESTCOMM_SRAM_H__
+
+#include <asm/rheap.h>
+#include <asm/mmu.h>
+#include <linux/spinlock.h>
+
+
+/* Structure used internally */
+     /* The internals are here for the inline functions
+      * sake, certainly not for the user to mess with !
+      */
+struct bcom_sram {
+     phys_addr_t              base_phys;
+     void                    *base_virt;

__iomem for base_virt?

I'll take a look

quoted

+     unsigned int             size;
+     rh_info_t               *rh;
+     spinlock_t               lock;
+};
+
+extern struct bcom_sram *bcom_sram;
+
+
+/* Public API */
+extern int  bcom_sram_init(struct device_node *sram_node, char
*owner);
+extern void bcom_sram_cleanup(void);
+
+extern void* bcom_sram_alloc(int size, int align, phys_addr_t *phys);
+extern void  bcom_sram_free(void *ptr);
+
+static inline phys_addr_t bcom_sram_va2pa(void *va) {

should take bcom_sram handle as a param

quoted

+     return bcom_sram->base_phys +
+             (unsigned long)(va - bcom_sram->base_virt);

shouldn't this cast be phys_addr_t?

I don't think so.  ->base_phys is of type phys_addr_t;
(va-bcom_sram->base_virt) is just an offset from ->base_phys.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195