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

[Simon Glass]

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

>
>>
>>>
>>> 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.

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.