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

[Marek Vasut]

On 11/30/2016 02:17 AM, Shawn Lin wrote:
[...]
>>> +static inline int rockchip_sfc_pio_read(struct rockchip_sfc *sfc,
>>> u_char *buf,
>>> +                    size_t len)
>>> +{
>>> +    u32 dwords, rx_wl, count, i, tmp;
>>> +    unsigned long timeout;
>>> +    int ret = 0;
>>> +    u32 *tbuf = (u32 *)buf;
>>> +    u_char *tbuf2;
>>> +
>>> +    /*
>>> +     * Align bytes to dwords, and get the remained bytes.
>>> +     * We should always round down to DWORD as the ahb for
>>> +     * Rockchip Socs won't support non-aligned-to-DWORD transfer.
>>> +     * So please don't use any APIs that will finally use non-aligned
>>> +     * read, for instance, memcpy_fromio or ioread32_rep etc.
>>> +     */
>>> +    dwords = len >> 2;
>>> +    len = len & 0x3;
>>
>> Won't this overwrite some bits past the $buf if you write more than $len
>> bytes into this memory location ?
>>
> 
> I can't see the possibility here to overwrite $buf, no?

Looking at the code again, I believe not, although it's quite
non-trivial piece of code.

>>> +    while (dwords) {
>>> +        rx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
>>> +             SFC_FSR_RX_WATER_LVL_SHIFT) &
>>> +             SFC_FSR_RX_WATER_LVL_MASK;
>>> +
>>> +        if (rx_wl > 0) {
>>> +            count = min_t(u32, dwords, rx_wl);
>>> +            for (i = 0; i < count; i++) {
>>> +                *tbuf++ = readl_relaxed(sfc->regbase +
>>> +                            SFC_DATA);
>>> +                dwords--;
>>> +            }
>>> +
>>> +            if (dwords == 0)
>>> +                break;
>>> +            timeout = 0;
>>> +        } else {
>>> +            mdelay(1);
>>> +            if (timeout++ > SFC_MAX_IDLE_RETRY) {
>>> +                ret = -ETIMEDOUT;
>>> +                break;
>>> +            }
>>> +        }
>>> +    }
>>> +
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    /* Read the remained bytes */
>>> +    timeout = 0;
>>> +    tbuf2 = (u_char *)tbuf;
>>> +    while (len) {
>>> +        rx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
>>> +             SFC_FSR_RX_WATER_LVL_SHIFT) &
>>> +             SFC_FSR_RX_WATER_LVL_MASK;
>>> +        if (rx_wl > 0) {
>>> +            tmp = readl_relaxed(sfc->regbase + SFC_DATA);
>>> +            for (i = 0; i < len; i++)
>>> +                tbuf2[i] = (u8)((tmp >> (i * 8)) & 0xff);
>>> +            goto done;
>>> +        } else {
>>> +            mdelay(1);
>>> +            if (timeout++ > SFC_MAX_IDLE_RETRY) {
>>> +                ret = -ETIMEDOUT;
>>> +                break;
>>> +            }
>>> +        }
>>> +    }

[...]

>>> +static int rockchip_sfc_probe(struct platform_device *pdev)
>>> +{
>>> +    struct device *dev = &pdev->dev;
>>> +    struct resource *res;
>>> +    struct rockchip_sfc *sfc;
>>> +    int ret;
>>> +
>>> +    sfc = devm_kzalloc(dev, sizeof(*sfc), GFP_KERNEL);
>>> +    if (!sfc)
>>> +        return -ENOMEM;
>>> +
>>> +    platform_set_drvdata(pdev, sfc);
>>> +    sfc->dev = dev;
>>> +
>>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +    sfc->regbase = devm_ioremap_resource(dev, res);
>>> +    if (IS_ERR(sfc->regbase))
>>> +        return PTR_ERR(sfc->regbase);
>>> +
>>> +    sfc->clk = devm_clk_get(&pdev->dev, "sfc");
>>> +    if (IS_ERR(sfc->clk)) {
>>> +        dev_err(&pdev->dev, "Failed to get sfc interface clk\n");
>>> +        return PTR_ERR(sfc->clk);
>>> +    }
>>> +
>>> +    sfc->hclk = devm_clk_get(&pdev->dev, "hsfc");
>>> +    if (IS_ERR(sfc->hclk)) {
>>> +        dev_err(&pdev->dev, "Failed to get sfc ahp clk\n");
>>> +        return PTR_ERR(sfc->hclk);
>>> +    }
>>> +
>>> +    ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>>> +    if (ret) {
>>> +        dev_warn(dev, "Unable to set dma mask\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    sfc->buffer = dmam_alloc_coherent(dev, SFC_DMA_MAX_LEN,
>>> +            &sfc->dma_buffer, GFP_KERNEL);
>>> +    if (!sfc->buffer)
>>> +        return -ENOMEM;
>>> +
>>> +    mutex_init(&sfc->lock);
>>> +
>>> +    ret = clk_prepare_enable(sfc->hclk);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "Failed to enable hclk\n");
>>> +        goto err_hclk;
>>> +    }
>>> +
>>> +    ret = clk_prepare_enable(sfc->clk);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "Failed to enable clk\n");
>>> +        goto err_clk;
>>> +    }
>>> +
>>> +    sfc->use_dma = !of_property_read_bool(sfc->dev->of_node,
>>> +                          "rockchip,sfc-no-dma");
>>> +
>>> +    sfc->negative_edge = of_device_is_compatible(sfc->dev->of_node,
>>> +                             "rockchip,rk1108-sfc");
>>
>> I think this should rather be a boolean property -- but isn't this
>> something like CPOL or CPHA anyway ?
> 
> It isn't the same as CPOL or CPHA. And this value should be Soc
> specificed.

So what is this about ? Can you explain what this negative_edge thing is
and how it works on the bus-level ?

[...]

-- 
Best regards,
Marek Vasut