Re: [PATCH] Hisilicon LPC driver

[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?

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
--
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/