[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-pci
Subject:    RE: [PATCH 3/3] PCI: mvebu: add wrapped I/O access functions
From:       Seungwon Jeon <tgih.jun () samsung ! com>
Date:       2013-08-29 9:35:14
Message-ID: 000a01cea49b$10832780$31897680$%jun () samsung ! com
[Download RAW message or body]

On Wednesday, August 28, 2013, Thomas Petazzoni wrote
> Dear Seungwon Jeon,
> 
> On Wed, 28 Aug 2013 20:53:47 +0900, Seungwon Jeon wrote:
> > read/write operation for I/O mem is wrapped with hiding
> > base address inside in order to simplify the usage.
> >
> > Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
> 
> I'm fine with the principle, but I have a few comments below.
> 
> > ---
> >  drivers/pci/host/pci-mvebu.c |  105 +++++++++++++++++++++++++++---------------
> >  1 files changed, 68 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> > index d66530e..db8b00b 100644
> > --- a/drivers/pci/host/pci-mvebu.c
> > +++ b/drivers/pci/host/pci-mvebu.c
> > @@ -141,29 +141,59 @@ struct mvebu_pcie_port {
> >  	size_t iowin_size;
> >  };
> >
> > +static inline void mvebu_writeb(struct mvebu_pcie_port *port, u8 val, u32 reg)
> > +{
> > +	writeb(val, port->base + reg);
> > +}
> > +
> > +static inline u8 mvebu_readb(struct mvebu_pcie_port *port, u32 reg)
> > +{
> > +	return readb(port->base + reg);
> > +}
> 
> Those ones are not needed, all registers of this IP block are 32 bits
> registers.

Dear Thomas Petazzoni,

This is because I saw mvebu_pcie_hw_wr_conf(), which has various bit access.
But it could be modified with both writel and readl, right?

> 
> > +
> > +static inline void mvebu_writew(struct mvebu_pcie_port *port, u16 val, u32 reg)
> > +{
> > +	writew(val, port->base + reg);
> > +}
> > +
> > +static inline u16 mvebu_readw(struct mvebu_pcie_port *port, u32 reg)
> > +{
> > +	return readw(port->base + reg);
> > +}
> 
> Provided a small change is done below, those will also not be needed.
> 
> > +static inline void mvebu_writel(struct mvebu_pcie_port *port, u32 val, u32 reg)
> > +{
> > +	writel(val, port->base + reg);
> > +}
> > +
> > +static inline u32 mvebu_readl(struct mvebu_pcie_port *port, u32 reg)
> > +{
> > +	return readl(port->base + reg);
> > +}
> 
> Ok, but they should be called mvebu_pcie_readl() and
> mvebu_pcie_writel(), because they are specific to the PCIe IP block.
> Once you've done that, you'll realize that it's in fact longer to write:
> 
> 	mvebu_pcie_readl(port, PCIE_FOO);
> 
> than
> 
> 	readl(port->base + PCIE_FOO);
> 
> but ok, I do not oppose to the change.
> 
> >  	/* Master + slave enable. */
> > -	cmd = readw(port->base + PCIE_CMD_OFF);
> > +	cmd = mvebu_readw(port, PCIE_CMD_OFF);
> >  	cmd |= PCI_COMMAND_IO;
> >  	cmd |= PCI_COMMAND_MEMORY;
> >  	cmd |= PCI_COMMAND_MASTER;
> > -	writew(cmd, port->base + PCIE_CMD_OFF);
> > +	mvebu_writew(port, cmd, PCIE_CMD_OFF);
> 
> I believe this can be turned into a 32 bits read/modify/write, because
> the register is really a 32 bits registers, on both Armada 370/XP and
> Kirkwood (I guess Dove is the same).
You mean that here, readw and writew can be replaced by 32 bits access?
Then, I could take only 32-bit access functions.

Thanks,
Seungwon Jeon
> 
> With this change, you can keep only the 32 bits variants of the
> accessors functions at the top of the file.
> 
> Thanks,
> 
> Thomas
> --
> Thomas Petazzoni, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]