(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!) 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
