Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Rajan Vaja <RAJANV@xilinx.com>
To:     Michael Tretter <m.tretter@pengutronix.de>
CC:     "mturquette@baylibre.com" <mturquette@baylibre.com>,
        "sboyd@kernel.org" <sboyd@kernel.org>,
        Michal Simek <michals@xilinx.com>,
        Jolly Shah <JOLLYS@xilinx.com>,
        "dan.carpenter@oracle.com" <dan.carpenter@oracle.com>,
        "gustavo@embeddedor.com" <gustavo@embeddedor.com>,
        "linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kernel@pengutronix.de" <kernel@pengutronix.de>
Subject: RE: [PATCH] clk: zynqmp: Add support for custom type flags
Thread-Topic: [PATCH] clk: zynqmp: Add support for custom type flags
Thread-Index: AQHVlUmOtzK6cxVYuU2nwcMuzOsMk6d/7BAAgAXqeBA=
Date:   Mon, 11 Nov 2019 12:42:51 +0000
Message-ID: <BYAPR02MB40557E376D93824F48D103A3B7740@BYAPR02MB4055.namprd02.prod.outlook.com>
References: <1573117086-7405-1-git-send-email-rajan.vaja@xilinx.com>
 <20191107174928.71a921f0@litschi.hi.pengutronix.de>
In-Reply-To: <20191107174928.71a921f0@litschi.hi.pengutronix.de>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: xilinx.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: acb7db0f-7110-46bd-aeff-08d766a4aa88
X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Nov 2019 12:42:51.4590
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: SBJdP+/AsPVVqbmhdfdE5z0VFxb/WFiv/XdduKmEEHQNc/+SotflWO9kf0M2YSE1RLNa1PNnMQOQ0jnNLAD/SQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR02MB4040
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Michael,

Thanks for the review.

> -----Original Message-----
> From: Michael Tretter <m.tretter@pengutronix.de>
> Sent: 07 November 2019 08:49
> To: Rajan Vaja <RAJANV@xilinx.com>
> Cc: mturquette@baylibre.com; sboyd@kernel.org; Michal Simek
> <michals@xilinx.com>; Jolly Shah <JOLLYS@xilinx.com>;
> dan.carpenter@oracle.com; gustavo@embeddedor.com; linux-
> clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; kernel@pengutronix.de
> Subject: Re: [PATCH] clk: zynqmp: Add support for custom type flags
>=20
> EXTERNAL EMAIL
>=20
> On Thu, 07 Nov 2019 00:58:06 -0800, Rajan Vaja wrote:
> > Store extra custom type flags received from firmware.
> >
> > Signed-off-by: Rajan Vaja <rajan.vaja@xilinx.com>
> > Signed-off-by: Jolly Shah <jolly.shah@xilinx.com>
> > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > ---
> >  drivers/clk/zynqmp/clkc.c    | 8 +++++++-
> >  drivers/clk/zynqmp/divider.c | 4 ++--
> >  2 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
> > index a11f93e..0dea55e 100644
> > --- a/drivers/clk/zynqmp/clkc.c
> > +++ b/drivers/clk/zynqmp/clkc.c
> > @@ -2,7 +2,7 @@
> >  /*
> >   * Zynq UltraScale+ MPSoC clock controller
> >   *
> > - *  Copyright (C) 2016-2018 Xilinx
> > + *  Copyright (C) 2016-2019 Xilinx
> >   *
> >   * Based on drivers/clk/zynq/clkc.c
> >   */
> > @@ -86,6 +86,8 @@ struct topology_resp {
> >  #define CLK_TOPOLOGY_TYPE            GENMASK(3, 0)
> >  #define CLK_TOPOLOGY_FLAGS           GENMASK(23, 8)
> >  #define CLK_TOPOLOGY_TYPE_FLAGS              GENMASK(31, 24)
> > +#define CLK_TOPOLOGY_TYPE_FLAG2              GENMASK(7, 4)
>=20
> What kind of function do these flags indicate? The name is really not
> obvious to me.
>=20
> I would prefer if the defines are kept in the order of the bits in the
> field, i.e., the new define should go between CLK_TOPOLOGY_TYPE and
> CLK_TOPOLOGY_FLAGS.
[Rajan] Ok I will change this in next version.

>=20
> > +#define CLK_TOPOLOGY_TYPE_FLAG_BITS  8
> >       u32 topology[CLK_GET_TOPOLOGY_RESP_WORDS];
> >  };
> >
> > @@ -396,6 +398,10 @@ static int __zynqmp_clock_get_topology(struct
> clock_topology *topology,
> >               topology[*nnodes].type_flag =3D
> >                               FIELD_GET(CLK_TOPOLOGY_TYPE_FLAGS,
> >                                         response->topology[i]);
> > +             topology[*nnodes].type_flag |=3D
> > +                     FIELD_GET(CLK_TOPOLOGY_TYPE_FLAG2,
> > +                               response->topology[i]) <<
> > +                     CLK_TOPOLOGY_TYPE_FLAG_BITS;
>=20
> Shifting the new flags into the existing type_flag field seems like a
> source for code that is really difficult to read. Maybe use a new field
> in the topology for the new flags with a proper name?
[Rajan] Will do it in next version.

>=20
> >               (*nnodes)++;
> >       }
> >
> > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.=
c
> > index d8f5b70d..d376529 100644
> > --- a/drivers/clk/zynqmp/divider.c
> > +++ b/drivers/clk/zynqmp/divider.c
> > @@ -2,7 +2,7 @@
> >  /*
> >   * Zynq UltraScale+ MPSoC Divider support
> >   *
> > - *  Copyright (C) 2016-2018 Xilinx
> > + *  Copyright (C) 2016-2019 Xilinx
> >   *
> >   * Adjustable divider clock implementation
> >   */
> > @@ -37,7 +37,7 @@
> >   */
> >  struct zynqmp_clk_divider {
> >       struct clk_hw hw;
> > -     u8 flags;
> > +     u16 flags;
>=20
> This change looks unrelated to the remaining patch.
[Rajan] Flag width was changed to accommodate custom flags. So needed to ch=
ange it to u16.
But as you suggested in above comment, will use new variable for custom fla=
g, and so this change won't
Be required.

Thanks,
Rajan
>=20
> Michael
>=20
> >       bool is_frac;
> >       u32 clk_id;
> >       u32 div_type;