Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[dkota]

On 2018-08-17 20:29, Mark Brown wrote:

On Fri, Aug 17, 2018 at 04:06:06PM +0530, dk...@codeaurora.org wrote:


Could you please clarify on below query.


I'm not seeing any further questions in your e-mail?
My bad; i mean, you to comment on below explanation on using 
cur_speed_hz instead of clk_get_rate().



+   mas->cur_speed_hz = spi_slv->max_speed_hz;


Why can't you use clk_get_rate() instead? Or call clk_set_rate() with
the rate you want the master clk to run at and then divide that down
from there?



> Not sure I follow, the intention is to run the controller clock based on
> the slave's max frequency.



That's good. The problem I see is that we have to specify the max
frequency in the controller/bus node, and also in the child/slave
node.
It should only need to be specified in the slave node, so making the
cur_speed_hz equal the max_speed_hz is problematic. The current speed
of
the master should be determined by calling clk_get_rate().


We don't require that the slaves all individually set a speed since it
gets a bit redundant, it should be enough to just use the default the
controller provides.  A bigger problem with this is that the driver
will
never see a transfer which doesn't explicitly have a speed set as the
core will ensure something is set, open coding this logic in every
driver would obviously be tiresome.


clock_get_rate() will returns the rate which got set as per the clock
plan(which was the rounded up frequency) which can be less than or equal
to the requested frequency. For that reason using the cur_speed_hz to
store the requested frequency and skip clock configuration for the
consecutive transfers with same frequency.

I have uploaded the new patchset addressing all these review comments.
Please review it.

--Dilip

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Mark Brown]

On Fri, Aug 17, 2018 at 04:06:06PM +0530, dk...@codeaurora.org wrote:

> Could you please clarify on below query.

I'm not seeing any further questions in your e-mail?


signature.asc
Description: PGP signature

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[dkota]

On 2018-08-14 20:33, Mark Brown wrote:

On Tue, Aug 14, 2018 at 02:30:02PM +0530, dk...@codeaurora.org wrote:

On 2018-08-10 22:16, Mark Brown wrote:
> On Fri, Aug 10, 2018 at 09:59:46PM +0530, dk...@codeaurora.org wrote:



> > delay_usecs is for inter-transfer delays within a message rather than
> > after the initial chip select assert (it can be used to keep chip
> > select
> > asserted for longer after the final transfer too).  Obviously this is
> > also something that shouldn't be configured in a driver specific
> > fashion.



> Hmmm ok, so you mean don't send these as controller_data, rather add
> new
> members to the spi_device struct ?


spi_cs_clk_delay -> Adds Delay from CS line toggle to Clock line 
toggle

spi_inter_words_delay -> Adds inter-word delay for each transfer.



Could you please provide more information on accommodating these
parameters in SPI core structures like spi_device or spi_transfer? Why
because these are very
specific to Qualcomm SPI GENI controller.


I'm not sure what specific information you're looking for here - these
things are not obviously specific to your controller, I'm even aware of
other controllers which can do them.

If we define them in spi core framework structures, SPI Slave driver 
will

program and expect it in the SPI transfers.


Sure.


Thanks Brown for clarifying it. As similar fields are not present in the 
spi core framework i thought these are not generic across the 
controllers.

I will add these fields in the SPI core framework structures.

Could you please clarify on below query.


+   mas->cur_speed_hz = spi_slv->max_speed_hz;


Why can't you use clk_get_rate() instead? Or call clk_set_rate() with
the rate you want the master clk to run at and then divide that down
from there?



> Not sure I follow, the intention is to run the controller clock based on
> the slave's max frequency.



That's good. The problem I see is that we have to specify the max
frequency in the controller/bus node, and also in the child/slave
node.
It should only need to be specified in the slave node, so making the
cur_speed_hz equal the max_speed_hz is problematic. The current speed
of
the master should be determined by calling clk_get_rate().


We don't require that the slaves all individually set a speed since it
gets a bit redundant, it should be enough to just use the default the
controller provides.  A bigger problem with this is that the driver
will
never see a transfer which doesn't explicitly have a speed set as the
core will ensure something is set, open coding this logic in every
driver would obviously be tiresome.


clock_get_rate() will returns the rate which got set as per the clock
plan(which was the rounded up frequency) which can be less than or equal
to the requested frequency. For that reason using the cur_speed_hz to
store the requested frequency and skip clock configuration for the
consecutive transfers with same frequency.

--Dilip

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Mark Brown]

On Tue, Aug 14, 2018 at 02:30:02PM +0530, dk...@codeaurora.org wrote:
> On 2018-08-10 22:16, Mark Brown wrote:
> > On Fri, Aug 10, 2018 at 09:59:46PM +0530, dk...@codeaurora.org wrote:

> > > delay_usecs is for inter-transfer delays within a message rather than
> > > after the initial chip select assert (it can be used to keep chip
> > > select
> > > asserted for longer after the final transfer too).  Obviously this is
> > > also something that shouldn't be configured in a driver specific
> > > fashion.

> > Hmmm ok, so you mean don't send these as controller_data, rather add
> > new
> > members to the spi_device struct ?

> spi_cs_clk_delay -> Adds Delay from CS line toggle to Clock line toggle
> spi_inter_words_delay -> Adds inter-word delay for each transfer.

> Could you please provide more information on accommodating these
> parameters in SPI core structures like spi_device or spi_transfer? Why
> because these are very
> specific to Qualcomm SPI GENI controller.

I'm not sure what specific information you're looking for here - these
things are not obviously specific to your controller, I'm even aware of
other controllers which can do them.

> If we define them in spi core framework structures, SPI Slave driver will
> program and expect it in the SPI transfers.

Sure.


signature.asc
Description: PGP signature

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[dkota]

On 2018-08-10 22:16, Mark Brown wrote:

On Fri, Aug 10, 2018 at 09:59:46PM +0530, dk...@codeaurora.org wrote:

Now the need is, how to communicate the SPI controller maximum 
frequency to

SPI core framework?
Is it by DTSI entry or hardcoding in the SPI controller driver?


If you've got a limit that exists in the IP the hard code it in the
driver.


My stand is for providing the DTSI entry.
Why because, this keeps SPI controller driver generic across the 
boards and

portable.
Also it is not against to Device tree usage because maximum frequency
is describing the property of the hardware.


If the limit the controller has is not coming from the clock tree then
presumably it's a physical limitation of the silicon and isn't going to
vary per board.  If the limit is coming from the board then it should 
be

specified per slave since different slaves may have different
requirements on different boards.


Agree with you guys, will hard code the controller maximum frequency in 
the driver.

I will address this change in the next patchset.


Could you all please comment on the other points too:


Do you mean spi-rx-delay-us and spi-tx-delay-us properties? Those are
documented but don't seem to be used. There's also the delay_usecs
part
of the spi_transfer structure, which may be what you're talking
about.


delay_usecs is for inter-transfer delays within a message rather than
after the initial chip select assert (it can be used to keep chip
select
asserted for longer after the final transfer too).  Obviously this is
also something that shouldn't be configured in a driver specific
fashion.



Hmmm ok, so you mean don't send these as controller_data, rather add
new
members to the spi_device struct ?


spi_cs_clk_delay -> Adds Delay from CS line toggle to Clock line toggle
spi_inter_words_delay -> Adds inter-word delay for each transfer.

Could you please provide more information on accommodating these
parameters in SPI core structures like spi_device or spi_transfer? Why 
because these are very

specific to Qualcomm SPI GENI controller.

If we define them in spi core framework structures, SPI Slave driver 
will program and expect it in the SPI transfers.



+   mas->cur_speed_hz = spi_slv->max_speed_hz;


Why can't you use clk_get_rate() instead? Or call clk_set_rate() with
the rate you want the master clk to run at and then divide that down
from there?



> Not sure I follow, the intention is to run the controller clock based on
> the slave's max frequency.



That's good. The problem I see is that we have to specify the max
frequency in the controller/bus node, and also in the child/slave
node.
It should only need to be specified in the slave node, so making the
cur_speed_hz equal the max_speed_hz is problematic. The current speed
of
the master should be determined by calling clk_get_rate().


We don't require that the slaves all individually set a speed since it
gets a bit redundant, it should be enough to just use the default the
controller provides.  A bigger problem with this is that the driver
will
never see a transfer which doesn't explicitly have a speed set as the
core will ensure something is set, open coding this logic in every
driver would obviously be tiresome.


clock_get_rate() will returns the rate which got set as per the clock 
plan(which was the rounded up frequency) which can be less than or equal 
to the requested frequency. For that reason using the cur_speed_hz to 
store the requested frequency and skip clock configuration for the 
consecutive transfers with same frequency.



--Dilip

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Trent Piepho]

On Thu, 2018-08-09 at 12:37 -0700, Doug Anderson wrote:
> On Thu, Aug 9, 2018 at 11:24 AM, Trent Piepho  wrote:
> > 
> I think we're in agreement but perhaps there's a miscommunication here?
> 
> I'm saying that we _shouldn't_ put the max-speed of the master in the
> device tree.  The max speed for the IP block ought to be in code
> either in the clock driver or in the SPI driver based on the
> compatible string.

Yes, totally agree.

Usually the clock framework provides a clock to the SPI IP block at
some rate, or range of rates.  Based on that clock, the SPI IP block
has a max SPI clock, and the driver can calculate it.

We do have a concept of a SPI master max clock, but it's never needed
to be specified in the DT.

Consider userspace access via spidev.  The transfer asks for 100 MHz,
but the spi master driver can not possibly program that speed into
whatever register(s) control the spi clock.  The SPI core handles that
case of limiting a transfer to the master's max.

> ...as one argument _against_ putting a max-speed for the master in the
> device tree I'm saying that we can already specify a max-speed of each
> slave in the device tree.  The max speed of the slave should take into
> account whatever factors are specific to this board including trace
> lengths, noise, etc.  If we somehow did need to get a max speed in the
> device tree it seems like we could just adjust the max speed for the
> slave to be something lower.  In other words if you know a board has
> an sdm845 on it and you know that the SPI clock on sdm845 can't go
> over 50 MHz then it would make no sense to suggest that we should run
> the SPI clock for a device at 100 MHz.

