<rockosov>
narmstrong: xdarklight Hello! Do you think it is possible to merge the a1 clock driver in the current merge window, or is it too late, and we need to wait for the 6.5 merge window? We have many device tree changes in our hands and some wifi-related patches that are currently blocked by the clock driver review. Additionally, we are not sending the clock driver to the Uboot because we are
<rockosov>
unsure about the final clock bindings names.
<rockosov>
xdarklight: I believe I have tagged the right person, Martin Blumenstingl :-)
<f_>
By the way. rockosov: Want to know the current status of my TF-A fork?
<f_>
But if you and your team got much further than me then we could join forces as you said before =)
<rockosov>
f_: We didn't bringup your fork on our side, but it's interesting for us. I think we can try on the next week
Daaanct12 has quit [Ping timeout: 256 seconds]
<rockosov>
f_: BTW, what's SoC SKU do you use? We have devices based on A311D, S905Y2, A113X, A113L, S905D3
<f_>
I only have a S905 set-top box.
<f_>
So this port is only tested on gxbb.
<rockosov>
f_: g12a (or g12b) I'm always confusing them
<rockosov>
gxbb, oky
<narmstrong>
rockosov: since jbrunet seems to be off right now, I have no idea
Daaanct12 has joined #linux-amlogic
<f_>
Status: My fork can go as far as trying to load an image (and then fail)
<rockosov>
f_: I think, we can try your fork on A311D, S905Y2. It's very near each other
<f_>
Maybe. Good luck on that.
<f_>
Although I'm pretty sure Amlogic uses the same BL2 codebase since gxbb.
<xdarklight>
rockosov: I'm Martin, indeed. do you mean 6.4 or 6.5 with "current merge window"? 6.4 won't be possible as new feature merges typically stop at -rc6 stage of the previous version. since we're at 6.4-rc3 the next possible version is 6.5 (and I think that's achievable)
<f_>
(didn't look much into it. Right now it's just imported from Amlogic's old BL2 sources)
<rockosov>
xdarklight: Good to know, now I'm sure that it's your nickname :-) Sorry for misunderstanding :-)
<rockosov>
xdarklight: About SET_NO_REPARENT flags... We can discuss it here, if you don't mind
<xdarklight>
rockosov: yep, that would be great. it's bank holiday in Germany today so I'll ping you again after lunch - then we can discuss it and hopefully come to a conclusion
<rockosov>
f_: BTW, what's bl2/bl31/bl30 do you use? Are you building them from the atf repo as well?
<f_>
I use the proprietary BL30 for now.
<f_>
BL31 is built from upstream TF-A.
<f_>
And, well, BL2 is the TF-A one I'm trying to port.
<rockosov>
xdarklight: Ah, no problem! Have a wonderful holiday! Feel free to message me whenever you have a moment!
<rockosov>
f_: hmmm, I thought bl30 is also a build artifact from atf project
<f_>
Nah. BL30 actually (if I remember correctly) isn't part of TF-A, but is rather a separate firmware on its own.
<rockosov>
f_: got it, thank you!
<f_>
No problem! =)
<f_>
rockosov: Also. My BL2 does have a very unreliable DRAM init.
<rockosov>
f_: Yep, this theme is in the dark room for me... Usually, we ask Amlogic to provide DDR trainings and tune DDR FW for us...
<rockosov>
xdarklight: BTW, Amlogic provided some information about AUDDDS clock, I will post it to the mail thread. But it's still so poor :-)
<f_>
rockosov: The thing is, I almost didn't touch Amlogic's code.
<xdarklight>
rockosov: so, about this RTC clock. I think it also exists on the G12A/SM1 series and we use it to generate a 32kHz clock for HDMI CEC
<xdarklight>
rockosov: is there any case where the RTC clock should not run at 32kHz?
<xdarklight>
(on your A1 SoC that is)
<rockosov>
xdarklight: In the real life, I suppose, no. But from HW perspective you can generate any rate with m1/m2/n1/n2 parameters
<rockosov>
xdarklight: To be honest, we have tested RTC clock for wifi 32k clkin, but in the end of all, we have used pwm :-)
<rockosov>
xdarklight: But it's because of we have already frozen HW layout and we can't use GENCLK for wifi 32k clkin, pinmuxing HW setup is different
<xdarklight>
rockosov: I see, Amlogic's pin muxing is simple so you can't have any signal on any pin that you want
<xdarklight>
rockosov: let's say that there's this imaginary board design where the RTC clock is used for the wifi 32kHz clock input (through the GENCLK output). in this case the RTC clock should be set to 32kHz. but why do we now forbid pwm_a_sel to choose the best possible parent for any given PWM duty cycle/period ?
<xdarklight>
in your patch pwm_a_sel has .flags = CLK_SET_RATE_NO_REPARENT - my understanding is to make sure that CCF won't try to change the RTC clock rate. but even with .flags = 0 it wouldn't because that means CLK_SET_RATE_PARENT is not set, so it will not try to modify the RTC (or any other parent) clock rate
<rockosov>
xdarklight: Am I understanding you correctly? In the scenario you described with the imaginary board, it's possible to set pwm_a_sel parent to RTC clock using assigned-clocks and then set the 32k rate to appropriate PWM, without enabling the SET_RATE_PARENT flag. If we don't enable this flag, it's likely that the CCF will choose the RTC clock since it already has the required rate of 32k.
<rockosov>
xdarklight: And your approach is to remove all flags from RTC children and move their CLKIDs to the public part of the bindings to allow device tree developers to set their parent directly.
<xdarklight>
rockosov: wait, let's go step by step (I think this is the part I'm struggling to understand): in which case would you want pwm_a_sel to use RTC as parent?
<rockosov>
xdarklight: Unfortunately, I don't have a real example to provide. My assumption is that the RTC clock is more efficient for generating 32k, and in general, I would prefer to be able to set it as the parent permanently. However, this is just a theoretical perspective.
<rockosov>
xdarklight: There is a real certain example for GENCLK. Since GENCLK is a debug clock, the SET_NO_REPARENT flag must be used.
<xdarklight>
rockosov: I'm with you: having SET_NO_REPARENT for GENCLK is fine for me
<xdarklight>
rockosov: let's look at the updated PWM calculation from Heiner's series: freq = div64_u64(NSEC_PER_SEC * (u64)0xffff, period);
<xdarklight>
rockosov: so for a 32kHz parent clock to be applicable we need a period of 1999969482ns (that's almost 2 seconds)
<xdarklight>
rockosov: for the VDDCPU regulator on my G12A board a PWM period of 1250ns is used. I'm not saying that nobody will ever use a 2 second period, but I have not come across across such a large number yet. so I don't think that we should optimize for that case
vagrantc has joined #linux-amlogic
<rockosov>
xdarklight: Per my understanding, there different things: input clock for pwm and output signal on pwm pins. For example, currently we are generating pwm 32k rate using pwm-clock driver, which setup output pwm signal using period and duty
<rockosov>
xdarklight: For example, input clock may be 32k and we can divide it if needed
<rockosov>
xdarklight: In the absence of real-life examples, it appears that the RTC children no-reparenting approach may not be the most optimal choice. Therefore, it may be preferable to remove the NO_REPARENT flags, with the exception of GENCLK.
<xdarklight>
rockosov: ah your "use RTC as input clock to let the PWM controller generate a 32kHz based off of it" example is an interesting one
<xdarklight>
rockosov: can't we improve the PWM controller driver so it would be able to automatically pick the RTC parent? right now Heiner's calculation is what I mentioned above. we could add freq = NSEC_PER_SEC / period and if clk_round_rate(channel->clk, freq) == freq then we don't bother multiplying with 0xffff
<xdarklight>
rockosov: in your case that would be: 32kHz = 30518ns, so: 1000000000 / 30518 = 32768 (which includes integer precision rounding)
<rockosov>
xdarklight: If we discover a compelling reason to set the NO_REPARENT flag, we can modify the driver accordingly. For now, it's just theoretical perspective, as I mentioned
<rockosov>
xdarklight: Oh, good idea, yes, we can do it
<rockosov>
xdarklight: But what's about other children? A1 has other clock objects which can be inherited from RTC. Do we need to handle them specially... good question
<xdarklight>
rockosov: yep, that's a good question :-) let's see what we have: pwm (already discussed), genclk (already discussed), dsp, cec, sys
<xdarklight>
rockosov: I suspect that CEC is "don't care" because it has the same dual divider setup for itself. so whether we use RTC or it's own 32kHz clock is probably not relevant
<xdarklight>
rockosov: for the "sys" clock: I'm not sure if we can even change it at runtime
<rockosov>
xdarklight: For CEC I agree, it seems like that
<rockosov>
xdarklight: I didn't set this flag for sys clock. It's RO, you are totally right
<rockosov>
xdarklight: About DSP.. It's a full-blown mystery for me :-) Our A113L SKU doesn't have it, so I don't know additional information about that..
<xdarklight>
rockosov: okay that leaves the dsp clock: I'd be surprised if sub 1MHz clocks are useful for a DSP. not sure if Amlogic uses a Cadence/Tensilica HiFi 4 DSP like the NXP i.MX RT600: the NXP application note states that the DSP can be run with up to 600MHz - https://www.nxp.com/docs/en/application-note/AN12762.pdf
<xdarklight>
(I've no idea if that code is also for a Tensilica DSP though)
<rockosov>
xdarklight: So, finally, we need to remove NO_REPARENT flag for all clock objects, except GENCLK. But what's about SET_RATE_PARENT? Do we really need it or not? Maybe, it's safer to disable parent rate propagation for such RTC children
<xdarklight>
rockosov: disabling rate propagation (= not using SET_RATE_PARENT) is fine for me, we use this approach for various other clocks already
<xdarklight>
rockosov: then I think we should have two next steps: 1) discussing this idea of having pwm-meson pick a more precise parent with Heiner and 2) removing CLK_SET_RATE_NO_REPARENT from the clock patches
<xdarklight>
rockosov: it would be great if we can start with the pwm-meson driver discussion. in parallel I'll the time to complete your clock driver patch review (my goal is to do that until Sunday evening). then I'll ask you to send what will be the final v16 series
<f_>
Now. io_read is just going to run handle->entity->dev_handle->funcs->read().
<f_>
The question is, what is read()?
<f_>
By which I mean, which function is it actually running?
<rockosov>
xdarklight: Alright, that seems like a good option. I will share our idea with Heiner for review and initiate a discussion.
<xdarklight>
rockosov: great, thank you!
<xdarklight>
rockosov: also thanks for pinging me on IRC as well as for the nice discussion :-)
<rockosov>
xdarklight: Thank you for your time and support, really appreciate it! What do you think, it's okay to share a link to our IRC discussion backup in the next v16 series in order to provide an explanation for the flag changing?