Hi Vladimi,
Many thanks for the code review
On 07/18/2017 10:19 PM, Vladimir Zapolskiy wrote:
> Hello Gabriel,
>
> On 07/18/2017 10:53 AM, 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>
>> ---
>> .../devicetree/bindings/clock/st,stm32h7-rcc.txt | 81 ++
>> drivers/clk/Makefile | 1 +
>> drivers/clk/clk-stm32h7.c | 1522 ++++++++++++++++++++
>> include/dt-bindings/clock/stm32h7-clks.h | 165 +++
>> include/dt-bindings/mfd/stm32h7-rcc.h | 136 ++
>> 5 files changed, 1905 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/clock/st,stm32h7-rcc.txt
>> create mode 100644 drivers/clk/clk-stm32h7.c
>> create mode 100644 include/dt-bindings/clock/stm32h7-clks.h
>> create mode 100644 include/dt-bindings/mfd/stm32h7-rcc.h
>>
>> diff --git a/Documentation/devicetree/bindings/clock/st,stm32h7-rcc.txt b/Documentation/devicetree/bindings/clock/st,stm32h7-rcc.txt
>> new file mode 100644
>> index 0000000..e41e4ac
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/st,stm32h7-rcc.txt
>> @@ -0,0 +1,81 @@
>> +STMicroelectronics STM32H7 Reset and Clock Controller
>> +=====================================================
>> +
>> +The RCC IP is both a reset and a clock controller.
>> +
>> +Please refer to clock-bindings.txt for common clock controller binding usage.
>> +Please also refer to reset.txt for common reset controller binding usage.
>> +
>> +Required properties:
>> +- compatible: Should be:
>> + "st,stm32h743-rcc"
>> +
>> +- reg: should be register base and length as documented in the
>> + datasheet
>> +
>> +- #reset-cells: 1, see below
>> +
>> +- #clock-cells : from common clock binding; shall be set to 1
>> +
>> +- clocks: External oscillator clock phandle
>> + - high speed external clock signal (HSE)
>> + - low speed external clock signal (LSE)
>> + - external I2S clock (I2S_CKIN)
>> +
>> +- st,syscfg: phandle for pwrcfg, mandatory to disable/enable backup domain
>> + write protection (RTC clock).
>> +
> please make a clear decision if "st,syscfg" property is mandatory or not.
> From the driver code this property is optional, and the clock driver
> is expected to work properly, if the property is omitted. Do I miss
> anything?
You right, in the driver code it's optional.
I will change it in dt binding documentation.
>
>> diff --git a/drivers/clk/clk-stm32h7.c b/drivers/clk/clk-stm32h7.c
>> new file mode 100644
>> index 0000000..2608c40
>> --- /dev/null
>> +++ b/drivers/clk/clk-stm32h7.c
> [snip]
>
>> +static const char * const ltdc_src[] = {"pll3_r"};
>> +
>> +/* 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));
> (0 << 8) is zero.
ok
>
>> +}
> IMHO a version below is better:
>
> static inline void enable_power_domain_write_protection(bool enable)
> {
> if (pdrm)
> regmap_update_bits(pdrm, 0x00, BIT(8), enable ? 0x0: BIT(8));
> }
>
>> +
>> +static inline bool is_enable_power_domain_write_protection(void)
> is_enabled_...
ok
>> +{
>> + if (pdrm) {
>> + u32 val;
>> +
>> + regmap_read(pdrm, 0x00, &val);
>> +
>> + return !(val & 0x100);
>> + }
>> + return 0;
>> +}
> Please replace (1 << 8) and 0x100 all above with a macro or at least
> BIT(8).
ok
>> +
>> +/* 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 10000
>> +
>> +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);
>> +
>> + /* We can't use readl_poll_timeout() because we can blocked if
>> + * someone enables this clock before clocksource changes.
>> + * Only jiffies counter is available. Jiffies are incremented by
>> + * interruptions and enable op does not allow to be interrupted.
>> + */
>> + do {
>> + bit_status = !(readl(gate->reg) & BIT(rgate->bit_rdy));
>> +
>> + if (bit_status)
>> + udelay(100);
>> +
>> + } while (bit_status && --timeout);
>> +
>> + if (rgate->backup_domain && dbp_status)
>> + enable_power_domain_write_protection();
>> +
>> + return bit_status;
>> +}
> I'm not convinced that the repetitive lockless pattern
>
> dbp_status = is_enable_power_domain_write_protection();
> if (dbp_status)
> disable_power_domain_write_protection();
>
> do_something();
>
> if (dbp_status)
> enable_power_domain_write_protection();
>
> is correct in a concurrent environment.
>
> You still have a risk of running do_something() *after* a call of
> enable_power_domain_write_protection().
Humm, after long discussion with the hardware ip owner, he recommended
to disable power domain write protection only one time at the init
(don't disable/enable dynamically).
Then i can suppress this code
dbp_status = is_enable_power_domain_write_protection();
if (dbp_status)
disable_power_domain_write_protection()
...
if (dbp_status)
enable_power_domain_write_protection();
Best regards
Gabriel
> The best option for you is either to switch to ordinary power domains
> and use their API or to move the driver to use regmaps, and at least
> in the latter case you don't need to wrap your code by CCF code (!),
> and as a result you don't need to export truly internal to CCF function
> clk_gate_is_enabled().
>
> --
> With best wishes,
> Vladimir
��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
