[v3,03/13] pci: introduce helper function pci_shift_word/long which returns shifted value.

introduce helper function pci_shift_{word, long}() which returns
returns shifted word/long of given position and range.
They will be used later.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.h |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

On Wed, Sep 15, 2010 at 02:38:16PM +0900, Isaku Yamahata wrote:
> introduce helper function pci_shift_{word, long}() which returns
> returns shifted word/long of given position and range.
> They will be used later.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

So I think the reason you *think* you need these
is because you set the wmask wrong: you make all
capability readonly and then write a ton of
custom code to tweak it. Instead,
Make writeable registers writeable, even if
read always returns 0.
Then in your handler do
if (dev->config[offset] & mask) {
	handle bit write
	dev->config[offset] &= ~mask;
}

no range checks necessary.

If you need to do something on register change,
just keep the old state in your structure,
then

	dev->exp.a != dev->config[offset]
tells you there was a change.


BTW if you need to do this to words or longs,
not just bytes, maybe pci_set_word/pci_clear_word would be helpful?

> ---
>  hw/pci.h |   19 +++++++++++++++++++
>  1 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci.h b/hw/pci.h
> index f4ea97a..630631b 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -327,6 +327,25 @@ pci_config_set_interrupt_pin(uint8_t *pci_config, uint8_t val)
>      pci_set_byte(&pci_config[PCI_INTERRUPT_PIN], val);
>  }
>  
> +static inline uint32_t
> +pci_shift_long(uint32_t addr, uint32_t val, uint32_t pos)
> +{
> +    if (addr >= pos) {
> +        assert(addr - pos <= 32 / 8);
> +        val <<= (addr - pos) * 8;
> +    } else {
> +        assert(pos - addr <= 32 / 8);
> +        val >>= (pos - addr) * 8;
> +    }
> +    return val;
> +}
> +
> +static inline uint16_t
> +pci_shift_word(uint32_t addr, uint32_t val, uint32_t pos)
> +{
> +    return pci_shift_long(addr, val, pos);
> +}
> +
>  typedef int (*pci_qdev_initfn)(PCIDevice *dev);
>  typedef struct {
>      DeviceInfo qdev;
> -- 
> 1.7.1.1

On Wed, Sep 15, 2010 at 02:49:50PM +0200, Michael S. Tsirkin wrote:
> On Wed, Sep 15, 2010 at 02:38:16PM +0900, Isaku Yamahata wrote:
> > introduce helper function pci_shift_{word, long}() which returns
> > returns shifted word/long of given position and range.
> > They will be used later.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> So I think the reason you *think* you need these
> is because you set the wmask wrong: you make all
> capability readonly and then write a ton of
> custom code to tweak it. Instead,
> Make writeable registers writeable, even if
> read always returns 0.
> Then in your handler do
> if (dev->config[offset] & mask) {
> 	handle bit write
> 	dev->config[offset] &= ~mask;
> }
> 
> no range checks necessary.
> 
> If you need to do something on register change,
> just keep the old state in your structure,
> then
> 
> 	dev->exp.a != dev->config[offset]
> tells you there was a change.

Okay, I'll give it a try.


> BTW if you need to do this to words or longs,
> not just bytes, maybe pci_set_word/pci_clear_word would be helpful?

Agreed.

thanks,

> > ---
> >  hw/pci.h |   19 +++++++++++++++++++
> >  1 files changed, 19 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/pci.h b/hw/pci.h
> > index f4ea97a..630631b 100644
> > --- a/hw/pci.h
> > +++ b/hw/pci.h
> > @@ -327,6 +327,25 @@ pci_config_set_interrupt_pin(uint8_t *pci_config, uint8_t val)
> >      pci_set_byte(&pci_config[PCI_INTERRUPT_PIN], val);
> >  }
> >  
> > +static inline uint32_t
> > +pci_shift_long(uint32_t addr, uint32_t val, uint32_t pos)
> > +{
> > +    if (addr >= pos) {
> > +        assert(addr - pos <= 32 / 8);
> > +        val <<= (addr - pos) * 8;
> > +    } else {
> > +        assert(pos - addr <= 32 / 8);
> > +        val >>= (pos - addr) * 8;
> > +    }
> > +    return val;
> > +}
> > +
> > +static inline uint16_t
> > +pci_shift_word(uint32_t addr, uint32_t val, uint32_t pos)
> > +{
> > +    return pci_shift_long(addr, val, pos);
> > +}
> > +
> >  typedef int (*pci_qdev_initfn)(PCIDevice *dev);
> >  typedef struct {
> >      DeviceInfo qdev;
> > -- 
> > 1.7.1.1
>