This patch adds the pruss MFD driver and associated include files.
Signed-off-by: Subhasish Ghosh <[email protected]>
---
drivers/mfd/Kconfig | 10 +
drivers/mfd/Makefile | 1 +
drivers/mfd/da8xx_pru.c | 446 +++++++++++++++++++++++++++++++
include/linux/mfd/pruss/da8xx_pru.h | 122 +++++++++
include/linux/mfd/pruss/da8xx_prucore.h | 74 +++++
5 files changed, 653 insertions(+), 0 deletions(-)
create mode 100644 drivers/mfd/da8xx_pru.c
create mode 100644 include/linux/mfd/pruss/da8xx_pru.h
create mode 100644 include/linux/mfd/pruss/da8xx_prucore.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index fd01836..6c437df 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -81,6 +81,16 @@ config MFD_DM355EVM_MSP
boards. MSP430 firmware manages resets and power sequencing,
inputs from buttons and the IR remote, LEDs, an RTC, and more.
+config MFD_DA8XX_PRUSS
+ tristate "Texas Instruments DA8XX PRUSS support"
+ depends on ARCH_DAVINCI && ARCH_DAVINCI_DA850
+ select MFD_CORE
+ help
+ This driver provides support api's for the programmable
+ realtime unit (PRU) present on TI's da8xx processors. It
+ provides basic read, write, config, enable, disable
+ routines to facilitate devices emulated on it.
+
config HTC_EGPIO
bool "HTC EGPIO support"
depends on GENERIC_HARDIRQS && GPIOLIB && ARM
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index a54e2c7..670d6b0 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_HTC_PASIC3) += htc-pasic3.o
obj-$(CONFIG_HTC_I2CPLD) += htc-i2cpld.o
obj-$(CONFIG_MFD_DAVINCI_VOICECODEC) += davinci_voicecodec.o
+obj-$(CONFIG_MFD_DA8XX_PRUSS) += da8xx_pru.o
obj-$(CONFIG_MFD_DM355EVM_MSP) += dm355evm_msp.o
obj-$(CONFIG_MFD_STMPE) += stmpe.o
diff --git a/drivers/mfd/da8xx_pru.c b/drivers/mfd/da8xx_pru.c
new file mode 100644
index 0000000..f7868a4
--- /dev/null
+++ b/drivers/mfd/da8xx_pru.c
@@ -0,0 +1,446 @@
+/*
+ * Copyright (C) 2010 Texas Instruments Incorporated
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
+ * whether express or implied; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/module.h>
+#include <linux/bitops.h>
+#include <linux/interrupt.h>
+#include <linux/errno.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/mfd/pruss/da8xx_prucore.h>
+#include <linux/mfd/pruss/da8xx_pru.h>
+#include <linux/mfd/core.h>
+#include <linux/io.h>
+#include <mach/da8xx.h>
+
+struct da8xx_pruss {
+ struct device *dev;
+ struct resource *res;
+ struct clk *clk;
+ u32 clk_freq;
+ void __iomem *ioaddr;
+};
+
+u32 pruss_get_clk_freq(struct device *dev)
+{
+ struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
+
+ return pruss->clk_freq;
+}
+EXPORT_SYMBOL(pruss_get_clk_freq);
+
+u32 pruss_disable(struct device *dev, u8 pruss_num)
+{
+ struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
+ da8xx_prusscore_regs h_pruss;
+ u32 temp_reg;
+
+ if (pruss_num == DA8XX_PRUCORE_0) {
+ /* Disable PRU0 */
+ h_pruss = (da8xx_prusscore_regs)
+ ((u32) pruss->ioaddr + 0x7000);
+
+ temp_reg = __raw_readl(&h_pruss->CONTROL);
+ temp_reg = (temp_reg &
+ ~DA8XX_PRUCORE_CONTROL_COUNTENABLE_MASK) |
+ ((DA8XX_PRUCORE_CONTROL_COUNTENABLE_DISABLE <<
+ DA8XX_PRUCORE_CONTROL_COUNTENABLE_SHIFT) &
+ DA8XX_PRUCORE_CONTROL_COUNTENABLE_MASK);
+ __raw_writel(temp_reg, &h_pruss->CONTROL);
+
+ temp_reg = __raw_readl(&h_pruss->CONTROL);
+ temp_reg = (temp_reg &
+ ~DA8XX_PRUCORE_CONTROL_ENABLE_MASK) |
+ ((DA8XX_PRUCORE_CONTROL_ENABLE_DISABLE <<
+ DA8XX_PRUCORE_CONTROL_ENABLE_SHIFT) &
+ DA8XX_PRUCORE_CONTROL_ENABLE_MASK);
+ __raw_writel(temp_reg, &h_pruss->CONTROL);
+
+ /* Reset PRU0 */
+ __raw_writel(DA8XX_PRUCORE_CONTROL_RESETVAL,
+ &h_pruss->CONTROL);
+ } else if (pruss_num == DA8XX_PRUCORE_1) {
+ /* Disable PRU1 */
+ h_pruss = (da8xx_prusscore_regs)
+ ((u32) pruss->ioaddr + 0x7800);
+ temp_reg = __raw_readl(&h_pruss->CONTROL);
+ temp_reg = (temp_reg &
+ ~DA8XX_PRUCORE_CONTROL_COUNTENABLE_MASK) |
+ ((DA8XX_PRUCORE_CONTROL_COUNTENABLE_DISABLE <<
+ DA8XX_PRUCORE_CONTROL_COUNTENABLE_SHIFT) &
+ DA8XX_PRUCORE_CONTROL_COUNTENABLE_MASK);
+ __raw_writel(temp_reg, &h_pruss->CONTROL);
+
+ temp_reg = __raw_readl(&h_pruss->CONTROL);
+ temp_reg = (temp_reg &
+ ~DA8XX_PRUCORE_CONTROL_ENABLE_MASK) |
+ ((DA8XX_PRUCORE_CONTROL_ENABLE_DISABLE <<
+ DA8XX_PRUCORE_CONTROL_ENABLE_SHIFT) &
+ DA8XX_PRUCORE_CONTROL_ENABLE_MASK);
+ __raw_writel(temp_reg, &h_pruss->CONTROL);
+
+ /* Reset PRU1 */
+ __raw_writel(DA8XX_PRUCORE_CONTROL_RESETVAL, &h_pruss->CONTROL);
+ } else
+ return -EINVAL;
+
+ return 0;
+}
+EXPORT_SYMBOL(pruss_disable);
+
+u32 pruss_enable(struct device *dev, u8 pruss_num)
+{
+ struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
+ da8xx_prusscore_regs h_pruss;
+
+ if (pruss_num == DA8XX_PRUCORE_0) {
+ /* Reset PRU0 */
+ h_pruss = (da8xx_prusscore_regs)
+ ((u32) pruss->ioaddr + 0x7000);
+ __raw_writel(DA8XX_PRUCORE_CONTROL_RESETVAL,
+ &h_pruss->CONTROL);
+ } else if (pruss_num == DA8XX_PRUCORE_1) {
+ /* Reset PRU1 */
+ h_pruss = (da8xx_prusscore_regs)
+ ((u32) pruss->ioaddr + 0x7800);
+ __raw_writel(DA8XX_PRUCORE_CONTROL_RESETVAL,
+ &h_pruss->CONTROL);
+ } else
+ return -EINVAL;
+
+ return 0;
+}
+EXPORT_SYMBOL(pruss_enable);
+
+/* Load the specified PRU with code */
+u32 pruss_load(struct device *dev, u8 pruss_num,
+ u32 *pruss_code, u32 code_size_in_words)
+{
+ struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
+ u32 *pruss_iram;
+ u32 i;
+
+ if (pruss_num == DA8XX_PRUCORE_0) {
+ pruss_iram = (u32 *) ((u32) pruss->ioaddr + 0x8000);
+ } else if (pruss_num == DA8XX_PRUCORE_1) {
+ pruss_iram = (u32 *) ((u32) pruss->ioaddr + 0xc000);
+ } else
+ return -EINVAL;
+
+ pruss_enable(dev, pruss_num);
+
+ /* Copy dMAX code to its instruction RAM */
+ for (i = 0; i < code_size_in_words; i++) {
+ __raw_writel(pruss_code[i], (pruss_iram + i));
+ }
+ return 0;
+}
+EXPORT_SYMBOL(pruss_load);
+
+u32 pruss_run(struct device *dev, u8 pruss_num)
+{
+ struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
+ da8xx_prusscore_regs h_pruss;
+
+ u32 temp_reg;
+
+ if (pruss_num == DA8XX_PRUCORE_0) {
+ /* DA8XX_PRUCORE_0_REGS; */
+ h_pruss = (da8xx_prusscore_regs)
+ ((u32) pruss->ioaddr + 0x7000);
+ } else if (pruss_num == DA8XX_PRUCORE_1) {
+ /* DA8XX_PRUCORE_1_REGS; */
+ h_pruss = (da8xx_prusscore_regs)
+ ((u32) pruss->ioaddr + 0x7800);
+ } else
+ return -EINVAL;
+
+ /* Enable dMAX, let it execute the code we just copied */
+ temp_reg = __raw_readl(&h_pruss->CONTROL);
+ temp_reg = (temp_reg &
+ ~DA8XX_PRUCORE_CONTROL_COUNTENABLE_MASK) |
+ ((DA8XX_PRUCORE_CONTROL_COUNTENABLE_ENABLE <<
+ DA8XX_PRUCORE_CONTROL_COUNTENABLE_SHIFT) &
+ DA8XX_PRUCORE_CONTROL_COUNTENABLE_MASK);
+ __raw_writel(temp_reg, &h_pruss->CONTROL);
+
+ temp_reg = __raw_readl(&h_pruss->CONTROL);
+ temp_reg = (temp_reg &
+ ~DA8XX_PRUCORE_CONTROL_ENABLE_MASK) |
+ ((DA8XX_PRUCORE_CONTROL_ENABLE_ENABLE <<
+ DA8XX_PRUCORE_CONTROL_ENABLE_SHIFT) &
+ DA8XX_PRUCORE_CONTROL_ENABLE_MASK);
+ __raw_writel(temp_reg, &h_pruss->CONTROL);
+ return 0;
+}
+EXPORT_SYMBOL(pruss_run);
+
+u32 pruss_wait_for_halt(struct device *dev, u8 pruss_num, u32 timeout)
+{
+ struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
+ da8xx_prusscore_regs h_pruss;
+ u32 temp_reg;
+ u32 cnt = timeout;
+
+ if (pruss_num == DA8XX_PRUCORE_0) {
+ /* DA8XX_PRUCORE_0_REGS; */
+ h_pruss = (da8xx_prusscore_regs)
+ ((u32) pruss->ioaddr + 0x7000);
+ } else if (pruss_num == DA8XX_PRUCORE_1) {
+ /* DA8XX_PRUCORE_1_REGS; */
+ h_pruss = (da8xx_prusscore_regs)
+ ((u32) pruss->ioaddr + 0x7800);
+ } else
+ return -EINVAL;
+
+ while (cnt--) {
+ temp_reg = __raw_readl(&h_pruss->CONTROL);
+ if (((temp_reg & DA8XX_PRUCORE_CONTROL_RUNSTATE_MASK) >>
+ DA8XX_PRUCORE_CONTROL_RUNSTATE_SHIFT) ==
+ DA8XX_PRUCORE_CONTROL_RUNSTATE_HALT)
+ break;
+ }
+ if (cnt == 0)
+ return -EBUSY;
+
+ return 0;
+}
+EXPORT_SYMBOL(pruss_wait_for_halt);
+
+s16 pruss_writeb(struct device *dev, u32 u32offset,
+ u8 *pu8datatowrite, u16 u16bytestowrite)
+{
+ struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
+ u8 *pu8addresstowrite;
+ u16 u16loop;
+ u32offset = (u32)pruss->ioaddr + u32offset;
+ pu8addresstowrite = (u8 *) (u32offset);
+
+ for (u16loop = 0; u16loop < u16bytestowrite; u16loop++)
+ __raw_writeb(*pu8datatowrite++, pu8addresstowrite++);
+ return 0;
+}
+EXPORT_SYMBOL(pruss_writeb);
+
+s16 pruss_readb(struct device *dev, u32 u32offset,
+ u8 *pu8datatoread, u16 u16bytestoread)
+{
+ struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
+ u8 *pu8addresstoread;
+ u16 u16loop;
+ u32offset = (u32)pruss->ioaddr + u32offset;
+ pu8addresstoread = (u8 *) (u32offset);
+
+ for (u16loop = 0; u16loop < u16bytestoread; u16loop++)
+ *pu8datatoread++ = __raw_readb(pu8addresstoread++);
+ return 0;
+}
+EXPORT_SYMBOL(pruss_readb);
+
+s16 pruss_writel(struct device *dev, u32 u32offset,
+ u32 *pu32datatowrite, s16 u16wordstowrite)
+{
+ struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
+ u32 *pu32addresstowrite;
+ s16 u16loop;
+
+ /* TODO: Get all the driver API's fixed */
+ u32offset = (u32)pruss->ioaddr + u32offset;
+ pu32addresstowrite = (u32 *)(u32offset);
+
+ for (u16loop = 0; u16loop < u16wordstowrite; u16loop++)
+ __raw_writel(*pu32datatowrite++, pu32addresstowrite++);
+ return 0;
+}
+EXPORT_SYMBOL(pruss_writel);
+
+s16 pruss_readl(struct device *dev, u32 u32offset,
+ u32 *pu32datatoread, s16 u16wordstoread)
+{
+ struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
+ u32 *pu32addresstoread;
+ s16 u16loop;
+
+ /* TODO: Get all the driver API's fixed */
+ u32offset = (u32)pruss->ioaddr + u32offset;
+ pu32addresstoread = (u32 *)(u32offset);
+
+ for (u16loop = 0; u16loop < u16wordstoread; u16loop++)
+ *pu32datatoread++ = __raw_readl(pu32addresstoread++);
+ return 0;
+}
+EXPORT_SYMBOL(pruss_readl);
+
+s16 pruss_writew(struct device *dev, u32 u32offset,
+ u16 *pu16datatowrite, s16 u16wordstowrite)
+{
+ struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
+ u32 *pu32addresstowrite;
+ s16 u16loop;
+
+ /* TODO: Get all the driver API's fixed */
+ u32offset = (u32)pruss->ioaddr + u32offset;
+ pu32addresstowrite = (u32 *)(u32offset);
+
+ for (u16loop = 0; u16loop < u16wordstowrite; u16loop++) {
+ __raw_writew(*(pu16datatowrite++), (pu32addresstowrite++));
+ }
+ return 0;
+}
+EXPORT_SYMBOL(pruss_writew);
+
+s16 pruss_readw(struct device *dev, u32 u32offset,
+ u16 *pu16datatoread, s16 u16wordstoread)
+{
+ struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
+ u32 *pu32addresstoread;
+ s16 u16loop;
+
+ /* TODO: Get all the driver API's fixed */
+ u32offset = (u32)pruss->ioaddr + u32offset;
+ pu32addresstoread = (u32 *)(u32offset);
+
+ for (u16loop = 0; u16loop < u16wordstoread; u16loop++)
+ *pu16datatoread++ = __raw_readw(pu32addresstoread++);
+ return 0;
+}
+EXPORT_SYMBOL(pruss_readw);
+
+static int pruss_mfd_add_devices(struct platform_device *pdev)
+{
+ struct da8xx_pruss_devices *dev_data = pdev->dev.platform_data;
+ struct device *dev = &pdev->dev;
+ struct mfd_cell cell;
+ u32 err, count;
+
+ for (count = 0; (dev_data + count)->dev_name != NULL; count++) {
+ memset(&cell, 0, sizeof(struct mfd_cell));
+ cell.id = count;
+ cell.name = (dev_data + count)->dev_name;
+ cell.platform_data = (dev_data + count)->pdata;
+ cell.data_size = (dev_data + count)->pdata_size;
+
+ err = mfd_add_devices(dev, 0, &cell, 1, NULL, 0);
+ if (err) {
+ dev_err(dev, "cannot add mfd cells\n");
+ return err;
+ }
+ }
+ return err;
+}
+
+static int __devinit da8xx_pruss_probe(struct platform_device *pdev)
+{
+ struct da8xx_pruss *pruss_dev = NULL;
+ u32 err;
+
+ pruss_dev = kzalloc(sizeof(struct da8xx_pruss), GFP_KERNEL);
+ if (!pruss_dev)
+ return -ENOMEM;
+
+ pruss_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!pruss_dev->res) {
+ dev_err(&pdev->dev,
+ "unable to get pruss memory resources!\n");
+ err = -ENODEV;
+ goto probe_exit_kfree;
+ }
+
+ if (!request_mem_region(pruss_dev->res->start, resource_size(pruss_dev->res),
+ dev_name(&pdev->dev))) {
+ dev_err(&pdev->dev, "pruss memory region already claimed!\n");
+ err = -EBUSY;
+ goto probe_exit_kfree;
+ }
+
+ pruss_dev->ioaddr = ioremap(pruss_dev->res->start,
+ resource_size(pruss_dev->res));
+ if (!pruss_dev->ioaddr) {
+ dev_err(&pdev->dev, "ioremap failed\n");
+ err = -ENOMEM;
+ goto probe_exit_free_region;
+ }
+
+ pruss_dev->clk = clk_get(NULL, "pruss");
+ if (IS_ERR(pruss_dev->clk)) {
+ dev_err(&pdev->dev, "no clock available: pruss\n");
+ err = -ENODEV;
+ pruss_dev->clk = NULL;
+ goto probe_exit_iounmap;
+ }
+
+ clk_enable(pruss_dev->clk);
+ pruss_dev->clk_freq = clk_get_rate(pruss_dev->clk);
+
+ err = pruss_mfd_add_devices(pdev);
+ if (err)
+ goto probe_exit_clock;
+
+ platform_set_drvdata(pdev, pruss_dev);
+ pruss_dev->dev = &pdev->dev;
+ return 0;
+
+probe_exit_clock:
+ clk_put(pruss_dev->clk);
+ clk_disable(pruss_dev->clk);
+probe_exit_iounmap:
+ iounmap(pruss_dev->ioaddr);
+probe_exit_free_region:
+ release_mem_region(pruss_dev->res->start, resource_size(pruss_dev->res));
+probe_exit_kfree:
+ kfree(pruss_dev);
+ return err;
+}
+
+static int __devexit da8xx_pruss_remove(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct da8xx_pruss *pruss = dev_get_drvdata(dev);
+
+ mfd_remove_devices(dev);
+ clk_disable(pruss->clk);
+ clk_put(pruss->clk);
+ iounmap(pruss->ioaddr);
+ release_mem_region(pruss->res->start, resource_size(pruss->res));
+ kfree(pruss);
+ dev_set_drvdata(dev, NULL);
+ return 0;
+}
+
+static struct platform_driver da8xx_pruss_driver = {
+ .probe = da8xx_pruss_probe,
+ .remove = __devexit_p(da8xx_pruss_remove),
+ .driver = {
+ .name = "da8xx_pruss",
+ .owner = THIS_MODULE,
+ }
+};
+
+static int __init da8xx_pruss_init(void)
+{
+ return platform_driver_register(&da8xx_pruss_driver);
+}
+module_init(da8xx_pruss_init);
+
+static void __exit da8xx_pruss_exit(void)
+{
+ platform_driver_unregister(&da8xx_pruss_driver);
+}
+module_exit(da8xx_pruss_exit);
+
+MODULE_DESCRIPTION("Programmable Realtime Unit (PRU) Driver");
+MODULE_AUTHOR("Subhasish Ghosh");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/pruss/da8xx_pru.h b/include/linux/mfd/pruss/da8xx_pru.h
new file mode 100644
index 0000000..68d8421
--- /dev/null
+++ b/include/linux/mfd/pruss/da8xx_pru.h
@@ -0,0 +1,122 @@
+/*
+ * Copyright (C) 2010 Texas Instruments Incorporated
+ * Author: Jitendra Kumar <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
+ * whether express or implied; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ */
+
+#ifndef _PRUSS_H_
+#define _PRUSS_H_
+
+#include <linux/types.h>
+#include <linux/platform_device.h>
+#include "da8xx_prucore.h"
+
+#define PRUSS_NUM0 DA8XX_PRUCORE_0
+#define PRUSS_NUM1 DA8XX_PRUCORE_1
+
+#define PRUSS_PRU0_BASE_ADDRESS 0
+#define PRUSS_INTC_BASE_ADDRESS (PRUSS_PRU0_BASE_ADDRESS + 0x4000)
+#define PRUSS_INTC_GLBLEN (PRUSS_INTC_BASE_ADDRESS + 0x10)
+#define PRUSS_INTC_GLBLNSTLVL (PRUSS_INTC_BASE_ADDRESS + 0x1C)
+#define PRUSS_INTC_STATIDXSET (PRUSS_INTC_BASE_ADDRESS + 0x20)
+#define PRUSS_INTC_STATIDXCLR (PRUSS_INTC_BASE_ADDRESS + 0x24)
+#define PRUSS_INTC_ENIDXSET (PRUSS_INTC_BASE_ADDRESS + 0x28)
+#define PRUSS_INTC_ENIDXCLR (PRUSS_INTC_BASE_ADDRESS + 0x2C)
+#define PRUSS_INTC_HSTINTENIDXSET (PRUSS_INTC_BASE_ADDRESS + 0x34)
+#define PRUSS_INTC_HSTINTENIDXCLR (PRUSS_INTC_BASE_ADDRESS + 0x38)
+#define PRUSS_INTC_GLBLPRIIDX (PRUSS_INTC_BASE_ADDRESS + 0x80)
+#define PRUSS_INTC_STATSETINT0 (PRUSS_INTC_BASE_ADDRESS + 0x200)
+#define PRUSS_INTC_STATSETINT1 (PRUSS_INTC_BASE_ADDRESS + 0x204)
+#define PRUSS_INTC_STATCLRINT0 (PRUSS_INTC_BASE_ADDRESS + 0x280)
+#define PRUSS_INTC_STATCLRINT1 (PRUSS_INTC_BASE_ADDRESS + 0x284)
+#define PRUSS_INTC_ENABLESET0 (PRUSS_INTC_BASE_ADDRESS + 0x300)
+#define PRUSS_INTC_ENABLESET1 (PRUSS_INTC_BASE_ADDRESS + 0x304)
+#define PRUSS_INTC_ENABLECLR0 (PRUSS_INTC_BASE_ADDRESS + 0x380)
+#define PRUSS_INTC_ENABLECLR1 (PRUSS_INTC_BASE_ADDRESS + 0x384)
+#define PRUSS_INTC_CHANMAP0 (PRUSS_INTC_BASE_ADDRESS + 0x400)
+#define PRUSS_INTC_CHANMAP1 (PRUSS_INTC_BASE_ADDRESS + 0x404)
+#define PRUSS_INTC_CHANMAP2 (PRUSS_INTC_BASE_ADDRESS + 0x408)
+#define PRUSS_INTC_CHANMAP3 (PRUSS_INTC_BASE_ADDRESS + 0x40C)
+#define PRUSS_INTC_CHANMAP4 (PRUSS_INTC_BASE_ADDRESS + 0x410)
+#define PRUSS_INTC_CHANMAP5 (PRUSS_INTC_BASE_ADDRESS + 0x414)
+#define PRUSS_INTC_CHANMAP6 (PRUSS_INTC_BASE_ADDRESS + 0x418)
+#define PRUSS_INTC_CHANMAP7 (PRUSS_INTC_BASE_ADDRESS + 0x41C)
+#define PRUSS_INTC_CHANMAP8 (PRUSS_INTC_BASE_ADDRESS + 0x420)
+#define PRUSS_INTC_CHANMAP9 (PRUSS_INTC_BASE_ADDRESS + 0x424)
+#define PRUSS_INTC_CHANMAP10 (PRUSS_INTC_BASE_ADDRESS + 0x428)
+#define PRUSS_INTC_CHANMAP11 (PRUSS_INTC_BASE_ADDRESS + 0x42C)
+#define PRUSS_INTC_CHANMAP12 (PRUSS_INTC_BASE_ADDRESS + 0x430)
+#define PRUSS_INTC_CHANMAP13 (PRUSS_INTC_BASE_ADDRESS + 0x434)
+#define PRUSS_INTC_CHANMAP14 (PRUSS_INTC_BASE_ADDRESS + 0x438)
+#define PRUSS_INTC_CHANMAP15 (PRUSS_INTC_BASE_ADDRESS + 0x43C)
+#define PRUSS_INTC_HOSTMAP0 (PRUSS_INTC_BASE_ADDRESS + 0x800)
+#define PRUSS_INTC_HOSTMAP1 (PRUSS_INTC_BASE_ADDRESS + 0x804)
+#define PRUSS_INTC_HOSTMAP2 (PRUSS_INTC_BASE_ADDRESS + 0x808)
+#define PRUSS_INTC_POLARITY0 (PRUSS_INTC_BASE_ADDRESS + 0xD00)
+#define PRUSS_INTC_POLARITY1 (PRUSS_INTC_BASE_ADDRESS + 0xD04)
+#define PRUSS_INTC_TYPE0 (PRUSS_INTC_BASE_ADDRESS + 0xD80)
+#define PRUSS_INTC_TYPE1 (PRUSS_INTC_BASE_ADDRESS + 0xD84)
+#define PRUSS_INTC_HOSTINTEN (PRUSS_INTC_BASE_ADDRESS + 0x1500)
+#define PRUSS_INTC_HOSTINTLVL_MAX 9
+
+#define PRU_INTC_CHAN_123_HOST (0x03020100)
+#define PRU_INTC_CHAN_4567_HOST (0x07060504)
+#define PRU_INTC_CHAN_89_HOST (0x00000908)
+
+#define PRU_INTC_CHAN_0_SYSEVT_31 (0x00000000)
+#define PRU_INTC_CHAN_12_SYSEVT (0x02020100)
+#define PRU_INTC_CHAN_34_SYSEVT_36_39 (0x04040303)
+#define PRU_INTC_CHAN_56_SYSEVT_40_43 (0x06060505)
+#define PRU_INTC_CHAN_78_SYSEVT_44_47 (0x08080707)
+#define PRU_INTC_CHAN_9_SYSEVT_48_49 (0x00010909)
+#define PRU_INTC_CHAN_0123_SYSEVT_32_35 (0x03020100)
+#define PRU_INTC_CHAN_4567_SYSEVT_36_39 (0x07060504)
+#define PRU_INTC_CHAN_8923_SYSEVT_40_43 (0x03020908)
+#define PRU_INTC_CHAN_4567_SYSEVT_44_47 (0x07060504)
+
+struct da8xx_pruss_devices {
+ const char *dev_name;
+ void *pdata;
+ size_t pdata_size;
+ int (*setup)(void);
+};
+
+u32 pruss_get_clk_freq(struct device *dev);
+
+u32 pruss_enable(struct device *dev, u8 pruss_num);
+
+u32 pruss_load(struct device *dev, u8 pruss_num, u32 *pruss_code,
+ u32 code_size_in_words);
+
+u32 pruss_run(struct device *dev, u8 pruss_num);
+
+u32 pruss_wait_for_halt(struct device *dev, u8 pruss_num, u32 timeout);
+
+u32 pruss_disable(struct device *dev, u8 pruss_num);
+
+s16 pruss_writeb(struct device *dev, u32 u32offset,
+ u8 *pu8datatowrite, u16 u16wordstowrite);
+
+s16 pruss_readb(struct device *dev, u32 u32offset,
+ u8 *pu8datatoread, u16 u16wordstoread);
+
+s16 pruss_readl(struct device *dev, u32 u32offset,
+ u32 *pu32datatoread, s16 s16wordstoread);
+
+s16 pruss_writel(struct device *dev, u32 u32offset,
+ u32 *pu32datatoread, s16 s16wordstoread);
+
+s16 pruss_writew(struct device *dev, u32 u32offset,
+ u16 *u16datatowrite, s16 u16wordstowrite);
+
+s16 pruss_readw(struct device *dev, u32 u32offset,
+ u16 *pu32datatoread, s16 u16wordstoread);
+#endif /* End _PRUSS_H_ */
diff --git a/include/linux/mfd/pruss/da8xx_prucore.h b/include/linux/mfd/pruss/da8xx_prucore.h
new file mode 100644
index 0000000..81f2ff9
--- /dev/null
+++ b/include/linux/mfd/pruss/da8xx_prucore.h
@@ -0,0 +1,74 @@
+/*
+ * Copyright (C) 2010 Texas Instruments Incorporated
+ * Author: Jitendra Kumar <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
+ * whether express or implied; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ */
+
+#ifndef _DA8XX_PRUCORE_H_
+#define _DA8XX_PRUCORE_H_
+
+#include <linux/types.h>
+
+#define DA8XX_PRUCORE_0 (0)
+#define DA8XX_PRUCORE_1 (1)
+
+#define DA8XX_PRUCORE_CONTROL_PCRESETVAL_MASK (0xFFFF0000u)
+#define DA8XX_PRUCORE_CONTROL_PCRESETVAL_SHIFT (0x00000010u)
+#define DA8XX_PRUCORE_CONTROL_PCRESETVAL_RESETVAL (0x00000000u)
+#define DA8XX_PRUCORE_CONTROL_RUNSTATE_MASK (0x00008000u)
+#define DA8XX_PRUCORE_CONTROL_RUNSTATE_SHIFT (0x0000000Fu)
+#define DA8XX_PRUCORE_CONTROL_RUNSTATE_RESETVAL (0x00000000u)
+#define DA8XX_PRUCORE_CONTROL_RUNSTATE_HALT (0x00000000u)
+#define DA8XX_PRUCORE_CONTROL_RUNSTATE_RUN (0x00000001u)
+#define DA8XX_PRUCORE_CONTROL_SINGLESTEP_MASK (0x00000100u)
+#define DA8XX_PRUCORE_CONTROL_SINGLESTEP_SHIFT (0x00000008u)
+#define DA8XX_PRUCORE_CONTROL_SINGLESTEP_RESETVAL (0x00000000u)
+#define DA8XX_PRUCORE_CONTROL_SINGLESTEP_FREERUN (0x00000000u)
+#define DA8XX_PRUCORE_CONTROL_SINGLESTEP_SINGLE (0x00000001u)
+#define DA8XX_PRUCORE_CONTROL_COUNTENABLE_MASK (0x00000008u)
+#define DA8XX_PRUCORE_CONTROL_COUNTENABLE_SHIFT (0x00000003u)
+#define DA8XX_PRUCORE_CONTROL_COUNTENABLE_RESETVAL (0x00000000u)
+#define DA8XX_PRUCORE_CONTROL_COUNTENABLE_DISABLE (0x00000000u)
+#define DA8XX_PRUCORE_CONTROL_COUNTENABLE_ENABLE (0x00000001u)
+#define DA8XX_PRUCORE_CONTROL_SLEEPING_MASK (0x00000004u)
+#define DA8XX_PRUCORE_CONTROL_SLEEPING_SHIFT (0x00000002u)
+#define DA8XX_PRUCORE_CONTROL_SLEEPING_RESETVAL (0x00000000u)
+#define DA8XX_PRUCORE_CONTROL_SLEEPING_NOTASLEEP (0x00000000u)
+#define DA8XX_PRUCORE_CONTROL_SLEEPING_ASLEEP (0x00000001u)
+#define DA8XX_PRUCORE_CONTROL_ENABLE_MASK (0x00000002u)
+#define DA8XX_PRUCORE_CONTROL_ENABLE_SHIFT (0x00000001u)
+#define DA8XX_PRUCORE_CONTROL_ENABLE_RESETVAL (0x00000000u)
+#define DA8XX_PRUCORE_CONTROL_ENABLE_DISABLE (0x00000000u)
+#define DA8XX_PRUCORE_CONTROL_ENABLE_ENABLE (0x00000001u)
+#define DA8XX_PRUCORE_CONTROL_SOFTRESET_MASK (0x00000001u)
+#define DA8XX_PRUCORE_CONTROL_SOFTRESET_SHIFT (0x00000000u)
+#define DA8XX_PRUCORE_CONTROL_SOFTRESET_RESETVAL (0x00000000u)
+#define DA8XX_PRUCORE_CONTROL_SOFTRESET_RESET (0x00000000u)
+#define DA8XX_PRUCORE_CONTROL_SOFTRESET_OUT_OF_RESET (0x00000001u)
+#define DA8XX_PRUCORE_CONTROL_RESETVAL (0x00000000u)
+
+typedef struct {
+ u32 CONTROL;
+ u32 STATUS;
+ u32 WAKEUP;
+ u32 CYCLECNT;
+ u32 STALLCNT;
+ u8 RSVD0[12];
+ u32 CONTABBLKIDX0;
+ u32 CONTABBLKIDX1;
+ u32 CONTABPROPTR0;
+ u32 CONTABPROPTR1;
+ u8 RSVD1[976];
+ u32 INTGPR[32];
+ u32 INTCTER[32];
+} *da8xx_prusscore_regs;
+
+#endif
--
1.7.2.3
Hi Subhasish,
On Fri, Feb 11, 2011 at 08:21:20PM +0530, Subhasish Ghosh wrote:
> This patch adds the pruss MFD driver and associated include files.
A more detailed changelog would be better. What is this device, why is it an
MFD and what are its potential subdevices ?
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index fd01836..6c437df 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -81,6 +81,16 @@ config MFD_DM355EVM_MSP
> boards. MSP430 firmware manages resets and power sequencing,
> inputs from buttons and the IR remote, LEDs, an RTC, and more.
>
> +config MFD_DA8XX_PRUSS
> + tristate "Texas Instruments DA8XX PRUSS support"
> + depends on ARCH_DAVINCI && ARCH_DAVINCI_DA850
Why are we depending on those ?
> + select MFD_CORE
> + help
> + This driver provides support api's for the programmable
> + realtime unit (PRU) present on TI's da8xx processors. It
> + provides basic read, write, config, enable, disable
> + routines to facilitate devices emulated on it.
Please fix your identation here.
> --- /dev/null
> +++ b/drivers/mfd/da8xx_pru.c
> @@ -0,0 +1,446 @@
> +/*
> + * Copyright (C) 2010 Texas Instruments Incorporated
s/2010/2011/ ?
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
> + * whether express or implied; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <linux/bitops.h>
> +#include <linux/interrupt.h>
> +#include <linux/errno.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/mfd/pruss/da8xx_prucore.h>
> +#include <linux/mfd/pruss/da8xx_pru.h>
> +#include <linux/mfd/core.h>
> +#include <linux/io.h>
> +#include <mach/da8xx.h>
Is that include needed ?
> +struct da8xx_pruss {
What is the "ss" suffix for ?
> +u32 pruss_disable(struct device *dev, u8 pruss_num)
> +{
> + struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
> + da8xx_prusscore_regs h_pruss;
> + u32 temp_reg;
> +
> + if (pruss_num == DA8XX_PRUCORE_0) {
> + /* Disable PRU0 */
> + h_pruss = (da8xx_prusscore_regs)
> + ((u32) pruss->ioaddr + 0x7000);
So it seems you're doing this in several places, and I have a few comments:
- You don't need the da8xx_prusscore_regs at all.
- Define the register map through a set of #define in your header file.
- Use a static routine that takes the core number and returns the register map
offset.
Then routines like this one will look a lot more readable.
> +s16 pruss_writeb(struct device *dev, u32 u32offset,
> + u8 *pu8datatowrite, u16 u16bytestowrite)
>From CodingStyle: "Encoding the type of a function into the name (so-called
Hungarian notation) is brain damaged"
Also, all your exported routines severely lack any sort of locking. An IO
mutex or spinlock is mandatory here.
> +static int pruss_mfd_add_devices(struct platform_device *pdev)
> +{
> + struct da8xx_pruss_devices *dev_data = pdev->dev.platform_data;
> + struct device *dev = &pdev->dev;
> + struct mfd_cell cell;
> + u32 err, count;
> +
> + for (count = 0; (dev_data + count)->dev_name != NULL; count++) {
> + memset(&cell, 0, sizeof(struct mfd_cell));
> + cell.id = count;
> + cell.name = (dev_data + count)->dev_name;
> + cell.platform_data = (dev_data + count)->pdata;
> + cell.data_size = (dev_data + count)->pdata_size;
> +
> + err = mfd_add_devices(dev, 0, &cell, 1, NULL, 0);
> + if (err) {
> + dev_err(dev, "cannot add mfd cells\n");
> + return err;
> + }
> + }
> + return err;
> +}
So, what are the potential subdevices for this driver ? If it's a really
dynamic setup, I'm fine with passing those as platform data but then do it so
that you pass a NULL terminated da8xx_pruss_devices array. That will avoid
most of the ugly casts you're doing here.
> diff --git a/include/linux/mfd/pruss/da8xx_pru.h b/include/linux/mfd/pruss/da8xx_pru.h
> new file mode 100644
> index 0000000..68d8421
> --- /dev/null
> +++ b/include/linux/mfd/pruss/da8xx_pru.h
> @@ -0,0 +1,122 @@
> +/*
> + * Copyright (C) 2010 Texas Instruments Incorporated
> + * Author: Jitendra Kumar <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
> + * whether express or implied; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +
> +#ifndef _PRUSS_H_
> +#define _PRUSS_H_
> +
> +#include <linux/types.h>
> +#include <linux/platform_device.h>
> +#include "da8xx_prucore.h"
> +
> +#define PRUSS_NUM0 DA8XX_PRUCORE_0
> +#define PRUSS_NUM1 DA8XX_PRUCORE_1
Those are unused.
> diff --git a/include/linux/mfd/pruss/da8xx_prucore.h b/include/linux/mfd/pruss/da8xx_prucore.h
> new file mode 100644
> index 0000000..81f2ff9
> --- /dev/null
> +++ b/include/linux/mfd/pruss/da8xx_prucore.h
Please rename your mfd include directory to include/linux/mfd/da8xx/, so that
one can match it with the drivers/mfd/da8xx driver code.
> +typedef struct {
> + u32 CONTROL;
> + u32 STATUS;
> + u32 WAKEUP;
> + u32 CYCLECNT;
> + u32 STALLCNT;
> + u8 RSVD0[12];
> + u32 CONTABBLKIDX0;
> + u32 CONTABBLKIDX1;
> + u32 CONTABPROPTR0;
> + u32 CONTABPROPTR1;
> + u8 RSVD1[976];
> + u32 INTGPR[32];
> + u32 INTCTER[32];
> +} *da8xx_prusscore_regs;
Again, we don't need that structure.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
Thank you for your comments.
--------------------------------------------------
From: "Samuel Ortiz" <[email protected]>
Sent: Monday, February 21, 2011 10:00 PM
To: "Subhasish Ghosh" <[email protected]>
Cc: <[email protected]>;
<[email protected]>; <[email protected]>;
<[email protected]>; <[email protected]>; "open list"
<[email protected]>
Subject: Re: [PATCH v2 01/13] mfd: pruss mfd driver.
> Hi Subhasish,
>
> On Fri, Feb 11, 2011 at 08:21:20PM +0530, Subhasish Ghosh wrote:
>> This patch adds the pruss MFD driver and associated include files.
> A more detailed changelog would be better. What is this device, why is it
> an
> MFD and what are its potential subdevices ?
>
SG -- Will add a more detailed change log.
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index fd01836..6c437df 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -81,6 +81,16 @@ config MFD_DM355EVM_MSP
>> boards. MSP430 firmware manages resets and power sequencing,
>> inputs from buttons and the IR remote, LEDs, an RTC, and more.
>>
>> +config MFD_DA8XX_PRUSS
>> + tristate "Texas Instruments DA8XX PRUSS support"
>> + depends on ARCH_DAVINCI && ARCH_DAVINCI_DA850
> Why are we depending on those ?
SG -- The PRUSS core in only available within DA850 and DA830,
DA830 support is not yet implemented.
>
>
>> + select MFD_CORE
>> + help
>> + This driver provides support api's for the programmable
>> + realtime unit (PRU) present on TI's da8xx processors. It
>> + provides basic read, write, config, enable, disable
>> + routines to facilitate devices emulated on it.
> Please fix your identation here.
SG -- Will do.
>
>> --- /dev/null
>> +++ b/drivers/mfd/da8xx_pru.c
>> @@ -0,0 +1,446 @@
>> +/*
>> + * Copyright (C) 2010 Texas Instruments Incorporated
> s/2010/2011/ ?
>
>> + * This program is free software; you can redistribute it and/or modify
>> it
>> + * under the terms of the GNU General Public License as published by
>> the
>> + * Free Software Foundation version 2.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
>> + * whether express or implied; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * General Public License for more details.
>> + */
>> +
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/types.h>
>> +#include <linux/module.h>
>> +#include <linux/bitops.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/errno.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/clk.h>
>> +#include <linux/mfd/pruss/da8xx_prucore.h>
>> +#include <linux/mfd/pruss/da8xx_pru.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/io.h>
>> +#include <mach/da8xx.h>
> Is that include needed ?
SG - Will remove if not needed.
>
>
>> +struct da8xx_pruss {
> What is the "ss" suffix for ?
SG -- We call it the PRU Sub-System.
>
>
>> +u32 pruss_disable(struct device *dev, u8 pruss_num)
>> +{
>> + struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
>> + da8xx_prusscore_regs h_pruss;
>> + u32 temp_reg;
>> +
>> + if (pruss_num == DA8XX_PRUCORE_0) {
>> + /* Disable PRU0 */
>> + h_pruss = (da8xx_prusscore_regs)
>> + ((u32) pruss->ioaddr + 0x7000);
> So it seems you're doing this in several places, and I have a few
> comments:
>
> - You don't need the da8xx_prusscore_regs at all.
> - Define the register map through a set of #define in your header file.
> - Use a static routine that takes the core number and returns the register
> map
> offset.
>
> Then routines like this one will look a lot more readable.
SG -- There are a huge number of PRUSS registers. A lot of them are reserved
and are expected to change as development on the
controller is still ongoing. If we use #defines to plot all the
registers, then first, there are too many array type registers which will
need to be duplicated.
And if the offset of one of the macro changes in between, we
will require to adjust all the rest of the macros. So I felt like a
structure was a better option here.
I also named the structure elements as per the register names.
so why should it be less readable ? But, we can definitely use macros to
define the PRUSS0 & 1
offsets instead of the magic numbers as above.
>
>> +s16 pruss_writeb(struct device *dev, u32 u32offset,
>> + u8 *pu8datatowrite, u16 u16bytestowrite)
> From CodingStyle: "Encoding the type of a function into the name
> (so-called
> Hungarian notation) is brain damaged"
SG -- Will change.
>
> Also, all your exported routines severely lack any sort of locking. An IO
> mutex or spinlock is mandatory here.
SG - As per our current implementation, we do not have two devices running
simultaneously on the PRU,
so we do not have any way to test it. We have kept this as an
enhancement if request comes in for
multiple devices.
>
>> +static int pruss_mfd_add_devices(struct platform_device *pdev)
>> +{
>> + struct da8xx_pruss_devices *dev_data = pdev->dev.platform_data;
>> + struct device *dev = &pdev->dev;
>> + struct mfd_cell cell;
>> + u32 err, count;
>> +
>> + for (count = 0; (dev_data + count)->dev_name != NULL; count++) {
>> + memset(&cell, 0, sizeof(struct mfd_cell));
>> + cell.id = count;
>> + cell.name = (dev_data + count)->dev_name;
>> + cell.platform_data = (dev_data + count)->pdata;
>> + cell.data_size = (dev_data + count)->pdata_size;
>> +
>> + err = mfd_add_devices(dev, 0, &cell, 1, NULL, 0);
>> + if (err) {
>> + dev_err(dev, "cannot add mfd cells\n");
>> + return err;
>> + }
>> + }
>> + return err;
>> +}
> So, what are the potential subdevices for this driver ? If it's a really
> dynamic setup, I'm fine with passing those as platform data but then do it
> so
> that you pass a NULL terminated da8xx_pruss_devices array. That will avoid
> most of the ugly casts you're doing here.
SG -- I did not follow your recommendations here, could you please
elaborate.
I am already checking the dev_name for a NULL.
This device is basically a microcontroller within DA850, so
basically any device or protocol can be
emulated on it. Currently, we have emulated 8 UARTS using the
two PRUs and also a CAN device.
>
>> diff --git a/include/linux/mfd/pruss/da8xx_pru.h
>> b/include/linux/mfd/pruss/da8xx_pru.h
>> new file mode 100644
>> index 0000000..68d8421
>> --- /dev/null
>> +++ b/include/linux/mfd/pruss/da8xx_pru.h
>> @@ -0,0 +1,122 @@
>> +/*
>> + * Copyright (C) 2010 Texas Instruments Incorporated
>> + * Author: Jitendra Kumar <[email protected]>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> it
>> + * under the terms of the GNU General Public License as published by
>> the
>> + * Free Software Foundation version 2.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
>> + * whether express or implied; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * General Public License for more details.
>> + */
>> +
>> +#ifndef _PRUSS_H_
>> +#define _PRUSS_H_
>> +
>> +#include <linux/types.h>
>> +#include <linux/platform_device.h>
>> +#include "da8xx_prucore.h"
>> +
>> +#define PRUSS_NUM0 DA8XX_PRUCORE_0
>> +#define PRUSS_NUM1 DA8XX_PRUCORE_1
> Those are unused.
SG - These are getting used by the CAN & UART drivers.
>
>> diff --git a/include/linux/mfd/pruss/da8xx_prucore.h
>> b/include/linux/mfd/pruss/da8xx_prucore.h
>> new file mode 100644
>> index 0000000..81f2ff9
>> --- /dev/null
>> +++ b/include/linux/mfd/pruss/da8xx_prucore.h
> Please rename your mfd include directory to include/linux/mfd/da8xx/, so
> that
> one can match it with the drivers/mfd/da8xx driver code.
SG - Will do.
>
>
>> +typedef struct {
>> + u32 CONTROL;
>> + u32 STATUS;
>> + u32 WAKEUP;
>> + u32 CYCLECNT;
>> + u32 STALLCNT;
>> + u8 RSVD0[12];
>> + u32 CONTABBLKIDX0;
>> + u32 CONTABBLKIDX1;
>> + u32 CONTABPROPTR0;
>> + u32 CONTABPROPTR1;
>> + u8 RSVD1[976];
>> + u32 INTGPR[32];
>> + u32 INTCTER[32];
>> +} *da8xx_prusscore_regs;
> Again, we don't need that structure.
SG - As mentioned above.
>
> Cheers,
> Samuel.
>
>
> --
> Intel Open Source Technology Centre
> http://oss.intel.com/
Hi Subhasish,
On Tue, Feb 22, 2011 at 11:13:38AM +0530, Subhasish Ghosh wrote:
> Thank you for your comments.
No problem.
> >>diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> >>index fd01836..6c437df 100644
> >>--- a/drivers/mfd/Kconfig
> >>+++ b/drivers/mfd/Kconfig
> >>@@ -81,6 +81,16 @@ config MFD_DM355EVM_MSP
> >> boards. MSP430 firmware manages resets and power sequencing,
> >> inputs from buttons and the IR remote, LEDs, an RTC, and more.
> >>
> >>+config MFD_DA8XX_PRUSS
> >>+ tristate "Texas Instruments DA8XX PRUSS support"
> >>+ depends on ARCH_DAVINCI && ARCH_DAVINCI_DA850
> >Why are we depending on those ?
>
> SG -- The PRUSS core in only available within DA850 and DA830,
> DA830 support is not yet implemented.
Sure, but if there are no actual code dependencies, I'd like to get rid of
those depends.
> >>+u32 pruss_disable(struct device *dev, u8 pruss_num)
> >>+{
> >>+ struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
> >>+ da8xx_prusscore_regs h_pruss;
> >>+ u32 temp_reg;
> >>+
> >>+ if (pruss_num == DA8XX_PRUCORE_0) {
> >>+ /* Disable PRU0 */
> >>+ h_pruss = (da8xx_prusscore_regs)
> >>+ ((u32) pruss->ioaddr + 0x7000);
> >So it seems you're doing this in several places, and I have a few
> >comments:
> >
> >- You don't need the da8xx_prusscore_regs at all.
> >- Define the register map through a set of #define in your header file.
> >- Use a static routine that takes the core number and returns the
> >register map
> >offset.
> >
> >Then routines like this one will look a lot more readable.
>
> SG -- There are a huge number of PRUSS registers. A lot of them are
> reserved and are expected to change as development on the
> controller is still ongoing.
First of all, from what I read in your patch you're only using the CONTROL
offset.
> If we use #defines to plot
> all the registers, then first, there are too many array type
> registers which will need to be duplicated.
What I'm expecting is a small set of defines for the register offsets. You
have 13 fields in your da8xx_prusscore_regs, you only need to define 13
register offsets.
So, if you have a:
static u32 reg_offset(struct device *dev, u8 pru_num)
{
struct da8xx_pruss *pru = dev_get_drvdata(dev->parent);
switch (pru_num) {
case DA8XX_PRUCORE_0:
return (u32) pru->ioaddr + 0x7000;
case DA8XX_PRUCORE_1:
return (u32) pru->ioaddr + 0x7800;
default:
return 0;
}
then routines like pruss_enable (which should return an int, btw) would look
like:
int pruss_enable(struct device *dev, u8 pruss_num)
{
u32 offset = reg_offset(dev, pruss_num);
if (offset == 0)
return -EINVAL;
__raw_writel(DA8XX_PRUCORE_CONTROL_RESETVAL,
offset + PRU_CORE_CONTROL);
return 0;
}
> >Also, all your exported routines severely lack any sort of locking. An IO
> >mutex or spinlock is mandatory here.
>
> SG - As per our current implementation, we do not have two devices
> running simultaneously on the PRU,
> so we do not have any way to test it. We have kept this as an
> enhancement if request comes in for
> multiple devices.
It's not about having multiple devices at the same time, it's about having
multiple callers writing and reading to the same registers. Since you're
exporting all your I/O routines you have no way to prevent 2 drivers from
writing to the same register at the "same" time. You need locking here,
regardless of the number of devices that you can have on a system.
> >>+static int pruss_mfd_add_devices(struct platform_device *pdev)
> >>+{
> >>+ struct da8xx_pruss_devices *dev_data = pdev->dev.platform_data;
> >>+ struct device *dev = &pdev->dev;
> >>+ struct mfd_cell cell;
> >>+ u32 err, count;
> >>+
> >>+ for (count = 0; (dev_data + count)->dev_name != NULL; count++) {
> >>+ memset(&cell, 0, sizeof(struct mfd_cell));
> >>+ cell.id = count;
> >>+ cell.name = (dev_data + count)->dev_name;
> >>+ cell.platform_data = (dev_data + count)->pdata;
> >>+ cell.data_size = (dev_data + count)->pdata_size;
> >>+
> >>+ err = mfd_add_devices(dev, 0, &cell, 1, NULL, 0);
> >>+ if (err) {
> >>+ dev_err(dev, "cannot add mfd cells\n");
> >>+ return err;
> >>+ }
> >>+ }
> >>+ return err;
> >>+}
> >So, what are the potential subdevices for this driver ? If it's a really
> >dynamic setup, I'm fine with passing those as platform data but
> >then do it so
> >that you pass a NULL terminated da8xx_pruss_devices array. That will avoid
> >most of the ugly casts you're doing here.
>
> SG -- I did not follow your recommendations here, could you please
> elaborate.
> I am already checking the dev_name for a NULL.
> This device is basically a microcontroller within DA850,
> so basically any device or protocol can be
> emulated on it. Currently, we have emulated 8 UARTS using
> the two PRUs and also a CAN device.
Ok, I wasnt sure you can emulate anything on that thing. So I'm fine with you
passing all your devices through platform_data. But I'd prefer this routine to
look like:
[...]
for (count = 0; dev_data[count] != NULL; count++) {
memset(&cell, 0, sizeof(struct mfd_cell));
cell.id = count;
cell.name = dev_data[count]->dev_name;
cell.platform_data = dev_data[count]->pdata;
cell.data_size = dev_data[count]->pdata_size;
Looks nicer to me.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
On 02/22/2011 11:31 AM, Samuel Ortiz wrote:
> Hi Subhasish,
>
> On Tue, Feb 22, 2011 at 11:13:38AM +0530, Subhasish Ghosh wrote:
>> Thank you for your comments.
> No problem.
>
>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>>> index fd01836..6c437df 100644
>>>> --- a/drivers/mfd/Kconfig
>>>> +++ b/drivers/mfd/Kconfig
>>>> @@ -81,6 +81,16 @@ config MFD_DM355EVM_MSP
>>>> boards. MSP430 firmware manages resets and power sequencing,
>>>> inputs from buttons and the IR remote, LEDs, an RTC, and more.
>>>>
>>>> +config MFD_DA8XX_PRUSS
>>>> + tristate "Texas Instruments DA8XX PRUSS support"
>>>> + depends on ARCH_DAVINCI && ARCH_DAVINCI_DA850
>>> Why are we depending on those ?
>>
>> SG -- The PRUSS core in only available within DA850 and DA830,
>> DA830 support is not yet implemented.
> Sure, but if there are no actual code dependencies, I'd like to get rid of
> those depends.
>
>>>> +u32 pruss_disable(struct device *dev, u8 pruss_num)
>>>> +{
>>>> + struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
>>>> + da8xx_prusscore_regs h_pruss;
>>>> + u32 temp_reg;
>>>> +
>>>> + if (pruss_num == DA8XX_PRUCORE_0) {
>>>> + /* Disable PRU0 */
>>>> + h_pruss = (da8xx_prusscore_regs)
>>>> + ((u32) pruss->ioaddr + 0x7000);
>>> So it seems you're doing this in several places, and I have a few
>>> comments:
>>>
>>> - You don't need the da8xx_prusscore_regs at all.
>>> - Define the register map through a set of #define in your header file.
>>> - Use a static routine that takes the core number and returns the
>>> register map
>>> offset.
>>>
>>> Then routines like this one will look a lot more readable.
>>
>> SG -- There are a huge number of PRUSS registers. A lot of them are
>> reserved and are expected to change as development on the
>> controller is still ongoing.
> First of all, from what I read in your patch you're only using the CONTROL
> offset.
>
>> If we use #defines to plot
>> all the registers, then first, there are too many array type
>> registers which will need to be duplicated.
> What I'm expecting is a small set of defines for the register offsets. You
> have 13 fields in your da8xx_prusscore_regs, you only need to define 13
> register offsets.
>
> So, if you have a:
>
> static u32 reg_offset(struct device *dev, u8 pru_num)
> {
> struct da8xx_pruss *pru = dev_get_drvdata(dev->parent);
>
> switch (pru_num) {
> case DA8XX_PRUCORE_0:
> return (u32) pru->ioaddr + 0x7000;
> case DA8XX_PRUCORE_1:
> return (u32) pru->ioaddr + 0x7800;
> default:
> return 0;
> }
>
>
> then routines like pruss_enable (which should return an int, btw) would look
> like:
>
> int pruss_enable(struct device *dev, u8 pruss_num)
> {
> u32 offset = reg_offset(dev, pruss_num);
>
> if (offset == 0)
> return -EINVAL;
>
> __raw_writel(DA8XX_PRUCORE_CONTROL_RESETVAL,
> offset + PRU_CORE_CONTROL);
>
> return 0;
> }
All registers are memory mapped and could nicely be described by
structures (and sub-structures). Therefore we asked to considerer
structs, at least for the Pruss SocketCAN drivers. That would result in
much much clearer and better readable code. The code above would shrink to:
__raw_writel(DA8XX_PRUCORE_CONTROL_RESETVAL,
&prucore[pruss_num].control);
And proper type checking would be ensured as well.
Wolfgang.
On Tue, Feb 22, 2011 at 11:48:51AM +0100, Wolfgang Grandegger wrote:
> On 02/22/2011 11:31 AM, Samuel Ortiz wrote:
> > Hi Subhasish,
> >
> > On Tue, Feb 22, 2011 at 11:13:38AM +0530, Subhasish Ghosh wrote:
> >> Thank you for your comments.
> > No problem.
> >
> >>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> >>>> index fd01836..6c437df 100644
> >>>> --- a/drivers/mfd/Kconfig
> >>>> +++ b/drivers/mfd/Kconfig
> >>>> @@ -81,6 +81,16 @@ config MFD_DM355EVM_MSP
> >>>> boards. MSP430 firmware manages resets and power sequencing,
> >>>> inputs from buttons and the IR remote, LEDs, an RTC, and more.
> >>>>
> >>>> +config MFD_DA8XX_PRUSS
> >>>> + tristate "Texas Instruments DA8XX PRUSS support"
> >>>> + depends on ARCH_DAVINCI && ARCH_DAVINCI_DA850
> >>> Why are we depending on those ?
> >>
> >> SG -- The PRUSS core in only available within DA850 and DA830,
> >> DA830 support is not yet implemented.
> > Sure, but if there are no actual code dependencies, I'd like to get rid of
> > those depends.
> >
> >>>> +u32 pruss_disable(struct device *dev, u8 pruss_num)
> >>>> +{
> >>>> + struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
> >>>> + da8xx_prusscore_regs h_pruss;
> >>>> + u32 temp_reg;
> >>>> +
> >>>> + if (pruss_num == DA8XX_PRUCORE_0) {
> >>>> + /* Disable PRU0 */
> >>>> + h_pruss = (da8xx_prusscore_regs)
> >>>> + ((u32) pruss->ioaddr + 0x7000);
> >>> So it seems you're doing this in several places, and I have a few
> >>> comments:
> >>>
> >>> - You don't need the da8xx_prusscore_regs at all.
> >>> - Define the register map through a set of #define in your header file.
> >>> - Use a static routine that takes the core number and returns the
> >>> register map
> >>> offset.
> >>>
> >>> Then routines like this one will look a lot more readable.
> >>
> >> SG -- There are a huge number of PRUSS registers. A lot of them are
> >> reserved and are expected to change as development on the
> >> controller is still ongoing.
> > First of all, from what I read in your patch you're only using the CONTROL
> > offset.
> >
> >> If we use #defines to plot
> >> all the registers, then first, there are too many array type
> >> registers which will need to be duplicated.
> > What I'm expecting is a small set of defines for the register offsets. You
> > have 13 fields in your da8xx_prusscore_regs, you only need to define 13
> > register offsets.
> >
> > So, if you have a:
> >
> > static u32 reg_offset(struct device *dev, u8 pru_num)
> > {
> > struct da8xx_pruss *pru = dev_get_drvdata(dev->parent);
> >
> > switch (pru_num) {
> > case DA8XX_PRUCORE_0:
> > return (u32) pru->ioaddr + 0x7000;
> > case DA8XX_PRUCORE_1:
> > return (u32) pru->ioaddr + 0x7800;
> > default:
> > return 0;
> > }
> >
> >
> > then routines like pruss_enable (which should return an int, btw) would look
> > like:
> >
> > int pruss_enable(struct device *dev, u8 pruss_num)
> > {
> > u32 offset = reg_offset(dev, pruss_num);
> >
> > if (offset == 0)
> > return -EINVAL;
> >
> > __raw_writel(DA8XX_PRUCORE_CONTROL_RESETVAL,
> > offset + PRU_CORE_CONTROL);
> >
> > return 0;
> > }
>
> All registers are memory mapped and could nicely be described by
> structures (and sub-structures). Therefore we asked to considerer
> structs, at least for the Pruss SocketCAN drivers.
>
> That would result in
> much much clearer and better readable code. The code above would shrink to:
>
> __raw_writel(DA8XX_PRUCORE_CONTROL_RESETVAL,
> &prucore[pruss_num].control);
This driver seems to exclusively use the control offset, which is why I don't
see an absolute need for doing this mapping.
But if both maps are contiguous then doing the mapping would prevent us from
calling reg_offset() and would bring some advantage. I'd then be fine with it.
For now, da8xx_prusscore_regs seems to be larger than the 0x800 interval
between the 2 maps, so I have no idea if both maps are indeed contiguous.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
I am not sure if I understood you correctly, but the current sizeof the
structure da8xx_prusscore_regs is 0x500.
Here is a link to the PRUSS memory map:
http://processors.wiki.ti.com/index.php/PRUSS_Memory_Map
The offset 0x00007000 is the PRU0 reg offset and 0x00007800 is the PRU1 reg
offset.
We cannot have a register file larger than this, but lot of space is left
out, possibly for future development.
--------------------------------------------------
From: "Samuel Ortiz" <[email protected]>
Sent: Tuesday, February 22, 2011 5:03 PM
To: "Wolfgang Grandegger" <[email protected]>
Cc: "Subhasish Ghosh" <[email protected]>;
<[email protected]>;
<[email protected]>; <[email protected]>; "open
list" <[email protected]>; <[email protected]>;
<[email protected]>
Subject: Re: [PATCH v2 01/13] mfd: pruss mfd driver.
> On Tue, Feb 22, 2011 at 11:48:51AM +0100, Wolfgang Grandegger wrote:
>> On 02/22/2011 11:31 AM, Samuel Ortiz wrote:
>> > Hi Subhasish,
>> >
>> > On Tue, Feb 22, 2011 at 11:13:38AM +0530, Subhasish Ghosh wrote:
>> >> Thank you for your comments.
>> > No problem.
>> >
>> >>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> >>>> index fd01836..6c437df 100644
>> >>>> --- a/drivers/mfd/Kconfig
>> >>>> +++ b/drivers/mfd/Kconfig
>> >>>> @@ -81,6 +81,16 @@ config MFD_DM355EVM_MSP
>> >>>> boards. MSP430 firmware manages resets and power sequencing,
>> >>>> inputs from buttons and the IR remote, LEDs, an RTC, and more.
>> >>>>
>> >>>> +config MFD_DA8XX_PRUSS
>> >>>> + tristate "Texas Instruments DA8XX PRUSS support"
>> >>>> + depends on ARCH_DAVINCI && ARCH_DAVINCI_DA850
>> >>> Why are we depending on those ?
>> >>
>> >> SG -- The PRUSS core in only available within DA850 and DA830,
>> >> DA830 support is not yet implemented.
>> > Sure, but if there are no actual code dependencies, I'd like to get rid
>> > of
>> > those depends.
>> >
>> >>>> +u32 pruss_disable(struct device *dev, u8 pruss_num)
>> >>>> +{
>> >>>> + struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
>> >>>> + da8xx_prusscore_regs h_pruss;
>> >>>> + u32 temp_reg;
>> >>>> +
>> >>>> + if (pruss_num == DA8XX_PRUCORE_0) {
>> >>>> + /* Disable PRU0 */
>> >>>> + h_pruss = (da8xx_prusscore_regs)
>> >>>> + ((u32) pruss->ioaddr + 0x7000);
>> >>> So it seems you're doing this in several places, and I have a few
>> >>> comments:
>> >>>
>> >>> - You don't need the da8xx_prusscore_regs at all.
>> >>> - Define the register map through a set of #define in your header
>> >>> file.
>> >>> - Use a static routine that takes the core number and returns the
>> >>> register map
>> >>> offset.
>> >>>
>> >>> Then routines like this one will look a lot more readable.
>> >>
>> >> SG -- There are a huge number of PRUSS registers. A lot of them are
>> >> reserved and are expected to change as development on the
>> >> controller is still ongoing.
>> > First of all, from what I read in your patch you're only using the
>> > CONTROL
>> > offset.
>> >
>> >> If we use #defines to plot
>> >> all the registers, then first, there are too many array type
>> >> registers which will need to be duplicated.
>> > What I'm expecting is a small set of defines for the register offsets.
>> > You
>> > have 13 fields in your da8xx_prusscore_regs, you only need to define 13
>> > register offsets.
>> >
>> > So, if you have a:
>> >
>> > static u32 reg_offset(struct device *dev, u8 pru_num)
>> > {
>> > struct da8xx_pruss *pru = dev_get_drvdata(dev->parent);
>> >
>> > switch (pru_num) {
>> > case DA8XX_PRUCORE_0:
>> > return (u32) pru->ioaddr + 0x7000;
>> > case DA8XX_PRUCORE_1:
>> > return (u32) pru->ioaddr + 0x7800;
>> > default:
>> > return 0;
>> > }
>> >
>> >
>> > then routines like pruss_enable (which should return an int, btw) would
>> > look
>> > like:
>> >
>> > int pruss_enable(struct device *dev, u8 pruss_num)
>> > {
>> > u32 offset = reg_offset(dev, pruss_num);
>> >
>> > if (offset == 0)
>> > return -EINVAL;
>> >
>> > __raw_writel(DA8XX_PRUCORE_CONTROL_RESETVAL,
>> > offset + PRU_CORE_CONTROL);
>> >
>> > return 0;
>> > }
>>
>> All registers are memory mapped and could nicely be described by
>> structures (and sub-structures). Therefore we asked to considerer
>> structs, at least for the Pruss SocketCAN drivers.
>>
>> That would result in
>> much much clearer and better readable code. The code above would shrink
>> to:
>>
>> __raw_writel(DA8XX_PRUCORE_CONTROL_RESETVAL,
>> &prucore[pruss_num].control);
> This driver seems to exclusively use the control offset, which is why I
> don't
> see an absolute need for doing this mapping.
> But if both maps are contiguous then doing the mapping would prevent us
> from
> calling reg_offset() and would bring some advantage. I'd then be fine with
> it.
> For now, da8xx_prusscore_regs seems to be larger than the 0x800 interval
> between the 2 maps, so I have no idea if both maps are indeed contiguous.
>
> Cheers,
> Samuel.
>
> --
> Intel Open Source Technology Centre
> http://oss.intel.com/
On 02/22/2011 01:49 PM, Subhasish Ghosh wrote:
> I am not sure if I understood you correctly, but the current sizeof the
> structure da8xx_prusscore_regs is 0x500.
>
> Here is a link to the PRUSS memory map:
> http://processors.wiki.ti.com/index.php/PRUSS_Memory_Map
>
> The offset 0x00007000 is the PRU0 reg offset and 0x00007800 is the PRU1
> reg offset.
> We cannot have a register file larger than this, but lot of space is
> left out, possibly for future development.
What do you mean with "larger than this"? You ioremap the whole space
and unsued space could be filled with padding bytes:
struct pruss_regs {
u8 ram0[0x0200];
u8 res0[0x1e00];
u8 ram1[0x0200];
u8 res1[0x1e00];
struct pruss_intc_regs intc;
struct pruss_core_regs core[2];
};
Then:
pruss_dev->regs = ioremap(pruss_dev->res->start,
resource_size(pruss_dev->res));
__raw_writel(DA8XX_PRUCORE_CONTROL_RESETVAL,
&pruss_dev->regs.core[pruss_num].control);
That's simple, transparent and save without magic offset handling.
Wolfgang.
--------------------------------------------------
From: "Samuel Ortiz" <[email protected]>
Sent: Tuesday, February 22, 2011 4:01 PM
To: "Subhasish Ghosh" <[email protected]>
Cc: <[email protected]>;
<[email protected]>; <[email protected]>;
<[email protected]>; <[email protected]>; "open list"
<[email protected]>
Subject: Re: [PATCH v2 01/13] mfd: pruss mfd driver.
> Hi Subhasish,
>
> On Tue, Feb 22, 2011 at 11:13:38AM +0530, Subhasish Ghosh wrote:
>> Thank you for your comments.
> No problem.
>
>> >>diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> >>index fd01836..6c437df 100644
>> >>--- a/drivers/mfd/Kconfig
>> >>+++ b/drivers/mfd/Kconfig
>> >>@@ -81,6 +81,16 @@ config MFD_DM355EVM_MSP
>> >> boards. MSP430 firmware manages resets and power sequencing,
>> >> inputs from buttons and the IR remote, LEDs, an RTC, and more.
>> >>
>> >>+config MFD_DA8XX_PRUSS
>> >>+ tristate "Texas Instruments DA8XX PRUSS support"
>> >>+ depends on ARCH_DAVINCI && ARCH_DAVINCI_DA850
>> >Why are we depending on those ?
>>
>> SG -- The PRUSS core in only available within DA850 and DA830,
>> DA830 support is not yet implemented.
> Sure, but if there are no actual code dependencies, I'd like to get rid of
> those depends.
SG -- The PRU Clock and Power is the dependency here.
This is available in arch/arm/mach-davinci/da850.c
The source is specific to the SOC clock tree.
>
>> >>+u32 pruss_disable(struct device *dev, u8 pruss_num)
>> >>+{
>> >>+ struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
>> >>+ da8xx_prusscore_regs h_pruss;
>> >>+ u32 temp_reg;
>> >>+
>> >>+ if (pruss_num == DA8XX_PRUCORE_0) {
>> >>+ /* Disable PRU0 */
>> >>+ h_pruss = (da8xx_prusscore_regs)
>> >>+ ((u32) pruss->ioaddr + 0x7000);
>> >So it seems you're doing this in several places, and I have a few
>> >comments:
>> >
>> >- You don't need the da8xx_prusscore_regs at all.
>> >- Define the register map through a set of #define in your header file.
>> >- Use a static routine that takes the core number and returns the
>> >register map
>> >offset.
>> >
>> >Then routines like this one will look a lot more readable.
>>
>> SG -- There are a huge number of PRUSS registers. A lot of them are
>> reserved and are expected to change as development on the
>> controller is still ongoing.
> First of all, from what I read in your patch you're only using the CONTROL
> offset.
>
>> If we use #defines to plot
>> all the registers, then first, there are too many array type
>> registers which will need to be duplicated.
> What I'm expecting is a small set of defines for the register offsets. You
> have 13 fields in your da8xx_prusscore_regs, you only need to define 13
> register offsets.
>
> So, if you have a:
>
> static u32 reg_offset(struct device *dev, u8 pru_num)
> {
> struct da8xx_pruss *pru = dev_get_drvdata(dev->parent);
>
> switch (pru_num) {
> case DA8XX_PRUCORE_0:
> return (u32) pru->ioaddr + 0x7000;
> case DA8XX_PRUCORE_1:
> return (u32) pru->ioaddr + 0x7800;
> default:
> return 0;
> }
>
>
> then routines like pruss_enable (which should return an int, btw) would
> look
> like:
>
> int pruss_enable(struct device *dev, u8 pruss_num)
> {
> u32 offset = reg_offset(dev, pruss_num);
>
> if (offset == 0)
> return -EINVAL;
>
> __raw_writel(DA8XX_PRUCORE_CONTROL_RESETVAL,
> offset + PRU_CORE_CONTROL);
>
> return 0;
> }
>
>> >Also, all your exported routines severely lack any sort of locking. An
>> >IO
>> >mutex or spinlock is mandatory here.
>>
>> SG - As per our current implementation, we do not have two devices
>> running simultaneously on the PRU,
>> so we do not have any way to test it. We have kept this as an
>> enhancement if request comes in for
>> multiple devices.
> It's not about having multiple devices at the same time, it's about having
> multiple callers writing and reading to the same registers. Since you're
> exporting all your I/O routines you have no way to prevent 2 drivers from
> writing to the same register at the "same" time. You need locking here,
> regardless of the number of devices that you can have on a system.
>
SG - Ok, will do
>
>> >>+static int pruss_mfd_add_devices(struct platform_device *pdev)
>> >>+{
>> >>+ struct da8xx_pruss_devices *dev_data = pdev->dev.platform_data;
>> >>+ struct device *dev = &pdev->dev;
>> >>+ struct mfd_cell cell;
>> >>+ u32 err, count;
>> >>+
>> >>+ for (count = 0; (dev_data + count)->dev_name != NULL; count++) {
>> >>+ memset(&cell, 0, sizeof(struct mfd_cell));
>> >>+ cell.id = count;
>> >>+ cell.name = (dev_data + count)->dev_name;
>> >>+ cell.platform_data = (dev_data + count)->pdata;
>> >>+ cell.data_size = (dev_data + count)->pdata_size;
>> >>+
>> >>+ err = mfd_add_devices(dev, 0, &cell, 1, NULL, 0);
>> >>+ if (err) {
>> >>+ dev_err(dev, "cannot add mfd cells\n");
>> >>+ return err;
>> >>+ }
>> >>+ }
>> >>+ return err;
>> >>+}
>> >So, what are the potential subdevices for this driver ? If it's a really
>> >dynamic setup, I'm fine with passing those as platform data but
>> >then do it so
>> >that you pass a NULL terminated da8xx_pruss_devices array. That will
>> >avoid
>> >most of the ugly casts you're doing here.
>>
>> SG -- I did not follow your recommendations here, could you please
>> elaborate.
>> I am already checking the dev_name for a NULL.
>> This device is basically a microcontroller within DA850,
>> so basically any device or protocol can be
>> emulated on it. Currently, we have emulated 8 UARTS using
>> the two PRUs and also a CAN device.
> Ok, I wasnt sure you can emulate anything on that thing. So I'm fine with
> you
> passing all your devices through platform_data. But I'd prefer this
> routine to
> look like:
>
> [...]
> for (count = 0; dev_data[count] != NULL; count++) {
> memset(&cell, 0, sizeof(struct mfd_cell));
> cell.id = count;
> cell.name = dev_data[count]->dev_name;
> cell.platform_data = dev_data[count]->pdata;
> cell.data_size = dev_data[count]->pdata_size;
>
> Looks nicer to me.
SG - I have a problem here, dev_data was initialized as a structure array.
static struct da8xx_pruss_devices pruss_devices[] = {
{
.dev_name = "da8xx_pruss_can",
.pdata = &can_data,
.pdata_size = sizeof(can_data),
.setup = da850_evm_setup_pruss_can,
.num_resources = 0,
.resources = NULL,
},
{
.dev_name = "da8xx_pruss_uart",
.pdata = &suart_data,
.pdata_size = sizeof(suart_data),
.setup = da850_evm_setup_pruss_suart,
.num_resources = ARRAY_SIZE(da850_evm_suart_resource),
.resources = da850_evm_suart_resource,
},
{
.dev_name = NULL,
},
};
How can I initialize the last array element to NULL!
I think, I must have some type of delimiter.
>
> Cheers,
> Samuel.
>
> --
> Intel Open Source Technology Centre
> http://oss.intel.com/
On Tue, Feb 22, 2011 at 11:31:27AM +0100, Samuel Ortiz wrote:
> So, if you have a:
>
> static u32 reg_offset(struct device *dev, u8 pru_num)
> {
> struct da8xx_pruss *pru = dev_get_drvdata(dev->parent);
>
> switch (pru_num) {
> case DA8XX_PRUCORE_0:
> return (u32) pru->ioaddr + 0x7000;
> case DA8XX_PRUCORE_1:
> return (u32) pru->ioaddr + 0x7800;
> default:
> return 0;
> }
No. Please don't encourage people to have 'u32' as valid cookies for
readl,writel et.al. Always make the mmio cookies __iomem pointer like.