On 04/07/2016 04:10 AM, Jiancheng Xue wrote:
> 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.
Would you please stop top-posting ? It rubs some people the wrong way.
> 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);
Is all the ad-hoc memcpying necessary? I think you can use
dma_map_single() and co to obtain DMAble memory for your
controller's use and then you can probably get rid of most
of this stuff.
> } 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 <[email protected]>
>>>>> Signed-off-by: Jiancheng Xue <[email protected]>
>>>>> Acked-by: Rob Herring <[email protected]>
>>>>> Reviewed-by: Ezequiel Garcia <[email protected]>
>>>>> Reviewed-by: Jagan Teki <[email protected]>
>>>>> ---
>>>>> 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);
>>
>> .
>>
>
--
Best regards,
Marek Vasut
