Re: Need some guidance on i2c-ocores driver

Re: Need some guidance on i2c-ocores driver

[York Sun]

Resend to LKML

Lee,

This question is actually more about MFD. Can you point me to the possible
causes for my failure below?

Thanks.

York


On 04/16/2015 05:02 PM, York Sun wrote:
> Julia and other experts,
> 
> I am seeking for help on my device driver.
> 
> I am working on a module driver for a FPGA design with open core I2C controller
> memory-mapped to BAR2. I have searched up and down and found only one driver
> (drivers/mfd/timberdale.c) with similar implementation. Following timberdale
> driver, I am able to load the driver, but blocked by devm_ioremap_resource(). It
> is always in conflict with my BAR2. I wonder if I did something wrong. Here is
> the flow I tracked so far. (By the way, I am using an older kernel 3.12. The new
> 4.0 kernel crashes when booting on my platform. I plan to move to newer kernel
> later).
> 
> mfd_add_devices()
> |
> |--mfd_add_device()
>     |
>     |--platform_device_add()
>         |
>         |--insert_resource()  /* this passed */
>         |   |
>         |   |--insert_resource_conflict()
>         |
>         |--device_add()
>             |
>             |--bus_probe_device()
>                 |
>                 |--devm_ioremap_resource()
>                     |
>                     |--devm_request_mem_region()
>                         |
>                         |--__request_region() /* this always conflicts */
>                             |
>                             |--__request_resource()
> 
> My driver is called RedStone DMA. Here is my debug output
> 
>> root@p1022ds:~# insmod redstone_mfd.ko 
>> RedStone DMA 0002:05:00.0: pci_enable_device() successful
>> RedStone DMA 0002:05:00.0: MSI-X init successful
>> York kernel: Calling devm_ioremap_resource()
>> York kernel kernel/resource.c __request_region 977: conflict=[??? 0xc00080000-0xc00087fff flags 0x80000000]
>> ocores-i2c ocores-i2c: can't request region for resource [mem 0xc00086000-0xc0008601f]
>> ocores-i2c: probe of ocores-i2c failed with error -16
>> RedStone DMA 0002:05:00.0: BAR[0] 0x0000000c00000000-0x0000000c0007ffff flags 0x0014220c, length 524288
>> RedStone DMA 0002:05:00.0: BAR[1] 0x0000000000000000-0x0000000000000000 flags 0x00000000, length 0
>> RedStone DMA 0002:05:00.0: BAR[2] 0x0000000c00080000-0x0000000c00087fff flags 0x00040200, length 32768
>> RedStone DMA 0002:05:00.0: BAR[3] 0x0000000000000000-0x0000000000000000 flags 0x00000000, length 0
>> RedStone DMA 0002:05:00.0: BAR[4] 0x0000000000000000-0x0000000000000000 flags 0x00000000, length 0
>> RedStone DMA 0002:05:00.0: BAR[5] 0x0000000000000000-0x0000000000000000 flags 0x00000000, length 0
>> RedStone DMA 0002:05:00.0: Version 1.4, built on 4-16-15, id 0
>> root@p1022ds:~# 
> 
> Can you shed some light on this?
> 
> Thanks.
> 
> York
> 

Re: Need some guidance on i2c-ocores driver

[Lee Jones]

On Fri, 17 Apr 2015, York Sun wrote:

> Resend to LKML
> 
> Lee,
> 
> This question is actually more about MFD. Can you point me to the possible
> causes for my failure below?

It's hard to tell exactly without code, but it looks like you're
trying to allocate overlapping memory regions.  Double check all of
your addresses.  For DT you need to take a look at your 'reg'
properties, for traditional platform data it's best to grep for
IORESOURCE_MEM.