What you might do is determine the slave can run at 100 MHz, in some
cases.  The board, traces, master, etc. can do it, in some cases.  But
it's also possible to change the clocking design or settings at a
higher level, which trickles down to the SPI master IP block having a
lower clock.  This is handled by the master max speed being less then
the slave's max speed.

The master max wouldn't be in the DT, since it's probably determined at
run time based on input clock to the master.  Based on the SoC core
clock, voltage, power mode, etc.

So you might have a DT with a slave at 100 MHz, even if in some cases
the master's max is less than 100 MHz.  It's ok to depend on the master
to be the limiting factor.

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Trent Piepho]

On Fri, 2018-08-10 at 21:59 +0530, dk...@codeaurora.org wrote:
> 
> Here are my couple of cents:
> SPI controller maximum frequency can be lesser than or equal to Clock 
> framework's maximum
> frequency, so should not rely on the Clock framework.

But there is probably some means, via the controller's IP block input
clock, combined with the controller's IP version (from the compatible
string and a device data table), to derive the controller's max clock.

At least, AFAICT, this has been the case for all existing master
drivers.

> 
> Now the need is, how to communicate the SPI controller maximum frequency 
> to SPI core framework?
> Is it by DTSI entry or hardcoding in the SPI controller driver?
> 
> My stand is for providing the DTSI entry.
> Why because, this keeps SPI controller driver generic across the boards 
> and portable.
> Also it is not against to Device tree usage because maximum frequency
> is describing the property of the hardware.

My point is that not everything that possibly can go in the device tree
should be placed there, just because it can be.  If the master driver
can figure out its max speed, let it do that.

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Doug Anderson]

Hi,

On Fri, Aug 10, 2018 at 9:29 AM,   wrote:
> Here are my couple of cents:
> SPI controller maximum frequency can be lesser than or equal to Clock
> framework's maximum
> frequency, so should not rely on the Clock framework.

You could make that argument on other SoCs perhaps, but from what I've
seen in Qualcomm clock drivers there is a hardcoded frequency plan
that takes into account how fast the IP blocks can go.

-Doug

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Doug Anderson]

Hi,

On Fri, Aug 10, 2018 at 9:43 AM, Mark Brown  wrote:
>> IMO the line marked "/* UNNEEDED */" below should be removed:
>
>> ...
>> spi-max-frequency = <5000>;  /* UNNEEDED */
>
> This is a line in the device tree (which I agree shouldn't be there),
> not code in the SPI driver?

My whole point was that the line above shouldn't be there.  If you
agree to that then we're done.  Everything else was my suggestions of
ways the driver would deal with that line not being there.

-Doug

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Mark Brown]

On Fri, Aug 10, 2018 at 09:59:46PM +0530, dk...@codeaurora.org wrote:

> Now the need is, how to communicate the SPI controller maximum frequency to
> SPI core framework?
> Is it by DTSI entry or hardcoding in the SPI controller driver?

If you've got a limit that exists in the IP the hard code it in the
driver.

> My stand is for providing the DTSI entry.
> Why because, this keeps SPI controller driver generic across the boards and
> portable.
> Also it is not against to Device tree usage because maximum frequency
> is describing the property of the hardware.

If the limit the controller has is not coming from the clock tree then
presumably it's a physical limitation of the silicon and isn't going to
vary per board.  If the limit is coming from the board then it should be
specified per slave since different slaves may have different
requirements on different boards.


signature.asc
Description: PGP signature

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Mark Brown]

On Fri, Aug 10, 2018 at 09:27:05AM -0700, Doug Anderson wrote:
> On Fri, Aug 10, 2018 at 9:13 AM, Mark Brown  wrote:
> > On Fri, Aug 10, 2018 at 08:40:17AM -0700, Doug Anderson wrote:

> >> The clock framework should be able to accomplish what you want.  If
> >> you just request the rate it will do its best to make the rate
> >> requested.  If we want to see what clock would be set before setting

> > The request could be massively off the deliverable rate - 50% or more.

> Agreed.  If the clock is massively off and that causes problems then
> someone will need to debug it and find a solution.  I'm not aware of
> us being in that case in the driver in question.

For the logic in the SPI core it's relatively common, lots of SPI
controllers are limited to something in the region of 10-12.5MHz but it's
common for devices to be able to run up to the region of 30MHz.

> >> >> 3. If you really truly need code in the SPI driver then make sure you
> >> >> include a compatible string for the SoC and have a table in the driver
> >> >> that's found with of_device_get_match_data().  AKA:

> >> It wouldn't be open-coding, it would be a different way of specifying
> >> things.  In my understanding it's always a judgement call about how
> >
> > If you're saying we need clock rate selection logic (which is what it
> > sounds like) rather than data then that seems like a problem.
> 
> We're talking past each other I think.  Maybe a concrete example helps?

...

> IMO the line marked "/* UNNEEDED */" below should be removed:

> ...
> spi-max-frequency = <5000>;  /* UNNEEDED */

This is a line in the device tree (which I agree shouldn't be there),
not code in the SPI driver?

> ...and then the driver should say "oh, I have a compatible string of
> "qcom,geni-spi-sdm845" so I know my controller's max frequency must be
> 50 MHz.  It can get that information using of_device_get_match_data().

> Hopefully that's clearer?

That's just a data table, when you talk about code in the driver
(particularly given the wider discussion of what the maximum rate might
be) it sounds rather more involved than that.


signature.asc
Description: PGP signature

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[dkota]

On 2018-08-10 21:43, Mark Brown wrote:

On Fri, Aug 10, 2018 at 08:40:17AM -0700, Doug Anderson wrote:
On Fri, Aug 10, 2018 at 3:52 AM, Mark Brown  
wrote:



> This is more about matching the data rate between the two drivers - the
> clock framework could (and possibly should) reasonably return an error
> here, we're trying to ensure that drivers and controllers work well
> together here.



The clock framework should be able to accomplish what you want.  If
you just request the rate it will do its best to make the rate
requested.  If we want to see what clock would be set before setting


The request could be massively off the deliverable rate - 50% or more.


is "close enough" in this case?  I haven't gone and dug, but I've
always seen people only specify a max rate for SPI.  ...so as long as
the clock framework gives us something <= the clock we requested then
we should be OK, right?  If / when this becomes a problem then we
should add a min/max to "struct spi_transfer", no?


On the other hand if I ask for my audio to be played at 44.1kHz and the
clock framework says "yes, sure" and gives me 8kHz then the user
experience will be poor.


Note that there are also clk_set_rate_range(), clk_set_min_rate(), and
clk_set_max_rate() though I'm told those might be a bit quirky.


They're certainly not widely used at any rate.


...but maybe we don't need to argue about this anyway since IMHO we
should just use the clk framework to figure out our maximum speed.


It looks like that's true in this case.


>> 3. If you really truly need code in the SPI driver then make sure you
>> include a compatible string for the SoC and have a table in the driver
>> that's found with of_device_get_match_data().  AKA:



>>   compatible = "qcom,geni-spi-sdm845", "qcom,geni-spi";



> A controller driver really shouldn't need to be open coding anything.



It wouldn't be open-coding, it would be a different way of specifying
things.  In my understanding it's always a judgement call about how


If you're saying we need clock rate selection logic (which is what it
sounds like) rather than data then that seems like a problem.


Here are my couple of cents:
SPI controller maximum frequency can be lesser than or equal to Clock 
framework's maximum

frequency, so should not rely on the Clock framework.

SPI controller maximum frequency should not be derived from the SPI 
slave maximum frequency.
Consider the case where N number of slaves connected to SPI controller 
then there will

be N number of slave maximum frequency.
Also it does not look meaningful in communicating the SPI slave maximum 
frequency

as SPI controller maximum frequency to the SPI core framework.

Hence relying on SPI slave maximum frequency for SPI controller maximum 
frequency can be eliminated.


Now the need is, how to communicate the SPI controller maximum frequency 
to SPI core framework?

Is it by DTSI entry or hardcoding in the SPI controller driver?

My stand is for providing the DTSI entry.
Why because, this keeps SPI controller driver generic across the boards 
and portable.

Also it is not against to Device tree usage because maximum frequency
is describing the property of the hardware.

Please add your points.

--Dilip

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Doug Anderson]

Hi,

On Fri, Aug 10, 2018 at 9:13 AM, Mark Brown  wrote:
> On Fri, Aug 10, 2018 at 08:40:17AM -0700, Doug Anderson wrote:
>> On Fri, Aug 10, 2018 at 3:52 AM, Mark Brown  wrote:
>
>> > This is more about matching the data rate between the two drivers - the
>> > clock framework could (and possibly should) reasonably return an error
>> > here, we're trying to ensure that drivers and controllers work well
>> > together here.
>
>> The clock framework should be able to accomplish what you want.  If
>> you just request the rate it will do its best to make the rate
>> requested.  If we want to see what clock would be set before setting
>
> The request could be massively off the deliverable rate - 50% or more.

Agreed.  If the clock is massively off and that causes problems then
someone will need to debug it and find a solution.  I'm not aware of
us being in that case in the driver in question.


>> >> 3. If you really truly need code in the SPI driver then make sure you
>> >> include a compatible string for the SoC and have a table in the driver
>> >> that's found with of_device_get_match_data().  AKA:
>
>> >>   compatible = "qcom,geni-spi-sdm845", "qcom,geni-spi";
>
>> > A controller driver really shouldn't need to be open coding anything.
>
>> It wouldn't be open-coding, it would be a different way of specifying
>> things.  In my understanding it's always a judgement call about how
>
> If you're saying we need clock rate selection logic (which is what it
> sounds like) rather than data then that seems like a problem.

We're talking past each other I think.  Maybe a concrete example helps?

---

IMO the line marked "/* UNNEEDED */" below should be removed:


spi0: spi@88 {
compatible = "qcom,geni-spi";
...
spi-max-frequency = <5000>;  /* UNNEEDED */

device@0 {
spi-max-frequency = <2500>;
...
}
};

