Re: [PATCH v5 07/10] PCI/IDE: Add IDE establishment helpers

[Alexey Kardashevskiy <aik@amd.com>]

On 5/9/25 11:27, dan.j.williams@intel.com wrote:
> Alexey Kardashevskiy wrote:
> [..]
>>> +static void set_ide_sel_ctl(struct pci_dev *pdev, struct pci_ide *ide, int pos,
>>> +			    bool enable)
>>> +{
>>> +	u32 val = FIELD_PREP(PCI_IDE_SEL_CTL_ID, ide->stream_id) |
>>> +		  FIELD_PREP(PCI_IDE_SEL_CTL_CFG_EN, pdev->ide_cfg) |
>>
>> There was:
>> FIELD_PREP(PCI_IDE_SEL_CTL_DEFAULT, 1) |
>>
>> and now it is gone, why? And it is not in any change log, took me a while to find out what broke.
> 
> Oh, sorry, it results from this feedback.
> 
> http://lore.kernel.org/9683c850-3152-4da5-97f1-3e86ba39e8d3@nvidia.com
> 
> ...but since the address association registers are not programmed then
> nothing routes TLPs to the IDE stream. My mistake.
> 
> We may eventually need the ability for the stream allocation path to also
> allocate a traffic class in the root-port, but for now this assumes single
> device TC==0.
> 
> For now I am adding this:
> 
> -- 8< --
> diff --git a/drivers/pci/ide.c b/drivers/pci/ide.c
> index 4f5aa42e05ef..610603865d9e 100644
> --- a/drivers/pci/ide.c
> +++ b/drivers/pci/ide.c
> @@ -379,10 +379,12 @@ struct pci_ide_partner *pci_ide_to_settings(struct pci_dev *pdev, struct pci_ide
>   }
>   EXPORT_SYMBOL_GPL(pci_ide_to_settings);
>   
> -static void set_ide_sel_ctl(struct pci_dev *pdev, struct pci_ide *ide, int pos,
> +static void set_ide_sel_ctl(struct pci_dev *pdev, struct pci_ide *ide,
> +			    struct pci_ide_partner *settings, int pos,
>   			    bool enable)
>   {
>   	u32 val = FIELD_PREP(PCI_IDE_SEL_CTL_ID, ide->stream_id) |
> +		  FIELD_PREP(PCI_IDE_SEL_CTL_DEFAULT, settings->default_stream) |
>   		  FIELD_PREP(PCI_IDE_SEL_CTL_CFG_EN, pdev->ide_cfg) |
>   		  FIELD_PREP(PCI_IDE_SEL_CTL_TEE_LIMITED, pdev->ide_tee_limit) |
>   		  FIELD_PREP(PCI_IDE_SEL_CTL_EN, enable);
> @@ -424,7 +426,7 @@ void pci_ide_stream_setup(struct pci_dev *pdev, struct pci_ide *ide)
>   	 * Setup control register early for devices that expect
>   	 * stream_id is set during key programming.
>   	 */
> -	set_ide_sel_ctl(pdev, ide, pos, false);
> +	set_ide_sel_ctl(pdev, ide, settings, pos, false);
>   	settings->setup = 1;
>   }
>   EXPORT_SYMBOL_GPL(pci_ide_stream_setup);
> @@ -481,12 +483,12 @@ int pci_ide_stream_enable(struct pci_dev *pdev, struct pci_ide *ide)
>   
>   	pos = sel_ide_offset(pdev, settings);
>   
> -	set_ide_sel_ctl(pdev, ide, pos, true);
> +	set_ide_sel_ctl(pdev, ide, settings, pos, true);
>   
>   	pci_read_config_dword(pdev, pos + PCI_IDE_SEL_STS, &val);
>   	if (FIELD_GET(PCI_IDE_SEL_STS_STATE, val) !=
>   	    PCI_IDE_SEL_STS_STATE_SECURE) {
> -		set_ide_sel_ctl(pdev, ide, pos, false);
> +		set_ide_sel_ctl(pdev, ide, settings, pos, false);
>   		return -ENXIO;
>   	}
>   
> diff --git a/include/linux/pci-ide.h b/include/linux/pci-ide.h
> index cf1f7a10e8e0..a2d3fb4a289b 100644
> --- a/include/linux/pci-ide.h
> +++ b/include/linux/pci-ide.h
> @@ -24,6 +24,8 @@ enum pci_ide_partner_select {
>    * @rid_start: Partner Port Requester ID range start
>    * @rid_start: Partner Port Requester ID range end
>    * @stream_index: Selective IDE Stream Register Block selection
> + * @default_stream: Endpoint uses this stream for all upstream TLPs regardless of
> + * 		    address and RID association registers
>    * @setup: flag to track whether to run pci_ide_stream_teardown() for this
>    *	   partner slot
>    * @enable: flag whether to run pci_ide_stream_disable() for this partner slot
> @@ -32,6 +34,7 @@ struct pci_ide_partner {
>   	u16 rid_start;
>   	u16 rid_end;
>   	u8 stream_index;
> +	unsigned int default_stream:1;


This sets "Default" on both ends and the rootport does not need it in my setup (it does not seem to affect anything though) - the  rootport always knows the stream ID from the RMP entry of a MMIO being accessed. May be move it to pci_ide_partner? Thanks,

>   	unsigned int setup:1;
>   	unsigned int enable:1;
>   };

-- 
Alexey