| Message ID | 1555320234-15802-1-git-send-email-masonccyang@mxic.com.tw |
|---|---|
| Headers | show |
| Series | Add Macronix MX25F0A MFD driver for raw nand and spi | expand |
On Mon, Apr 15, 2019 at 05:23:53PM +0800, Mason Yang wrote:
> Patch Macronix MX25F0A SPI controller driver according to it's MFD driver.
It'd be much better to describe what the above actually means - what
changes have been made in the introduction of the MFD driver? It does
feel like there's not as much abstraction as I'd expect between the MFD
and the child, there's a lot of peering into the parent and enabling and
disabling individual clocks for example rather than either having this
hidden behind a function or just having the clocks owned by the child
driver. The driver also isn't using the MFD interfaces to pass through
the register subblocks for the IP - instead the child driver is peering
straight into the MFD structure and looking at a variable in there.
On Tue, Apr 30, 2019 at 06:23:21PM +0800, masonccyang@mxic.com.tw wrote: > > It'd be much better to describe what the above actually means - what > > changes have been made in the introduction of the MFD driver? It does > > feel like there's not as much abstraction as I'd expect between the MFD > > and the child, there's a lot of peering into the parent and enabling and > > disabling individual clocks for example rather than either having this > > hidden behind a function or just having the clocks owned by the child > > driver. > Do you mean I should remove ps_clk/send_clk/send_dly_clk resource from MFD > and patch them to spi-mxic.c ? > Or any other ? I think you need to have a clear idea that you can explain as to what the MFD is and how it's split up. What's being abstracted, what's not and why. Without knowing anything about the device or what the series is trying to accomplish it's hard to be sure exactly what the best thing to do is. > The driver also isn't using the MFD interfaces to pass through > > the register subblocks for the IP - instead the child driver is peering > > straight into the MFD structure and looking at a variable in there. > Patch regmap for mfd, nand and spi ? > or any other patches is needed ? This is a memory mapped device so there should be no need to use regmap unless you want to. The MFD subsystem has facilities for passing through memory regions to subdevices.
Hi Mason, masonccyang@mxic.com.tw wrote on Fri, 17 May 2019 17:30:21 +0800: > Hi Miquel, > > > > + > > > +static void mxic_nand_select_chip(struct nand_chip *chip, int chipnr) > > > > _select_target() is preferred now > > Do you mean I implement mxic_nand_select_target() to control #CS ? > > If so, I need to call mxic_nand_select_target( ) to control #CS ON > and then #CS OFF in _exec_op() due to nand_select_target()<in nand_base,c> > is still calling chip->legacy.select_chip ? You must forget about the ->select_chip() callback. Now it should be handled directly from the controller driver. Please have a look at the commit pointed against the marvell_nand.c driver. [...] > > > + if (!mxic) > > > + return -ENOMEM; > > > + > > > + nand_chip = &mxic->nand; > > > + mtd = nand_to_mtd(nand_chip); > > > + mtd->dev.parent = pdev->dev.parent; > > > + nand_chip->ecc.priv = NULL; > > > + nand_set_flash_node(nand_chip, pdev->dev.parent->of_node); > > > + nand_chip->priv = mxic; > > > + > > > + mxic->mfd = mfd; > > > + > > > + nand_chip->legacy.select_chip = mxic_nand_select_chip; > > > > Please don't implement legacy interfaces. You can check in > > marvell_nand.c how this is handled now: > > > > b25251414f6e mtd: rawnand: marvell: Stop implementing ->select_chip() > > > > Does it mean chip->legacy.select_chip() will phase-out ? Yes it will. Thanks, Miquèl
Hi Miquel, > > > > > > + > > > > +static void mxic_nand_select_chip(struct nand_chip *chip, int chipnr) > > > > > > _select_target() is preferred now > > > > Do you mean I implement mxic_nand_select_target() to control #CS ? > > > > If so, I need to call mxic_nand_select_target( ) to control #CS ON > > and then #CS OFF in _exec_op() due to nand_select_target()<in nand_base,c> > > is still calling chip->legacy.select_chip ? > > You must forget about the ->select_chip() callback. Now it should be > handled directly from the controller driver. Please have a look at the > commit pointed against the marvell_nand.c driver. I have no Marvell NFC datasheet and have one question. In marvell_nand.c, there is no xxx_deselect_target() or something like that doing #CS OFF. marvell_nfc_select_target() seems always to make one of chip or die #CS keep low. Is it right ? How to make all #CS keep high for NAND to enter low-power standby mode if driver don't use "legacy.select_chip()" ? thanks & 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. =====================================================================
Hi Mason, masonccyang@mxic.com.tw wrote on Thu, 23 May 2019 16:58:02 +0800: > Hi Miquel, > > > > > > > > > + > > > > > +static void mxic_nand_select_chip(struct nand_chip *chip, int > chipnr) > > > > > > > > _select_target() is preferred now > > > > > > Do you mean I implement mxic_nand_select_target() to control #CS ? > > > > > > If so, I need to call mxic_nand_select_target( ) to control #CS ON > > > and then #CS OFF in _exec_op() due to nand_select_target()<in > nand_base,c> > > > is still calling chip->legacy.select_chip ? > > > > You must forget about the ->select_chip() callback. Now it should be > > handled directly from the controller driver. Please have a look at the > > commit pointed against the marvell_nand.c driver. > > I have no Marvell NFC datasheet and have one question. > > In marvell_nand.c, there is no xxx_deselect_target() or > something like that doing #CS OFF. > marvell_nfc_select_target() seems always to make one of chip or die > #CS keep low. > > Is it right ? Yes, AFAIR there is no "de-assert" mechanism in this controller. > > How to make all #CS keep high for NAND to enter > low-power standby mode if driver don't use "legacy.select_chip()" ? See commit 02b4a52604a4 ("mtd: rawnand: Make ->select_chip() optional when ->exec_op() is implemented") which states: "When [->select_chip() is] not implemented, the core is assuming the CS line is automatically asserted/deasserted by the driver ->exec_op() implementation." Of course, the above is right only when the controller driver supports the ->exec_op() interface. So if you think it is not too time consuming and worth the trouble to assert/deassert the CS at each operation, you may do it in your driver. Thanks, Miquèl
Hi Miquel, > > > > > > +static void mxic_nand_select_chip(struct nand_chip *chip, int > > chipnr) > > > > > > > > > > _select_target() is preferred now > > > > > > > > Do you mean I implement mxic_nand_select_target() to control #CS ? > > > > > > > > If so, I need to call mxic_nand_select_target( ) to control #CS ON > > > > and then #CS OFF in _exec_op() due to nand_select_target()<in > > nand_base,c> > > > > is still calling chip->legacy.select_chip ? > > > > > > You must forget about the ->select_chip() callback. Now it should be > > > handled directly from the controller driver. Please have a look at the > > > commit pointed against the marvell_nand.c driver. > > > > I have no Marvell NFC datasheet and have one question. > > > > In marvell_nand.c, there is no xxx_deselect_target() or > > something like that doing #CS OFF. > > marvell_nfc_select_target() seems always to make one of chip or die > > #CS keep low. > > > > Is it right ? > > Yes, AFAIR there is no "de-assert" mechanism in this controller. > > > > > How to make all #CS keep high for NAND to enter > > low-power standby mode if driver don't use "legacy.select_chip()" ? > > See commit 02b4a52604a4 ("mtd: rawnand: Make ->select_chip() optional > when ->exec_op() is implemented") which states: > > "When [->select_chip() is] not implemented, the core is assuming > the CS line is automatically asserted/deasserted by the driver > ->exec_op() implementation." > > Of course, the above is right only when the controller driver supports > the ->exec_op() interface. Currently, it seems that we will get the incorrect data and error operation due to CS in error toggling if CS line is controlled in ->exec_op(). i.e,. 1) In nand_onfi_detect() to call nand_exec_op() twice by nand_read_param_page_op() and annd_read_data_op() 2) In nand_write_page_xxx to call nand_exec_op() many times by nand_prog_page_begin_op(), nand_write_data_op() and nand_prog_page_end_op(). Should we consider to add a CS line controller in struct nand_controller i.e,. struct nand_controller { struct mutex lock; const struct nand_controller_ops *ops; + void (*select_chip)(struct nand_chip *chip, int cs); }; to replace legacy.select_chip() ? To patch in nand_select_target() and nand_deselect_target() void nand_select_target(struct nand_chip *chip, unsigned int cs) { /* * cs should always lie between 0 and chip->numchips, when that's not * the case it's a bug and the caller should be fixed. */ if (WARN_ON(cs > chip->numchips)) return; chip->cur_cs = cs; + if (chip->controller->select_chip) + chip->controller->select_chip(chip, cs); + if (chip->legacy.select_chip) chip->legacy.select_chip(chip, cs); } void nand_deselect_target(struct nand_chip *chip) { + if (chip->controller->select_chip) + chip->controller->select_chip(chip, -1); + if (chip->legacy.select_chip) chip->legacy.select_chip(chip, -1); chip->cur_cs = -1; } > > So if you think it is not too time consuming and worth the trouble to > assert/deassert the CS at each operation, you may do it in your driver. > > > Thanks, > Miquèl thanks & 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. =====================================================================
Hi Mason, masonccyang@mxic.com.tw wrote on Wed, 29 May 2019 11:12:08 +0800: > Hi Miquel, > > > > > > > > +static void mxic_nand_select_chip(struct nand_chip *chip, int > > > > chipnr) > > > > > > > > > > > > _select_target() is preferred now > > > > > > > > > > Do you mean I implement mxic_nand_select_target() to control #CS ? > > > > > > > > > > If so, I need to call mxic_nand_select_target( ) to control #CS ON > > > > > and then #CS OFF in _exec_op() due to nand_select_target()<in > > > nand_base,c> > > > > > is still calling chip->legacy.select_chip ? > > > > > > > > You must forget about the ->select_chip() callback. Now it should be > > > > handled directly from the controller driver. Please have a look at > the > > > > commit pointed against the marvell_nand.c driver. > > > > > > I have no Marvell NFC datasheet and have one question. > > > > > > In marvell_nand.c, there is no xxx_deselect_target() or > > > something like that doing #CS OFF. > > > marvell_nfc_select_target() seems always to make one of chip or die > > > #CS keep low. > > > > > > Is it right ? > > > > Yes, AFAIR there is no "de-assert" mechanism in this controller. > > > > > > > > How to make all #CS keep high for NAND to enter > > > low-power standby mode if driver don't use "legacy.select_chip()" ? > > > > See commit 02b4a52604a4 ("mtd: rawnand: Make ->select_chip() optional > > when ->exec_op() is implemented") which states: > > > > "When [->select_chip() is] not implemented, the core is assuming > > the CS line is automatically asserted/deasserted by the driver > > ->exec_op() implementation." > > > > Of course, the above is right only when the controller driver supports > > the ->exec_op() interface. > > Currently, it seems that we will get the incorrect data and error > operation due to CS in error toggling if CS line is controlled in > ->exec_op(). Most of the chips today are CS-don't-care, which chip are you using? Is this behavior publicly documented? Is this LPM mode always activated? > i.e,. > > 1) In nand_onfi_detect() to call nand_exec_op() twice by > nand_read_param_page_op() and annd_read_data_op() > > 2) In nand_write_page_xxx to call nand_exec_op() many times by > nand_prog_page_begin_op(), nand_write_data_op() and > nand_prog_page_end_op(). > > > Should we consider to add a CS line controller in struct nand_controller > i.e,. > > struct nand_controller { > struct mutex lock; > const struct nand_controller_ops *ops; > + void (*select_chip)(struct nand_chip *chip, int cs); > }; > > to replace legacy.select_chip() ? > No, if really needed, we could add a "macro op done" flag in the nand operation structure. Thanks, Miquèl
Hi Miquel, > > > > > > > > > +static void mxic_nand_select_chip(struct nand_chip *chip, int > > > > > > chipnr) > > > > > > > > > > > > > > _select_target() is preferred now > > > > > > > > > > > > Do you mean I implement mxic_nand_select_target() to control #CS ? > > > > > > > > > > > > If so, I need to call mxic_nand_select_target( ) to control #CS ON > > > > > > and then #CS OFF in _exec_op() due to nand_select_target()<in > > > > nand_base,c> > > > > > > is still calling chip->legacy.select_chip ? > > > > > > > > > > You must forget about the ->select_chip() callback. Now it should be > > > > > handled directly from the controller driver. Please have a look at > > the > > > > > commit pointed against the marvell_nand.c driver. > > > > > > > > I have no Marvell NFC datasheet and have one question. > > > > > > > > In marvell_nand.c, there is no xxx_deselect_target() or > > > > something like that doing #CS OFF. > > > > marvell_nfc_select_target() seems always to make one of chip or die > > > > #CS keep low. > > > > > > > > Is it right ? > > > > > > Yes, AFAIR there is no "de-assert" mechanism in this controller. > > > > > > > > > > > How to make all #CS keep high for NAND to enter > > > > low-power standby mode if driver don't use "legacy.select_chip()" ? > > > > > > See commit 02b4a52604a4 ("mtd: rawnand: Make ->select_chip() optional > > > when ->exec_op() is implemented") which states: > > > > > > "When [->select_chip() is] not implemented, the core is assuming > > > the CS line is automatically asserted/deasserted by the driver > > > ->exec_op() implementation." > > > > > > Of course, the above is right only when the controller driver supports > > > the ->exec_op() interface. > > > > Currently, it seems that we will get the incorrect data and error > > operation due to CS in error toggling if CS line is controlled in > > ->exec_op(). > > Most of the chips today are CS-don't-care, which chip are you using? I think CS-don't-care means read-write operation for NAND device to reside on the same memory bus as other Flash or SRAM devices. Other devices on the memory bus can then be accessed while the NAND Flash is busy with internal operations. This capability is very important for designs that require multiple NAND Flash devices on the same bus. > > Is this behavior publicly documented? > CS# pin goes High enter standby mode to reduce power consumption, i.e,. standby mode w/ CS# keep High, standby current: 10 uA (Typ for 3.3V NAND) otherwise, current is more than 1 mA. i.e,. page read current, 25 mA (Typ for 3.3V NAND) > Is this LPM mode always activated? > > > i.e,. > > > > 1) In nand_onfi_detect() to call nand_exec_op() twice by > > nand_read_param_page_op() and annd_read_data_op() > > > > 2) In nand_write_page_xxx to call nand_exec_op() many times by > > nand_prog_page_begin_op(), nand_write_data_op() and > > nand_prog_page_end_op(). > > > > > > Should we consider to add a CS line controller in struct nand_controller > > i.e,. > > > > struct nand_controller { > > struct mutex lock; > > const struct nand_controller_ops *ops; > > + void (*select_chip)(struct nand_chip *chip, int cs); > > }; > > > > to replace legacy.select_chip() ? > > > > No, if really needed, we could add a "macro op done" flag in the nand > operation structure. > Is this "macron op done" flag good for multiple NAND devices on the same bus ? Any other way to control CS# pin? if user application is really care of power consumption, i.e,. loT. > > Thanks, > Miquèl thanks & 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. =====================================================================
Hi Mason, On Tue, 18 Jun 2019 09:24:14 +0800 masonccyang@mxic.com.tw wrote: > Hi Miquel, > > > > > > > > > > > > +static void mxic_nand_select_chip(struct nand_chip *chip, > int > > > > > > > > chipnr) > > > > > > > > > > > > > > > > _select_target() is preferred now > > > > > > > > > > > > > > Do you mean I implement mxic_nand_select_target() to control > #CS ? > > > > > > > > > > > > > > If so, I need to call mxic_nand_select_target( ) to control > #CS ON > > > > > > > and then #CS OFF in _exec_op() due to nand_select_target()<in > > > > > > nand_base,c> > > > > > > > is still calling chip->legacy.select_chip ? > > > > > > > > > > > > You must forget about the ->select_chip() callback. Now it > should be > > > > > > handled directly from the controller driver. Please have a look > at > > > the > > > > > > commit pointed against the marvell_nand.c driver. > > > > > > > > > > I have no Marvell NFC datasheet and have one question. > > > > > > > > > > In marvell_nand.c, there is no xxx_deselect_target() or > > > > > something like that doing #CS OFF. > > > > > marvell_nfc_select_target() seems always to make one of chip or > die > > > > > #CS keep low. > > > > > > > > > > Is it right ? > > > > > > > > Yes, AFAIR there is no "de-assert" mechanism in this controller. > > > > > > > > > > > > > > How to make all #CS keep high for NAND to enter > > > > > low-power standby mode if driver don't use "legacy.select_chip()" > ? > > > > > > > > See commit 02b4a52604a4 ("mtd: rawnand: Make ->select_chip() > optional > > > > when ->exec_op() is implemented") which states: > > > > > > > > "When [->select_chip() is] not implemented, the core is > assuming > > > > the CS line is automatically asserted/deasserted by the driver > > > > ->exec_op() implementation." > > > > > > > > Of course, the above is right only when the controller driver > supports > > > > the ->exec_op() interface. > > > > > > Currently, it seems that we will get the incorrect data and error > > > operation due to CS in error toggling if CS line is controlled in > > > ->exec_op(). > > > > Most of the chips today are CS-don't-care, which chip are you using? > > I think CS-don't-care means read-write operation for NAND device to reside > on the same memory bus as other Flash or SRAM devices. Other devices on > the > memory bus can then be accessed while the NAND Flash is busy with internal > > operations. This capability is very important for designs that require > multiple > NAND Flash devices on the same bus. Yes, we know what CS-dont-care mean, what we want to know is whether your chip supports that or not. And if it supports it, I don't understand why you have a problem when asserting/de-asserting on each ->exec_op() call. > > > > > Is this behavior publicly documented? > > > > CS# pin goes High enter standby mode to reduce power consumption, > i.e,. standby mode w/ CS# keep High, standby current: 10 uA (Typ for 3.3V > NAND) > otherwise, current is more than 1 mA. > i.e,. page read current, 25 mA (Typ for 3.3V NAND) That's not what we were looking for. We want to know what happens when the CS line is de-asserted in the middle of a NAND operation (like read param page). I'd expect the NAND to retain its state so that the operation can be resumed when the CS line is asserted again. If that's not the case that means the NAND is not really CS-dont-care compliant. > > > > Is this LPM mode always activated? > > > > > i.e,. > > > > > > 1) In nand_onfi_detect() to call nand_exec_op() twice by > > > nand_read_param_page_op() and annd_read_data_op() > > > > > > 2) In nand_write_page_xxx to call nand_exec_op() many times by > > > nand_prog_page_begin_op(), nand_write_data_op() and > > > nand_prog_page_end_op(). > > > > > > > > > Should we consider to add a CS line controller in struct > nand_controller > > > i.e,. > > > > > > struct nand_controller { > > > struct mutex lock; > > > const struct nand_controller_ops *ops; > > > + void (*select_chip)(struct nand_chip *chip, int cs); > > > }; > > > > > > to replace legacy.select_chip() ? > > > > > > > No, if really needed, we could add a "macro op done" flag in the nand > > operation structure. > > > > Is this "macron op done" flag good for multiple NAND devices on > the same bus ? It's completely orthogonal to the multi-chip feature, so yes, it should work just fine. > > Any other way to control CS# pin? if user application is really > care of power consumption, i.e,. loT. No, the user is not in control of the CS pin, only the driver can do that. Can you please point us to the datasheet of the NAND you're testing, or something close enough? Thanks, Boris
On Tue, 18 Jun 2019 08:14:36 +0200 Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > > > > > > > > > > How to make all #CS keep high for NAND to enter > > > > > > low-power standby mode if driver don't use "legacy.select_chip()" > > ? > > > > > > > > > > See commit 02b4a52604a4 ("mtd: rawnand: Make ->select_chip() > > optional > > > > > when ->exec_op() is implemented") which states: > > > > > > > > > > "When [->select_chip() is] not implemented, the core is > > assuming > > > > > the CS line is automatically asserted/deasserted by the driver > > > > > ->exec_op() implementation." > > > > > > > > > > Of course, the above is right only when the controller driver > > supports > > > > > the ->exec_op() interface. > > > > > > > > Currently, it seems that we will get the incorrect data and error > > > > operation due to CS in error toggling if CS line is controlled in > > > > ->exec_op(). Oh, and please provide the modifications you added on top of this patch. Right now we're speculating on what you've done which is definitely not an efficient way to debug this sort of issues.
Hi Boris, > > Re: [PATCH v3 2/4] mtd: rawnand: Add Macronix MX25F0A NAND controller > > On Tue, 18 Jun 2019 08:14:36 +0200 > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > > > > > > > > > > > > > How to make all #CS keep high for NAND to enter > > > > > > > low-power standby mode if driver don't use "legacy.select_chip()" > > > ? > > > > > > > > > > > > See commit 02b4a52604a4 ("mtd: rawnand: Make ->select_chip() > > > optional > > > > > > when ->exec_op() is implemented") which states: > > > > > > > > > > > > "When [->select_chip() is] not implemented, the core is > > > assuming > > > > > > the CS line is automatically asserted/deasserted by the driver > > > > > > ->exec_op() implementation." > > > > > > > > > > > > Of course, the above is right only when the controller driver > > > supports > > > > > > the ->exec_op() interface. > > > > > > > > > > Currently, it seems that we will get the incorrect data and error > > > > > operation due to CS in error toggling if CS line is controlled in > > > > > ->exec_op(). > > Oh, and please provide the modifications you added on top of this patch. > Right now we're speculating on what you've done which is definitely not > an efficient way to debug this sort of issues. The patch is to add in beginning of ->exec_op() to control CS# low and before return from ->exec_op() to control CS# High. i.e,. static in mxic_nand_exec_op( ) { cs_to_low(); cs_to_high(); return; } But for nand_onfi_detect(), it calls nand_read_param_page_op() and then nand_read_data_op(). mxic_nand_exec_op() be called twice for nand_onfi_detect() and driver will get incorrect ONFI parameter table data from nand_read_data_op(). thanks & 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. =====================================================================
Hi Mason, masonccyang@mxic.com.tw wrote on Wed, 19 Jun 2019 16:04:43 +0800: > Hi Boris, > > > > > Re: [PATCH v3 2/4] mtd: rawnand: Add Macronix MX25F0A NAND controller > > > > On Tue, 18 Jun 2019 08:14:36 +0200 > > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > > > > > > > > > > > > > > > > How to make all #CS keep high for NAND to enter > > > > > > > > low-power standby mode if driver don't use > "legacy.select_chip()" > > > > ? > > > > > > > > > > > > > > See commit 02b4a52604a4 ("mtd: rawnand: Make ->select_chip() > > > > optional > > > > > > > when ->exec_op() is implemented") which states: > > > > > > > > > > > > > > "When [->select_chip() is] not implemented, the core > is > > > > assuming > > > > > > > the CS line is automatically asserted/deasserted by the > driver > > > > > > > ->exec_op() implementation." > > > > > > > > > > > > > > Of course, the above is right only when the controller driver > > > > > supports > > > > > > > the ->exec_op() interface. > > > > > > > > > > > > Currently, it seems that we will get the incorrect data and > error > > > > > > operation due to CS in error toggling if CS line is controlled > in > > > > > > ->exec_op(). > > > > Oh, and please provide the modifications you added on top of this patch. > > Right now we're speculating on what you've done which is definitely not > > an efficient way to debug this sort of issues. > We really need to see the datasheet of the NAND chip which has a problem and how this LPM mode is advertized to understand what the chip expects and eventually how to work-around it. > The patch is to add in beginning of ->exec_op() to control CS# low and > before return from ->exec_op() to control CS# High. > i.e,. > static in mxic_nand_exec_op( ) > { > cs_to_low(); > > > > cs_to_high(); > return; > } > > But for nand_onfi_detect(), > it calls nand_read_param_page_op() and then nand_read_data_op(). > mxic_nand_exec_op() be called twice for nand_onfi_detect() Yes, this is expected and usually chips don't care. > and > driver will get incorrect ONFI parameter table data from > nand_read_data_op(). > Thanks, Miquèl
On Wed, 19 Jun 2019 16:04:43 +0800 masonccyang@mxic.com.tw wrote: > Hi Boris, > > > > > Re: [PATCH v3 2/4] mtd: rawnand: Add Macronix MX25F0A NAND controller > > > > On Tue, 18 Jun 2019 08:14:36 +0200 > > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > > > > > > > > > > > > > > > > How to make all #CS keep high for NAND to enter > > > > > > > > low-power standby mode if driver don't use > "legacy.select_chip()" > > > > ? > > > > > > > > > > > > > > See commit 02b4a52604a4 ("mtd: rawnand: Make ->select_chip() > > > > optional > > > > > > > when ->exec_op() is implemented") which states: > > > > > > > > > > > > > > "When [->select_chip() is] not implemented, the core > is > > > > assuming > > > > > > > the CS line is automatically asserted/deasserted by the > driver > > > > > > > ->exec_op() implementation." > > > > > > > > > > > > > > Of course, the above is right only when the controller driver > > > > > supports > > > > > > > the ->exec_op() interface. > > > > > > > > > > > > Currently, it seems that we will get the incorrect data and > error > > > > > > operation due to CS in error toggling if CS line is controlled > in > > > > > > ->exec_op(). > > > > Oh, and please provide the modifications you added on top of this patch. > > Right now we're speculating on what you've done which is definitely not > > an efficient way to debug this sort of issues. > > The patch is to add in beginning of ->exec_op() to control CS# low and > before return from ->exec_op() to control CS# High. > i.e,. > static in mxic_nand_exec_op( ) > { > cs_to_low(); > > > > cs_to_high(); > return; > } > > But for nand_onfi_detect(), > it calls nand_read_param_page_op() and then nand_read_data_op(). > mxic_nand_exec_op() be called twice for nand_onfi_detect() and > driver will get incorrect ONFI parameter table data from > nand_read_data_op(). And I think it's valid to release the CE pin between read_param_page_op() (CMD(0xEC)+ADDR(0x0)) and read_data_op() (data cycles) if your chip is CE-dont-care compliant. So, either you have a problem with your controller driver (CS-related timings are incorrect) or your chip is not CE-dont-care compliant.
Hi Miquel, > > > > > > Re: [PATCH v3 2/4] mtd: rawnand: Add Macronix MX25F0A NAND controller > > > > > > On Tue, 18 Jun 2019 08:14:36 +0200 > > > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > > > > > > > > > > > > > > > > > > > How to make all #CS keep high for NAND to enter > > > > > > > > > low-power standby mode if driver don't use > > "legacy.select_chip()" > > > > > ? > > > > > > > > > > > > > > > > See commit 02b4a52604a4 ("mtd: rawnand: Make ->select_chip() > > > > > optional > > > > > > > > when ->exec_op() is implemented") which states: > > > > > > > > > > > > > > > > "When [->select_chip() is] not implemented, the core > > is > > > > > assuming > > > > > > > > the CS line is automatically asserted/deasserted by the > > driver > > > > > > > > ->exec_op() implementation." > > > > > > > > > > > > > > > > Of course, the above is right only when the controller driver > > > > > > > supports > > > > > > > > the ->exec_op() interface. > > > > > > > > > > > > > > Currently, it seems that we will get the incorrect data and > > error > > > > > > > operation due to CS in error toggling if CS line is controlled > > in > > > > > > > ->exec_op(). > > > > > > Oh, and please provide the modifications you added on top of this patch. > > > Right now we're speculating on what you've done which is definitely not > > > an efficient way to debug this sort of issues. > > > > We really need to see the datasheet of the NAND chip which has a > problem and how this LPM mode is advertized to understand what the > chip expects and eventually how to work-around it. okay, got it and thanks. > > > The patch is to add in beginning of ->exec_op() to control CS# low and > > before return from ->exec_op() to control CS# High. > > i.e,. > > static in mxic_nand_exec_op( ) > > { > > cs_to_low(); > > > > > > > > cs_to_high(); > > return; > > } > > > > But for nand_onfi_detect(), > > it calls nand_read_param_page_op() and then nand_read_data_op(). > > mxic_nand_exec_op() be called twice for nand_onfi_detect() > > Yes, this is expected and usually chips don't care. got it and I will try to fix it on my NFC driver. > > > and > > driver will get incorrect ONFI parameter table data from > > nand_read_data_op(). > > > > Thanks, > Miquèl 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. =====================================================================
Hi Boris, > > > > > > Re: [PATCH v3 2/4] mtd: rawnand: Add Macronix MX25F0A NAND controller > > > > > > On Tue, 18 Jun 2019 08:14:36 +0200 > > > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > > > > > > > > > > > > > > > > > > > How to make all #CS keep high for NAND to enter > > > > > > > > > low-power standby mode if driver don't use > > "legacy.select_chip()" > > > > > ? > > > > > > > > > > > > > > > > See commit 02b4a52604a4 ("mtd: rawnand: Make ->select_chip() > > > > > optional > > > > > > > > when ->exec_op() is implemented") which states: > > > > > > > > > > > > > > > > "When [->select_chip() is] not implemented, the core > > is > > > > > assuming > > > > > > > > the CS line is automatically asserted/deasserted by the > > driver > > > > > > > > ->exec_op() implementation." > > > > > > > > > > > > > > > > Of course, the above is right only when the controller driver > > > > > > > supports > > > > > > > > the ->exec_op() interface. > > > > > > > > > > > > > > Currently, it seems that we will get the incorrect data and > > error > > > > > > > operation due to CS in error toggling if CS line is controlled > > in > > > > > > > ->exec_op(). > > > > > > Oh, and please provide the modifications you added on top of this patch. > > > Right now we're speculating on what you've done which is definitely not > > > an efficient way to debug this sort of issues. > > > > The patch is to add in beginning of ->exec_op() to control CS# low and > > before return from ->exec_op() to control CS# High. > > i.e,. > > static in mxic_nand_exec_op( ) > > { > > cs_to_low(); > > > > > > > > cs_to_high(); > > return; > > } > > > > But for nand_onfi_detect(), > > it calls nand_read_param_page_op() and then nand_read_data_op(). > > mxic_nand_exec_op() be called twice for nand_onfi_detect() and > > driver will get incorrect ONFI parameter table data from > > nand_read_data_op(). > > And I think it's valid to release the CE pin between > read_param_page_op() (CMD(0xEC)+ADDR(0x0)) and read_data_op() (data > cycles) if your chip is CE-dont-care compliant. So, either you have a > problem with your controller driver (CS-related timings are incorrect) > or your chip is not CE-dont-care compliant. Understood, I will try to fix it on my NFC driver. And I think CS# pin goes to high is much important for power consumption. thanks & 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. =====================================================================
On Wed, 19 Jun 2019 16:55:52 +0800 masonccyang@mxic.com.tw wrote: > Hi Boris, > > > > > > > > > Re: [PATCH v3 2/4] mtd: rawnand: Add Macronix MX25F0A NAND > controller > > > > > > > > On Tue, 18 Jun 2019 08:14:36 +0200 > > > > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > How to make all #CS keep high for NAND to enter > > > > > > > > > > low-power standby mode if driver don't use > > > "legacy.select_chip()" > > > > > > ? > > > > > > > > > > > > > > > > > > See commit 02b4a52604a4 ("mtd: rawnand: Make > ->select_chip() > > > > > > optional > > > > > > > > > when ->exec_op() is implemented") which states: > > > > > > > > > > > > > > > > > > "When [->select_chip() is] not implemented, the > core > > > is > > > > > > assuming > > > > > > > > > the CS line is automatically asserted/deasserted by the > > > > driver > > > > > > > > > ->exec_op() implementation." > > > > > > > > > > > > > > > > > > Of course, the above is right only when the controller > driver > > > > > > > > > supports > > > > > > > > > the ->exec_op() interface. > > > > > > > > > > > > > > > > Currently, it seems that we will get the incorrect data and > > > > error > > > > > > > > operation due to CS in error toggling if CS line is > controlled > > > in > > > > > > > > ->exec_op(). > > > > > > > > Oh, and please provide the modifications you added on top of this > patch. > > > > Right now we're speculating on what you've done which is definitely > not > > > > an efficient way to debug this sort of issues. > > > > > > The patch is to add in beginning of ->exec_op() to control CS# low and > > > > before return from ->exec_op() to control CS# High. > > > i.e,. > > > static in mxic_nand_exec_op( ) > > > { > > > cs_to_low(); > > > > > > > > > > > > cs_to_high(); > > > return; > > > } > > > > > > But for nand_onfi_detect(), > > > it calls nand_read_param_page_op() and then nand_read_data_op(). > > > mxic_nand_exec_op() be called twice for nand_onfi_detect() and > > > driver will get incorrect ONFI parameter table data from > > > nand_read_data_op(). > > > > And I think it's valid to release the CE pin between > > read_param_page_op() (CMD(0xEC)+ADDR(0x0)) and read_data_op() (data > > cycles) if your chip is CE-dont-care compliant. So, either you have a > > problem with your controller driver (CS-related timings are incorrect) > > or your chip is not CE-dont-care compliant. > > Understood, I will try to fix it on my NFC driver. Before you do that, can you please try to understand where the problem comes from and explain it to us? Hacking the NFC driver is only meaningful if the problem is on the NFC side. If your NAND chip does not support when the CS pin goes high between read_param_page_op() and read_data_op() the problem should be fixed in the core.
Hi Boris, > > > > > Re: [PATCH v3 2/4] mtd: rawnand: Add Macronix MX25F0A NAND > > controller > > > > > > > > > > On Tue, 18 Jun 2019 08:14:36 +0200 > > > > > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > How to make all #CS keep high for NAND to enter > > > > > > > > > > > low-power standby mode if driver don't use > > > > "legacy.select_chip()" > > > > > > > ? > > > > > > > > > > > > > > > > > > > > See commit 02b4a52604a4 ("mtd: rawnand: Make > > ->select_chip() > > > > > > > optional > > > > > > > > > > when ->exec_op() is implemented") which states: > > > > > > > > > > > > > > > > > > > > "When [->select_chip() is] not implemented, the > > core > > > > is > > > > > > > assuming > > > > > > > > > > the CS line is automatically asserted/deasserted by the > > > > > > driver > > > > > > > > > > ->exec_op() implementation." > > > > > > > > > > > > > > > > > > > > Of course, the above is right only when the controller > > driver > > > > > > > > > > > supports > > > > > > > > > > the ->exec_op() interface. > > > > > > > > > > > > > > > > > > Currently, it seems that we will get the incorrect data and > > > > > > error > > > > > > > > > operation due to CS in error toggling if CS line is > > controlled > > > > in > > > > > > > > > ->exec_op(). > > > > > > > > > > Oh, and please provide the modifications you added on top of this > > patch. > > > > > Right now we're speculating on what you've done which is definitely > > not > > > > > an efficient way to debug this sort of issues. > > > > > > > > The patch is to add in beginning of ->exec_op() to control CS# low and > > > > > > before return from ->exec_op() to control CS# High. > > > > i.e,. > > > > static in mxic_nand_exec_op( ) > > > > { > > > > cs_to_low(); > > > > > > > > > > > > > > > > cs_to_high(); > > > > return; > > > > } > > > > > > > > But for nand_onfi_detect(), > > > > it calls nand_read_param_page_op() and then nand_read_data_op(). > > > > mxic_nand_exec_op() be called twice for nand_onfi_detect() and > > > > driver will get incorrect ONFI parameter table data from > > > > nand_read_data_op(). > > > > > > And I think it's valid to release the CE pin between > > > read_param_page_op() (CMD(0xEC)+ADDR(0x0)) and read_data_op() (data > > > cycles) if your chip is CE-dont-care compliant. So, either you have a > > > problem with your controller driver (CS-related timings are incorrect) > > > or your chip is not CE-dont-care compliant. > > > > Understood, I will try to fix it on my NFC driver. > > Before you do that, can you please try to understand where the problem > comes from and explain it to us? Hacking the NFC driver is only > meaningful if the problem is on the NFC side. If your NAND chip does > not support when the CS pin goes high between read_param_page_op() and > read_data_op() the problem should be fixed in the core. I think I have fixed the problem and the root cause is the our NFC's TX FIFO counter do a unnecessary reset in CS control function. Our NFC implement read-write buffer transfer to send command, address and data. A unnecessary reset to TX FIFO will send a command byte out first and this extra command will make something wrong to next operation. For now, doing CS# control in ->exec_op() is OK to me. thanks & 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. =====================================================================
Hi Miquel, > > > > > > > > > How to make all #CS keep high for NAND to enter > > > > > > > > > low-power standby mode if driver don't use > > "legacy.select_chip()" > > > > > ? > > > > > > > > > > > > > > > > See commit 02b4a52604a4 ("mtd: rawnand: Make ->select_chip() > > > > > optional > > > > > > > > when ->exec_op() is implemented") which states: > > > > > > > > > > > > > > > > "When [->select_chip() is] not implemented, the core > > is > > > > > assuming > > > > > > > > the CS line is automatically asserted/deasserted by the > > driver > > > > > > > > ->exec_op() implementation." > > > > > > > > > > > > > > > > Of course, the above is right only when the controller driver > > > > > > > supports > > > > > > > > the ->exec_op() interface. > > > > > > > > > > > > > > Currently, it seems that we will get the incorrect data and > > error > > > > > > > operation due to CS in error toggling if CS line is controlled > > in > > > > > > > ->exec_op(). > > > > > > Oh, and please provide the modifications you added on top of this patch. > > > Right now we're speculating on what you've done which is definitely not > > > an efficient way to debug this sort of issues. > > > > We really need to see the datasheet of the NAND chip which has a > problem and how this LPM mode is advertized to understand what the > chip expects and eventually how to work-around it. > > > The patch is to add in beginning of ->exec_op() to control CS# low and > > before return from ->exec_op() to control CS# High. > > i.e,. > > static in mxic_nand_exec_op( ) > > { > > cs_to_low(); > > > > > > > > cs_to_high(); > > return; > > } > > > > But for nand_onfi_detect(), > > it calls nand_read_param_page_op() and then nand_read_data_op(). > > mxic_nand_exec_op() be called twice for nand_onfi_detect() > > Yes, this is expected and usually chips don't care. As I replied to Boris's email previously. I think I have fixed the problem and the root cause is the our NFC's TX FIFO counter do a unnecessary reset in CS control function. currently, doing CS# control in ->exec_op() is OK to me. In addition, by Jones's comments about MFD, I will re-submit this raw NAND ctlr driver instead of MFD and raw-nand. -----------------------------------------------------------------------> MFD is for registering child devices of chips which offer genuine cross-subsystem functionality. It is not designed for mode selecting, or as a place to shove shared code just because a better location doesn't appear to exist. ------------------------------------------------------------------------< thanks & 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. =====================================================================
Hi, v3 patch is to rename the title of SPI controller driver. "Patch Macronix SPI controller driver according to MX25F0A MFD driver" v2s patches is to support Macronix MX25F0A MFD driver for raw nand and spi controller which is separated form previous patchset: https://patchwork.kernel.org/patch/10874679/ thanks for your review. best regards, Mason Mason Yang (4): mfd: Add Macronix MX25F0A MFD controller driver mtd: rawnand: Add Macronix MX25F0A NAND controller spi: Patch Macronix SPI controller driver according to MX25F0A MFD driver dt-bindings: mfd: Document Macronix MX25F0A controller bindings .../devicetree/bindings/mfd/mxic-mx25f0a.txt | 51 ++++ drivers/mfd/Kconfig | 9 + drivers/mfd/Makefile | 1 + drivers/mfd/mxic-mx25f0a.c | 84 ++++++ drivers/mtd/nand/raw/Kconfig | 6 + drivers/mtd/nand/raw/Makefile | 1 + drivers/mtd/nand/raw/mxic_nand.c | 294 +++++++++++++++++++++ drivers/spi/spi-mxic.c | 275 ++++--------------- include/linux/mfd/mxic-mx25f0a.h | 175 ++++++++++++ 9 files changed, 670 insertions(+), 226 deletions(-) create mode 100644 Documentation/devicetree/bindings/mfd/mxic-mx25f0a.txt create mode 100644 drivers/mfd/mxic-mx25f0a.c create mode 100644 drivers/mtd/nand/raw/mxic_nand.c create mode 100644 include/linux/mfd/mxic-mx25f0a.h