---

Said another way: I don't think we should specify the
spi-max-frequency of the _master_ in the device tree.  If there is
_really_ no way to get the max speed from the clock framework and we
_really_ need a per-controller max speed on all geni controllers on
SDM845 then IMO the above should be:

spi0: spi@88 {
compatible = "qcom,geni-spi-sdm845", "qcom,geni-spi";
...

device@0 {
spi-max-frequency = <2500>;
...
}
};

...and then the driver should say "oh, I have a compatible string of
"qcom,geni-spi-sdm845" so I know my controller's max frequency must be
50 MHz.  It can get that information using of_device_get_match_data().

---

Hopefully that's clearer?


-Doug

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Mark Brown]

On Fri, Aug 10, 2018 at 08:40:17AM -0700, Doug Anderson wrote:
> On Fri, Aug 10, 2018 at 3:52 AM, Mark Brown  wrote:

> > This is more about matching the data rate between the two drivers - the
> > clock framework could (and possibly should) reasonably return an error
> > here, we're trying to ensure that drivers and controllers work well
> > together here.

> The clock framework should be able to accomplish what you want.  If
> you just request the rate it will do its best to make the rate
> requested.  If we want to see what clock would be set before setting

The request could be massively off the deliverable rate - 50% or more.

> is "close enough" in this case?  I haven't gone and dug, but I've
> always seen people only specify a max rate for SPI.  ...so as long as
> the clock framework gives us something <= the clock we requested then
> we should be OK, right?  If / when this becomes a problem then we
> should add a min/max to "struct spi_transfer", no?

On the other hand if I ask for my audio to be played at 44.1kHz and the
clock framework says "yes, sure" and gives me 8kHz then the user
experience will be poor.

> Note that there are also clk_set_rate_range(), clk_set_min_rate(), and
> clk_set_max_rate() though I'm told those might be a bit quirky.

They're certainly not widely used at any rate.

> ...but maybe we don't need to argue about this anyway since IMHO we
> should just use the clk framework to figure out our maximum speed.

It looks like that's true in this case.

> >> 3. If you really truly need code in the SPI driver then make sure you
> >> include a compatible string for the SoC and have a table in the driver
> >> that's found with of_device_get_match_data().  AKA:

> >>   compatible = "qcom,geni-spi-sdm845", "qcom,geni-spi";

> > A controller driver really shouldn't need to be open coding anything.

> It wouldn't be open-coding, it would be a different way of specifying
> things.  In my understanding it's always a judgement call about how

If you're saying we need clock rate selection logic (which is what it
sounds like) rather than data then that seems like a problem.


signature.asc
Description: PGP signature

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Doug Anderson]

Hi,

On Fri, Aug 10, 2018 at 3:52 AM, Mark Brown  wrote:
> On Thu, Aug 09, 2018 at 11:03:55AM -0700, Doug Anderson wrote:
>> On Fri, Aug 3, 2018 at 5:18 AM,   wrote:
>
>> > Also, spi core framework will set the transfer speed to controller max
>> > frequency
>> > if transfer frequency is greater than controller max frequency.
>> > Please mention if you have a other opinion.
>
>> 1. It sure seems like the clock framework could be enforcing the max
>> speed here.  SPI can just ask for the speed and the clock framework
>> will pick the highest speed it can if you ask for one too high.  Isn't
>> that the whole point of the "struct freq_tbl" in the clock driver?
>
> This is more about matching the data rate between the two drivers - the
> clock framework could (and possibly should) reasonably return an error
> here, we're trying to ensure that drivers and controllers work well
> together here.

The clock framework should be able to accomplish what you want.  If
you just request the rate it will do its best to make the rate
requested.  If we want to see what clock would be set before setting
it, we could use clk_round_rate().  Then we'd have to decide if the
clock framework is giving us something close enough.  ...but then what
is "close enough" in this case?  I haven't gone and dug, but I've
always seen people only specify a max rate for SPI.  ...so as long as
the clock framework gives us something <= the clock we requested then
we should be OK, right?  If / when this becomes a problem then we
should add a min/max to "struct spi_transfer", no?

Note that there are also clk_set_rate_range(), clk_set_min_rate(), and
clk_set_max_rate() though I'm told those might be a bit quirky.


>> 2. The device tree writer already provides a max clock speed for each
>> SPI slave in the device tree.  ...shouldn't the device tree writer
>> already be taking into account the max of the SPI port when setting
>> this value?
>
> Yes.  We're overriding this because drivers can set a speed from code
> (this is especially common when devices have variable maximum speeds for
> different operations).

OK, makes sense.  You can still cap the max per SPI-slave in the
device tree if you really have to though, right?

...but maybe we don't need to argue about this anyway since IMHO we
should just use the clk framework to figure out our maximum speed.


>> 3. If you really truly need code in the SPI driver then make sure you
>> include a compatible string for the SoC and have a table in the driver
>> that's found with of_device_get_match_data().  AKA:
>
>>   compatible = "qcom,geni-spi-sdm845", "qcom,geni-spi";
>
> A controller driver really shouldn't need to be open coding anything.

It wouldn't be open-coding, it would be a different way of specifying
things.  In my understanding it's always a judgement call about how
much to put in a table that's matched via an SoC-specific compatible
string and how much to describe the SoC-specific behavior device tree
properties.  On the spectrum I've found it's usually best to error on
the side of having a table matched via SoC-specific compatible
strings.

For an example commit of moving _away_ from describing SoC-specific
behavior and just matching on the compatible, see commit 0fbc47d9e426
("phy: rockchip-typec: deprecate some DT properties for various
register fields.") and the associated bindings commit 6912d4f40bc7
("dt-bindings: phy-rockchip-typec: deprecate some register
properties.").


-Doug

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Mark Brown]

On Thu, Aug 09, 2018 at 11:03:55AM -0700, Doug Anderson wrote:
> On Fri, Aug 3, 2018 at 5:18 AM,   wrote:

> > Also, spi core framework will set the transfer speed to controller max
> > frequency
> > if transfer frequency is greater than controller max frequency.
> > Please mention if you have a other opinion.

> 1. It sure seems like the clock framework could be enforcing the max
> speed here.  SPI can just ask for the speed and the clock framework
> will pick the highest speed it can if you ask for one too high.  Isn't
> that the whole point of the "struct freq_tbl" in the clock driver?

This is more about matching the data rate between the two drivers - the
clock framework could (and possibly should) reasonably return an error
here, we're trying to ensure that drivers and controllers work well
together here.

> 2. The device tree writer already provides a max clock speed for each
> SPI slave in the device tree.  ...shouldn't the device tree writer
> already be taking into account the max of the SPI port when setting
> this value?

Yes.  We're overriding this because drivers can set a speed from code
(this is especially common when devices have variable maximum speeds for
different operations).

> 3. If you really truly need code in the SPI driver then make sure you
> include a compatible string for the SoC and have a table in the driver
> that's found with of_device_get_match_data().  AKA:

>   compatible = "qcom,geni-spi-sdm845", "qcom,geni-spi";

A controller driver really shouldn't need to be open coding anything.


signature.asc
Description: PGP signature

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Doug Anderson]

Hi,

On Thu, Aug 9, 2018 at 11:24 AM, Trent Piepho  wrote:
> On Thu, 2018-08-09 at 11:03 -0700, Doug Anderson wrote:
>> On Fri, Aug 3, 2018 at 5:18 AM,   wrote:
>> > > > +   if (of_property_read_u32(pdev->dev.of_node, 
>> > > > "spi-max-frequency",
>> > > > +   &spi->max_speed_hz)) {
>> >
>> >
>> > > Why does this need to come from DT?
>> >
>> >
>> > This is required to set the SPI controller max frequency.
>> > As it is specific to the controller, so looks meaningful to specify it in
>> > dtsi.
>> > Also, spi core framework will set the transfer speed to controller max
>> > frequency
>> > if transfer frequency is greater than controller max frequency.
>> > Please mention if you have a other opinion.
>>
>> Here are my thoughts:
>>
>> 1. It sure seems like the clock framework could be enforcing the max
>> speed here.  SPI can just ask for the speed and the clock framework
>> will pick the highest speed it can if you ask for one too high.  Isn't
>> that the whole point of the "struct freq_tbl" in the clock driver?
>>
>>
>> 2. The device tree writer already provides a max clock speed for each
>> SPI slave in the device tree.  ...shouldn't the device tree writer
>> already be taking into account the max of the SPI port when setting
>> this value?
>
> No, the way it works is the actual speed is the lesser of the master's
> max and the slave device's max.
>
> But usually the master's max is determined by:
>
> 1. The input clock the SPI master device, as provided by the clock
> framework.  Usually the max SPI clock will be this clock /2 or
> something like that.
>
> 2. The master has some maximum clock as part of its specifications, in
> which case the driver basically hard codes it.  Maybe it is different
> based on model and the driver has a table.
>
> The device tree isn't really meant to be a way to remove all data from
> the device driver just because it can be, or shift work from the driver
> author to the device tree author.
>
> So, one shouldn't specify the master max in the DT if the driver could
> figure it out.
>
> Sometimes you see a field in the DT and I think the thought process
> that put it there was, "I don't understand how to set this register,
> I'll just stick in the device tree and then it's Someone Else's Problem
> to find the correct value."
>
> The max speed that some board can talk to a SPI slave might be from the
> specs of the slave device or maybe it's due to the traces on the board
> itself that is the limiting factor.  In the latter case the convention
> is to consider this part of the slave's max speed and put into the DT
> in the slave's DT node max speed property.
>
> So the same spi eeprom chip might have have a max in the DT of 20 MHz
> on one board, copied out of the eeprom's datasheet.  But on another
> board it has a max of 10 MHz because on that board's layout it only
> works up to 10.

I think we're in agreement but perhaps there's a miscommunication here?

I'm saying that we _shouldn't_ put the max-speed of the master in the
device tree.  The max speed for the IP block ought to be in code
either in the clock driver or in the SPI driver based on the
compatible string.

...as one argument _against_ putting a max-speed for the master in the
device tree I'm saying that we can already specify a max-speed of each
slave in the device tree.  The max speed of the slave should take into
account whatever factors are specific to this board including trace
lengths, noise, etc.  If we somehow did need to get a max speed in the
device tree it seems like we could just adjust the max speed for the
slave to be something lower.  In other words if you know a board has
an sdm845 on it and you know that the SPI clock on sdm845 can't go
over 50 MHz then it would make no sense to suggest that we should run
the SPI clock for a device at 100 MHz.


-Doug

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Trent Piepho]

On Thu, 2018-08-09 at 11:03 -0700, Doug Anderson wrote:
> On Fri, Aug 3, 2018 at 5:18 AM,   wrote:
> > > > +   if (of_property_read_u32(pdev->dev.of_node, "spi-max-frequency",
> > > > +   &spi->max_speed_hz)) {
> > 
> > 
> > > Why does this need to come from DT?
> > 
> > 
> > This is required to set the SPI controller max frequency.
> > As it is specific to the controller, so looks meaningful to specify it in
> > dtsi.
> > Also, spi core framework will set the transfer speed to controller max
> > frequency
> > if transfer frequency is greater than controller max frequency.
> > Please mention if you have a other opinion.
> 
> Here are my thoughts:
> 
> 1. It sure seems like the clock framework could be enforcing the max
> speed here.  SPI can just ask for the speed and the clock framework
> will pick the highest speed it can if you ask for one too high.  Isn't
> that the whole point of the "struct freq_tbl" in the clock driver?
> 
> 
> 2. The device tree writer already provides a max clock speed for each
> SPI slave in the device tree.  ...shouldn't the device tree writer
> already be taking into account the max of the SPI port when setting
> this value?

No, the way it works is the actual speed is the lesser of the master's
max and the slave device's max.

But usually the master's max is determined by:

1. The input clock the SPI master device, as provided by the clock
framework.  Usually the max SPI clock will be this clock /2 or
something like that.

2. The master has some maximum clock as part of its specifications, in
which case the driver basically hard codes it.  Maybe it is different
based on model and the driver has a table.

The device tree isn't really meant to be a way to remove all data from
the device driver just because it can be, or shift work from the driver
author to the device tree author.

So, one shouldn't specify the master max in the DT if the driver could
figure it out.

Sometimes you see a field in the DT and I think the thought process
that put it there was, "I don't understand how to set this register,
I'll just stick in the device tree and then it's Someone Else's Problem
to find the correct value."

The max speed that some board can talk to a SPI slave might be from the
specs of the slave device or maybe it's due to the traces on the board
itself that is the limiting factor.  In the latter case the convention
is to consider this part of the slave's max speed and put into the DT
in the slave's DT node max speed property.

So the same spi eeprom chip might have have a max in the DT of 20 MHz
on one board, copied out of the eeprom's datasheet.  But on another
board it has a max of 10 MHz because on that board's layout it only
works up to 10.

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Doug Anderson]

Hi,

On Fri, Aug 3, 2018 at 5:18 AM,   wrote:
>>> +   if (of_property_read_u32(pdev->dev.of_node, "spi-max-frequency",
>>> +   &spi->max_speed_hz)) {
>
>
>> Why does this need to come from DT?
>
>
> This is required to set the SPI controller max frequency.
> As it is specific to the controller, so looks meaningful to specify it in
> dtsi.
> Also, spi core framework will set the transfer speed to controller max
> frequency
> if transfer frequency is greater than controller max frequency.
> Please mention if you have a other opinion.

Here are my thoughts:

1. It sure seems like the clock framework could be enforcing the max
speed here.  SPI can just ask for the speed and the clock framework
will pick the highest speed it can if you ask for one too high.  Isn't
that the whole point of the "struct freq_tbl" in the clock driver?


2. The device tree writer already provides a max clock speed for each
SPI slave in the device tree.  ...shouldn't the device tree writer
already be taking into account the max of the SPI port when setting
this value?


3. If you really truly need code in the SPI driver then make sure you
include a compatible string for the SoC and have a table in the driver
that's found with of_device_get_match_data().  AKA:

  compatible = "qcom,geni-spi-sdm845", "qcom,geni-spi";

...as per #1 and #2 I don't think this is useful


-Doug

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[dkota]

Hi Stephen and Mark,


Do you mean spi-rx-delay-us and spi-tx-delay-us properties? Those are
documented but don't seem to be used. There's also the delay_usecs 
part
of the spi_transfer structure, which may be what you're talking 
about.


delay_usecs is for inter-transfer delays within a message rather than
after the initial chip select assert (it can be used to keep chip 
select

asserted for longer after the final transfer too).  Obviously this is
also something that shouldn't be configured in a driver specific
fashion.



Hmmm ok, so you mean don't send these as controller_data, rather add 
new

members to the spi_device struct ?


spi_cs_clk_delay -> Adds Delay from CS line toggle to Clock line toggle
spi_inter_words_delay -> Adds inter-word delay for each transfer.

Could you please provide more information on accommodating these 
parameters in
SPI core structures like spi_device or spi_transfer? Why because these 
are very

specific to SPI GENI controller.


+   if (of_property_read_u32(pdev->dev.of_node, 
"spi-max-frequency",

+   &spi->max_speed_hz)) {



Why does this need to come from DT?


This is required to set the SPI controller max frequency.
As it is specific to the controller, so looks meaningful to specify it 
in dtsi.
Also, spi core framework will set the transfer speed to controller max 
frequency

if transfer frequency is greater than controller max frequency.
Please mention if you have a other opinion.

Code snippet from spi.c

2826if (ctlr->max_speed_hz && xfer->speed_hz > ctlr->max_speed_hz)
2827xfer->speed_hz = ctlr->max_speed_hz;



+   mas->cur_speed_hz = spi_slv->max_speed_hz;


Why can't you use clk_get_rate() instead? Or call clk_set_rate() with
the rate you want the master clk to run at and then divide that down
from there?



> Not sure I follow, the intention is to run the controller clock based on
> the slave's max frequency.



That's good. The problem I see is that we have to specify the max
frequency in the controller/bus node, and also in the child/slave 
node.

It should only need to be specified in the slave node, so making the
cur_speed_hz equal the max_speed_hz is problematic. The current speed 
of

the master should be determined by calling clk_get_rate().


We don't require that the slaves all individually set a speed since it
gets a bit redundant, it should be enough to just use the default the
controller provides.  A bigger problem with this is that the driver 
will

never see a transfer which doesn't explicitly have a speed set as the
core will ensure something is set, open coding this logic in every
driver would obviously be tiresome.


clock_get_rate() will returns the rate which got set as per the
clock plan(which was the rounded up frequency). For that reason
using the cur_speed_hz.

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Matthias Kaehlcke]

On Thu, May 03, 2018 at 03:34:43PM -0600, Girish Mahadevan wrote:
> This driver supports GENI based SPI Controller in the Qualcomm SOCs. The
> Qualcomm Generic Interface (GENI) is a programmable module supporting a
> wide range of serial interfaces including SPI. This driver supports SPI
> operations using FIFO mode of transfer.
> 
> Signed-off-by: Girish Mahadevan 
> ---
>  drivers/spi/Kconfig   |  12 +
>  drivers/spi/Makefile  |   1 +
>  drivers/spi/spi-geni-qcom.c   | 766 
> ++
>  include/linux/spi/spi-geni-qcom.h |  14 +
>  4 files changed, 793 insertions(+)
>  create mode 100644 drivers/spi/spi-geni-qcom.c
>  create mode 100644 include/linux/spi/spi-geni-qcom.h
> 
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> new file mode 100644
> index 000..eecc634
> --- /dev/null
> +++ b/drivers/spi/spi-geni-qcom.c
>
> ...
>
> +static irqreturn_t geni_spi_isr(int irq, void *dev)
> +{
> + struct spi_geni_master *mas = dev;
> + struct geni_se *se = &mas->se;
> + u32 m_irq = 0;
> + irqreturn_t ret = IRQ_HANDLED;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&mas->lock, flags);
> + if (pm_runtime_status_suspended(dev)) {

kasan is unhappy about geni_spi_isr:

[3.206593] BUG: KASAN: slab-out-of-bounds in geni_spi_isr+0x978/0xbf4
[3.213310] Read of size 4 at addr ffc0da803b04 by task swapper/0/1
[3.221664] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.14.47 #20
[3.227936] Hardware name: Google Cheza (DT)
[3.232341] Call trace:
[3.234884] [] dump_backtrace+0x0/0x6d0
[3.240441] [] show_stack+0x20/0x2c
[3.245649] [] __dump_stack+0x20/0x28
[3.251034] [] dump_stack+0xcc/0xf4
[3.256240] [] print_address_description+0x70/0x238
[3.262868] [] kasan_report+0x1cc/0x260
[3.268425] [] __asan_report_load4_noabort+0x2c/0x38
[3.275142] [] geni_spi_isr+0x978/0xbf4
...
[3.662568] Allocated by task 1:
[3.665908]  kasan_kmalloc+0xb4/0x174
[3.669693]  __kmalloc+0x260/0x2f4
[3.673201]  __spi_alloc_controller+0x38/0x180
[3.677781]  spi_geni_probe+0x38/0x574
[3.681647]  platform_drv_probe+0xac/0x134
[3.685865]  driver_probe_device+0x470/0x4f4
[3.690268]  __driver_attach+0xe8/0x128
[3.694228]  bus_for_each_dev+0x104/0x16c
[3.698356]  driver_attach+0x48/0x54
[3.702052]  bus_add_driver+0x258/0x3d0
[3.706010]  driver_register+0x1ac/0x230
[3.710056]  __platform_driver_register+0xcc/0xdc
[3.714906]  spi_geni_driver_init+0x1c/0x24
[3.719220]  do_one_initcall+0x22c/0x3c4
[3.723266]  kernel_init_freeable+0x31c/0x40c
[3.727753]  kernel_init+0x14/0x10c
[3.731349]  ret_from_fork+0x10/0x18

Reason is that 'dev' is passed to pm_runtime_status_suspended(), when
it should be 'mas->dev'.

As this bug indicates kernel developers have strong expectations what
a variable called 'dev' represents, I suggest to change it to
something like 'data'.

Thanks

Matthias

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Mark Brown]

On Thu, May 24, 2018 at 10:25:58AM -0600, Mahadevan, Girish wrote:

> The reason I have the cur_speed_hz is that there is a max_speed_hz which
> is the max frequency the slave can do; but every transfer can also
> specify a speed_hz and override this.

Every transfer *will* specify a speed, you should never see a transfer
that doesn't specify a speed.

> > delay_usecs is for inter-transfer delays within a message rather than
> > after the initial chip select assert (it can be used to keep chip select
> > asserted for longer after the final transfer too).  Obviously this is
> > also something that shouldn't be configured in a driver specific
> > fashion.

> Hmmm ok, so you mean don't send these as controller_data, rather add new
> members to the spi_device struct ?

Yes, that'd be one way to do it.


signature.asc
Description: PGP signature

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Mahadevan, Girish]

Hi Mark, Stephen

On 5/22/2018 11:30 AM, Mark Brown wrote:
>> That's good. The problem I see is that we have to specify the max
>> frequency in the controller/bus node, and also in the child/slave node.
>> It should only need to be specified in the slave node, so making the
>> cur_speed_hz equal the max_speed_hz is problematic. The current speed of
>> the master should be determined by calling clk_get_rate().
>
> We don't require that the slaves all individually set a speed since it
> gets a bit redundant, it should be enough to just use the default the
> controller provides.  A bigger problem with this is that the driver will
> never see a transfer which doesn't explicitly have a speed set as the
> core will ensure something is set, open coding this logic in every
> driver would obviously be tiresome.

Sorry , I need some more clarification.

When I register the master, I specify the max rate the core can support
(50 Mhz). So any transaction that comes to the driver is going to be
requesting at-most 50 Mhz.

The reason I have the cur_speed_hz is that there is a max_speed_hz which
is the max frequency the slave can do; but every transfer can also
specify a speed_hz and override this.

So my point is we can do upto 4 slaves on a given controller, each slave
can have a different max speed and each slave's transfer can override
the max_frequency of that slave device.
(the default frequency is the master's max frequency)

>> Do you mean spi-rx-delay-us and spi-tx-delay-us properties? Those are
>> documented but don't seem to be used. There's also the delay_usecs part
>> of the spi_transfer structure, which may be what you're talking about.
> 
> delay_usecs is for inter-transfer delays within a message rather than
> after the initial chip select assert (it can be used to keep chip select
> asserted for longer after the final transfer too).  Obviously this is
> also something that shouldn't be configured in a driver specific
> fashion.
> 

Hmmm ok, so you mean don't send these as controller_data, rather add new
members to the spi_device struct ?

Best Regards
Girish

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora
Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Mark Brown]

On Mon, May 21, 2018 at 03:45:09PM -0600, Mahadevan, Girish wrote:

> I can resubmit the SPI binding documentation as part of this patch series.

Yes, and realy I'd expect to see the SPI subdevices being documented in
a separate SPI binding document rather than just lumped in with the
parent documentation.  The same thing probably applies to the rest of
the bindings in there which I'm guessing have also not been reviewed in
the relevant subsystems.


signature.asc
Description: PGP signature

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Mark Brown]

On Tue, May 22, 2018 at 09:46:39AM -0700, Stephen Boyd wrote:
> Quoting Mahadevan, Girish (2018-05-21 08:52:47)

> > Not sure I follow, the intention is to run the controller clock based on
> > the slave's max frequency.

> That's good. The problem I see is that we have to specify the max
> frequency in the controller/bus node, and also in the child/slave node.
> It should only need to be specified in the slave node, so making the
> cur_speed_hz equal the max_speed_hz is problematic. The current speed of
> the master should be determined by calling clk_get_rate().

We don't require that the slaves all individually set a speed since it
gets a bit redundant, it should be enough to just use the default the
controller provides.  A bigger problem with this is that the driver will
never see a transfer which doesn't explicitly have a speed set as the
core will ensure something is set, open coding this logic in every
driver would obviously be tiresome.

> > The intention was to allow a client to specify slave specific timing
> > requirements, e.g CS-CLK delay (based on the slave's data sheet).
> > So that the client drivers could setup these delays and pass it in part
> > of the controller_data member of the spi_device data structure.
> > The header file was meant to expose these timing params that the client
> > could specify. I honestly didn't know how else a client could specify
> > these to the controller driver.

> Do you mean spi-rx-delay-us and spi-tx-delay-us properties? Those are
> documented but don't seem to be used. There's also the delay_usecs part
> of the spi_transfer structure, which may be what you're talking about.

delay_usecs is for inter-transfer delays within a message rather than
after the initial chip select assert (it can be used to keep chip select
asserted for longer after the final transfer too).  Obviously this is
also something that shouldn't be configured in a driver specific
fashion.


signature.asc
Description: PGP signature

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Stephen Boyd]

Quoting Mahadevan, Girish (2018-05-21 08:52:47)
> Hi Stephen
> 
> On 5/11/2018 4:30 PM, Stephen Boyd wrote:
> 
> >> +   if (mode & SPI_CPHA)
> >> +   cpha |= CPHA;
> >> +
> >> +   if (spi_slv->mode & SPI_CS_HIGH)
> >> +   demux_output_inv |= BIT(spi_slv->chip_select);
> >> +
> >> +   if (spi_slv->controller_data) {
> >> +   u32 cs_clk_delay = 0;
> >> +   u32 inter_words_delay = 0;
> >> +
> >> +   delay_params =
> >> +   (struct spi_geni_qcom_ctrl_data *) 
> >> spi_slv->controller_data;
> >> +   cs_clk_delay =
> >> +   (delay_params->spi_cs_clk_delay << SPI_CS_CLK_DELAY_SHFT)
> >> +   & 
> >> SPI_CS_CLK_DELAY_MSK;
> >> +   inter_words_delay =
> >> +   delay_params->spi_inter_words_delay &
> >> +   SPI_INTER_WORDS_DELAY_MSK;
> >> +   spi_delay_params =
> >> +   (inter_words_delay | cs_clk_delay);
> >> +   }
> >> +
> >> +   demux_sel = spi_slv->chip_select;
> >> +   mas->cur_speed_hz = spi_slv->max_speed_hz;
> > 
> > Why can't you use clk_get_rate() instead? Or call clk_set_rate() with
> > the rate you want the master clk to run at and then divide that down
> > from there?
> 
> Not sure I follow, the intention is to run the controller clock based on
> the slave's max frequency.

That's good. The problem I see is that we have to specify the max
frequency in the controller/bus node, and also in the child/slave node.
It should only need to be specified in the slave node, so making the
cur_speed_hz equal the max_speed_hz is problematic. The current speed of
the master should be determined by calling clk_get_rate().

> > 
> >> +   }
> >> +
> >> +   if (unlikely(!mas->setup)) {
> >> +   int proto = geni_se_read_proto(se);
> >> +   unsigned int major;
> >> +   unsigned int minor;
> >> +   unsigned int step;
> >> +
> >> +   if (unlikely(proto != GENI_SE_SPI)) {
> >> +   dev_err(mas->dev, "Invalid proto %d\n", proto);
> >> +   return -ENXIO;
> >> +   }
> >> +   mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se);
> >> +   mas->rx_fifo_depth = geni_se_get_rx_fifo_depth(se);
> >> +   mas->tx_fifo_width = geni_se_get_tx_fifo_width(se);
> >> +   geni_se_init(se, 0x0, (mas->tx_fifo_depth - 2));
> > 
> > Why 2? Drop extra parenthesis please.
> 
> This is what the hardware programming doc recommends, the parameter is
> actually the rx_rfr_watermark, something that doesn't apply to non-UART
> protocols.
> (I will add a detailed comment)

Thanks. Just wanted to make sure we don't forget what the magic number
means.

>  an else if.
> > 
> >> +   m_param |= FRAGMENTATION;
> >> +   }
> >> +
> >> +   mas->cur_xfer = xfer;
> >> +   if (m_cmd & SPI_TX_ONLY) {
> >> +   mas->tx_rem_bytes = xfer->len;
> >> +   writel_relaxed(trans_len, se->base + SE_SPI_TX_TRANS_LEN);
> >> +   }
> >> +
> >> +   if (m_cmd & SPI_RX_ONLY) {
> >> +   writel_relaxed(trans_len, se->base + SE_SPI_RX_TRANS_LEN);
> >> +   mas->rx_rem_bytes = xfer->len;
> >> +   }
> >> +   writel_relaxed(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
> >> +   geni_se_setup_m_cmd(se, m_cmd, m_param);
> >> +   if (m_cmd & SPI_TX_ONLY)
> >> +   writel_relaxed(mas->tx_wm, se->base + 
> >> SE_GENI_TX_WATERMARK_REG);
> > 
> > This can't be combined with above m_cmd & SPI_TX_ONLY statement?
> 
> No, writing to this register should be done after we enqueue the command
> to the GENI engine(the geni_se_setup_m_cmd), but some of the transaction
> properties (length etc) should be setup before we enqueue the GENI command.

Ok.

> 
> > 
> > More things can be unsigned?
> > 
> >> +   fifo_byte = (u8 *)&fifo_word;
> >> +   for (j = 0; j < bytes_to_write; j++)
> >> +   fifo_byte[j] = tx_buf[i++];
> > 
> > Why are we doing all this work to fill in a fifo byte at a time? tx_buf
> > should be a buffer of bytes that we can iowrite32_rep() with directly.
> > And then we could just run iowrite32_rep() with some count of bytes to
> > write? I suppose we may run into problems with unaligned size buffers,
> > but it sounds like that doesn't happen?
> 
> I did this for 2 reasons.
> 1.The core can handle different byte order transmissions (e.g MSB
> first), I am quite honestly not sure how the client's will setup the
> data buffer in these cases.(for bigger word sizes , 16/32)
> [we plan to support these]
> 2. For non-byte aligned word sizes (e,g 9), I am not enabling FIFO word
> packing (meaning 1 SPI-WORD/FIFO-WORD in these cases).

Alright. I'm not certain how the incoming buffer looks when clients
request certain modes

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Mahadevan, Girish]

