lkml.org 
[lkml]   [2018]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/2] spi: Add Renesas R-Car Gen3 RPC SPI controller driver
From
Date
On 12/05/2018 02:15 PM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

> On Wed, Dec 5, 2018 at 1:57 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 12/05/2018 08:44 AM, masonccyang@mxic.com.tw wrote:
>>>> "Marek Vasut" <marek.vasut@gmail.com>
>>>> 2018/12/05 上午 10:04
>>>> On 12/03/2018 10:18 AM, Mason Yang wrote:
>>>>> Add a driver for Renesas R-Car Gen3 RPC SPI controller.
>>>>>
>>>>> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
>
>>>>> +static void rpc_spi_hw_init(struct rpc_spi *rpc)
>>>>> +{
>>>>> + /*
>>>>> + * NOTE: The 0x260 are undocumented bits, but they must be set.
>>>>> + * RPC_PHYCNT_STRTIM is strobe timing adjustment bit,
>>>>> + * 0x0 : the delay is biggest,
>>>>> + * 0x1 : the delay is 2nd biggest,
>>>>> + * 0x3 or 0x6 is a recommended value.
>>>>> + */
>>>>
>>>> Doesn't this vary by SoC ? I think H3 ES1.0 had different value here,
>>>> but I might be wrong.
>>>
>>> I check the Renesas bare-metal code, mini_monitor v4.01.
>>> It set 0x03 or 0x0 and I test them w/ 0x0, 0x3 and 0x6 are all OK.
>>
>> Shouldn't this somehow use the soc_device_match() then and configure it
>> accordingly for various chips ? Like eg. the r8a7795-cpg-mssr driver does.
>
> Please don't use soc_device_match() for per-SoC configuration, if
> you already have of_device_id.data.

I mean, the value is different on H3 ES1 and ES2 iirc, that's what
soc_device_match() is for, right ?

> BTW, this drivers support r8a7795 only (for now), as per the compatible
> value.

77995

>>>>> +#ifdef CONFIG_RESET_CONTROLLER
>>>>
>>>> Just make the driver depend on reset controller.
>>>
>>> ?
>>> please refer to
>>> https://github.com/torvalds/linux/blob/master/drivers/clk/renesas/renesas-cpg-mssr.c
>>>
>>> line 124 ~ 126
>>
>> This seems like a stopgap measure for systems which do not have a reset
>> api compatible controller. Geert ?
>
> So far CONFIG_RESET_CONTROLLER is optional.

My understanding is that for this IP, it can well be mandatory, since
all the chips have a reset wired to the IP internally.

>>>>> +static int rpc_spi_io_xfer(struct rpc_spi *rpc,
>>>>> + const void *tx_buf, void *rx_buf)
>>>>> +{
>>>>> + u32 smenr, smcr, data, pos = 0;
>>>>> + int ret = 0;
>>>>> +
>>>>> + regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
>>>>> + RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
>>>>> + RPC_CMNCR_BSZ(0));
>>>>> + regmap_write(rpc->regmap, RPC_SMDRENR, 0x0);
>>>>> + regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
>>>>> + regmap_write(rpc->regmap, RPC_SMDMCR, rpc->dummy);
>>>>> + regmap_write(rpc->regmap, RPC_SMADR, rpc->addr);
>>>>> +
>>>>> + if (tx_buf) {
>>>>> + smenr = rpc->smenr;
>>>>> +
>>>>> + while (pos < rpc->xferlen) {
>>>>> + u32 nbytes = rpc->xferlen - pos;
>>>>> +
>>>>> + regmap_write(rpc->regmap, RPC_SMWDR0,
>>>>> + *(u32 *)(tx_buf + pos));
>>>>
>>>> *(u32 *) cast is probably not needed , fix casts globally.
>>>
>>> It must have it!
>>
>> Why ?
>
> Else you get a compiler warning, as tx_bug is void *.

Don't you need some get_unaligned() in that case ? txbuf+pos can well be
unaligned if it's void * .

>>>>> +#ifdef CONFIG_PM_SLEEP
>>>>> +static int rpc_spi_suspend(struct device *dev)
>>>>> +{
>>>>> + struct platform_device *pdev = to_platform_device(dev);
>>>>> + struct spi_master *master = platform_get_drvdata(pdev);
>>>>> +
>>>>> + return spi_master_suspend(master);
>>>>
>>>> Won't the SPI NOR lose state across suspend ? Is that a problem ?
>>>
>>> I don't think so.
>>> Because when the device is not in operation and CS# is high,
>>> it is put in stand-by mode.
>>
>> Is the power to the SPI NOR retained ?
>
> Not if PSCI system suspend turns of power to the SoC.

And is that a problem ?

--
Best regards,
Marek Vasut

\
 
 \ /
  Last update: 2018-12-05 14:25    [W:0.138 / U:1.168 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site