<marex>
rfs613: can you try and push your management to do something about the DRAM situation ?
<marex>
rfs613: it can be done as separate patchset, just ... try ?
<rfs613>
marex: well the situation is they picked a SoC for $reasons (budget, features, etc, no regard for DRAM drivers etc). And now they want to support their product for a long time. Which is all reasonable.
<rfs613>
Many companies in this situation just maintain their own private repos, and heopfully keep them up to date, etc.
<rfs613>
A few have realized that working with upstream is better in the long term.
<marex>
rfs613: except there would be no upstream if companies just start turning it into dumping ground
<rfs613>
I can predict what will happen if I ask about DRAM... they will turn around and ask me how long it would take me to "fix it". Because they really don't know any of the details.
<marex>
rfs613: you can try and coordinate with NishanthMenon
<rfs613>
marex: I'm not familiar with that name
<marex>
rfs613: TI person
ikarso has quit [Quit: Connection closed for inactivity]
<marex>
rfs613: note that I wont block the DRAM controller driver thing, but ... please, try and unscrew the situation if you can
thopiekar has quit [Ping timeout: 255 seconds]
<rfs613>
marex: okay. I guess we could start by trying to talk to the maintainer(s) of the exisitng denali controllers... see if we can figure out if the IP is in fact similar enough that we can share a driver.
thopiekar has joined #u-boot
<rfs613>
though I kind of expect they won't have much inclination to mess about with something that is working... esp if the end product requires certifications etc.
<marex>
its better then inaction
<rfs613>
Nishant is the k3 maitainer?
<rfs613>
get_maintainer gives Bryan Brattlof
<marex>
rfs613: I think NishanthMenon is somehow wired into the TI stuff, yes, but I dont know the details
<NishanthMenon>
rfa613: are you guys talking to cdns?
<rfs613>
NishanthMenon: oh hi there ;-)
<rfs613>
at the moment no... I'm quite a few levels removed from them.
<NishanthMenon>
rfs613: hi. I am upstream kernel maintainer actually... I do dab in UBoot time to time. We do directly get our driver from cdns.
apritzel_ has quit [Ping timeout: 240 seconds]
<NishanthMenon>
the trouble we have had to face (haven't paid close attention to ur specific driver) has been the register incompatible spins that cdns keeps spinning out for every minor feature change we requested. As far as I can gather, there hasn't been a solution..
<rfs613>
NishanthMenon: yes, they do seem to have a huge number of registers, not really documented, and the list differs from k3 to rk3399 to the rzn1 (that i'm working on)
<NishanthMenon>
We have tried to force a level of sanity by appropriate abstraction as possible, but heck, it is really painful.
<NishanthMenon>
Rfs613: hehe... We had to force level of abstraction, but really, the documentation is up to SoC vendor. Marex: Regmap for 4000 registers? I will run out of Sram ;)
<NishanthMenon>
rfs613: TI did a tool sysconfig to restrict config only to mandatory regs per SoC. That reduced the set down.
<rfs613>
NishanthMenon: yeah so we end up using whatever init code the SoC vendor provides... even though it's for similar IP...
<rfs613>
and then as soon as it boots, passes mtest, extended temperature, and emissions testing, then nobody's allowed to touch it ...
<NishanthMenon>
With cdns, it sounds similar IP, lack of register offset level discipline is a killer
zibolo has quit [Ping timeout: 240 seconds]
<NishanthMenon>
K. I gotta run a bit. Might be worth a more drill down. Is rk3339 folks willing to have a conversation?
<rfs613>
NishanthMenon: if it was just offsets, we could handle that with (a big) list of defines... not pretty but avoids runtime overhead (you mentioned limited SRAM)
<NishanthMenon>
I can gladly facilitate a cross soc vendor discussion here or at eloss or whatever elc is called
zibolo has joined #u-boot
<NishanthMenon>
Rfs613 we tried to do with regoffsets in macro(see our dts includes) and tooling to reduce to bare min
<marex>
NishanthMenon: you coming to Prague too ?
<rfs613>
NishanthMenon: great, let's talk for sure.
<NishanthMenon>
marex: hadn't finalized, but I could..
<marex>
NishanthMenon: oh cool, ping me if you need any help/hints/whatever ...
<NishanthMenon>
Marex sure. Thanks
<marex>
/wrt local stuff I mean
<NishanthMenon>
Understood;)
<NishanthMenon>
rfs613: can u see if rk folks are willing to chat cdns driver?
<NishanthMenon>
I can try and get our guys together.. and maybe also see if I can get cdns involved.
<rfs613>
NishanthMenon: I don't have any contacts but I will try emailing recent conributors
<marex>
that would be mighty cool, really
<rfs613>
(contacts at rk I mean)
<NishanthMenon>
rfs613: thanks. Yeah, if you could get rk folks , then it will be a better conversation than having to waste time in reverse engineering.. I have all the design folks near my office, so i can get detailed TI side info
<NishanthMenon>
Gotta run ATM. Kids and park ;)
<rfs613>
NishanthMenon: ok, mine just fell asleep! good chatting, talk again sometime.
<rfs613>
marex: happen to know how much longer the merge window will be open?
<marex>
rfs613: are you worried about the rzn1 patches ?
<marex>
rfs613: just remind me to pick them once you sort out the rest of the stuff and that it should all land in 2023.07
<marex>
rfs613: it is probably OK if it goes in after rc1 too
<rfs613>
marex: well i could repost them now, though the DDR controller bit is still a little rough.
<marex>
rfs613: next week is fine too, though I will be busy until late Friday
<marex>
rfs613: so no rush
<rfs613>
i'm in London next week for Linaro connect, so I'm trying to get stuff done before I leave.
<marex>
have fun
<rfs613>
i'll do my best ;-)
thopiekar has quit [Ping timeout: 252 seconds]
thopiekar_ has joined #u-boot
prabhakarlad has quit [Quit: Client closed]
mmu_man has quit [Ping timeout: 248 seconds]
macromorgan has quit [Read error: Connection reset by peer]
macromorgan has joined #u-boot
persmule has quit [Remote host closed the connection]
goliath has quit [Quit: SIGSEGV]
thopiekar_ has quit [Ping timeout: 255 seconds]
thopiekar has joined #u-boot
macromorgan has quit [Read error: Connection reset by peer]
macromorgan has joined #u-boot
vagrantc has quit [Quit: leaving]
pgreco has joined #u-boot
pgreco_ has quit [Ping timeout: 260 seconds]
zibolo has quit [Quit: No Ping reply in 180 seconds.]
zibolo has joined #u-boot
macromorgan has quit [Read error: Connection reset by peer]
macromorgan has joined #u-boot
sakman_ has joined #u-boot
Wouter0100670440 has joined #u-boot
pgreco_ has joined #u-boot
sakman has quit [Remote host closed the connection]
Wouter010067044 has quit [Quit: Ping timeout (120 seconds)]
<Mis012[m]>
hi, I'm trying to port a ccf driver that makes a considerable use of the ccf features that are apparently "too deep for a bootloader", and I keep on finding really hard to explain things
<Mis012[m]>
for example, one of the above functions implies that clk.id is per-driver, while the other implies it's globally unique
Tooniis[m] has joined #u-boot
<Mis012[m]>
the id that clk_of_xlate_default is reading from the devicetree is certainly not globally unique, and it seems like assuming that it is should result in stuff exploding...
<Mis012[m]>
it also can be zero afaik, and I think I saw some function expecting 0 to mean invalid id
konradybcio[m] has joined #u-boot
Leopold has joined #u-boot
Leopold_ has quit [Ping timeout: 252 seconds]
Leopold has quit [Ping timeout: 255 seconds]
jaganteki has joined #u-boot
mmu_man has quit [Ping timeout: 240 seconds]
Leopold has joined #u-boot
mmu_man has joined #u-boot
kettenis has quit [Ping timeout: 248 seconds]
<marex>
Mis012[m]: which SoC is that ?
<Mis012[m]>
this is common ccf code
<Mis012[m]>
possibly even common clk code for non-ccf drivers as well
<Mis012[m]>
the ccf support was added with the imx driver afaict, and that driver seems to call the get_by_id function with the id gotten from of_xlate
<Mis012[m]>
well, at least one of them
<Mis012[m]>
imx8mn
<Mis012[m]>
I assume they all behave similarly
<Mis012[m]>
marex: the SoC I'm working on is msm8998, I'm in the process of nuking the very ugly pseudo driver model drivers it currently uses and it seems that porting the clk driver from Linux is not even near the complexity of porting the pinctrl driver, which was actually pretty easy
<Mis012[m]>
"it" being qcom SoCs in general, there is no msm8998 specific support in u-boot yet
<marex>
ha
<marex>
so what is exactly the problem with the u-boot simplified CCF framework ?
<marex>
how does the DT reference to clock look like ?
<marex>
clock and one cell of ID I guess ?
<Mis012[m]>
yes
<Mis012[m]>
clock driver
<marex>
looks very similar to the imx behavior, so, what's the problem ?
<Mis012[m]>
there can be multiple clocks per driver
<marex>
OK, sure, that should be fine
<Mis012[m]>
if there's only one, the id is ommited
<Mis012[m]>
marex: the problem is that if you look at those two functions
<Mis012[m]>
one of them clearly assumes that the ID is globally unique
<Mis012[m]>
which will NEVER be the case
<marex>
which one ?
<marex>
where does this code come from ?
<Mis012[m]>
clk_get_by_id
<marex>
oh
<Mis012[m]>
clk-uclass.c
<marex>
use clk_get_by_name then ?
<Mis012[m]>
that's... not the issue
<Mis012[m]>
this function is broken
<marex>
which one, clk_get_by_id ? why ?
<marex>
it seems to be legacy pre-ccf thing
<Mis012[m]>
the only reason that it's not exploding right now that I can think of is that those imx SoCs only ever have one non-zero-aguments clock controller defined in the dts
<Mis012[m]>
marex: I think they're different clocks
ikarso has quit [Quit: Connection closed for inactivity]
<Mis012[m]>
I'm still trying to wrap my head around this
<marex>
that code looks dubious all right
<marex>
well, stick a printf() there and figure out what happens, that's simplest
<Mis012[m]>
I have SWD, it's more work for me to printf stuff :P
<Mis012[m]>
also remember that I don't have the imx :P
<Mis012[m]>
marex: it seems to me the ccf "simplification" does a lot of wild stuff in order to shave every last member of `struct clk`
<Mis012[m]>
Linux ccf has the ops per-clock in the struct clk
<marex>
it does, because that code has to run in SPL
<Mis012[m]>
my SPL has 1MB of SRAM available to it just in cache-as-TCM, how much cache do those imx SoCs have...
<marex>
64kiB to 256 kiB on very generous SoCs
<Mis012[m]>
marex: it seems that the `struct clk` in the parameter list is the one that's filled by of_xlate, while the one it gets with `_by_id` is for some non-driver-model backend clock that is equivalent to some particular `ops`
<Mis012[m]>
I question that whatever they're doing is actually saving RAM
<Mis012[m]>
it's definitely making life miserable for anyone with an SoC that has reasonable amounts of SRAM
<Mis012[m]>
marex: the fix for that function to have any place existing would be this
<Mis012[m]>
but then the imx driver would have to know which device the clock is for, at which point they might as well use those sizeof(void *) bytes to store the ops and avoid all this clownery
<marex>
Mis012[m]: feel free to send patches, just make sure to CC Sean ( Forty-Bot )
<marex>
and keep in mind, the imx does not have too much SRAM, just like majority of other SoCs
<Mis012[m]>
well, I'd rather first discuss this
<marex>
Mis012[m]: you can send RFC patch to start that discussion
<Mis012[m]>
it seems to me that the only reason for all this circus is to avoid having the ops pointer, but clearly you would need a different pointer if you were to fix that function to not be extremely broken
<Mis012[m]>
I'd rather have a response to that assumption before I try to refactor imx code while not even having an imx SoC
<marex>
really, send an RFC patch, that's the best starting point
<marex>
it doesnt have to be perfect, just a draft
<Mis012[m]>
well, there is a LOT of stuff that I'd want to refactor if it turns out that all this mess isn't actually saving RAM
<Mis012[m]>
and I'd rather have it explained to me why it's believed to save RAM over any saner approach
<Mis012[m]>
before I spend a lot of work on a particular saner approach
<marex>
Mis012[m]: you dont have to spend a lot of work on it, just start with something to get the conversation going
<marex>
that generally seems to work better than no patch at all
<Mis012[m]>
guess I can try...
goliath has joined #u-boot
slobodan has quit [Ping timeout: 260 seconds]
thopiekar has quit [Ping timeout: 252 seconds]
thopiekar has joined #u-boot
apritzel_ has joined #u-boot
Leopold has quit [Ping timeout: 255 seconds]
Leopold has joined #u-boot
* Forty-Bot
has been a bit unattentive to u-booting lately, and (more recently) under the weather
<marex>
Forty-Bot: welcome back
<Forty-Bot>
in general terms, I think CCF is not a very good fit for U-Boot, since it adds a lot of complexity by doing dynamically what could be done statically
<Forty-Bot>
and things like tracking clock usage are both not done properly and not necesary
<Forty-Bot>
Mis012[m]: yeah, CCF basically assumes it's the only thing making clocks on the system
<Forty-Bot>
so I would avoid things like clk_get_by_id if I were you
<Forty-Bot>
normally struct clk is a "thick pointer"
thopiekar has quit [Ping timeout: 265 seconds]
Wouter0100670440 has joined #u-boot
<Forty-Bot>
like, it's not really the internal driver representation of the clock
<Forty-Bot>
it's just enough that you can pass it to the driver and the driver can figure out the backing clock
<Mis012[m]>
it does seem that way
<marex>
Forty-Bot: btw clock refcounting would be nice for enable/disable
<Forty-Bot>
but for CCF, there is one "blessed" struct clk which is some device's ->priv which is the actual backing struct clk
<Mis012[m]>
but it's overloading the same struct clk for internal representation it seems
<Forty-Bot>
this really should be a separate struct altogether
<Forty-Bot>
like linux
<Mis012[m]>
I couldn't agree more
<Forty-Bot>
but it's not because whoever implemented this didn't think far enough ahead
<Forty-Bot>
marex: we have it, but it doesn't work because the refcounting is on the thick pointer and not on the backing struct
<Forty-Bot>
so it's basically useless
<Forty-Bot>
IMO if you have to turn off a clock before booting, do it in board code
<Forty-Bot>
we aren't here to save power in U-Boot, just to boot something
<marex>
please no more dumping crap into board code again
<Forty-Bot>
it's not really something you can do generically
<Mis012[m]>
+1
<marex>
Forty-Bot: its not really about power saving, but about turning shared clock on/off correctly
<Mis012[m]>
also, u-boot should clean after itself
<Forty-Bot>
IMO we should leave cleanup to Linux which usually has a better picture of the hardware
<Forty-Bot>
like, a lot of drivers don't have remove()s
<Mis012[m]>
no, Linux should start with clean state
<Forty-Bot>
so the clocks never get disabled
<marex>
Forty-Bot: nope, Linux must be started with hardware in queiscent state, else Linux can shoot itself in the leg with e.g. DMA that would rewrite some piece of memory
<marex>
Forty-Bot: been there, with USB, nope, really, nope
<Forty-Bot>
sure, but we don't need to disable clocks for that
<marex>
Forty-Bot: that really depends
<Forty-Bot>
or if we do, it should be handled explicitly either in board code or in the affected driver's remove
<Mis012[m]>
x86 is the prime example of how not to do stuff
<marex>
Forty-Bot: in driver remove obv
<Forty-Bot>
the current refcounting is just broken
<marex>
Forty-Bot: I didnt even realize we had clock refcounting, so, yeah
<Forty-Bot>
it should also be opt-in or something
<Mis012[m]>
the current refcounting is broken, but shall be fixed along with separating out the backing structure
<Forty-Bot>
since 90% of clocks aren't shared
<Mis012[m]>
Forty-Bot: would you agree that the whole not having ops per clock doesn't actually save RAM unless you have the utterly broken get_by_id which relies on there only being one clock driver?
<Forty-Bot>
can you elaborate a bit on what you mean by "ops per clock"
<Mis012[m]>
Linux has set_rate etc ops per each clock's backing struct
<Mis012[m]>
meanwhile in u-boot imx creates pseudo drivers for each instsnce of ops that the Linux driver had
<Forty-Bot>
ops per clock is definitely something CCF should have
<Forty-Bot>
the whole driver per clock is nuts
<Mis012[m]>
I'm glad we agree on this
<Forty-Bot>
like, we could easily remove rate/flags/enable_count and stick in an optional ops
<Forty-Bot>
and maybe a priv
<Mis012[m]>
well, flags are needed
<Forty-Bot>
they are not
<Forty-Bot>
there is one flag and no one uses it
<Mis012[m]>
the set_rate_parent flag is quite important to me
<Mis012[m]>
and it should probably be handled by ccf
<Mis012[m]>
enable_count should be a thing at least optionally
<Forty-Bot>
set_rate_parent should really be internal to CCF to avoid confusion
<Forty-Bot>
enable_count is not useful
<Forty-Bot>
because of the thick pointer/blessed struct confusion
<Mis012[m]>
but that will be fixed
<Mis012[m]>
at which point it will be useful
<Forty-Bot>
it's not something which should be in struct clk
<Forty-Bot>
needs to go somewhere else
<Mis012[m]>
rate has no place being cached, fwiw neither do other things
<Mis012[m]>
Forty-Bot: not in the thick pointer onr
<Forty-Bot>
yes, especially since we don't keep track of this stuff like Linux
<Mis012[m]>
well, you can call get_rate every time
<Mis012[m]>
so no point caching stuff
<Forty-Bot>
yeah, IMO if it's something expensive like I2C the driver should do the caching
<Mis012[m]>
could probably abuse regmap caching for i2c
<Mis012[m]>
at least someone has an incentive to support non-mmio regmap backends :P
<Mis012[m]>
does spmi also not use regmap then? seems sad