> Thanks.
> 
> York
> 
> 
> On 04/16/2015 05:02 PM, York Sun wrote:
> > Julia and other experts,
> > 
> > I am seeking for help on my device driver.
> > 
> > I am working on a module driver for a FPGA design with open core I2C controller
> > memory-mapped to BAR2. I have searched up and down and found only one driver
> > (drivers/mfd/timberdale.c) with similar implementation. Following timberdale
> > driver, I am able to load the driver, but blocked by devm_ioremap_resource(). It
> > is always in conflict with my BAR2. I wonder if I did something wrong. Here is
> > the flow I tracked so far. (By the way, I am using an older kernel 3.12. The new
> > 4.0 kernel crashes when booting on my platform. I plan to move to newer kernel
> > later).
> > 
> > mfd_add_devices()
> > |
> > |--mfd_add_device()
> >     |
> >     |--platform_device_add()
> >         |
> >         |--insert_resource()  /* this passed */
> >         |   |
> >         |   |--insert_resource_conflict()
> >         |
> >         |--device_add()
> >             |
> >             |--bus_probe_device()
> >                 |
> >                 |--devm_ioremap_resource()
> >                     |
> >                     |--devm_request_mem_region()
> >                         |
> >                         |--__request_region() /* this always conflicts */
> >                             |
> >                             |--__request_resource()
> > 
> > My driver is called RedStone DMA. Here is my debug output
> > 
> >> root@p1022ds:~# insmod redstone_mfd.ko 
> >> RedStone DMA 0002:05:00.0: pci_enable_device() successful
> >> RedStone DMA 0002:05:00.0: MSI-X init successful
> >> York kernel: Calling devm_ioremap_resource()
> >> York kernel kernel/resource.c __request_region 977: conflict=[??? 0xc00080000-0xc00087fff flags 0x80000000]
> >> ocores-i2c ocores-i2c: can't request region for resource [mem 0xc00086000-0xc0008601f]
> >> ocores-i2c: probe of ocores-i2c failed with error -16
> >> RedStone DMA 0002:05:00.0: BAR[0] 0x0000000c00000000-0x0000000c0007ffff flags 0x0014220c, length 524288
> >> RedStone DMA 0002:05:00.0: BAR[1] 0x0000000000000000-0x0000000000000000 flags 0x00000000, length 0
> >> RedStone DMA 0002:05:00.0: BAR[2] 0x0000000c00080000-0x0000000c00087fff flags 0x00040200, length 32768
> >> RedStone DMA 0002:05:00.0: BAR[3] 0x0000000000000000-0x0000000000000000 flags 0x00000000, length 0
> >> RedStone DMA 0002:05:00.0: BAR[4] 0x0000000000000000-0x0000000000000000 flags 0x00000000, length 0
> >> RedStone DMA 0002:05:00.0: BAR[5] 0x0000000000000000-0x0000000000000000 flags 0x00000000, length 0
> >> RedStone DMA 0002:05:00.0: Version 1.4, built on 4-16-15, id 0
> >> root@p1022ds:~# 
> > 
> > Can you shed some light on this?
> > 
> > Thanks.
> > 
> > York
> > 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: Need some guidance on i2c-ocores driver

[York Sun]

On 04/19/2015 11:42 PM, Lee Jones wrote:
> On Fri, 17 Apr 2015, York Sun wrote:
> 
>> Resend to LKML
>>
>> Lee,
>>
>> This question is actually more about MFD. Can you point me to the possible
>> causes for my failure below?
> 
> It's hard to tell exactly without code, but it looks like you're
> trying to allocate overlapping memory regions.  Double check all of
> your addresses.  For DT you need to take a look at your 'reg'
> properties, for traditional platform data it's best to grep for
> IORESOURCE_MEM.
> 
Lee,

It _is_ overlapping. How could it not be? The resource for the I2C is mapped to
BAR2. So the resource is overlapping with BAR2. It is alway the case, isn't it?
What I don't understand is how MFD works with the resources if it is guaranteed
overlapping. Did I get something wrong?

Look at the reference code I took, drivers/mfd/timberdale.c, when
mfd_add_devices() is called, it uses &dev->resource as the base. So the BAR will
be the parent. Check the code in mfd-core.c, mfd_add_device(),

if ((cell->resources[r].flags & IORESOURCE_MEM) && mem_base) {
	res[r].parent = mem_base;
	res[r].start = mem_base->start + cell->resources[r].start;
	res[r].end = mem_base->start + cell->resources[r].end;
}

So the MFD resource is within its parent. When later the device driver request a
region, will it get conflict with the parent?

York

Re: Need some guidance on i2c-ocores driver

[Lee Jones]

On Mon, 20 Apr 2015, York Sun wrote:

