| Message ID | 1413372349-4254-1-git-send-email-addy.ke@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 15, 2014 at 07:25:49PM +0800, Addy Ke wrote: > + if (WARN_ON(rs->speed > MAX_SCLK_OUT)) > + rs->speed = MAX_SCLK_OUT; > + > + /* the minimum divsor is 2 */ > + if (rs->max_freq < 2 * rs->speed) { > + clk_set_rate(rs->spiclk, 2 * rs->speed); > + rs->max_freq = clk_get_rate(rs->spiclk); > + } I'll apply this but you should be checking the return code from clk_set_rate() here, please send a followup patch doing that. It might also be worth consdering just setting the rate unconditionally here, it seems like it should make things simpler.
hi, Mark On 2014/10/15 21:04, Mark Brown wrote: > On Wed, Oct 15, 2014 at 07:25:49PM +0800, Addy Ke wrote: > >> + if (WARN_ON(rs->speed > MAX_SCLK_OUT)) >> + rs->speed = MAX_SCLK_OUT; >> + >> + /* the minimum divsor is 2 */ >> + if (rs->max_freq < 2 * rs->speed) { >> + clk_set_rate(rs->spiclk, 2 * rs->speed); >> + rs->max_freq = clk_get_rate(rs->spiclk); >> + } > > I'll apply this but you should be checking the return code from > clk_set_rate() here, please send a followup patch doing that. It might If clk_set_rate return error, do I only put dev_warn here or return error value to spi core? > also be worth consdering just setting the rate unconditionally here, it > seems like it should make things simpler. > I think we need. If we set the rate unconditionally here, clk_set_rate() will be executed in each spi transfer.
On Thu, Oct 16, 2014 at 05:16:02PM +0800, addy ke wrote: > On 2014/10/15 21:04, Mark Brown wrote: > > On Wed, Oct 15, 2014 at 07:25:49PM +0800, Addy Ke wrote: > >> + if (WARN_ON(rs->speed > MAX_SCLK_OUT)) > >> + rs->speed = MAX_SCLK_OUT; > >> + /* the minimum divsor is 2 */ > >> + if (rs->max_freq < 2 * rs->speed) { > >> + clk_set_rate(rs->spiclk, 2 * rs->speed); > >> + rs->max_freq = clk_get_rate(rs->spiclk); > >> + } > > I'll apply this but you should be checking the return code from > > clk_set_rate() here, please send a followup patch doing that. It might > If clk_set_rate return error, do I only put dev_warn here or return error value to spi core? It'd be better to return an error if we need to set the rate and can't do it. > > also be worth consdering just setting the rate unconditionally here, it > > seems like it should make things simpler. > I think we need. > If we set the rate unconditionally here, clk_set_rate() will be executed in each spi transfer. Is that really such a high cost?
On 2014/10/16 17:34, Mark Brown wrote: > On Thu, Oct 16, 2014 at 05:16:02PM +0800, addy ke wrote: >> On 2014/10/15 21:04, Mark Brown wrote: >>> On Wed, Oct 15, 2014 at 07:25:49PM +0800, Addy Ke wrote: > >>>> + if (WARN_ON(rs->speed > MAX_SCLK_OUT)) >>>> + rs->speed = MAX_SCLK_OUT; > >>>> + /* the minimum divsor is 2 */ >>>> + if (rs->max_freq < 2 * rs->speed) { >>>> + clk_set_rate(rs->spiclk, 2 * rs->speed); >>>> + rs->max_freq = clk_get_rate(rs->spiclk); >>>> + } > >>> I'll apply this but you should be checking the return code from >>> clk_set_rate() here, please send a followup patch doing that. It might > >> If clk_set_rate return error, do I only put dev_warn here or return error value to spi core? > > It'd be better to return an error if we need to set the rate and can't > do it. > >>> also be worth consdering just setting the rate unconditionally here, it >>> seems like it should make things simpler. > >> I think we need. >> If we set the rate unconditionally here, clk_set_rate() will be executed in each spi transfer. > > Is that really such a high cost? > Not high cost, but I think if the default spi_clk is enough, we do not need to set spi_clk again. Maybe we can only set spi_clk as (2 * MAX_SCLK_OUT) in probe().
On Thu, Oct 16, 2014 at 05:58:51PM +0800, addy ke wrote: Please fix your mailer to word wrap within paragraphs. > On 2014/10/16 17:34, Mark Brown wrote: > >> If we set the rate unconditionally here, clk_set_rate() will be executed in each spi transfer. > > Is that really such a high cost? > Not high cost, but I think if the default spi_clk is enough, we do not need to set spi_clk again. > Maybe we can only set spi_clk as (2 * MAX_SCLK_OUT) in probe(). It's not too bad to do it where it is, it just seems better to be simpler.
diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c index f96ea8a..3044c6c 100644 --- a/drivers/spi/spi-rockchip.c +++ b/drivers/spi/spi-rockchip.c @@ -145,6 +145,9 @@ #define RXBUSY (1 << 0) #define TXBUSY (1 << 1) +/* sclk_out: spi master internal logic in rk3x can support 50Mhz */ +#define MAX_SCLK_OUT 50000000 + enum rockchip_ssi_type { SSI_MOTO_SPI = 0, SSI_TI_SSP, @@ -496,6 +499,15 @@ static void rockchip_spi_config(struct rockchip_spi *rs) dmacr |= RF_DMA_EN; } + if (WARN_ON(rs->speed > MAX_SCLK_OUT)) + rs->speed = MAX_SCLK_OUT; + + /* the minimum divsor is 2 */ + if (rs->max_freq < 2 * rs->speed) { + clk_set_rate(rs->spiclk, 2 * rs->speed); + rs->max_freq = clk_get_rate(rs->spiclk); + } + /* div doesn't support odd number */ div = max_t(u32, rs->max_freq / rs->speed, 1); div = (div + 1) & 0xfffe;
Because the minimum divisor in rk3x's spi controller is 2, if spi_clk is less than 2 * sclk_out, we can't get the right divisor. So we must set spi_clk again to match slave request. Signed-off-by: Addy Ke <addy.ke@rock-chips.com> --- drivers/spi/spi-rockchip.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)