lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAPdUM4N=k-pPxdhPQgVHh0fb+nqkgs7StwM2Pb6C8gcjx6dw-w@mail.gmail.com>
Date:	Thu, 15 May 2014 19:14:47 +0530
From:	Rahul Sharma <rahul.sharma@...sung.com>
To:	Tushar Behera <tushar.behera@...aro.org>
Cc:	Pankaj Dubey <pankaj.dubey@...sung.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Mike Turquette <mturquette@...aro.org>,
	Tomasz Figa <t.figa@...sung.com>,
	Kukjin Kim <kgene.kim@...sung.com>,
	Kumar Gala <galak@...eaurora.org>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Mark Rutland <mark.rutland@....com>,
	Pawel Moll <pawel.moll@....com>,
	Rob Herring <robh+dt@...nel.org>,
	sunil joshi <joshi@...sung.com>
Subject: Re: [PATCH 1/4] clk: samsung: out: Add infrastructure to register CLKOUT

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@...aro.org> 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@...aro.org>
>>> CC: Tomasz Figa <t.figa@...sung.com>
>>> ---
>>>   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@...r.kernel.org
> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