> 
> 
> On 04/19/2015 11:42 PM, Lee Jones wrote:
> > On Fri, 17 Apr 2015, York Sun wrote:
> > 
> >> Resend to LKML
> >>
> >> Lee,
> >>
> >> This question is actually more about MFD. Can you point me to the possible
> >> causes for my failure below?
> > 
> > It's hard to tell exactly without code, but it looks like you're
> > trying to allocate overlapping memory regions.  Double check all of
> > your addresses.  For DT you need to take a look at your 'reg'
> > properties, for traditional platform data it's best to grep for
> > IORESOURCE_MEM.
> > 
> Lee,
> 
> It _is_ overlapping. How could it not be? The resource for the I2C is mapped to
> BAR2. So the resource is overlapping with BAR2. It is alway the case, isn't it?
> What I don't understand is how MFD works with the resources if it is guaranteed
> overlapping. Did I get something wrong?
> 
> Look at the reference code I took, drivers/mfd/timberdale.c, when
> mfd_add_devices() is called, it uses &dev->resource as the base. So the BAR will
> be the parent. Check the code in mfd-core.c, mfd_add_device(),
> 
> if ((cell->resources[r].flags & IORESOURCE_MEM) && mem_base) {
> 	res[r].parent = mem_base;
> 	res[r].start = mem_base->start + cell->resources[r].start;
> 	res[r].end = mem_base->start + cell->resources[r].end;
> }
> 
> So the MFD resource is within its parent. When later the device driver request a
> region, will it get conflict with the parent?

I doubt you'll want to map the same memory area in both the parent and
the child device drivers.  Only map the registers you plan to use in
the driver you plan to use them.  If you need multiple devices to
access the same registers then you need to create an API, complete
with locking, in the MFD parent device.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: Need some guidance on i2c-ocores driver

[York Sun]

On 04/20/2015 11:16 AM, Lee Jones wrote:
> On Mon, 20 Apr 2015, York Sun wrote:
> 
>>
>>
>> On 04/19/2015 11:42 PM, Lee Jones wrote:
>>> On Fri, 17 Apr 2015, York Sun wrote:
>>>
>>>> Resend to LKML
>>>>
>>>> Lee,
>>>>
>>>> This question is actually more about MFD. Can you point me to the possible
>>>> causes for my failure below?
>>>
>>> It's hard to tell exactly without code, but it looks like you're
>>> trying to allocate overlapping memory regions.  Double check all of
>>> your addresses.  For DT you need to take a look at your 'reg'
>>> properties, for traditional platform data it's best to grep for
>>> IORESOURCE_MEM.
>>>
>> Lee,
>>
>> It _is_ overlapping. How could it not be? The resource for the I2C is mapped to
>> BAR2. So the resource is overlapping with BAR2. It is alway the case, isn't it?
>> What I don't understand is how MFD works with the resources if it is guaranteed
>> overlapping. Did I get something wrong?
>>
>> Look at the reference code I took, drivers/mfd/timberdale.c, when
>> mfd_add_devices() is called, it uses &dev->resource as the base. So the BAR will
>> be the parent. Check the code in mfd-core.c, mfd_add_device(),
>>
>> if ((cell->resources[r].flags & IORESOURCE_MEM) && mem_base) {
>> 	res[r].parent = mem_base;
>> 	res[r].start = mem_base->start + cell->resources[r].start;
>> 	res[r].end = mem_base->start + cell->resources[r].end;
>> }
>>
>> So the MFD resource is within its parent. When later the device driver request a
>> region, will it get conflict with the parent?
> 
> I doubt you'll want to map the same memory area in both the parent and
> the child device drivers.  Only map the registers you plan to use in
> the driver you plan to use them.  If you need multiple devices to
> access the same registers then you need to create an API, complete
> with locking, in the MFD parent device.
> 

Thanks for pointing out. I thought at first the conflict was due to my
pci_ioremap_bar(). I went ahead to remove the mapping but still not working.
Your email inspired me to take a deeper look at my code and I found my error. I
have called pci_request_regions() which reserves all BARs. I think that's my
root cause. Thanks for helping me.

York

Re: Need some guidance on i2c-ocores driver

[Lee Jones]

> >> mfd_add_devices() is called, it uses &dev->resource as the base. So the BAR will
> >> be the parent. Check the code in mfd-core.c, mfd_add_device(),
> >>
> >> if ((cell->resources[r].flags & IORESOURCE_MEM) && mem_base) {
> >> 	res[r].parent = mem_base;
> >> 	res[r].start = mem_base->start + cell->resources[r].start;
> >> 	res[r].end = mem_base->start + cell->resources[r].end;
> >> }
> >>
> >> So the MFD resource is within its parent. When later the device driver request a
> >> region, will it get conflict with the parent?
> > 
> > I doubt you'll want to map the same memory area in both the parent and
> > the child device drivers.  Only map the registers you plan to use in
> > the driver you plan to use them.  If you need multiple devices to
> > access the same registers then you need to create an API, complete
> > with locking, in the MFD parent device.
> 
> Thanks for pointing out. I thought at first the conflict was due to my
> pci_ioremap_bar(). I went ahead to remove the mapping but still not working.
> Your email inspired me to take a deeper look at my code and I found my error. I
> have called pci_request_regions() which reserves all BARs. I think that's my
> root cause. Thanks for helping me.

