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

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

Hello!

On 12/11/2018 12:26 PM, masonccyang@mxic.com.tw wrote:

[...]
>>    I'd already started the v2 driver review before you posted v3, so
>> here goes...
>>
>> On 12/03/2018 12:18 PM, Mason Yang wrote:
>>
>>> Add a driver for Renesas R-Car Gen3 RPC SPI controller.
>>>
>>> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
>> [...]
>>> diff --git a/drivers/spi/spi-renesas-rpc.c b/drivers/spi/spi-renesas-rpc.c
>>> new file mode 100644
>>> index 0000000..ac9094e
>>> --- /dev/null
>>> +++ b/drivers/spi/spi-renesas-rpc.c
>>> @@ -0,0 +1,808 @@
>> [...]
>>> +#define RPC_DRCR      0x000C   /* R/W */
>>> +#define RPC_DRCR_SSLN      BIT(24)
>>> +#define RPC_DRCR_RBURST(v)   (((v) & 0x1F) << 16)
>>
>>    More like ((v) - 1), like in another register...

   I mean you can pass the read data burst length as a # of data units,
thus just substracting 1, like you did in the other case...

>> [...]
>>> +#define RPC_DREAR      0x0014   /* R/W */
>>> +#define RPC_DREAR_EAC      BIT(0)
>>
>>    The manual says it takes bits 0 to 2...
> 
> yup, EAC[2:0]
> but as datasheet description, either 0 or 1 is OK to BIT(0),
> other than above setting is prohibited

   I'd prefer that this matches the manual. #define the values or
just pass them to RPC_DREAR_EAC(v).

>> [...]
>>> +#define RPC_SMADR      0x0028   /* R/W */
>>> +#define RPC_SMOPR      0x002C   /* R/W */
>>> +#define RPC_SMOPR_OPD0(o)   (((o) & 0xFF) << 0)
>>> +#define RPC_SMOPR_OPD1(o)   (((o) & 0xFF) << 8)
>>> +#define RPC_SMOPR_OPD2(o)   (((o) & 0xFF) << 16)
>>> +#define RPC_SMOPR_OPD3(o)   (((o) & 0xFF) << 24)
>>
>>   You either go in descending or ascending order, not both. :-)
> 
> I can't get your point.

   You #define in the ascending order of the bit # (shift count) here and
in the descending order elsewhere.

[...]
>>> +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.
>>> +    */
>>> +   regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL |
>>> +              RPC_PHYCNT_STRTIM(0x6) | 0x260);
>>> +
>>> +   /*
>>> +    * NOTE: The 0x31511144 and 0x431 are undocumented bits,
>>> +    *    but they must be set for RPC_PHYOFFSET1 & RPC_PHYOFFSET2.
>>> +    */
>>> +   regmap_write(rpc->regmap, RPC_PHYOFFSET1, 0x31511144);
>>> +   regmap_write(rpc->regmap, RPC_PHYOFFSET2, 0x431);
>>
>>    0x400 is actually documented, bits 0..7 are read only and defaultto 0x31...

> I got these values from R-Car bare-metal code, mini-Monitor v4.01.
> 
> What should I describe these bits 0x400 and 0x31 if it is needed?

#define PHYOFFSET2_OCTTMG(v)	((v) & 0x7) << 8)

   The read-modify-write operation ios preferred in this casa, so that
0x31 doesn't appear anywhere.

[...]
>>> +
>>> +         if (nbytes > 4) {
>>> +            nbytes = 4;
>>> +            smcr = rpc->smcr |
>>> +                   RPC_SMCR_SPIE | RPC_SMCR_SSLKP;
>>> +         } else {
>>> +            smcr = rpc->smcr | RPC_SMCR_SPIE;
>>> +         }
>>
>>    Please do this repeated part outside *if*:
> 
> ?
> The procedure is different !

   Where?!

>>          smcr = rpc->smcr | RPC_SMCR_SPIE;
>>          if (nbytes > 4) {
>>             nbytes = 4;
>>             smcr |= RPC_SMCR_SSLKP;
>>          }
[...]
>>> +
>>> +         if (nbytes > 4)
>>> +            nbytes = 4;
>>> +
>>> +         regmap_write(rpc->regmap, RPC_SMENR, rpc->smenr);
>>> +         regmap_write(rpc->regmap, RPC_SMCR,
>>> +                 rpc->smcr | RPC_SMCR_SPIE);
>>> +         ret = wait_msg_xfer_end(rpc);
>>> +         if (ret)
>>> +            goto out;
>>> +
>>> +         regmap_read(rpc->regmap, RPC_SMRDR0, &data);
>>> +         memcpy_fromio(rx_buf + pos, (void *)&data, nbytes);
>>
>>    What?! The 'data' variable is not in a MMIO region, you can use
>> plain memcpy().
>> Not sure about the endianness tho...
> 
> yup, the 'data' variable is not in MMIO region!
> patch it to memcpy() rather than memcpy_fromio().

   Also, pointer casts to 'void *' are automatic...

[...]
>>> +static ssize_t rpc_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
>>> +                   u64 offs, size_t len, void *buf)
>>> +{
>>> +   struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
>>> +   int ret;
>>> +
>>> +   if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
>>> +      return -EINVAL;
>>> +
>>> +   ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
>>> +   if (ret)
>>> +      return ret;
>>> +
>>> +   rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
>>> +                &desc->info.op_tmpl, &offs, &len);
>>> +
>>> +   regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_SFDE |
>>> +           RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
>>> +           RPC_CMNCR_BSZ(0));
>>> +   regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RBURST(0x1f) |
>>
>>    RPC_DRCR_RBURST(32), please.
> 
> ?
> the max value is 31 = 0x1f

   See above!

