On register r/w macros/procedures of drivers/media/pci

On register r/w macros/procedures of drivers/media/pci

[Andrey Utkin]

I am starting a work on driver for techwell tw5864 media grabber&encoder.
I am basing on tw68 driver (mentorship, advising and testing by board
owners are appreciated). And here (and in some other
drivers/media/pci/ drivers) I see what confuses me:

tw68-core.c:
        dev->lmmio = ioremap(pci_resource_start(pci_dev, 0),
                             pci_resource_len(pci_dev, 0));
        dev->bmmio = (__u8 __iomem *)dev->lmmio;

tw68.h:
#define tw_readl(reg)           readl(dev->lmmio + ((reg) >> 2))
#define tw_readb(reg)           readb(dev->bmmio + (reg))
#define tw_writel(reg, value)   writel((value), dev->lmmio + ((reg) >> 2))
#define tw_writeb(reg, value)   writeb((value), dev->bmmio + (reg))

I am mostly userland developer and I wouldn't expect bmmio pointer to
contain value numerically different from lmmio after such simple
casting.
But still, if this is correct, then how should I implement
tw_{read,write}w to operate on 2 bytes (a word)? Similarly, it would
look like this:
#define tw_readl(reg)           readl(dev->lmmio + ((reg) >> 1))
That looks odd.

In contrary, in drivers/media/pci/dm1105, we see no explicit shifting
of register address. It uses {in,out}{b,w,l} macros, which seem to
turn out the same {read,write}{b,w,l} (with some reservations):
http://lxr.free-electrons.com/source/include/asm-generic/io.h#L354

dm1105.c:#define dm_io_mem(reg) ((unsigned long)(&dev->io_mem[reg]))
dm1105.c:#define dm_readb(reg)          inb(dm_io_mem(reg))
dm1105.c:#define dm_writeb(reg, value)  outb((value), (dm_io_mem(reg)))
dm1105.c:#define dm_readw(reg)          inw(dm_io_mem(reg))
dm1105.c:#define dm_writew(reg, value)  outw((value), (dm_io_mem(reg)))
dm1105.c:#define dm_readl(reg)          inl(dm_io_mem(reg))
dm1105.c:#define dm_writel(reg, value)  outl((value), (dm_io_mem(reg)))

This looks like contradiction to me (shifting register address vs. no
shifting), so that one practice may be even just wrong and broken
(which is hard to believe due to active maintenance of all drivers).
I highly appreciate your help me in determining the best practice to
follow in this new driver.
Thanks.

-- 
Bluecherry developer.

Re: On register r/w macros/procedures of drivers/media/pci

[Hans Verkuil]

On 04/19/2015 09:36 AM, Andrey Utkin wrote:
> I am starting a work on driver for techwell tw5864 media grabber&encoder.
> I am basing on tw68 driver (mentorship, advising and testing by board
> owners are appreciated). And here (and in some other
> drivers/media/pci/ drivers) I see what confuses me:
> 
> tw68-core.c:
>         dev->lmmio = ioremap(pci_resource_start(pci_dev, 0),
>                              pci_resource_len(pci_dev, 0));
>         dev->bmmio = (__u8 __iomem *)dev->lmmio;
> 
> tw68.h:
> #define tw_readl(reg)           readl(dev->lmmio + ((reg) >> 2))
> #define tw_readb(reg)           readb(dev->bmmio + (reg))
> #define tw_writel(reg, value)   writel((value), dev->lmmio + ((reg) >> 2))
> #define tw_writeb(reg, value)   writeb((value), dev->bmmio + (reg))
> 
> I am mostly userland developer and I wouldn't expect bmmio pointer to
> contain value numerically different from lmmio after such simple
> casting.

Check the types of llmio and bbmio:

        u32                     __iomem *lmmio;
        u8                      __iomem *bmmio;

So the values of the pointers are the same, but the types are not.

So 'lmmio + 1' == 'bmmio + sizeof(u32)' == 'bbmio + 4'.

Since all the registers are defined as byte offsets relative to the start
of the memory map you cannot just do 'lmmio + reg' since that would be a
factor 4 off. Instead you have to divide by 4 to get it back in line.

Frankly, I don't think lmmio is necessary at all since readl/writel don't
need a u32 pointer at all since they use void pointers. I never noticed
that when I cleaned up the tw68 driver. Using 'void __iomem *mmio' instead
of lmmio/bmmio and dropping the shifts in the tw_ macros would work just
as well.

> But still, if this is correct, then how should I implement
> tw_{read,write}w to operate on 2 bytes (a word)? Similarly, it would
> look like this:
> #define tw_readl(reg)           readl(dev->lmmio + ((reg) >> 1))

As suggested above, just use a single void __iomem *mmio pointer.

