linux-kernel - Re: [PATCH v2 3/9] arm: twr-k70f120m: clock driver for Kinetis SoC

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]

Date:	Wed, 15 Jul 2015 09:31:17 +0200 (CEST)
From:	Paul Osmialowski <pawelo@...g.net.pl>
To:	Linus Walleij <linus.walleij@...aro.org>
cc:	Paul Osmialowski <pawelo@...g.net.pl>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Jiri Slaby <jslaby@...e.cz>, Kumar Gala <galak@...eaurora.org>,
	Mark Rutland <mark.rutland@....com>,
	Michael Turquette <mturquette@...libre.com>,
	Pawel Moll <pawel.moll@....com>,
	Rob Herring <robh+dt@...nel.org>,
	Russell King <linux@....linux.org.uk>,
	Stephen Boyd <sboyd@...eaurora.org>,
	Vinod Koul <vinod.koul@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>, linux-clk@...r.kernel.org,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	dmaengine@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Nicolas Pitre <nicolas.pitre@...aro.org>,
	Paul Bolle <pebolle@...cali.nl>,
	Thomas Gleixner <tglx@...utronix.de>,
	Uwe Kleine-Koenig <u.kleine-koenig@...gutronix.de>,
	Anson Huang <b20788@...escale.com>,
	Frank Li <Frank.Li@...escale.com>,
	Jingchang Lu <jingchang.lu@...escale.com>,
	Rob Herring <r.herring@...escale.com>,
	Yuri Tikhonov <yur@...raft.com>,
	Sergei Poselenov <sposelenov@...raft.com>,
	Alexander Potashev <aspotashev@...raft.com>
Subject: Re: [PATCH v2 3/9] arm: twr-k70f120m: clock driver for Kinetis SoC

Hi Linus,

Thanks for all of your comments, I'll consider them during my works on the 
next iteration. However, I have doubts about this one:

On Tue, 14 Jul 2015, Linus Walleij wrote:

> On Tue, Jun 30, 2015 at 2:27 PM, Paul Osmialowski <pawelo@...g.net.pl> wrote:
> 
> > Based on K70P256M150SF3RM.pdf K70 Sub-Family Reference Manual, Rev. 3.
> >
> > Signed-off-by: Paul Osmialowski <pawelo@...g.net.pl>
> (...)
> > +struct kinetis_sim_regs {
> > +       u32 sopt1;      /* System Options Register 1 */
> > +       u32 rsv0[1024];
> > +       u32 sopt2;      /* System Options Register 2 */
> > +       u32 rsv1;
> > +       u32 sopt4;      /* System Options Register 4 */
> > +       u32 sopt5;      /* System Options Register 5 */
> > +       u32 sopt6;      /* System Options Register 6 */
> > +       u32 sopt7;      /* System Options Register 7 */
> > +       u32 rsv2[2];
> > +       u32 sdid;       /* System Device Identification Register */
> > +       u32 scgc[KINETIS_SIM_CG_NUMREGS];       /* Clock Gating Regs 1...7 */
> > +       u32 clkdiv1;    /* System Clock Divider Register 1 */
> > +       u32 clkdiv2;    /* System Clock Divider Register 2 */
> > +       u32 fcfg1;      /* Flash Configuration Register 1 */
> > +       u32 fcfg2;      /* Flash Configuration Register 2 */
> > +       u32 uidh;       /* Unique Identification Register High */
> > +       u32 uidmh;      /* Unique Identification Register Mid-High */
> > +       u32 uidml;      /* Unique Identification Register Mid Low */
> > +       u32 uidl;       /* Unique Identification Register Low */
> > +       u32 clkdiv3;    /* System Clock Divider Register 3 */
> > +       u32 clkdiv4;    /* System Clock Divider Register 4 */
> > +       u32 mcr;        /* Misc Control Register */
> > +};
> 
> Now there is this design pattern where you copy the datasheet
> register map to a struct again.
> 
> This is not good if there is a second revision of the hardware and some
> registers are shuffled around. IMO it is better to just use #defines
> for register
> offsets, so you can do exceptions later. Else a new hardware revision
> leads to a new struct with new accessor functions etc etc.
> 

I don't see how replacing this structure with bunch of defines could make 
anyones life easier. As registers are shuffled around due to updated 
hardware revision they could be shuffled in this structure too. Doing 
this with buch of defines would require eager and careful adaptation of 
all the defines. I don't see how this could be easier.

Note that I'm not making any instances of the structure (it is used only 
for casting), so shuffling its fields around should not affect the code 
that follows.

After recent purge it is only used within this macro:

#define KINETIS_SIM_PTR(base, reg) \
        (&(((struct kinetis_sim_regs *)(base))->reg))

...and used like this:

        ioread32(KINETIS_SIM_PTR(sim, clkdiv1));

IMHO changing the struct internals does not require subsequent changes in 
any of those.

Thanks,
Paul
--
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/