Re: [PATCH] Hisilicon LPC driver
From: Rongrong Zou
Date: Wed Dec 02 2015 - 05:11:56 EST
å 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@xxxxxxxxx>
Signed-off-by: lijianhua <Jueying0518@xxxxxxxxx>
---
.../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
.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/