> That looks odd.
> 
> In contrary, in drivers/media/pci/dm1105, we see no explicit shifting
> of register address. It uses {in,out}{b,w,l} macros, which seem to
> turn out the same {read,write}{b,w,l} (with some reservations):
> http://lxr.free-electrons.com/source/include/asm-generic/io.h#L354
> 
> dm1105.c:#define dm_io_mem(reg) ((unsigned long)(&dev->io_mem[reg]))
> dm1105.c:#define dm_readb(reg)          inb(dm_io_mem(reg))
> dm1105.c:#define dm_writeb(reg, value)  outb((value), (dm_io_mem(reg)))
> dm1105.c:#define dm_readw(reg)          inw(dm_io_mem(reg))
> dm1105.c:#define dm_writew(reg, value)  outw((value), (dm_io_mem(reg)))
> dm1105.c:#define dm_readl(reg)          inl(dm_io_mem(reg))
> dm1105.c:#define dm_writel(reg, value)  outl((value), (dm_io_mem(reg)))
> 
> This looks like contradiction to me (shifting register address vs. no
> shifting), so that one practice may be even just wrong and broken
> (which is hard to believe due to active maintenance of all drivers).
> I highly appreciate your help me in determining the best practice to
> follow in this new driver.
> Thanks.
> 

Hope this helps,

	Hans

Re: On register r/w macros/procedures of drivers/media/pci

[Andrey Utkin]

On Sun, Apr 19, 2015 at 11:28 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> Check the types of llmio and bbmio:
>
>         u32                     __iomem *lmmio;
>         u8                      __iomem *bmmio;
>
> So the values of the pointers are the same, but the types are not.
>
> So 'lmmio + 1' == 'bmmio + sizeof(u32)' == 'bbmio + 4'.
>
> Since all the registers are defined as byte offsets relative to the start
> of the memory map you cannot just do 'lmmio + reg' since that would be a
> factor 4 off. Instead you have to divide by 4 to get it back in line.
>
> Frankly, I don't think lmmio is necessary at all since readl/writel don't
> need a u32 pointer at all since they use void pointers. I never noticed
> that when I cleaned up the tw68 driver. Using 'void __iomem *mmio' instead
> of lmmio/bmmio and dropping the shifts in the tw_ macros would work just
> as well.

>
> Hope this helps,

Oh, indeed, I have forgot this basic thing of pointer arithmetics.
Thanks a lot for elaboration and the proposed solution.

-- 
Bluecherry developer.

Re: On register r/w macros/procedures of drivers/media/pci

[Krzysztof Hałasa]

Andrey Utkin <andrey.utkin@corp.bluecherry.net> writes:

> I am starting a work on driver for techwell tw5864 media grabber&encoder.

If this is tw6864 then I have a driver mostly completed.
Actually I'm using tw6869 but I think this is very similar (4 channels
instead of 8 and PCI instead of PCIe). I have 6864s but haven't yet
tried using them for trivial reasons.

This is BTW completely different chip (family) than the old tw68x.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: On register r/w macros/procedures of drivers/media/pci

[Andrey Utkin]

On Mon, Apr 20, 2015 at 2:18 PM, Krzysztof Hałasa <khalasa@piap.pl> wrote:
> Andrey Utkin <andrey.utkin@corp.bluecherry.net> writes:
>
>> I am starting a work on driver for techwell tw5864 media grabber&encoder.
>
> If this is tw6864 then I have a driver mostly completed.
> Actually I'm using tw6869 but I think this is very similar (4 channels
> instead of 8 and PCI instead of PCIe). I have 6864s but haven't yet
> tried using them for trivial reasons.
>
> This is BTW completely different chip (family) than the old tw68x.

Please check first digit. I mean _5_864, in your post there's 6869.
Please specify.
Very interested.anyway. Thanks.

-- 
Bluecherry developer.

Re: On register r/w macros/procedures of drivers/media/pci

[Krzysztof Hałasa]

Andrey Utkin <andrey.utkin@corp.bluecherry.net> writes:

> Please check first digit. I mean _5_864, in your post there's 6869.

Ok, I just thought it may be a typo. I can now see 5864 is a H.264
encoder, while 686x are simpler frame-grabbers only.

Sorry for the noise.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: On register r/w macros/procedures of drivers/media/pci

[Andrey Utkin]

On Mon, Apr 20, 2015 at 3:45 PM, Krzysztof Hałasa <khalasa@piap.pl> wrote:
> Andrey Utkin <andrey.utkin@corp.bluecherry.net> writes:
>
>> Please check first digit. I mean _5_864, in your post there's 6869.
>
> Ok, I just thought it may be a typo. I can now see 5864 is a H.264
> encoder, while 686x are simpler frame-grabbers only.
>
> Sorry for the noise.

Nothing to excuse for!
Thanks for the info anyway!

-- 
Bluecherry developer.