Hi Stephen,
Thanks for reviewing.
On 06/22/2017 12:07 AM, Stephen Boyd wrote:
> On 06/07, gabriel.fernandez@xxxxxx wrote:
>> From: Gabriel Fernandez <gabriel.fernandez@xxxxxx>
>>
>> This patch enables clocks for STM32H743 boards.
>>
>> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@xxxxxx>
>>
>> for MFD changes:
>> Acked-by: Lee Jones <lee.jones@xxxxxxxxxx>
>>
>> for DT-Bindings
>> Acked-by: Rob Herring <robh@xxxxxxxxxx>
>> v4:
>> - rename lock into stm32rcc_lock
>> - don't use clk_readl()
>> - remove useless parentheses with GENMASK
>> - fix parents of timer_x clocks
>> - suppress pll configuration from DT
>> - fix kbuild warning
>>
>> v3:
>> - fix compatible string "stm32h7-pll" into "st,stm32h7-pll"
>> - fix bad parent name for mco2 clock
>> - set CLK_SET_RATE_PARENT for ltdc clock
>> - set CLK_IGNORE_UNUSED for pll1
>> - disable power domain write protection on disable ops if needed
>>
>>
>> v2:
>> - rename compatible string "stm32,pll" into "stm32h7-pll"
>> - suppress "st,pllrge" property
>> - suppress "st, frac-status" property
>> - change management of "st,frac" property
>> 0 : enable 0 pll integer mode
>> other values : enable pll in fractional mode (value is
>> the fractional factor)
> Please drop the changelog from commit text.
strange, i added the changelog after 'git format-patch'
>
>> diff --git a/drivers/clk/clk-stm32h7.c b/drivers/clk/clk-stm32h7.c
>> new file mode 100644
>> index 0000000..2907c1f
>> --- /dev/null
>> +++ b/drivers/clk/clk-stm32h7.c
>> @@ -0,0 +1,1532 @@
>> +/* Power domain helper */
>> +static inline void disable_power_domain_write_protection(void)
>> +{
>> + if (pdrm)
>> + regmap_update_bits(pdrm, 0x00, (1 << 8), (1 << 8));
>> +}
>> +
>> +static inline void enable_power_domain_write_protection(void)
>> +{
>> + if (pdrm)
>> + regmap_update_bits(pdrm, 0x00, (1 << 8), (0 << 8));
>> +}
>> +
>> +static inline int is_enable_power_domain_write_protection(void)
> Return bool, not int?
ok
>
>> +{
>> + if (pdrm) {
>> + u32 val;
>> +
>> + regmap_read(pdrm, 0x00, &val);
>> +
>> + return !(val & 0x100);
>> + }
>> + return -1;
> Returning -1 looks odd.
ok i will change it
>
>> +}
>> +
>> +/* Gate clock with ready bit and backup domain management */
>> +struct stm32_ready_gate {
>> + struct clk_gate gate;
>> + u8 bit_rdy;
>> + u8 backup_domain;
>> +};
>> +
>> +#define to_ready_gate_clk(_rgate) container_of(_rgate, struct stm32_ready_gate,\
>> + gate)
>> +
>> +#define RGATE_TIMEOUT 600000
>> +
>> +static int ready_gate_clk_is_enabled(struct clk_hw *hw)
>> +{
>> + return clk_gate_ops.is_enabled(hw);
>> +}
> Perhaps we should expose clk_gate_ops::is_enabled as functions
> that can be directly called and assigned in places like this so
> we don't need wrapper functions that do nothing besides forward
> the call.
ok i will add a patch in clk.c and clk-provider.h to export 'clk_gate_is_enabled'
>
>> +
>> +static int ready_gate_clk_enable(struct clk_hw *hw)
>> +{
>> + struct clk_gate *gate = to_clk_gate(hw);
>> + struct stm32_ready_gate *rgate = to_ready_gate_clk(gate);
>> + int dbp_status;
>> + int bit_status;
>> + unsigned int timeout = RGATE_TIMEOUT;
>> +
>> + if (clk_gate_ops.is_enabled(hw))
>> + return 0;
>> +
>> + dbp_status = is_enable_power_domain_write_protection();
>> +
>> + if (rgate->backup_domain && dbp_status)
>> + disable_power_domain_write_protection();
>> +
>> + clk_gate_ops.enable(hw);
>> +
>> + do {
>> + bit_status = !(readl(gate->reg) & BIT(rgate->bit_rdy));
>> +
>> + if (bit_status)
>> + udelay(1000);
>> +
>> + } while (bit_status && --timeout);
> readl_poll_timeout?
last time it didn't work, i will investigate again
>> +
>> +/* RTC clock */
>> +static u8 rtc_mux_get_parent(struct clk_hw *hw)
>> +{
>> + return clk_mux_ops.get_parent(hw);
>> +}
>> +
>> +static int rtc_mux_set_parent(struct clk_hw *hw, u8 index)
>> +{
>> + int dbp_status;
>> + int err;
>> +
>> + dbp_status = is_enable_power_domain_write_protection();
>> +
>> + if (dbp_status)
>> + disable_power_domain_write_protection();
>> +
>> + err = clk_mux_ops.set_parent(hw, index);
>> +
>> + if (dbp_status)
>> + enable_power_domain_write_protection();
>> +
>> + return err;
>> +}
>> +
>> +static int rtc_mux_determine_rate(struct clk_hw *hw,
>> + struct clk_rate_request *req)
>> +{
>> + return clk_mux_ops.determine_rate(hw, req);
>> +}
> In this case we have that function exposed already so it could
> be assigned.
ok i will use __clk_mux_determine_rate
>> +
>> +static const struct clk_ops rtc_mux_ops = {
>> + .get_parent = rtc_mux_get_parent,
>> + .set_parent = rtc_mux_set_parent,
>> + .determine_rate = rtc_mux_determine_rate,
>> +};
>> +
>> +/* Clock gate with backup domain protection management */
>> +static int bd_gate_is_enabled(struct clk_hw *hw)
>> +{
>> + return clk_gate_ops.is_enabled(hw);
>> +}
>> +
>> +static int bd_gate_enable(struct clk_hw *hw)
>> +{
>> + int dbp_status;
>> + int err;
>> +
>> + if (bd_gate_is_enabled(hw))
>> + return 0;
>> +
> [...]
>> +
>> + return;
>> +
>> +err_free_clks:
>> + kfree(clk_data);
>> +}
>> +
>> +CLK_OF_DECLARE_DRIVER(stm32h7_rcc, "st,stm32h743-rcc", stm32h7_rcc_init);
> Can you add a comment above this why we can't do a split design
> with a platform driver and a CLK_OF_DECLARE_DRIVER() routine here
> and also mention the other driver that's probing against the same
> compatible?
>
ok
��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
