| Message ID | 20170503031658.29299-4-wens@csie.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi, On Wed, May 03, 2017 at 11:16:53AM +0800, Chen-Yu Tsai wrote: > The MMC clocks on newer SoCs, such as the A83T and H3, support the > "new timing mode". Under this mode, the output of the clock is divided > by 2, and the clock delays no longer apply. > > Due to how the clock tree is modeled and setup, we need to model > this function in two places, the master mmc clock and the two > child phase clocks. In the mmc clock, we can easily model the > mode bit as an extra variable post-divider. In the phase clocks, > we check the bit and return -ENOTSUPP if the bit is set, signaling > that the phase clocks are not to be used. > > This patch introduces a class of phase clocks that checks the > timing mode bit. We've been over this quite some times already... How do you retrieve the mode used in the clock so that you can also switch the MMC driver in the new mode? And do you prevent that from happening on the DT we already have that use the old clock drivers that do not support this new timing at all? Maxime
Hi, On Thu, May 4, 2017 at 4:34 AM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Hi, > > On Wed, May 03, 2017 at 11:16:53AM +0800, Chen-Yu Tsai wrote: >> The MMC clocks on newer SoCs, such as the A83T and H3, support the >> "new timing mode". Under this mode, the output of the clock is divided >> by 2, and the clock delays no longer apply. >> >> Due to how the clock tree is modeled and setup, we need to model >> this function in two places, the master mmc clock and the two >> child phase clocks. In the mmc clock, we can easily model the >> mode bit as an extra variable post-divider. In the phase clocks, >> we check the bit and return -ENOTSUPP if the bit is set, signaling >> that the phase clocks are not to be used. >> >> This patch introduces a class of phase clocks that checks the >> timing mode bit. > > We've been over this quite some times already... > > How do you retrieve the mode used in the clock so that you can also > switch the MMC driver in the new mode? This part is covered. As mentioned above, the phase clks will return -ENOTSUPP if the mmc clk is set in the new timing mode. The MMC driver would try getting the current phase, check if it fails, and if it does, realizes the clk is in new timing mode, and will switch the mmc controller to it as well. I think this semantic fits the hardware well enough, though it might be confusing without the accompanying comments. See this commit for an example on how it should work. https://github.com/wens/linux/commit/41e386f1f945a0a135a20a00601b977a001353da > And do you prevent that from happening on the DT we already have that > use the old clock drivers that do not support this new timing at all? I admit this is not taken care of in this patch. Mostly because there is no existing DT for the A83T to cater to. For the H3 and other SoCs that do, we need some way of asking the CCU to switch the mode. IIRC the output and sample clocks do not support a phase delay of 0. So what if we design the semantic for this class of clocks as, if phase == 0, switch to new timing mode, otherwise use old timing mode? That would allow us to support this without adding extra functions, and kind of matches what the hardware does. If you're using the new timing mode, the delays in the mmc clk are bypassed. Regards ChenYu -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 04, 2017 at 11:08:43AM +0800, Chen-Yu Tsai wrote: > Hi, > > On Thu, May 4, 2017 at 4:34 AM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > Hi, > > > > On Wed, May 03, 2017 at 11:16:53AM +0800, Chen-Yu Tsai wrote: > >> The MMC clocks on newer SoCs, such as the A83T and H3, support the > >> "new timing mode". Under this mode, the output of the clock is divided > >> by 2, and the clock delays no longer apply. > >> > >> Due to how the clock tree is modeled and setup, we need to model > >> this function in two places, the master mmc clock and the two > >> child phase clocks. In the mmc clock, we can easily model the > >> mode bit as an extra variable post-divider. In the phase clocks, > >> we check the bit and return -ENOTSUPP if the bit is set, signaling > >> that the phase clocks are not to be used. > >> > >> This patch introduces a class of phase clocks that checks the > >> timing mode bit. > > > > We've been over this quite some times already... > > > > How do you retrieve the mode used in the clock so that you can also > > switch the MMC driver in the new mode? > > This part is covered. As mentioned above, the phase clks will return > -ENOTSUPP if the mmc clk is set in the new timing mode. The MMC driver > would try getting the current phase, check if it fails, and if it does, > realizes the clk is in new timing mode, and will switch the mmc controller > to it as well. I think this semantic fits the hardware well enough, > though it might be confusing without the accompanying comments. This is confusing, and impossible to debug without getting your hands in the driver. Triggerring the new mode is also not really something that is rate-based. We might want to switch to it explicitly, without any doubt. This is what we're doing for the A64 for example. Since some rates can be obtained both in the old and new modes likewise, you don't have that guarantee in the MMC driver and might end up in the old mode, even though you actually want it. And you can't. Which is why I argued several times for a custom function calls. Every other solutions that we evaluated so far was a hack, at best, and was not entirely predictable and debuggable, which is not a very nice combination. > See this commit for an example on how it should work. > https://github.com/wens/linux/commit/41e386f1f945a0a135a20a00601b977a001353da This is unrelated, but it breaks A64. A64 doesn't have the phase clocks at all, and need to be switched to the new mode all the time, without any cooperation from the CCU. Your code will try to change the phase, fail since you don't have any phase clock, and fall back to not using the new mode. I think we need to make a distinction between MMC controllers that can use both the old and new mode (A23, A33, H3, A83T, etc.) and the one that always need to use it (A64 eMMC). You should also retrieve whether you can use the new mode or not after changing the rate and not at probe time. Each call to clk_set_rate might change that. > > And do you prevent that from happening on the DT we already have that > > use the old clock drivers that do not support this new timing at all? > > I admit this is not taken care of in this patch. Mostly because there > is no existing DT for the A83T to cater to. For the H3 and other SoCs > that do, we need some way of asking the CCU to switch the mode. > > IIRC the output and sample clocks do not support a phase delay of 0. That's a weak assumption, since we're not even sure what the phase actually is. My recollection is that it doesn't, but I'm not entirely sure, and both the datasheet and the BSP code are not really clear on that part either. > So what if we design the semantic for this class of clocks as, if > phase == 0, switch to new timing mode, otherwise use old timing mode? > That would allow us to support this without adding extra functions, > and kind of matches what the hardware does. If you're using the new > timing mode, the delays in the mmc clk are bypassed. To be honest, I have the feeling that it should be a separate discussion. The whole A83T support has been completely stalled because of a single board's eMMC. This is pretty much a cross-SoC issue, at least 4 other SoCs have that issue already, and we need to deal with this for all of them. Merging a clock driver without the support for the MMC new mode would just bring us in the same situation than the A33 or H3, which I believe quite good. Maxime
diff --git a/drivers/clk/sunxi-ng/ccu_phase.c b/drivers/clk/sunxi-ng/ccu_phase.c index 400c58ad72fd..e6ff7551c855 100644 --- a/drivers/clk/sunxi-ng/ccu_phase.c +++ b/drivers/clk/sunxi-ng/ccu_phase.c @@ -8,6 +8,7 @@ * the License, or (at your option) any later version. */ +#include <linux/bitops.h> #include <linux/clk-provider.h> #include <linux/spinlock.h> @@ -124,3 +125,49 @@ const struct clk_ops ccu_phase_ops = { .get_phase = ccu_phase_get_phase, .set_phase = ccu_phase_set_phase, }; + +/* + * The MMC clocks on newer SoCs support the "new timing mode". Under + * this mode, the output of the clock is divided by 2, and the clock + * delays no longer apply. + * + * Due to how the clock tree is modeled and setup, we need to model + * this function in two places, the master mmc clock and the two + * child phase clocks. In the mmc clock, we can easily model the + * mode bit as an extra variable post-divider. In the phase clocks, + * we check the bit and return -ENOTSUPP if the bit is set, signaling + * that the phase clocks are not to be used. + * + * We do not support runtime configuration of the modes. Instead a + * mode is enforced at CCU probe time. + */ +#define CCU_MMC_NEW_TIMING_MODE BIT(30) + +static int ccu_phase_mmc_new_timing_get_phase(struct clk_hw *hw) +{ + struct ccu_phase *phase = hw_to_ccu_phase(hw); + u32 reg; + + reg = readl(phase->common.base + phase->common.reg); + if (reg & CCU_MMC_NEW_TIMING_MODE) + return -ENOTSUPP; + + return ccu_phase_get_phase(hw); +} + +static int ccu_phase_mmc_new_timing_set_phase(struct clk_hw *hw, int degrees) +{ + struct ccu_phase *phase = hw_to_ccu_phase(hw); + u32 reg; + + reg = readl(phase->common.base + phase->common.reg); + if (reg & CCU_MMC_NEW_TIMING_MODE) + return -ENOTSUPP; + + return ccu_phase_set_phase(hw, degrees); +} + +const struct clk_ops ccu_phase_mmc_new_timing_ops = { + .get_phase = ccu_phase_mmc_new_timing_get_phase, + .set_phase = ccu_phase_mmc_new_timing_set_phase, +}; diff --git a/drivers/clk/sunxi-ng/ccu_phase.h b/drivers/clk/sunxi-ng/ccu_phase.h index 75a091a4c565..c514d1798cdd 100644 --- a/drivers/clk/sunxi-ng/ccu_phase.h +++ b/drivers/clk/sunxi-ng/ccu_phase.h @@ -38,6 +38,20 @@ struct ccu_phase { } \ } +#define SUNXI_CCU_PHASE_MMC_NEW_TIMING(_struct, _name, _parent, _reg, \ + _shift, _width, _flags) \ + struct ccu_phase _struct = { \ + .shift = _shift, \ + .width = _width, \ + .common = { \ + .reg = _reg, \ + .hw.init = CLK_HW_INIT(_name, \ + _parent, \ + &ccu_phase_mmc_new_timing_ops, \ + _flags), \ + } \ + } + static inline struct ccu_phase *hw_to_ccu_phase(struct clk_hw *hw) { struct ccu_common *common = hw_to_ccu_common(hw); @@ -47,4 +61,6 @@ static inline struct ccu_phase *hw_to_ccu_phase(struct clk_hw *hw) extern const struct clk_ops ccu_phase_ops; +extern const struct clk_ops ccu_phase_mmc_new_timing_ops; + #endif /* _CCU_PHASE_H_ */
The MMC clocks on newer SoCs, such as the A83T and H3, support the "new timing mode". Under this mode, the output of the clock is divided by 2, and the clock delays no longer apply. Due to how the clock tree is modeled and setup, we need to model this function in two places, the master mmc clock and the two child phase clocks. In the mmc clock, we can easily model the mode bit as an extra variable post-divider. In the phase clocks, we check the bit and return -ENOTSUPP if the bit is set, signaling that the phase clocks are not to be used. This patch introduces a class of phase clocks that checks the timing mode bit. Signed-off-by: Chen-Yu Tsai <wens@csie.org> --- drivers/clk/sunxi-ng/ccu_phase.c | 47 ++++++++++++++++++++++++++++++++++++++++ drivers/clk/sunxi-ng/ccu_phase.h | 16 ++++++++++++++ 2 files changed, 63 insertions(+)