+++ Marek Vasut [2010-07-30 07:12 +0200]:
> Dne Pá 30. července 2010 07:02:46 Eric Miao napsal(a):
> > >> erm. Why move the FPGA/CPLD register definitions out of balloon3.h
> > >> (along with the others) and put them in balloon3.c?
> > >> Is that really good kernel form?
> > >
> > > They are not used anywhere else so they don't need to be in that file
> > > publically available.
> >
> > If the FPGA/CPLD is _not_ solely for the purpose of CF/PCMCIA,
> > nor it is like an MFD device where the functionality can be clearly
> > separated from different ranges of registers, I'd agree with Wookey
> > better to keep them in balloon3.h.
The FPGA/CPLD is used for NAND access, PCMCIA control, serial port
remapping, provides the Samosa bus (a simple 16-bit bus), address
mapping to the backplane concector, and any other
random functionality people add to the FPGA version (which has loads
of logic space, but very few pins, left).
It feels right to keep all the registers which are 'soft' and defined
by the VHDL rather than real hardware together in one header file to
me, but you guys are the style gurus round here.
> Well the registers used are solely for PCMCIA (or rather CF socket on the board,
> which clears your question below).
That is correct.
> > >> And surely keeping the FPGA_VIRT offset makes it clearer where the
> > >> magic addresses come from?
> > >
> > > You mean PHYS offset I believe. That is a good idea, but I'd firstly like
> > > to distribute all the addresses in balloon3.h into drivers (well all
> > > that can be distributed) and then flip it over so all registers will be
> > > defined by offset from some FPGA base address. That's not possible yet
> > > so for consistency's sake, I do it this way now. The patch flipping it
> > > to offsets will be very easy, but this cleanup needs a two-stage plan.
OK, makes sense.
> > And the offsets were a bit different from their originals here?
> I ioremap-ed them.
> >
> > > - #define BALLOON3_PCMCIA0_REG (BALLOON3_FPGA_VIRT +
> > > 0x00e00008)
> >
> > And
> >
> > > +#define BALLOON3_CF_STATUS_REG (0x10e00008)
> >
> > Even the name has been changed?
>
> This is more appropriate. The thing on the board is a CF socket.
Well, that depends what you plug in. These lines are available on the
backplane connector as well as the CF socket on the back of the board.
Nothing stopping you putting a PCMCIA socket (or device) on a board
plugged in the backplane and then it would be PCMCIA. I guess we used
'PCMCIA' because the pxa270 docs did? It's not at all a big deal.
In practice no-one has made such an add-on PCMCIA board yet so this
argument is not very convincing - but that's why things were named as
they were.
If we change the reg names to the more helpful '*_CF_*' then maybe the
function names in balloon3_pcmcia_ops should change too to match for
consistency?
> > >> And finally as this code is quite dependent on VHDL version numbers
> > >> and we know we have a number of incompatible versions around
> > >> should we add a version check to ensure that we have the same VHDL
> > >> version as the driver is expecting? I guess it should be of the 'at
> > >> least n' form, otherwise later VHDL updates would break the driver,
> > >> probbaly for no good reason.
> > >
> > > Yes, the check is in place.
> > >
> > > + if (ver > 0x0201)
> > > + pr_warn("The FPGA code, version 0x%04x, is newer than
> > > rel-0.3. " + "PCMCIA/CF support might be broken in
> > > this version!", + ver);
Sorry, missed that. :-)
Thinking about it, I guess that's an argument for keeping the Reg
definitions with the driver so they can go inside the version check,
as they will change with later VHDL+PCMCIA driver pairs:
e.g. in current (broken for PCMCIA) FPGA the reg addresses are (the status/set reg moved):
+/* FPGA/CPLD registers */
+#define BALLOON3_CF_CONTROL_CLEAR_ADDR __BALLOON3_REG(BALLOON3_FPGA_PHYS+0x00e00008)
+#define BALLOON3_CF_STATUS_ADDR __BALLOON3_REG(BALLOON3_FPGA_PHYS+0x00e00008)
+#define BALLOON3_CF_CONTROL_SET_ADDR __BALLOON3_REG(BALLOON3_FPGA_PHYS+0x00e01008)
Wookey
--
Principal hats: Linaro, Emdebian, Wookware, Balloonboard, ARM
http://wookware.org/
(text/plain)