Date:	Sat, 15 Nov 2014 13:32:07 -0200
From:	DATACOM - Erico Nunes <erico.nunes@...acom.ind.br>
To:	Stanimir Varbanov <svarbanov@...sol.com>,
	Grant Likely <grant.likely@...aro.org>,
	Arnd Bergmann <arnd@...db.de>
CC:	robh+dt@...nel.org, devicetree@...r.kernel.org,
	sameo@...ux.intel.com, lee.jones@...aro.org,
	linux-kernel@...r.kernel.org
Subject: Re: Creating a new platform_bus inside a spi_driver



On 11/13/2014 06:52 AM, Stanimir Varbanov wrote:
> Hi Grant, Arnd and Erico
> 
> On 11/11/2014 01:07 PM, Grant Likely wrote:
>> On Fri, 07 Nov 2014 18:04:35 +0100
>> , Arnd Bergmann <arnd@...db.de>
>>  wrote:
>>> On Friday 07 November 2014 14:37:26 DATACOM - Érico Nunes wrote:
>>>> Hello Arnd and all,
>>>>
>>>> On 11/07/2014 08:04 AM, Arnd Bergmann wrote:
>>>>> On Thursday 06 November 2014 18:02:52 DATACOM - Érico Nunes wrote:
>>>>>> The idea is that "fpga-spi" is a spi_driver which instantiates all of the
>>>>>> "fpga-deviceN" as platform_devices, through the use of
>>>>>> of_platform_populate(dev->of_node, NULL, NULL, dev).
>>>>>>
>>>>>> The visible problem we're facing with this approach is that, as the internal
>>>>>> platform_devices have a "reg" property, of_platform_populate() eventually
>>>>>> triggers an address translation which is apparently trying to translate the
>>>>>> addresses of the internal platform_bus to addresses of the processor memory
>>>>>> map.
>>>>>> This translation is however not part of our intention, as we intend to have an
>>>>>> internal bus with its own memory map.
>>>>>> This fails when __of_translate_address() reaches the spi-master boundary
>>>>>> because (as it seems to make sense) it isn't possible to translate them past
>>>>>> that.
>>>>>> A KERN_ERR rated message like
>>>>>> "prom_parse: Bad cell count for /soc@...00000/spi@...0/fpga@1"
>>>>>> is thrown by __of_translate_address() and later it is not possible to obtain
>>>>>> the "reg" address with platform_get_resource().
>>>>>>
>>>>>> On this scenario, we have a few questions and, depending on the outcome of
>>>>>> these, possibly a patch.
>>>>>>
>>>>>> 1. Is it possible to have an internal platform_bus with a different memory map
>>>>>> as we intended? Or are platform_busses and platform_devices supposed to always
>>>>>> be mapped on the processor memory map?
>>>>> It's inconsistent. We have some code that assumes that platform devices
>>>>> are always memory mapped, and some other code that breaks this assumption.
>>>>
>>>> By this I take that the platform subsystem could be made generic so it can be
>>>> used in both ways (mapped to processor memory map or mapped to a private memory
>>>> map). There seems to be no strict requirement enforcing it to be processor
>>>> memory map.
>>>>
>>>> Is this correct?
>>>
>>> It could be, but I'm sure if that is a good idea or not. It might complicate
>>> things elsewhere, so it would at least need careful testing and consensus
>>> among a broader group of developers.
>>
>> I don't think it is a good idea. I would prefer to make the behaviour of
>> of_platform_populate() generic so it could work for multiple bus
>> types rather than reusing/abusing platform_device in this way.
>>
>> If the devices on the FPGA were memory mapped, it would be a different
>> situation, but being behind an SPI bus where the access to those devices
>> must always be mediated by the SPI controller, it would be better to
>> have a separate bus type and a separate pool of drivers for those
>> devices.
>>

Grant,
could you please elaborate a little on "making the behaviour of
of_platform_populate() generic so it could work for multiple bus types
without reusing/abusing platform_device"?

Initially we thought of having a separate bus type, but that felt such a
duplication of a platform_bus that we decided to try to reuse.

On the other thread line about the patch posted in my opening comment,
it was suggested that we could change __of_translate_address(), for
example to allow a translation end-point to be passed it. This seems
like a good idea to me, however with this comment here it is still not
clear to me whether this change would be likely accepted or not.

> 
> This is exactly the same problem that we have on Qualcomm SPMI PMIC mfd
> driver. We had a discussion at [1] where we tried to solve it by
> IORESOURCE_REG. Unfortunately there is no conclusion yet.
> 
> I'm glad to see that someone else have the same issue, maybe it is time
> to restart the discussion. The last proposal from Grant was to implement
> dev_get_localbus_address() API in drivers/base.

I took the time to read through your thread and indeed a lot of this
discussion is common. It is now clear that we should not be using
IORESOURCE_MEM as it was assumed in the original patch, which was one of
my original questions.
We are still not using the mfd framework because of the need to maintain
the table of "compatible" drivers in code, and this requires double
maintenance of code instead of adding a new device to the dts.
Nevertheless, I tested your "RFC: add function for localbus address"
patch, and your solution worked right away for my case too (without mfd)
by just changing my drivers to get IORESOURCE_REG instead.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists