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,  <dkota@xxxxxxxxxxxxxx> 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.