Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1298989 > unrolled thread
| Started by | Rongrong Zou <zourongrong@gmail.com> |
|---|---|
| First post | 2015-12-29 14:40 +0100 |
| Last post | 2016-01-14 14:20 +0100 |
| Articles | 20 on this page of 57 — 7 participants |
Back to article view | Back to linux.kernel
[PATCH v1 0/3] ARM64 LPC: legacy ISA I/O support Rongrong Zou <zourongrong@gmail.com> - 2015-12-29 14:40 +0100
[PATCH v1 2/3] ARM64 LPC: LPC driver implementation Rongrong Zou <zourongrong@gmail.com> - 2015-12-29 14:40 +0100
Re: [PATCH v1 2/3] ARM64 LPC: LPC driver implementation Arnd Bergmann <arnd@arndb.de> - 2015-12-29 15:00 +0100
Re: [PATCH v1 2/3] ARM64 LPC: LPC driver implementation Rongrong Zou <zourongrong@huawei.com> - 2015-12-29 15:10 +0100
Re: [PATCH v1 2/3] ARM64 LPC: LPC driver implementation Arnd Bergmann <arnd@arndb.de> - 2015-12-29 15:20 +0100
[PATCH v1 1/3] ARM64 LPC: indirect ISA PORT IO introduced Rongrong Zou <zourongrong@gmail.com> - 2015-12-29 14:40 +0100
Re: [PATCH v1 1/3] ARM64 LPC: indirect ISA PORT IO introduced Arnd Bergmann <arnd@arndb.de> - 2015-12-29 14:50 +0100
Re: [PATCH v1 1/3] ARM64 LPC: indirect ISA PORT IO introduced Rongrong Zou <zourongrong@huawei.com> - 2015-12-29 15:30 +0100
Re: [PATCH v1 1/3] ARM64 LPC: indirect ISA PORT IO introduced Arnd Bergmann <arnd@arndb.de> - 2015-12-29 15:40 +0100
Re: [PATCH v1 1/3] ARM64 LPC: indirect ISA PORT IO introduced Rongrong Zou <zourongrong@huawei.com> - 2015-12-30 02:40 +0100
Re: [PATCH v1 1/3] ARM64 LPC: indirect ISA PORT IO introduced Arnd Bergmann <arnd@arndb.de> - 2015-12-30 10:00 +0100
Re: [PATCH v1 1/3] ARM64 LPC: indirect ISA PORT IO introduced Rongrong Zou <zourongrong@huawei.com> - 2015-12-30 10:40 +0100
Re: [PATCH v1 1/3] ARM64 LPC: indirect ISA PORT IO introduced Arnd Bergmann <arnd@arndb.de> - 2015-12-30 10:50 +0100
Re: [PATCH v1 1/3] ARM64 LPC: indirect ISA PORT IO introduced Will Deacon <will.deacon@arm.com> - 2016-01-04 11:20 +0100
Re: [PATCH v1 1/3] ARM64 LPC: indirect ISA PORT IO introduced Rongrong Zou <zourongrong@huawei.com> - 2016-01-04 11:30 +0100
[PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@gmail.com> - 2015-12-29 14:40 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2015-12-29 15:00 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2015-12-30 10:10 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@huawei.com> - 2015-12-31 15:20 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2015-12-31 15:50 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@huawei.com> - 2016-01-03 13:30 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2016-01-04 12:20 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@gmail.com> - 2016-01-04 17:00 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2016-01-04 17:40 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@huawei.com> - 2016-01-05 13:10 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2016-01-05 13:30 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@huawei.com> - 2016-01-06 14:40 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@huawei.com> - 2016-01-07 04:40 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rolland Chau <zourongrong@gmail.com> - 2016-01-10 10:40 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@gmail.com> - 2016-01-10 14:40 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc liviu.dudau@arm.com - 2016-01-11 17:20 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@gmail.com> - 2016-01-12 03:40 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc liviu.dudau@arm.com - 2016-01-12 10:10 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@huawei.com> - 2016-01-12 10:30 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc liviu.dudau@arm.com - 2016-01-12 11:20 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@huawei.com> - 2016-01-12 12:10 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc liviu.dudau@arm.com - 2016-01-12 12:30 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@huawei.com> - 2016-01-12 13:00 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc liviu.dudau@arm.com - 2016-01-12 16:20 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2016-01-13 00:00 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-01-13 07:20 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@gmail.com> - 2016-01-13 07:40 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2016-01-13 10:30 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2016-01-13 11:20 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc liviu.dudau@arm.com - 2016-01-13 11:40 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc liviu.dudau@arm.com - 2016-01-13 11:20 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2016-01-13 00:00 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc liviu.dudau@arm.com - 2016-01-13 11:10 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2016-01-13 11:40 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@gmail.com> - 2016-01-13 12:10 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc liviu.dudau@arm.com - 2016-01-13 12:30 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-01-14 01:20 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@gmail.com> - 2016-01-14 03:10 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-01-14 04:50 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@gmail.com> - 2016-01-14 05:50 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-01-14 12:30 +0100
Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@gmail.com> - 2016-01-14 14:20 +0100
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
| From | Rongrong Zou <zourongrong@huawei.com> |
|---|---|
| Date | 2016-01-03 13:30 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qMPXk-5nR-5@gated-at.bofh.it> |
| In reply to | #1299676 |
在 2015/12/31 23:00, Rongrong Zou 写道:
>
>
> 2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd@arndb.de <mailto:arnd@arndb.de>>:
> >
> > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
> > > 在 2015/12/30 17:06, Arnd Bergmann 写道:
> > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
> > > >> +Example:
> > > >> +
> > > >> + lpc_0: lpc@a01b0000 {
> > > >> + #address-cells = <1>;
> > > >> + #size-cells = <1>;
> > > >> + compatible = "low-pin-count";
> > > >> + reg = <0x0 0xa01b0000 0x0 0x10000>;
> > > >> + };
> > > >
> > > > One more thought: please try to stick as closely as possible to the existing
> > > > ISA binding that is documented at
> > > >
> > > > http://www.firmware.org/1275/bindings/isa/isa0_4d.ps
> > > From the specification, I think I should use 2 32bit integer to describe the isa addr in dts.
> > > >
> > > > In particular, this should cover the possibility of describing both memory
> > > > and I/O spaces in child devices.
> > > >
> > >
> > > I found below config in powerpc dts "arch/powerpc/boot/dts/mpc8544ds.dts"
> > >
> > > isa@1e {
> > > device_type = "isa";
> > > #interrupt-cells = <2>;
> > > #size-cells = <1>;
> > > #address-cells = <2>;
> > > reg = <0xf000 0x0 0x0 0x0 0x0>;
> > > ranges = <0x1 0x0 0x1000000 0x0 0x0
> > > 0x1000>;
> > > interrupt-parent = <&i8259>;
> > >
> > >
> > >
> > > rtc@70 {
> > > compatible = "pnpPNP,b00";
> > > reg = <0x1 0x70 0x2>;
> > > };
> > > the isa space in child-node: reg = <0x1 0x70 0x2>;
> > > 0x1 means IO space, 70 means addr, 0x2 is size.
> > > but when i config the following in dts, the ipmi_0 node can't be probed,
> > > I think there may be some problems.
> > >
> > > lpc_0: lpc@a01b0000 {
> > > compatible = "low-pin-count";
> > > device_type = "isa";
> > > #address-cells = <2>;
> > > #size-cells = <1>;
> > > reg = <0x0 0xa01b0000 0x0 0x10000>;
> > >
> > > ipmi_0:ipmi@000000e4{
> > > device_type = "ipmi";
> > > compatible = "ipmi-bt";
> > > reg = <0x1 0x000000e4 0x4>;
> > > };
> >
> > The DT sample above looks good in principle. I believe what you are missing
> > here is code in your driver to scan the child nodes to create the platform
> > devices. of_bus_isa_translate() should work with your definition here
> > and create the correct IORESOURCE_IO resources. You don't have any MMIO
> > resources, so the absence of a ranges property is ok. Maybe all you
> > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
> >
>
> You are right. thanks, i'll try on test board . if i get the correct result , the new patch
> will be sent later. By the way, it's my another email account use when i at home.
I tried, and there need some additional changes.
isa@a01b0000 {
/*the node name should start with "isa", because of below definition
* static int of_bus_isa_match(struct device_node *np)
* {
* return !strcmp(np->name, "isa");
* }
*/
compatible = "low-pin-count";
device_type = "isa";
#address-cells = <2>;
#size-cells = <1>;
reg = <0x0 0xa01b0000 0x0 0x10000>;
ranges = <0x1 0x0 0x0 0x0 0x1000>;
/*
* ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
*
*/
ipmi_0:ipmi@000000e4{
device_type = "ipmi";
compatible = "ipmi-bt";
reg = <0x1 0x000000e4 0x4>;
};
drivers\of\address.c
static int __of_address_to_resource(struct device_node *dev,
const __be32 *addrp, u64 size, unsigned int flags,
const char *name, struct resource *r)
{
u64 taddr;
if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
return -EINVAL;
taddr = of_translate_address(dev, addrp);
if (taddr == OF_BAD_ADDR)
return -EINVAL;
memset(r, 0, sizeof(struct resource));
if (flags & IORESOURCE_IO) {
unsigned long port;
/*****************************************************************/
/*legacy port(< 0x1000) is reserved, and need no translation here*/
/*****************************************************************/
if(taddr + size < PCIBIOS_MIN_IO){
r->start = taddr;
r->end = taddr + size - 1;
}
else{
port = pci_address_to_pio(taddr);
if (port == (unsigned long)-1)
return -EINVAL;
r->start = port;
r->end = port + size - 1;
}
} else {
r->start = taddr;
r->end = taddr + size - 1;
}
r->flags = flags;
r->name = name ? name : dev->full_name;
return 0;
}
>
> > 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]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-01-04 12:20 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qNbl8-2J8-5@gated-at.bofh.it> |
| In reply to | #1300292 |
On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
> 在 2015/12/31 23:00, Rongrong Zou 写道:
> > 2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd@arndb.de <mailto:arnd@arndb.de>>:
> > > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
> > > > 在 2015/12/30 17:06, Arnd Bergmann 写道:
> > > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
> > >
> > > The DT sample above looks good in principle. I believe what you are missing
> > > here is code in your driver to scan the child nodes to create the platform
> > > devices. of_bus_isa_translate() should work with your definition here
> > > and create the correct IORESOURCE_IO resources. You don't have any MMIO
> > > resources, so the absence of a ranges property is ok. Maybe all you
> > > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
> > >
> >
> > You are right. thanks, i'll try on test board . if i get the correct result , the new patch
> > will be sent later. By the way, it's my another email account use when i at home.
>
> I tried, and there need some additional changes.
>
> isa@a01b0000 {
>
> /*the node name should start with "isa", because of below definition
> * static int of_bus_isa_match(struct device_node *np)
> * {
> * return !strcmp(np->name, "isa");
> * }
Looks good. It would be nicer to match on device_type than on name,
but this is ancient code and it's probably best not to touch it
so we don't accidentally break some old SPARC or PPC system.
> */
> compatible = "low-pin-count";
> device_type = "isa";
> #address-cells = <2>;
> #size-cells = <1>;
> reg = <0x0 0xa01b0000 0x0 0x10000>;
> ranges = <0x1 0x0 0x0 0x0 0x1000>;
> /*
> * ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
> *
> */
> ipmi_0:ipmi@000000e4{
> device_type = "ipmi";
> compatible = "ipmi-bt";
> reg = <0x1 0x000000e4 0x4>;
> };
>
This looks wrong: the property above says that the I/O port range is
translated to MMIO address 0x00000000 to 0x00010000, which is not
true on your hardware. I think this needs to be changed in the code
so the ranges property is not required for I/O ports.
> drivers\of\address.c
> static int __of_address_to_resource(struct device_node *dev,
> const __be32 *addrp, u64 size, unsigned int flags,
> const char *name, struct resource *r)
> {
> u64 taddr;
>
> if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> return -EINVAL;
> taddr = of_translate_address(dev, addrp);
> if (taddr == OF_BAD_ADDR)
> return -EINVAL;
> memset(r, 0, sizeof(struct resource));
> if (flags & IORESOURCE_IO) {
> unsigned long port;
>
> /*****************************************************************/
> /*legacy port(< 0x1000) is reserved, and need no translation here*/
> /*****************************************************************/
> if(taddr + size < PCIBIOS_MIN_IO){
> r->start = taddr;
> r->end = taddr + size - 1;
> }
I don't like having a special case based on the address here,
the same kind of hack might be needed for PCI I/O spaces in
hardware that uses an indirect method like your LPC bus
does, and the code above will not work on any LPC implementation
that correctly multiplexes its I/O ports with the first PCI domain.
I think it would be better to avoid translating the port into
a physical address to start with just to translate it back into
a port number, what we need instead is the offset between the
bus specific port number and the linux port number. I've added
Liviu to Cc, he wrote this code originally and may have some idea
of how we could do that.
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]
| From | Rongrong Zou <zourongrong@gmail.com> |
|---|---|
| Date | 2016-01-04 17:00 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qNfI8-5oL-31@gated-at.bofh.it> |
| In reply to | #1300636 |
在 2016/1/4 19:13, Arnd Bergmann 写道:
> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>> 在 2015/12/31 23:00, Rongrong Zou 写道:
>>> 2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd@arndb.de <mailto:arnd@arndb.de>>:
>>> > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
>>> > > 在 2015/12/30 17:06, Arnd Bergmann 写道:
>>> > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
>>> >
>>> > The DT sample above looks good in principle. I believe what you are missing
>>> > here is code in your driver to scan the child nodes to create the platform
>>> > devices. of_bus_isa_translate() should work with your definition here
>>> > and create the correct IORESOURCE_IO resources. You don't have any MMIO
>>> > resources, so the absence of a ranges property is ok. Maybe all you
>>> > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
>>> >
>>>
>>> You are right. thanks, i'll try on test board . if i get the correct result , the new patch
>>> will be sent later. By the way, it's my another email account use when i at home.
>>
>> I tried, and there need some additional changes.
>>
>> isa@a01b0000 {
>>
>> /*the node name should start with "isa", because of below definition
>> * static int of_bus_isa_match(struct device_node *np)
>> * {
>> * return !strcmp(np->name, "isa");
>> * }
>
> Looks good. It would be nicer to match on device_type than on name,
> but this is ancient code and it's probably best not to touch it
> so we don't accidentally break some old SPARC or PPC system.
>
>> */
>> compatible = "low-pin-count";
>> device_type = "isa";
>> #address-cells = <2>;
>> #size-cells = <1>;
>> reg = <0x0 0xa01b0000 0x0 0x10000>;
>> ranges = <0x1 0x0 0x0 0x0 0x1000>;
>> /*
>> * ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
>> *
>> */
>> ipmi_0:ipmi@000000e4{
>> device_type = "ipmi";
>> compatible = "ipmi-bt";
>> reg = <0x1 0x000000e4 0x4>;
>> };
>>
>
> This looks wrong: the property above says that the I/O port range is
> translated to MMIO address 0x00000000 to 0x00010000, which is not
> true on your hardware. I think this needs to be changed in the code
> so the ranges property is not required for I/O ports.
Ranges property can set empty, but this means 1:1 translation. the I/O
port range is translated to MMIO address 0x00000001 00000000 to
0x00000001 00000004, it looks wrong else. I wonder if anyone get legacy
I/O port resource from dts.
For ipmi driver, I can get I/O port resource by DMI rather than dts.
>
>> drivers\of\address.c
>> static int __of_address_to_resource(struct device_node *dev,
>> const __be32 *addrp, u64 size, unsigned int flags,
>> const char *name, struct resource *r)
>> {
>> u64 taddr;
>>
>> if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
>> return -EINVAL;
>> taddr = of_translate_address(dev, addrp);
>> if (taddr == OF_BAD_ADDR)
>> return -EINVAL;
>> memset(r, 0, sizeof(struct resource));
>> if (flags & IORESOURCE_IO) {
>> unsigned long port;
>>
>> /*****************************************************************/
>> /*legacy port(< 0x1000) is reserved, and need no translation here*/
>> /*****************************************************************/
>> if(taddr + size < PCIBIOS_MIN_IO){
>> r->start = taddr;
>> r->end = taddr + size - 1;
>> }
>
> I don't like having a special case based on the address here,
> the same kind of hack might be needed for PCI I/O spaces in
> hardware that uses an indirect method like your LPC bus
> does, and the code above will not work on any LPC implementation
> that correctly multiplexes its I/O ports with the first PCI domain.
>
> I think it would be better to avoid translating the port into
> a physical address to start with just to translate it back into
> a port number, what we need instead is the offset between the
> bus specific port number and the linux port number. I've added
> Liviu to Cc, he wrote this code originally and may have some idea
> of how we could do that.
>
> 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]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-01-04 17:40 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qNgkO-5T6-37@gated-at.bofh.it> |
| In reply to | #1300847 |
On Tuesday 05 January 2016 00:04:19 Rongrong Zou wrote:
> 在 2016/1/4 19:13, Arnd Bergmann 写道:
> > On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
> >> 在 2015/12/31 23:00, Rongrong Zou 写道:
> >> */
> >> compatible = "low-pin-count";
> >> device_type = "isa";
> >> #address-cells = <2>;
> >> #size-cells = <1>;
> >> reg = <0x0 0xa01b0000 0x0 0x10000>;
> >> ranges = <0x1 0x0 0x0 0x0 0x1000>;
> >> /*
> >> * ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
> >> *
> >> */
> >> ipmi_0:ipmi@000000e4{
> >> device_type = "ipmi";
> >> compatible = "ipmi-bt";
> >> reg = <0x1 0x000000e4 0x4>;
> >> };
> >>
> >
> > This looks wrong: the property above says that the I/O port range is
> > translated to MMIO address 0x00000000 to 0x00010000, which is not
> > true on your hardware. I think this needs to be changed in the code
> > so the ranges property is not required for I/O ports.
>
> Ranges property can set empty, but this means 1:1 translation. the I/O
> port range is translated to MMIO address 0x00000001 00000000 to
> 0x00000001 00000004, it looks wrong else. I wonder if anyone get legacy
> I/O port resource from dts.
As I said, nothing should really require the ranges property here, unless
you have a valid IORESOURCE_MEM translation. The code that requires
the ranges to be present is wrong.
> For ipmi driver, I can get I/O port resource by DMI rather than dts.
No, the ipmi driver uses the resource that belongs to the platform
device already, you can't rely on DMI data to be present there.
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]
| From | Rongrong Zou <zourongrong@huawei.com> |
|---|---|
| Date | 2016-01-05 13:10 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qNyB5-2yO-43@gated-at.bofh.it> |
| In reply to | #1300877 |
在 2016/1/5 0:34, Arnd Bergmann 写道:
> On Tuesday 05 January 2016 00:04:19 Rongrong Zou wrote:
>> 在 2016/1/4 19:13, Arnd Bergmann 写道:
>>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>>>> 在 2015/12/31 23:00, Rongrong Zou 写道:
>>>> */
>>>> compatible = "low-pin-count";
>>>> device_type = "isa";
>>>> #address-cells = <2>;
>>>> #size-cells = <1>;
>>>> reg = <0x0 0xa01b0000 0x0 0x10000>;
>>>> ranges = <0x1 0x0 0x0 0x0 0x1000>;
>>>> /*
>>>> * ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
>>>> *
>>>> */
>>>> ipmi_0:ipmi@000000e4{
>>>> device_type = "ipmi";
>>>> compatible = "ipmi-bt";
>>>> reg = <0x1 0x000000e4 0x4>;
>>>> };
>>>>
>>>
>>> This looks wrong: the property above says that the I/O port range is
>>> translated to MMIO address 0x00000000 to 0x00010000, which is not
>>> true on your hardware. I think this needs to be changed in the code
>>> so the ranges property is not required for I/O ports.
>>
>> Ranges property can set empty, but this means 1:1 translation. the I/O
>> port range is translated to MMIO address 0x00000001 00000000 to
>> 0x00000001 00000004, it looks wrong else. I wonder if anyone get legacy
>> I/O port resource from dts.
>
> As I said, nothing should really require the ranges property here, unless
> you have a valid IORESOURCE_MEM translation. The code that requires
> the ranges to be present is wrong.
>
I think the openfirmware(DT) do not support for those unmapped I/O ports, because I
must get resource by calling of_address_to_resource(), which have to call
pci_address_to_pio() when resource type is IORESOURCE_IO. I'm sorry I have no
better idea for this now. Maybe liviu can give me some opinions.
/**
* of_address_to_resource - Translate device tree address and return as resource
*
* Note that if your address is a PIO address, the conversion will fail if
* the physical address can't be internally converted to an IO token with
* pci_address_to_pio(), that is because it's either called to early or it
* can't be matched to any host bridge IO space
*/
int of_address_to_resource(struct device_node *dev, int index,
struct resource *r)
>> For ipmi driver, I can get I/O port resource by DMI rather than dts.
>
> No, the ipmi driver uses the resource that belongs to the platform
> device already, you can't rely on DMI data to be present there.
Ipmi has a lot of way to be discovered(ACPI, DMI, hardcoded, hot-add,
openfirmware and a few other), I think we just use one of them, not all of them.
It depend on vendor's hardware solution actually.
>
> Arnd
>
> .
>
Thanks,
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] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-01-05 13:30 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qNyUq-2G0-1@gated-at.bofh.it> |
| In reply to | #1301477 |
On Tuesday 05 January 2016 19:59:49 Rongrong Zou wrote: > 在 2016/1/5 0:34, Arnd Bergmann 写道: > > On Tuesday 05 January 2016 00:04:19 Rongrong Zou wrote: > >> 在 2016/1/4 19:13, Arnd Bergmann 写道: > >>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote: > >>>> 在 2015/12/31 23:00, Rongrong Zou 写道: > >> Ranges property can set empty, but this means 1:1 translation. the I/O > >> port range is translated to MMIO address 0x00000001 00000000 to > >> 0x00000001 00000004, it looks wrong else. I wonder if anyone get legacy > >> I/O port resource from dts. > > > > As I said, nothing should really require the ranges property here, unless > > you have a valid IORESOURCE_MEM translation. The code that requires > > the ranges to be present is wrong. > > > > I think the openfirmware(DT) do not support for those unmapped I/O ports, because I > must get resource by calling of_address_to_resource(), which have to call > pci_address_to_pio() when resource type is IORESOURCE_IO. I'm sorry I have no > better idea for this now. Maybe liviu can give me some opinions. I think on x86 it works (or used to work, few people use open firmware on x86 these days, and it may be broken), and the pci_address_to_pio() call behaves differently when PCI_IOBASE is set. x86 never maps I/O ports into memory mapped I/O addresses, they have their own way of accessing them just like your platform. > /** > * of_address_to_resource - Translate device tree address and return as resource > * > * Note that if your address is a PIO address, the conversion will fail if > * the physical address can't be internally converted to an IO token with > * pci_address_to_pio(), that is because it's either called to early or it > * can't be matched to any host bridge IO space > */ > int of_address_to_resource(struct device_node *dev, int index, > struct resource *r) The problem here seems to be that the code assumes that either the I/O ports are always mapped or they are never mapped (no PCI_IOBASE). We need to extend it because now we can have the combination of the two. > >> For ipmi driver, I can get I/O port resource by DMI rather than dts. > > > > No, the ipmi driver uses the resource that belongs to the platform > > device already, you can't rely on DMI data to be present there. > > Ipmi has a lot of way to be discovered(ACPI, DMI, hardcoded, hot-add, > openfirmware and a few other), I think we just use one of them, not all of them. > It depend on vendor's hardware solution actually. I don't think we should mix multiple methods here: if the bus is described in DT, all its children should be there as well. Otherwise you get into problems e.g. if you have multiple instances of the LPC bus and the Linux I/O addresses for one or more of them have an offset to the bus specific addresses. The bus probe code decides what the Linux I/O port numbers are, but DMI and other methods have no idea of the mapping. As long as there is only one instance, using the first 0x1000 addresses with a 1:1 mapping saves us a bit of trouble, but I'd be worried about relying on that assumption too much. 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]
| From | Rongrong Zou <zourongrong@huawei.com> |
|---|---|
| Date | 2016-01-06 14:40 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qNWtI-1L8-17@gated-at.bofh.it> |
| In reply to | #1301485 |
在 2016/1/5 20:19, Arnd Bergmann 写道:
> On Tuesday 05 January 2016 19:59:49 Rongrong Zou wrote:
>> 在 2016/1/5 0:34, Arnd Bergmann 写道:
>>> On Tuesday 05 January 2016 00:04:19 Rongrong Zou wrote:
>>>> 在 2016/1/4 19:13, Arnd Bergmann 写道:
>>>>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>>>>>> 在 2015/12/31 23:00, Rongrong Zou 写道:
>>>> Ranges property can set empty, but this means 1:1 translation. the I/O
>>>> port range is translated to MMIO address 0x00000001 00000000 to
>>>> 0x00000001 00000004, it looks wrong else. I wonder if anyone get legacy
>>>> I/O port resource from dts.
>>>
>>> As I said, nothing should really require the ranges property here, unless
>>> you have a valid IORESOURCE_MEM translation. The code that requires
>>> the ranges to be present is wrong.
>>>
>>
>> I think the openfirmware(DT) do not support for those unmapped I/O ports, because I
>> must get resource by calling of_address_to_resource(), which have to call
>> pci_address_to_pio() when resource type is IORESOURCE_IO. I'm sorry I have no
>> better idea for this now. Maybe liviu can give me some opinions.
>
> I think on x86 it works (or used to work, few people use open firmware on
> x86 these days, and it may be broken), and the pci_address_to_pio() call
> behaves differently when PCI_IOBASE is set. x86 never maps I/O ports into
> memory mapped I/O addresses, they have their own way of accessing them
> just like your platform.
>
>> /**
>> * of_address_to_resource - Translate device tree address and return as resource
>> *
>> * Note that if your address is a PIO address, the conversion will fail if
>> * the physical address can't be internally converted to an IO token with
>> * pci_address_to_pio(), that is because it's either called to early or it
>> * can't be matched to any host bridge IO space
>> */
>> int of_address_to_resource(struct device_node *dev, int index,
>> struct resource *r)
>
> The problem here seems to be that the code assumes that either the I/O ports
> are always mapped or they are never mapped (no PCI_IOBASE). We need to extend
> it because now we can have the combination of the two.
I am considering the following solution:
Adding unmapped isa io functions in
drivers/of/address.c,
static LIST_HEAD(legacy_io_range_list);
int isa_register_io_range(phys_addr_t addr, resource_size_t size);
/* before I call isa(LPC) bus driver, the input I/O port must be translated to phys_addr_t
(the least 16bit means port addr on bus, the second 16bit means bus id)*/
phys_addr_t isa_pio_to_bus_addr(unsigned long pio);
/* the returned PIO do not conflict with PIO get from pci_address_to_pio*/
unsigned long isa_bus_addr_to_pio(phys_addr_t address);
drivers/bus/lpc.c
lpc_bus_probe()
{
isa_register_io_range(phys_addr_t addr, resource_size_t size);
}
inb(unsigned long port)
{
unsigned short bus;
phys_addr_t addr;
/*hit isa port range*/
if(addr = isa_pio_to_bus_addr(port))
{
bus = (addr >> 16) & 0xffff;
call lpc driver with addr;
return lpc_read_byte(bus, addr);
}
else /*not hit*/
{
return readb(PCI_IOBASE + port);
}
}
>
>>>> For ipmi driver, I can get I/O port resource by DMI rather than dts.
>>>
>>> No, the ipmi driver uses the resource that belongs to the platform
>>> device already, you can't rely on DMI data to be present there.
>>
>> Ipmi has a lot of way to be discovered(ACPI, DMI, hardcoded, hot-add,
>> openfirmware and a few other), I think we just use one of them, not all of them.
>> It depend on vendor's hardware solution actually.
>
> I don't think we should mix multiple methods here: if the bus is described
> in DT, all its children should be there as well. Otherwise you get into problems
> e.g. if you have multiple instances of the LPC bus and the Linux I/O addresses
> for one or more of them have an offset to the bus specific addresses.
>
> The bus probe code decides what the Linux I/O port numbers are, but DMI
> and other methods have no idea of the mapping. As long as there is only
> one instance, using the first 0x1000 addresses with a 1:1 mapping saves
> us a bit of trouble, but I'd be worried about relying on that assumption
> too much.
>
> Arnd
>
>
> .
>
Thanks,
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] | [prev] | [next] | [standalone]
| From | Rongrong Zou <zourongrong@huawei.com> |
|---|---|
| Date | 2016-01-07 04:40 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qO9AC-2gg-3@gated-at.bofh.it> |
| In reply to | #1302766 |
在 2016/1/6 21:36, Rongrong Zou 写道:
> 在 2016/1/5 20:19, Arnd Bergmann 写道:
>> On Tuesday 05 January 2016 19:59:49 Rongrong Zou wrote:
>>> 在 2016/1/5 0:34, Arnd Bergmann 写道:
>>>> On Tuesday 05 January 2016 00:04:19 Rongrong Zou wrote:
>>>>> 在 2016/1/4 19:13, Arnd Bergmann 写道:
>>>>>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>>>>>>> 在 2015/12/31 23:00, Rongrong Zou 写道:
>>>>> Ranges property can set empty, but this means 1:1 translation. the I/O
>>>>> port range is translated to MMIO address 0x00000001 00000000 to
>>>>> 0x00000001 00000004, it looks wrong else. I wonder if anyone get legacy
>>>>> I/O port resource from dts.
>>>>
>>>> As I said, nothing should really require the ranges property here, unless
>>>> you have a valid IORESOURCE_MEM translation. The code that requires
>>>> the ranges to be present is wrong.
>>>>
>>>
>>> I think the openfirmware(DT) do not support for those unmapped I/O ports, because I
>>> must get resource by calling of_address_to_resource(), which have to call
>>> pci_address_to_pio() when resource type is IORESOURCE_IO. I'm sorry I have no
>>> better idea for this now. Maybe liviu can give me some opinions.
>>
>> I think on x86 it works (or used to work, few people use open firmware on
>> x86 these days, and it may be broken), and the pci_address_to_pio() call
>> behaves differently when PCI_IOBASE is set. x86 never maps I/O ports into
>> memory mapped I/O addresses, they have their own way of accessing them
>> just like your platform.
>>
The big problem is:
The return value of_translate_address can be only an cpu addr,
there is no map from cpuaddr to I/O port in x86 as you said,
we still can't get io resource.
static int __of_address_to_resource(struct device_node *dev,
const __be32 *addrp, u64 size, unsigned int flags,
const char *name, struct resource *r)
{
...
taddr = of_translate_address(dev, addrp);
...
}
>>> /**
>>> * of_address_to_resource - Translate device tree address and return as resource
>>> *
>>> * Note that if your address is a PIO address, the conversion will fail if
>>> * the physical address can't be internally converted to an IO token with
>>> * pci_address_to_pio(), that is because it's either called to early or it
>>> * can't be matched to any host bridge IO space
>>> */
>>> int of_address_to_resource(struct device_node *dev, int index,
>>> struct resource *r)
>>
>> The problem here seems to be that the code assumes that either the I/O ports
>> are always mapped or they are never mapped (no PCI_IOBASE). We need to extend
>> it because now we can have the combination of the two.
>
>
> I am considering the following solution:
>
> Adding unmapped isa io functions in
>
> drivers/of/address.c,
>
> static LIST_HEAD(legacy_io_range_list);
> int isa_register_io_range(phys_addr_t addr, resource_size_t size);
>
> /* before I call isa(LPC) bus driver, the input I/O port must be translated to phys_addr_t
> (the least 16bit means port addr on bus, the second 16bit means bus id)*/
>
> phys_addr_t isa_pio_to_bus_addr(unsigned long pio);
>
> /* the returned PIO do not conflict with PIO get from pci_address_to_pio*/
> unsigned long isa_bus_addr_to_pio(phys_addr_t address);
>
> drivers/bus/lpc.c
>
> lpc_bus_probe()
> {
> isa_register_io_range(phys_addr_t addr, resource_size_t size);
> }
>
> inb(unsigned long port)
> {
> unsigned short bus;
> phys_addr_t addr;
> /*hit isa port range*/
> if(addr = isa_pio_to_bus_addr(port))
> {
> bus = (addr >> 16) & 0xffff;
>
> call lpc driver with addr;
> return lpc_read_byte(bus, addr);
> }
> else /*not hit*/
> {
> return readb(PCI_IOBASE + port);
> }
> }
>
>
>
>>
>>>>> For ipmi driver, I can get I/O port resource by DMI rather than dts.
>>>>
>>>> No, the ipmi driver uses the resource that belongs to the platform
>>>> device already, you can't rely on DMI data to be present there.
>>>
>>> Ipmi has a lot of way to be discovered(ACPI, DMI, hardcoded, hot-add,
>>> openfirmware and a few other), I think we just use one of them, not all of them.
>>> It depend on vendor's hardware solution actually.
>>
>> I don't think we should mix multiple methods here: if the bus is described
>> in DT, all its children should be there as well. Otherwise you get into problems
>> e.g. if you have multiple instances of the LPC bus and the Linux I/O addresses
>> for one or more of them have an offset to the bus specific addresses.
>>
>> The bus probe code decides what the Linux I/O port numbers are, but DMI
>> and other methods have no idea of the mapping. As long as there is only
>> one instance, using the first 0x1000 addresses with a 1:1 mapping saves
>> us a bit of trouble, but I'd be worried about relying on that assumption
>> too much.
>>
>> Arnd
>>
>>
>> .
--
Thanks,
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] | [prev] | [next] | [standalone]
| From | Rolland Chau <zourongrong@gmail.com> |
|---|---|
| Date | 2016-01-10 10:40 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qPkDE-2iG-13@gated-at.bofh.it> |
| In reply to | #1301485 |
On 2016/1/5 20:19, Arnd Bergmann wrote:
> On Tuesday 05 January 2016 19:59:49 Rongrong Zou wrote:
>> 在 2016/1/5 0:34, Arnd Bergmann 写道:
>>> On Tuesday 05 January 2016 00:04:19 Rongrong Zou wrote:
>>>> 在 2016/1/4 19:13, Arnd Bergmann 写道:
>>>>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>>>>>> 在 2015/12/31 23:00, Rongrong Zou 写道:
>>>> Ranges property can set empty, but this means 1:1 translation. the I/O
>>>> port range is translated to MMIO address 0x00000001 00000000 to
>>>> 0x00000001 00000004, it looks wrong else. I wonder if anyone get legacy
>>>> I/O port resource from dts.
>>>
>>> As I said, nothing should really require the ranges property here, unless
>>> you have a valid IORESOURCE_MEM translation. The code that requires
>>> the ranges to be present is wrong.
>>>
>>
>> I think the openfirmware(DT) do not support for those unmapped I/O ports, because I
>> must get resource by calling of_address_to_resource(), which have to call
>> pci_address_to_pio() when resource type is IORESOURCE_IO. I'm sorry I have no
>> better idea for this now. Maybe liviu can give me some opinions.
>
> I think on x86 it works (or used to work, few people use open firmware on
> x86 these days, and it may be broken), and the pci_address_to_pio() call
> behaves differently when PCI_IOBASE is set. x86 never maps I/O ports into
> memory mapped I/O addresses, they have their own way of accessing them
> just like your platform.
>
>> /**
>> * of_address_to_resource - Translate device tree address and return as resource
>> *
>> * Note that if your address is a PIO address, the conversion will fail if
>> * the physical address can't be internally converted to an IO token with
>> * pci_address_to_pio(), that is because it's either called to early or it
>> * can't be matched to any host bridge IO space
>> */
>> int of_address_to_resource(struct device_node *dev, int index,
>> struct resource *r)
>
> The problem here seems to be that the code assumes that either the I/O ports
> are always mapped or they are never mapped (no PCI_IOBASE). We need to extend
> it because now we can have the combination of the two.
Arnd , please take a look at the new solution when you have a moment,thanks.
I modify the "drivers/of/address.c" to address the problem above.
There are some assumptions:
1) ISA(LPC) Like bus must be scanned before PCI.
2) a property "unmapped-size" is introduced to ISA bus DT config, it means the address
range of the bus. if bus-A have a port range(0-99), bus-B have a port range(0-199), then
the cpu port range (0-99) is reserved for bus-A and cpu port range is reserved for bus-B.
3) It it
---
drivers/of/address.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 70 insertions(+), 6 deletions(-)
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 9582c57..d3c71e5 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -682,8 +682,16 @@ struct io_range {
static LIST_HEAD(io_range_list);
static DEFINE_SPINLOCK(io_range_lock);
+phy_addr_t pci_pio_start = 0;
+EXPORT_SYMBOL(pci_pio_start)
#endif
+int isa_register_unmapped_io_range(resource_size_t size)
+{
+ pci_pio_start += size;
+}
+
+
/*
* Record the PCI IO range (expressed as CPU physical address + size).
* Return a negative value if an error has occured, zero otherwise
@@ -694,7 +702,7 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
#ifdef PCI_IOBASE
struct io_range *range;
- resource_size_t allocated_size = 0;
+ resource_size_t allocated_size = pci_pio_start;
/* check if the range hasn't been previously recorded */
spin_lock(&io_range_lock);
@@ -743,7 +751,7 @@ phys_addr_t pci_pio_to_address(unsigned long pio)
#ifdef PCI_IOBASE
struct io_range *range;
- resource_size_t allocated_size = 0;
+ resource_size_t allocated_size = pci_pio_start;
if (pio > IO_SPACE_LIMIT)
return address;
@@ -766,7 +774,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
{
#ifdef PCI_IOBASE
struct io_range *res;
- resource_size_t offset = 0;
+ resource_size_t offset = pci_pio_start;
unsigned long addr = -1;
spin_lock(&io_range_lock);
@@ -788,21 +796,77 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
#endif
}
+static u64 of_get_unmapped_pio(struct device_node *dev, const __be32 *inaddr)
+{
+ struct device_node *np, *parent = NULL;
+ struct of_bus * bus, *pbus;
+ struct property *p_property;
+ int rlen;
+ u32 size = 0;
+ u32 port_base = 0;
+
+ /*IF NOT I/O port*/
+ if(*(in_addr) != 0x01)
+ return OF_BAD_ADDR;
+ else
+ *in_addr = 0;
+
+ parent = of_get_parent(dev);
+ if(parent == NULL)
+ return result;
+ bus = of_match_bus(parent);
+ if(!strcmp(bus->name, "isa")) {
+ p_property = of_find_property(parent, "ranges", &rlen);
+ /*no ranges property, this is a non-bridged isa bus, and the port on it is unmapped*/
+ if(p_property == NULL) {
+ for_each_node_by_name(np, "isa") {
+ if((np != parent) &&
+ !of_find_property(parent, "ranges", &rlen) &&
+ !of_property_read_u32(parent, "unmapped-size", &size)) {
+ port_base += size;
+ }
+ else if (np == parent) {
+ of_bus_default_translate(in_addr + 1, port_base, 1);
+ break;
+ }
+ else {
+ continue;
+ }
+ }
+ return of_read_number(in_addr, 2);
+
+ }
+
+ }
+ return OF_BAD_ADDR;
+}
+
static int __of_address_to_resource(struct device_node *dev,
const __be32 *addrp, u64 size, unsigned int flags,
const char *name, struct resource *r)
{
u64 taddr;
+ u8 addr_type = 0;
if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
return -EINVAL;
taddr = of_translate_address(dev, addrp);
- if (taddr == OF_BAD_ADDR)
- return -EINVAL;
+ if (taddr == OF_BAD_ADDR) {
+
+ taddr == of_get_unmapped_pio(dev, addrp);
+ if(taddr == OF_BAD_ADDR)
+ return -EINVAL
+ else /*we get unmapped I/O port successfully*/
+ addr_type = 1;
+ }
+
memset(r, 0, sizeof(struct resource));
if (flags & IORESOURCE_IO) {
unsigned long port;
- port = pci_address_to_pio(taddr);
+ if(!addr_type)
+ port = pci_address_to_pio(taddr);
+ else
+ port = (unsigned long)taddr;
if (port == (unsigned long)-1)
return -EINVAL;
r->start = port;
--
In bus device probe code
static int lpc_probe(struct platform_device *pdev)
{
...
if(ret = of_property_read_u32((pdev->dev).of_node, "unmapped-size", &io_ranges))
return ret;
lpc_dev->bus_size = io_ranges;
isa_register_io_range(io_ranges);
ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
...
return 0;
}
>
>>>> For ipmi driver, I can get I/O port resource by DMI rather than dts.
>>>
>>> No, the ipmi driver uses the resource that belongs to the platform
>>> device already, you can't rely on DMI data to be present there.
>>
>> Ipmi has a lot of way to be discovered(ACPI, DMI, hardcoded, hot-add,
>> openfirmware and a few other), I think we just use one of them, not all of them.
>> It depend on vendor's hardware solution actually.
>
> I don't think we should mix multiple methods here: if the bus is described
> in DT, all its children should be there as well. Otherwise you get into problems
> e.g. if you have multiple instances of the LPC bus and the Linux I/O addresses
> for one or more of them have an offset to the bus specific addresses.
>
> The bus probe code decides what the Linux I/O port numbers are, but DMI
> and other methods have no idea of the mapping. As long as there is only
> one instance, using the first 0x1000 addresses with a 1:1 mapping saves
> us a bit of trouble, but I'd be worried about relying on that assumption
> too much.
>
> Arnd
>
[toc] | [prev] | [next] | [standalone]
| From | Rongrong Zou <zourongrong@gmail.com> |
|---|---|
| Date | 2016-01-10 14:40 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qPonU-4Lw-3@gated-at.bofh.it> |
| In reply to | #1305522 |
在 2016/1/10 17:29, Rolland Chau 写道:
> On 2016/1/5 20:19, Arnd Bergmann wrote:
>> On Tuesday 05 January 2016 19:59:49 Rongrong Zou wrote:
>>> 在 2016/1/5 0:34, Arnd Bergmann 写道:
>>>> On Tuesday 05 January 2016 00:04:19 Rongrong Zou wrote:
>>>>> 在 2016/1/4 19:13, Arnd Bergmann 写道:
>>>>>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>>>>>>> 在 2015/12/31 23:00, Rongrong Zou 写道:
>>>>> Ranges property can set empty, but this means 1:1 translation. the I/O
>>>>> port range is translated to MMIO address 0x00000001 00000000 to
>>>>> 0x00000001 00000004, it looks wrong else. I wonder if anyone get
>>>>> legacy
>>>>> I/O port resource from dts.
>>>>
>>>> As I said, nothing should really require the ranges property here,
>>>> unless
>>>> you have a valid IORESOURCE_MEM translation. The code that requires
>>>> the ranges to be present is wrong.
>>>>
>>>
>>> I think the openfirmware(DT) do not support for those unmapped I/O
>>> ports, because I
>>> must get resource by calling of_address_to_resource(), which have to
>>> call
>>> pci_address_to_pio() when resource type is IORESOURCE_IO. I'm sorry I
>>> have no
>>> better idea for this now. Maybe liviu can give me some opinions.
>>
>> I think on x86 it works (or used to work, few people use open firmware on
>> x86 these days, and it may be broken), and the pci_address_to_pio() call
>> behaves differently when PCI_IOBASE is set. x86 never maps I/O ports into
>> memory mapped I/O addresses, they have their own way of accessing them
>> just like your platform.
>>
>>> /**
>>> * of_address_to_resource - Translate device tree address and
>>> return as resource
>>> *
>>> * Note that if your address is a PIO address, the conversion will
>>> fail if
>>> * the physical address can't be internally converted to an IO
>>> token with
>>> * pci_address_to_pio(), that is because it's either called to
>>> early or it
>>> * can't be matched to any host bridge IO space
>>> */
>>> int of_address_to_resource(struct device_node *dev, int index,
>>> struct resource *r)
>>
>> The problem here seems to be that the code assumes that either the I/O
>> ports
>> are always mapped or they are never mapped (no PCI_IOBASE). We need to
>> extend
>> it because now we can have the combination of the two.
>
>
> Arnd , please take a look at the new solution when you have a
> moment,thanks.
> I modify the "drivers/of/address.c" to address the problem above.
>
> There are some assumptions:
>
> 1) ISA(LPC) Like bus must be scanned before PCI.
> 2) a property "unmapped-size" is introduced to ISA bus DT config, it
> means the address
> range of the bus. if bus-A have a port range(0-99), bus-B have a port
> range(0-199), then
> the cpu port range (0-99) is reserved for bus-A and cpu port range (100, 299) is
> reserved for bus-B.
> 3) It it
I sorry, I made a mistake, please ignore 3). And I added the cpu port
range(100, 299) for bus-B.
> ---
> drivers/of/address.c | 76
> +++++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 70 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 9582c57..d3c71e5 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -682,8 +682,16 @@ struct io_range {
>
> static LIST_HEAD(io_range_list);
> static DEFINE_SPINLOCK(io_range_lock);
> +phy_addr_t pci_pio_start = 0;
> +EXPORT_SYMBOL(pci_pio_start)
> #endif
>
> +int isa_register_unmapped_io_range(resource_size_t size)
> +{
> + pci_pio_start += size;
> +}
> +
> +
> /*
> * Record the PCI IO range (expressed as CPU physical address + size).
> * Return a negative value if an error has occured, zero otherwise
> @@ -694,7 +702,7 @@ int __weak pci_register_io_range(phys_addr_t addr,
> resource_size_t size)
>
> #ifdef PCI_IOBASE
> struct io_range *range;
> - resource_size_t allocated_size = 0;
> + resource_size_t allocated_size = pci_pio_start;
>
> /* check if the range hasn't been previously recorded */
> spin_lock(&io_range_lock);
> @@ -743,7 +751,7 @@ phys_addr_t pci_pio_to_address(unsigned long pio)
>
> #ifdef PCI_IOBASE
> struct io_range *range;
> - resource_size_t allocated_size = 0;
> + resource_size_t allocated_size = pci_pio_start;
>
> if (pio > IO_SPACE_LIMIT)
> return address;
> @@ -766,7 +774,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t
> address)
> {
> #ifdef PCI_IOBASE
> struct io_range *res;
> - resource_size_t offset = 0;
> + resource_size_t offset = pci_pio_start;
> unsigned long addr = -1;
>
> spin_lock(&io_range_lock);
> @@ -788,21 +796,77 @@ unsigned long __weak
> pci_address_to_pio(phys_addr_t address)
> #endif
> }
>
> +static u64 of_get_unmapped_pio(struct device_node *dev, const __be32
> *inaddr)
> +{
> + struct device_node *np, *parent = NULL;
> + struct of_bus * bus, *pbus;
> + struct property *p_property;
> + int rlen;
> + u32 size = 0;
> + u32 port_base = 0;
> +
> + /*IF NOT I/O port*/
> + if(*(in_addr) != 0x01)
> + return OF_BAD_ADDR;
> + else
> + *in_addr = 0;
> +
> + parent = of_get_parent(dev);
> + if(parent == NULL)
> + return result;
> + bus = of_match_bus(parent);
> + if(!strcmp(bus->name, "isa")) {
> + p_property = of_find_property(parent, "ranges", &rlen);
> + /*no ranges property, this is a non-bridged isa bus, and
> the port on it is unmapped*/
> + if(p_property == NULL) {
> + for_each_node_by_name(np, "isa") {
> + if((np != parent) &&
> + !of_find_property(parent, "ranges",
> &rlen) &&
> + !of_property_read_u32(parent,
> "unmapped-size", &size)) {
> + port_base += size;
> + }
> + else if (np == parent) {
> + of_bus_default_translate(in_addr
> + 1, port_base, 1);
> + break;
> + }
> + else {
> + continue;
> + }
> + }
> + return of_read_number(in_addr, 2);
> +
> + }
> +
> + }
> + return OF_BAD_ADDR;
> +}
> +
> static int __of_address_to_resource(struct device_node *dev,
> const __be32 *addrp, u64 size, unsigned int flags,
> const char *name, struct resource *r)
> {
> u64 taddr;
> + u8 addr_type = 0;
>
> if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> return -EINVAL;
> taddr = of_translate_address(dev, addrp);
> - if (taddr == OF_BAD_ADDR)
> - return -EINVAL;
> + if (taddr == OF_BAD_ADDR) {
> +
> + taddr == of_get_unmapped_pio(dev, addrp);
> + if(taddr == OF_BAD_ADDR)
> + return -EINVAL
> + else /*we get unmapped I/O port successfully*/
> + addr_type = 1;
> + }
> +
> memset(r, 0, sizeof(struct resource));
> if (flags & IORESOURCE_IO) {
> unsigned long port;
> - port = pci_address_to_pio(taddr);
> + if(!addr_type)
> + port = pci_address_to_pio(taddr);
> + else
> + port = (unsigned long)taddr;
> if (port == (unsigned long)-1)
> return -EINVAL;
> r->start = port;
[toc] | [prev] | [next] | [standalone]
| From | liviu.dudau@arm.com |
|---|---|
| Date | 2016-01-11 17:20 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qPNmh-4Mf-3@gated-at.bofh.it> |
| In reply to | #1300636 |
On Mon, Jan 04, 2016 at 12:13:05PM +0100, Arnd Bergmann wrote:
> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
> > 在 2015/12/31 23:00, Rongrong Zou 写道:
> > > 2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd@arndb.de <mailto:arnd@arndb.de>>:
> > > > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
> > > > > 在 2015/12/30 17:06, Arnd Bergmann 写道:
> > > > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
> > > >
> > > > The DT sample above looks good in principle. I believe what you are missing
> > > > here is code in your driver to scan the child nodes to create the platform
> > > > devices. of_bus_isa_translate() should work with your definition here
> > > > and create the correct IORESOURCE_IO resources. You don't have any MMIO
> > > > resources, so the absence of a ranges property is ok. Maybe all you
> > > > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
> > > >
> > >
> > > You are right. thanks, i'll try on test board . if i get the correct result , the new patch
> > > will be sent later. By the way, it's my another email account use when i at home.
> >
> > I tried, and there need some additional changes.
> >
> > isa@a01b0000 {
> >
> > /*the node name should start with "isa", because of below definition
> > * static int of_bus_isa_match(struct device_node *np)
> > * {
> > * return !strcmp(np->name, "isa");
> > * }
>
> Looks good. It would be nicer to match on device_type than on name,
> but this is ancient code and it's probably best not to touch it
> so we don't accidentally break some old SPARC or PPC system.
>
> > */
> > compatible = "low-pin-count";
> > device_type = "isa";
> > #address-cells = <2>;
> > #size-cells = <1>;
> > reg = <0x0 0xa01b0000 0x0 0x10000>;
> > ranges = <0x1 0x0 0x0 0x0 0x1000>;
> > /*
> > * ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
> > *
> > */
> > ipmi_0:ipmi@000000e4{
> > device_type = "ipmi";
> > compatible = "ipmi-bt";
> > reg = <0x1 0x000000e4 0x4>;
> > };
> >
>
> This looks wrong: the property above says that the I/O port range is
> translated to MMIO address 0x00000000 to 0x00010000, which is not
> true on your hardware. I think this needs to be changed in the code
> so the ranges property is not required for I/O ports.
>
> > drivers\of\address.c
> > static int __of_address_to_resource(struct device_node *dev,
> > const __be32 *addrp, u64 size, unsigned int flags,
> > const char *name, struct resource *r)
> > {
> > u64 taddr;
> >
> > if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> > return -EINVAL;
> > taddr = of_translate_address(dev, addrp);
> > if (taddr == OF_BAD_ADDR)
> > return -EINVAL;
> > memset(r, 0, sizeof(struct resource));
> > if (flags & IORESOURCE_IO) {
> > unsigned long port;
> >
> > /*****************************************************************/
> > /*legacy port(< 0x1000) is reserved, and need no translation here*/
> > /*****************************************************************/
> > if(taddr + size < PCIBIOS_MIN_IO){
> > r->start = taddr;
> > r->end = taddr + size - 1;
> > }
>
> I don't like having a special case based on the address here,
> the same kind of hack might be needed for PCI I/O spaces in
> hardware that uses an indirect method like your LPC bus
> does, and the code above will not work on any LPC implementation
> that correctly multiplexes its I/O ports with the first PCI domain.
>
> I think it would be better to avoid translating the port into
> a physical address to start with just to translate it back into
> a port number, what we need instead is the offset between the
> bus specific port number and the linux port number. I've added
> Liviu to Cc, he wrote this code originally and may have some idea
> of how we could do that.
Hi,
Getting back to work after a longer holiday, my brain might not be running
at full speed here, so I'm trying to clarify things a bit here.
It looks to me like Rongrong is trying to trap the inb()/outb() calls that he
added to arm64 by patch 1/3 and redirect those operations to the memory
mapped LPC driver. I think the whole redirection and registration of inb/outb
ops can be made cleaner, so that the general concept resembles the DMA ops
registration? (I have this mental picture that what Rongrong is trying to do
is similar to what a DMA engine does, except this is slowing down things to
byte level). If that is done properly in the parent node, then we should not
care what the PCIBIOS_MIN_IO value is as the inb()/outb() calls will always
go through the redirection for the children.
As for the ranges property: does he wants the ipmi-bt driver to see in the
reg property the legacy ISA I/O ports values or the CPU addresses? If the former,
then I agree that the range property should not be required, but also the
reg values need to be changed (drop the top bit). If the later, then the
ranges property is required to do the proper translation.
Rongrong, removing the ranges property and with a reg = <0xe4 0x4> property
in the ipmi-bt node, what IO_RESOURCE type resources do you get back from
the of_address_to_resource() translation?
Best regards,
Liviu
>
> Arnd
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
[toc] | [prev] | [next] | [standalone]
| From | Rongrong Zou <zourongrong@gmail.com> |
|---|---|
| Date | 2016-01-12 03:40 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qPX2i-2Wn-13@gated-at.bofh.it> |
| In reply to | #1306457 |
On 2016/1/12 0:14, liviu.dudau@arm.com wrote:
> On Mon, Jan 04, 2016 at 12:13:05PM +0100, Arnd Bergmann wrote:
>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>>> 在 2015/12/31 23:00, Rongrong Zou 写道:
>>>> 2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd@arndb.de <mailto:arnd@arndb.de>>:
>>>> > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
>>>> > > 在 2015/12/30 17:06, Arnd Bergmann 写道:
>>>> > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
>>>> >
>>>> > The DT sample above looks good in principle. I believe what you are missing
>>>> > here is code in your driver to scan the child nodes to create the platform
>>>> > devices. of_bus_isa_translate() should work with your definition here
>>>> > and create the correct IORESOURCE_IO resources. You don't have any MMIO
>>>> > resources, so the absence of a ranges property is ok. Maybe all you
>>>> > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
>>>> >
>>>>
>>>> You are right. thanks, i'll try on test board . if i get the correct result , the new patch
>>>> will be sent later. By the way, it's my another email account use when i at home.
>>>
>>> I tried, and there need some additional changes.
>>>
>>> isa@a01b0000 {
>>>
>>> /*the node name should start with "isa", because of below definition
>>> * static int of_bus_isa_match(struct device_node *np)
>>> * {
>>> * return !strcmp(np->name, "isa");
>>> * }
>>
>> Looks good. It would be nicer to match on device_type than on name,
>> but this is ancient code and it's probably best not to touch it
>> so we don't accidentally break some old SPARC or PPC system.
>>
>>> */
>>> compatible = "low-pin-count";
>>> device_type = "isa";
>>> #address-cells = <2>;
>>> #size-cells = <1>;
>>> reg = <0x0 0xa01b0000 0x0 0x10000>;
>>> ranges = <0x1 0x0 0x0 0x0 0x1000>;
>>> /*
>>> * ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
>>> *
>>> */
>>> ipmi_0:ipmi@000000e4{
>>> device_type = "ipmi";
>>> compatible = "ipmi-bt";
>>> reg = <0x1 0x000000e4 0x4>;
>>> };
>>>
>>
>> This looks wrong: the property above says that the I/O port range is
>> translated to MMIO address 0x00000000 to 0x00010000, which is not
>> true on your hardware. I think this needs to be changed in the code
>> so the ranges property is not required for I/O ports.
>>
>>> drivers\of\address.c
>>> static int __of_address_to_resource(struct device_node *dev,
>>> const __be32 *addrp, u64 size, unsigned int flags,
>>> const char *name, struct resource *r)
>>> {
>>> u64 taddr;
>>>
>>> if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
>>> return -EINVAL;
>>> taddr = of_translate_address(dev, addrp);
>>> if (taddr == OF_BAD_ADDR)
>>> return -EINVAL;
>>> memset(r, 0, sizeof(struct resource));
>>> if (flags & IORESOURCE_IO) {
>>> unsigned long port;
>>>
>>> /*****************************************************************/
>>> /*legacy port(< 0x1000) is reserved, and need no translation here*/
>>> /*****************************************************************/
>>> if(taddr + size < PCIBIOS_MIN_IO){
>>> r->start = taddr;
>>> r->end = taddr + size - 1;
>>> }
>>
>> I don't like having a special case based on the address here,
>> the same kind of hack might be needed for PCI I/O spaces in
>> hardware that uses an indirect method like your LPC bus
>> does, and the code above will not work on any LPC implementation
>> that correctly multiplexes its I/O ports with the first PCI domain.
>>
>> I think it would be better to avoid translating the port into
>> a physical address to start with just to translate it back into
>> a port number, what we need instead is the offset between the
>> bus specific port number and the linux port number. I've added
>> Liviu to Cc, he wrote this code originally and may have some idea
>> of how we could do that.
>
> Hi,
Hi Liviu,
Thanks for reviewing this.
>
> Getting back to work after a longer holiday, my brain might not be running
> at full speed here, so I'm trying to clarify things a bit here.
>
> It looks to me like Rongrong is trying to trap the inb()/outb() calls that he
> added to arm64 by patch 1/3 and redirect those operations to the memory
> mapped LPC driver. I think the whole redirection and registration of inb/outb
> ops can be made cleaner, so that the general concept resembles the DMA ops
> registration? (I have this mental picture that what Rongrong is trying to do
> is similar to what a DMA engine does, except this is slowing down things to
> byte level). If that is done properly in the parent node, then we should not
> care what the PCIBIOS_MIN_IO value is as the inb()/outb() calls will always
> go through the redirection for the children.
>
> As for the ranges property: does he wants the ipmi-bt driver to see in the
> reg property the legacy ISA I/O ports values or the CPU addresses? If the former,
> then I agree that the range property should not be required, but also the
> reg values need to be changed (drop the top bit). If the later, then the
> ranges property is required to do the proper translation.
The former, thanks.
>
> Rongrong, removing the ranges property and with a reg = <0xe4 0x4> property
> in the ipmi-bt node, what IO_RESOURCE type resources do you get back from
> the of_address_to_resource() translation?
I want to get IORESOURCE_IO type resource, but if the parent node drop the
"rangs" property, the of_address_to_resource() translation will return with -EINVAL.
>
> Best regards,
> Liviu
>
>
>>
>> Arnd
>>
>
Regards,
Rongrong
[toc] | [prev] | [next] | [standalone]
| From | liviu.dudau@arm.com |
|---|---|
| Date | 2016-01-12 10:10 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qQ37H-7fJ-1@gated-at.bofh.it> |
| In reply to | #1306943 |
On Tue, Jan 12, 2016 at 10:39:36AM +0800, Rongrong Zou wrote:
> On 2016/1/12 0:14, liviu.dudau@arm.com wrote:
> >On Mon, Jan 04, 2016 at 12:13:05PM +0100, Arnd Bergmann wrote:
> >>On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
> >>>在 2015/12/31 23:00, Rongrong Zou 写道:
> >>>>2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd@arndb.de <mailto:arnd@arndb.de>>:
> >>>> > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
> >>>> > > 在 2015/12/30 17:06, Arnd Bergmann 写道:
> >>>> > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
> >>>> >
> >>>> > The DT sample above looks good in principle. I believe what you are missing
> >>>> > here is code in your driver to scan the child nodes to create the platform
> >>>> > devices. of_bus_isa_translate() should work with your definition here
> >>>> > and create the correct IORESOURCE_IO resources. You don't have any MMIO
> >>>> > resources, so the absence of a ranges property is ok. Maybe all you
> >>>> > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
> >>>> >
> >>>>
> >>>>You are right. thanks, i'll try on test board . if i get the correct result , the new patch
> >>>>will be sent later. By the way, it's my another email account use when i at home.
> >>>
> >>>I tried, and there need some additional changes.
> >>>
> >>>isa@a01b0000 {
> >>>
> >>>/*the node name should start with "isa", because of below definition
> >>>* static int of_bus_isa_match(struct device_node *np)
> >>>* {
> >>>* return !strcmp(np->name, "isa");
> >>>* }
> >>
> >>Looks good. It would be nicer to match on device_type than on name,
> >>but this is ancient code and it's probably best not to touch it
> >>so we don't accidentally break some old SPARC or PPC system.
> >>
> >>>*/
> >>> compatible = "low-pin-count";
> >>> device_type = "isa";
> >>> #address-cells = <2>;
> >>> #size-cells = <1>;
> >>> reg = <0x0 0xa01b0000 0x0 0x10000>;
> >>> ranges = <0x1 0x0 0x0 0x0 0x1000>;
> >>>/*
> >>>* ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
> >>>*
> >>>*/
> >>> ipmi_0:ipmi@000000e4{
> >>> device_type = "ipmi";
> >>> compatible = "ipmi-bt";
> >>> reg = <0x1 0x000000e4 0x4>;
> >>>};
> >>>
> >>
> >>This looks wrong: the property above says that the I/O port range is
> >>translated to MMIO address 0x00000000 to 0x00010000, which is not
> >>true on your hardware. I think this needs to be changed in the code
> >>so the ranges property is not required for I/O ports.
> >>
> >>>drivers\of\address.c
> >>>static int __of_address_to_resource(struct device_node *dev,
> >>> const __be32 *addrp, u64 size, unsigned int flags,
> >>> const char *name, struct resource *r)
> >>>{
> >>> u64 taddr;
> >>>
> >>> if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> >>> return -EINVAL;
> >>> taddr = of_translate_address(dev, addrp);
> >>> if (taddr == OF_BAD_ADDR)
> >>> return -EINVAL;
> >>> memset(r, 0, sizeof(struct resource));
> >>> if (flags & IORESOURCE_IO) {
> >>> unsigned long port;
> >>>
> >>>/*****************************************************************/
> >>>/*legacy port(< 0x1000) is reserved, and need no translation here*/
> >>>/*****************************************************************/
> >>> if(taddr + size < PCIBIOS_MIN_IO){
> >>> r->start = taddr;
> >>> r->end = taddr + size - 1;
> >>> }
> >>
> >>I don't like having a special case based on the address here,
> >>the same kind of hack might be needed for PCI I/O spaces in
> >>hardware that uses an indirect method like your LPC bus
> >>does, and the code above will not work on any LPC implementation
> >>that correctly multiplexes its I/O ports with the first PCI domain.
> >>
> >>I think it would be better to avoid translating the port into
> >>a physical address to start with just to translate it back into
> >>a port number, what we need instead is the offset between the
> >>bus specific port number and the linux port number. I've added
> >>Liviu to Cc, he wrote this code originally and may have some idea
> >>of how we could do that.
> >
> >Hi,
>
> Hi Liviu,
>
> Thanks for reviewing this.
>
> >
> >Getting back to work after a longer holiday, my brain might not be running
> >at full speed here, so I'm trying to clarify things a bit here.
> >
> >It looks to me like Rongrong is trying to trap the inb()/outb() calls that he
> >added to arm64 by patch 1/3 and redirect those operations to the memory
> >mapped LPC driver. I think the whole redirection and registration of inb/outb
> >ops can be made cleaner, so that the general concept resembles the DMA ops
> >registration? (I have this mental picture that what Rongrong is trying to do
> >is similar to what a DMA engine does, except this is slowing down things to
> >byte level). If that is done properly in the parent node, then we should not
> >care what the PCIBIOS_MIN_IO value is as the inb()/outb() calls will always
> >go through the redirection for the children.
> >
> >As for the ranges property: does he wants the ipmi-bt driver to see in the
> >reg property the legacy ISA I/O ports values or the CPU addresses? If the former,
> >then I agree that the range property should not be required, but also the
> >reg values need to be changed (drop the top bit). If the later, then the
> >ranges property is required to do the proper translation.
>
> The former, thanks.
>
> >
> >Rongrong, removing the ranges property and with a reg = <0xe4 0x4> property
> >in the ipmi-bt node, what IO_RESOURCE type resources do you get back from
> >the of_address_to_resource() translation?
>
> I want to get IORESOURCE_IO type resource, but if the parent node drop the
> "rangs" property, the of_address_to_resource() translation will return with -EINVAL.
Have you tracked what part of the code is sensitive to the presence of "ranges"
property? Does of_get_address() call returns the IO_RESOURCE flag set without "ranges"?
Best regards,
Liviu
>
> >
> >Best regards,
> >Liviu
> >
> >
> >>
> >> Arnd
> >>
> >
> Regards,
> Rongrong
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
[toc] | [prev] | [next] | [standalone]
| From | Rongrong Zou <zourongrong@huawei.com> |
|---|---|
| Date | 2016-01-12 10:30 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qQ3r3-7nB-1@gated-at.bofh.it> |
| In reply to | #1307125 |
在 2016/1/12 17:07, liviu.dudau@arm.com 写道:
> On Tue, Jan 12, 2016 at 10:39:36AM +0800, Rongrong Zou wrote:
>> On 2016/1/12 0:14, liviu.dudau@arm.com wrote:
>>> On Mon, Jan 04, 2016 at 12:13:05PM +0100, Arnd Bergmann wrote:
>>>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>>>>> 在 2015/12/31 23:00, Rongrong Zou 写道:
>>>>>> 2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd@arndb.de <mailto:arnd@arndb.de>>:
>>>>>> > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
>>>>>> > > 在 2015/12/30 17:06, Arnd Bergmann 写道:
>>>>>> > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
>>>>>> >
>>>>>> > The DT sample above looks good in principle. I believe what you are missing
>>>>>> > here is code in your driver to scan the child nodes to create the platform
>>>>>> > devices. of_bus_isa_translate() should work with your definition here
>>>>>> > and create the correct IORESOURCE_IO resources. You don't have any MMIO
>>>>>> > resources, so the absence of a ranges property is ok. Maybe all you
>>>>>> > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
>>>>>> >
>>>>>>
>>>>>> You are right. thanks, i'll try on test board . if i get the correct result , the new patch
>>>>>> will be sent later. By the way, it's my another email account use when i at home.
>>>>>
>>>>> I tried, and there need some additional changes.
>>>>>
>>>>> isa@a01b0000 {
>>>>>
>>>>> /*the node name should start with "isa", because of below definition
>>>>> * static int of_bus_isa_match(struct device_node *np)
>>>>> * {
>>>>> * return !strcmp(np->name, "isa");
>>>>> * }
>>>>
>>>> Looks good. It would be nicer to match on device_type than on name,
>>>> but this is ancient code and it's probably best not to touch it
>>>> so we don't accidentally break some old SPARC or PPC system.
>>>>
>>>>> */
>>>>> compatible = "low-pin-count";
>>>>> device_type = "isa";
>>>>> #address-cells = <2>;
>>>>> #size-cells = <1>;
>>>>> reg = <0x0 0xa01b0000 0x0 0x10000>;
>>>>> ranges = <0x1 0x0 0x0 0x0 0x1000>;
>>>>> /*
>>>>> * ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
>>>>> *
>>>>> */
>>>>> ipmi_0:ipmi@000000e4{
>>>>> device_type = "ipmi";
>>>>> compatible = "ipmi-bt";
>>>>> reg = <0x1 0x000000e4 0x4>;
>>>>> };
>>>>>
>>>>
>>>> This looks wrong: the property above says that the I/O port range is
>>>> translated to MMIO address 0x00000000 to 0x00010000, which is not
>>>> true on your hardware. I think this needs to be changed in the code
>>>> so the ranges property is not required for I/O ports.
>>>>
>>>>> drivers\of\address.c
>>>>> static int __of_address_to_resource(struct device_node *dev,
>>>>> const __be32 *addrp, u64 size, unsigned int flags,
>>>>> const char *name, struct resource *r)
>>>>> {
>>>>> u64 taddr;
>>>>>
>>>>> if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
>>>>> return -EINVAL;
>>>>> taddr = of_translate_address(dev, addrp);
>>>>> if (taddr == OF_BAD_ADDR)
>>>>> return -EINVAL;
>>>>> memset(r, 0, sizeof(struct resource));
>>>>> if (flags & IORESOURCE_IO) {
>>>>> unsigned long port;
>>>>>
>>>>> /*****************************************************************/
>>>>> /*legacy port(< 0x1000) is reserved, and need no translation here*/
>>>>> /*****************************************************************/
>>>>> if(taddr + size < PCIBIOS_MIN_IO){
>>>>> r->start = taddr;
>>>>> r->end = taddr + size - 1;
>>>>> }
>>>>
>>>> I don't like having a special case based on the address here,
>>>> the same kind of hack might be needed for PCI I/O spaces in
>>>> hardware that uses an indirect method like your LPC bus
>>>> does, and the code above will not work on any LPC implementation
>>>> that correctly multiplexes its I/O ports with the first PCI domain.
>>>>
>>>> I think it would be better to avoid translating the port into
>>>> a physical address to start with just to translate it back into
>>>> a port number, what we need instead is the offset between the
>>>> bus specific port number and the linux port number. I've added
>>>> Liviu to Cc, he wrote this code originally and may have some idea
>>>> of how we could do that.
>>>
>>> Hi,
>>
>> Hi Liviu,
>>
>> Thanks for reviewing this.
>>
>>>
>>> Getting back to work after a longer holiday, my brain might not be running
>>> at full speed here, so I'm trying to clarify things a bit here.
>>>
>>> It looks to me like Rongrong is trying to trap the inb()/outb() calls that he
>>> added to arm64 by patch 1/3 and redirect those operations to the memory
>>> mapped LPC driver. I think the whole redirection and registration of inb/outb
>>> ops can be made cleaner, so that the general concept resembles the DMA ops
>>> registration? (I have this mental picture that what Rongrong is trying to do
>>> is similar to what a DMA engine does, except this is slowing down things to
>>> byte level). If that is done properly in the parent node, then we should not
>>> care what the PCIBIOS_MIN_IO value is as the inb()/outb() calls will always
>>> go through the redirection for the children.
>>>
>>> As for the ranges property: does he wants the ipmi-bt driver to see in the
>>> reg property the legacy ISA I/O ports values or the CPU addresses? If the former,
>>> then I agree that the range property should not be required, but also the
>>> reg values need to be changed (drop the top bit). If the later, then the
>>> ranges property is required to do the proper translation.
>>
>> The former, thanks.
>>
>>>
>>> Rongrong, removing the ranges property and with a reg = <0xe4 0x4> property
>>> in the ipmi-bt node, what IO_RESOURCE type resources do you get back from
>>> the of_address_to_resource() translation?
>>
>> I want to get IORESOURCE_IO type resource, but if the parent node drop the
>> "rangs" property, the of_address_to_resource() translation will return with -EINVAL.
>
> Have you tracked what part of the code is sensitive to the presence of "ranges"
> property? Does of_get_address() call returns the IO_RESOURCE flag set without "ranges"?
>
Yes, IO_RESOURCE flag can be get without "ranges".
I tracked the code, it is at of_translate_one(), Below is the calling infomation.
of_address_to_resource-> __of_address_to_resource ->of_translate_address->
__of_translate_address(dev, in_addr, "ranges")->of_translate_one()
static int of_translate_one(struct device_node *parent, struct of_bus *bus,
struct of_bus *pbus, __be32 *addr,
int na, int ns, int pna, const char *rprop)
{
const __be32 *ranges;
unsigned int rlen;
int rone;
u64 offset = OF_BAD_ADDR;
ranges = of_get_property(parent, rprop, &rlen);
if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
pr_debug("OF: no ranges; cannot translate\n");
return 1;
}
...
}
> Best regards,
> Liviu
>
>>
>>>
>>> Best regards,
>>> Liviu
>>>
>>>
>>>>
>>>> Arnd
>>>>
>>>
>> Regards,
>> Rongrong
>>
>
--
Regards,
Rongrong
[toc] | [prev] | [next] | [standalone]
| From | liviu.dudau@arm.com |
|---|---|
| Date | 2016-01-12 11:20 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qQ4du-7Y1-29@gated-at.bofh.it> |
| In reply to | #1307138 |
On Tue, Jan 12, 2016 at 05:25:56PM +0800, Rongrong Zou wrote:
> 在 2016/1/12 17:07, liviu.dudau@arm.com 写道:
> >On Tue, Jan 12, 2016 at 10:39:36AM +0800, Rongrong Zou wrote:
> >>On 2016/1/12 0:14, liviu.dudau@arm.com wrote:
> >>>On Mon, Jan 04, 2016 at 12:13:05PM +0100, Arnd Bergmann wrote:
> >>>>On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
> >>>>>在 2015/12/31 23:00, Rongrong Zou 写道:
> >>>>>>2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd@arndb.de <mailto:arnd@arndb.de>>:
> >>>>>> > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
> >>>>>> > > 在 2015/12/30 17:06, Arnd Bergmann 写道:
> >>>>>> > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
> >>>>>> >
> >>>>>> > The DT sample above looks good in principle. I believe what you are missing
> >>>>>> > here is code in your driver to scan the child nodes to create the platform
> >>>>>> > devices. of_bus_isa_translate() should work with your definition here
> >>>>>> > and create the correct IORESOURCE_IO resources. You don't have any MMIO
> >>>>>> > resources, so the absence of a ranges property is ok. Maybe all you
> >>>>>> > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
> >>>>>> >
> >>>>>>
> >>>>>>You are right. thanks, i'll try on test board . if i get the correct result , the new patch
> >>>>>>will be sent later. By the way, it's my another email account use when i at home.
> >>>>>
> >>>>>I tried, and there need some additional changes.
> >>>>>
> >>>>>isa@a01b0000 {
> >>>>>
> >>>>>/*the node name should start with "isa", because of below definition
> >>>>>* static int of_bus_isa_match(struct device_node *np)
> >>>>>* {
> >>>>>* return !strcmp(np->name, "isa");
> >>>>>* }
> >>>>
> >>>>Looks good. It would be nicer to match on device_type than on name,
> >>>>but this is ancient code and it's probably best not to touch it
> >>>>so we don't accidentally break some old SPARC or PPC system.
> >>>>
> >>>>>*/
> >>>>> compatible = "low-pin-count";
> >>>>> device_type = "isa";
> >>>>> #address-cells = <2>;
> >>>>> #size-cells = <1>;
> >>>>> reg = <0x0 0xa01b0000 0x0 0x10000>;
> >>>>> ranges = <0x1 0x0 0x0 0x0 0x1000>;
> >>>>>/*
> >>>>>* ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
> >>>>>*
> >>>>>*/
> >>>>> ipmi_0:ipmi@000000e4{
> >>>>> device_type = "ipmi";
> >>>>> compatible = "ipmi-bt";
> >>>>> reg = <0x1 0x000000e4 0x4>;
> >>>>>};
> >>>>>
> >>>>
> >>>>This looks wrong: the property above says that the I/O port range is
> >>>>translated to MMIO address 0x00000000 to 0x00010000, which is not
> >>>>true on your hardware. I think this needs to be changed in the code
> >>>>so the ranges property is not required for I/O ports.
> >>>>
> >>>>>drivers\of\address.c
> >>>>>static int __of_address_to_resource(struct device_node *dev,
> >>>>> const __be32 *addrp, u64 size, unsigned int flags,
> >>>>> const char *name, struct resource *r)
> >>>>>{
> >>>>> u64 taddr;
> >>>>>
> >>>>> if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> >>>>> return -EINVAL;
> >>>>> taddr = of_translate_address(dev, addrp);
> >>>>> if (taddr == OF_BAD_ADDR)
> >>>>> return -EINVAL;
> >>>>> memset(r, 0, sizeof(struct resource));
> >>>>> if (flags & IORESOURCE_IO) {
> >>>>> unsigned long port;
> >>>>>
> >>>>>/*****************************************************************/
> >>>>>/*legacy port(< 0x1000) is reserved, and need no translation here*/
> >>>>>/*****************************************************************/
> >>>>> if(taddr + size < PCIBIOS_MIN_IO){
> >>>>> r->start = taddr;
> >>>>> r->end = taddr + size - 1;
> >>>>> }
> >>>>
> >>>>I don't like having a special case based on the address here,
> >>>>the same kind of hack might be needed for PCI I/O spaces in
> >>>>hardware that uses an indirect method like your LPC bus
> >>>>does, and the code above will not work on any LPC implementation
> >>>>that correctly multiplexes its I/O ports with the first PCI domain.
> >>>>
> >>>>I think it would be better to avoid translating the port into
> >>>>a physical address to start with just to translate it back into
> >>>>a port number, what we need instead is the offset between the
> >>>>bus specific port number and the linux port number. I've added
> >>>>Liviu to Cc, he wrote this code originally and may have some idea
> >>>>of how we could do that.
> >>>
> >>>Hi,
> >>
> >>Hi Liviu,
> >>
> >>Thanks for reviewing this.
> >>
> >>>
> >>>Getting back to work after a longer holiday, my brain might not be running
> >>>at full speed here, so I'm trying to clarify things a bit here.
> >>>
> >>>It looks to me like Rongrong is trying to trap the inb()/outb() calls that he
> >>>added to arm64 by patch 1/3 and redirect those operations to the memory
> >>>mapped LPC driver. I think the whole redirection and registration of inb/outb
> >>>ops can be made cleaner, so that the general concept resembles the DMA ops
> >>>registration? (I have this mental picture that what Rongrong is trying to do
> >>>is similar to what a DMA engine does, except this is slowing down things to
> >>>byte level). If that is done properly in the parent node, then we should not
> >>>care what the PCIBIOS_MIN_IO value is as the inb()/outb() calls will always
> >>>go through the redirection for the children.
> >>>
> >>>As for the ranges property: does he wants the ipmi-bt driver to see in the
> >>>reg property the legacy ISA I/O ports values or the CPU addresses? If the former,
> >>>then I agree that the range property should not be required, but also the
> >>>reg values need to be changed (drop the top bit). If the later, then the
> >>>ranges property is required to do the proper translation.
> >>
> >>The former, thanks.
> >>
> >>>
> >>>Rongrong, removing the ranges property and with a reg = <0xe4 0x4> property
> >>>in the ipmi-bt node, what IO_RESOURCE type resources do you get back from
> >>>the of_address_to_resource() translation?
> >>
> >>I want to get IORESOURCE_IO type resource, but if the parent node drop the
> >>"rangs" property, the of_address_to_resource() translation will return with -EINVAL.
> >
> >Have you tracked what part of the code is sensitive to the presence of "ranges"
> >property? Does of_get_address() call returns the IO_RESOURCE flag set without "ranges"?
> >
>
>
> Yes, IO_RESOURCE flag can be get without "ranges".
> I tracked the code, it is at of_translate_one(), Below is the calling infomation.
>
> of_address_to_resource-> __of_address_to_resource ->of_translate_address->
> __of_translate_address(dev, in_addr, "ranges")->of_translate_one()
>
>
> static int of_translate_one(struct device_node *parent, struct of_bus *bus,
> struct of_bus *pbus, __be32 *addr,
> int na, int ns, int pna, const char *rprop)
> {
> const __be32 *ranges;
> unsigned int rlen;
> int rone;
> u64 offset = OF_BAD_ADDR;
>
> ranges = of_get_property(parent, rprop, &rlen);
> if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> pr_debug("OF: no ranges; cannot translate\n");
> return 1;
> }
> ...
> }
OK, looking at of_translate_one() comments it looks like a missing "ranges" property is
only accepted on PowerPC. I suggest you have an empty "ranges" property in your isa
parent node, that will signal to the OF parsing code that the mapping is 1:1. Then have
the IPMI node use the reg = <0x0 0xe4 4>; property values instead of reg = <0x1 0xe4 4>;
Best regards,
Liviu
>
> >Best regards,
> >Liviu
> >
> >>
> >>>
> >>>Best regards,
> >>>Liviu
> >>>
> >>>
> >>>>
> >>>> Arnd
> >>>>
> >>>
> >>Regards,
> >>Rongrong
> >>
> >
> --
> Regards,
> Rongrong
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
[toc] | [prev] | [next] | [standalone]
| From | Rongrong Zou <zourongrong@huawei.com> |
|---|---|
| Date | 2016-01-12 12:10 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qQ4ZP-5t-3@gated-at.bofh.it> |
| In reply to | #1307213 |
在 2016/1/12 18:14, liviu.dudau@arm.com 写道:
> On Tue, Jan 12, 2016 at 05:25:56PM +0800, Rongrong Zou wrote:
>> 在 2016/1/12 17:07, liviu.dudau@arm.com 写道:
>>> On Tue, Jan 12, 2016 at 10:39:36AM +0800, Rongrong Zou wrote:
>>>> On 2016/1/12 0:14, liviu.dudau@arm.com wrote:
>>>>> On Mon, Jan 04, 2016 at 12:13:05PM +0100, Arnd Bergmann wrote:
>>>>>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>>>>>>> 在 2015/12/31 23:00, Rongrong Zou 写道:
>>>>>>>> 2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd@arndb.de <mailto:arnd@arndb.de>>:
>>>>>>>> > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
>>>>>>>> > > 在 2015/12/30 17:06, Arnd Bergmann 写道:
>>>>>>>> > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
>>>>>>>> >
>>>>>>>> > The DT sample above looks good in principle. I believe what you are missing
>>>>>>>> > here is code in your driver to scan the child nodes to create the platform
>>>>>>>> > devices. of_bus_isa_translate() should work with your definition here
>>>>>>>> > and create the correct IORESOURCE_IO resources. You don't have any MMIO
>>>>>>>> > resources, so the absence of a ranges property is ok. Maybe all you
>>>>>>>> > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
>>>>>>>> >
>>>>>>>>
>>>>>>>> You are right. thanks, i'll try on test board . if i get the correct result , the new patch
>>>>>>>> will be sent later. By the way, it's my another email account use when i at home.
>>>>>>>
>>>>>>> I tried, and there need some additional changes.
>>>>>>>
>>>>>>> isa@a01b0000 {
>>>>>>>
>>>>>>> /*the node name should start with "isa", because of below definition
>>>>>>> * static int of_bus_isa_match(struct device_node *np)
>>>>>>> * {
>>>>>>> * return !strcmp(np->name, "isa");
>>>>>>> * }
>>>>>>
>>>>>> Looks good. It would be nicer to match on device_type than on name,
>>>>>> but this is ancient code and it's probably best not to touch it
>>>>>> so we don't accidentally break some old SPARC or PPC system.
>>>>>>
>>>>>>> */
>>>>>>> compatible = "low-pin-count";
>>>>>>> device_type = "isa";
>>>>>>> #address-cells = <2>;
>>>>>>> #size-cells = <1>;
>>>>>>> reg = <0x0 0xa01b0000 0x0 0x10000>;
>>>>>>> ranges = <0x1 0x0 0x0 0x0 0x1000>;
>>>>>>> /*
>>>>>>> * ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
>>>>>>> *
>>>>>>> */
>>>>>>> ipmi_0:ipmi@000000e4{
>>>>>>> device_type = "ipmi";
>>>>>>> compatible = "ipmi-bt";
>>>>>>> reg = <0x1 0x000000e4 0x4>;
>>>>>>> };
>>>>>>>
>>>>>>
>>>>>> This looks wrong: the property above says that the I/O port range is
>>>>>> translated to MMIO address 0x00000000 to 0x00010000, which is not
>>>>>> true on your hardware. I think this needs to be changed in the code
>>>>>> so the ranges property is not required for I/O ports.
>>>>>>
>>>>>>> drivers\of\address.c
>>>>>>> static int __of_address_to_resource(struct device_node *dev,
>>>>>>> const __be32 *addrp, u64 size, unsigned int flags,
>>>>>>> const char *name, struct resource *r)
>>>>>>> {
>>>>>>> u64 taddr;
>>>>>>>
>>>>>>> if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
>>>>>>> return -EINVAL;
>>>>>>> taddr = of_translate_address(dev, addrp);
>>>>>>> if (taddr == OF_BAD_ADDR)
>>>>>>> return -EINVAL;
>>>>>>> memset(r, 0, sizeof(struct resource));
>>>>>>> if (flags & IORESOURCE_IO) {
>>>>>>> unsigned long port;
>>>>>>>
>>>>>>> /*****************************************************************/
>>>>>>> /*legacy port(< 0x1000) is reserved, and need no translation here*/
>>>>>>> /*****************************************************************/
>>>>>>> if(taddr + size < PCIBIOS_MIN_IO){
>>>>>>> r->start = taddr;
>>>>>>> r->end = taddr + size - 1;
>>>>>>> }
>>>>>>
>>>>>> I don't like having a special case based on the address here,
>>>>>> the same kind of hack might be needed for PCI I/O spaces in
>>>>>> hardware that uses an indirect method like your LPC bus
>>>>>> does, and the code above will not work on any LPC implementation
>>>>>> that correctly multiplexes its I/O ports with the first PCI domain.
>>>>>>
>>>>>> I think it would be better to avoid translating the port into
>>>>>> a physical address to start with just to translate it back into
>>>>>> a port number, what we need instead is the offset between the
>>>>>> bus specific port number and the linux port number. I've added
>>>>>> Liviu to Cc, he wrote this code originally and may have some idea
>>>>>> of how we could do that.
>>>>>
>>>>> Hi,
>>>>
>>>> Hi Liviu,
>>>>
>>>> Thanks for reviewing this.
>>>>
>>>>>
>>>>> Getting back to work after a longer holiday, my brain might not be running
>>>>> at full speed here, so I'm trying to clarify things a bit here.
>>>>>
>>>>> It looks to me like Rongrong is trying to trap the inb()/outb() calls that he
>>>>> added to arm64 by patch 1/3 and redirect those operations to the memory
>>>>> mapped LPC driver. I think the whole redirection and registration of inb/outb
>>>>> ops can be made cleaner, so that the general concept resembles the DMA ops
>>>>> registration? (I have this mental picture that what Rongrong is trying to do
>>>>> is similar to what a DMA engine does, except this is slowing down things to
>>>>> byte level). If that is done properly in the parent node, then we should not
>>>>> care what the PCIBIOS_MIN_IO value is as the inb()/outb() calls will always
>>>>> go through the redirection for the children.
>>>>>
>>>>> As for the ranges property: does he wants the ipmi-bt driver to see in the
>>>>> reg property the legacy ISA I/O ports values or the CPU addresses? If the former,
>>>>> then I agree that the range property should not be required, but also the
>>>>> reg values need to be changed (drop the top bit). If the later, then the
>>>>> ranges property is required to do the proper translation.
>>>>
>>>> The former, thanks.
>>>>
>>>>>
>>>>> Rongrong, removing the ranges property and with a reg = <0xe4 0x4> property
>>>>> in the ipmi-bt node, what IO_RESOURCE type resources do you get back from
>>>>> the of_address_to_resource() translation?
>>>>
>>>> I want to get IORESOURCE_IO type resource, but if the parent node drop the
>>>> "rangs" property, the of_address_to_resource() translation will return with -EINVAL.
>>>
>>> Have you tracked what part of the code is sensitive to the presence of "ranges"
>>> property? Does of_get_address() call returns the IO_RESOURCE flag set without "ranges"?
>>>
>>
>>
>> Yes, IO_RESOURCE flag can be get without "ranges".
>> I tracked the code, it is at of_translate_one(), Below is the calling infomation.
>>
>> of_address_to_resource-> __of_address_to_resource ->of_translate_address->
>> __of_translate_address(dev, in_addr, "ranges")->of_translate_one()
>>
>>
>> static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>> struct of_bus *pbus, __be32 *addr,
>> int na, int ns, int pna, const char *rprop)
>> {
>> const __be32 *ranges;
>> unsigned int rlen;
>> int rone;
>> u64 offset = OF_BAD_ADDR;
>>
>> ranges = of_get_property(parent, rprop, &rlen);
>> if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
>> pr_debug("OF: no ranges; cannot translate\n");
>> return 1;
>> }
>> ...
>> }
>
> OK, looking at of_translate_one() comments it looks like a missing "ranges" property is
> only accepted on PowerPC. I suggest you have an empty "ranges" property in your isa
> parent node, that will signal to the OF parsing code that the mapping is 1:1. Then have
> the IPMI node use the reg = <0x0 0xe4 4>; property values instead of reg = <0x1 0xe4 4>;
But in this condition, I still can't get the right resource type IORESOURCE_IO, I just get
the MMIO resource E4:E7. Please see the url at https://lkml.org/lkml/2016/1/5/199, the empty
ranges has been discussed.
>
> Best regards,
> Liviu
>
>>
>>> Best regards,
>>> Liviu
>>>
>>>>
>>>>>
>>>>> Best regards,
>>>>> Liviu
>>>>>
>>>>>
>>>>>>
>>>>>> Arnd
>>>>>>
>>>>>
>>>> Regards,
>>>> Rongrong
>>>>
>>>
>> --
>> Regards,
>> Rongrong
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
Regards,
Rongrong
[toc] | [prev] | [next] | [standalone]
| From | liviu.dudau@arm.com |
|---|---|
| Date | 2016-01-12 12:30 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qQ5jc-cc-11@gated-at.bofh.it> |
| In reply to | #1307268 |
On Tue, Jan 12, 2016 at 07:05:29PM +0800, Rongrong Zou wrote:
> 在 2016/1/12 18:14, liviu.dudau@arm.com 写道:
> >On Tue, Jan 12, 2016 at 05:25:56PM +0800, Rongrong Zou wrote:
> >>在 2016/1/12 17:07, liviu.dudau@arm.com 写道:
> >>>On Tue, Jan 12, 2016 at 10:39:36AM +0800, Rongrong Zou wrote:
> >>>>On 2016/1/12 0:14, liviu.dudau@arm.com wrote:
> >>>>>On Mon, Jan 04, 2016 at 12:13:05PM +0100, Arnd Bergmann wrote:
> >>>>>>On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
> >>>>>>>在 2015/12/31 23:00, Rongrong Zou 写道:
> >>>>>>>>2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd@arndb.de <mailto:arnd@arndb.de>>:
> >>>>>>>> > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
> >>>>>>>> > > 在 2015/12/30 17:06, Arnd Bergmann 写道:
> >>>>>>>> > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
> >>>>>>>> >
> >>>>>>>> > The DT sample above looks good in principle. I believe what you are missing
> >>>>>>>> > here is code in your driver to scan the child nodes to create the platform
> >>>>>>>> > devices. of_bus_isa_translate() should work with your definition here
> >>>>>>>> > and create the correct IORESOURCE_IO resources. You don't have any MMIO
> >>>>>>>> > resources, so the absence of a ranges property is ok. Maybe all you
> >>>>>>>> > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
> >>>>>>>> >
> >>>>>>>>
> >>>>>>>>You are right. thanks, i'll try on test board . if i get the correct result , the new patch
> >>>>>>>>will be sent later. By the way, it's my another email account use when i at home.
> >>>>>>>
> >>>>>>>I tried, and there need some additional changes.
> >>>>>>>
> >>>>>>>isa@a01b0000 {
> >>>>>>>
> >>>>>>>/*the node name should start with "isa", because of below definition
> >>>>>>>* static int of_bus_isa_match(struct device_node *np)
> >>>>>>>* {
> >>>>>>>* return !strcmp(np->name, "isa");
> >>>>>>>* }
> >>>>>>
> >>>>>>Looks good. It would be nicer to match on device_type than on name,
> >>>>>>but this is ancient code and it's probably best not to touch it
> >>>>>>so we don't accidentally break some old SPARC or PPC system.
> >>>>>>
> >>>>>>>*/
> >>>>>>> compatible = "low-pin-count";
> >>>>>>> device_type = "isa";
> >>>>>>> #address-cells = <2>;
> >>>>>>> #size-cells = <1>;
> >>>>>>> reg = <0x0 0xa01b0000 0x0 0x10000>;
> >>>>>>> ranges = <0x1 0x0 0x0 0x0 0x1000>;
> >>>>>>>/*
> >>>>>>>* ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
> >>>>>>>*
> >>>>>>>*/
> >>>>>>> ipmi_0:ipmi@000000e4{
> >>>>>>> device_type = "ipmi";
> >>>>>>> compatible = "ipmi-bt";
> >>>>>>> reg = <0x1 0x000000e4 0x4>;
> >>>>>>>};
> >>>>>>>
> >>>>>>
> >>>>>>This looks wrong: the property above says that the I/O port range is
> >>>>>>translated to MMIO address 0x00000000 to 0x00010000, which is not
> >>>>>>true on your hardware. I think this needs to be changed in the code
> >>>>>>so the ranges property is not required for I/O ports.
> >>>>>>
> >>>>>>>drivers\of\address.c
> >>>>>>>static int __of_address_to_resource(struct device_node *dev,
> >>>>>>> const __be32 *addrp, u64 size, unsigned int flags,
> >>>>>>> const char *name, struct resource *r)
> >>>>>>>{
> >>>>>>> u64 taddr;
> >>>>>>>
> >>>>>>> if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> >>>>>>> return -EINVAL;
> >>>>>>> taddr = of_translate_address(dev, addrp);
> >>>>>>> if (taddr == OF_BAD_ADDR)
> >>>>>>> return -EINVAL;
> >>>>>>> memset(r, 0, sizeof(struct resource));
> >>>>>>> if (flags & IORESOURCE_IO) {
> >>>>>>> unsigned long port;
> >>>>>>>
> >>>>>>>/*****************************************************************/
> >>>>>>>/*legacy port(< 0x1000) is reserved, and need no translation here*/
> >>>>>>>/*****************************************************************/
> >>>>>>> if(taddr + size < PCIBIOS_MIN_IO){
> >>>>>>> r->start = taddr;
> >>>>>>> r->end = taddr + size - 1;
> >>>>>>> }
> >>>>>>
> >>>>>>I don't like having a special case based on the address here,
> >>>>>>the same kind of hack might be needed for PCI I/O spaces in
> >>>>>>hardware that uses an indirect method like your LPC bus
> >>>>>>does, and the code above will not work on any LPC implementation
> >>>>>>that correctly multiplexes its I/O ports with the first PCI domain.
> >>>>>>
> >>>>>>I think it would be better to avoid translating the port into
> >>>>>>a physical address to start with just to translate it back into
> >>>>>>a port number, what we need instead is the offset between the
> >>>>>>bus specific port number and the linux port number. I've added
> >>>>>>Liviu to Cc, he wrote this code originally and may have some idea
> >>>>>>of how we could do that.
> >>>>>
> >>>>>Hi,
> >>>>
> >>>>Hi Liviu,
> >>>>
> >>>>Thanks for reviewing this.
> >>>>
> >>>>>
> >>>>>Getting back to work after a longer holiday, my brain might not be running
> >>>>>at full speed here, so I'm trying to clarify things a bit here.
> >>>>>
> >>>>>It looks to me like Rongrong is trying to trap the inb()/outb() calls that he
> >>>>>added to arm64 by patch 1/3 and redirect those operations to the memory
> >>>>>mapped LPC driver. I think the whole redirection and registration of inb/outb
> >>>>>ops can be made cleaner, so that the general concept resembles the DMA ops
> >>>>>registration? (I have this mental picture that what Rongrong is trying to do
> >>>>>is similar to what a DMA engine does, except this is slowing down things to
> >>>>>byte level). If that is done properly in the parent node, then we should not
> >>>>>care what the PCIBIOS_MIN_IO value is as the inb()/outb() calls will always
> >>>>>go through the redirection for the children.
> >>>>>
> >>>>>As for the ranges property: does he wants the ipmi-bt driver to see in the
> >>>>>reg property the legacy ISA I/O ports values or the CPU addresses? If the former,
> >>>>>then I agree that the range property should not be required, but also the
> >>>>>reg values need to be changed (drop the top bit). If the later, then the
> >>>>>ranges property is required to do the proper translation.
> >>>>
> >>>>The former, thanks.
> >>>>
> >>>>>
> >>>>>Rongrong, removing the ranges property and with a reg = <0xe4 0x4> property
> >>>>>in the ipmi-bt node, what IO_RESOURCE type resources do you get back from
> >>>>>the of_address_to_resource() translation?
> >>>>
> >>>>I want to get IORESOURCE_IO type resource, but if the parent node drop the
> >>>>"rangs" property, the of_address_to_resource() translation will return with -EINVAL.
> >>>
> >>>Have you tracked what part of the code is sensitive to the presence of "ranges"
> >>>property? Does of_get_address() call returns the IO_RESOURCE flag set without "ranges"?
> >>>
> >>
> >>
> >>Yes, IO_RESOURCE flag can be get without "ranges".
Earlier, you said this ^
> >>I tracked the code, it is at of_translate_one(), Below is the calling infomation.
> >>
> >>of_address_to_resource-> __of_address_to_resource ->of_translate_address->
> >>__of_translate_address(dev, in_addr, "ranges")->of_translate_one()
> >>
> >>
> >>static int of_translate_one(struct device_node *parent, struct of_bus *bus,
> >> struct of_bus *pbus, __be32 *addr,
> >> int na, int ns, int pna, const char *rprop)
> >>{
> >> const __be32 *ranges;
> >> unsigned int rlen;
> >> int rone;
> >> u64 offset = OF_BAD_ADDR;
> >>
> >> ranges = of_get_property(parent, rprop, &rlen);
> >> if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> >> pr_debug("OF: no ranges; cannot translate\n");
> >> return 1;
> >> }
> >> ...
> >>}
> >
> >OK, looking at of_translate_one() comments it looks like a missing "ranges" property is
> >only accepted on PowerPC. I suggest you have an empty "ranges" property in your isa
> >parent node, that will signal to the OF parsing code that the mapping is 1:1. Then have
> >the IPMI node use the reg = <0x0 0xe4 4>; property values instead of reg = <0x1 0xe4 4>;
>
> But in this condition, I still can't get the right resource type IORESOURCE_IO, I just get
> the MMIO resource E4:E7. Please see the url at https://lkml.org/lkml/2016/1/5/199, the empty
> ranges has been discussed.
So, when you use an empty "ranges" of_get_address() doesn't return the right flags? What resource
do you actually get, MMIO is not a valid value.
Liviu
>
> --
> Regards,
> Rongrong
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
[toc] | [prev] | [next] | [standalone]
| From | Rongrong Zou <zourongrong@huawei.com> |
|---|---|
| Date | 2016-01-12 13:00 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qQ5Mf-nk-39@gated-at.bofh.it> |
| In reply to | #1307288 |
在 2016/1/12 19:27, liviu.dudau@arm.com 写道:
> On Tue, Jan 12, 2016 at 07:05:29PM +0800, Rongrong Zou wrote:
>> 在 2016/1/12 18:14, liviu.dudau@arm.com 写道:
>>> On Tue, Jan 12, 2016 at 05:25:56PM +0800, Rongrong Zou wrote:
>>>> 在 2016/1/12 17:07, liviu.dudau@arm.com 写道:
>>>>> On Tue, Jan 12, 2016 at 10:39:36AM +0800, Rongrong Zou wrote:
>>>>>> On 2016/1/12 0:14, liviu.dudau@arm.com wrote:
>>>>>>> On Mon, Jan 04, 2016 at 12:13:05PM +0100, Arnd Bergmann wrote:
>>>>>>>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>>>>>>>>> 在 2015/12/31 23:00, Rongrong Zou 写道:
>>>>>>>>>> 2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd@arndb.de <mailto:arnd@arndb.de>>:
>>>>>>>>>> > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
>>>>>>>>>> > > 在 2015/12/30 17:06, Arnd Bergmann 写道:
>>>>>>>>>> > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
>>>>>>>>>> >
>>>>>>>>>> > The DT sample above looks good in principle. I believe what you are missing
>>>>>>>>>> > here is code in your driver to scan the child nodes to create the platform
>>>>>>>>>> > devices. of_bus_isa_translate() should work with your definition here
>>>>>>>>>> > and create the correct IORESOURCE_IO resources. You don't have any MMIO
>>>>>>>>>> > resources, so the absence of a ranges property is ok. Maybe all you
>>>>>>>>>> > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
>>>>>>>>>> >
>>>>>>>>>>
>>>>>>>>>> You are right. thanks, i'll try on test board . if i get the correct result , the new patch
>>>>>>>>>> will be sent later. By the way, it's my another email account use when i at home.
>>>>>>>>>
>>>>>>>>> I tried, and there need some additional changes.
>>>>>>>>>
>>>>>>>>> isa@a01b0000 {
>>>>>>>>>
>>>>>>>>> /*the node name should start with "isa", because of below definition
>>>>>>>>> * static int of_bus_isa_match(struct device_node *np)
>>>>>>>>> * {
>>>>>>>>> * return !strcmp(np->name, "isa");
>>>>>>>>> * }
>>>>>>>>
>>>>>>>> Looks good. It would be nicer to match on device_type than on name,
>>>>>>>> but this is ancient code and it's probably best not to touch it
>>>>>>>> so we don't accidentally break some old SPARC or PPC system.
>>>>>>>>
>>>>>>>>> */
>>>>>>>>> compatible = "low-pin-count";
>>>>>>>>> device_type = "isa";
>>>>>>>>> #address-cells = <2>;
>>>>>>>>> #size-cells = <1>;
>>>>>>>>> reg = <0x0 0xa01b0000 0x0 0x10000>;
>>>>>>>>> ranges = <0x1 0x0 0x0 0x0 0x1000>;
>>>>>>>>> /*
>>>>>>>>> * ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
>>>>>>>>> *
>>>>>>>>> */
>>>>>>>>> ipmi_0:ipmi@000000e4{
>>>>>>>>> device_type = "ipmi";
>>>>>>>>> compatible = "ipmi-bt";
>>>>>>>>> reg = <0x1 0x000000e4 0x4>;
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>
>>>>>>>> This looks wrong: the property above says that the I/O port range is
>>>>>>>> translated to MMIO address 0x00000000 to 0x00010000, which is not
>>>>>>>> true on your hardware. I think this needs to be changed in the code
>>>>>>>> so the ranges property is not required for I/O ports.
>>>>>>>>
>>>>>>>>> drivers\of\address.c
>>>>>>>>> static int __of_address_to_resource(struct device_node *dev,
>>>>>>>>> const __be32 *addrp, u64 size, unsigned int flags,
>>>>>>>>> const char *name, struct resource *r)
>>>>>>>>> {
>>>>>>>>> u64 taddr;
>>>>>>>>>
>>>>>>>>> if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
>>>>>>>>> return -EINVAL;
>>>>>>>>> taddr = of_translate_address(dev, addrp);
>>>>>>>>> if (taddr == OF_BAD_ADDR)
>>>>>>>>> return -EINVAL;
>>>>>>>>> memset(r, 0, sizeof(struct resource));
>>>>>>>>> if (flags & IORESOURCE_IO) {
>>>>>>>>> unsigned long port;
>>>>>>>>>
>>>>>>>>> /*****************************************************************/
>>>>>>>>> /*legacy port(< 0x1000) is reserved, and need no translation here*/
>>>>>>>>> /*****************************************************************/
>>>>>>>>> if(taddr + size < PCIBIOS_MIN_IO){
>>>>>>>>> r->start = taddr;
>>>>>>>>> r->end = taddr + size - 1;
>>>>>>>>> }
>>>>>>>>
>>>>>>>> I don't like having a special case based on the address here,
>>>>>>>> the same kind of hack might be needed for PCI I/O spaces in
>>>>>>>> hardware that uses an indirect method like your LPC bus
>>>>>>>> does, and the code above will not work on any LPC implementation
>>>>>>>> that correctly multiplexes its I/O ports with the first PCI domain.
>>>>>>>>
>>>>>>>> I think it would be better to avoid translating the port into
>>>>>>>> a physical address to start with just to translate it back into
>>>>>>>> a port number, what we need instead is the offset between the
>>>>>>>> bus specific port number and the linux port number. I've added
>>>>>>>> Liviu to Cc, he wrote this code originally and may have some idea
>>>>>>>> of how we could do that.
>>>>>>>
>>>>>>> Hi,
>>>>>>
>>>>>> Hi Liviu,
>>>>>>
>>>>>> Thanks for reviewing this.
>>>>>>
>>>>>>>
>>>>>>> Getting back to work after a longer holiday, my brain might not be running
>>>>>>> at full speed here, so I'm trying to clarify things a bit here.
>>>>>>>
>>>>>>> It looks to me like Rongrong is trying to trap the inb()/outb() calls that he
>>>>>>> added to arm64 by patch 1/3 and redirect those operations to the memory
>>>>>>> mapped LPC driver. I think the whole redirection and registration of inb/outb
>>>>>>> ops can be made cleaner, so that the general concept resembles the DMA ops
>>>>>>> registration? (I have this mental picture that what Rongrong is trying to do
>>>>>>> is similar to what a DMA engine does, except this is slowing down things to
>>>>>>> byte level). If that is done properly in the parent node, then we should not
>>>>>>> care what the PCIBIOS_MIN_IO value is as the inb()/outb() calls will always
>>>>>>> go through the redirection for the children.
>>>>>>>
>>>>>>> As for the ranges property: does he wants the ipmi-bt driver to see in the
>>>>>>> reg property the legacy ISA I/O ports values or the CPU addresses? If the former,
>>>>>>> then I agree that the range property should not be required, but also the
>>>>>>> reg values need to be changed (drop the top bit). If the later, then the
>>>>>>> ranges property is required to do the proper translation.
>>>>>>
>>>>>> The former, thanks.
>>>>>>
>>>>>>>
>>>>>>> Rongrong, removing the ranges property and with a reg = <0xe4 0x4> property
>>>>>>> in the ipmi-bt node, what IO_RESOURCE type resources do you get back from
>>>>>>> the of_address_to_resource() translation?
>>>>>>
>>>>>> I want to get IORESOURCE_IO type resource, but if the parent node drop the
>>>>>> "rangs" property, the of_address_to_resource() translation will return with -EINVAL.
>>>>>
>>>>> Have you tracked what part of the code is sensitive to the presence of "ranges"
>>>>> property? Does of_get_address() call returns the IO_RESOURCE flag set without "ranges"?
>>>>>
>>>>
>>>>
>>>> Yes, IO_RESOURCE flag can be get without "ranges".
>
> Earlier, you said this ^
>
>>>> I tracked the code, it is at of_translate_one(), Below is the calling infomation.
>>>>
>>>> of_address_to_resource-> __of_address_to_resource ->of_translate_address->
>>>> __of_translate_address(dev, in_addr, "ranges")->of_translate_one()
>>>>
>>>>
>>>> static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>>>> struct of_bus *pbus, __be32 *addr,
>>>> int na, int ns, int pna, const char *rprop)
>>>> {
>>>> const __be32 *ranges;
>>>> unsigned int rlen;
>>>> int rone;
>>>> u64 offset = OF_BAD_ADDR;
>>>>
>>>> ranges = of_get_property(parent, rprop, &rlen);
>>>> if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
>>>> pr_debug("OF: no ranges; cannot translate\n");
>>>> return 1;
>>>> }
>>>> ...
>>>> }
>>>
>>> OK, looking at of_translate_one() comments it looks like a missing "ranges" property is
>>> only accepted on PowerPC. I suggest you have an empty "ranges" property in your isa
>>> parent node, that will signal to the OF parsing code that the mapping is 1:1. Then have
>>> the IPMI node use the reg = <0x0 0xe4 4>; property values instead of reg = <0x1 0xe4 4>;
>>
>> But in this condition, I still can't get the right resource type IORESOURCE_IO, I just get
>> the MMIO resource E4:E7. Please see the url at https://lkml.org/lkml/2016/1/5/199, the empty
>> ranges has been discussed.
>
> So, when you use an empty "ranges" of_get_address() doesn't return the right flags? What resource
> do you actually get, MMIO is not a valid value.
I'm sorry I did not describe clearly.
Without "ranges", of_get_address() will return with valid value, but of_address_to_resource()
will return with -EINVAL;
int of_address_to_resource(struct device_node *dev, int index,
struct resource *r)
{
...
/* flags can be get here, without ranges property reqired.
* if the reg = <0x0 0xe4 4>, I can get flag of IORESOURCE_MEM,
* if the reg = <0x1 0xe4 4>, I can get flag of IORESOURCE_IO,
*/
addrp = of_get_address(dev, index, &size, &flags);
if (addrp == NULL)
return -EINVAL;
/* Get optional "reg-names" property to add a name to a resource */
of_property_read_string_index(dev, "reg-names", index, &name);
/* If it is empty ranges, and the flag is IORESOURCE_MEM then the below __of_address_to_resource return with valid addr,
* if it is empty ranges, and the flag is IORESOURCE_IO then return with -EINVAL, because
* pci_address_to_pio() will be called when flag is IORESOURCE_IO.
* if the ranges is absent, then return with -EINVAL.
*/
return __of_address_to_resource(dev, addrp, size, flags, name, r);
}
I want to get resource with flag = IORESOURCE_IO, resource.start=0XE4, resource.size=0X4,
>
> Liviu
>
>>
>> --
>> Regards,
>> Rongrong
>>
>
--
Regards,
Rongrong
[toc] | [prev] | [next] | [standalone]
| From | liviu.dudau@arm.com |
|---|---|
| Date | 2016-01-12 16:20 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qQ8TL-2LK-7@gated-at.bofh.it> |
| In reply to | #1307324 |
On Tue, Jan 12, 2016 at 07:56:47PM +0800, Rongrong Zou wrote:
> 在 2016/1/12 19:27, liviu.dudau@arm.com 写道:
> >On Tue, Jan 12, 2016 at 07:05:29PM +0800, Rongrong Zou wrote:
> >>在 2016/1/12 18:14, liviu.dudau@arm.com 写道:
> >>>On Tue, Jan 12, 2016 at 05:25:56PM +0800, Rongrong Zou wrote:
> >>>>在 2016/1/12 17:07, liviu.dudau@arm.com 写道:
> >>>>>On Tue, Jan 12, 2016 at 10:39:36AM +0800, Rongrong Zou wrote:
> >>>>>>On 2016/1/12 0:14, liviu.dudau@arm.com wrote:
> >>>>>>>On Mon, Jan 04, 2016 at 12:13:05PM +0100, Arnd Bergmann wrote:
> >>>>>>>>On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
> >>>>>>>>>在 2015/12/31 23:00, Rongrong Zou 写道:
> >>>>>>>>>>2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd@arndb.de <mailto:arnd@arndb.de>>:
> >>>>>>>>>> > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
> >>>>>>>>>> > > 在 2015/12/30 17:06, Arnd Bergmann 写道:
> >>>>>>>>>> > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
> >>>>>>>>>> >
> >>>>>>>>>> > The DT sample above looks good in principle. I believe what you are missing
> >>>>>>>>>> > here is code in your driver to scan the child nodes to create the platform
> >>>>>>>>>> > devices. of_bus_isa_translate() should work with your definition here
> >>>>>>>>>> > and create the correct IORESOURCE_IO resources. You don't have any MMIO
> >>>>>>>>>> > resources, so the absence of a ranges property is ok. Maybe all you
> >>>>>>>>>> > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
> >>>>>>>>>> >
> >>>>>>>>>>
> >>>>>>>>>>You are right. thanks, i'll try on test board . if i get the correct result , the new patch
> >>>>>>>>>>will be sent later. By the way, it's my another email account use when i at home.
> >>>>>>>>>
> >>>>>>>>>I tried, and there need some additional changes.
> >>>>>>>>>
> >>>>>>>>>isa@a01b0000 {
> >>>>>>>>>
> >>>>>>>>>/*the node name should start with "isa", because of below definition
> >>>>>>>>>* static int of_bus_isa_match(struct device_node *np)
> >>>>>>>>>* {
> >>>>>>>>>* return !strcmp(np->name, "isa");
> >>>>>>>>>* }
> >>>>>>>>
> >>>>>>>>Looks good. It would be nicer to match on device_type than on name,
> >>>>>>>>but this is ancient code and it's probably best not to touch it
> >>>>>>>>so we don't accidentally break some old SPARC or PPC system.
> >>>>>>>>
> >>>>>>>>>*/
> >>>>>>>>> compatible = "low-pin-count";
> >>>>>>>>> device_type = "isa";
> >>>>>>>>> #address-cells = <2>;
> >>>>>>>>> #size-cells = <1>;
> >>>>>>>>> reg = <0x0 0xa01b0000 0x0 0x10000>;
> >>>>>>>>> ranges = <0x1 0x0 0x0 0x0 0x1000>;
> >>>>>>>>>/*
> >>>>>>>>>* ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
> >>>>>>>>>*
> >>>>>>>>>*/
> >>>>>>>>> ipmi_0:ipmi@000000e4{
> >>>>>>>>> device_type = "ipmi";
> >>>>>>>>> compatible = "ipmi-bt";
> >>>>>>>>> reg = <0x1 0x000000e4 0x4>;
> >>>>>>>>>};
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>>This looks wrong: the property above says that the I/O port range is
> >>>>>>>>translated to MMIO address 0x00000000 to 0x00010000, which is not
> >>>>>>>>true on your hardware. I think this needs to be changed in the code
> >>>>>>>>so the ranges property is not required for I/O ports.
> >>>>>>>>
> >>>>>>>>>drivers\of\address.c
> >>>>>>>>>static int __of_address_to_resource(struct device_node *dev,
> >>>>>>>>> const __be32 *addrp, u64 size, unsigned int flags,
> >>>>>>>>> const char *name, struct resource *r)
> >>>>>>>>>{
> >>>>>>>>> u64 taddr;
> >>>>>>>>>
> >>>>>>>>> if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> >>>>>>>>> return -EINVAL;
> >>>>>>>>> taddr = of_translate_address(dev, addrp);
> >>>>>>>>> if (taddr == OF_BAD_ADDR)
> >>>>>>>>> return -EINVAL;
> >>>>>>>>> memset(r, 0, sizeof(struct resource));
> >>>>>>>>> if (flags & IORESOURCE_IO) {
> >>>>>>>>> unsigned long port;
> >>>>>>>>>
> >>>>>>>>>/*****************************************************************/
> >>>>>>>>>/*legacy port(< 0x1000) is reserved, and need no translation here*/
> >>>>>>>>>/*****************************************************************/
> >>>>>>>>> if(taddr + size < PCIBIOS_MIN_IO){
> >>>>>>>>> r->start = taddr;
> >>>>>>>>> r->end = taddr + size - 1;
> >>>>>>>>> }
> >>>>>>>>
> >>>>>>>>I don't like having a special case based on the address here,
> >>>>>>>>the same kind of hack might be needed for PCI I/O spaces in
> >>>>>>>>hardware that uses an indirect method like your LPC bus
> >>>>>>>>does, and the code above will not work on any LPC implementation
> >>>>>>>>that correctly multiplexes its I/O ports with the first PCI domain.
> >>>>>>>>
> >>>>>>>>I think it would be better to avoid translating the port into
> >>>>>>>>a physical address to start with just to translate it back into
> >>>>>>>>a port number, what we need instead is the offset between the
> >>>>>>>>bus specific port number and the linux port number. I've added
> >>>>>>>>Liviu to Cc, he wrote this code originally and may have some idea
> >>>>>>>>of how we could do that.
> >>>>>>>
> >>>>>>>Hi,
> >>>>>>
> >>>>>>Hi Liviu,
> >>>>>>
> >>>>>>Thanks for reviewing this.
> >>>>>>
> >>>>>>>
> >>>>>>>Getting back to work after a longer holiday, my brain might not be running
> >>>>>>>at full speed here, so I'm trying to clarify things a bit here.
> >>>>>>>
> >>>>>>>It looks to me like Rongrong is trying to trap the inb()/outb() calls that he
> >>>>>>>added to arm64 by patch 1/3 and redirect those operations to the memory
> >>>>>>>mapped LPC driver. I think the whole redirection and registration of inb/outb
> >>>>>>>ops can be made cleaner, so that the general concept resembles the DMA ops
> >>>>>>>registration? (I have this mental picture that what Rongrong is trying to do
> >>>>>>>is similar to what a DMA engine does, except this is slowing down things to
> >>>>>>>byte level). If that is done properly in the parent node, then we should not
> >>>>>>>care what the PCIBIOS_MIN_IO value is as the inb()/outb() calls will always
> >>>>>>>go through the redirection for the children.
> >>>>>>>
> >>>>>>>As for the ranges property: does he wants the ipmi-bt driver to see in the
> >>>>>>>reg property the legacy ISA I/O ports values or the CPU addresses? If the former,
> >>>>>>>then I agree that the range property should not be required, but also the
> >>>>>>>reg values need to be changed (drop the top bit). If the later, then the
> >>>>>>>ranges property is required to do the proper translation.
> >>>>>>
> >>>>>>The former, thanks.
> >>>>>>
> >>>>>>>
> >>>>>>>Rongrong, removing the ranges property and with a reg = <0xe4 0x4> property
> >>>>>>>in the ipmi-bt node, what IO_RESOURCE type resources do you get back from
> >>>>>>>the of_address_to_resource() translation?
> >>>>>>
> >>>>>>I want to get IORESOURCE_IO type resource, but if the parent node drop the
> >>>>>>"rangs" property, the of_address_to_resource() translation will return with -EINVAL.
> >>>>>
> >>>>>Have you tracked what part of the code is sensitive to the presence of "ranges"
> >>>>>property? Does of_get_address() call returns the IO_RESOURCE flag set without "ranges"?
> >>>>>
> >>>>
> >>>>
> >>>>Yes, IO_RESOURCE flag can be get without "ranges".
> >
> >Earlier, you said this ^
> >
> >>>>I tracked the code, it is at of_translate_one(), Below is the calling infomation.
> >>>>
> >>>>of_address_to_resource-> __of_address_to_resource ->of_translate_address->
> >>>>__of_translate_address(dev, in_addr, "ranges")->of_translate_one()
> >>>>
> >>>>
> >>>>static int of_translate_one(struct device_node *parent, struct of_bus *bus,
> >>>> struct of_bus *pbus, __be32 *addr,
> >>>> int na, int ns, int pna, const char *rprop)
> >>>>{
> >>>> const __be32 *ranges;
> >>>> unsigned int rlen;
> >>>> int rone;
> >>>> u64 offset = OF_BAD_ADDR;
> >>>>
> >>>> ranges = of_get_property(parent, rprop, &rlen);
> >>>> if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> >>>> pr_debug("OF: no ranges; cannot translate\n");
> >>>> return 1;
> >>>> }
> >>>> ...
> >>>>}
> >>>
> >>>OK, looking at of_translate_one() comments it looks like a missing "ranges" property is
> >>>only accepted on PowerPC. I suggest you have an empty "ranges" property in your isa
> >>>parent node, that will signal to the OF parsing code that the mapping is 1:1. Then have
> >>>the IPMI node use the reg = <0x0 0xe4 4>; property values instead of reg = <0x1 0xe4 4>;
> >>
> >>But in this condition, I still can't get the right resource type IORESOURCE_IO, I just get
> >>the MMIO resource E4:E7. Please see the url at https://lkml.org/lkml/2016/1/5/199, the empty
> >>ranges has been discussed.
> >
> >So, when you use an empty "ranges" of_get_address() doesn't return the right flags? What resource
> >do you actually get, MMIO is not a valid value.
>
>
>
> I'm sorry I did not describe clearly.
> Without "ranges", of_get_address() will return with valid value, but of_address_to_resource()
> will return with -EINVAL;
>
> int of_address_to_resource(struct device_node *dev, int index,
> struct resource *r)
> {
> ...
> /* flags can be get here, without ranges property reqired.
> * if the reg = <0x0 0xe4 4>, I can get flag of IORESOURCE_MEM,
> * if the reg = <0x1 0xe4 4>, I can get flag of IORESOURCE_IO,
That is strange, the parent node has #address-cells = <2> so the first two numbers should be part
of the address and not influence the flags. Can you add some debugging in of_get_address() and
try to figure out what bus is used in *flags = bus->get_flags(prop) ?
> */
> addrp = of_get_address(dev, index, &size, &flags);
>
> if (addrp == NULL)
> return -EINVAL;
>
> /* Get optional "reg-names" property to add a name to a resource */
> of_property_read_string_index(dev, "reg-names", index, &name);
>
>
> /* If it is empty ranges, and the flag is IORESOURCE_MEM then the below __of_address_to_resource return with valid addr,
> * if it is empty ranges, and the flag is IORESOURCE_IO then return with -EINVAL, because
> * pci_address_to_pio() will be called when flag is IORESOURCE_IO.
> * if the ranges is absent, then return with -EINVAL.
> */
> return __of_address_to_resource(dev, addrp, size, flags, name, r);
> }
>
> I want to get resource with flag = IORESOURCE_IO, resource.start=0XE4, resource.size=0X4,
Yes, that is what I expect you should get as well.
Liviu
>
> >
> >Liviu
> >
> >>
> >>--
> >>Regards,
> >>Rongrong
> >>
> >
>
>
> --
> Regards,
> Rongrong
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-01-13 00:00 +0100 |
| Subject | Re: [PATCH v1 3/3] ARM64 LPC: update binding doc |
| Message-ID | <qQg4W-7wg-19@gated-at.bofh.it> |
| In reply to | #1307524 |
On Tuesday 12 January 2016 15:13:35 liviu.dudau@arm.com wrote:
> > int of_address_to_resource(struct device_node *dev, int index,
> > struct resource *r)
> > {
> > ...
> > /* flags can be get here, without ranges property reqired.
> > * if the reg = <0x0 0xe4 4>, I can get flag of IORESOURCE_MEM,
> > * if the reg = <0x1 0xe4 4>, I can get flag of IORESOURCE_IO,
>
> That is strange, the parent node has #address-cells = <2> so the first two numbers should be part
> of the address and not influence the flags. Can you add some debugging in of_get_address() and
> try to figure out what bus is used in *flags = bus->get_flags(prop) ?
>
>
This is the standard ISA binding. The first cell is the address space
(IO or MEM), the second cell is the address within that space. This
is similar to how PCI works.
Arnd
[toc] | [prev] | [next] | [standalone]
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
Back to top | Article view | linux.kernel
csiph-web