[PATCH 2/2] mtd: spi-nor: add rockchip serial flash controller driver

[Shawn Lin]

Hi Marek,

On 2016/11/21 5:11, Marek Vasut wrote:
> On 11/16/2016 02:59 AM, Shawn Lin wrote:
>> Hi Marek,
>
> Hi,
>
> [...]
>
>>>> +static int rockchip_sfc_read_reg(struct spi_nor *nor, u8 opcode,
>>>> +                 u8 *buf, int len)
>>>> +{
>>>> +    struct rockchip_sfc_priv *priv = nor->priv;
>>>> +    struct rockchip_sfc *sfc = priv->sfc;
>>>> +    int ret;
>>>> +    u32 tmp;
>>>> +    u32 i;
>>>> +
>>>> +    ret = rockchip_sfc_op_reg(nor, opcode, len, SFC_CMD_DIR_RD);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    while (len > 0) {
>>>> +        tmp = readl_relaxed(sfc->regbase + SFC_DATA);
>>>> +        for (i = 0; i < len; i++)
>>>> +            *buf++ = (u8)((tmp >> (i * 8)) & 0xff);
>>>
>>> Won't this fail for len > 4 ?
>>
>> nope, this loop will reduce 4 for each successful readl. And
>> reading the remained bytes which isn't aligned to DWORD, isn't it?
>
> Try for len = 8 ... it will write 8 bytes to the buf, but half of them
> would be zero. I believe it should look like:
>
>         for (i = 0; i < 4 /* was len */; i++)
>             *buf++ = (u8)((tmp >> (i * 8)) & 0xff);
>

you're right, I was misunderstanding your comment and fixed it in V2. :)

>>>
>>> Also, you can use ioread32_rep() here, but (!) that won't work for
>>> unaligned reads, which I dunno if they can happen here, but please do
>>> double-check.
>>
>> yes, I have checked this API as well as others like memcpy_{to,from}io
>> , etc. They will generate a external abort for arm core as the unaligned
>> (DWORD) read/write via AHB aren't supported by Rockchip Socs. So I have
>> to open code these stuff. This could be easily found for other
>> upstreamed rockchip drivers. :)
>
> This is normal, but you can still use the _rep variant if you handle the
> corner cases.
>

Sure, I will keep improving it once more comment for my v2 sent last
friday there. :)

>>>
>>>> +        len = len - 4;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>
> [...]
>
>>>> +static ssize_t rockchip_sfc_write(struct spi_nor *nor, loff_t to,
>>>> +                  size_t len, const u_char *write_buf)
>>>> +{
>>>> +    struct rockchip_sfc_priv *priv = nor->priv;
>>>> +    struct rockchip_sfc *sfc = priv->sfc;
>>>> +    size_t offset;
>>>> +    int ret;
>>>> +    dma_addr_t dma_addr = 0;
>>>> +
>>>> +    if (!sfc->use_dma)
>>>> +        goto no_dma;
>>>
>>> Seems like there's a lot of similarity between read/write .
>>
>> I was thinking to combine read/write with a extra argument to
>> indicate WR/RD. But as we could see still some differece between
>> WR and RD and there are already some condiction checks. So it
>> will make the code hard to read with stuffing lots of condition
>> checks. So I splited out read and write strightforward. :)
>
> Hrm, is it that bad ?
>
>>>> +    for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
>>>> +        size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
>>>> +
>>>> +        dma_addr = dma_map_single(NULL, (void *)write_buf,
>>>> +                      trans, DMA_TO_DEVICE);
>>>> +        if (dma_mapping_error(sfc->dev, dma_addr)) {
>>>> +            dma_addr = 0;
>>>> +            memcpy(sfc->buffer, write_buf + offset, trans);
>>>> +        }
>>>> +
>>>> +        /* Fail to map dma, use pre-allocated area instead */
>>>> +        ret = rockchip_sfc_dma_transfer(nor, to + offset,
>>>> +                        dma_addr ? dma_addr :
>>>> +                        sfc->dma_buffer,
>>>> +                        trans, SFC_CMD_DIR_WR);
>>>> +        if (dma_addr)
>>>> +            dma_unmap_single(NULL, dma_addr,
>>>> +                     trans, DMA_TO_DEVICE);
>>>> +        if (ret) {
>>>> +            dev_warn(nor->dev, "DMA write timeout\n");
>>>> +            return ret;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return len;
>>>> +no_dma:
>>>> +    ret = rockchip_sfc_pio_transfer(nor, to, len,
>>>> +                    (u_char *)write_buf, SFC_CMD_DIR_WR);
>>>> +    if (ret) {
>>>> +        dev_warn(nor->dev, "PIO write timeout\n");
>>>> +        return ret;
>>>> +    }
>>>> +    return len;
>>>> +}
>
> [...]
>
>>>> +static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < sfc->num_chip; i++)
>>>> +        mtd_device_unregister(&sfc->nor[i]->mtd);
>>>> +}
>>>> +
>>>> +static int rockchip_sfc_register_all(struct rockchip_sfc *sfc)
>>>> +{
>>>> +    struct device *dev = sfc->dev;
>>>> +    struct device_node *np;
>>>> +    int ret;
>>>> +
>>>> +    for_each_available_child_of_node(dev->of_node, np) {
>>>> +        ret = rockchip_sfc_register(np, sfc);
>>>> +        if (ret)
>>>> +            goto fail;
>>>> +
>>>> +        if (sfc->num_chip == SFC_MAX_CHIP_NUM) {
>>>> +            dev_warn(dev, "Exceeds the max cs limitation\n");
>>>> +            break;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +
>>>> +fail:
>>>> +    dev_err(dev, "Failed to register all chip\n");
>>>> +    rockchip_sfc_unregister_all(sfc);
>>>
>>> See cadence qspi where we only unregister the registered flashes.
>>> Implement it the same way here.
>>>
>>
>> yup, but I'm afraid that rockchip_sfc_unregister_all confused you
>> as it actually unregisters the registered ones, not for all.
>>
>> static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
>> {
>>         int i;
>>
>>         for (i = 0; i < sfc->num_chip; i++)
>>                 mtd_device_unregister(&sfc->nor[i]->mtd);
>> }
>>
>> sfc->num_chip stands for how many flashes registered successfully.
>
> Does it work if you have a hole in there ? Like if you have a flash on
> chipselect 0 and chipselect 2 ?

Yes it does, as it won't leave a room for chipselect 1 whose node isn't
present, which means there isn't a hole in there at all. :)

>
>>>> +    return ret;
>>>> +}
>
> [...]
>
>


-- 
Best Regards
Shawn Lin