Re: [PATCH] mtd: rawnand: mxic: Enable and prepare clocks in probe

[ycllin]

Hi Miquel,

+ Jaime from Macronix.

Jaime will check and confirm this issue.

> Re: [PATCH] mtd: rawnand: mxic: Enable and prepare clocks in probe
> 
> 
> Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote on Tue, 17 Aug 2021
> 12:55:21 +0200:
> 
> > Hi Evgeny,
> > 
> > +Mason from Macronix
> 
> Mason's e-mail bounced, including YouChing in the discussion who may
> also have an answer or perhaps knows who to include in the discussion
> (see below for the context and question).
> 
> > 
> > Evgeny Novikov <novikov@xxxxxxxxx> wrote on Tue, 17 Aug 2021 13:36:03
> > +0300:
> > 
> > > Hi Miquel,
> > > 
> > > On 16.08.2021 11:01, Miquel Raynal wrote:
> > > > Hi Andy,
> > > >
> > > > Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote on Thu, 12 Aug 
2021
> > > > 15:13:10 +0300:
> > > > 
> > > >> On Thursday, August 12, 2021, Evgeny Novikov <novikov@xxxxxxxxx> 
wrote:
> > > >> 
> > > >>> It seems that mxic_nfc_probe() missed invocation of
> > > >>> mxic_nfc_clk_enable(). The patch fixed that. In addition, error 
handling
> > > >>> was refined appropriately. 
> > > >>
> > > >> NAK. Until you provide a deeper analysis, like how this works 
before your
> > > >> change.
> > > >>
> > > >>
> > > >> Please, don’t blindly generate patches, this can even your bot 
do, just
> > > >> think about each change and preferable test on the real hardware.
> > > >>
> > > >> The above is to all your lovely contributions.
> > > >>
> > > >> 
> > > >>> Found by Linux Driver Verification project (linuxtesting.org).
> > > >>>
> > > >>> Signed-off-by: Evgeny Novikov <novikov@xxxxxxxxx>
> > > >>> Co-developed-by: Kirill Shilimanov 
<kirill.shilimanov@xxxxxxxxxx>
> > > >>> Signed-off-by: Kirill Shilimanov <kirill.shilimanov@xxxxxxxxxx>
> > > >>> ---
> > > >>>   drivers/mtd/nand/raw/mxic_nand.c | 16 ++++++++++++----
> > > >>>   1 file changed, 12 insertions(+), 4 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/mtd/nand/raw/mxic_nand.c 
b/drivers/mtd/nand/raw/mxic_
> > > >>> nand.c
> > > >>> index da1070993994..37e75bf60ee5 100644
> > > >>> --- a/drivers/mtd/nand/raw/mxic_nand.c
> > > >>> +++ b/drivers/mtd/nand/raw/mxic_nand.c
> > > >>> @@ -509,9 +509,15 @@ static int mxic_nfc_probe(struct 
platform_device
> > > >>> *pdev)
> > > >>>          if (IS_ERR(nfc->send_dly_clk))
> > > >>>                  return PTR_ERR(nfc->send_dly_clk);
> > > >>>
> > > >>> +       err = mxic_nfc_clk_enable(nfc);
> > > >>> +       if (err)
> > > >>> +               return err; 
> > > > As Andy said, this is not needed.
> > > > 
> > > >>> +
> > > >>>          nfc->regs = devm_platform_ioremap_resource(pdev, 0);
> > > >>> -       if (IS_ERR(nfc->regs))
> > > >>> -               return PTR_ERR(nfc->regs);
> > > >>> +       if (IS_ERR(nfc->regs)) {
> > > >>> +               err = PTR_ERR(nfc->regs);
> > > >>> +               goto fail;
> > > >>> +       }
> > > >>>
> > > >>>          nand_chip = &nfc->chip;
> > > >>>          mtd = nand_to_mtd(nand_chip);
> > > >>> @@ -527,8 +533,10 @@ static int mxic_nfc_probe(struct 
platform_device
> > > >>> *pdev)
> > > >>>          nand_chip->controller = &nfc->controller;
> > > >>>
> > > >>>          irq = platform_get_irq(pdev, 0);
> > > >>> -       if (irq < 0)
> > > >>> -               return irq;
> > > >>> +       if (irq < 0) {
> > > >>> +               err = irq;
> > > >>> +               goto fail; 
> > > > However some reworking is needed in the error path.
> > > >
> > > > That goto statement should be renamed and devm_request_irq() 
should not
> > > > jump to it.
> > > > 
> > > 
> > > We still need some help and clarification from those who are very 
familiar
> with this sort of drivers or/and can test this particular driver. 
> mxic_nfc_clk_enable() is the complementary function for 
mxic_nfc_clk_disable
> (). No other functions invoke 
clk_prepare_enable()/clk_disable_unprepare() in 
> the driver. Unlikely somebody in its environment does that since driver 
> specific clocks are dealt with. At the moment the driver invokes 
> mxic_nfc_clk_disable() on error handling paths in probe, in remove and 
in 
> mxic_nfc_set_freq(). mxic_nfc_clk_enable() is called just by 
mxic_nfc_set_freq
> () that moreover does this after calling mxic_nfc_clk_disable() first. 
So, we 
> did not find any place in the driver that invokes mxic_nfc_clk_enable() 
prior 
> to mxic_nfc_clk_disable(). Basing on this we added mxic_nfc_clk_enable() 
just 
> after getting clocks. As I explained in the previous large e-mail, we 
may be 
> wrong in our understanding of the driver environment or/and at 
specification 
> of requirements being checked. It would be great if you will point out 
on our mistakes.
> > 
> > Enabling the clocks seems to only be needed to access the NAND device
> > and not the registers of the controller. Mason, is this statement
> > right? If this statement is wrong, then your proposal is not entirely
> > wrong in the sense that we must enable the missing clock *before*
> > accessing any register.
> > 
> > Otherwise for the two other clocks, we don't really care to enable 
them
> > before setting the actual frequency in ->setup_interface() (called by
> > nand_scan()) because at this point we don't yet know what timing mode
> > is best. Disabling the clock is not an issue even though it was not
> > enabled in the fist place anyway.
> > 
> > In all cases, the error path is wrong.
> > 
> > Thanks,
> > Miquèl

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.

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