[...]
>>> +   regmap_write(rpc->regmap, RPC_DRDMCR, rpc->dummy);
>>> +   regmap_write(rpc->regmap, RPC_DRDRENR, 0);
>>> +
>>> +   memcpy_fromio(buf, rpc->linear.map + desc->info.offset + offs, len);
>>
>>    What if the read direct-mapped area is less than U32_MAX in size
>> (and it will be,
>> according to your bindings)?
> 
> the max direct mapping read area is 64 KByte description in DTS.

   You don't check for this before reading (but you do for writing)!

[...]
>>> +static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
>>> +               u64 offs, size_t len, const void *buf)
>>> +{
>>> +   struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
>>> +   int ret;
>>> +
>>> +   if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
>>> +      return -EINVAL;
>>> +
>>> +   if (WARN_ON(len > RPC_WBUF_SIZE))
>>> +      return -EIO;
>>
>>    Why not write 256 bytes and return w/that?
> 
> in manual 62.3.13 Write Buffer Operation,
> transfer size to device is 256-byte unit.

   Why not write 256 bytes max and just return 256? 

[...]
>> [...]

>>> +static void rpc_spi_transfer_setup(struct rpc_spi *rpc,
>>> +               struct spi_message *msg)
>>> +{
>>> +   struct spi_transfer *t, xfer[4] = { };
>>> +   u32 i, xfercnt, xferpos = 0;
>>> +
>>> +   rpc->totalxferlen = 0;
>>> +   rpc->xfer_dir = SPI_MEM_NO_DATA;
>>> +
>>> +   list_for_each_entry(t, &msg->transfers, transfer_list) {
>>> +      if (t->tx_buf) {
>>> +         xfer[xferpos].tx_buf = t->tx_buf;
>>> +         xfer[xferpos].tx_nbits = t->tx_nbits;
>>> +      }
>>> +
>>> +      if (t->rx_buf) {
>>> +         xfer[xferpos].rx_buf = t->rx_buf;
>>> +         xfer[xferpos].rx_nbits = t->rx_nbits;
>>> +      }
>>> +
>>> +      if (t->len) {
>>> +         xfer[xferpos++].len = t->len;
>>> +         rpc->totalxferlen += t->len;
>>> +      }
>>> +
>>> +      if (list_is_last(&t->transfer_list, &msg->transfers)) {
>>> +         if (xferpos > 1 && t->rx_buf) {
>>> +            rpc->xfer_dir = SPI_MEM_DATA_IN;
>>> +            rpc->smcr = RPC_SMCR_SPIRE;
>>> +         } else if (xferpos > 1 && t->tx_buf) {
>>
>>    Why check 'xferpos' twice?
>>
>>> +            rpc->xfer_dir = SPI_MEM_DATA_OUT;
>>> +            rpc->smcr = RPC_SMCR_SPIWE;
>>> +         }
>>> +      }
>>> +   }
> 
> patch it to check 'xferpos' only one time.
> -------------------------------------------------------------
>                  if (list_is_last(&t->transfer_list, &msg->transfers)) {
>                          if (xferpos > 1) {
>                                         if (tx->rx_buf) {                                                
>                                  rpc->xfer_dir = SPI_MEM_DATA_IN;
>                                  rpc->smcr = RPC_SMCR_SPIRE;
>                                  } else if (t->tx_buf) {
>                                  rpc->xfer_dir = SPI_MEM_DATA_OUT;
>                                  rpc->smcr = RPC_SMCR_SPIWE;
>                                  }
>                                 }
>                  }
> ----------------------------------------------------------

   You got the idea but please reformat this properly..

[...]
>>
>>> +      for (i = 0; i < xfer[1].len; i++)
>>> +         rpc->addr |= (u32)((u8 *)xfer[1].tx_buf)[i]
>>> +               << (8 * (xfer[1].len - i - 1));
>>
>>    Ugh, you need get_unaligned_*()...
> 
> for accessing a single byte quantity, ((u8 *)xfer[1].tx_buf)[i] ?

   Ugh, never start a new line with an operator, lease it on a 1st, broken up line.

[...]
>>> +static const struct regmap_config rpc_spi_regmap_config = {
>>> +   .reg_bits = 32,
>>> +   .val_bits = 32,
>>> +   .reg_stride = 4,
>>> +   .fast_io = true,
>>> +   .max_register = RPC_WBUF + RPC_WBUF_SIZE,
>>
>>    Ugh... 0x8100/4 regs, of which the majority isn't used... :-/

   Seriously, you don't use regmap for the write buffer anyway...

[...]
>> > +err_put_master:
>> > +   spi_master_put(master);
>> > +   pm_runtime_disable(&pdev->dev);
>> > +
>> > +   return ret;
>> > +}
>> > +
>> > +static int rpc_spi_remove(struct platform_device *pdev)
>> > +{
>> > +   struct spi_master *master = platform_get_drvdata(pdev);
>> > +
>> > +   pm_runtime_disable(&pdev->dev);
>> > +   spi_unregister_master(master);
>>
>>    No spi_master_put() here?
> 
> put_device() in spi_unregister_master().

   Why call spi_master_put() in the probe() method's error path?

> best regards,
> Mason

> CONFIDENTIALITY NOTE:
> 
> This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.
> 
> Macronix International Co., Ltd.
> 
> =====================================================================

   Please consider sending patches from e.g. your GMail account to avoid this legelese
crap.

MBR, Sergei