You are welcome.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Why isn't IRQ shared for i2c-ocore

[York Sun]

Lee,

I have a follow up question regarding interrupt. I see many I2C bus drivers
request interrupt with flag = 0. Why not using IRQF_SHARED?

I understand the performance concern. If there is any other reason, I want to
know about it before I go too far on my driver.

York

Re: Why isn't IRQ shared for i2c-ocore

[Lee Jones]

> I have a follow up question regarding interrupt. I see many I2C bus drivers
> request interrupt with flag = 0. Why not using IRQF_SHARED?

Probably because that particular IRQ is only used by the I2C
Controller.  I'm not exactly sure that you're getting at?  Why do you
think it should be shared?  You should only flag it as shared if it
is.

> I understand the performance concern. If there is any other reason, I want to
> know about it before I go too far on my driver.

This sort of stuff is trivial to fix and shouldn't stand in the way
of you writing and submitting v1 of your driver.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: Why isn't IRQ shared for i2c-ocore

[York Sun]

On 05/07/2015 12:01 AM, Lee Jones wrote:
>> I have a follow up question regarding interrupt. I see many I2C bus drivers
>> request interrupt with flag = 0. Why not using IRQF_SHARED?
> 
> Probably because that particular IRQ is only used by the I2C
> Controller.  I'm not exactly sure that you're getting at?  Why do you
> think it should be shared?  You should only flag it as shared if it
> is.

I am working on a driver for multifunction card with open core I2C controller.
The interrupt is shared between I2C and other controller.

> 
>> I understand the performance concern. If there is any other reason, I want to
>> know about it before I go too far on my driver.
> 
> This sort of stuff is trivial to fix and shouldn't stand in the way
> of you writing and submitting v1 of your driver.
> 

It is trivial to fix. I just want to know if there is any reason the interrupt
wasn't shared. Seems not.

York

Re: Why isn't IRQ shared for i2c-ocore

[Geert Uytterhoeven]

On Thu, May 7, 2015 at 9:01 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> I have a follow up question regarding interrupt. I see many I2C bus drivers
>> request interrupt with flag = 0. Why not using IRQF_SHARED?
>
> Probably because that particular IRQ is only used by the I2C
> Controller.  I'm not exactly sure that you're getting at?  Why do you
> think it should be shared?  You should only flag it as shared if it
> is.

However, that's something the driver can't know.
Sharing interrupts is an integration property. The same IP core may share its
interrupt on one SoC, and not on another.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Why isn't IRQ shared for i2c-ocore

[Lee Jones]

On Fri, 08 May 2015, Geert Uytterhoeven wrote:

> On Thu, May 7, 2015 at 9:01 AM, Lee Jones <lee.jones@linaro.org> wrote:
> >> I have a follow up question regarding interrupt. I see many I2C bus drivers
> >> request interrupt with flag = 0. Why not using IRQF_SHARED?
> >
> > Probably because that particular IRQ is only used by the I2C
> > Controller.  I'm not exactly sure that you're getting at?  Why do you
> > think it should be shared?  You should only flag it as shared if it
> > is.
> 
> However, that's something the driver can't know.
> Sharing interrupts is an integration property. The same IP core may share its
> interrupt on one SoC, and not on another.

I guess that would depend on the IP.  If this is part of an MFD, you'd
know if you only hand a single interrupt line coming into the chip or
not.  If the IP can be moved around (copy & pasted) into different
chips, then yes, that might change.

How does one share an interrupt with other drivers if all them don't
know the IRQ is shared thought?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: Why isn't IRQ shared for i2c-ocore

[Geert Uytterhoeven]

