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
