Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

From: Jiancheng Xue
Date: Wed Apr 06 2016 - 22:13:47 EST

Next message: Ming Lei: "Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs"
Previous message: Greg KH: "Re: [PULL] lkdtm update (4.6-rc3)"
In reply to: Brian Norris: "Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver"
Next in thread: Marek Vasut: "Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Brian,
Thank you very much for your comments. I'll fix these issues in next version.
In addition, for easy understanding I'd like to rewrite hisi_spi_nor_write and
hisi_spi_nor_read. Your comments on these modifications will be highly appreciated.

static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
size_t *retlen, u_char *read_buf)
{
struct hifmc_priv *priv = nor->priv;
struct hifmc_host *host = priv->host;
int i;

/* read all bytes in only one time */
if (len <= HIFMC_DMA_MAX_LEN) {
hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
len, FMC_OP_READ);
memcpy(read_buf, host->buffer, len);
} else {
/* read HIFMC_DMA_MAX_LEN bytes at a time */
for (i = 0; i < len; i += HIFMC_DMA_MAX_LEN) {
hisi_spi_nor_dma_transfer(nor, from + i, host->dma_buffer,
HIFMC_DMA_MAX_LEN, FMC_OP_READ);
memcpy(read_buf + i, host->buffer, HIFMC_DMA_MAX_LEN);
}
/* read remaining bytes */
i -= HIFMC_DMA_MAX_LEN;
hisi_spi_nor_dma_transfer(nor, from + i, host->dma_buffer,
len - i, FMC_OP_READ);
memcpy(read_buf + i, host->buffer, len - i);
}
*retlen = len;

return 0;
}

Because "len" passed from spi_nor_write is smaller than nor->page_size, and nor->page_size
is smaller than the length of host->dma_buffer. We can transfer "len" bytes data by
hisi_spi_nor_dma_transfer at one time. hisi_spi_nor_write can be simplified like below:

static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
size_t len, size_t *retlen, const u_char *write_buf)
{
struct hifmc_priv *priv = nor->priv;
struct hifmc_host *host = priv->host;

/* len is smaller than dma buffer length*/
memcpy(host->buffer, write_buf, len);
hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, len,
FMC_OP_WRITE);

*retlen = len;
}

Regards,
Jiancheng

