linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: masonccyang@mxic.com.tw
To: "Boris Brezillon" <boris.brezillon@collabora.com>
Cc: anders.roxell@linaro.org, bbrezillon@kernel.org,
	christophe.kerello@st.com, computersforpeace@gmail.com,
	devicetree@vger.kernel.org, dwmw2@infradead.org,
	juliensu@mxic.com.tw, lee.jones@linaro.org,
	liang.yang@amlogic.com, linux-kernel@vger.kernel.org,
	linux-mtd@lists.infradead.org, marek.vasut@gmail.com,
	mark.rutland@arm.com, miquel.raynal@bootlin.com,
	paul@crapouillou.net, paul.burton@mips.com, richard@nod.at,
	robh+dt@kernel.org, stefan@agner.ch, vigneshr@ti.com
Subject: Re: [PATCH v6 1/2] mtd: rawnand: Add Macronix raw NAND controller driver
Date: Mon, 5 Aug 2019 17:21:50 +0800	[thread overview]
Message-ID: <OF566978C2.AD9BE9D7-ON4825844D.0030DD04-4825844D.00336FFE@mxic.com.tw> (raw)
In-Reply-To: <20190801082233.759f6ae9@collabora.com>


Hi Boris,

> > +
> > +struct mxic_nand_ctlr {
> > +   struct clk *ps_clk;
> > +   struct clk *send_clk;
> > +   struct clk *send_dly_clk;
> > +   void __iomem *regs;
> > +   struct nand_controller controller;
> > +   struct device *dev;
> > +   void *priv;
> 
> Looks like this priv field point to a nand_chip object. Please replace
> it by:
> 
>    struct nand_chip *chip;

okay, will fix.

> 
> > +};
> > +
> > +struct mxic_nand_chip {
> > +   struct nand_chip chip;
> > +};
> 
> No need to define your own nand_chip struct if all it contains is the
> base definition.

okay, will fix.