Hi Mark,

On 5/17/2018 1:21 AM, Mark Brown wrote:
> On Mon, May 07, 2018 at 02:29:45PM -0600, Mahadevan, Girish wrote:
>> On 5/3/2018 5:38 PM, Mark Brown wrote:
> 
>>> This is a DT based driver but there is no binding documentation.
>>> Binding documentation is required for any new DT stuff.
> 
>> The DT documentation for the SPI driver was done as part of this patch series
>> https://patchwork.kernel.org/patch/10318125/ 
> 
> I can't follow the link as I'm working offline but since I've no record
> of having seen a copy of any bindings for review and I'm fairly sure I'd
> have remembered any bindings without code I'm very disappointed -
> bindings should be being reviewed by the relevant maintainers just like
> code.
>   
> Fortunately as far as I can tell whereever you sent that to it doesn't
> seem to have been applied but that makes it even more disappointing that
> they're not being sent.
> 

https://patchwork.kernel.org/patch/10318125/
[
Add device tree binding support for the QCOM GENI SE driver.

Signed-off-by: Karthikeyan Ramasubramanian 
Signed-off-by: Sagar Dharia 
Signed-off-by: Girish Mahadevan 
Reviewed-by: Rob Herring 
Reviewed-by: Stephen Boyd 
]

is a patch train for Generic Interface (GENI) based Qualcomm Universal
Peripheral (QUP) wrapper. The wrapper can contain one or more mini cores
that can be used to implement different serial protocols (I2C/SPI/UART).
We'd submitted the DT bindings for that wrapper core and for UART/I2C
drivers which were part of that patch train; but there was a comment to
add the SPI binding document even without the SPI driver (attaching that
email thread).
I can resubmit the SPI binding documentation as part of this patch series.

Best Regards
Girish

> ...
> 
> Please include human readable descriptions of things like commits and
> issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet access.
> I do frequently catch up on my mail on flights or while otherwise
> travelling so this is even more pressing for me than just being about
> making things a bit easier to read.
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora
Forum, a Linux Foundation Collaborative Project.

Subject:
Fwd: Re: [PATCH v3 1/4] dt-bindings: soc: qcom: Add device tree binding for 
GENI SE
From:
Karthik Ramasubramanian 
Date:
5/21/2018 10:44 AM
To:
"Mahadevan, Girish" 




 Forwarded Message 
Subject: Re: [PATCH v3 1/4] dt-bindings: soc: qcom: Add device tree
binding for GENI SE
Date: Tue, 6 Mar 2018 10:13:09 -0700
From: Karthik Ramasubramanian 
To: Rob Herring 
CC: Jonathan Corbet , Andy Gross
, David Brown , Mark
Rutland , Wolfram Sang , Greg
Kroah-Hartman , linux-...@vger.kernel.org,
linux-arm-msm ,
devicet...@vger.kernel.org, Linux I2C ,
linux-ser...@vger.kernel.org, Jiri Slaby ,
evgr...@chromium.org, acour...@chromium.org, Sagar Dharia
, Girish Mahadevan 



On 3/6/2018 6:22 AM, Rob Herring wrote:
> > On Mon, Mar 5, 2018 at 6:55 PM, Karthik Ramasubramanian
> >  wrote:
>> >>
>> >>
>> >> On 3/5/2018 4:58 PM, Rob Herring wrote:
>>> >>>
>>> >>> On Tue, Feb 27, 2018 at 06:38:06PM -0700, Karthikeyan Ramasubramanian
>>> >>> wrote:
 
  Add device tree binding support for the QCOM GENI SE driver.
 
  Signed-off-by: Karthikeyan Ramasubramanian 
  Signed-off-by: Sagar Dharia 
  Signed-off-by: Girish Mahadevan 
  ---
 .../devicetree/bindings/soc/qcom/qcom,geni-se.txt  | 89
  ++
 1 file changed, 89 insertions(+)
 create mode 100644
  Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
 
  diff --git 
  a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
  b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
  new file mode 100644
  index 000..fe6a0c0
  --- /dev/null
  +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
  @@ -0,0 +1,89 @@
  +Qualcomm Technologies, Inc. GENI Serial Engine QUP Wrapper Controller
  +
  +Generic Interface (GENI) based Qualcomm Universal Peripheral (QUP)
  wrapper
  +is a programmable module for supporting a wide range of serial
  interfaces
  +like UART, SPI, I2C, I3C, etc. A single QUP module can provide upto 8
  Serial
  +Interfaces, using its internal Serial Engines. The GENI Serial Engine
  QUP
  +Wrapper controller is modeled as a node with zero or more child nodes
  each
  +representing a serial engine.
  +
  +Required properties:
  +- compatible:  Must be "qcom,geni-se-qup".
  +- reg: Must contain QUP register address and length.
  +- clock-names: Must contain "m-ahb" and "s-ahb".
  +- clocks:  AHB clocks needed by 

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Mahadevan, Girish]

Hi Stephen

On 5/11/2018 4:30 PM, Stephen Boyd wrote:

>> +   if (mode & SPI_CPHA)
>> +   cpha |= CPHA;
>> +
>> +   if (spi_slv->mode & SPI_CS_HIGH)
>> +   demux_output_inv |= BIT(spi_slv->chip_select);
>> +
>> +   if (spi_slv->controller_data) {
>> +   u32 cs_clk_delay = 0;
>> +   u32 inter_words_delay = 0;
>> +
>> +   delay_params =
>> +   (struct spi_geni_qcom_ctrl_data *) spi_slv->controller_data;
>> +   cs_clk_delay =
>> +   (delay_params->spi_cs_clk_delay << SPI_CS_CLK_DELAY_SHFT)
>> +   & 
>> SPI_CS_CLK_DELAY_MSK;
>> +   inter_words_delay =
>> +   delay_params->spi_inter_words_delay &
>> +   SPI_INTER_WORDS_DELAY_MSK;
>> +   spi_delay_params =
>> +   (inter_words_delay | cs_clk_delay);
>> +   }
>> +
>> +   demux_sel = spi_slv->chip_select;
>> +   mas->cur_speed_hz = spi_slv->max_speed_hz;
> 
> Why can't you use clk_get_rate() instead? Or call clk_set_rate() with
> the rate you want the master clk to run at and then divide that down
> from there?

Not sure I follow, the intention is to run the controller clock based on
the slave's max frequency.

>> +
>> +   ret = pm_runtime_get_sync(mas->dev);
>> +   if (ret < 0) {
>> +   dev_err(mas->dev, "Error enabling SE resources\n");
>> +   pm_runtime_put_noidle(mas->dev);
>> +   goto exit_prepare_transfer_hardware;
>> +   } else {
>> +   ret = 0;
> 
> Does pm_runtime_get_sync() return anything besides 0 on success?

This will go away, since I will switch to using auto-runtime option
provided by the framework.

> 
>> +   }
>> +
>> +   if (unlikely(!mas->setup)) {
>> +   int proto = geni_se_read_proto(se);
>> +   unsigned int major;
>> +   unsigned int minor;
>> +   unsigned int step;
>> +
>> +   if (unlikely(proto != GENI_SE_SPI)) {
>> +   dev_err(mas->dev, "Invalid proto %d\n", proto);
>> +   return -ENXIO;
>> +   }
>> +   mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se);
>> +   mas->rx_fifo_depth = geni_se_get_rx_fifo_depth(se);
>> +   mas->tx_fifo_width = geni_se_get_tx_fifo_width(se);
>> +   geni_se_init(se, 0x0, (mas->tx_fifo_depth - 2));
> 
> Why 2? Drop extra parenthesis please.

This is what the hardware programming doc recommends, the parameter is
actually the rx_rfr_watermark, something that doesn't apply to non-UART
protocols.
(I will add a detailed comment)
 an else if.
> 
>> +   m_param |= FRAGMENTATION;
>> +   }
>> +
>> +   mas->cur_xfer = xfer;
>> +   if (m_cmd & SPI_TX_ONLY) {
>> +   mas->tx_rem_bytes = xfer->len;
>> +   writel_relaxed(trans_len, se->base + SE_SPI_TX_TRANS_LEN);
>> +   }
>> +
>> +   if (m_cmd & SPI_RX_ONLY) {
>> +   writel_relaxed(trans_len, se->base + SE_SPI_RX_TRANS_LEN);
>> +   mas->rx_rem_bytes = xfer->len;
>> +   }
>> +   writel_relaxed(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
>> +   geni_se_setup_m_cmd(se, m_cmd, m_param);
>> +   if (m_cmd & SPI_TX_ONLY)
>> +   writel_relaxed(mas->tx_wm, se->base + 
>> SE_GENI_TX_WATERMARK_REG);
> 
> This can't be combined with above m_cmd & SPI_TX_ONLY statement?

No, writing to this register should be done after we enqueue the command
to the GENI engine(the geni_se_setup_m_cmd), but some of the transaction
properties (length etc) should be setup before we enqueue the GENI command.
   setup_fifo_xfer(xfer, mas, slv->mode, spi);
>> +   timeout = wait_for_completion_timeout(&mas->xfer_done,
>> +   msecs_to_jiffies(SPI_XFER_TIMEOUT_MS));
> 
> Can you implement the 'handle_err' for the controller and call
> spi_finalize_current_transfer() from the interrupt handler when the
> transfer completes? The completion variable stuff and timeout code in
> this driver can hopefully all go away.

Will do (thanks for the suggestion).

> 
> More things can be unsigned?
> 
>> +   fifo_byte = (u8 *)&fifo_word;
>> +   for (j = 0; j < bytes_to_write; j++)
>> +   fifo_byte[j] = tx_buf[i++];
> 
> Why are we doing all this work to fill in a fifo byte at a time? tx_buf
> should be a buffer of bytes that we can iowrite32_rep() with directly.
> And then we could just run iowrite32_rep() with some count of bytes to
> write? I suppose we may run into problems with unaligned size buffers,
> but it sounds like that doesn't happen?

I did this for 2 reasons.
1.The core can handle different byte order transmissions (e.g MSB
first), I am quite honestly not sure how the client's will setup the
data buffer in these cases.(for bigger word siz

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Mark Brown]

On Mon, May 07, 2018 at 02:29:45PM -0600, Mahadevan, Girish wrote:
> On 5/3/2018 5:38 PM, Mark Brown wrote:

> > This is a DT based driver but there is no binding documentation.
> > Binding documentation is required for any new DT stuff.

> The DT documentation for the SPI driver was done as part of this patch series
> https://patchwork.kernel.org/patch/10318125/ 

I can't follow the link as I'm working offline but since I've no record
of having seen a copy of any bindings for review and I'm fairly sure I'd
have remembered any bindings without code I'm very disappointed -
bindings should be being reviewed by the relevant maintainers just like
code.

Fortunately as far as I can tell whereever you sent that to it doesn't
seem to have been applied but that makes it even more disappointing that
they're not being sent.

...

Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.


signature.asc
Description: PGP signature

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Stephen Boyd]

Quoting Girish Mahadevan (2018-05-03 14:34:43)
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 9b31351..358d60a 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -564,6 +564,18 @@ config SPI_QUP
>   This driver can also be built as a module.  If so, the module
>   will be called spi_qup.
>  
> +config SPI_QCOM_GENI
> +   tristate "Qualcomm SPI controller with QUP interface"

This is the same help text as the SPI_QUP config up above. Please make
it different somehow by adding GENI or something like that instead of
QUP?

> +   depends on ARCH_QCOM || (ARM && COMPILE_TEST)

This driver uses the GENI wrapper code so it may need to have a better
Kconfig dependency than this.

> +   help
> + This driver supports GENI serial engine based SPI controller in
> + master mode on the Qualcomm Technologies Inc.'s SoCs. If you say
> + yes to this option, support will be included for the built-in SPI

Drop "built-in"?

> + interface on the Qualcomm Technologies Inc.'s SoCs.
> +
> + This driver can also be built as a module.  If so, the module
> + will be called spi-geni-qcom.
> +
>  config SPI_S3C24XX
> tristate "Samsung S3C24XX series SPI"
> depends on ARCH_S3C24XX
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index a3ae2b7..cc90d6e 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -77,6 +77,7 @@ spi-pxa2xx-platform-objs  := spi-pxa2xx.o 
> spi-pxa2xx-dma.o
>  obj-$(CONFIG_SPI_PXA2XX)   += spi-pxa2xx-platform.o
>  obj-$(CONFIG_SPI_PXA2XX_PCI)   += spi-pxa2xx-pci.o
>  obj-$(CONFIG_SPI_QUP)  += spi-qup.o
> +obj-$(CONFIG_SPI_QCOM_GENI)+= spi-geni-qcom.o

This should come before QUP.

>  obj-$(CONFIG_SPI_ROCKCHIP) += spi-rockchip.o
>  obj-$(CONFIG_SPI_RB4XX)+= spi-rb4xx.o
>  obj-$(CONFIG_SPI_RSPI) += spi-rspi.o
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> new file mode 100644
> index 000..eecc634
> --- /dev/null
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -0,0 +1,766 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

include platform_device.h instead of of_platform.h?

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define SPI_NUM_CHIPSELECT 4

Why do we need the define? It's used one place.

> +#define SPI_XFER_TIMEOUT_MS250

Same comment.

> +/* SPI SE specific registers */
> +#define SE_SPI_CPHA0x224
> +#define SE_SPI_LOOPBACK0x22c
> +#define SE_SPI_CPOL0x230
> +#define SE_SPI_DEMUX_OUTPUT_INV0x24c
> +#define SE_SPI_DEMUX_SEL   0x250
> +#define SE_SPI_TRANS_CFG   0x25c
> +#define SE_SPI_WORD_LEN0x268
> +#define SE_SPI_TX_TRANS_LEN0x26c
> +#define SE_SPI_RX_TRANS_LEN0x270
> +#define SE_SPI_PRE_POST_CMD_DLY0x274
> +#define SE_SPI_DELAY_COUNTERS  0x278
> +
> +/* SE_SPI_CPHA register fields */
> +#define CPHA   BIT(0)

Can you put these defines next to the register that they correspond to?
Then we don't need the duplicate comment to indicate what registers they
are used with. 

> +
> +/* SE_SPI_LOOPBACK register fields */
> +#define LOOPBACK_ENABLE0x1
> +#define NORMAL_MODE0x0
> +#define LOOPBACK_MSK   GENMASK(1, 0)
> +
> +/* SE_SPI_CPOL register fields */
> +#define CPOL   BIT(2)
> +
> +/* SE_SPI_DEMUX_OUTPUT_INV register fields */
> +#define CS_DEMUX_OUTPUT_INV_MSKGENMASK(3, 0)
> +
> +/* SE_SPI_DEMUX_SEL register fields */
> +#define CS_DEMUX_OUTPUT_SELGENMASK(3, 0)
> +
> +/* SE_SPI_TX_TRANS_CFG register fields */
> +#define CS_TOGGLE  BIT(0)
> +
> +/* SE_SPI_WORD_LEN register fields */
> +#define WORD_LEN_MSK   GENMASK(9, 0)
> +#define MIN_WORD_LEN   4
> +
> +/* SPI_TX/SPI_RX_TRANS_LEN fields */
> +#define TRANS_LEN_MSK  GENMASK(23, 0)
> +
> +/* SE_SPI_DELAY_COUNTERS */
> +#define SPI_INTER_WORDS_DELAY_MSK  GENMASK(9, 0)
> +#define SPI_CS_CLK_DELAY_MSK   GENMASK(19, 10)
> +#define SPI_CS_CLK_DELAY_SHFT  10
> +
> +/* M_CMD OP codes for SPI */
> +#define SPI_TX_ONLY1
> +#define SPI_RX_ONLY2
> +#define SPI_FULL_DUPLEX3
> +#define SPI_TX_RX  7
> +#define SPI_CS_ASSERT  8
> +#define SPI_CS_DEASSERT9
> +#define SPI_SCK_ONLY   10
> +/* M_CMD params for SPI */
> +#define SPI_PRE_CMD_DELAY  BIT(0)
> +#define TIMESTAMP_BEFORE   BIT(1)
> +#define FRAGMENTATION  BIT(2)
> +#define TIMESTAMP_AFTERBIT(3)
> +#define POST_CMD_DELAY BIT(4)
> +
> +static irqreturn_t geni_spi_isr(int irq, void *dev);
> +
> +struct spi_geni_m

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Mahadevan, Girish]

Hi Mark

On 5/3/2018 5:38 PM, Mark Brown wrote:
> On Thu, May 03, 2018 at 03:34:43PM -0600, Girish Mahadevan wrote:
>> This driver supports GENI based SPI Controller in the Qualcomm SOCs. The
>> Qualcomm Generic Interface (GENI) is a programmable module supporting a
>> wide range of serial interfaces including SPI. This driver supports SPI
>> operations using FIFO mode of transfer.
> 
> This is a DT based driver but there is no binding documentation.
> Binding documentation is required for any new DT stuff.
>

The DT documentation for the SPI driver was done as part of this patch
series
https://patchwork.kernel.org/patch/10318125/

>> +   depends on ARCH_QCOM || (ARM && COMPILE_TEST)
> > Why the ARM dependency?  There's no architecture specific headers
> included...

Agree, I will remove it. I will add the dependency on QCOM_GENI_SE(to be
consistent with the other GENI_QUP protocol drivers (I2C and UART))

