| Message ID | 1396523431-14519-2-git-send-email-harinik@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 03, 2014 at 04:40:31PM +0530, Harini Katakam wrote: > +Optional properties: > +- num-cs : Number of chip selects used. How does this translate to the hardware? > + num-cs = /bits/ 16 <4>; What's going on with the /bits/ - is this something that's required for the property?
Hi Mark, On Fri, Apr 4, 2014 at 3:04 AM, Mark Brown <broonie@kernel.org> wrote: > On Thu, Apr 03, 2014 at 04:40:31PM +0530, Harini Katakam wrote: > >> +Optional properties: >> +- num-cs : Number of chip selects used. > > How does this translate to the hardware? This IP can drive 4 slaves. The CS line to be driven is selected in spi device structure and that is driven by the software. > >> + num-cs = /bits/ 16 <4>; > > What's going on with the /bits/ - is this something that's required for > the property? The master->num-chipselect property is 16 bit but writing <4> here directly leads to 0 being read in of_property_read (because it's big endian). Instead using of property read u32 and then copying, we decided to do this. This was discussed on v2 between Michal and Rob: >>>> + num-chip-select = /bits/ 16 <4>; >> >> I was expecting you will comment this a little bit. :-) >> Because all just reading this num-cs as 32bit and then >> assigning this value to master->num_chipselect which is 16bit. > > Well, everyone else has that problem then. Obviously it takes a bit > more care than just reading into a u32, but that is a kernel problem > and not a problem of the binding. They are not reading it directly with read_u32 but they are using intermediate u32 value which is assigned to u16 which is fine. This pattern is in most drivers(maybe all). The point is if binding should or can't simplify driver code. And from your reaction above I expect that it is up to driver owner and binding doc how you want to do it. Regards, Harini -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Harini, On Fri, Apr 4, 2014 at 5:00 AM, Harini Katakam <harinikatakamlinux@gmail.com> wrote: >>> + num-cs = /bits/ 16 <4>; >> >> What's going on with the /bits/ - is this something that's required for >> the property? > > The master->num-chipselect property is 16 bit but writing <4> here directly > leads to 0 being read in of_property_read (because it's big endian). > Instead using of property read u32 and then copying, we decided to do this. > This was discussed on v2 between Michal and Rob: >>>>> + num-chip-select = /bits/ 16 <4>; >>> >>> I was expecting you will comment this a little bit. :-) >>> Because all just reading this num-cs as 32bit and then >>> assigning this value to master->num_chipselect which is 16bit. >> >> Well, everyone else has that problem then. Obviously it takes a bit >> more care than just reading into a u32, but that is a kernel problem >> and not a problem of the binding. > They are not reading it directly with read_u32 but they are using > intermediate u32 value which is assigned to u16 which is fine. > This pattern is in most drivers(maybe all). > The point is if binding should or can't simplify driver code. > And from your reaction above I expect that it is up to driver > owner and binding doc how you want to do it. IMHO this "/bits/ 16" doesn't simplify the binding. As "num-cs" is a generic spi subsystem binding, it should not be restricted to 16 bits for the sake of a driver. As your hardware can drive 4 chip selects, you could represent it in 3 bits (don't!). Simple integers are 32 bit in DT, so use a temporary. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 04, 2014 at 08:30:38AM +0530, Harini Katakam wrote: > On Fri, Apr 4, 2014 at 3:04 AM, Mark Brown <broonie@kernel.org> wrote: > > On Thu, Apr 03, 2014 at 04:40:31PM +0530, Harini Katakam wrote: > >> +Optional properties: > >> +- num-cs : Number of chip selects used. > > How does this translate to the hardware? > This IP can drive 4 slaves. > The CS line to be driven is selected in spi device structure and > that is driven by the software. So why specify this in the binding at all? If the device always has the same number of chip selects it's redundant. > >> + num-cs = /bits/ 16 <4>; > > What's going on with the /bits/ - is this something that's required for > > the property? > The master->num-chipselect property is 16 bit but writing <4> here directly > leads to 0 being read in of_property_read (because it's big endian). > Instead using of property read u32 and then copying, we decided to do this. > This was discussed on v2 between Michal and Rob: No, don't do that. You're contorting the binding to serve Linux's implementation needs and you've not documented this requirement either. If there *was* a need to have the number be 16 bits you'd need to document that reqirement in the binding, though like I say I don't think the number needs to be there at all.
On 04/04/2014 10:09 AM, Geert Uytterhoeven wrote: > Hi Harini, > > On Fri, Apr 4, 2014 at 5:00 AM, Harini Katakam > <harinikatakamlinux@gmail.com> wrote: >>>> + num-cs = /bits/ 16 <4>; >>> >>> What's going on with the /bits/ - is this something that's required for >>> the property? >> >> The master->num-chipselect property is 16 bit but writing <4> here directly >> leads to 0 being read in of_property_read (because it's big endian). >> Instead using of property read u32 and then copying, we decided to do this. >> This was discussed on v2 between Michal and Rob: >>>>>> + num-chip-select = /bits/ 16 <4>; >>>> >>>> I was expecting you will comment this a little bit. :-) >>>> Because all just reading this num-cs as 32bit and then >>>> assigning this value to master->num_chipselect which is 16bit. >>> >>> Well, everyone else has that problem then. Obviously it takes a bit >>> more care than just reading into a u32, but that is a kernel problem >>> and not a problem of the binding. >> They are not reading it directly with read_u32 but they are using >> intermediate u32 value which is assigned to u16 which is fine. >> This pattern is in most drivers(maybe all). >> The point is if binding should or can't simplify driver code. >> And from your reaction above I expect that it is up to driver >> owner and binding doc how you want to do it. > > IMHO this "/bits/ 16" doesn't simplify the binding. > > As "num-cs" is a generic spi subsystem binding, it should not be > restricted to 16 bits for the sake of a driver. As your hardware can drive 4 > chip selects, you could represent it in 3 bits (don't!). > > Simple integers are 32 bit in DT, so use a temporary. No problem to keep it there to 32bit range. I really appreciate that discussion we have about it. Just a note: If "num-cs" is the part of generic spi subsystem binding maybe it should be moved directly to spi core as is done for example for timeout-sec in watchdog. Also this should be listed in any Documentation/devicetree/bindings/spi/spi.txt file that this is generic spi binding. Thanks, Michal
Hi Mark, On Fri, Apr 4, 2014 at 3:39 PM, Mark Brown <broonie@kernel.org> wrote: > On Fri, Apr 04, 2014 at 08:30:38AM +0530, Harini Katakam wrote: >> On Fri, Apr 4, 2014 at 3:04 AM, Mark Brown <broonie@kernel.org> wrote: >> > On Thu, Apr 03, 2014 at 04:40:31PM +0530, Harini Katakam wrote: > >> >> +Optional properties: >> >> +- num-cs : Number of chip selects used. > >> > How does this translate to the hardware? > >> This IP can drive 4 slaves. >> The CS line to be driven is selected in spi device structure and >> that is driven by the software. > > So why specify this in the binding at all? If the device always has the > same number of chip selects it's redundant. I'm sorry, I should have explained that better. The IP can support upto 4 chip selects. The num-cs value here is the number of chip selects actually used on the board. > >> >> + num-cs = /bits/ 16 <4>; > >> > What's going on with the /bits/ - is this something that's required for >> > the property? > >> The master->num-chipselect property is 16 bit but writing <4> here directly >> leads to 0 being read in of_property_read (because it's big endian). >> Instead using of property read u32 and then copying, we decided to do this. >> This was discussed on v2 between Michal and Rob: > > No, don't do that. You're contorting the binding to serve Linux's > implementation needs and you've not documented this requirement either. > If there *was* a need to have the number be 16 bits you'd need to > document that reqirement in the binding, though like I say I don't think > the number needs to be there at all. OK. I'll remove the /bits/ 16 and handle it in the driver. Regards, Harini -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 4, 2014 at 10:14 PM, Harini Katakam <harinikatakamlinux@gmail.com> wrote: > Hi Mark, > > On Fri, Apr 4, 2014 at 3:39 PM, Mark Brown <broonie@kernel.org> wrote: >> On Fri, Apr 04, 2014 at 08:30:38AM +0530, Harini Katakam wrote: >>> On Fri, Apr 4, 2014 at 3:04 AM, Mark Brown <broonie@kernel.org> wrote: >>> > On Thu, Apr 03, 2014 at 04:40:31PM +0530, Harini Katakam wrote: >> >>> >> +Optional properties: >>> >> +- num-cs : Number of chip selects used. >> >>> > How does this translate to the hardware? >> >>> This IP can drive 4 slaves. >>> The CS line to be driven is selected in spi device structure and >>> that is driven by the software. >> >> So why specify this in the binding at all? If the device always has the >> same number of chip selects it's redundant. > > I'm sorry, I should have explained that better. > The IP can support upto 4 chip selects. > The num-cs value here is the number of chip selects actually used on the board. > Just to clarify - do you mean SoC or board level? Regards, Peter >> >>> >> + num-cs = /bits/ 16 <4>; >> >>> > What's going on with the /bits/ - is this something that's required for >>> > the property? >> >>> The master->num-chipselect property is 16 bit but writing <4> here directly >>> leads to 0 being read in of_property_read (because it's big endian). >>> Instead using of property read u32 and then copying, we decided to do this. >>> This was discussed on v2 between Michal and Rob: >> >> No, don't do that. You're contorting the binding to serve Linux's >> implementation needs and you've not documented this requirement either. >> If there *was* a need to have the number be 16 bits you'd need to >> document that reqirement in the binding, though like I say I don't think >> the number needs to be there at all. > > OK. I'll remove the /bits/ 16 and handle it in the driver. > > Regards, > Harini > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Fri, Apr 4, 2014 at 5:54 PM, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Fri, Apr 4, 2014 at 10:14 PM, Harini Katakam > <harinikatakamlinux@gmail.com> wrote: >> Hi Mark, >> >> On Fri, Apr 4, 2014 at 3:39 PM, Mark Brown <broonie@kernel.org> wrote: >>> On Fri, Apr 04, 2014 at 08:30:38AM +0530, Harini Katakam wrote: >>>> On Fri, Apr 4, 2014 at 3:04 AM, Mark Brown <broonie@kernel.org> wrote: >>>> > On Thu, Apr 03, 2014 at 04:40:31PM +0530, Harini Katakam wrote: >>> >>>> >> +Optional properties: >>>> >> +- num-cs : Number of chip selects used. >>> >>>> > How does this translate to the hardware? >>> >>>> This IP can drive 4 slaves. >>>> The CS line to be driven is selected in spi device structure and >>>> that is driven by the software. >>> >>> So why specify this in the binding at all? If the device always has the >>> same number of chip selects it's redundant. >> >> I'm sorry, I should have explained that better. >> The IP can support upto 4 chip selects. >> The num-cs value here is the number of chip selects actually used on the board. >> > > Just to clarify - do you mean SoC or board level? > I mean board level. One more reason it will be useful to keep this property is because there is support for adding a decoder where extended slaves can be selected through the IP's control register. (This is not currently implemented in the driver but will be in the future.) Regards, Harini -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 04, 2014 at 05:44:23PM +0530, Harini Katakam wrote: > On Fri, Apr 4, 2014 at 3:39 PM, Mark Brown <broonie@kernel.org> wrote: > > On Fri, Apr 04, 2014 at 08:30:38AM +0530, Harini Katakam wrote: > >> This IP can drive 4 slaves. > >> The CS line to be driven is selected in spi device structure and > >> that is driven by the software. > > So why specify this in the binding at all? If the device always has the > > same number of chip selects it's redundant. > I'm sorry, I should have explained that better. > The IP can support upto 4 chip selects. > The num-cs value here is the number of chip selects actually used on the board. Why does that need to be configured? Surely the presence of slaves is enough information.
Hi Mark, On Fri, Apr 4, 2014 at 6:16 PM, Mark Brown <broonie@kernel.org> wrote: > On Fri, Apr 04, 2014 at 05:44:23PM +0530, Harini Katakam wrote: >> On Fri, Apr 4, 2014 at 3:39 PM, Mark Brown <broonie@kernel.org> wrote: >> > On Fri, Apr 04, 2014 at 08:30:38AM +0530, Harini Katakam wrote: > >> >> This IP can drive 4 slaves. >> >> The CS line to be driven is selected in spi device structure and >> >> that is driven by the software. > >> > So why specify this in the binding at all? If the device always has the >> > same number of chip selects it's redundant. > >> I'm sorry, I should have explained that better. >> The IP can support upto 4 chip selects. >> The num-cs value here is the number of chip selects actually used on the board. > > Why does that need to be configured? Surely the presence of slaves is > enough information. OK. I understand. Can you comment on the case where a decoder is used? There is support for adding a decoder where extended slaves can be selected through the IP's control register. (This is not currently implemented in the driver but will be in the future.) How should the driver know whether it is 4 or 16 select lines for example? This has to be written to master->num_chipselect. Regards, Harini -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Fri, Apr 4, 2014 at 7:38 PM, Harini Katakam <harinikatakamlinux@gmail.com> wrote: > Hi Mark, > > On Fri, Apr 4, 2014 at 6:16 PM, Mark Brown <broonie@kernel.org> wrote: >> On Fri, Apr 04, 2014 at 05:44:23PM +0530, Harini Katakam wrote: >>> On Fri, Apr 4, 2014 at 3:39 PM, Mark Brown <broonie@kernel.org> wrote: >>> > On Fri, Apr 04, 2014 at 08:30:38AM +0530, Harini Katakam wrote: >> >>> >> This IP can drive 4 slaves. >>> >> The CS line to be driven is selected in spi device structure and >>> >> that is driven by the software. >> >>> > So why specify this in the binding at all? If the device always has the >>> > same number of chip selects it's redundant. >> >>> I'm sorry, I should have explained that better. >>> The IP can support upto 4 chip selects. >>> The num-cs value here is the number of chip selects actually used on the board. >> >> Why does that need to be configured? Surely the presence of slaves is >> enough information. > > OK. I understand. > Can you comment on the case where a decoder is used? > > There is support for adding a decoder where extended slaves can be selected > through the IP's control register. > (This is not currently implemented in the driver but will be in the future.) > > How should the driver know whether it is 4 or 16 select lines for example? > This has to be written to master->num_chipselect. > > Regards, > Harini Just adding to my above comment. Alternately, I could not use the "num-cs" property and add another optional property to be used only when required. The driver sets a default value already. Regards, Harini -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 04, 2014 at 07:38:14PM +0530, Harini Katakam wrote: > OK. I understand. > Can you comment on the case where a decoder is used? > There is support for adding a decoder where extended slaves can be selected > through the IP's control register. > (This is not currently implemented in the driver but will be in the future.) > How should the driver know whether it is 4 or 16 select lines for example? > This has to be written to master->num_chipselect. That's the sort of case where it's useful yes - depending on how the implementation is done it may be sensible to just have a property specifying if a decoder is there (if it just changes the encoding in the register for example).
Hi Mark, On Fri, Apr 4, 2014 at 8:12 PM, Mark Brown <broonie@kernel.org> wrote: > On Fri, Apr 04, 2014 at 07:38:14PM +0530, Harini Katakam wrote: > >> OK. I understand. >> Can you comment on the case where a decoder is used? > >> There is support for adding a decoder where extended slaves can be selected >> through the IP's control register. >> (This is not currently implemented in the driver but will be in the future.) > >> How should the driver know whether it is 4 or 16 select lines for example? >> This has to be written to master->num_chipselect. > > That's the sort of case where it's useful yes - depending on how the > implementation is done it may be sensible to just have a property > specifying if a decoder is there (if it just changes the encoding in the > register for example). OK, great. That approach will work. Regards, Harini -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Apr 5, 2014 at 12:30 AM, Harini Katakam <harinikatakamlinux@gmail.com> wrote: > Hi, > > On Fri, Apr 4, 2014 at 7:38 PM, Harini Katakam > <harinikatakamlinux@gmail.com> wrote: >> Hi Mark, >> >> On Fri, Apr 4, 2014 at 6:16 PM, Mark Brown <broonie@kernel.org> wrote: >>> On Fri, Apr 04, 2014 at 05:44:23PM +0530, Harini Katakam wrote: >>>> On Fri, Apr 4, 2014 at 3:39 PM, Mark Brown <broonie@kernel.org> wrote: >>>> > On Fri, Apr 04, 2014 at 08:30:38AM +0530, Harini Katakam wrote: >>> >>>> >> This IP can drive 4 slaves. >>>> >> The CS line to be driven is selected in spi device structure and >>>> >> that is driven by the software. >>> >>>> > So why specify this in the binding at all? If the device always has the >>>> > same number of chip selects it's redundant. >>> >>>> I'm sorry, I should have explained that better. >>>> The IP can support upto 4 chip selects. >>>> The num-cs value here is the number of chip selects actually used on the board. Shouldnt it be a property of the controller not the board? How for example do we differentiate between different implementations of cadence SPI controller that only supports up to two CS lines? My thinking is that this property should always be present and = 4 in the Zynq case as the controller always has 4 physical CS lines coming out (before any decoders, pin muxes or slaves etc.). >>> >>> Why does that need to be configured? Surely the presence of slaves is >>> enough information. And presence of slaves / board info is inferable from subnodes. >> >> OK. I understand. >> Can you comment on the case where a decoder is used? >> >> There is support for adding a decoder where extended slaves can be selected >> through the IP's control register. >> (This is not currently implemented in the driver but will be in the future.) >> I think thats seperate information. is-decoded-cs property of something. >> How should the driver know whether it is 4 or 16 select lines for example? >> This has to be written to master->num_chipselect. >> If you only have one property (== 4) how do you tell the difference between 4 un-decoded CS lines vs 2 decoded CS lines? >> Regards, >> Harini > > Just adding to my above comment. > Alternately, I could not use the "num-cs" property and add another > optional property to be used only when required. I think num-cs has validity as the number of CS lines implemented in the controller. For any given SoC, it's constant but could differ between SoCs. Regards, Peter > The driver sets a default value already. > > Regards, > Harini > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Peter, On Sat, Apr 5, 2014 at 4:44 AM, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Sat, Apr 5, 2014 at 12:30 AM, Harini Katakam > <harinikatakamlinux@gmail.com> wrote: >> Hi, >> >> On Fri, Apr 4, 2014 at 7:38 PM, Harini Katakam >> <harinikatakamlinux@gmail.com> wrote: >>> Hi Mark, >>> >>> On Fri, Apr 4, 2014 at 6:16 PM, Mark Brown <broonie@kernel.org> wrote: >>>> On Fri, Apr 04, 2014 at 05:44:23PM +0530, Harini Katakam wrote: >>>>> On Fri, Apr 4, 2014 at 3:39 PM, Mark Brown <broonie@kernel.org> wrote: >>>>> > On Fri, Apr 04, 2014 at 08:30:38AM +0530, Harini Katakam wrote: >>>> >>>>> >> This IP can drive 4 slaves. >>>>> >> The sCS line to be driven is selected in spi device structure and >>>>> >> that is driven by the software. >>>> >>>>> > So why specify this in the binding at all? If the device always has the >>>>> > same number of chip selects it's redundant. >>>> >>>>> I'm sorry, I should have explained that better. >>>>> The IP can support upto 4 chip selects. >>>>> The num-cs value here is the number of chip selects actually used on the board. > > Shouldnt it be a property of the controller not the board? How for > example do we differentiate between different implementations of > cadence SPI controller that only supports up to two CS lines? My > thinking is that this property should always be present and = 4 in the > Zynq case as the controller always has 4 physical CS lines coming out > (before any decoders, pin muxes or slaves etc.). > >>>> >>>> Why does that need to be configured? Surely the presence of slaves is >>>> enough information. > > And presence of slaves / board info is inferable from subnodes. > >>> >>> OK. I understand. >>> Can you comment on the case where a decoder is used? >>> >>> There is support for adding a decoder where extended slaves can be selected >>> through the IP's control register. >>> (This is not currently implemented in the driver but will be in the future.) >>> > > I think thats seperate information. is-decoded-cs property of something. > >>> How should the driver know whether it is 4 or 16 select lines for example? >>> This has to be written to master->num_chipselect. >>> > > If you only have one property (== 4) how do you tell the difference > between 4 un-decoded CS lines vs 2 decoded CS lines? > If an SoC implements 2 CS and sets "decoder" property, then we'd configure the register with "select decode" bit and write 0b0011 to the slave select field to select 4th slave. If decoder property is not set, then we'd write 0b1000 to select 4th slave. But yes, if the SoC only implements 2 CS, doesn't use decoder but if there is an erroneous input to set_cs for 4th slave, it would be a problem. > > I think num-cs has validity as the number of CS lines implemented in > the controller. For any given SoC, it's constant but could differ > between SoCs. > I dint consider that this property will be useful to SoC's implementing <4 CS; master->num_chipselect (when set to this property's value) will be used to check error conditions. Regards, Harini -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Apr 5, 2014 at 4:05 PM, Harini Katakam <harinikatakamlinux@gmail.com> wrote: > Hi Peter, > > On Sat, Apr 5, 2014 at 4:44 AM, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Sat, Apr 5, 2014 at 12:30 AM, Harini Katakam >> <harinikatakamlinux@gmail.com> wrote: >>> Hi, >>> >>> On Fri, Apr 4, 2014 at 7:38 PM, Harini Katakam >>> <harinikatakamlinux@gmail.com> wrote: >>>> Hi Mark, >>>> >>>> On Fri, Apr 4, 2014 at 6:16 PM, Mark Brown <broonie@kernel.org> wrote: >>>>> On Fri, Apr 04, 2014 at 05:44:23PM +0530, Harini Katakam wrote: >>>>>> On Fri, Apr 4, 2014 at 3:39 PM, Mark Brown <broonie@kernel.org> wrote: >>>>>> > On Fri, Apr 04, 2014 at 08:30:38AM +0530, Harini Katakam wrote: >>>>> >>>>>> >> This IP can drive 4 slaves. >>>>>> >> The sCS line to be driven is selected in spi device structure and >>>>>> >> that is driven by the software. >>>>> >>>>>> > So why specify this in the binding at all? If the device always has the >>>>>> > same number of chip selects it's redundant. >>>>> >>>>>> I'm sorry, I should have explained that better. >>>>>> The IP can support upto 4 chip selects. >>>>>> The num-cs value here is the number of chip selects actually used on the board. >> >> Shouldnt it be a property of the controller not the board? How for >> example do we differentiate between different implementations of >> cadence SPI controller that only supports up to two CS lines? My >> thinking is that this property should always be present and = 4 in the >> Zynq case as the controller always has 4 physical CS lines coming out >> (before any decoders, pin muxes or slaves etc.). >> >>>>> >>>>> Why does that need to be configured? Surely the presence of slaves is >>>>> enough information. >> >> And presence of slaves / board info is inferable from subnodes. >> >>>> >>>> OK. I understand. >>>> Can you comment on the case where a decoder is used? >>>> >>>> There is support for adding a decoder where extended slaves can be selected >>>> through the IP's control register. >>>> (This is not currently implemented in the driver but will be in the future.) >>>> >> >> I think thats seperate information. is-decoded-cs property of something. >> >>>> How should the driver know whether it is 4 or 16 select lines for example? >>>> This has to be written to master->num_chipselect. >>>> >> >> If you only have one property (== 4) how do you tell the difference >> between 4 un-decoded CS lines vs 2 decoded CS lines? >> > > If an SoC implements 2 CS and sets "decoder" property, > then we'd configure the register with "select decode" bit and write > 0b0011 to the slave select field to select 4th slave. > > If decoder property is not set, then we'd write 0b1000 to select 4th slave. > What are your proposed dt bindings for these differing options - what is num-cs in each case? > But yes, if the SoC only implements 2 CS, doesn't use decoder but > if there is an erroneous input to set_cs for 4th slave, it would be a problem. > Sure, that's an error condition though that should be caught by SPI because master->num_chipselect would only be 2 right? Regards, Peter >> >> I think num-cs has validity as the number of CS lines implemented in >> the controller. For any given SoC, it's constant but could differ >> between SoCs. >> > > I dint consider that this property will be useful to SoC's implementing <4 CS; > master->num_chipselect (when set to this property's value) > will be used to check error conditions. > > Regards, > Harini -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Peter, On Sun, Apr 6, 2014 at 5:13 AM, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Sat, Apr 5, 2014 at 4:05 PM, Harini Katakam > <harinikatakamlinux@gmail.com> wrote: >> Hi Peter, >> >> On Sat, Apr 5, 2014 at 4:44 AM, Peter Crosthwaite >> <peter.crosthwaite@xilinx.com> wrote: >>> On Sat, Apr 5, 2014 at 12:30 AM, Harini Katakam >>> <harinikatakamlinux@gmail.com> wrote: >>>> Hi, >>>> >>>> On Fri, Apr 4, 2014 at 7:38 PM, Harini Katakam >>>> <harinikatakamlinux@gmail.com> wrote: >>>>> Hi Mark, >>>>> >>>>> On Fri, Apr 4, 2014 at 6:16 PM, Mark Brown <broonie@kernel.org> wrote: >>>>>> On Fri, Apr 04, 2014 at 05:44:23PM +0530, Harini Katakam wrote: >>>>>>> On Fri, Apr 4, 2014 at 3:39 PM, Mark Brown <broonie@kernel.org> wrote: >>>>>>> > On Fri, Apr 04, 2014 at 08:30:38AM +0530, Harini Katakam wrote: >>>>>> >>>>>>> >> This IP can drive 4 slaves. >>>>>>> >> The sCS line to be driven is selected in spi device structure and >>>>>>> >> that is driven by the software. >>>>>> >>>>>>> > So why specify this in the binding at all? If the device always has the >>>>>>> > same number of chip selects it's redundant. >>>>>> >>>>>>> I'm sorry, I should have explained that better. >>>>>>> The IP can support upto 4 chip selects. >>>>>>> The num-cs value here is the number of chip selects actually used on the board. >>> >>> Shouldnt it be a property of the controller not the board? How for >>> example do we differentiate between different implementations of >>> cadence SPI controller that only supports up to two CS lines? My >>> thinking is that this property should always be present and = 4 in the >>> Zynq case as the controller always has 4 physical CS lines coming out >>> (before any decoders, pin muxes or slaves etc.). >>> >>>>>> >>>>>> Why does that need to be configured? Surely the presence of slaves is >>>>>> enough information. >>> >>> And presence of slaves / board info is inferable from subnodes. >>> >>>>> >>>>> OK. I understand. >>>>> Can you comment on the case where a decoder is used? >>>>> >>>>> There is support for adding a decoder where extended slaves can be selected >>>>> through the IP's control register. >>>>> (This is not currently implemented in the driver but will be in the future.) >>>>> >>> >>> I think thats seperate information. is-decoded-cs property of something. >>> >>>>> How should the driver know whether it is 4 or 16 select lines for example? >>>>> This has to be written to master->num_chipselect. >>>>> >>> >>> If you only have one property (== 4) how do you tell the difference >>> between 4 un-decoded CS lines vs 2 decoded CS lines? >>> >> >> If an SoC implements 2 CS and sets "decoder" property, >> then we'd configure the register with "select decode" bit and write >> 0b0011 to the slave select field to select 4th slave. >> >> If decoder property is not set, then we'd write 0b1000 to select 4th slave. >> > > What are your proposed dt bindings for these differing options - what > is num-cs in each case? I think it will be better to have num-cs equal to the max slaves allowed and the property to indicate if decoder is present helps driver configure the register accordingly. (master->num_chipselect is written with "num-cs" value) For ex., 1. 4 slaves without decoder num-cs = <4> is-decoded-cs = <false> 2. 4 slaves driven through 2 to 4 decoder: num-cs = <4> is-decoded-cs = <true> The other option is have num-cs reflect number of CS before decoder but the above option makes more sense as num-cs directly reflects the max valid slaves. > >> But yes, if the SoC only implements 2 CS, doesn't use decoder but >> if there is an erroneous input to set_cs for 4th slave, it would be a problem. >> > > Sure, that's an error condition though that should be caught by SPI > because master->num_chipselect would only be 2 right? > Yes. Regards, Harini > Regards, > Peter > >>> >>> I think num-cs has validity as the number of CS lines implemented in >>> the controller. For any given SoC, it's constant but could differ >>> between SoCs. >>> >> >> I dint consider that this property will be useful to SoC's implementing <4 CS; >> master->num_chipselect (when set to this property's value) >> will be used to check error conditions. >> >> Regards, >> Harini -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/spi/spi-cadence.txt b/Documentation/devicetree/bindings/spi/spi-cadence.txt new file mode 100644 index 0000000..ccb7599 --- /dev/null +++ b/Documentation/devicetree/bindings/spi/spi-cadence.txt @@ -0,0 +1,27 @@ +Cadence SPI controller Device Tree Bindings +------------------------------------------- + +Required properties: +- compatible : Should be "cdns,spi-r1p6" or "xlnx,zynq-spi-r1p6". +- reg : Physical base address and size of SPI registers map. +- interrupts : Property with a value describing the interrupt + number. +- interrupt-parent : Must be core interrupt controller +- clock-names : List of input clock names - "ref_clk", "pclk" + (See clock bindings for details). +- clocks : Clock phandles (see clock bindings for details). + +Optional properties: +- num-cs : Number of chip selects used. + +Example: + + spi@e0007000 { + compatible = "xlnx,zynq-spi-r1p6"; + clock-names = "ref_clk", "pclk"; + clocks = <&clkc 26>, <&clkc 35>; + interrupt-parent = <&intc>; + interrupts = <0 49 4>; + num-cs = /bits/ 16 <4>; + reg = <0xe0007000 0x1000>; + } ;
Add spi-cadence bindings documentation. Signed-off-by: Harini Katakam <harinik@xilinx.com> --- v2 changes: - Separate patch for bindings. - Add xilinx compatible string; Make compatible string first in the node. - Use property name num-cs. Make this property optional. --- .../devicetree/bindings/spi/spi-cadence.txt | 27 ++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 Documentation/devicetree/bindings/spi/spi-cadence.txt