linux-kernel - Re: [PATCH v2 6/8] PCI: al: Add support for DW based driver type

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: <27d8e8a8bba4aeb845d6c953f3a07a29875fef02.camel@amazon.com>
Date:   Mon, 22 Jul 2019 15:38:46 +0000
From:   "Chocron, Jonathan" <jonnyc@...zon.com>
To:     "jingoohan1@...il.com" <jingoohan1@...il.com>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "lorenzo.pieralisi@....com" <lorenzo.pieralisi@....com>,
        "Gustavo.Pimentel@...opsys.com" <Gustavo.Pimentel@...opsys.com>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Woodhouse, David" <dwmw@...zon.co.uk>,
        "Hanoch, Uri" <hanochu@...zon.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "Wasserstrom, Barak" <barakw@...zon.com>,
        "Saidi, Ali" <alisaidi@...zon.com>,
        "Hawa, Hanna" <hhhawa@...zon.com>,
        "Shenhar, Talel" <talel@...zon.com>,
        "Krupnik, Ronen" <ronenk@...zon.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "benh@...nel.crashing.org" <benh@...nel.crashing.org>,
        "Chocron, Jonathan" <jonnyc@...zon.com>
Subject: Re: [PATCH v2 6/8] PCI: al: Add support for DW based driver type

On Mon, 2019-07-22 at 08:54 +0000, Gustavo Pimentel wrote:
> 
> > 
> > > > +static inline void al_pcie_target_bus_set(struct al_pcie
> > > > *pcie,
> > > > +					  u8 target_bus,
> > > > +					  u8 mask_target_bus)
> > > > +{
> > > > +	void __iomem *cfg_control_addr;
> > > > +	u32 reg;
> > > > +
> > > > +	reg = FIELD_PREP(CFG_TARGET_BUS_MASK_MASK,
> > > > mask_target_bus) |
> > > > +	      FIELD_PREP(CFG_TARGET_BUS_BUSNUM_MASK,
> > > > target_bus);
> > > > +
> > > > +	cfg_control_addr = (void __iomem *)((uintptr_t)pcie-
> > > > > controller_base +
> > > > 
> > > > +			   AXI_BASE_OFFSET + pcie-
> > > > >reg_offsets.ob_ctrl
> > > > +
> > > > +			   CFG_TARGET_BUS);
> > > > +
> > > > +	writel(reg, cfg_control_addr);
> > > 
> > > From what I'm seeing you commonly use writel() and readl() with a
> > > common 
> > > base address, such as pcie->controller_base + AXI_BASE_OFFSET.
> > > I'd suggest to creating a writel and readl with that offset
> > > built-in.
> > > 
> > 
> > I prefer to keep it generic, since in future revisions we might
> > want to
> > access regs which are not in the AXI region. You think I should add
> > wrappers which simply hide the pcie->controller_base part?
> 
> I and other developers typically do that, but it's a suggestion. IMHO
> it 
> helps to keep the code cleaner and more readable.
> 

Added al_pcie_controller_readl/writel wrappers.

-- 
Thanks,
   Jonathan