On Fri, Dec 07, 2018 at 07:09:56PM +0200, Andy Shevchenko wrote: > (Cc: Rafael and linux-acpi. Rafael, a short context for you, there is a device > connected externally to the IoT ACPI-enabled x86-board via I2C bus. The driver > needs a clock frequency used inside that device to perform correctly, since > ACPI has not yet concept of clock provider I proposed to add a property widely > used for other IPs in a similar way, but DT people strongly reject my > approach. If you may have a chance to look and maybe suggest the approach > which satisfies both sides, it would be really nice!) One person does not equate to 'DT people'. Actually, I'm fine with this change. I don't think we should necessarily require using the clock binding in simple cases just needing to know what a particular clock frequency is. However, ACPI not having a clock provider is not an argument for DT bindings. The whole notion of sharing them in the first place is flawed. Rob > > On Fri, Dec 07, 2018 at 06:05:14PM +0200, Vladimir Zapolskiy wrote: > > On 12/07/2018 03:48 PM, Andy Shevchenko wrote: > > > On Fri, Dec 07, 2018 at 10:18:26AM +0200, Vladimir Zapolskiy wrote: > > >> On 12/05/2018 04:11 PM, Andy Shevchenko wrote: > > >>> For the platforms which have no clock provider for the sc16is7xx type of UART, > > >>> introduce an alternative clock-frequency property which would be used instead. > > >> > > >> the subject has a typo in 'clock-frequence', then can you please tell me more, > > >> how is it possible that an SC16IS7xx IC has no clock provider connected to it? > > > > > > I better ask Grigorii about this, since I have no hardware at my possession. > > > > > >> And if there is one, then please just describe it in device tree as well. > > > > > > Tell me how to do this for ACPI? > > > > > > > I didn't grasp the connection between your update of IC device tree bindings > > and ACPI, please elaborate in the context of updating device tree bindings > > documentation and supported properties. > > OK. > > > I do care about purity of device tree bindings of SC16IS7xx IC, which is > > found on some of my boards with description in DTB, that's why I object to > > this series. > > I also support the purity of many drivers and modules in the software (Linux > kernel), but because of existing hardware and customers I can't reject their > needs. That's why, unfortunately, the drivers' code is full of quirks. > > If I would object on each of those cases, I would end up with the OS which > supports almost nothing. > > > >>> +- clock-frequency: The source clock frequency for the IC. > > >>> > > >> > > >> I strongly dislike this change, I'm inclined to cast a NAK to the series. > > > > > > To be productive, please propose the alternative, otherwise your NAK is nothing > > > to do with a real hardware and approach I proposed. > > > > As I've said 'clock-frequency' property is not a hardware property of SC16IS7xx > > IC, it is a hardware property of some external hardware component, it may > > provide a volatile clock rate, which you miss, and it should be described > > separately in DT. > > I disagree with you. > > Crystal or PLL or another clock source, even being external component, still is > internal to the blackbox called UART-I2C adapter. > > > The current approach with 'clocks' property addresses all cases perfectly, > > even if your change is an attempt to solve some actual problem, you haven't > > managed to describe it in the commit message. > > I will fix the commit message. I agree it's not perfect. > > > NAK for the added property, which makes obtaining of the clock supplier > > frequency equivocal. > > Your NAK is nothing w/o proposed alternative which would work in a case of ACPI > enabled platform. > > > >> 1. 'clock-frequency' is a very specific device tree property, in my opinion > > >> its presence is justified on sort of clock provider devices only (like I2C > > >> controllers), unfortunately the property was added to a number of device > > >> tree bindings improperly, mainly it was done before introduction of > > >> "assigned-clocks" and "assigned-clock-parents" properties in CCF, and then > > >> it was blindly copied. > > > > > > OK, I will wait for your patch to remove such from, for example, 8250_dw.c > > > where same problem had been targeted in the same way. > > > > I'm not interested to fix legacy device tree binding issues added in 2011, > > equally I'm not going to close my eyes on right the same issues, when someone > > attempts to spread them further today. > > Any proposal, be constructive, please. > > > >> 2. SC16IS7xx type of UARTs is a regular clock consumer, ICs always have a valid > > >> clock provider connected to XTAL1 (and XTAL2 in case of a connected > > >> crystal oscillator), thus, if needed, the driver can get input clock rate > > >> by calling standard clk_get_rate(), so the presence of the required 'clocks' > > >> property is sufficient. > > > > > > So what? > > > There is a hardware, there is a clock provider hidden in it. How you would > > > describe it? Platform data? Why? > > > > > > > What do you mean by 'hardware'? PCB, SC16IS7xx IC or something else? > > > > What do you mean by 'a clock provider hidden in it'? > > > > Please find the hidden clock provided and describe it in a proper way, for DTS > > changes please reference to Documentation/devicetree/bindings/clock contents. > > Again, try to think about the world not being just arm + dts. > > > >> 3. In some very specific corner cases it might be needed to add another > > >> "assigned-clocks" and "assigned-clock-parents" properties to a particular > > >> device node on a particular board, but their explicit description in device > > >> tree bindings is not needed. > > > > > > Can DT people once in life think outside the box?! > > > > > > > The rhetorical question doesn't sound like a nice supporting argument of > > your change. > > > > Please don't slip into arrogance, and please concentrate on technical aspects. > > Please, read your sentence above and tell me what the alternative I have? > > -- > With Best Regards, > Andy Shevchenko > >
