Date:   Thu, 2 Feb 2023 17:27:17 +0100
From:   Lorenzo Pieralisi <lpieralisi@kernel.org>
To:     Sergio Paracuellos <sergio.paracuellos@gmail.com>
Cc:     linux-pci@vger.kernel.org, bhelgaas@google.com, robh@kernel.org,
        kw@linux.com, matthias.bgg@gmail.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] PCI: mt7621: Sleep a bit after power on the PCIs phy
 ports
Message-ID: <Y9vkZb963iK1yPVL@lpieralisi>
References: <20221231074041.264738-1-sergio.paracuellos@gmail.com>
 <CAMhs-H8sSvn5bfHde-RT0ejqpCujCWvgRFJLtbLXqPcnvGdgtQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAMhs-H8sSvn5bfHde-RT0ejqpCujCWvgRFJLtbLXqPcnvGdgtQ@mail.gmail.com>
Precedence: bulk

On Mon, Jan 23, 2023 at 09:55:20AM +0100, Sergio Paracuellos wrote:
> Hi!
> 
> On Sat, Dec 31, 2022 at 8:40 AM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > Some devices like ZBT WE1326 and ZBT WF3526-P and some Netgear models need
> > to sleep a bit after call to mt7621_pcie_init_port() driver function to get
> > into reliable boots for both warm and hard resets. The needed time for these
> > devices to always detect the ports seems to be from 75 to 100 milliseconds.
> > There is no datasheet or something similar to really understand why this
> > extra time is needed in these devices but not in most of the boards which
> > use mt7621 SoC. This issue has been reported by openWRT community and the
> > complete discussion is in [0]. The selected time of 100 milliseconds has
> > been also tested in these devices ending up in an always working platform.
> > Hence, properly add the extra 100 milliseconds msleep() function call to make
> > also these devices work.
> >
> > [0]: https://github.com/openwrt/openwrt/pull/11220
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> > Hi Bjorn / Lorenzo,
> >
> > As per Lorenzo comments in v1[0] here it is the patch with changes in commit
> > message and introducing a new definition for this needed extra delay time.
> > I wish you the best new year for you both.
> >
> > Changes in v2:
> > - Add a new define 'INIT_PORTS_DELAY_MS' avoiding to reuse 'PERST_DELAY_MS'.
> > - Rewrite commit message and add a link to openWRT discussion.
> >
> > Previous patch lore link:
> > [0]: https://lore.kernel.org/lkml/20221209071703.2891714-1-sergio.paracuellos@gmail.com/T/
> >
> > Thanks,
> >     Sergio Paracuellos
> >
> >  drivers/pci/controller/pcie-mt7621.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
> > index ee7aad09d627..63a5f4463a9f 100644
> > --- a/drivers/pci/controller/pcie-mt7621.c
> > +++ b/drivers/pci/controller/pcie-mt7621.c
> > @@ -60,6 +60,7 @@
> >  #define PCIE_PORT_LINKUP               BIT(0)
> >  #define PCIE_PORT_CNT                  3
> >
> > +#define INIT_PORTS_DELAY_MS            100
> >  #define PERST_DELAY_MS                 100
> >
> >  /**
> > @@ -369,6 +370,7 @@ static int mt7621_pcie_init_ports(struct mt7621_pcie *pcie)
> >                 }
> >         }
> >
> > +       msleep(INIT_PORTS_DELAY_MS);
> >         mt7621_pcie_reset_ep_deassert(pcie);
> >
> >         tmp = NULL;
> > --
> > 2.25.1
> >
> 
> Gentle ping on this patch :-).

I was about to merge it - there are a couple of things that I don't
like.

First one is the comment, "Sleep a bit" does not mean anything. I'd
rather say "delay port initialization" or something like that, be
precise please.

More importantly, this is a fix but it is not clear from the commit
log. Please report what's wrong in the commit log.

I will rework the commit log and merge it then (if you want to avoid
another version just post an updated log here and I will merge it).

Thanks,
Lorenzo