From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jagan Teki Date: Mon, 7 Jan 2019 00:52:35 +0530 Subject: [U-Boot] [PATCH v5 00/26] clk: Add Allwinner CLK, RESET support In-Reply-To: References: <20181231165927.13803-1-jagan@amarulasolutions.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: u-boot@lists.denx.de On Sun, Jan 6, 2019 at 6:49 PM Andr=C3=A9 Przywara = wrote: > > On 31/12/2018 16:59, Jagan Teki wrote: > > Hi Jagan, > > many thanks for picking this up, I was about to come back to this > myself. I am looking at the pinctrl part at the moment, so good you are > working on the clocks! > > TL;DR: I am good with the first patches, but would like to drop the last > five 5 patches from this series, and discuss the whole tree approach (or > at least the patch) further. > > > Although the previous version[1] is properly handled the clock gates > > with enable and disable management, but this series is trying to add > > some more complex Allwinner CLK architecture by handling parent clock > > and other CLK attributes. > > > > Allwinner Clock control unit comprises of parent clocks, gates, multipl= exers, > > dividers, multipliers, pre/post dividers and flags etc. > > > > So, the U-Boot implementation of ccu has divided into gates and tree. > > gates are generic clock configuration of enable/disable bit management > > and these can be handled via ccu_clock_gate, which is almost same as > > previous version changes. > > So I like this part very much, all those gates and resets are nicely > readable. > Also the removal of the special sunxi USB drivers is very welcome. > So up until patch including 21/26, except 15/26, I am all in. Yes my plan is to do the same. > > > Tree clock has more Allwinner CLK attributes like clock type, fixed clo= cks, > > misc clocks, mp, nk, nkm, nkmp, pre/post div, flags etc and these can be > > managed via ccu_clock_tree. > > Mmmh, this looks quite complex for U-Boot's very limited needs to me. > As far as I can see we basically have a static setup for most devices, > the ones where we actually change the parent clocks are only clock > consumers, namely DE/TCON and MMC. The former chooses a parent once, > based on the calculated clock rate. Only for MMC we switch it from OSC24 > (for the initial SPI-like setup phase) to PLL_PERIPH0 at runtime. > But however this does not really affect the tree, and also we never do > re-parenting. > Under this premises, can't we do this somewhat simpler? At the end of > the day this is U-Boot, not a sophisticated kernel, and we got away with > quite simple clock code so far. Yes, Idea is to go with limited clock setups but the thing is as you know Allwinner deals numerous number of clock attributes and I found it difficult to handle them w/o traversing the re-parenting stuff. I have tried simple version of set_rate in previous version[2] but it wouldn't ended as we desired. These operations need re-parenting, so we have to handle parents in clock attributes. So finally I came up with tree structure. My idea here is to support get and set rate with minimal re-parenting like OSC24, PLL_PERIPH0, PLL_PERIPH1 not adding much code like Linux does. Like GATE macro, TREE macro is also a readable structure with all possible/needed clk attributes at one glance. The code that handle these tree attributes may look confusing, but it is obvious because it would need to handle re-parenting. If SoC has a big CLK architecture like this and to support all or needed clock operations, re-parenting may be one of painful code for SoC driver in U-Boot, Since CLK framework doesn't handle these generically(may be no need) as compared to Linux. and the existing SoC like mediatek clock code does the same. At, this point, I think this tree approach can be possible way to move since it handle all clock attribute types and yes we can only support clock re-parenting which are required for U-Boot. > Meanwhile I am struggling to understand your approach, which looks > impressive on the first glance, but leave me scratching my head when > looking at details. I will comment on 15/26 for the parts I am wondering > about. I was trying to add MMC support and hit some issues. As I said above there is no generic way to handle re-parenting in U-Boot, and we need to traverse based on the SoC Clock architecture. 15/26 handling get_rate, say for example UART request a get_rate. |=3D=3D> OSC_32K UART-->APB2--->|=3D=3D> OSC_24M |=3D=3D> PLL_PH0 =3D=3D=3D> OSC_24M |=3D=3D> PLL_PH0 Each clock tree, has clock type so UART is marked as MISC since it can't not usual clock tree unlike MMC. MMC can be MP clock type and so..on So, if driver requesting get_rate with UART clock, then we need to traverse all the UART clock tree and get the rate based on the type and identified parent clock. UART is MISC so it can directly trigger parent clock abp2, apb2 need to find the parent using mux shift, width, once apb2 found the parent it will go to next as so on. > > On the SPI part: > I am bit puzzled about the last 5 patches dealing with SPI. I see that > they depend on the others, but would like to deal with them separately. True, but these are previous version changes ie reason I grouped it here. anyway will handle accordingly in next versions. > First, patch 25/26 (the sun6i SPI driver) is a bit surprising to find in > here. I guess you want to have an easy user for another clock, but I > believe this should be a separate series. There were other posts already > for a sun6i driver, also I think we should have a unified driver instead > of duplicating most of it again. See Oskari's latest post. Yes, I saw this sun6i driver is lasting for many releases and my plan it to unified the driver w/o any ifdef. > > Secondly: I don't think the SPI clocks are correct. You model them as > simple gates only, kind of ignoring the divider settings. This is what > the current sun4i driver does as well, but: it writes 1U << 31 into the > clock register, which explicitly writes 0 to the other bits, setting 24 > MHz as the input rate. Your approach would read-modify-write the > register, just setting bit 31 and leaving the other ones as they were > before. > So why isn't that using your new tree approach? Looks like a standard > mux/divider clock to me. Yes these yet to implement, ie reason I have just added clk tree only on A64. Idea is to send next version for full support on all SoC types. [2] https://patchwork.ozlabs.org/patch/955993/