Re: [PATCH v4 1/2] hw/cxl: Refactored Identify Switch Device & Get Physical Port State

[Arpit Kumar <arpit1.kumar@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 4192 bytes --]

On 17/09/25 04:55PM, Jonathan Cameron wrote:
>On Tue, 16 Sep 2025 13:37:35 +0530
>Arpit Kumar <arpit1.kumar@samsung.com> wrote:
>
>> -Storing physical ports info during enumeration.
>> -Refactored changes using physical ports info for
>>  Identify Switch Device (Opcode 5100h) & Get Physical Port State
>>  (Opcode 5101h) physical switch FM-API command set.
>>
>> Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
>
>Hi Arpit.  One question inline, and one comment on code I've moved
>around whilst queue this up.  I'll push out a tree to gitlab
>(probably tomorrow) and when I do please check I didn't mess that up!
>
>Jonathan
>

Hi Jonathan,
Thank you for the swift response and review comments.
Sure, will look into gitlab tree once up.

Thanks,
Arpit
>
>> +static CXLRetCode cxl_set_port_type(CXLUpstreamPort *ports, int pnum,
>> +                                    CXLCCI *cci)
>> +{
>> +    uint8_t current_port_config_state;
>> +    uint8_t connected_device_type;
>> +    uint8_t supported_ld_count;
>> +    uint16_t lnkcap, lnkcap2, lnksta;
>> +    PCIBus *bus;
>> +    PCIDevice *port_dev;
>> +    PCIEPort *usp = PCIE_PORT(cci->d);
>> +
>> +    if (usp->port == pnum) {
>> +        port_dev = PCI_DEVICE(usp);
>> +        current_port_config_state = CXL_PORT_CONFIG_STATE_USP;
>> +        connected_device_type = CXL_PORT_CONNECTED_DEV_TYPE_NONE;
>> +        supported_ld_count = 0;
>> +    } else {
>> +        bus = &PCI_BRIDGE(cci->d)->sec_bus;
>> +        port_dev = pcie_find_port_by_pn(bus, pnum);
>> +        if (port_dev) { /* DSP */
>> +            PCIDevice *ds_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(port_dev))
>> +                ->devices[0];
>> +            current_port_config_state = CXL_PORT_CONFIG_STATE_DSP;
>> +            if (ds_dev) {
>> +                if (object_dynamic_cast(OBJECT(ds_dev), TYPE_CXL_TYPE3)) {
>> +                    /* To-do: controllable */
>
>In what sense controllable?  It should always match what the downstream device
>is presenting as.  Do you ultimately mean if we mess with the alternate modes
>and reset the port to have it come up as a PCI only device?
>This will need to be more complex as we add different CXL type 3 device support
>of course, but I'd still expect to auto detect it rather that control it directly.
>

This is with respect to your review comment from v1 patch:
https://lore.kernel.org/qemu-devel/20250602135942.2773823-1-arpit1.kumar@samsung.com/T/
As per my understanding, controllable was identification of the specific type of 
CXL type 3 device and accordingly initializing connected_device_type. Since you 
mention auto-detect, does it mean using object_get_typename() to identify the type
of device and initiliaze it directly to connected_device_type rather than specifying
it explicitly. If yes, then this anyways rules out controllable part, hence the comment
can be removed.

>> +                    connected_device_type = CXL_PORT_CONNECTED_DEV_TYPE_3_SLD;
>> +                } else {
>> +                    connected_device_type = CXL_PORT_CONNECTED_DEV_TYPE_PCIE;
>> +                }
>> +            } else {
>> +                connected_device_type = CXL_PORT_CONNECTED_DEV_TYPE_NONE;
>> +            }
>> +            supported_ld_count = 3;
>> +        } else {
>> +            return CXL_MBOX_INVALID_INPUT;
>> +        }
>> +    }
>
>>  void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
>>                                    DeviceState *d, size_t payload_max)
>>  {
>> @@ -4691,6 +4706,7 @@ void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
>>      cci->d = d;
>>      cci->intf = intf;
>>      cxl_init_cci(cci, payload_max);
>> +    cxl_set_phy_port_info(cci);
>>  }
>>
>>  void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max)
>> @@ -4777,4 +4793,5 @@ void cxl_initialize_usp_mctpcci(CXLCCI *cci, DeviceState *d, DeviceState *intf,
>>      cci->d = d;
>>      cci->intf = intf;
>>      cxl_init_cci(cci, payload_max);
>> +    cxl_set_phy_port_info(cci);
>
>I'll shift this to a later patch whilst picking this up for my staging tree.
>I want this ahead of where we introduce cxl_initialize_usp_mctpcci.
>
Okay
>>  }

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]