On Fri, Mar 22, 2024 at 4:39 AM Tudor Ambarus <tudor.ambarus@xxxxxxxxxx> wrote: > > Hi, Sam! > > On 3/1/24 00:13, Sam Protsenko wrote: > > I fail to see how this patch fixes anything. Instead it looks to me it > > replaces the (already) correctly implemented logic with incorrect one. > > I opened another thread asking for feedback on whether it's safe to > re-parent the USI MUX to OSCCLK at run-time, find it here: > https://lore.kernel.org/linux-samsung-soc/71df1d6b-f40b-4896-a672-c5f0f526fb1f@xxxxxxxxxx/T/#m588abb87eb5fd8817d71d06b94c91eb84928e06b > > Jaewon came up with the idea on verifying what the downstream clock > driver does. I added some prints in the driver, and indeed the USI MUX > re-parents to OSCCLK on low SPI clock rates in the GS101 case. > > Thus I'll respin this patch set fixing GS101 on low USI clock rates by > re-parenting the USI MUX to OSCCLK. I'll leave exynos850 out if I don't > hear back from you, but I think it deserves the same fix. Allowing SPI > to modify the clock rate of HSI2C/I3C at run-time is bad IMO. > Re-parenting the USI MUX to OSCCLK fixes this problem, HSI2C/I3C will no > longer be affected on low SPI clock rates. > Yes, please leave Exynos850 out of it, if possible. It's fine with me if you send it for gs101, as it's you who is going to maintain that platform further, so it's for the maintainers to decide. I'll refrain from reviewing that particular patch. For Exynos850 driver, I'm convinced the SPI clock derivation is already implemented in the correct way (exactly as it was designed in HW), and doing anything else would be a hack, and frankly this sole fact is already enough of argumentation for me. There is also the whole bunch of use-cases which I think could be affected by using OSCCLK, e.g.: clock signal integrity, runtime PM concerns, possible interference in case of automatic clock control enablement, etc. I don't even want to think about all possible pitfalls which implementation of this non-standard and undocumented behavior could create. So as the only person who currently supports Exynos850 drivers (apart from the maintainers, of course), I would strictly oppose this particular OSCCLK change. > Cheers, > ta
