Re: [PATCH 1/4] clk: samsung: out: Add infrastructure to register CLKOUT

[Rahul Sharma]

Hi Tushar,

Basically you are adding a new clock-type for Clkout. IMO clkout
is not a special hardware. Existing clock types can be reused to
support clkout. I see 3 major problem here:

1) Clkout -> (Mux + Gate). You clubbed mux and gate together, and
exposing as a single clock which is something like a composite clock.
IMO this is not a recommended way in CCF.

2) New Clock Type: Since clkout is just a combination of a simple
mux and gate which are already supported, it is a unnecessary
duplication.

3) Clkout registered along with CMU: which is not correct. Clkout is in PMU
(Separate physical IP) and should be registered as a independent Clock
provider which provides 1 mux and 1 gate clock (As if now). It should also be
well connected with main CMU.

I understand the challenge in using regmap interface for a clock provider. But
we need to identify a clean solution. IMHO a independent clock provider with
iomap, is relatively cleaner approach till CCF is not ready with regmap based
reg access for clock registers.

Experts!! please comment.

Regards,
Rahul Sharma.

On 12 May 2014 10:16, Tushar Behera <tushar.behera@xxxxxxxxxx> wrote:
> On 05/10/2014 09:21 AM, Pankaj Dubey wrote:
>> On 05/09/2014 10:00 PM, Tushar Behera wrote:
>>> All SoC in Exynos-series have a clock with name XCLKOUT to provide
>>> debug information about various clocks available in the SoC. The register
>>> controlling the MUX and GATE of this clock is provided within PMU domain.
>>> Since PMU domain can't be dedicatedly mapped by every driver, the
>>> register
>>> needs to be handled through a regmap handle provided by PMU syscon
>>> controller. Right now, CCF doesn't allow regmap based MUX and GATE
>>> clocks,
>>> hence a dedicated clock provider for XCLKOUT is added here.
>>>
>>> Signed-off-by: Tushar Behera <tushar.behera@xxxxxxxxxx>
>>> CC: Tomasz Figa <t.figa@xxxxxxxxxxx>
>>> ---
>>>   drivers/clk/samsung/Makefile  |    2 +-
>>>   drivers/clk/samsung/clk-out.c |  181
>>> +++++++++++++++++++++++++++++++++++++++++
>>>   drivers/clk/samsung/clk.h     |   33 ++++++++
>>>   3 files changed, 215 insertions(+), 1 deletion(-)
>>>   create mode 100644 drivers/clk/samsung/clk-out.c
>>>
>
> [ ... ]
>
>>> +/**
>>> + * struct samsung_clkout_soc_data: SoC specific register details
>>> + * @reg: Offset of CLKOUT register from PMU base
>>
>> how about naming this variable as "offset" instead of "reg".
>>
>
> Okay, I will change that.
>
> [ ... ]
>
>>> +u8 samsung_clkout_get_parent(struct clk_hw *hw)
>>> +{
>>> +    struct samsung_clkout *clkout = to_clk_out(hw);
>>> +    const struct samsung_clkout_soc_data *soc_data = clkout->soc_data;
>>> +    unsigned int parent_mask = BIT(soc_data->mux_width) - 1;
>>> +    unsigned int val;
>>> +    int ret;
>>> +
>>> +    ret = regmap_read(clkout->regmap, soc_data->reg, &val);
>>
>> Do we really need to keep return value in "ret" as I can't see you are
>> using it anywhere?
>>
>
> Right, we are not using that and can be removed.
>
>>> +
>>> +    return (val >> soc_data->mux_shift) & parent_mask;
>>> +}
>>> +
>
> [ ... ]
>
>>> +/* All existing Exynos serial of SoCs have common values for this
>>> offsets. */
>> typo: serial/series/
>
> Sure. Thanks for your review.
>
> --
> Tushar Behera
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/