* in_be32() etc
@ 2012-02-23 11:29 Russell King - ARM Linux
2012-02-23 20:18 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 4+ messages in thread
From: Russell King - ARM Linux @ 2012-02-23 11:29 UTC (permalink / raw)
To: Grant Likely, Benjamin Herrenschmidt, Paul Mackerras,
linuxppc-dev
What's this stuff doing in generic drivers?
See drivers/gpio/gpio-xilinx.c:
static int xgpio_get(struct gpio_chip *gc, unsigned int gpio)
{
struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
return (in_be32(mm_gc->regs + XGPIO_DATA_OFFSET) >> gpio) & 1;
}
include/linux/of_gpio.h:
struct of_mm_gpio_chip {
struct gpio_chip gc;
void (*save_regs)(struct of_mm_gpio_chip *mm_gc);
void __iomem *regs;
};
Why am I being asked to add in_be32() etc to ARMs io.h ? Why do we need
yet another set of IO accessors? Is there something wrong with
ioread*()/ioread*be() etc?
My guess is this stems from a lack of proper review:
/*
* Low level MMIO accessors
*
* This provides the non-bus specific accessors to MMIO. Those are PowerPC
* specific and thus shouldn't be used in generic code. The accessors
* provided here are:
*
* in_8, in_le16, in_be16, in_le32, in_be32, in_le64, in_be64
* out_8, out_le16, out_be16, out_le32, out_be32, out_le64, out_be64
* _insb, _insw_ns, _insl_ns, _outsb, _outsw_ns, _outsl_ns
$ grep '\(out\|in\)_be32' drivers/ -rl
drivers/media/video/fsl-viu.c
drivers/macintosh/via-pmu.c
drivers/gpio/gpio-mpc8xxx.c
drivers/gpio/gpio-mpc5200.c
drivers/gpio/gpio-xilinx.c
drivers/edac/mpc85xx_edac.c
drivers/tty/serial/mpc52xx_uart.c
drivers/tty/serial/ucc_uart.c
drivers/tty/serial/cpm_uart/cpm_uart_core.c
drivers/scsi/mvme16x_scsi.c
drivers/char/xilinx_hwicap/buffer_icap.c
drivers/char/xilinx_hwicap/fifo_icap.c
drivers/rtc/rtc-mpc5121.c
drivers/pcmcia/m8xx_pcmcia.c
drivers/ata/pata_scc.c
drivers/ata/pata_mpc52xx.c
drivers/ide/scc_pata.c
drivers/net/ethernet/xilinx/xilinx_emaclite.c
drivers/net/ethernet/xilinx/ll_temac_main.c
drivers/net/ethernet/freescale/ucc_geth.c
drivers/net/ethernet/freescale/fsl_pq_mdio.c
drivers/net/ethernet/freescale/fec_mpc52xx.c
drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
drivers/net/ethernet/freescale/fs_enet/mii-fec.c
drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
drivers/net/ethernet/freescale/fs_enet/mac-fec.c
drivers/net/ethernet/freescale/fs_enet/mac-scc.c
drivers/net/ethernet/freescale/fs_enet/fs_enet.h
drivers/net/ethernet/freescale/fec_mpc52xx_phy.c
drivers/net/ethernet/freescale/gianfar.h
drivers/net/ethernet/freescale/ucc_geth_ethtool.c
drivers/net/ethernet/tundra/tsi108_eth.h
drivers/net/ethernet/ibm/emac/tah.c
drivers/net/ethernet/ibm/emac/zmii.c
drivers/net/ethernet/ibm/emac/rgmii.c
drivers/net/ethernet/ibm/emac/core.c
drivers/net/ethernet/ibm/emac/debug.c
drivers/net/ethernet/toshiba/spider_net.c
drivers/net/can/flexcan.c
drivers/net/can/mscan/mpc5xxx_can.c
drivers/video/xilinxfb.c
drivers/video/fsl-diu-fb.c
drivers/video/platinumfb.c
drivers/spi/spi-fsl-spi.c
drivers/spi/spi-fsl-lib.h
drivers/spi/spi-mpc52xx-psc.c
drivers/spi/spi-mpc512x-psc.c
drivers/watchdog/mpc8xxx_wdt.c
drivers/watchdog/pika_wdt.c
drivers/mmc/host/sdhci-pltfm.h
drivers/mmc/host/sdhci-of-esdhc.c
drivers/crypto/caam/regs.h
drivers/crypto/talitos.c
drivers/usb/gadget/fsl_qe_udc.c
drivers/usb/gadget/fsl_udc_core.c
drivers/usb/host/fhci-hcd.c
drivers/usb/host/ehci-fsl.c
drivers/usb/host/ehci-ppc-of.c
drivers/usb/host/fhci-tds.c
drivers/usb/host/fsl-mph-dr-of.c
drivers/usb/otg/fsl_otg.c
drivers/dma/fsldma.c
drivers/dma/fsldma.h
drivers/dma/mpc512x_dma.c
drivers/i2c/busses/i2c-mpc.c
drivers/i2c/busses/i2c-cpm.c
drivers/input/serio/xilinx_ps2.c
drivers/isdn/hisax/avm_pci.c
drivers/mtd/nand/ppchameleonevb.c
drivers/mtd/nand/fsl_elbc_nand.c
drivers/mtd/nand/mpc5121_nfc.c
drivers/mtd/nand/ndfc.c
drivers/mtd/nand/socrates_nand.c
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: in_be32() etc
2012-02-23 11:29 in_be32() etc Russell King - ARM Linux
@ 2012-02-23 20:18 ` Benjamin Herrenschmidt
2012-02-23 20:25 ` Russell King - ARM Linux
0 siblings, 1 reply; 4+ messages in thread
From: Benjamin Herrenschmidt @ 2012-02-23 20:18 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: linuxppc-dev, Paul Mackerras
On Thu, 2012-02-23 at 11:29 +0000, Russell King - ARM Linux wrote:
> What's this stuff doing in generic drivers?
Well, I suppose that's because the xilinx stuff used to be ppc
only ? :-)
> See drivers/gpio/gpio-xilinx.c:
> static int xgpio_get(struct gpio_chip *gc, unsigned int gpio)
> {
> struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
>
> return (in_be32(mm_gc->regs + XGPIO_DATA_OFFSET) >> gpio) & 1;
> }
>
> include/linux/of_gpio.h:
> struct of_mm_gpio_chip {
> struct gpio_chip gc;
> void (*save_regs)(struct of_mm_gpio_chip *mm_gc);
> void __iomem *regs;
> };
>
> Why am I being asked to add in_be32() etc to ARMs io.h ? Why do we need
> yet another set of IO accessors? Is there something wrong with
> ioread*()/ioread*be() etc?
Nope, nothing wrong with them, the driver should be fixed. in_be* is
historical ppc stuff.
> My guess is this stems from a lack of proper review
That or history. Our readX/writeX used to be more PCI specific (have
infrastructure to work around PCI bridge bugs) which some drivers
avoided using the in_/out_ variants, in some case it's just pure
history, etc... Some of these things are ancient.
Cheers,
Ben.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: in_be32() etc
2012-02-23 20:18 ` Benjamin Herrenschmidt
@ 2012-02-23 20:25 ` Russell King - ARM Linux
2012-02-23 20:27 ` Grant Likely
0 siblings, 1 reply; 4+ messages in thread
From: Russell King - ARM Linux @ 2012-02-23 20:25 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Paul Mackerras
On Fri, Feb 24, 2012 at 07:18:59AM +1100, Benjamin Herrenschmidt wrote:
> On Thu, 2012-02-23 at 11:29 +0000, Russell King - ARM Linux wrote:
> > What's this stuff doing in generic drivers?
>
> Well, I suppose that's because the xilinx stuff used to be ppc
> only ? :-)
Note that's just the first one grep turned up. I've no idea which specific
drivers the ST SPEAr people are wanting these accessors for (they didn't
include that information in their submission adding them to ARM.)
> > See drivers/gpio/gpio-xilinx.c:
> > static int xgpio_get(struct gpio_chip *gc, unsigned int gpio)
> > {
> > struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> >
> > return (in_be32(mm_gc->regs + XGPIO_DATA_OFFSET) >> gpio) & 1;
> > }
> >
> > include/linux/of_gpio.h:
> > struct of_mm_gpio_chip {
> > struct gpio_chip gc;
> > void (*save_regs)(struct of_mm_gpio_chip *mm_gc);
> > void __iomem *regs;
> > };
> >
> > Why am I being asked to add in_be32() etc to ARMs io.h ? Why do we need
> > yet another set of IO accessors? Is there something wrong with
> > ioread*()/ioread*be() etc?
>
> Nope, nothing wrong with them, the driver should be fixed. in_be* is
> historical ppc stuff.
>
> > My guess is this stems from a lack of proper review
>
> That or history. Our readX/writeX used to be more PCI specific (have
> infrastructure to work around PCI bridge bugs) which some drivers
> avoided using the in_/out_ variants, in some case it's just pure
> history, etc... Some of these things are ancient.
So, if I tell the SPEAr people that any driver they come across using
these old in_XX should be converted to use ioread*() you'll be happy
for that to happen?
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: in_be32() etc
2012-02-23 20:25 ` Russell King - ARM Linux
@ 2012-02-23 20:27 ` Grant Likely
0 siblings, 0 replies; 4+ messages in thread
From: Grant Likely @ 2012-02-23 20:27 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Paul Mackerras, linuxppc-dev
On Thu, Feb 23, 2012 at 1:25 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Feb 24, 2012 at 07:18:59AM +1100, Benjamin Herrenschmidt wrote:
>> On Thu, 2012-02-23 at 11:29 +0000, Russell King - ARM Linux wrote:
>> > What's this stuff doing in generic drivers?
>>
>> Well, I suppose that's because the xilinx stuff used to be ppc
>> only ? :-)
>
> Note that's just the first one grep turned up. =A0I've no idea which spec=
ific
> drivers the ST SPEAr people are wanting these accessors for (they didn't
> include that information in their submission adding them to ARM.)
>
>> > See drivers/gpio/gpio-xilinx.c:
>> > static int xgpio_get(struct gpio_chip *gc, unsigned int gpio)
>> > {
>> > =A0 =A0 =A0 =A0 struct of_mm_gpio_chip *mm_gc =3D to_of_mm_gpio_chip(g=
c);
>> >
>> > =A0 =A0 =A0 =A0 return (in_be32(mm_gc->regs + XGPIO_DATA_OFFSET) >> gp=
io) & 1;
>> > }
>> >
>> > include/linux/of_gpio.h:
>> > struct of_mm_gpio_chip {
>> > =A0 =A0 =A0 =A0 struct gpio_chip gc;
>> > =A0 =A0 =A0 =A0 void (*save_regs)(struct of_mm_gpio_chip *mm_gc);
>> > =A0 =A0 =A0 =A0 void __iomem *regs;
>> > };
>> >
>> > Why am I being asked to add in_be32() etc to ARMs io.h ? =A0Why do we =
need
>> > yet another set of IO accessors? =A0Is there something wrong with
>> > ioread*()/ioread*be() etc?
>>
>> Nope, nothing wrong with them, the driver should be fixed. in_be* is
>> historical ppc stuff.
>>
>> > My guess is this stems from a lack of proper review
>>
>> That or history. Our readX/writeX used to be more PCI specific (have
>> infrastructure to work around PCI bridge bugs) which some drivers
>> avoided using the in_/out_ variants, in some case it's just pure
>> history, etc... Some of these things are ancient.
>
> So, if I tell the SPEAr people that any driver they come across using
> these old in_XX should be converted to use ioread*() you'll be happy
> for that to happen?
yes
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-02-23 20:27 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-02-23 11:29 in_be32() etc Russell King - ARM Linux
2012-02-23 20:18 ` Benjamin Herrenschmidt
2012-02-23 20:25 ` Russell King - ARM Linux
2012-02-23 20:27 ` Grant Likely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).