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

[Marek Vasut]

On 12/06/2018 06:56 AM, masonccyang@xxxxxxxxxxx wrote:
> Hi Geert,
> 
>> "Geert Uytterhoeven" <geert@xxxxxxxxxxxxxx>
>> 2018/12/05 äå 05:06
>>
>> To
>>
>> masonccyang@xxxxxxxxxxx,
>>
>> cc
>>
>> "Mark Brown" <broonie@xxxxxxxxxx>, "Marek Vasut"
>> <marek.vasut@xxxxxxxxx>, "Linux Kernel Mailing List" <linux-
>> kernel@xxxxxxxxxxxxxxx>, "linux-spi" <linux-spi@xxxxxxxxxxxxxxx>,
>> "Boris Brezillon" <boris.brezillon@xxxxxxxxxxx>, "Linux-Renesas"
>> <linux-renesas-soc@xxxxxxxxxxxxxxx>, "Geert Uytterhoeven" <geert
>> +renesas@xxxxxxxxx>, juliensu@xxxxxxxxxxx, "Simon Horman"
>> <horms@xxxxxxxxxxxx>, zhengxunli@xxxxxxxxxxx
>>
>> Subject
>>
>> Re: [PATCH v2 1/2] spi: Add Renesas R-Car Gen3 RPC SPI controller driver
>>
>> Hi Mason,
>>
>> On Mon, Dec 3, 2018 at 10:19 AM Mason Yang <masonccyang@xxxxxxxxxxx>
> wrote:
>> > Add a driver for Renesas R-Car Gen3 RPC SPI controller.
>> >
>> > Signed-off-by: Mason Yang <masonccyang@xxxxxxxxxxx>
>>
>> Thanks for your patch!
>>
>> > --- a/drivers/spi/Kconfig
>> > +++ b/drivers/spi/Kconfig
>> > @@ -528,6 +528,12 @@ config SPI_RSPI
>> > Â Â Â Â help
>> > Â Â Â Â Â SPI driver for Renesas RSPI and QSPI blocks.
>> >
>> > +config SPI_RENESAS_RPC
>> > + Â Â Â tristate "Renesas R-Car Gen3 RPC SPI controller"
>> > + Â Â Â depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
>>
>> So this driver is intended for SuperH SoCs, too?
>> If not, please drop the dependency.
>>
> 
> okay, I will drop "SUPERH".
> 
>> > --- /dev/null
>> > +++ b/drivers/spi/spi-renesas-rpc.c
>>
>> > +#ifdef CONFIG_RESET_CONTROLLER
>> > +static int rpc_spi_do_reset(struct rpc_spi *rpc)
>>
>> What's the purpose of the reset routine?
> 
> in case RPC xfer is time-out due to something wrong in RPC module,
> as Marek comments.
> 
>> Given the #ifdef, is it optional or required?
>>
>> > +{
>> > + Â Â Â int i, ret;
>> > +
>> > + Â Â Â ret = reset_control_reset(rpc->rstc);
>> > + Â Â Â if (ret)
>> > + Â Â Â Â Â Â Â return ret;
>> > +
>> > + Â Â Â for (i = 0; i < LOOP_TIMEOUT; i++) {
>> > + Â Â Â Â Â Â Â ret = reset_control_status(rpc->rstc);
>> > + Â Â Â Â Â Â Â if (ret == 0)
>> > + Â Â Â Â Â Â Â Â Â Â Â return 0;
>> > + Â Â Â Â Â Â Â usleep_range(0, 1);
>> > + Â Â Â }
>>
>> Why do you need this loop?
>> The delay in cpg_mssr_reset() should be sufficient.
>>
> 
> yup, I know there is already 35 us delay in cpg_mssr_reset().
> If you think reset_control_status()checking is not necessary,
> I will drop it.
> 
>> > +
>> > + Â Â Â return -ETIMEDOUT;
>> > +}
>> > +#else
>> > +static int rpc_spi_do_reset(struct rpc_spi *rpc)
>> > +{
>> > + Â Â Â return -ETIMEDOUT;
>> > +}
>> > +#endif
>>
>> > +static int rpc_spi_transfer_one_message(struct spi_master *master,
>> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct spi_message *msg)
>> > +{
>> > + Â Â Â struct rpc_spi *rpc = spi_master_get_devdata(master);
>> > + Â Â Â struct spi_transfer *t;
>> > + Â Â Â int ret;
>> > +
>> > + Â Â Â rpc_spi_transfer_setup(rpc, msg);
>> > +
>> > + Â Â Â list_for_each_entry(t, &msg->transfers, transfer_list) {
>> > + Â Â Â Â Â Â Â if (!list_is_last(&t->transfer_list, &msg->transfers))
>> > + Â Â Â Â Â Â Â Â Â Â Â continue;
>> > + Â Â Â Â Â Â Â ret = rpc_spi_xfer_message(rpc, t);
>>
>> rpc_spi_xfer_message() sounds like a bad name to me, given it operates
>> on a transfer, not on a message.
>>
> 
> Because RPC send a entire SPI message at one time, not separately,
> that is the 1'st transfer is for command, the 2'nd transfer is for
> address/data
> and so on.
> The reason is CS# pin control restriction in RPC HW module.
> 
> 
>> > + Â Â Â Â Â Â Â if (ret)
>> > + Â Â Â Â Â Â Â Â Â Â Â goto out;
>> > + Â Â Â }
>> > +
>> > + Â Â Â msg->status = 0;
>> > + Â Â Â msg->actual_length = rpc->totalxferlen;
>> > +out:
>> > + Â Â Â spi_finalize_current_message(master);
>> > + Â Â Â return 0;
>> > +}
>>
>>
>> > +static int rpc_spi_probe(struct platform_device *pdev)
>> > +{
>>
>> > + Â Â Â rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
>> > + Â Â Â if (IS_ERR(rpc->rstc))
>> > + Â Â Â Â Â Â Â return PTR_ERR(rpc->rstc);
>>
>> This will return an error if CONFIG_RESET_CONTROLLER is not set, hence
>> the #ifdef above is moot.
>>
> 
> You are right.
> so, I should do
> Option 1: remove #CONFIG_RESET_CONTROLLER
> Option 2: add #CONFIG_RESET_CONTROLLER for
> devm_reset_control_get_exclusive()
> 
> please comments on it, thanks.
> 
> 
>> > +
>> > + Â Â Â pm_runtime_enable(&pdev->dev);
>> > + Â Â Â master->auto_runtime_pm = true;
>> > +
>> > + Â Â Â master->num_chipselect = 1;
>> > + Â Â Â master->mem_ops = &rpc_spi_mem_ops;
>> > + Â Â Â master->transfer_one_message = rpc_spi_transfer_one_message;
>>
>> Is there any reason you cannot use the standard
>> spi_transfer_one_message, i.e. provide spi_controller.transfer_one()
>> instead of spi_controller.transfer_one_message()?
>>
> 
> It seems there is a RPC HW restriction in CS# pin control.
> Therefore, it can't send the 1'st spi-transfer for command and then
> keeping CS# pin goes low for the 2'nd spi-transfer for address/data and
> so on.

Isn't register DRCR bit SSLN/SSLE exactly for this purpose ?

-- 
Best regards,
Marek Vasut