> 
> > +
> > +static int mxic_nfc_clk_enable(struct mxic_nand_ctlr *nfc)
> > +{
> > +   int ret;
> > +
> > +   ret = clk_prepare_enable(nfc->ps_clk);
> > +   if (ret)
> > +      return ret;
> > +
> > +   ret = clk_prepare_enable(nfc->send_clk);
> > +   if (ret)
> > +      goto err_ps_clk;
> > +
> > +   ret = clk_prepare_enable(nfc->send_dly_clk);
> > +   if (ret)
> > +      goto err_send_dly_clk;
> > +
> > +   return ret;
> > +
> > +err_send_dly_clk:
> > +   clk_disable_unprepare(nfc->send_clk);
> > +err_ps_clk:
> > +   clk_disable_unprepare(nfc->ps_clk);
> > +
> > +   return ret;
> > +}
> > +
> > +static void mxic_nfc_clk_disable(struct mxic_nand_ctlr *nfc)
> > +{
> > +   clk_disable_unprepare(nfc->send_clk);
> > +   clk_disable_unprepare(nfc->send_dly_clk);
> > +   clk_disable_unprepare(nfc->ps_clk);
> > +}
> > +
> > +static void mxic_nfc_set_input_delay(struct mxic_nand_ctlr *nfc, u8 
idly_code)
> > +{
> > +   writel(IDLY_CODE_VAL(0, idly_code) |
> > +          IDLY_CODE_VAL(1, idly_code) |
> > +          IDLY_CODE_VAL(2, idly_code) |
> > +          IDLY_CODE_VAL(3, idly_code),
> > +          nfc->regs + IDLY_CODE(0));
> > +   writel(IDLY_CODE_VAL(4, idly_code) |
> > +          IDLY_CODE_VAL(5, idly_code) |
> > +          IDLY_CODE_VAL(6, idly_code) |
> > +          IDLY_CODE_VAL(7, idly_code),
> > +          nfc->regs + IDLY_CODE(1));
> > +}
> > +
> > +static int mxic_nfc_clk_setup(struct mxic_nand_ctlr *nfc, unsigned 
long freq)
> > +{
> > +   int ret;
> > +
> > +   ret = clk_set_rate(nfc->send_clk, freq);
> > +   if (ret)
> > +      return ret;
> > +
> > +   ret = clk_set_rate(nfc->send_dly_clk, freq);
> > +   if (ret)
> > +      return ret;
> > +
> > +   /*
> > +    * A constant delay range from 0x0 ~ 0x1F for input delay,
> > +    * the unit is 78 ps, the max input delay is 2.418 ns.
> > +    */
> > +   mxic_nfc_set_input_delay(nfc, 0xf);
> 
> Just curious. Shouldn't we use that to support EDO modes? This being
> said, a delay of 2.5ns will not be enough for EDO...

This mxic_nfc_set_input_delay() thing is for Data IO pins and these delay
are for internal #RE path latch Data.

> 
> > +
> > +   /*
> > +    * Phase degree = 360 * freq * output-delay
> > +    * where output-delay is a constant value 1 ns in FPGA.
> > +    *
> > +    * Get Phase degree = 360 * freq * 1 ns
> > +    *                  = 360 * freq * 1 sec / 1000000000
> > +    *                  = 9 * freq / 25000000
> > +    */
> > +   ret = clk_set_phase(nfc->send_dly_clk, 9 * freq / 25000000);
> > +   if (ret)
> > +      return ret;
> > +
> > +   return 0;
> > +}
> > +
> > +static int mxic_nfc_set_freq(struct mxic_nand_ctlr *nfc, unsigned 
long freq)
> > +{
> > +   int ret;
> > +
> > +   if (freq > MXIC_NFC_MAX_CLK_HZ)
> > +      freq = MXIC_NFC_MAX_CLK_HZ;
> > +
> > +   mxic_nfc_clk_disable(nfc);
> > +   ret = mxic_nfc_clk_setup(nfc, freq);
> > +   if (ret)
> > +      return ret;
> > +
> > +   ret = mxic_nfc_clk_enable(nfc);
> > +   if (ret)
> > +      return ret;
> > +
> > +   return 0;
> > +}
> > +
> > +static void mxic_nfc_hw_init(struct mxic_nand_ctlr *nfc)
> > +{
> > +   writel(DATA_STROB_EDO_EN, nfc->regs + DATA_STROB);
> 
> Oh, no, here is the EDO flag. BTW, you should not have it set by
> default, it's something you configure in your ->setup_data_interface()
> implementation.

okay, got it and will fix it.

> 
> > +   writel(HC_CFG_NIO(8) | HC_CFG_TYPE(1, HC_CFG_TYPE_RAW_NAND) |
> > +          HC_CFG_SLV_ACT(0) | HC_CFG_MAN_CS_EN |
> > +          HC_CFG_IDLE_SIO_LVL(1), nfc->regs + HC_CFG);
> > +   writel(INT_STS_ALL, nfc->regs + INT_STS_EN);
> > +   writel(0x0, nfc->regs + ONFI_DIN_CNT(0));
> > +   writel(0, nfc->regs + LRD_CFG);
> > +   writel(0, nfc->regs + LRD_CTRL);
> > +   writel(0x0, nfc->regs + HC_EN);
> > +
> > +   /* Default 10 MHz to setup tRC_min/tWC_min:100 ns */
> > +   mxic_nfc_set_freq(nfc, 10000000);
> 
> Again, not something you should configure here, but I guess having a
> default setting does not hurt.

okay, will fix it.

> 
> > +}
> > +
> > +static void mxic_nfc_cs_enable(struct mxic_nand_ctlr *nfc)
> > +{
> > +   writel(readl(nfc->regs + HC_CFG) | HC_CFG_MAN_CS_EN,
> > +          nfc->regs + HC_CFG);
> > +   writel(HC_CFG_MAN_CS_ASSERT | readl(nfc->regs + HC_CFG),
> > +          nfc->regs + HC_CFG);
> > +}
> > +
> > +static void mxic_nfc_cs_disable(struct mxic_nand_ctlr *nfc)
> > +{
> > +   writel(~HC_CFG_MAN_CS_ASSERT & readl(nfc->regs + HC_CFG),
> > +          nfc->regs + HC_CFG);
> > +}
> > +
> > +static int  mxic_nfc_wait_ready(struct nand_chip *chip)
> > +{
> > +   struct mxic_nand_ctlr *nfc = nand_get_controller_data(chip);
> > +   u32 sts;
> > +
> > +   return readl_poll_timeout(nfc->regs + INT_STS, sts,
> > +              sts & INT_RDY_PIN, 0, USEC_PER_SEC);
> 
> You're not using interrupts at all? For things like R/B wait it's
> usually a good thing to rely on interrupts instead of status-polling.

In our current FPGA bitstreams, only implement status-polling.
Interrupts will implement in ASIC.


> > +
> > +static int mxic_nfc_setup_data_interface(struct nand_chip *chip, int 
chipnr,
> > +                const struct nand_data_interface *conf)
> > +{
> > +   struct mxic_nand_ctlr *nfc = nand_get_controller_data(chip);
> > +   const struct nand_sdr_timings *sdr;
> > +   unsigned long freq;
> > +
> > +   sdr = nand_get_sdr_timings(conf);
> > +   if (IS_ERR(sdr))
> > +      return PTR_ERR(sdr);
> > +
> > +   if (chipnr < 0)
> 
> Please use the NAND_DATA_IFACE_CHECK_ONLY macro for this check:
> 
>    if (chipnr == NAND_DATA_IFACE_CHECK_ONLY)
>       return 0;
> 

okay, will fix.

> > +      return 0;
> > +
> > +   if (sdr->tRC_min)
> > +      freq = 1000000000 / (sdr->tRC_min / 1000);
> 
> Please use NSEC_PER_SEC instead of 1000000000. And I think you can get
> rid of the check on sdr->tRC_min (it should never be 0).

got it, thanks.

> 
> > +
> > +   return mxic_nfc_set_freq(nfc, freq);
> 
> You should set the EDO when ->tRC_min < 30000 IIRC, clear it otherwise.
> 

okay, will fix,


> > +}
> > +
> > +static const struct nand_controller_ops mxic_nand_controller_ops = {
> > +   .exec_op = mxic_nfc_exec_op,
> > +   .setup_data_interface = mxic_nfc_setup_data_interface,
> > +};
> > +
> > +static int mxic_nfc_probe(struct platform_device *pdev)
> > +{
> > +   struct mtd_info *mtd;
> > +   struct mxic_nand_ctlr *nfc;
> > +   struct mxic_nand_chip *mxic_nand;
> > +   struct nand_chip *nand_chip;
> > +   struct resource *res;
> > +   int err;
> > +
> > +   nfc = devm_kzalloc(&pdev->dev, sizeof(struct mxic_nand_ctlr),
> > +            GFP_KERNEL);
> > +   if (!nfc)
> > +      return -ENOMEM;
> > +
> > +   mxic_nand = devm_kzalloc(&pdev->dev, sizeof(struct 
mxic_nand_chip),
> > +             GFP_KERNEL);
> > +   if (!mxic_nand)
> > +      return -ENOMEM;
> > +
> > +   nfc->ps_clk = devm_clk_get(&pdev->dev, "ps");
> > +   if (IS_ERR(nfc->ps_clk))
> > +      return PTR_ERR(nfc->ps_clk);
> > +
> > +   nfc->send_clk = devm_clk_get(&pdev->dev, "send");
> > +   if (IS_ERR(nfc->send_clk))
> > +      return PTR_ERR(nfc->send_clk);
> > +
> > +   nfc->send_dly_clk = devm_clk_get(&pdev->dev, "send_dly");
> > +   if (IS_ERR(nfc->send_dly_clk))
> > +      return PTR_ERR(nfc->send_dly_clk);
> > +
> > +   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +   nfc->regs = devm_ioremap_resource(&pdev->dev, res);
> > +   if (IS_ERR(nfc->regs))
> > +      return PTR_ERR(nfc->regs);
> > +
> > +   nand_chip = &mxic_nand->chip;
> > +   mtd = nand_to_mtd(nand_chip);
> > +   mtd->dev.parent = &pdev->dev;
> > +   nand_chip->ecc.priv = NULL;
> 
> No need to do this NULL assignment, the object is allocated with
> devm_kzalloc().

okay, got it.

> 
> > +   nand_set_flash_node(nand_chip, pdev->dev.of_node);
> 
> The flash node should be a child of pdev->dev.of_node,
> pdev->dev.of_node is representing your controller not the NAND chip.

I should also patch DTS to add a subnode which is connected to NAND
controller, as your comments on
[PATCH v6 2/2] dt-bindings: mtd: Document Macronix raw NAND controller 
bindings

right ?

> 
> > +   nand_chip->priv = nfc;
> > +   nfc->dev = &pdev->dev;
> > +   nfc->priv = nand_chip;
> > +
> > +   nfc->controller.ops = &mxic_nand_controller_ops;
> > +   nand_controller_init(&nfc->controller);
> > +   nand_chip->controller = &nfc->controller;
> > +
> > +   mxic_nfc_hw_init(nfc);
> > +
> > +   err = nand_scan(nand_chip, 1);
> > +   if (err)
> > +      goto fail;
> > +
> > +   err = mtd_device_register(mtd, NULL, 0);
> > +   if (err)
> > +      goto fail;
> > +
> > +   platform_set_drvdata(pdev, nfc);
> > +   return 0;
> > +
> > +fail:
> > +   mxic_nfc_clk_disable(nfc);
> 
> Looks like you never call mxic_nfc_clk_enable(), which means you'll end
> up with unbalanced prepare/enable counts. Also not sure how that can
> work unless the bootloader takes care of enabling the clks for you.

mxic_nfc_set_freq() will do that.


thanks for your time and review.

best regards,
Mason


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================


  reply	other threads:[~2019-08-05  9:23 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-01  3:55 [PATCH v6 0/2] " Mason Yang
2019-08-01  3:55 ` [PATCH v6 1/2] mtd: rawnand: " Mason Yang
2019-08-01  6:22   ` Boris Brezillon
2019-08-05  9:21     ` masonccyang [this message]
2019-08-16 10:10     ` masonccyang
2019-08-01  3:55 ` [PATCH v6 2/2] dt-bindings: mtd: Document Macronix raw NAND controller bindings Mason Yang
2019-08-01  5:57   ` Boris Brezillon
2019-08-01  9:17     ` masonccyang
2019-08-01  9:23       ` Boris Brezillon
2019-08-01  7:13   ` Miquel Raynal
2019-08-01  9:32     ` masonccyang
2019-08-01  9:36       ` Miquel Raynal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=OF566978C2.AD9BE9D7-ON4825844D.0030DD04-4825844D.00336FFE@mxic.com.tw \
    --to=masonccyang@mxic.com.tw \
    --cc=anders.roxell@linaro.org \
    --cc=bbrezillon@kernel.org \
    --cc=boris.brezillon@collabora.com \
    --cc=christophe.kerello@st.com \
    --cc=computersforpeace@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dwmw2@infradead.org \
    --cc=juliensu@mxic.com.tw \
    --cc=lee.jones@linaro.org \
    --cc=liang.yang@amlogic.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=marek.vasut@gmail.com \
    --cc=mark.rutland@arm.com \
    --cc=miquel.raynal@bootlin.com \
    --cc=paul.burton@mips.com \
    --cc=paul@crapouillou.net \
    --cc=richard@nod.at \
    --cc=robh+dt@kernel.org \
    --cc=stefan@agner.ch \
    --cc=vigneshr@ti.com \
    --subject='Re: [PATCH v6 1/2] mtd: rawnand: Add Macronix raw NAND controller driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).