Re: [PATCH 1/1] pickles: Add new PCI Pickle

[Darshit Shah]

Hi Mohammed,

Thanks for the review. Some responses inline...

On Tue, Sep 17, 2024, at 23:23, Mohammad-Reza Nabipoor wrote:
> Hi Darshit.
>
> Thank you for the patch.
>
>
> On Tue, Sep 17, 2024 at 08:33:42PM GMT, Darshit Shah wrote:
>> diff --git a/pickles/pci/pci_common.pk b/pickles/pci/pci_common.pk
>> new file mode 100644
>> index 0000000..4e91745
>> --- /dev/null
>> +++ b/pickles/pci/pci_common.pk
> [...]
>> +/**
>> + * Basic Registers
>> + */
>> +type reg_32 = uint<32>;
>> +type reg_16 = uint<16>;
>> +type reg_8 = uint<8>;
>
>
> Do we really need these types and uint<...> is not readable enough?
> If yes, please use names like `PCI_Reg32`, `PCI_Reg16` and `PCI_Reg8`.
>
It's not strictly necessary. I started writing this for a personal project and 
found the reg_* format more readable.
But I'm getting rid of it in favour of just using the native data types.

>
>> +
>> +//  While the PCI base specification differentiates between RsvdZ and 
>> RsvdP, we
>> +//  don't need that in Poke. The only difference between the two is what 
>> happens
>> +//  when you write to the bit, which is not something our representation 
>> needs
>> +//  to care about.
>> +type rsvdz = struct bit {
>> +    bit rsvdz == 0x0;
>> +    method _print = void:
>> +    {
>> +        print "rsvdz: 0";
>> +    }
>> +};
>
>
> We use the following code style:
>
> type PIC_RsvdZ =
>   struct
>     {
>       uint<1> rsvdz == 0;
>
>       // ...
>     };
>
> And `bit' is defined in `std-types.pk' file.
> This file is not loaded by some users of `libpoke' (like `gdb' integration).
> To make this pickle also useful for those applications, it's better to not use
> these "Standard Types".

This hurts the most. `bit` as a data type is a lot more readable than 
`uint<1>`. But for the moment,
I'm switching to using the uint directly.

Despite having removed the 3 definitions, i'm still keeping the file, since it 
will come in handy in the future
when I extend the pickle to also describe the various capabilities

>
>> -- 
>> 2.46.1
>> 
>> 
>> 
>> diff --git a/pickles/pci/pci.pk b/pickles/pci/pci.pk
>> new file mode 100644
>> index 0000000..10fb5ac
>> --- /dev/null
>> +++ b/pickles/pci/pci.pk
>> +
>> +type PCI_Command = struct reg_16 {
>> +    rsvdz;
>> +    rsvdz;
>> +    rsvdz;
>> +    rsvdz;
>> +    rsvdz;
>> +    bit int_disable;
>> +    bit fast_b2b_tx_enable == 0b0;
>
>
> Instead of `0b0' you can use `0' or `0T' or `0t`.
> `0T' and `0t' are of type `uint<1>'.
> `0' and `0b0' are of type `int<32>' (which is compatible due to implicit
> conversions).

Done
>
>
>> +    bit serr_enable;
>> +    bit idsel_stepping == 0b0;
>> +    bit parity_error_response;
>> +    bit vga_palette_snoop == 0b0;
>> +    bit mem_write_invalidate == 0b0;
>> +    bit sp_cycle_enable == 0b0;
>> +    bit bus_master_enable;
>> +    bit mem_enable;
>> +    bit io_enable;
>> +};
>> +
>> +type PCI_Status = struct reg_16 {
>> +    bit parity_error;
>> +    bit sig_sys_error;
>> +    bit rcvd_master_abort;
>> +    bit rcvd_target_abort;
>> +    bit sig_target_abort;
>> +    uint<2> devsel_timing == 0 as uint<2>;
>
> The `as uint<2>' is not necessary.

Done
>
>> +    bit master_data_parity_error;
>> +    bit fast_b2b_tx_cap;
>> +    rsvdz;
>> +    bit Mhz66_cap == 0b0;
>> +    bit cap_list;
>> +    bit int_status;
>> +    rsvdz;
>> +    rsvdz;
>> +    bit imm_readiness;
>> +};
>> +
>> +type PCI_Hdr_Type = struct reg_8 {
>> +    uint<7> hdr_layout;
>> +    bit multi_fn;
>> +};
>> +
>> +type PCI_BAR = struct {
>> +    reg_32;
>> +};
>> +
>> +type PCI_Type0 = struct {
>> +    PCI_BAR[6] bars;
>> +    reg_32 cardbus_cis_ptr == 0x0U;
>> +    reg_16 subsystem_vendor_id;
>> +    reg_16 subsystem_id;
>> +    reg_32 exp_rom_base_addr;
>> +    offset<reg_8,B> cap_ptr;
>> +    uint<56> rsvdz == 0;
>> +    reg_8 int_line;
>> +    reg_8 int_pin;
>> +    reg_8 min_gnt;
>> +    reg_8 max_lat;
>> +};
>
>
> It is a good practice to add a size assertion after defining these
> types like: `assert (#PCI_Type0 == 48#B);'
> ```
>
Done. And thanks since this found a bug in my definitions
>
>> +
>> +type PCI_Type1  = struct {
>> +    PCI_BAR bar_0;
>> +    PCI_BAR bar_1;
>> +    reg_8 primary_bus_nr;
>> +    reg_8 sec_bus_nr;
>> +    reg_8 sub_bus_nr;
>> +    reg_8 sec_lat_timer;
>> +    reg_8 io_base;
>> +    reg_8 io_limit;
>> +    reg_16 sec_status;
>> +    reg_16 mem_base;
>> +    reg_16 mem_limit;
>> +    reg_16 prefetch_mem_base;
>> +    reg_16 prefetch_mem_limit;
>> +    reg_32 prefetch_mem_base_upper;
>> +    reg_32 prefetch_mem_limit_upper;
>> +    reg_16 io_base_upper;
>> +    reg_16 io_base_limit;
>> +    offset<reg_16,B> cap_ptr;
>> +    uint<24> rsvdz == 0;
>> +    reg_32 exp_rom_base;
>> +    reg_8 int_line;
>> +    reg_8 int_pin;
>> +    reg_16 bridge_ctrl;
>> +};
>> +
>> +
>> +type PCI_Config_Space = struct {
>> +    reg_16 vendor;
>> +    reg_16 device;
>> +    PCI_Command command;
>> +    PCI_Status status;
>> +    reg_8 revision_id;
>> +    uint<24> class_code;
>> +    reg_8 cache_line_size;
>> +    reg_8 pri_lat_timer == 0;
>> +    PCI_Hdr_Type header;
>> +    reg_8 BIST;
>> +    if (header.hdr_layout == 0b0)
>> +        PCI_Type0 endpt_specific_data;
>> +    if (header.hdr_layout == 0b1)
>> +        PCI_Type1 bridge_specific_data;
>> +    // These are technically not a part of Type 0 / Type 1 structures
>> +    // But I define them in there for ease. Instead, we can use Field Labels
>> +    // to create virtual fields here that point to the correct data
>> +    reg_8 cap_ptr @ 0x34#B;
>> +    reg_8 int_line @ 0x3C#B;
>> +    reg_8 int_pin @ 0x3D#B;
>
>
> Instead of field labels (which only works for mapped data), you can use 
> `computed' fields:
>
> ```
>       computed uint<8> cap_ptr;
>       computed uint<8> int_line;
>       computed uint<8> int_pin;
>
>       method get_cap_ptr = uint<8>:
>       { return header.hdr_layout == 0 ? endpt_specific_data.cap_ptr : /* ... 
> */ ; }
>       method get_int_line = uint<8>: {/*...*/}
>       method get_int_pin  = uint<8>: {/*...*/}
>
>       method set_cap_ptr = (uint<8> cap_ptr) void: { /* ... */ }
>       // ...
> ```
>

It's a lot more verbose than the field labels. But I've made the appropriate 
changes.

>
>> +};
>> +
>> +assert ((PCI_Config_Space {})'size == 0x40#B);
>
>
>
> Regards,
> Mohammad-Reza