Dne Pá 30. července 2010 14:54:13 Wookey napsal(a):
> +++ 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.
We're keeping them in the main header file for now I guess. But in the end, I'd
like to push offsets from the FPGA_VIRT into drivers. So have only FPGA_VIRT in
the main header file and offset in drivers. That might be good. But I'll see.
>
> > 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?
It doesn't really matter.
>
> > > >> 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+0x00e000
> 08) +#define
> BALLOON3_CF_STATUS_ADDR
__BALLOON3_REG(BALLOON3_FPGA_PHYS+0x00e00008)
> +#define
> BALLOON3_CF_CONTROL_SET_ADDR
__BALLOON3_REG(BALLOON3_FPGA_PHYS+0x00e01008
> )
I believe eventually this can be implemented for both v0.3 and v1.0 of FPGA.
There would just be two pairs of offsets in the driver in the end. But for now,
let's stick with v0.3 version of FPGA.
>
>
> Wookey
(text/plain)