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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
