Hi Neil, On Wed, Dec 13, 2023 at 05:44:28PM +0100, Neil Armstrong wrote: > Le 13/12/2023 à 09:36, Maxime Ripard a écrit : > > On Wed, Dec 13, 2023 at 08:43:00AM +0100, Uwe Kleine-König wrote: > > > On Wed, Dec 13, 2023 at 08:16:04AM +0100, Maxime Ripard wrote: > > > > On Tue, Dec 12, 2023 at 06:26:37PM +0100, Uwe Kleine-König wrote: > > > > > clk_rate_exclusive_get() returns zero unconditionally. Most users "know" > > > > > that and don't check the return value. This series fixes the four users > > > > > that do error checking on the returned value and then makes function > > > > > return void. > > > > > > > > > > Given that the changes to the drivers are simple and so merge conflicts > > > > > (if any) should be easy to handle, I suggest to merge this complete > > > > > series via the clk tree. > > > > > > > > I don't think it's the right way to go about it. > > > > > > > > clk_rate_exclusive_get() should be expected to fail. For example if > > > > there's another user getting an exclusive rate on the same clock. > > > > > > > > If we're not checking for it right now, then it should probably be > > > > fixed, but the callers checking for the error are right to do so if they > > > > rely on an exclusive rate. It's the ones that don't that should be > > > > modified. > > > > > > If some other consumer has already "locked" a clock that I call > > > clk_rate_exclusive_get() for, this isn't an error. In my bubble I call > > > this function because I don't want the rate to change e.g. because I > > > setup some registers in the consuming device to provide a fixed UART > > > baud rate or i2c bus frequency (and that works as expected). > > > > I guess it's a larger conversation, but I don't see how that can > > possibly work. > > > > The way the API is designed, you have no guarantee (outside of > > clk_rate_exclusive_*) that the rate is going to change. > > > > And clk_rate_exclusive_get() doesn't allow the rate to change while in > > the "critical section". > > > > So the only possible thing to do is clk_set_rate() + > > clk_rate_exclusive_get(). > > There's clk_set_rate_exclusive() for this purpose. Sure. But that assumes you'll never need to change the rate while in the critical section. > > So there's a window where the clock can indeed be changed, and the > > consumer that is about to lock its rate wouldn't be aware of it. > > > > I guess it would work if you don't care about the rate at all, you just > > want to make sure it doesn't change. > > > > Out of the 7 users of that function, 3 are in that situation, so I guess > > it's fair. > > > > 3 are open to that race condition I mentioned above. > > > > 1 is calling clk_set_rate while in the critical section, which works if > > there's a single user but not if there's multiple, so it should be > > discouraged. > > > > > In this case I won't be able to change the rate of the clock, but that > > > is signalled by clk_set_rate() failing (iff and when I need awother > > > rate) which also seems the right place to fail to me. > > > > Which is ignored by like half the callers, including the one odd case I > > mentioned above. > > > > And that's super confusing still: you can *always* get exclusivity, but > > not always do whatever you want with the rate when you have it? How are > > drivers supposed to recover from that? You can handle failing to get > > exclusivity, but certainly not working around variable guarantees. > > > > > It's like that since clk_rate_exclusive_get() was introduced in 2017 > > > (commit 55e9b8b7b806ec3f9a8817e13596682a5981c19c). > > > > Right, but "it's always been that way" surely can't be an argument, > > otherwise you wouldn't have done that series in the first place. > > > > > BTW, I just noticed that my assertion "Most users \"know\" that > > > [clk_rate_exclusive_get() returns zero unconditionally]" is wrong. As of > > > next-20231213 there are 3 callers ignoring the return value of > > > clk_rate_exclusive_get() and 4 that handle (imaginary) returned errors. > > > I expected this function to be used more extensively. (In fact I think > > > it should be used more as several drivers rely on the clk rate not > > > changing.) > > > > Yes, but also it's super difficult to use in practice, and most devices > > don't care. > > > > The current situation is something like this: > > > > * Only a handful of devices really care about their clock rate, and > > often only for one of their clock if they have several. You would > > probably get all the devices that create an analog signal somehow > > there, so audio, display, i2c, spi, uarts, etc. Plus the ones doing > > frequency scaling so CPU and GPUs. > > > > * CPUs and GPUs are very likely to have a dedicated clock, so we can > > rule the "another user is going to mess with my clock" case. > > > > * UARTs/i2c/etc. are usually taking their clock from the bus interface > > directly which is pretty much never going to change (for good > > reason). And the rate of the bus is not really likely to change. > > > > * SPI/NAND/MMC usually have their dedicated clock too, and the bus > > rate is not likely to change after the initial setup either. > > > > So, the only affected devices are the ones generating external signals, > > with the rate changing during the life of the system. Even for audio or > > video devices, that's fairly unlikely to happen. And you need to have > > multiple devices sharing the same clock tree for that issue to occur, > > which is further reducing the chances it happens. > > Well, thanks for HW designers, this exists and some SoCs has less PLLs than > needed, and they can't be dedicated for some hw blocks. > > > > > Realistically speaking, this only occurs with multi-head display outputs > > where it's somewhat likely to have all the display controllers feeding > > from the same clock, and the power up of the various output is done in > > sequence which creates that situation. > > > > And even then, the clk_rate_exclusive_* interface effectively locks the > > entire clock subtree to its current rate, so the effect on the rest of > > the devices can be significant. > > > > So... yeah. Even though you're right, it's trying to address a problem > > that is super unlikely to happen with a pretty big hammer that might be > > too much for most. So it's not really surprising it's not used more. > > Honestly I tried my best to find a smart way to set the DSI clock tree > with only 2 endpoints of the tree, but CCF will explore all possibilities > and since you cannot set constraints, locking a sub-tree is the smartest > way I found. > In this case, the PLL is common between the DSI controller and video generator, > so to keep the expected clock ratio, the smart way is to set the freq on > one side, lock the subtree and set the rate on the other side. > An API permitting to set multiple rates to multiple clocks in a single call > would be the solution, but not sure if we could possibly write such algorithm. Sure, and it's working great for some SoCs, so it was a good solution for the problem you had at the time. For some other SoCs it's not working that well however, so we need to improve things for those. Maxime
Attachment:
signature.asc
Description: PGP signature
