On 03/11/2013 07:11 PM, Jon Hunter wrote: > > On 03/11/2013 12:57 PM, Javier Martinez Canillas wrote: >> On 03/11/2013 06:13 PM, Jon Hunter wrote: >>> >> >> Hi Jon, thanks a lot for your feedback. >> >>> On 03/10/2013 12:18 PM, Javier Martinez Canillas wrote: >>>> Besides being used to interface with external memory devices, >>>> the General-Purpose Memory Controller can be used to connect >>>> Pseudo-SRAM devices such as ethernet controllers to OMAP2+ >>>> processors using the GPMC as a data bus. >>>> >>>> The actual mapping between the GPMC address space and OMAP2+ >>>> address space is made using the GPMC DT "ranges" property. >>>> But also a explicit call to gpmc_cs_request() is needed. >>> >>> One problem with gpmc_cs_request() is that it will map the chip-select >>> to any physical location in the 1GB address space for the gpmc >>> controller. So in other words, it will ignore the ranges property >>> altogether. If you look at my code for NOR, I have added a >>> gpmc_cs_remap() function to remap the cs to the location as specified by >>> the device-tree. >>> >> >> I see, thanks for pointing this out. >> >>> Ideally we should change gpmc_cs_request() so we can pass the desire >>> base address that we want to map the gpmc cs too. I had started out that >>> way but it made the code some what messy and so I opted to create a >>> gpmc_cs_remap() function instead. The goal will be to get rid of >>> gpmc_cs_remap() once DT migration is completed and we can change >>> gpmc_cs_request() to map the cs to a specific address (see my FIXME >>> comment). >>> >> >> By looking at gpmc_probe_onenand_child() and gpmc_probe_nand_child() I see that >> these functions just allocates platform data and call gpmc_onenand_init() and >> gpmc_nand_init() accordingly. So if I understood right these functions have the >> same issue and need to call gpmc_cs_remap() too in order to map to the location >> specified on the DT. > > Ideally they should but it is not critical. > > So today for NAND and ONENAND the ranges property is completely ignored > (I just came to realise this recently). However, this works because the > address mapped by gpmc_cs_request() is passed to the NAND/ONENAND > drivers via the platform data. However, NOR (and your ethernet patch) we > can't pass via platform data and therefore we must remap. > > This needs to be fixed, but it is not critical in terms that it won't > crash. However, I fear your ethernet patch could :-o > >>> Your code probably works today because the cs is setup by the bootloader >>> and so when you request the cs in the kernel the mapping is not changed >>> from the bootloader settings. However, if the mapping in DT (ranges >>> property) is different from that setup by the bootloader then the kernel >>> would probably crash because the kernel would not remap it as expected. >>> >>>> So, this patch allows an ethernet chip to be defined as an >>>> GPMC child node an its chip-select memory address be requested. >>>> >>>> Signed-off-by: Javier Martinez Canillas <javier.martinez@xxxxxxxxxxxxxxx> >>>> --- >>>> >>>> Jon, >>>> >>>> This patch assumes that we have solved somehow the issue that a >>>> call to request_irq() is needed before before using a GPIO as an >>>> IRQ and this is no longer the case when using from Device Trees. >>>> >>>> Anyway, this is independent as how we solve this, whether is >>>> using Jan's patch [1], adding a .request function pointer to >>>> irq_chip as suggested by Stephen [2], or any other approach. >>>> >>>> [1]: https://patchwork.kernel.org/patch/2009331/ >>>> [2]: http://www.mail-archive.com/linux-omap@xxxxxxxxxxxxxxx/msg85592.html >>>> >>>> arch/arm/mach-omap2/gpmc.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 files changed, 45 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c >>>> index 4fe9ee7..d1bf48b 100644 >>>> --- a/arch/arm/mach-omap2/gpmc.c >>>> +++ b/arch/arm/mach-omap2/gpmc.c >>>> @@ -29,6 +29,7 @@ >>>> #include <linux/of.h> >>>> #include <linux/of_mtd.h> >>>> #include <linux/of_device.h> >>>> +#include <linux/of_address.h> >>>> #include <linux/mtd/nand.h> >>>> >>>> #include <linux/platform_data/mtd-nand-omap2.h> >>>> @@ -1296,6 +1297,42 @@ static int gpmc_probe_onenand_child(struct platform_device *pdev, >>>> } >>>> #endif >>>> >>>> +static int gpmc_probe_ethernet_child(struct platform_device *pdev, >>>> + struct device_node *child) >>>> +{ >>>> + int ret, cs; >>>> + unsigned long base; >>>> + struct resource res; >>>> + struct platform_device *of_dev; >>>> + >>>> + if (of_property_read_u32(child, "reg", &cs) < 0) { >>>> + dev_err(&pdev->dev, "%s has no 'reg' property\n", >>>> + child->full_name); >>>> + return -ENODEV; >>>> + } >>>> + >>>> + if (of_address_to_resource(child, 0, &res)) { >>>> + dev_err(&pdev->dev, "%s has malformed 'reg' property\n", >>>> + child->full_name); >>>> + return -ENODEV; >>>> + } >>>> + >>>> + ret = gpmc_cs_request(cs, resource_size(&res), &base); >>>> + if (IS_ERR_VALUE(ret)) { >>>> + dev_err(&pdev->dev, "cannot request GPMC CS %d\n", cs); >>>> + return ret; >>>> + } >>>> + >>>> + of_dev = of_platform_device_create(child, NULL, &pdev->dev); >>>> + if (!of_dev) { >>>> + dev_err(&pdev->dev, "cannot create platform device for %s\n", >>>> + child->full_name); >>>> + return -ENODEV; >>>> + } >>>> + >>>> + return 0; >>>> +} >>> >>> So this function does not setup the cs at all and so that means you are >>> dependent on having the bootloader configure the cs. I would really like >>> to get away from that sort of dependency. In fact I am wondering if we >>> could make the gpmc_probe_nor() function a gpmc_probe_generic() that we >>> can use for both nor and ethernet as we are doing very similar things >>> (if we add the timings and gpmc settings to the ethernet binding). >>> >> >> Yes, I also thought about a gmpc_probe_generic() for all GMPC child nodes since >> the chip-select setup is the same for all of them. >> >> The problem I saw was that gpmc_probe_{onenand,nand}_child() were just wrappers >> around gpmc_{onenand,nand}_init() and since the init functions call >> gpmc_cs_request(), I couldn't have a generic probe that call gpmc_cs_request() >> for all childs. > > Yes I was thinking about leaving nand and onenand the way they were but > using probe_generic() for nor and ethernet. > >> But since we should probably have to change this to call gpmc_cs_remap() besides >> gpmc_cs_request(), I think is better to not use gpmc_{onenand,nand}_init() at >> all and make this somehow generic. > > Yes but may be we could do this longer term and just get ethernet > working for now. > >> Actually, since the mapping (and the IORESOURCE_MEM struct resource allocation) >> is made by the DT core using the "ranges" property. I wonder if we could add >> some callback function (e.g: .range_request() ) that can be set by memory >> controllers that want to take an action (such as calling gpmc_cs_request() and >> gpmc_cs_remap() ) once each element in the "ranges" vector is processed by the >> DT core. > > Possibly but again I think that should be look at longer term. I think > you are on the right path. Care to see if you can make gpmc_probe_nor > into gpmc_probe_generic and make this work for ethernet too? Leave nand > and onenand as-is for now. > Yes, you are right, nand and onenand can be fixed later. I'll do what you suggest later this week and post the patches. >>> Also I think we need to add some DT binding documentation for this as well. >>> >> >> +1 > > Thanks > Jon > Thanks a lot and best regards, Jaiver -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