On 2016/4/4 14:44, Brian Norris wrote:
> Hi Jiancheng,
>
> Looking good. In addition to Marek's comments, I have just a few small
> comments. I'll post a small diff at the end, and a few inline comments.
>
> On Mon, Mar 28, 2016 at 05:15:28PM +0800, Jiancheng Xue wrote:
>> Hi Marek,
>> Firstly, thank you very much for your comments.
>>
>> On 2016/3/27 9:47, Marek Vasut wrote:
>>> On 03/26/2016 09:11 AM, Jiancheng Xue wrote:
>>>> Add hisilicon spi-nor flash controller driver
>>>>
>>>> Signed-off-by: Binquan Peng <pengbinquan@xxxxxxxxxx>
>>>> Signed-off-by: Jiancheng Xue <xuejiancheng@xxxxxxxxxx>
>>>> Acked-by: Rob Herring <robh@xxxxxxxxxx>
>>>> Reviewed-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxxxxxxxxx>
>>>> Reviewed-by: Jagan Teki <jteki@xxxxxxxxxxxx>
>>>> ---
>>>> change log
>>>> v9:
>>>> Fixed issues pointed by Jagan Teki.
>>>
>>> It'd be really great if you could list which exact issues you fixed.
>>>
>>
>> OK. I'll describe the history log more detailed in next version.
>>
>>>> v8:
>>>> Fixed issues pointed by Ezequiel Garcia and Brian Norris.
>>>> Moved dts binding file to mtd directory.
>>>> Changed the compatible string more specific.
>>>
>>> [...]
>
> ^^ You were using <linux/of_device.h> in here, even though you don't
> need any of its contents. You just wanted <linux/of.h> and
> <linux/platform_device.h>.
>
>>>
>>>> +/* Hardware register offsets and field definitions */
>>>> +#define FMC_CFG 0x00
>>>> +#define SPI_NOR_ADDR_MODE BIT(10)
>>>> +#define FMC_GLOBAL_CFG 0x04
>>>> +#define FMC_GLOBAL_CFG_WP_ENABLE BIT(6)
>>>> +#define FMC_SPI_TIMING_CFG 0x08
>>>> +#define TIMING_CFG_TCSH(nr) (((nr) & 0xf) << 8)
>>>> +#define TIMING_CFG_TCSS(nr) (((nr) & 0xf) << 4)
>>>> +#define TIMING_CFG_TSHSL(nr) ((nr) & 0xf)
>>>> +#define CS_HOLD_TIME 0x6
>>>> +#define CS_SETUP_TIME 0x6
>>>> +#define CS_DESELECT_TIME 0xf
>>>> +#define FMC_INT 0x18
>>>> +#define FMC_INT_OP_DONE BIT(0)
>>>> +#define FMC_INT_CLR 0x20
>>>> +#define FMC_CMD 0x24
>>>> +#define FMC_CMD_CMD1(_cmd) ((_cmd) & 0xff)
>>>
>>> Drop the underscores in the argument names.
>>>
>> OK.
>>>> +#define FMC_ADDRL 0x2c
>>>> +#define FMC_OP_CFG 0x30
>>>> +#define OP_CFG_FM_CS(_cs) ((_cs) << 11)
>>>> +#define OP_CFG_MEM_IF_TYPE(_type) (((_type) & 0x7) << 7)
>>>> +#define OP_CFG_ADDR_NUM(_addr) (((_addr) & 0x7) << 4)
>>>> +#define OP_CFG_DUMMY_NUM(_dummy) ((_dummy) & 0xf)
>>>> +#define FMC_DATA_NUM 0x38
>>>> +#define FMC_DATA_NUM_CNT(_n) ((_n) & 0x3fff)
>>>> +#define FMC_OP 0x3c
>>>> +#define FMC_OP_DUMMY_EN BIT(8)
>>>> +#define FMC_OP_CMD1_EN BIT(7)
>>>> +#define FMC_OP_ADDR_EN BIT(6)
>>>> +#define FMC_OP_WRITE_DATA_EN BIT(5)
>>>> +#define FMC_OP_READ_DATA_EN BIT(2)
>>>> +#define FMC_OP_READ_STATUS_EN BIT(1)
>>>> +#define FMC_OP_REG_OP_START BIT(0)
>>>
>>> [...]
>>>
>>>> +enum hifmc_iftype {
>>>> + IF_TYPE_STD,
>>>> + IF_TYPE_DUAL,
>>>> + IF_TYPE_DIO,
>>>> + IF_TYPE_QUAD,
>>>> + IF_TYPE_QIO,
>>>> +};
>>>> +
>>>> +struct hifmc_priv {
>>>> + int chipselect;
>>>
>>> Can chipselect be set to < 0 values ?
>>>
>> The type will be changed to u32.
>>
>>>> + u32 clkrate;
>>>> + struct hifmc_host *host;
>>>> +};
>>>> +
>>>> +#define HIFMC_MAX_CHIP_NUM 2
>>>
>>> This does not scale very well, use dynamic allocation.
>>>
>> OK. Dynamic allocation will be used in next version.
>>>> +struct hifmc_host {
>>>> + struct device *dev;
>>>> + struct mutex lock;
>>>> +
>>>> + void __iomem *regbase;
>>>> + void __iomem *iobase;
>>>> + struct clk *clk;
>>>> + void *buffer;
>>>> + dma_addr_t dma_buffer;
>>>> +
>>>> + struct spi_nor nor[HIFMC_MAX_CHIP_NUM];
>>>> + struct hifmc_priv priv[HIFMC_MAX_CHIP_NUM];
>>>> + int num_chip;
>>>> +};
>>>> +
>>>> +static inline int wait_op_finish(struct hifmc_host *host)
>>>> +{
>>>> + unsigned int reg;
>>>> +
>>>> + return readl_poll_timeout(host->regbase + FMC_INT, reg,
>>>> + (reg & FMC_INT_OP_DONE), 0, FMC_WAIT_TIMEOUT);
>>>> +}
>>>> +
>>>> +static int get_if_type(enum read_mode flash_read)
>>>> +{
>>>> + enum hifmc_iftype if_type;
>>>> +
>>>> + switch (flash_read) {
>>>> + case SPI_NOR_DUAL:
>>>> + if_type = IF_TYPE_DUAL;
>>>> + break;
>>>> + case SPI_NOR_QUAD:
>>>> + if_type = IF_TYPE_QUAD;
>>>> + break;
>>>> + case SPI_NOR_NORMAL:
>>>> + case SPI_NOR_FAST:
>>>> + default:
>>>> + if_type = IF_TYPE_STD;
>>>> + break;
>>>> + }
>>>> +
>>>> + return if_type;
>>>> +}
>>>> +
>>>> +static void hisi_spi_nor_init(struct hifmc_host *host)
>>>> +{
>>>> + unsigned int reg;
>>>
>>> Should be u32 here.
>>>
>> OK.
>>>> + reg = TIMING_CFG_TCSH(CS_HOLD_TIME)
>>>> + | TIMING_CFG_TCSS(CS_SETUP_TIME)
>>>> + | TIMING_CFG_TSHSL(CS_DESELECT_TIME);
>>>> + writel(reg, host->regbase + FMC_SPI_TIMING_CFG);
>>>> +}
>>>> +
>>>> +static int hisi_spi_nor_prep(struct spi_nor *nor, enum spi_nor_ops ops)
>>>> +{
>>>> + struct hifmc_priv *priv = nor->priv;
>>>> + struct hifmc_host *host = priv->host;
>>>> + int ret;
>>>> +
>>>> + mutex_lock(&host->lock);
>>>
>>> Why do you need the mutex lock here ? Let me guess -- SPI NOR framework
>>> locks a mutex in struct spi_nor , but that's not enough if you have
>>> multiple SPI NORs on the same bus, because concurrent access to multiple
>>> SPI NOR flashes needs locking on the controller level, right ?
>>>
>> Yes, you are quite right. The controller can connect with two SPI NOR flashes
>> on one board. This lock is used on the controller level.
>
> Yeah... we should probably implement some common controller logic in the
> core eventually. But the mutex is necessary for now.
>
>>>> + ret = clk_set_rate(host->clk, priv->clkrate);
>>>> + if (ret)
>>>> + goto out;
>>>> +
>>>> + ret = clk_prepare_enable(host->clk);
>>>> + if (ret)
>>>> + goto out;
>>>> +
>>>> + return 0;
>>>> +
>>>> +out:
>>>> + mutex_unlock(&host->lock);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static void hisi_spi_nor_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
>>>> +{
>>>> + struct hifmc_priv *priv = nor->priv;
>>>> + struct hifmc_host *host = priv->host;
>>>> +
>>>> + clk_disable_unprepare(host->clk);
>>>> + mutex_unlock(&host->lock);
>>>> +}
>>>> +
>>>> +static void hisi_spi_nor_cmd_prepare(struct hifmc_host *host, u8 cmd,
>>>> + u32 *opcfg)
>>>> +{
>>>> + u32 reg;
>>>> +
>>>> + *opcfg |= FMC_OP_CMD1_EN;
>>>> + switch (cmd) {
>>>> + case SPINOR_OP_RDID:
>>>> + case SPINOR_OP_RDSR:
>>>> + case SPINOR_OP_RDCR:
>>>> + *opcfg |= FMC_OP_READ_DATA_EN;
>>>> + break;
>>>> + case SPINOR_OP_WREN:
>>>> + reg = readl(host->regbase + FMC_GLOBAL_CFG);
>>>> + if (reg & FMC_GLOBAL_CFG_WP_ENABLE) {
>>>> + reg &= ~FMC_GLOBAL_CFG_WP_ENABLE;
>>>> + writel(reg, host->regbase + FMC_GLOBAL_CFG);
>>>> + }
>>>> + break;
>>>> + case SPINOR_OP_WRSR:
>>>> + *opcfg |= FMC_OP_WRITE_DATA_EN;
>>>> + break;
>>>> + case SPINOR_OP_BE_4K:
>>>> + case SPINOR_OP_BE_4K_PMC:
>>>> + case SPINOR_OP_SE_4B:
>>>> + case SPINOR_OP_SE:
>>>> + *opcfg |= FMC_OP_ADDR_EN;
>>>> + break;
>>>> + case SPINOR_OP_EN4B:
>>>> + reg = readl(host->regbase + FMC_CFG);
>>>> + reg |= SPI_NOR_ADDR_MODE;
>>>> + writel(reg, host->regbase + FMC_CFG);
>>>> + break;
>>>> + case SPINOR_OP_EX4B:
>>>> + reg = readl(host->regbase + FMC_CFG);
>>>> + reg &= ~SPI_NOR_ADDR_MODE;
>>>> + writel(reg, host->regbase + FMC_CFG);
>>>> + break;
>>>> + case SPINOR_OP_CHIP_ERASE:
>>>> + default:
>>>> + break;
>>>> + }
>>>
>>> Won't the driver fail if we add new instructions into the SPI NOR core
>>> which are not handled by this huge switch statement ?
>>>
>> Only some of commands are needed to process in this stage for the controller.
>> Others have no needs. So this function won't return failure.
>
> It's not ideal to have this sort of function if we can avoid it (since
> it's hard to figure out how to extend this for new opcodes). But it
> looks necessary for now.
>
>> I'll combine SPINOR_OP_CHIP_ERASE into the default case in next version.
>>
>>>> +}
>>>
>>> [...]
>>>
>>>> +static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
>>>> + size_t len, size_t *retlen, const u_char *write_buf)
>>>> +{
>>>> + struct hifmc_priv *priv = nor->priv;
>>>> + struct hifmc_host *host = priv->host;
>>>> + const unsigned char *ptr = write_buf;
>>>> + size_t actual_len;
>>>> +
>>>> + *retlen = 0;
>>>> + while (len > 0) {
>>>> + if (to & HIFMC_DMA_MASK)
>>>> + actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
>>>> + >= len ? len
>>>> + : (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
>>>
>>> Rewrite this as something like the following snippet, for the sake of
>>> readability:
>>>
>>> actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
>>> if (actual_len >= len)
>>> actual_len = len;
>>>
>> OK. Thank you.
>>>> + else
>>>> + actual_len = (len >= HIFMC_DMA_MAX_LEN)
>>>> + ? HIFMC_DMA_MAX_LEN : len;
>
> Wait, why do you need the else case? Isn't this just equivalent to the
> first case? I'd suggest consolidating these two blocks, and dropping the
> ?: entirely, since (like Marek said) it's hurting readability. So:
>
> /* Don't cross HIFMC_DMA_MAX_LEN alignment boundaries */
> if ((HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK)) >= len)
> actual_len = len;
> else
> actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
>
>>>> + memcpy(host->buffer, ptr, actual_len);
>>>> + hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
>>>> + FMC_OP_WRITE);
>>>> + to += actual_len;
>>>> + ptr += actual_len;
>>>> + len -= actual_len;
>>>> + *retlen += actual_len;
>>>> + }
>>>> +}
>
> [...]
>
> Here's my diff:
>
> diff --git a/drivers/mtd/spi-nor/hisi-sfc.c b/drivers/mtd/spi-nor/hisi-sfc.c
> index e974c92a0a25..0c58fd3b8790 100644
> --- a/drivers/mtd/spi-nor/hisi-sfc.c
> +++ b/drivers/mtd/spi-nor/hisi-sfc.c
> @@ -16,13 +16,15 @@
> * You should have received a copy of the GNU General Public License
> * along with this program. If not, see <http://www.gnu.org/licenses/>.
> */
> +
> #include <linux/clk.h>
> #include <linux/dma-mapping.h>
> #include <linux/iopoll.h>
> #include <linux/module.h>
> #include <linux/mtd/mtd.h>
> #include <linux/mtd/spi-nor.h>
> -#include <linux/of_platform.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> #include <linux/slab.h>
>
> /* Hardware register offsets and field definitions */
> @@ -343,13 +345,11 @@ static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
>
> *retlen = 0;
> while (len > 0) {
> - if (to & HIFMC_DMA_MASK)
> - actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
> - >= len ? len
> - : (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
> + /* Don't cross HIFMC_DMA_MAX_LEN alignment boundaries */
> + if ((HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK)) >= len)
> + actual_len = len;
> else
> - actual_len = (len >= HIFMC_DMA_MAX_LEN)
> - ? HIFMC_DMA_MAX_LEN : len;
> + actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
> memcpy(host->buffer, ptr, actual_len);
> hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
> FMC_OP_WRITE);
>
> .
>

Next message: Ming Lei: "Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs"
Previous message: Greg KH: "Re: [PULL] lkdtm update (4.6-rc3)"
In reply to: Brian Norris: "Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver"
Next in thread: Marek Vasut: "Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]