>>  obj-$(CONFIG_SPI_PXA2XX_PCI)+= spi-pxa2xx-pci.o
>>  obj-$(CONFIG_SPI_QUP)   += spi-qup.o
>> +obj-$(CONFIG_SPI_QCOM_GENI) += spi-geni-qcom.o
>>  obj-$(CONFIG_SPI_ROCKCHIP)  += spi-rockchip.o
> 
> Please keep Kconfig and Makefile alphabetically sorted to reduce
> conflicts.
> 
Ok.
>> +static struct spi_master *get_spi_master(struct device *dev)
>> +{
>> +struct platform_device *pdev = to_platform_device(dev);
>> +struct spi_master *spi = platform_get_drvdata(pdev);
>> +
>> +return spi;
>> +}
> 
> This doesn't look at all driver specific with the current naming but it
> actually is given that other drivers may use other driver data so it
> should be renamed.  I'm also not clear why it's bouncing through the
> platform device, dev_get_drvdata() exists.
> 
Agree, this function isn't needed, dev_get_drvdata() should be sufficient.
>> +static int spi_geni_unprepare_message(struct spi_master *spi_mas,
>> +struct spi_message *spi_msg)
>> +{
>> +struct spi_geni_master *mas = spi_master_get_devdata(spi_mas);
>> +
>> +mas->cur_speed_hz = 0;
>> +mas->cur_word_len = 0;
>> +return 0;
>> +}
> 
> Is this really useful?  If the driver needs to reconfigure for every
> message then it should just do that and not care about the state.  If it
> might end up caring about the state anyway that suggests there's some
> kind of bug somewhere that's being masked.
> 
Agree, it can be removed.
>> +static int spi_geni_prepare_transfer_hardware(struct spi_master *spi)
>> +{
>> +struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> +int ret = 0;
>> +struct geni_se *se = &mas->se;
>> +
>> +ret = pm_runtime_get_sync(mas->dev);
>> +if (ret < 0) {
> 
> Use auto_runtime_pm.
> 
Ok
>> +if (unlikely(!mas->setup)) {
>> +int proto = geni_se_read_proto(se);
> 
> Does this really need a likely/unlikely annotation - it shouldn't be any
> kind of hot path...  There's a lot of these annotations in the code.
> 
Ok
>> +ret = devm_request_irq(mas->dev, mas->irq, geni_spi_isr,
>> +   IRQF_TRIGGER_HIGH, "spi_geni", mas);
>> +if (ret) {
>> +dev_err(mas->dev, "Request_irq failed:%d: err:%d\n",
> 
> Why are we dynamically requesting the IRQ outside of probe?  Normally an
> interrupt is requested on startup and held through the life of the
> device.  I'm also not seeing any sign that it's freed except via devm...
> 
Ok, will move this to probe.
>> +spi->bus_num = of_alias_get_id(pdev->dev.of_node, "spi");
> 
> Don't do this, just set bus_num to -1 and let the core assign an ID.
> 
Ok.
>> +spi->dev.of_node = pdev->dev.of_node;
> 
> This is broken, the virtual SPI device does not exist in DT and this
> might break things.
> 
I don't follow, if I don't do this the framework won't be able to probe
the slave devices of the controller.
>> +pm_runtime_enable(&pdev->dev);
>> +ret = spi_register_master(spi);
> 
> No devm?
> 
Agree, I will change this to use devm_spi_register_master()

Best Regards
Girish
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora
Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Mark Brown]

On Thu, May 03, 2018 at 03:34:43PM -0600, Girish Mahadevan wrote:
> This driver supports GENI based SPI Controller in the Qualcomm SOCs. The
> Qualcomm Generic Interface (GENI) is a programmable module supporting a
> wide range of serial interfaces including SPI. This driver supports SPI
> operations using FIFO mode of transfer.

This is a DT based driver but there is no binding documentation.
Binding documentation is required for any new DT stuff.

> +   depends on ARCH_QCOM || (ARM && COMPILE_TEST)

Why the ARM dependency?  There's no architecture specific headers
included...

>  obj-$(CONFIG_SPI_PXA2XX_PCI) += spi-pxa2xx-pci.o
>  obj-$(CONFIG_SPI_QUP)+= spi-qup.o
> +obj-$(CONFIG_SPI_QCOM_GENI)  += spi-geni-qcom.o
>  obj-$(CONFIG_SPI_ROCKCHIP)   += spi-rockchip.o

Please keep Kconfig and Makefile alphabetically sorted to reduce
conflicts.

> +static struct spi_master *get_spi_master(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct spi_master *spi = platform_get_drvdata(pdev);
> +
> + return spi;
> +}

This doesn't look at all driver specific with the current naming but it
actually is given that other drivers may use other driver data so it
should be renamed.  I'm also not clear why it's bouncing through the
platform device, dev_get_drvdata() exists.

> +static int spi_geni_unprepare_message(struct spi_master *spi_mas,
> + struct spi_message *spi_msg)
> +{
> + struct spi_geni_master *mas = spi_master_get_devdata(spi_mas);
> +
> + mas->cur_speed_hz = 0;
> + mas->cur_word_len = 0;
> + return 0;
> +}

Is this really useful?  If the driver needs to reconfigure for every
message then it should just do that and not care about the state.  If it
might end up caring about the state anyway that suggests there's some
kind of bug somewhere that's being masked.

> +static int spi_geni_prepare_transfer_hardware(struct spi_master *spi)
> +{
> + struct spi_geni_master *mas = spi_master_get_devdata(spi);
> + int ret = 0;
> + struct geni_se *se = &mas->se;
> +
> + ret = pm_runtime_get_sync(mas->dev);
> + if (ret < 0) {

Use auto_runtime_pm.

> + if (unlikely(!mas->setup)) {
> + int proto = geni_se_read_proto(se);

Does this really need a likely/unlikely annotation - it shouldn't be any
kind of hot path...  There's a lot of these annotations in the code.

> + ret = devm_request_irq(mas->dev, mas->irq, geni_spi_isr,
> +IRQF_TRIGGER_HIGH, "spi_geni", mas);
> + if (ret) {
> + dev_err(mas->dev, "Request_irq failed:%d: err:%d\n",

Why are we dynamically requesting the IRQ outside of probe?  Normally an
interrupt is requested on startup and held through the life of the
device.  I'm also not seeing any sign that it's freed except via devm...

> + spi->bus_num = of_alias_get_id(pdev->dev.of_node, "spi");

Don't do this, just set bus_num to -1 and let the core assign an ID.

> + spi->dev.of_node = pdev->dev.of_node;

This is broken, the virtual SPI device does not exist in DT and this
might break things.

> + pm_runtime_enable(&pdev->dev);
> + ret = spi_register_master(spi);

No devm?


signature.asc
Description: PGP signature

[PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Girish Mahadevan]

This driver supports GENI based SPI Controller in the Qualcomm SOCs. The
Qualcomm Generic Interface (GENI) is a programmable module supporting a
wide range of serial interfaces including SPI. This driver supports SPI
operations using FIFO mode of transfer.

Signed-off-by: Girish Mahadevan 
---
 drivers/spi/Kconfig   |  12 +
 drivers/spi/Makefile  |   1 +
 drivers/spi/spi-geni-qcom.c   | 766 ++
 include/linux/spi/spi-geni-qcom.h |  14 +
 4 files changed, 793 insertions(+)
 create mode 100644 drivers/spi/spi-geni-qcom.c
 create mode 100644 include/linux/spi/spi-geni-qcom.h

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 9b31351..358d60a 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -564,6 +564,18 @@ config SPI_QUP
  This driver can also be built as a module.  If so, the module
  will be called spi_qup.
 
+config SPI_QCOM_GENI
+   tristate "Qualcomm SPI controller with QUP interface"
+   depends on ARCH_QCOM || (ARM && COMPILE_TEST)
+   help
+ This driver supports GENI serial engine based SPI controller in
+ master mode on the Qualcomm Technologies Inc.'s SoCs. If you say
+ yes to this option, support will be included for the built-in SPI
+ interface on the Qualcomm Technologies Inc.'s SoCs.
+
+ This driver can also be built as a module.  If so, the module
+ will be called spi-geni-qcom.
+
 config SPI_S3C24XX
tristate "Samsung S3C24XX series SPI"
depends on ARCH_S3C24XX
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index a3ae2b7..cc90d6e 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -77,6 +77,7 @@ spi-pxa2xx-platform-objs  := spi-pxa2xx.o 
spi-pxa2xx-dma.o
 obj-$(CONFIG_SPI_PXA2XX)   += spi-pxa2xx-platform.o
 obj-$(CONFIG_SPI_PXA2XX_PCI)   += spi-pxa2xx-pci.o
 obj-$(CONFIG_SPI_QUP)  += spi-qup.o
+obj-$(CONFIG_SPI_QCOM_GENI)+= spi-geni-qcom.o
 obj-$(CONFIG_SPI_ROCKCHIP) += spi-rockchip.o
 obj-$(CONFIG_SPI_RB4XX)+= spi-rb4xx.o
 obj-$(CONFIG_SPI_RSPI) += spi-rspi.o
diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
new file mode 100644
index 000..eecc634
--- /dev/null
+++ b/drivers/spi/spi-geni-qcom.c
@@ -0,0 +1,766 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define SPI_NUM_CHIPSELECT 4
+#define SPI_XFER_TIMEOUT_MS250
+/* SPI SE specific registers */
+#define SE_SPI_CPHA0x224
+#define SE_SPI_LOOPBACK0x22c
+#define SE_SPI_CPOL0x230
+#define SE_SPI_DEMUX_OUTPUT_INV0x24c
+#define SE_SPI_DEMUX_SEL   0x250
+#define SE_SPI_TRANS_CFG   0x25c
+#define SE_SPI_WORD_LEN0x268
+#define SE_SPI_TX_TRANS_LEN0x26c
+#define SE_SPI_RX_TRANS_LEN0x270
+#define SE_SPI_PRE_POST_CMD_DLY0x274
+#define SE_SPI_DELAY_COUNTERS  0x278
+
+/* SE_SPI_CPHA register fields */
+#define CPHA   BIT(0)
+
+/* SE_SPI_LOOPBACK register fields */
+#define LOOPBACK_ENABLE0x1
+#define NORMAL_MODE0x0
+#define LOOPBACK_MSK   GENMASK(1, 0)
+
+/* SE_SPI_CPOL register fields */
+#define CPOL   BIT(2)
+
+/* SE_SPI_DEMUX_OUTPUT_INV register fields */
+#define CS_DEMUX_OUTPUT_INV_MSKGENMASK(3, 0)
+
+/* SE_SPI_DEMUX_SEL register fields */
+#define CS_DEMUX_OUTPUT_SELGENMASK(3, 0)
+
+/* SE_SPI_TX_TRANS_CFG register fields */
+#define CS_TOGGLE  BIT(0)
+
+/* SE_SPI_WORD_LEN register fields */
+#define WORD_LEN_MSK   GENMASK(9, 0)
+#define MIN_WORD_LEN   4
+
+/* SPI_TX/SPI_RX_TRANS_LEN fields */
+#define TRANS_LEN_MSK  GENMASK(23, 0)
+
+/* SE_SPI_DELAY_COUNTERS */
+#define SPI_INTER_WORDS_DELAY_MSK  GENMASK(9, 0)
+#define SPI_CS_CLK_DELAY_MSK   GENMASK(19, 10)
+#define SPI_CS_CLK_DELAY_SHFT  10
+
+/* M_CMD OP codes for SPI */
+#define SPI_TX_ONLY1
+#define SPI_RX_ONLY2
+#define SPI_FULL_DUPLEX3
+#define SPI_TX_RX  7
+#define SPI_CS_ASSERT  8
+#define SPI_CS_DEASSERT9
+#define SPI_SCK_ONLY   10
+/* M_CMD params for SPI */
+#define SPI_PRE_CMD_DELAY  BIT(0)
+#define TIMESTAMP_BEFORE   BIT(1)
+#define FRAGMENTATION  BIT(2)
+#define TIMESTAMP_AFTERBIT(3)
+#define POST_CMD_DELAY BIT(4)
+
+static irqreturn_t geni_spi_isr(int irq, void *dev);
+
+struct spi_geni_master {
+   struct geni_se se;
+   int irq;
+   struct device *dev;
+   int rx_fifo_depth;
+   int tx_fifo_depth;
+   int tx_fifo_width;
+   int tx_wm;
+   bool setup;
+   u32 cur_speed_hz;
+