Re: [PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is actually set

[Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>]

The 03/04/2022 13:46, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Horatiu,
> 
> Am 2022-03-04 13:09, schrieb Horatiu Vultur:
> > 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 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.
> 
> Good to know.
> 
> > > 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.
> 
> Is there an I2C mux, too? 

It looks like also on sparx5 is an i2c mux[1]. The only difference is
that is controlled by pinctrl and not by SGPIO.

> Or just the SFP signals connected to
> the SGPIO? What I was seeing is that during probing of the SFPs
> the SFPs EEPROM is read and when the I2C mux is controlled by the
> SGPIO it will switch too late - or even worse, in the middle of
> a transaction. I would speculate the timing isn't that critical
> with signals just connected directly to the SFP.
> 
> In any case, I think it is pretty clear that it cannot work the
> way it is right now, no? See the very next paragraph...

Yes, I agree, this needs to be fixed.

> 
> > > 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.
> 
> .. here
> 
> -michael

[1] https://elixir.bootlin.com/linux/v5.17-rc6/source/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi#L404

-- 
/Horatiu