Re: [PATCH 1/2] spi: Add Renesas R-Car RPC SPI controller driver

[Boris Brezillon]

On Tue, 20 Nov 2018 14:09:05 +0100
Marek Vasut <marek.vasut@xxxxxxxxx> wrote:

> On 11/20/2018 08:23 AM, masonccyang@xxxxxxxxxxx wrote:
> > Hi Marek,  
> 
> Hi,
> 
> >> Marek Vasut <marek.vasut@xxxxxxxxx>
> >> 2018/11/19 äå 10:12
> >>
> >> To
> >>  
> >> > +
> >> > +static int rpc_spi_set_freq(struct rpc_spi *rpc, unsigned long freq)
> >> > +{
> >> > + Â int ret;
> >> > +
> >> > + Â if (rpc->cur_speed_hz == freq)
> >> > + Â Â Âreturn 0;
> >> > +
> >> > + Â clk_disable_unprepare(rpc->clk_rpc);
> >> > + Â ret = clk_set_rate(rpc->clk_rpc, freq);
> >> > + Â if (ret)
> >> > + Â Â Âreturn ret;
> >> > +
> >> > + Â ret = clk_prepare_enable(rpc->clk_rpc);
> >> > + Â if (ret)
> >> > + Â Â Âreturn ret;  
> >>
> >> Is this clock disable/update/enable really needed ? I'd think that
> >> clk_set_rate() would handle the rate update correctly.  
> > 
> > This is for run time PM mechanism in spi-mem layer and __spi_sync(),
> > you may refer to another patch [1].
> > 
> > [1]
> > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git/commit/?h=for-4.21&id=b942d80b0a394e8ea18fce3b032b4700439e8ca3  
> 
> I think Geert commented on the clock topic, so let's move it there.
> Disabling and enabling clock to change their rate looks real odd to me.

Look at the CLK_SET_RATE_GATE definition and its users and you'll see
it's not unusual to have such constraints on clks. Maybe your HW does
not have such constraints, but it's not particularly odd to do that
(though it could probably be automated by the clk framework somehow).