Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 4 Mar 2022 13:09:11 +0100
From:   Horatiu Vultur <horatiu.vultur@microchip.com>
To:     Michael Walle <michael@walle.cc>
CC:     Lars Povlsen <lars.povlsen@microchip.com>,
        Steen Hegelund <Steen.Hegelund@microchip.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        <UNGLinuxDriver@microchip.com>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-gpio@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Colin Foster <colin.foster@in-advantage.com>
Subject: Re: [PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is
 actually set
Message-ID: <20220304120911.i5rngplg5l6gnnyy@soft-dev3-1.localhost>
References: <20220224161021.2197263-1-michael@walle.cc>
 <20220224161021.2197263-6-michael@walle.cc>
 <20220225092427.jjilv3qo52crsmuw@soft-dev3-1.localhost>
 <2f8a215c67269d639290515931d10b78@walle.cc>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <2f8a215c67269d639290515931d10b78@walle.cc>
Precedence: bulk

The 02/25/2022 12:29, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Hi Michael,

Sorry for late reply.

> 
> Hi Horatiu,
> 
> Am 2022-02-25 10:24, schrieb Horatiu Vultur:
> > The 02/24/2022 17:10, Michael Walle wrote:
> > > Right now, when a gpio value is set, the actual hardware pin gets set
> > > asynchronously. When linux write the output register, it takes some
> > > time
> > > until it is actually propagated to the output shift registers. If that
> > > output port is connected to an I2C mux for example, the linux driver
> > > assumes the I2C bus is already switched although it is not.
> > > 
> > > Fortunately, there is a single shot mode with a feedback: you can
> > > trigger the single shot and the hardware will clear that bit once it
> > > has
> > > finished the clocking and strobed the load signal of the shift
> > > registers. This can take a considerable amount of time though.
> > > Measuremens have shown that it takes up to a whole burst cycle gap
> > > which
> > > is about 50ms on the largest setting. Therefore, we have to mark the
> > > output bank as sleepable. To avoid unnecessary waiting, just trigger
> > > the
> > > single shot if the value was actually changed.
> > 
> > I have tested this patch series on our lan9668 board and it worked
> > fine. Thanks!
> 
> Thanks for testing!
> 
> > I just have few questions:
> > 1. What about other boards/chips that have this sgpio, do they have
> > also
> >    the same issue? Because from what I recall on sparx5 they don't have
> >    this issue. I have seen it only on lan9668.
> 
> Unfortunatly, I don't have any knowledge what IP core is used in
> which SoC. I assumed the lan9668 used the same as the sparx5. If
> that is not the case, we need a new compatible. Do you know if it
> the same?

From what I see, it is the same IP.

> 
> On the sparx5 are there any peripheral who you would actually
> notice that the timing is off?

There are some SFP connected, similar to lan966x. So I don't understand
why that issue is not seen there.

> 
> That being said, I'd assume all the serial gpio controller has
> this "flaw". Simply because a register write won't block until the
> value is shifted out to the shift register and actualy loaded by
> strobing the load signal. It just depends on your burst setting
> (even with bursts off, and clocking all the time) on how large
> the delay is. So you might or might not notice it on a board.
> 
> Could you also have a look at the other supported sgpio block,
> the ocelot and the luton? I don't have any register description
> of these.
> 
> > 2. I remember that I have tried to fix this issue like this [1], but
> >    unfortunetly that was never accepted. Is this something that is
> > worth
> >    at looking at?
> 
> That fix is at the wrong place. You'd need to fix every gpio user, no?
> Instead this tries to fix the controller.

Yes, you are right.

> 
> > 
> > [1]
> > https://patchwork.ozlabs.org/project/linux-i2c/patch/20211103091839.1665672-3-horatiu.vultur@microchip.com/
> 
> -michael

-- 
/Horatiu