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

[Simon Glass]

Hi Jagan,

On 11 October 2014 12:06, Jagan Teki <jagannadh.teki at gmail.com> wrote:
> 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.

OK.

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

Let's pick this up when we have a specific problem to resolve, likely
platform data.

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.