Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Fri, 25 Feb 2022 12:29:50 +0100
From:   Michael Walle <michael@walle.cc>
To:     Horatiu Vultur <horatiu.vultur@microchip.com>
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
In-Reply-To: <20220225092427.jjilv3qo52crsmuw@soft-dev3-1.localhost>
References: <20220224161021.2197263-1-michael@walle.cc>
 <20220224161021.2197263-6-michael@walle.cc>
 <20220225092427.jjilv3qo52crsmuw@soft-dev3-1.localhost>
User-Agent: Roundcube Webmail/1.4.12
Message-ID: <2f8a215c67269d639290515931d10b78@walle.cc>
Precedence: bulk

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?

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

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.

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

-michael