Message-ID: <565EC3C2.1010609@huawei.com>
Date:	Wed, 2 Dec 2015 18:11:14 +0800
From:	Rongrong Zou <zourongrong@...wei.com>
To:	Arnd Bergmann <arnd@...db.de>
CC:	Rongrong Zou <zourongrong@...il.com>, <gregkh@...uxfoundation.org>,
	<lixiancai@...wei.com>, <linux-kernel@...r.kernel.org>,
	<lijianhua@...wei.com>, <linuxarm@...wei.com>, <minyard@....org>,
	lijianhua <Jueying0518@...il.com>,
	Will Deacon <will.deacon@....com>,
	Catalin Marinas <catalin.marinas@....com>
Subject: Re: [PATCH] Hisilicon LPC driver

在 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@...il.com>
>>>> Signed-off-by: lijianhua <Jueying0518@...il.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@...b0000 {
>>>> +		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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists