On Wed, Jun 26, 2013 at 3:01 PM, Sébastien Bourdeauducq <sebastien.bourdeaud...@lekernel.net> wrote: > On 06/26/2013 12:51 AM, Robert Jördens wrote: >> >> The first is the added finalize() method, > > Don't you want Mibuild's build() method to automatically call finalize() > instead of the user? > If the platform's finalize() is not wanted by some design, the method can > always be overloaded by deriving the Platform class. But I think most > designs want it, so it should be the default.
ack.
>> +_io = [
>> + ("usr_button", 0, Pins("V4"), IOStandard("LVCMOS33"),
>
> This should be named "user_btn" to be consistent with the other boards. The
> idea is that the code can be copy and pasted across boards without having to
> look up every time how the first pushbutton is called on the board du jour.
ack.
>> + ("clk_y1", 0, Pins("V10"), IOStandard("LVCMOS33")), # 40
>> MHz
>> + ("clk_y2", 0, Pins("K15"), IOStandard("LVCMOS33")), # 66
>> 2/3 MHz
>> + ("clk_y3", 0, Pins("C10"), IOStandard("LVCMOS33")), # 100
>> MHz
>
> Similarly, those should be called clk40, clk66_2_3 and clk100.
Those come in from external PLL. Their frequencies are programmable
and these are just the factory defaults. I'd rather keep those names
and have them aliased in projects that may or may not reprogram them
to different values. I reworded the comments though and removed the
add_platform_command() timing constraints for these.
>> + ("usr", 0,
>> + Subsignal("dip", Pins("B3 A3 B4 A4"),
>> Misc("PULLDOWN"),
>> + IOStandard("LVCMOS33")),
>> + Subsignal("led", Pins("P4 L6 F5 C2"),
>> Misc("SLEW=QUIETIO"),
>> + IOStandard("LVCMOS18")),
>> + ),
>
> Is there a reason for not using "user_led" (and outside a signal group) like
> on the other boards?
ack
>> +class Lx9Microboard(XilinxISEPlatform):
>
> The class should be named Platform (the board name is already in the module
> name).
That goes against all the other instances where the class name _is_
the module name. I don't see why it would be wise to have the same
class name correspond to different things.
But ack as the other platforms have it that way.
>> + def finalize(self, top):
>> + self.add_platform_command("""
>> +CONFIG VCCAUX = "3.3";
>
> This should be moved to __init__.
ack
>> +CONFIG PROHIBIT = P1,L3;
>
>
> I don't think CONFIG PROHIBIT makes sense when using Mibuild, which locks
> all IO pins - am I wrong?
ack
>> +PIN "BUFG_1.O" CLOCK_DEDICATED_ROUTE = FALSE;
>
> This constraint is specific to a design and should not be in the platform
> file.
ack.
>> A sidenote: Maybe literals (constants) should be verilog'ed as signed
>> constants.
>> Otherwise "signed_signal >= 0" is always true as 0 get output as
>> "1'd0" making the comparison unsigned. At least AFAICT.
>
> Fixed.
Thanks.
--
Robert Jordens.
0001-generic_platform.py-add-a-finalize-method.patch
Description: Binary data
0002-add-Avnet-Spartan6-LX9-Micrboard-platform.patch
Description: Binary data
_______________________________________________ Devel mailing list Devel@lists.milkymist.org https://ssl.serverraum.org/lists/listinfo/devel
