[U-Boot] [PATCH v3 05/29] dm: spi: Add a uclass for SPI

[Jagan Teki]

Hi Simon,

On 11 October 2014 23:13, Simon Glass <sjg at chromium.org> wrote:
> Hi Jagan,
>
> On 11 October 2014 11:33, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>> Hi Simon,
>>
>> On 11 October 2014 04:56, Simon Glass <sjg at chromium.org> wrote:
>>> Hi Jagan,
>>>
>>> On 10 October 2014 12:37, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>>> Hi Simon,
>>>>
>>>> On 10 October 2014 23:35, Simon Glass <sjg at chromium.org> wrote:
>>>>> Hi Jagan,
>>>>>
>>>>> On 10 October 2014 11:31, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>>>>> Hi Simon,
>>>>>>
>>>>>> Can you please see my comments below, I'm relating more about cs and bus
>>>>>> valid scenario which is available on current drivers wrt dm-spi.
>>>>>>
>>>>>> On 30 September 2014 01:05, Simon Glass <sjg at chromium.org> wrote:
>>>>>>> Add a uclass which provides access to SPI buses and includes operations
>>>>>>> required by SPI.
>>>>>>>
>>>>>>> For a time driver model will need to co-exist with the legacy SPI interface
>>>>>>> so some parts of the header file are changed depending on which is in use.
>>>>>>> The exports are adjusted also since some functions are not available with
>>>>>>> driver model.
>>>>>>>
>>>>>>> Boards must define CONFIG_DM_SPI to use driver model for SPI.
>>>>>>>
>>>>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes in v3:
>>>>>>> - Add a cs_info() method to the driver model SPI API
>>>>>>
>>>>>> I think this gives the enough info regarding cs on particular controller what
>>>>>> about the bus usage - which is also specific to controller it self, right?
>>>>>>
>>>>>> Say - spi controller on a particular soc, have spi0, spi1 means two buses
>>>>>> and 4 cs lines, cs0-cs3. How is this works in that case?
>>>>>>
>>>>>> And also I saw tegra20_sflash.c validates only cs
>>>>>>
>>>>>> int tegra20_sflash_cs_info(struct udevice *bus, unsigned int cs,
>>>>>>                            struct spi_cs_info *info)
>>>>>>
>>>>>> What about bus number check this particular tegra spi controller is concern.
>>>>>
>>>>> The bus device is passed in as a pointer. The bus number gets looked
>>>>> up to find the device for that bus number. See spi_get_bus_and_cs()
>>>>> for that.
>>>>>
>>>>> This is good because drivers no longer need to validate the bus number.
>>>>
>>>> Agreed, but for controlling "sf probe bus:cs " options bus number
>>>> should get from
>>>> the respective driver.
>>>
>>> I wonder if the issue here is a difference of understanding about bus
>>> and chip select. In my mind, bus is just a device. We use the bus
>>> number to specify which device we mean, but it is just a a
>>> convenience. If there were some other way of finding the device we
>>> would use it (as we do with device tree, for example).
>>
>> Yes, I do believe that by mentioning bus spi0, spi1 on the devicetree
>> so the respective bus get's bind'ed till that fine. But as we use runtime
>> probe using "sf probe bus:cs" where I would mention
>> # sf probe 2:0
>> selecting spi2 which is of third bus on the same controller driver will that
>> spi_cs_is_valid() will show spi2 is invalid for this particular driver?
>
> Run-time binding of SPI buses is supported by driver model (just call
> device_bind()) but not by the sf probe command. We should know what
> SPI buses exist through device tree or platform data.
>
> So no, the that method is intended to validate a chip select, not a bus.
>
>>
>>>
>>> Similarly, chip select is just a device. We look at the bus device,
>>> find the appropriate chip select number, and end up with a child
>>> device which is referred to by that chip select.
>>
>> Just like validating supported cs, through cs_info I assume the same
>> works for bus as well as this is a pure driver specific theory.
>
> Not at present. In fact I feel that both bus and cs should be known in
> advance, but to keep compatibility (and since there is no platform
> data available for chip selects) I have added the auto-bind used by
> the 'sf probe' and 'sspi' commands.
>
>>
>>>
>>>>
>>>> I have a suggestion, like in spi_find_bus_and_cs() or spi_get_bus_and_cs()
>>>> we have a function calls for uclass_get_device_by_seq() and
>>>> spi_find_bus_and_cs()
>>>> along with that ops->cs_info(bus, cs, info) by adding bus number check
>>>> for the driver.
>>>>
>>>> So-that there is no requirement to call spi_cs_is_valid from any other location
>>>> as bus and cs validates through commands itself.
>>>
>>> At present spi_cs_info() calls spi_find_chip_select(). I think I see
>>> what you are getting at - you don't want a device to be created unless
>>> cs_info() allows it, right? I will take a look.
>>
>> Yes ie my point, along with that instead of a separate caller for spi_cs_info()
>> why can't we call in the probe time, either from command or at the time of
>> bus_cs binding.
>>
>> Once the bus_cs binding done, and validate the mentioned bus and cs from
>> command or devicetree and return.
>>
>> Does this make sense, please comment?
>
> I'm starting to feel that (beyond the idea of using cs_info() more)
> what you are referring to here is best handled in follow-up discussion
> and patches once this initial support is merged. There seem to be
> several questions that are hard to answer until we actually come to
> implementation:
>
> - qspi (could be implemented in sandbox, I don't have a qspi board at present)
> - chip selects via GPIO (I am planning to look at this before long)
> - platform data for chip selects (likely will be used for at91 or
> something like that

OK then - will start moving with this initial stuff update accordingly
in follow-up
changes/discussions.

>
>>
>>>
>>>>
>>>> BTW: in tegra20_sflash.c, how does tegra20_sflash_cs_info() called?
>>>
>>> Only if someone calls spi_cs_info(). I don't think there are any
>>> callers at present. With tegra using device tree I'm not sure it would
>>> have any purpose - the chip selects and buses are all known.
>>
>> Yes devicetree will help to take but validating should be require as driver
>> only knows how many bus's with respective supported chip-selects based
>> on this command user may verify the same. what do think?
>
> I'm not sure what you are saying here. Can you please restate this?
>
> The device tree should be able to completely specify the bus and chip
> selects. I can't see a reason not to do this.

I just stated, cs and bus numbers we can get it from devicetree but
for validating
how many number of bus's and cs's for a specific controller we can get through
driver specific call through cs_info().[of course bus is not
validating as of now]

>
> Regards,
> Simon
>
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>> - Add a uclass for a generic SPI device (for use with the 'sspi' command)
>>>>>>> - Add missing comments to spi.h
>>>>>>> - Correct typo where 'slave' should say 'bus'
>>>>>>> - Fix two comment typos
>>>>>>> - Put the cs member back into spi_slave
>>>>>>> - Use an explicit chip select value instead of reusing device sequence number
>>>>>>>
>>>>>>> Changes in v2:
>>>>>>> - Add missing comments for struct spi_slave
>>>>>>> - Fix code nits from Daniel Schwierzeck
>>>>>>> - Use 'bus' instead of 'dev' to make the API clearer
>>>>>>>
>>>>>>>  common/exports.c         |   4 +-
>>>>>>>  drivers/spi/Makefile     |   4 +
>>>>>>>  drivers/spi/spi-uclass.c | 390 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>  include/dm/uclass-id.h   |   2 +
>>>>>>>  include/spi.h            | 254 +++++++++++++++++++++++++++++-
>>>>>>>  5 files changed, 650 insertions(+), 4 deletions(-)
>>>>>>>  create mode 100644 drivers/spi/spi-uclass.c
>>>>>>>
>>>>>
>>>>>
>>>>>>> +int spi_cs_is_valid(unsigned int busnum, unsigned int cs)
>>>>>>
>>>>>> Who is the caller for this?
>>>>>
>>>>> This can be called by boards, or anywhere really. It replaces the
>>>>> function with the same purpose in the old SPI framework.
>>>>
>>>> Please see above.
>>>>
>>> OK
>>

thanks!
-- 
Jagan.