Hi Jagan, On 11 October 2014 12:06, Jagan Teki <jagannadh.t...@gmail.com> wrote: > Hi Simon, > > On 11 October 2014 23:13, Simon Glass <s...@chromium.org> wrote: >> Hi Jagan, >> >> On 11 October 2014 11:33, Jagan Teki <jagannadh.t...@gmail.com> wrote: >>> Hi Simon, >>> >>> On 11 October 2014 04:56, Simon Glass <s...@chromium.org> wrote: >>>> Hi Jagan, >>>> >>>> On 10 October 2014 12:37, Jagan Teki <jagannadh.t...@gmail.com> wrote: >>>>> Hi Simon, >>>>> >>>>> On 10 October 2014 23:35, Simon Glass <s...@chromium.org> wrote: >>>>>> Hi Jagan, >>>>>> >>>>>> On 10 October 2014 11:31, Jagan Teki <jagannadh.t...@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 <s...@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 <s...@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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
