Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1280625 > unrolled thread

Re: [PATCH] Hisilicon LPC driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] Hisilicon LPC driver Rongrong Zou <zourongrong@huawei.com> - 2015-12-01 09:00 +0100
    Re: [PATCH] Hisilicon LPC driver Arnd Bergmann <arnd@arndb.de> - 2015-12-01 11:10 +0100
      Re: [PATCH] Hisilicon LPC driver Rongrong Zou <zourongrong@huawei.com> - 2015-12-02 11:20 +0100
        Re: [PATCH] Hisilicon LPC driver Arnd Bergmann <arnd@arndb.de> - 2015-12-02 14:40 +0100

#1280625 — Re: [PATCH] Hisilicon LPC driver

在 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1280724

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281587

在 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
>
> .
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281784

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web