linux-kernel - Re: [PATCH 1/6] ata: ahci: add AHCI support for Berlin SoCs

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, 23 Apr 2014 10:33:26 +0200
From:	Antoine Ténart 
	<antoine.tenart@...e-electrons.com>
To:	Rob Herring <robherring2@...il.com>
Cc:	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
	Tejun Heo <tj@...nel.org>, zmxu@...vell.com,
	Jisheng Zhang <jszhang@...vell.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-ide@...r.kernel.org,
	Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 1/6] ata: ahci: add AHCI support for Berlin SoCs

Rob,

On Tue, Apr 22, 2014 at 01:47:43PM -0500, Rob Herring wrote:
> On Tue, Apr 22, 2014 at 10:38 AM, Antoine Ténart
> <antoine.tenart@...e-electrons.com> wrote:

[…]

> > +static inline void ahci_berlin_reg_clear_set(void __iomem *reg, u32 clear_val,
> > +                                            u32 set_val)
> > +{
> > +       u32 regval;
> > +
> > +       regval = readl(reg);
> > +       regval &= ~(clear_val);
> > +       regval |= set_val;
> > +       writel(regval, reg);
> > +}
> > +
> > +static void ahci_berlin_init(void __iomem *mmio)
> 
> I don't really see the point of a function to do 2 register writes of
> magic values especially when the function name doesn't provide any
> indication of what you are doing really.
> 
> > +{
> > +       /* interface select */
> > +       ahci_berlin_reg_set(mmio + HOST_VSA_ADDR, BIT(2));
> > +       ahci_berlin_reg_set(mmio + HOST_VSA_ADDR,
> 
> 2 writes to the same reg? Is this supposed to be the VSA_DATA register?

You're right it's VSA_DATA.

> 
> > +                           BIT(21) | BIT(18) | BIT(5) | BIT(4) | BIT(2));
> 
> Use of BIT is only helpful to people that don't understand hex. Can
> you define what these bits are. Otherwise, just use 0x00240034 and
> note that it is magic value which you have no idea what the bits are.

This is magic. I'll update and add a comment.

> > +static void ahci_berlin_port_init(void __iomem *mmio, unsigned int ports)
> > +{
> > +       int p;
> > +
> > +       /* power down pll */
> > +       ahci_berlin_reg_set(mmio + HOST_VSA_ADDR, 0x0);
> > +       ahci_berlin_reg_setbits(mmio + HOST_VSA_DATA, BIT(6));
> > +
> > +       for (p = 0; p < ports; p++) {
> > +               /* port control register */
> > +               void __iomem *ctrl_reg = mmio + 0x100 + (p * 0x80);
> > +
> > +               /* set PHY mode to SATA, ref freq to 25 MHz */
> > +               ahci_berlin_reg_set(ctrl_reg + PORT_VSR_ADDR, 0x201);
> > +               ahci_berlin_reg_clear_set(ctrl_reg + PORT_VSR_DATA,
> > +                                         0xff, BIT(0));
> 
> So you have registers hidden behind an address and data register.
> Perhaps a read and write function to provide that access rather than
> all these set/clear bit functions.

I'd like to do so if I could name 0x201, 0x225 ...
I can also merge the two functions if you think it's better for
readability.

Thanks!

Antoine


-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
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/