* [PATCH] Hisilicon LPC driver
@ 2015-11-30 13:07 Rongrong Zou
2015-11-30 13:19 ` Arnd Bergmann
0 siblings, 1 reply; 6+ messages in thread
From: Rongrong Zou @ 2015-11-30 13:07 UTC (permalink / raw)
To: gregkh, arnd, lixiancai
Cc: linux-kernel, zourongrong, lijianhua, linuxarm, minyard,
Rongrong Zou, lijianhua
This is the Low Pin Count driver for Hisilicon Hi1610 SoC. It is used
for LPC master accessing LPC slave device.
We only implement I/O read and I/O write here, and the 2 interfaces are
exported for uart driver and ipmi_si driver.
Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
Signed-off-by: lijianhua <Jueying0518@gmail.com>
---
.../bindings/misc/hisilicon,low-pin-count.txt | 11 +
MAINTAINERS | 5 +
drivers/misc/Kconfig | 7 +
drivers/misc/Makefile | 1 +
drivers/misc/hisi_lpc.c | 292 +++++++++++++++++++++
include/linux/hisi_lpc.h | 83 ++++++
6 files changed, 399 insertions(+)
create mode 100644 Documentation/devicetree/bindings/misc/hisilicon,low-pin-count.txt
create mode 100644 drivers/misc/hisi_lpc.c
create mode 100644 include/linux/hisi_lpc.h
diff --git a/Documentation/devicetree/bindings/misc/hisilicon,low-pin-count.txt b/Documentation/devicetree/bindings/misc/hisilicon,low-pin-count.txt
new file mode 100644
index 0000000..05c1e19
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/hisilicon,low-pin-count.txt
@@ -0,0 +1,11 @@
+Hisilicon Low Pin Count bus
+
+Required properties
+- compatible: "hisilicon,low-pin-count"
+- reg specifies low pin count address range
+
+Example:
+ lpc_0: lpc@a01b0000 {
+ compatible = "hisilicon,low-pin-count";
+ ret = <0x0 0xa01b0000, 0x0, 0x10000>;
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 3f92804..352a4e2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5037,6 +5037,11 @@ F: include/uapi/linux/if_hippi.h
F: net/802/hippi.c
F: drivers/net/hippi/
+HISILICON LOW PIN COUNT DRIVER
+M: Rongrong Zou <zourongrong@huawei.com>
+S: Maintained
+F: drivers/misc/hisi-lpc/
+
HOST AP DRIVER
M: Jouni Malinen <j@w1.fi>
L: hostap@shmoo.com (subscribers-only)
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 22892c7..67f286f 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -525,6 +525,13 @@ config VEXPRESS_SYSCFG
bus. System Configuration interface is one of the possible means
of generating transactions on this bus.
+config HISI_LPC
+ bool "hisilicon Low Pin Count driver"
+ help
+ The hisilicon Low Pin Count controller can communicate with
+ device which have Low Pin Count slave controller. This driver
+ Support IO cycle only.
+
source "drivers/misc/c2port/Kconfig"
source "drivers/misc/eeprom/Kconfig"
source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 537d7f3..a8903f0dc 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -56,3 +56,4 @@ obj-$(CONFIG_GENWQE) += genwqe/
obj-$(CONFIG_ECHO) += echo/
obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
obj-$(CONFIG_CXL_BASE) += cxl/
+obj-$(CONFIG_HISI_LPC) += hisi_lpc.o
diff --git a/drivers/misc/hisi_lpc.c b/drivers/misc/hisi_lpc.c
new file mode 100644
index 0000000..aae9ff3
--- /dev/null
+++ b/drivers/misc/hisi_lpc.c
@@ -0,0 +1,292 @@
+#include <linux/version.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/errno.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/dma-mapping.h>
+#include <linux/list.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/hisi_lpc.h>
+
+#define LPC_REG_READ(reg, result) ((result) = readl(reg))
+
+#define LPC_REG_WRITE(reg, data) writel((data), (reg))
+
+
+struct hs_lpc_dev *lpc_dev;
+
+int lpc_master_write(unsigned int slv_access_mode, unsigned int cycle_type,
+ unsigned int addr, unsigned char *buf, unsigned int len)
+{
+ unsigned int i;
+ unsigned int lpc_cmd_value;
+ unsigned int lpc_irq_st_value;
+ unsigned int lpc_op_state_value;
+ unsigned int retry = 0;
+
+ /* para check */
+ if (!buf)
+ return -EFAULT;
+
+ if (slv_access_mode != HS_LPC_CMD_SAMEADDR_SING &&
+ slv_access_mode != HS_LPC_CMD_SAMEADDR_INC) {
+ dev_err(lpc_dev->dev, "Slv access mode error\n");
+ return -EINVAL;
+ }
+
+ if ((cycle_type != HS_LPC_CMD_TYPE_IO) &&
+ (cycle_type != HS_LPC_CMD_TYPE_MEM) &&
+ (cycle_type != HS_LPC_CMD_TYPE_FWH)) {
+ dev_err(lpc_dev->dev, "Cycle type error\n");
+ return -EINVAL;
+ }
+
+ if (len == 0) {
+ dev_err(lpc_dev->dev, "Write length zero\n");
+ return -EINVAL;
+ }
+
+ LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_IRQ_ST, HS_LPC_IRQ_CLEAR);
+ retry = 0;
+ while (0 == (LPC_REG_READ(lpc_dev->regs + HS_LPC_REG_OP_STATUS,
+ lpc_op_state_value) & HS_LPC_STATUS_DILE)) {
+ udelay(1);
+ retry++;
+ if (retry >= 10000) {
+ dev_err(lpc_dev->dev, "lpc W, wait idle time out\n");
+ return -ETIME;
+ }
+ }
+
+ /* set lpc master write cycle type and slv access mode */
+ lpc_cmd_value = HS_LPC_CMD_WRITE | cycle_type | slv_access_mode;
+ LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_CMD, lpc_cmd_value);
+
+ /* set lpc op len */
+ LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_OP_LEN, len);
+
+ /* Set write data */
+ for (i = 0; i < len; i++)
+ LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_WDATA, buf[i]);
+
+ /* set lpc addr config */
+ LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_ADDR, addr);
+
+ /* set lpc start work */
+ LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_START, START_WORK);
+
+ retry = 0;
+ while ((LPC_REG_READ(lpc_dev->regs + HS_LPC_REG_IRQ_ST,
+ lpc_irq_st_value) & HS_LPC_IRQ_OCCURRED) == 0) {
+ udelay(1);
+ retry++;
+ if (retry >= 10000) {
+ dev_err(lpc_dev->dev, "lpc write, time out\n");
+ return -ETIME;
+ }
+ }
+
+ LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_IRQ_ST, HS_LPC_IRQ_CLEAR);
+
+ if ((LPC_REG_READ(lpc_dev->regs + HS_LPC_REG_OP_STATUS,
+ lpc_op_state_value) & HS_LPC_OP_FINISHED) == HS_LPC_OP_FINISHED) {
+ return 0;
+ }
+
+ dev_err(lpc_dev->dev, "Lpc master write failed, op_result : 0x%x\n",
+ lpc_op_state_value);
+ return -1;
+}
+
+void lpc_io_write_byte(u8 value, unsigned long addr)
+{
+ unsigned long flags;
+ int ret;
+
+ if (!lpc_dev) {
+ pr_err("device is not register\n!");
+ return;
+ }
+ spin_lock_irqsave(&lpc_dev->lock, flags);
+ ret = lpc_master_write(HS_LPC_CMD_SAMEADDR_SING, HS_LPC_CMD_TYPE_IO,
+ addr, &value, 1);
+ spin_unlock_irqrestore(&lpc_dev->lock, flags);
+}
+EXPORT_SYMBOL(lpc_io_write_byte);
+
+int lpc_master_read(unsigned int slv_access_mode, unsigned int cycle_type,
+ unsigned int addr, unsigned char *buf, unsigned int len)
+{
+ unsigned int i;
+ unsigned int lpc_cmd_value;
+ unsigned int lpc_irq_st_value;
+ unsigned int lpc_rdata_value;
+ unsigned int lpc_op_state_value;
+ unsigned int retry = 0;
+
+ /* para check */
+ if (!buf)
+ return -EFAULT;
+
+ if (slv_access_mode != HS_LPC_CMD_SAMEADDR_SING &&
+ slv_access_mode != HS_LPC_CMD_SAMEADDR_INC) {
+ dev_err(lpc_dev->dev, "Slv access mode error\n");
+ return -EINVAL;
+ }
+
+ if (cycle_type != HS_LPC_CMD_TYPE_IO &&
+ cycle_type != HS_LPC_CMD_TYPE_MEM &&
+ cycle_type != HS_LPC_CMD_TYPE_FWH) {
+ dev_err(lpc_dev->dev, "Cycle type error\n");
+ return -EINVAL;
+ }
+
+ LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_IRQ_ST, HS_LPC_IRQ_CLEAR);
+
+ retry = 0;
+ while (0 == (LPC_REG_READ(lpc_dev->regs + HS_LPC_REG_OP_STATUS,
+ lpc_op_state_value) & HS_LPC_STATUS_DILE)) {
+ udelay(1);
+ retry++;
+ if (retry >= 10000) {
+ dev_err(lpc_dev->dev, "lpc read,wait idl timeout\n");
+ return -ETIME;
+ }
+ }
+
+ /* set lpc master read cycle type and slv access mode */
+ lpc_cmd_value = HS_LPC_CMD_READ | cycle_type | slv_access_mode;
+ LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_CMD, lpc_cmd_value);
+
+ /* set lpc op len */
+ LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_OP_LEN, len);
+
+ /* set lpc addr config */
+ LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_ADDR, addr);
+
+ /* set lpc start work */
+ LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_START, START_WORK);
+
+ while ((LPC_REG_READ(lpc_dev->regs + HS_LPC_REG_IRQ_ST,
+ lpc_irq_st_value) & HS_LPC_IRQ_OCCURRED) == 0) {
+ udelay(1);
+ retry++;
+ if (retry >= 10000) {
+ dev_err(lpc_dev->dev, "lpc read, time out\n");
+ return -ETIME;
+ }
+ }
+
+ LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_IRQ_ST, HS_LPC_IRQ_CLEAR);
+
+ /* Get read data */
+ if ((LPC_REG_READ(lpc_dev->regs + HS_LPC_REG_OP_STATUS,
+ lpc_op_state_value) & HS_LPC_OP_FINISHED) == HS_LPC_OP_FINISHED) {
+ for (i = 0; i < len; i++) {
+ LPC_REG_READ(lpc_dev->regs + HS_LPC_REG_RDATA,
+ lpc_rdata_value);
+ buf[i] = lpc_rdata_value;
+ }
+ return 0;
+ }
+
+ dev_err(lpc_dev->dev, "Lpc master read failed, op_result : 0x%x\n",
+ lpc_op_state_value);
+
+ return -1;
+}
+
+u8 lpc_io_read_byte(unsigned long addr)
+{
+ unsigned char value;
+ unsigned long flags;
+
+ if (!lpc_dev) {
+ pr_err("device is not register!\n");
+ return 0;
+ }
+
+ spin_lock_irqsave(&lpc_dev->lock, flags);
+ lpc_master_read(HS_LPC_CMD_SAMEADDR_SING,
+ HS_LPC_CMD_TYPE_IO, addr, &value, 1);
+ spin_unlock_irqrestore(&lpc_dev->lock, flags);
+ return value;
+}
+EXPORT_SYMBOL(lpc_io_read_byte);
+
+static int hs_lpc_probe(struct platform_device *pdev)
+{
+ struct resource *regs = NULL;
+
+ if (!pdev->dev.of_node) {
+ dev_err(&pdev->dev, "Device OF-Node is NULL\n");
+ return -EFAULT;
+ }
+
+ lpc_dev = devm_kzalloc(&pdev->dev,
+ sizeof(struct hs_lpc_dev), GFP_KERNEL);
+ if (!lpc_dev)
+ return -ENOMEM;
+
+ spin_lock_init(&lpc_dev->lock);
+ regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ lpc_dev->regs = devm_ioremap_resource(&pdev->dev, regs);
+ if (IS_ERR(lpc_dev->regs))
+ return PTR_ERR(lpc_dev->regs);
+
+ dev_info(&pdev->dev, "hisi low pin count initialized successfully\n");
+
+ lpc_dev->dev = &pdev->dev;
+ platform_set_drvdata(pdev, lpc_dev);
+
+ return 0;
+}
+
+static int hs_lpc_remove(struct platform_device *pdev)
+{
+ return 0;
+}
+
+static const struct of_device_id hs_lpc_pltfm_match[] = {
+ {
+ .compatible = "hisilicon,low-pin-count",
+ },
+ {},
+};
+
+static struct platform_driver hs_lpc_driver = {
+ .driver = {
+ .name = "hs-lpc",
+ .owner = THIS_MODULE,
+ .of_match_table = hs_lpc_pltfm_match,
+ },
+ .probe = hs_lpc_probe,
+ .remove = hs_lpc_remove,
+};
+
+static int __init hs_lpc_init_driver(void)
+{
+ return platform_driver_register(&hs_lpc_driver);
+}
+
+static void __exit hs_lpc_init_exit(void)
+{
+ platform_driver_unregister(&hs_lpc_driver);
+}
+
+arch_initcall(hs_lpc_init_driver);
+module_exit(hs_lpc_init_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Huawei Tech. Co., Ltd.");
+MODULE_DESCRIPTION("LPC driver for linux");
+MODULE_VERSION("v1.0");
diff --git a/include/linux/hisi_lpc.h b/include/linux/hisi_lpc.h
new file mode 100644
index 0000000..4cf93ee
--- /dev/null
+++ b/include/linux/hisi_lpc.h
@@ -0,0 +1,83 @@
+#ifndef __HISI_LPC_H__
+#define __HISI_LPC_H__
+
+#define HS_LPC_REG_START (0x00)
+#define HS_LPC_REG_OP_STATUS (0x04)
+#define HS_LPC_REG_IRQ_ST (0x08)
+#define HS_LPC_REG_OP_LEN (0x10)
+#define HS_LPC_REG_CMD (0x14)
+#define HS_LPC_REG_FWH_ID_MSIZE (0x18)
+#define HS_LPC_REG_ADDR (0x20)
+#define HS_LPC_REG_WDATA (0x24)
+#define HS_LPC_REG_RDATA (0x28)
+#define HS_LPC_REG_LONG_CNT (0x30)
+#define HS_LPC_REG_TX_FIFO_ST (0x50)
+#define HS_LPC_REG_RX_FIFO_ST (0x54)
+#define HS_LPC_REG_TIME_OUT (0x58)
+#define HS_LPC_REG_STRQ_CTRL0 (0x80)
+#define HS_LPC_REG_STRQ_CTRL1 (0x84)
+#define HS_LPC_REG_STRQ_INT (0x90)
+#define HS_LPC_REG_STRQ_INT_MASK (0x94)
+#define HS_LPC_REG_STRQ_STAT (0xa0)
+
+#define HS_LPC_CMD_SAMEADDR_SING (0x00000008)
+#define HS_LPC_CMD_SAMEADDR_INC (0x00000000)
+#define HS_LPC_CMD_TYPE_IO (0x00000000)
+#define HS_LPC_CMD_TYPE_MEM (0x00000002)
+#define HS_LPC_CMD_TYPE_FWH (0x00000004)
+#define HS_LPC_CMD_WRITE (0x00000001)
+#define HS_LPC_CMD_READ (0x00000000)
+
+#define HS_LPC_IRQ_CLEAR (0x02)
+#define HS_LPC_IRQ_OCCURRED (0x02)
+#define HS_LPC_STATUS_DILE (0x01)
+#define HS_LPC_OP_FINISHED (0x02)
+#define START_WORK (0x01)
+
+#define REG_OFFSET_IOMG033 0X84
+#define REG_OFFSET_IOMG035 0x8C
+#define REG_OFFSET_IOMG036 0x90
+#define REG_OFFSET_IOMG050 0xC8
+#define REG_OFFSET_IOMG049 0xC4
+#define REG_OFFSET_IOMG048 0xC0
+#define REG_OFFSET_IOMG047 0xBC
+#define REG_OFFSET_IOMG046 0xB8
+#define REG_OFFSET_IOMG045 0xB4
+
+#define REG_OFFSET_LPC_RESET 0xad8
+#define REG_OFFSET_LPC_BUS_RESET 0xae0
+#define REG_OFFSET_LPC_CLK_DIS 0x3a4
+#define REG_OFFSET_LPC_BUS_CLK_DIS 0x3ac
+#define REG_OFFSET_LPC_DE_RESET 0xadc
+#define REG_OFFSET_LPC_BUS_DE_RESET 0xae4
+#define REG_OFFSET_LPC_CLK_EN 0x3a0
+#define REG_OFFSET_LPC_BUS_CLK_EN 0x3a8
+
+#define LPC_FRAME_LEN (0x10)
+
+struct hs_lpc_dev {
+ spinlock_t lock;
+ void __iomem *regs;
+ struct device *dev;
+};
+
+#define LPC_CURR_STATUS_IDLE 0
+#define LPC_CURR_STATUS_START 1
+#define LPC_CURR_STATUS_TYPE_DIR 2
+#define LPC_CURR_STATUS_ADDR 3
+#define LPC_CURR_STATUS_MSIZE 4
+#define LPC_CURR_STATUS_WDATA 5
+#define LPC_CURR_STATUS_TARHOST 6
+#define LPC_CURR_STATUS_SYNC 7
+#define LPC_CURR_STATUS_RDATA 8
+#define LPC_CURR_STATUS_TARSLAVE 9
+#define LPC_CURR_STATUS_ABORT 10
+
+#define ALG_HCCS_SUBCTRL_BASE_ADDR (0xD0000000)
+#define REG_OFFSET_RESET_REQ 0x0AD0
+#define REG_OFFSET_RESET_DREQ 0x0AD4
+
+extern u8 lpc_io_read_byte(unsigned long addr);
+extern void lpc_io_write_byte(u8 value, unsigned long addr);
+#endif
+
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Hisilicon LPC driver
2015-11-30 13:07 [PATCH] Hisilicon LPC driver Rongrong Zou
@ 2015-11-30 13:19 ` Arnd Bergmann
2015-12-01 7:58 ` Rongrong Zou
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2015-11-30 13:19 UTC (permalink / raw)
To: Rongrong Zou
Cc: gregkh, lixiancai, linux-kernel, zourongrong, lijianhua, linuxarm,
minyard, lijianhua, Will Deacon, Catalin Marinas
On Monday 30 November 2015 21:07:17 Rongrong Zou wrote:
> This is the Low Pin Count driver for Hisilicon Hi1610 SoC. It is used
> for LPC master accessing LPC slave device.
>
> We only implement I/O read and I/O write here, and the 2 interfaces are
> exported for uart driver and ipmi_si driver.
>
> Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
> Signed-off-by: lijianhua <Jueying0518@gmail.com>
> ---
> .../bindings/misc/hisilicon,low-pin-count.txt | 11 +
> MAINTAINERS | 5 +
> drivers/misc/Kconfig | 7 +
> drivers/misc/Makefile | 1 +
> drivers/misc/hisi_lpc.c | 292 +++++++++++++++++++++
> include/linux/hisi_lpc.h | 83 ++++++
> 6 files changed, 399 insertions(+)
This should not be a misc driver.
> create mode 100644 Documentation/devicetree/bindings/misc/hisilicon,low-pin-count.txt
> create mode 100644 drivers/misc/hisi_lpc.c
> create mode 100644 include/linux/hisi_lpc.h
> diff --git a/Documentation/devicetree/bindings/misc/hisilicon,low-pin-count.txt b/Documentation/devicetree/bindings/misc/hisilicon,low-pin-count.txt
> new file mode 100644
> index 0000000..05c1e19
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/hisilicon,low-pin-count.txt
> @@ -0,0 +1,11 @@
> +Hisilicon Low Pin Count bus
> +
> +Required properties
> +- compatible: "hisilicon,low-pin-count"
> +- reg specifies low pin count address range
> +
> +Example:
> + lpc_0: lpc@a01b0000 {
> + compatible = "hisilicon,low-pin-count";
> + ret = <0x0 0xa01b0000, 0x0, 0x10000>;
> + };
The name is too generic, unless you can guarantee that Hisilicon has never
before made another implementation of an LPC interface, and never will
again.
I think you should create a child address space here using a
'#address-cells' and '#size-cells'.
> +#define LPC_REG_READ(reg, result) ((result) = readl(reg))
> +
> +#define LPC_REG_WRITE(reg, data) writel((data), (reg))
Remove the obfuscation here.
> +struct hs_lpc_dev *lpc_dev;
Avoid global data structures.
> + LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_IRQ_ST, HS_LPC_IRQ_CLEAR);
> + retry = 0;
> + while (0 == (LPC_REG_READ(lpc_dev->regs + HS_LPC_REG_OP_STATUS,
> + lpc_op_state_value) & HS_LPC_STATUS_DILE)) {
> + udelay(1);
> + retry++;
> + if (retry >= 10000) {
> + dev_err(lpc_dev->dev, "lpc W, wait idle time out\n");
> + return -ETIME;
> + }
> + }
Better release the spinlock here and call a sleeping function for the wait.
If the timeout is 10ms, you definitely don't want to keep interrupts disabled
the whole time.
If you can't find a good way to retry after getting the lock back, maybe
use a mutex here that you can keep locked the whole time.
> +void lpc_io_write_byte(u8 value, unsigned long addr)
> +{
> + unsigned long flags;
> + int ret;
> +
> + if (!lpc_dev) {
> + pr_err("device is not register\n!");
> + return;
> + }
> + spin_lock_irqsave(&lpc_dev->lock, flags);
> + ret = lpc_master_write(HS_LPC_CMD_SAMEADDR_SING, HS_LPC_CMD_TYPE_IO,
> + addr, &value, 1);
> + spin_unlock_irqrestore(&lpc_dev->lock, flags);
> +}
> +EXPORT_SYMBOL(lpc_io_write_byte);
Using your own accessor functions sounds wrong here. What you have
is essentially a PCI I/O space, right? As much as we all hate I/O
space (in particular the kind that is not memory mapped), I think this
should be hooked up to the generic inb/outb functions to allow
all the generic device drivers to work.
> diff --git a/include/linux/hisi_lpc.h b/include/linux/hisi_lpc.h
> new file mode 100644
> index 0000000..4cf93ee
> --- /dev/null
> +++ b/include/linux/hisi_lpc.h
Don't do a global header here, just move it into the main file.
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Hisilicon LPC driver
2015-11-30 13:19 ` Arnd Bergmann
@ 2015-12-01 7:58 ` Rongrong Zou
2015-12-01 10:00 ` Arnd Bergmann
0 siblings, 1 reply; 6+ messages in thread
From: Rongrong Zou @ 2015-12-01 7:58 UTC (permalink / raw)
To: Arnd Bergmann, Rongrong Zou
Cc: gregkh, lixiancai, linux-kernel, lijianhua, linuxarm, minyard,
lijianhua, Will Deacon, Catalin Marinas
在 2015/11/30 21:19, Arnd Bergmann 写道:
> On Monday 30 November 2015 21:07:17 Rongrong Zou wrote:
>> This is the Low Pin Count driver for Hisilicon Hi1610 SoC. It is used
>> for LPC master accessing LPC slave device.
>>
>> We only implement I/O read and I/O write here, and the 2 interfaces are
>> exported for uart driver and ipmi_si driver.
>>
>> Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
>> Signed-off-by: lijianhua <Jueying0518@gmail.com>
>> ---
>> .../bindings/misc/hisilicon,low-pin-count.txt | 11 +
>> MAINTAINERS | 5 +
>> drivers/misc/Kconfig | 7 +
>> drivers/misc/Makefile | 1 +
>> drivers/misc/hisi_lpc.c | 292 +++++++++++++++++++++
>> include/linux/hisi_lpc.h | 83 ++++++
>> 6 files changed, 399 insertions(+)
>
> This should not be a misc driver.
I an not sure which subsystem to place, do you have any sugguestion?
>
>> create mode 100644 Documentation/devicetree/bindings/misc/hisilicon,low-pin-count.txt
>> create mode 100644 drivers/misc/hisi_lpc.c
>> create mode 100644 include/linux/hisi_lpc.h
>> diff --git a/Documentation/devicetree/bindings/misc/hisilicon,low-pin-count.txt b/Documentation/devicetree/bindings/misc/hisilicon,low-pin-count.txt
>> new file mode 100644
>> index 0000000..05c1e19
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/misc/hisilicon,low-pin-count.txt
>> @@ -0,0 +1,11 @@
>> +Hisilicon Low Pin Count bus
>> +
>> +Required properties
>> +- compatible: "hisilicon,low-pin-count"
>> +- reg specifies low pin count address range
>> +
>> +Example:
>> + lpc_0: lpc@a01b0000 {
>> + compatible = "hisilicon,low-pin-count";
>> + ret = <0x0 0xa01b0000, 0x0, 0x10000>;
>> + };
>
> The name is too generic, unless you can guarantee that Hisilicon has never
> before made another implementation of an LPC interface, and never will
> again.
OK, I will fix it.
>
> I think you should create a child address space here using a
> '#address-cells' and '#size-cells'.
There are some mistake,it should be wrote like:
reg = <0x0 0xa01b0000 0x0 0x10000>;
>
>> +#define LPC_REG_READ(reg, result) ((result) = readl(reg))
>> +
>> +#define LPC_REG_WRITE(reg, data) writel((data), (reg))
>
> Remove the obfuscation here.
OK
>
>> +struct hs_lpc_dev *lpc_dev;
>
> Avoid global data structures.
>
OK
>> + LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_IRQ_ST, HS_LPC_IRQ_CLEAR);
>> + retry = 0;
>> + while (0 == (LPC_REG_READ(lpc_dev->regs + HS_LPC_REG_OP_STATUS,
>> + lpc_op_state_value) & HS_LPC_STATUS_DILE)) {
>> + udelay(1);
>> + retry++;
>> + if (retry >= 10000) {
>> + dev_err(lpc_dev->dev, "lpc W, wait idle time out\n");
>> + return -ETIME;
>> + }
>> + }
>
> Better release the spinlock here and call a sleeping function for the wait.
> If the timeout is 10ms, you definitely don't want to keep interrupts disabled
> the whole time.
>
> If you can't find a good way to retry after getting the lock back, maybe
> use a mutex here that you can keep locked the whole time.
>
The interface "lpc_io_read_byte" may be called in IRQ context by UART driver,
and in process context by ipmi driver.
>> +void lpc_io_write_byte(u8 value, unsigned long addr)
>> +{
>> + unsigned long flags;
>> + int ret;
>> +
>> + if (!lpc_dev) {
>> + pr_err("device is not register\n!");
>> + return;
>> + }
>> + spin_lock_irqsave(&lpc_dev->lock, flags);
>> + ret = lpc_master_write(HS_LPC_CMD_SAMEADDR_SING, HS_LPC_CMD_TYPE_IO,
>> + addr, &value, 1);
>> + spin_unlock_irqrestore(&lpc_dev->lock, flags);
>> +}
>> +EXPORT_SYMBOL(lpc_io_write_byte);
>
> Using your own accessor functions sounds wrong here. What you have
> is essentially a PCI I/O space, right? As much as we all hate I/O
> space (in particular the kind that is not memory mapped), I think this
> should be hooked up to the generic inb/outb functions to allow
> all the generic device drivers to work.
It is not a PCI I/O space, although we want access it like IO space.
Could you explain how to hook up to the generic inb/outb functions.
>
>> diff --git a/include/linux/hisi_lpc.h b/include/linux/hisi_lpc.h
>> new file mode 100644
>> index 0000000..4cf93ee
>> --- /dev/null
>> +++ b/include/linux/hisi_lpc.h
>
> Don't do a global header here, just move it into the main file.
>
Because in previous design, the uart driver should call lpc_io_write_byte
and lpc_io_write_byte. the header file must be included in uart_driver.c to
access its exported interface.
> Arnd
>
> .
>
Thanks for your comment.
Rongrong
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Hisilicon LPC driver
2015-12-01 7:58 ` Rongrong Zou
@ 2015-12-01 10:00 ` Arnd Bergmann
2015-12-02 10:11 ` Rongrong Zou
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2015-12-01 10:00 UTC (permalink / raw)
To: Rongrong Zou
Cc: Rongrong Zou, gregkh, lixiancai, linux-kernel, lijianhua,
linuxarm, minyard, lijianhua, Will Deacon, Catalin Marinas
On Tuesday 01 December 2015 15:58:36 Rongrong Zou wrote:
> 在 2015/11/30 21:19, Arnd Bergmann 写道:
> > On Monday 30 November 2015 21:07:17 Rongrong Zou wrote:
> >> This is the Low Pin Count driver for Hisilicon Hi1610 SoC. It is used
> >> for LPC master accessing LPC slave device.
> >>
> >> We only implement I/O read and I/O write here, and the 2 interfaces are
> >> exported for uart driver and ipmi_si driver.
> >>
> >> Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
> >> Signed-off-by: lijianhua <Jueying0518@gmail.com>
> >> ---
> >> .../bindings/misc/hisilicon,low-pin-count.txt | 11 +
> >> MAINTAINERS | 5 +
> >> drivers/misc/Kconfig | 7 +
> >> drivers/misc/Makefile | 1 +
> >> drivers/misc/hisi_lpc.c | 292 +++++++++++++++++++++
> >> include/linux/hisi_lpc.h | 83 ++++++
> >> 6 files changed, 399 insertions(+)
> >
> > This should not be a misc driver.
>
> I an not sure which subsystem to place, do you have any sugguestion?
It depends a bit on how things evolve. Try putting it into arch/arm64/kernel/
for the sake of discussion, and we can find a better place once we are
converging on an implementation.
> >> +Example:
> >> + lpc_0: lpc@a01b0000 {
> >> + compatible = "hisilicon,low-pin-count";
> >> + ret = <0x0 0xa01b0000, 0x0, 0x10000>;
> >> + };
> >
> >
> > I think you should create a child address space here using a
> > '#address-cells' and '#size-cells'.
>
> There are some mistake,it should be wrote like:
> reg = <0x0 0xa01b0000 0x0 0x10000>;
I saw that too but my comment was unrelated. What I meant is that
you should list the fact that there is a child address space and
that you might have devices attached to the LPC using the #address-cells
property.
> >> + LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_IRQ_ST, HS_LPC_IRQ_CLEAR);
> >> + retry = 0;
> >> + while (0 == (LPC_REG_READ(lpc_dev->regs + HS_LPC_REG_OP_STATUS,
> >> + lpc_op_state_value) & HS_LPC_STATUS_DILE)) {
> >> + udelay(1);
> >> + retry++;
> >> + if (retry >= 10000) {
> >> + dev_err(lpc_dev->dev, "lpc W, wait idle time out\n");
> >> + return -ETIME;
> >> + }
> >> + }
> >
> > Better release the spinlock here and call a sleeping function for the wait.
> > If the timeout is 10ms, you definitely don't want to keep interrupts disabled
> > the whole time.
> >
> > If you can't find a good way to retry after getting the lock back, maybe
> > use a mutex here that you can keep locked the whole time.
> >
>
> The interface "lpc_io_read_byte" may be called in IRQ context by UART driver,
> and in process context by ipmi driver.
inb/outb cannot return an error though, so the timeout handling will
have to change.
How did you determine the 10ms timeout? What is the scenario in which
the bus takes an extended time, or times out?
Note that you are not allowed to use dev_err() from a low-level I/O
accessor if that can be used by the UART driver for the console, otherwise
you get an instant deadlock here.
> >> +void lpc_io_write_byte(u8 value, unsigned long addr)
> >> +{
> >> + unsigned long flags;
> >> + int ret;
> >> +
> >> + if (!lpc_dev) {
> >> + pr_err("device is not register\n!");
> >> + return;
> >> + }
> >> + spin_lock_irqsave(&lpc_dev->lock, flags);
> >> + ret = lpc_master_write(HS_LPC_CMD_SAMEADDR_SING, HS_LPC_CMD_TYPE_IO,
> >> + addr, &value, 1);
> >> + spin_unlock_irqrestore(&lpc_dev->lock, flags);
> >> +}
> >> +EXPORT_SYMBOL(lpc_io_write_byte);
> >
> > Using your own accessor functions sounds wrong here. What you have
> > is essentially a PCI I/O space, right? As much as we all hate I/O
> > space (in particular the kind that is not memory mapped), I think this
> > should be hooked up to the generic inb/outb functions to allow
> > all the generic device drivers to work.
>
> It is not a PCI I/O space, although we want access it like IO space.
> Could you explain how to hook up to the generic inb/outb functions.
It's the same thing really, and we really want all I/O space to show
up in /proc/ioports and be accessible through a common interface.
As this is supposed to be ISA compatible, I think you may want to
enforce this one to come first, so all ISA drivers see the respective
devices at low port numbers that may be hardwired. This is also required
for ISAPNP operation.
I would expect that the I/O space on your LPC bus is muxed with the
PCI I/O space as it is typically done for x86 machines as well, can
you check if that is the case?
We have a similarly broken bus bridge on one of the PowerPC implementations,
so you could have a look at the code in arch/powerpc/kernel/io-workarounds.c
for this.
> >> diff --git a/include/linux/hisi_lpc.h b/include/linux/hisi_lpc.h
> >> new file mode 100644
> >> index 0000000..4cf93ee
> >> --- /dev/null
> >> +++ b/include/linux/hisi_lpc.h
> >
> > Don't do a global header here, just move it into the main file.
> >
> Because in previous design, the uart driver should call lpc_io_write_byte
> and lpc_io_write_byte. the header file must be included in uart_driver.c to
> access its exported interface.
I think it should go through linux/io.h instead, using the inb/outb
interface.
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Hisilicon LPC driver
2015-12-01 10:00 ` Arnd Bergmann
@ 2015-12-02 10:11 ` Rongrong Zou
2015-12-02 13:36 ` Arnd Bergmann
0 siblings, 1 reply; 6+ messages in thread
From: Rongrong Zou @ 2015-12-02 10:11 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rongrong Zou, gregkh, lixiancai, linux-kernel, lijianhua,
linuxarm, minyard, lijianhua, Will Deacon, Catalin Marinas
在 2015/12/1 18:00, Arnd Bergmann 写道:
> On Tuesday 01 December 2015 15:58:36 Rongrong Zou wrote:
>> 在 2015/11/30 21:19, Arnd Bergmann 写道:
>>> On Monday 30 November 2015 21:07:17 Rongrong Zou wrote:
>>>> This is the Low Pin Count driver for Hisilicon Hi1610 SoC. It is used
>>>> for LPC master accessing LPC slave device.
>>>>
>>>> We only implement I/O read and I/O write here, and the 2 interfaces are
>>>> exported for uart driver and ipmi_si driver.
>>>>
>>>> Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
>>>> Signed-off-by: lijianhua <Jueying0518@gmail.com>
>>>> ---
>>>> .../bindings/misc/hisilicon,low-pin-count.txt | 11 +
>>>> MAINTAINERS | 5 +
>>>> drivers/misc/Kconfig | 7 +
>>>> drivers/misc/Makefile | 1 +
>>>> drivers/misc/hisi_lpc.c | 292 +++++++++++++++++++++
>>>> include/linux/hisi_lpc.h | 83 ++++++
>>>> 6 files changed, 399 insertions(+)
>>>
>>> This should not be a misc driver.
>>
>> I an not sure which subsystem to place, do you have any sugguestion?
>
> It depends a bit on how things evolve. Try putting it into arch/arm64/kernel/
> for the sake of discussion, and we can find a better place once we are
> converging on an implementation.
>
>
>>>> +Example:
>>>> + lpc_0: lpc@a01b0000 {
>>>> + compatible = "hisilicon,low-pin-count";
>>>> + ret = <0x0 0xa01b0000, 0x0, 0x10000>;
>>>> + };
>>>
>>>
>>> I think you should create a child address space here using a
>>> '#address-cells' and '#size-cells'.
>>
>> There are some mistake,it should be wrote like:
>> reg = <0x0 0xa01b0000 0x0 0x10000>;
>
> I saw that too but my comment was unrelated. What I meant is that
> you should list the fact that there is a child address space and
> that you might have devices attached to the LPC using the #address-cells
> property.
>
>>>> + LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_IRQ_ST, HS_LPC_IRQ_CLEAR);
>>>> + retry = 0;
>>>> + while (0 == (LPC_REG_READ(lpc_dev->regs + HS_LPC_REG_OP_STATUS,
>>>> + lpc_op_state_value) & HS_LPC_STATUS_DILE)) {
>>>> + udelay(1);
>>>> + retry++;
>>>> + if (retry >= 10000) {
>>>> + dev_err(lpc_dev->dev, "lpc W, wait idle time out\n");
>>>> + return -ETIME;
>>>> + }
>>>> + }
>>>
>>> Better release the spinlock here and call a sleeping function for the wait.
>>> If the timeout is 10ms, you definitely don't want to keep interrupts disabled
>>> the whole time.
>>>
>>> If you can't find a good way to retry after getting the lock back, maybe
>>> use a mutex here that you can keep locked the whole time.
>>>
>>
>> The interface "lpc_io_read_byte" may be called in IRQ context by UART driver,
>> and in process context by ipmi driver.
>
> inb/outb cannot return an error though, so the timeout handling will
> have to change.
>
> How did you determine the 10ms timeout? What is the scenario in which
> the bus takes an extended time, or times out?
I check the SoC design,the bus hardware wait 65536 cycles(33M clock) befor time out.
It is about 2ms long, so 10ms is a too long time. Absence of the connected device will
cause the time out.
>
> Note that you are not allowed to use dev_err() from a low-level I/O
> accessor if that can be used by the UART driver for the console, otherwise
> you get an instant deadlock here.
>
>>>> +void lpc_io_write_byte(u8 value, unsigned long addr)
>>>> +{
>>>> + unsigned long flags;
>>>> + int ret;
>>>> +
>>>> + if (!lpc_dev) {
>>>> + pr_err("device is not register\n!");
>>>> + return;
>>>> + }
>>>> + spin_lock_irqsave(&lpc_dev->lock, flags);
>>>> + ret = lpc_master_write(HS_LPC_CMD_SAMEADDR_SING, HS_LPC_CMD_TYPE_IO,
>>>> + addr, &value, 1);
>>>> + spin_unlock_irqrestore(&lpc_dev->lock, flags);
>>>> +}
>>>> +EXPORT_SYMBOL(lpc_io_write_byte);
>>>
>>> Using your own accessor functions sounds wrong here. What you have
>>> is essentially a PCI I/O space, right? As much as we all hate I/O
>>> space (in particular the kind that is not memory mapped), I think this
>>> should be hooked up to the generic inb/outb functions to allow
>>> all the generic device drivers to work.
>>
>> It is not a PCI I/O space, although we want access it like IO space.
>> Could you explain how to hook up to the generic inb/outb functions.
>
> It's the same thing really, and we really want all I/O space to show
> up in /proc/ioports and be accessible through a common interface.
> As this is supposed to be ISA compatible, I think you may want to
> enforce this one to come first, so all ISA drivers see the respective
> devices at low port numbers that may be hardwired. This is also required
> for ISAPNP operation.
This is what i want, but i still have some problem with the implemention.
Do you mean I should redefine inb/outb in arch/arm64/kernel?
My sulotion: redefine inb(addr)
inb(addr)
{
if (addr is legacy_io_addr) {
call lpc_io_inb
}
else {
call readb(PCI_IOBASE + addr);
}
}
>
> I would expect that the I/O space on your LPC bus is muxed with the
> PCI I/O space as it is typically done for x86 machines as well, can
> you check if that is the case?
The legacy IO space can be reserved When we request IO resource in PCI in
our platform, but I'm not sure it can be done in other ARM SoC. The legacy
ISA IO is specified in PC99 specification,not in ARM.
>
> We have a similarly broken bus bridge on one of the PowerPC implementations,
> so you could have a look at the code in arch/powerpc/kernel/io-workarounds.c
> for this.
>
I should spend more time to catch on.
>>>> diff --git a/include/linux/hisi_lpc.h b/include/linux/hisi_lpc.h
>>>> new file mode 100644
>>>> index 0000000..4cf93ee
>>>> --- /dev/null
>>>> +++ b/include/linux/hisi_lpc.h
>>>
>>> Don't do a global header here, just move it into the main file.
>>>
>> Because in previous design, the uart driver should call lpc_io_write_byte
>> and lpc_io_write_byte. the header file must be included in uart_driver.c to
>> access its exported interface.
>
> I think it should go through linux/io.h instead, using the inb/outb
> interface.
>
> Arnd
>
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Hisilicon LPC driver
2015-12-02 10:11 ` Rongrong Zou
@ 2015-12-02 13:36 ` Arnd Bergmann
0 siblings, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2015-12-02 13:36 UTC (permalink / raw)
To: Rongrong Zou
Cc: Rongrong Zou, gregkh, lixiancai, linux-kernel, lijianhua,
linuxarm, minyard, lijianhua, Will Deacon, Catalin Marinas
On Wednesday 02 December 2015 18:11:14 Rongrong Zou wrote:
> 在 2015/12/1 18:00, Arnd Bergmann 写道:
> > On Tuesday 01 December 2015 15:58:36 Rongrong Zou wrote:
> >> 在 2015/11/30 21:19, Arnd Bergmann 写道:
> >>> On Monday 30 November 2015 21:07:17 Rongrong Zou wrote:
> >>>> + LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_IRQ_ST, HS_LPC_IRQ_CLEAR);
> >>>> + retry = 0;
> >>>> + while (0 == (LPC_REG_READ(lpc_dev->regs + HS_LPC_REG_OP_STATUS,
> >>>> + lpc_op_state_value) & HS_LPC_STATUS_DILE)) {
> >>>> + udelay(1);
> >>>> + retry++;
> >>>> + if (retry >= 10000) {
> >>>> + dev_err(lpc_dev->dev, "lpc W, wait idle time out\n");
> >>>> + return -ETIME;
> >>>> + }
> >>>> + }
> >>>
> >>> Better release the spinlock here and call a sleeping function for the wait.
> >>> If the timeout is 10ms, you definitely don't want to keep interrupts disabled
> >>> the whole time.
> >>>
> >>> If you can't find a good way to retry after getting the lock back, maybe
> >>> use a mutex here that you can keep locked the whole time.
> >>>
> >>
> >> The interface "lpc_io_read_byte" may be called in IRQ context by UART driver,
> >> and in process context by ipmi driver.
> >
> > inb/outb cannot return an error though, so the timeout handling will
> > have to change.
> >
> > How did you determine the 10ms timeout? What is the scenario in which
> > the bus takes an extended time, or times out?
>
> I check the SoC design,the bus hardware wait 65536 cycles(33M clock) befor time out.
> It is about 2ms long, so 10ms is a too long time. Absence of the connected device will
> cause the time out.
So you mean any access to an I/O port that isn't there can take several
milliseconds? That does seem really long still (though slightly better than
10ms or more that you are waiting above).
> >> It is not a PCI I/O space, although we want access it like IO space.
> >> Could you explain how to hook up to the generic inb/outb functions.
> >
> > It's the same thing really, and we really want all I/O space to show
> > up in /proc/ioports and be accessible through a common interface.
> > As this is supposed to be ISA compatible, I think you may want to
> > enforce this one to come first, so all ISA drivers see the respective
> > devices at low port numbers that may be hardwired. This is also required
> > for ISAPNP operation.
>
> This is what i want, but i still have some problem with the implemention.
> Do you mean I should redefine inb/outb in arch/arm64/kernel?
>
> My sulotion: redefine inb(addr)
> inb(addr)
> {
> if (addr is legacy_io_addr) {
> call lpc_io_inb
> }
> else {
> call readb(PCI_IOBASE + addr);
> }
> }
Yes, that would work, but also needs some if(IS_ENABLED(CONFIG_BROKEN_IOPORT))
etc. We probably don't want to enable this by default, only when building
a kernel for this SoC.
I would also make it a callback pointer, so your driver can be a
loadable module, and we can have different implementations in case
someone else also has their own incompatible LPC or PCI host bridge.
> > I would expect that the I/O space on your LPC bus is muxed with the
> > PCI I/O space as it is typically done for x86 machines as well, can
> > you check if that is the case?
>
> The legacy IO space can be reserved When we request IO resource in PCI in
> our platform, but I'm not sure it can be done in other ARM SoC. The legacy
> ISA IO is specified in PC99 specification,not in ARM.
LPC is a standard bus, and is implemented in all kinds of hardware, it has
nothing to do with the CPU architecture. Usually it is part of the PCI
host bridge, but it can also be a separate PCIe chip.
Reserving the whole legacy range is problematic if you also want to have
a VGA card, as the VGA BIOS (e.g. for nvidia) typically needs to access
some of the legacy VGA I/O ports for its POST. If the hardware doesn't
mux between the LPC and PCI I/O ports, you may have to do this in your
driver, so it tries to access both.
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-12-02 13:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-30 13:07 [PATCH] Hisilicon LPC driver Rongrong Zou
2015-11-30 13:19 ` Arnd Bergmann
2015-12-01 7:58 ` Rongrong Zou
2015-12-01 10:00 ` Arnd Bergmann
2015-12-02 10:11 ` Rongrong Zou
2015-12-02 13:36 ` Arnd Bergmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).