On Fri, May 8, 2015 at 8:57 AM, Lee Jones <lee.jones@linaro.org> wrote:
> On Fri, 08 May 2015, Geert Uytterhoeven wrote:
>
>> On Thu, May 7, 2015 at 9:01 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> >> I have a follow up question regarding interrupt. I see many I2C bus drivers
>> >> request interrupt with flag = 0. Why not using IRQF_SHARED?
>> >
>> > Probably because that particular IRQ is only used by the I2C
>> > Controller.  I'm not exactly sure that you're getting at?  Why do you
>> > think it should be shared?  You should only flag it as shared if it
>> > is.
>>
>> However, that's something the driver can't know.
>> Sharing interrupts is an integration property. The same IP core may share its
>> interrupt on one SoC, and not on another.
>
> I guess that would depend on the IP.  If this is part of an MFD, you'd
> know if you only hand a single interrupt line coming into the chip or
> not.  If the IP can be moved around (copy & pasted) into different
> chips, then yes, that might change.
>
> How does one share an interrupt with other drivers if all them don't
> know the IRQ is shared thought?

All drivers sharing the same interrupt must pass IRQF_SHARED, cfr. the
checks in __setup_irq().

Traditionally, PC (ISA) drivers didn't share interrupts, as the ISA bus
prohibited interrupt sharing.
Amiga drivers did.

New drivers should support IRQ sharing.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Why isn't IRQ shared for i2c-ocore

[Lee Jones]

On Fri, 08 May 2015, Geert Uytterhoeven wrote:

> On Fri, May 8, 2015 at 8:57 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Fri, 08 May 2015, Geert Uytterhoeven wrote:
> >
> >> On Thu, May 7, 2015 at 9:01 AM, Lee Jones <lee.jones@linaro.org> wrote:
> >> >> I have a follow up question regarding interrupt. I see many I2C bus drivers
> >> >> request interrupt with flag = 0. Why not using IRQF_SHARED?
> >> >
> >> > Probably because that particular IRQ is only used by the I2C
> >> > Controller.  I'm not exactly sure that you're getting at?  Why do you
> >> > think it should be shared?  You should only flag it as shared if it
> >> > is.
> >>
> >> However, that's something the driver can't know.
> >> Sharing interrupts is an integration property. The same IP core may share its
> >> interrupt on one SoC, and not on another.
> >
> > I guess that would depend on the IP.  If this is part of an MFD, you'd
> > know if you only hand a single interrupt line coming into the chip or
> > not.  If the IP can be moved around (copy & pasted) into different
> > chips, then yes, that might change.
> >
> > How does one share an interrupt with other drivers if all them don't
> > know the IRQ is shared thought?
> 
> All drivers sharing the same interrupt must pass IRQF_SHARED, cfr. the
> checks in __setup_irq().
> 
> Traditionally, PC (ISA) drivers didn't share interrupts, as the ISA bus
> prohibited interrupt sharing.
> Amiga drivers did.
> 
> New drivers should support IRQ sharing.

Precisely, which is why I'm confused by:

  "However, that's something the driver can't know."

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: Why isn't IRQ shared for i2c-ocore

[Geert Uytterhoeven]

On Fri, May 8, 2015 at 9:27 AM, Lee Jones <lee.jones@linaro.org> wrote:
> On Fri, 08 May 2015, Geert Uytterhoeven wrote:
>> On Fri, May 8, 2015 at 8:57 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> > On Fri, 08 May 2015, Geert Uytterhoeven wrote:
>> >> On Thu, May 7, 2015 at 9:01 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> >> >> I have a follow up question regarding interrupt. I see many I2C bus drivers
>> >> >> request interrupt with flag = 0. Why not using IRQF_SHARED?
>> >> >
>> >> > Probably because that particular IRQ is only used by the I2C
>> >> > Controller.  I'm not exactly sure that you're getting at?  Why do you
>> >> > think it should be shared?  You should only flag it as shared if it
>> >> > is.
>> >>
>> >> However, that's something the driver can't know.
>> >> Sharing interrupts is an integration property. The same IP core may share its
>> >> interrupt on one SoC, and not on another.
>> >
>> > I guess that would depend on the IP.  If this is part of an MFD, you'd
>> > know if you only hand a single interrupt line coming into the chip or
>> > not.  If the IP can be moved around (copy & pasted) into different
>> > chips, then yes, that might change.
>> >
>> > How does one share an interrupt with other drivers if all them don't
>> > know the IRQ is shared thought?
>>
>> All drivers sharing the same interrupt must pass IRQF_SHARED, cfr. the
>> checks in __setup_irq().
>>
>> Traditionally, PC (ISA) drivers didn't share interrupts, as the ISA bus
>> prohibited interrupt sharing.
>> Amiga drivers did.
>>
>> New drivers should support IRQ sharing.
>
> Precisely, which is why I'm confused by:
>
>   "However, that's something the driver can't know."

So better safe than sorry: always use IRQF_SHARED, unless you have
a very good reason not to use it.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds