[v3,2/4] PCI: of: Add API to retrieve equalization presets from device tree

Commit Message

Krishna Chaitanya Chundru Dec. 23, 2024, 6:51 a.m. UTC

PCIe equalization presets are predefined settings used to optimize
signal integrity by compensating for signal loss and distortion in
high-speed data transmission.

As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
configure lane equalization presets for each lane to enhance the PCIe
link reliability. Each preset value represents a different combination
of pre-shoot and de-emphasis values. For each data rate, different
registers are defined: for 8.0 GT/s, registers are defined in section
7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
an extra receiver preset hint, requiring 16 bits per lane, while the
remaining data rates use 8 bits per lane.

Based on the number of lanes and the supported data rate, this function
reads the device tree property and stores in the presets structure.

Signed-off-by: Krishna Chaitanya Chundru <[e-mail]>
---
 drivers/pci/of.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h | 17 +++++++++++++++--
 2 files changed, 60 insertions(+), 2 deletions(-)

Comments

Dmitry Baryshkov Dec. 23, 2024, 11:47 a.m. UTC | #1

On Mon, Dec 23, 2024 at 12:21:15PM +0530, Krishna Chaitanya Chundru wrote:
> PCIe equalization presets are predefined settings used to optimize
> signal integrity by compensating for signal loss and distortion in
> high-speed data transmission.
> 
> As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
> of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
> configure lane equalization presets for each lane to enhance the PCIe
> link reliability. Each preset value represents a different combination
> of pre-shoot and de-emphasis values. For each data rate, different
> registers are defined: for 8.0 GT/s, registers are defined in section
> 7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
> an extra receiver preset hint, requiring 16 bits per lane, while the
> remaining data rates use 8 bits per lane.
> 
> Based on the number of lanes and the supported data rate, this function
> reads the device tree property and stores in the presets structure.
> 
> Signed-off-by: Krishna Chaitanya Chundru <[e-mail]>
> ---
>  drivers/pci/of.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.h | 17 +++++++++++++++--
>  2 files changed, 60 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index dacea3fc5128..99e0e7ae12e9 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -826,3 +826,48 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
>  	return slot_power_limit_mw;
>  }
>  EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
> +

kerneldoc? Define who should free the memory and how.

> +int of_pci_get_equalization_presets(struct device *dev,
> +				    struct pci_eq_presets *presets,
> +				    int num_lanes)
> +{
> +	char name[20];
> +	void **preset;
> +	void *temp;
> +	int ret;
> +
> +	if (of_property_present(dev->of_node, "eq-presets-8gts")) {
> +		presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
> +		if (!presets->eq_presets_8gts)
> +			return -ENOMEM;
> +
> +		ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
> +						 presets->eq_presets_8gts, num_lanes);
> +		if (ret) {
> +			dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
> +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
> +		if (of_property_present(dev->of_node, name)) {
> +			temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
> +			if (!temp)
> +				return -ENOMEM;
> +
> +			ret = of_property_read_u8_array(dev->of_node, name,
> +							temp, num_lanes);
> +			if (ret) {
> +				dev_err(dev, "Error %s %d\n", name, ret);
> +				return ret;
> +			}
> +
> +			preset = (void **)((u8 *)presets + i * sizeof(void *));

Ugh.

> +			*preset = temp;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 14d00ce45bfa..82362d58bedc 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
>  }
>  
>  struct device_node;
> -
> +struct pci_eq_presets {
> +	void *eq_presets_8gts;
> +	void *eq_presets_16gts;
> +	void *eq_presets_32gts;
> +	void *eq_presets_64gts;

Why are all of those void*? 8gts is u16*, all other are u8*.

> +};

Empty lines before and after the struct definition.

>  #ifdef CONFIG_OF
>  int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
>  int of_get_pci_domain_nr(struct device_node *node);
> @@ -746,7 +751,9 @@ void pci_set_bus_of_node(struct pci_bus *bus);
>  void pci_release_bus_of_node(struct pci_bus *bus);
>  
>  int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge);
> -
> +int of_pci_get_equalization_presets(struct device *dev,
> +				    struct pci_eq_presets *presets,
> +				    int num_lanes);

Keep the empty line.

>  #else
>  static inline int
>  of_pci_parse_bus_range(struct device_node *node, struct resource *res)
> @@ -793,6 +800,12 @@ static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br
>  	return 0;
>  }
>  
> +static inline int of_pci_get_equalization_presets(struct device *dev,
> +						  struct pci_eq_presets *presets,
> +						  int num_lanes)
> +{
> +	return 0;
> +}
>  #endif /* CONFIG_OF */
>  
>  struct of_changeset;
> 
> -- 
> 2.34.1
>

Krishna Chaitanya Chundru Dec. 23, 2024, 2:32 p.m. UTC | #2

On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
> On Mon, Dec 23, 2024 at 12:21:15PM +0530, Krishna Chaitanya Chundru wrote:
>> PCIe equalization presets are predefined settings used to optimize
>> signal integrity by compensating for signal loss and distortion in
>> high-speed data transmission.
>>
>> As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
>> of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
>> configure lane equalization presets for each lane to enhance the PCIe
>> link reliability. Each preset value represents a different combination
>> of pre-shoot and de-emphasis values. For each data rate, different
>> registers are defined: for 8.0 GT/s, registers are defined in section
>> 7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
>> an extra receiver preset hint, requiring 16 bits per lane, while the
>> remaining data rates use 8 bits per lane.
>>
>> Based on the number of lanes and the supported data rate, this function
>> reads the device tree property and stores in the presets structure.
>>
>> Signed-off-by: Krishna Chaitanya Chundru <[e-mail]>
>> ---
>>   drivers/pci/of.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/pci/pci.h | 17 +++++++++++++++--
>>   2 files changed, 60 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>> index dacea3fc5128..99e0e7ae12e9 100644
>> --- a/drivers/pci/of.c
>> +++ b/drivers/pci/of.c
>> @@ -826,3 +826,48 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
>>   	return slot_power_limit_mw;
>>   }
>>   EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
>> +
> 
> kerneldoc? Define who should free the memory and how.
> 
I will update this in next series.
as we are allocating using devm_kzalloc it should be freed on driver
detach, as no special freeing is required.
>> +int of_pci_get_equalization_presets(struct device *dev,
>> +				    struct pci_eq_presets *presets,
>> +				    int num_lanes)
>> +{
>> +	char name[20];
>> +	void **preset;
>> +	void *temp;
>> +	int ret;
>> +
>> +	if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>> +		presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
>> +		if (!presets->eq_presets_8gts)
>> +			return -ENOMEM;
>> +
>> +		ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
>> +						 presets->eq_presets_8gts, num_lanes);
>> +		if (ret) {
>> +			dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
>> +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>> +		if (of_property_present(dev->of_node, name)) {
>> +			temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
>> +			if (!temp)
>> +				return -ENOMEM;
>> +
>> +			ret = of_property_read_u8_array(dev->of_node, name,
>> +							temp, num_lanes);
>> +			if (ret) {
>> +				dev_err(dev, "Error %s %d\n", name, ret);
>> +				return ret;
>> +			}
>> +
>> +			preset = (void **)((u8 *)presets + i * sizeof(void *));
> 
> Ugh.
> 
I was trying iterate over each element on the structure as presets holds 
the starting address of the structure and to that we are adding size of 
the void * point to go to each element. I did this way to reduce the
redundant code to read all the gts which has same way of storing the 
data from the device tree. I will add comments here in the next series.
>> +			*preset = temp;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 14d00ce45bfa..82362d58bedc 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
>>   }
>>   
>>   struct device_node;
>> -
>> +struct pci_eq_presets {
>> +	void *eq_presets_8gts;
>> +	void *eq_presets_16gts;
>> +	void *eq_presets_32gts;
>> +	void *eq_presets_64gts;
> 
> Why are all of those void*? 8gts is u16*, all other are u8*.
> 
To have common parsing logic I moved them to void*, as these are 
pointers actual memory is allocated by of_pci_get_equalization_presets()
based upon the gts these should not give any issues.
>> +};
> 
> Empty lines before and after the struct definition.
> 
ack.

- Krishna Chaitanya.
>>   #ifdef CONFIG_OF
>>   int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
>>   int of_get_pci_domain_nr(struct device_node *node);
>> @@ -746,7 +751,9 @@ void pci_set_bus_of_node(struct pci_bus *bus);
>>   void pci_release_bus_of_node(struct pci_bus *bus);
>>   
>>   int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge);
>> -
>> +int of_pci_get_equalization_presets(struct device *dev,
>> +				    struct pci_eq_presets *presets,
>> +				    int num_lanes);
> 
> Keep the empty line.
> 
>>   #else
>>   static inline int
>>   of_pci_parse_bus_range(struct device_node *node, struct resource *res)
>> @@ -793,6 +800,12 @@ static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br
>>   	return 0;
>>   }
>>   
>> +static inline int of_pci_get_equalization_presets(struct device *dev,
>> +						  struct pci_eq_presets *presets,
>> +						  int num_lanes)
>> +{
>> +	return 0;
>> +}
>>   #endif /* CONFIG_OF */
>>   
>>   struct of_changeset;
>>
>> -- 
>> 2.34.1
>>
>

Dmitry Baryshkov Dec. 23, 2024, 3:26 p.m. UTC | #3

On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
> 
> 
> On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
> > On Mon, Dec 23, 2024 at 12:21:15PM +0530, Krishna Chaitanya Chundru wrote:
> > > PCIe equalization presets are predefined settings used to optimize
> > > signal integrity by compensating for signal loss and distortion in
> > > high-speed data transmission.
> > > 
> > > As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
> > > of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
> > > configure lane equalization presets for each lane to enhance the PCIe
> > > link reliability. Each preset value represents a different combination
> > > of pre-shoot and de-emphasis values. For each data rate, different
> > > registers are defined: for 8.0 GT/s, registers are defined in section
> > > 7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
> > > an extra receiver preset hint, requiring 16 bits per lane, while the
> > > remaining data rates use 8 bits per lane.
> > > 
> > > Based on the number of lanes and the supported data rate, this function
> > > reads the device tree property and stores in the presets structure.
> > > 
> > > Signed-off-by: Krishna Chaitanya Chundru <[e-mail]>
> > > ---
> > >   drivers/pci/of.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > >   drivers/pci/pci.h | 17 +++++++++++++++--
> > >   2 files changed, 60 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > > index dacea3fc5128..99e0e7ae12e9 100644
> > > --- a/drivers/pci/of.c
> > > +++ b/drivers/pci/of.c
> > > @@ -826,3 +826,48 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
> > >   	return slot_power_limit_mw;
> > >   }
> > >   EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
> > > +
> > 
> > kerneldoc? Define who should free the memory and how.
> > 
> I will update this in next series.
> as we are allocating using devm_kzalloc it should be freed on driver
> detach, as no special freeing is required.
> > > +int of_pci_get_equalization_presets(struct device *dev,
> > > +				    struct pci_eq_presets *presets,
> > > +				    int num_lanes)
> > > +{
> > > +	char name[20];
> > > +	void **preset;
> > > +	void *temp;
> > > +	int ret;
> > > +
> > > +	if (of_property_present(dev->of_node, "eq-presets-8gts")) {
> > > +		presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
> > > +		if (!presets->eq_presets_8gts)
> > > +			return -ENOMEM;
> > > +
> > > +		ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
> > > +						 presets->eq_presets_8gts, num_lanes);
> > > +		if (ret) {
> > > +			dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > > +	for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
> > > +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
> > > +		if (of_property_present(dev->of_node, name)) {
> > > +			temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
> > > +			if (!temp)
> > > +				return -ENOMEM;
> > > +
> > > +			ret = of_property_read_u8_array(dev->of_node, name,
> > > +							temp, num_lanes);
> > > +			if (ret) {
> > > +				dev_err(dev, "Error %s %d\n", name, ret);
> > > +				return ret;
> > > +			}
> > > +
> > > +			preset = (void **)((u8 *)presets + i * sizeof(void *));
> > 
> > Ugh.
> > 
> I was trying iterate over each element on the structure as presets holds the
> starting address of the structure and to that we are adding size of the void
> * point to go to each element. I did this way to reduce the
> redundant code to read all the gts which has same way of storing the data
> from the device tree. I will add comments here in the next series.

Please rewrite this in a cleaner way. The code shouldn't raise
questions.

> > > +			*preset = temp;
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
> > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > index 14d00ce45bfa..82362d58bedc 100644
> > > --- a/drivers/pci/pci.h
> > > +++ b/drivers/pci/pci.h
> > > @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
> > >   }
> > >   struct device_node;
> > > -
> > > +struct pci_eq_presets {
> > > +	void *eq_presets_8gts;
> > > +	void *eq_presets_16gts;
> > > +	void *eq_presets_32gts;
> > > +	void *eq_presets_64gts;
> > 
> > Why are all of those void*? 8gts is u16*, all other are u8*.
> > 
> To have common parsing logic I moved them to void*, as these are pointers
> actual memory is allocated by of_pci_get_equalization_presets()
> based upon the gts these should not give any issues.

Please, don't. They have types. void pointers are for the opaque data.

> > > +};
> > 
> > Empty lines before and after the struct definition.
> > 
> ack.
> 
> - Krishna Chaitanya.
> > >   #ifdef CONFIG_OF
> > >   int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
> > >   int of_get_pci_domain_nr(struct device_node *node);
> > > @@ -746,7 +751,9 @@ void pci_set_bus_of_node(struct pci_bus *bus);
> > >   void pci_release_bus_of_node(struct pci_bus *bus);
> > >   int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge);
> > > -
> > > +int of_pci_get_equalization_presets(struct device *dev,
> > > +				    struct pci_eq_presets *presets,
> > > +				    int num_lanes);
> > 
> > Keep the empty line.
> > 
> > >   #else
> > >   static inline int
> > >   of_pci_parse_bus_range(struct device_node *node, struct resource *res)
> > > @@ -793,6 +800,12 @@ static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br
> > >   	return 0;
> > >   }
> > > +static inline int of_pci_get_equalization_presets(struct device *dev,
> > > +						  struct pci_eq_presets *presets,
> > > +						  int num_lanes)
> > > +{
> > > +	return 0;
> > > +}
> > >   #endif /* CONFIG_OF */
> > >   struct of_changeset;
> > > 
> > > -- 
> > > 2.34.1
> > > 
> >

Krishna Chaitanya Chundru Dec. 23, 2024, 4:43 p.m. UTC | #4

On 12/23/2024 8:56 PM, Dmitry Baryshkov wrote:
> On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
>>> On Mon, Dec 23, 2024 at 12:21:15PM +0530, Krishna Chaitanya Chundru wrote:
>>>> PCIe equalization presets are predefined settings used to optimize
>>>> signal integrity by compensating for signal loss and distortion in
>>>> high-speed data transmission.
>>>>
>>>> As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
>>>> of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
>>>> configure lane equalization presets for each lane to enhance the PCIe
>>>> link reliability. Each preset value represents a different combination
>>>> of pre-shoot and de-emphasis values. For each data rate, different
>>>> registers are defined: for 8.0 GT/s, registers are defined in section
>>>> 7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
>>>> an extra receiver preset hint, requiring 16 bits per lane, while the
>>>> remaining data rates use 8 bits per lane.
>>>>
>>>> Based on the number of lanes and the supported data rate, this function
>>>> reads the device tree property and stores in the presets structure.
>>>>
>>>> Signed-off-by: Krishna Chaitanya Chundru <[e-mail]>
>>>> ---
>>>>    drivers/pci/of.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>>    drivers/pci/pci.h | 17 +++++++++++++++--
>>>>    2 files changed, 60 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>>>> index dacea3fc5128..99e0e7ae12e9 100644
>>>> --- a/drivers/pci/of.c
>>>> +++ b/drivers/pci/of.c
>>>> @@ -826,3 +826,48 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
>>>>    	return slot_power_limit_mw;
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
>>>> +
>>>
>>> kerneldoc? Define who should free the memory and how.
>>>
>> I will update this in next series.
>> as we are allocating using devm_kzalloc it should be freed on driver
>> detach, as no special freeing is required.
>>>> +int of_pci_get_equalization_presets(struct device *dev,
>>>> +				    struct pci_eq_presets *presets,
>>>> +				    int num_lanes)
>>>> +{
>>>> +	char name[20];
>>>> +	void **preset;
>>>> +	void *temp;
>>>> +	int ret;
>>>> +
>>>> +	if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>>> +		presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
>>>> +		if (!presets->eq_presets_8gts)
>>>> +			return -ENOMEM;
>>>> +
>>>> +		ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
>>>> +						 presets->eq_presets_8gts, num_lanes);
>>>> +		if (ret) {
>>>> +			dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
>>>> +			return ret;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
>>>> +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>>> +		if (of_property_present(dev->of_node, name)) {
>>>> +			temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
>>>> +			if (!temp)
>>>> +				return -ENOMEM;
>>>> +
>>>> +			ret = of_property_read_u8_array(dev->of_node, name,
>>>> +							temp, num_lanes);
>>>> +			if (ret) {
>>>> +				dev_err(dev, "Error %s %d\n", name, ret);
>>>> +				return ret;
>>>> +			}
>>>> +
>>>> +			preset = (void **)((u8 *)presets + i * sizeof(void *));
>>>
>>> Ugh.
>>>
>> I was trying iterate over each element on the structure as presets holds the
>> starting address of the structure and to that we are adding size of the void
>> * point to go to each element. I did this way to reduce the
>> redundant code to read all the gts which has same way of storing the data
>> from the device tree. I will add comments here in the next series.
> 
> Please rewrite this in a cleaner way. The code shouldn't raise
> questions.
> 
>>>> +			*preset = temp;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>> index 14d00ce45bfa..82362d58bedc 100644
>>>> --- a/drivers/pci/pci.h
>>>> +++ b/drivers/pci/pci.h
>>>> @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
>>>>    }
>>>>    struct device_node;
>>>> -
>>>> +struct pci_eq_presets {
>>>> +	void *eq_presets_8gts;
>>>> +	void *eq_presets_16gts;
>>>> +	void *eq_presets_32gts;
>>>> +	void *eq_presets_64gts;
>>>
>>> Why are all of those void*? 8gts is u16*, all other are u8*.
>>>
>> To have common parsing logic I moved them to void*, as these are pointers
>> actual memory is allocated by of_pci_get_equalization_presets()
>> based upon the gts these should not give any issues.
> 
> Please, don't. They have types. void pointers are for the opaque data.
> 
ok.

I think then better to use v1 patch 
https://lore.kernel.org/all/[e-mail]/

konrad, any objection on using v1 as that will be cleaner way even if we
have some repetitive code.

- Krishna Chaitanya.

>>>> +};
>>>
>>> Empty lines before and after the struct definition.
>>>
>> ack.
>>
>> - Krishna Chaitanya.
>>>>    #ifdef CONFIG_OF
>>>>    int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
>>>>    int of_get_pci_domain_nr(struct device_node *node);
>>>> @@ -746,7 +751,9 @@ void pci_set_bus_of_node(struct pci_bus *bus);
>>>>    void pci_release_bus_of_node(struct pci_bus *bus);
>>>>    int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge);
>>>> -
>>>> +int of_pci_get_equalization_presets(struct device *dev,
>>>> +				    struct pci_eq_presets *presets,
>>>> +				    int num_lanes);
>>>
>>> Keep the empty line.
>>>
>>>>    #else
>>>>    static inline int
>>>>    of_pci_parse_bus_range(struct device_node *node, struct resource *res)
>>>> @@ -793,6 +800,12 @@ static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br
>>>>    	return 0;
>>>>    }
>>>> +static inline int of_pci_get_equalization_presets(struct device *dev,
>>>> +						  struct pci_eq_presets *presets,
>>>> +						  int num_lanes)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>>    #endif /* CONFIG_OF */
>>>>    struct of_changeset;
>>>>
>>>> -- 
>>>> 2.34.1
>>>>
>>>
>

Dmitry Baryshkov Dec. 23, 2024, 6:30 p.m. UTC | #5

On Mon, Dec 23, 2024 at 10:13:29PM +0530, Krishna Chaitanya Chundru wrote:
> 
> 
> On 12/23/2024 8:56 PM, Dmitry Baryshkov wrote:
> > On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
> > > 
> > > 
> > > On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
> > > > On Mon, Dec 23, 2024 at 12:21:15PM +0530, Krishna Chaitanya Chundru wrote:
> > > > > PCIe equalization presets are predefined settings used to optimize
> > > > > signal integrity by compensating for signal loss and distortion in
> > > > > high-speed data transmission.
> > > > > 
> > > > > As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
> > > > > of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
> > > > > configure lane equalization presets for each lane to enhance the PCIe
> > > > > link reliability. Each preset value represents a different combination
> > > > > of pre-shoot and de-emphasis values. For each data rate, different
> > > > > registers are defined: for 8.0 GT/s, registers are defined in section
> > > > > 7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
> > > > > an extra receiver preset hint, requiring 16 bits per lane, while the
> > > > > remaining data rates use 8 bits per lane.
> > > > > 
> > > > > Based on the number of lanes and the supported data rate, this function
> > > > > reads the device tree property and stores in the presets structure.
> > > > > 
> > > > > Signed-off-by: Krishna Chaitanya Chundru <[e-mail]>
> > > > > ---
> > > > >    drivers/pci/of.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > > > >    drivers/pci/pci.h | 17 +++++++++++++++--
> > > > >    2 files changed, 60 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > > > > index dacea3fc5128..99e0e7ae12e9 100644
> > > > > --- a/drivers/pci/of.c
> > > > > +++ b/drivers/pci/of.c
> > > > > @@ -826,3 +826,48 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
> > > > >    	return slot_power_limit_mw;
> > > > >    }
> > > > >    EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
> > > > > +
> > > > 
> > > > kerneldoc? Define who should free the memory and how.
> > > > 
> > > I will update this in next series.
> > > as we are allocating using devm_kzalloc it should be freed on driver
> > > detach, as no special freeing is required.
> > > > > +int of_pci_get_equalization_presets(struct device *dev,
> > > > > +				    struct pci_eq_presets *presets,
> > > > > +				    int num_lanes)
> > > > > +{
> > > > > +	char name[20];
> > > > > +	void **preset;
> > > > > +	void *temp;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (of_property_present(dev->of_node, "eq-presets-8gts")) {
> > > > > +		presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
> > > > > +		if (!presets->eq_presets_8gts)
> > > > > +			return -ENOMEM;
> > > > > +
> > > > > +		ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
> > > > > +						 presets->eq_presets_8gts, num_lanes);
> > > > > +		if (ret) {
> > > > > +			dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
> > > > > +			return ret;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
> > > > > +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
> > > > > +		if (of_property_present(dev->of_node, name)) {
> > > > > +			temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
> > > > > +			if (!temp)
> > > > > +				return -ENOMEM;
> > > > > +
> > > > > +			ret = of_property_read_u8_array(dev->of_node, name,
> > > > > +							temp, num_lanes);
> > > > > +			if (ret) {
> > > > > +				dev_err(dev, "Error %s %d\n", name, ret);
> > > > > +				return ret;
> > > > > +			}
> > > > > +
> > > > > +			preset = (void **)((u8 *)presets + i * sizeof(void *));
> > > > 
> > > > Ugh.
> > > > 
> > > I was trying iterate over each element on the structure as presets holds the
> > > starting address of the structure and to that we are adding size of the void
> > > * point to go to each element. I did this way to reduce the
> > > redundant code to read all the gts which has same way of storing the data
> > > from the device tree. I will add comments here in the next series.
> > 
> > Please rewrite this in a cleaner way. The code shouldn't raise
> > questions.
> > 
> > > > > +			*preset = temp;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
> > > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > > > index 14d00ce45bfa..82362d58bedc 100644
> > > > > --- a/drivers/pci/pci.h
> > > > > +++ b/drivers/pci/pci.h
> > > > > @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
> > > > >    }
> > > > >    struct device_node;
> > > > > -
> > > > > +struct pci_eq_presets {
> > > > > +	void *eq_presets_8gts;
> > > > > +	void *eq_presets_16gts;
> > > > > +	void *eq_presets_32gts;
> > > > > +	void *eq_presets_64gts;
> > > > 
> > > > Why are all of those void*? 8gts is u16*, all other are u8*.
> > > > 
> > > To have common parsing logic I moved them to void*, as these are pointers
> > > actual memory is allocated by of_pci_get_equalization_presets()
> > > based upon the gts these should not give any issues.
> > 
> > Please, don't. They have types. void pointers are for the opaque data.
> > 
> ok.
> 
> I think then better to use v1 patch
> https://lore.kernel.org/all/[e-mail]/
> 
> konrad, any objection on using v1 as that will be cleaner way even if we
> have some repetitive code.

Konrad had a nice suggestion about using the array of values. Please use
such an array for 16gts and above. This removes most of repetitive code.

Krishna Chaitanya Chundru Dec. 24, 2024, 9:17 a.m. UTC | #6

On 12/24/2024 12:00 AM, Dmitry Baryshkov wrote:
> On Mon, Dec 23, 2024 at 10:13:29PM +0530, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 12/23/2024 8:56 PM, Dmitry Baryshkov wrote:
>>> On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
>>>>
>>>>
>>>> On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
>>>>> On Mon, Dec 23, 2024 at 12:21:15PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>> PCIe equalization presets are predefined settings used to optimize
>>>>>> signal integrity by compensating for signal loss and distortion in
>>>>>> high-speed data transmission.
>>>>>>
>>>>>> As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
>>>>>> of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
>>>>>> configure lane equalization presets for each lane to enhance the PCIe
>>>>>> link reliability. Each preset value represents a different combination
>>>>>> of pre-shoot and de-emphasis values. For each data rate, different
>>>>>> registers are defined: for 8.0 GT/s, registers are defined in section
>>>>>> 7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
>>>>>> an extra receiver preset hint, requiring 16 bits per lane, while the
>>>>>> remaining data rates use 8 bits per lane.
>>>>>>
>>>>>> Based on the number of lanes and the supported data rate, this function
>>>>>> reads the device tree property and stores in the presets structure.
>>>>>>
>>>>>> Signed-off-by: Krishna Chaitanya Chundru <[e-mail]>
>>>>>> ---
>>>>>>     drivers/pci/of.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>>     drivers/pci/pci.h | 17 +++++++++++++++--
>>>>>>     2 files changed, 60 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>>>>>> index dacea3fc5128..99e0e7ae12e9 100644
>>>>>> --- a/drivers/pci/of.c
>>>>>> +++ b/drivers/pci/of.c
>>>>>> @@ -826,3 +826,48 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
>>>>>>     	return slot_power_limit_mw;
>>>>>>     }
>>>>>>     EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
>>>>>> +
>>>>>
>>>>> kerneldoc? Define who should free the memory and how.
>>>>>
>>>> I will update this in next series.
>>>> as we are allocating using devm_kzalloc it should be freed on driver
>>>> detach, as no special freeing is required.
>>>>>> +int of_pci_get_equalization_presets(struct device *dev,
>>>>>> +				    struct pci_eq_presets *presets,
>>>>>> +				    int num_lanes)
>>>>>> +{
>>>>>> +	char name[20];
>>>>>> +	void **preset;
>>>>>> +	void *temp;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>>>>> +		presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
>>>>>> +		if (!presets->eq_presets_8gts)
>>>>>> +			return -ENOMEM;
>>>>>> +
>>>>>> +		ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
>>>>>> +						 presets->eq_presets_8gts, num_lanes);
>>>>>> +		if (ret) {
>>>>>> +			dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
>>>>>> +			return ret;
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>> +	for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
>>>>>> +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>>>>> +		if (of_property_present(dev->of_node, name)) {
>>>>>> +			temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
>>>>>> +			if (!temp)
>>>>>> +				return -ENOMEM;
>>>>>> +
>>>>>> +			ret = of_property_read_u8_array(dev->of_node, name,
>>>>>> +							temp, num_lanes);
>>>>>> +			if (ret) {
>>>>>> +				dev_err(dev, "Error %s %d\n", name, ret);
>>>>>> +				return ret;
>>>>>> +			}
>>>>>> +
>>>>>> +			preset = (void **)((u8 *)presets + i * sizeof(void *));
>>>>>
>>>>> Ugh.
>>>>>
>>>> I was trying iterate over each element on the structure as presets holds the
>>>> starting address of the structure and to that we are adding size of the void
>>>> * point to go to each element. I did this way to reduce the
>>>> redundant code to read all the gts which has same way of storing the data
>>>> from the device tree. I will add comments here in the next series.
>>>
>>> Please rewrite this in a cleaner way. The code shouldn't raise
>>> questions.
>>>
>>>>>> +			*preset = temp;
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
>>>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>>>> index 14d00ce45bfa..82362d58bedc 100644
>>>>>> --- a/drivers/pci/pci.h
>>>>>> +++ b/drivers/pci/pci.h
>>>>>> @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
>>>>>>     }
>>>>>>     struct device_node;
>>>>>> -
>>>>>> +struct pci_eq_presets {
>>>>>> +	void *eq_presets_8gts;
>>>>>> +	void *eq_presets_16gts;
>>>>>> +	void *eq_presets_32gts;
>>>>>> +	void *eq_presets_64gts;
>>>>>
>>>>> Why are all of those void*? 8gts is u16*, all other are u8*.
>>>>>
>>>> To have common parsing logic I moved them to void*, as these are pointers
>>>> actual memory is allocated by of_pci_get_equalization_presets()
>>>> based upon the gts these should not give any issues.
>>>
>>> Please, don't. They have types. void pointers are for the opaque data.
>>>
>> ok.
>>
>> I think then better to use v1 patch
>> https://lore.kernel.org/all/[e-mail]/
>>
>> konrad, any objection on using v1 as that will be cleaner way even if we
>> have some repetitive code.
> 
> Konrad had a nice suggestion about using the array of values. Please use
> such an array for 16gts and above. This removes most of repetitive code.
> 
I don't feel having array in the preset structure looks good, I have
come up with this logic if you feel it is not so good I will go to the
suggested way by having array for 16gts and above.

        if (of_property_present(dev->of_node, "eq-presets-8gts")) {
                 presets->eq_presets_8gts = devm_kzalloc(dev, 
sizeof(u16) * num_lanes, GFP_KERNEL);
                 if (!presets->eq_presets_8gts)
                         return -ENOMEM;

                 ret = of_property_read_u16_array(dev->of_node, 
"eq-presets-8gts",
 
presets->eq_presets_8gts, num_lanes);
                 if (ret) {
                         dev_err(dev, "Error reading eq-presets-8gts 
%d\n", ret);
                         return ret;
                 }
         }

         for (int i = EQ_PRESET_TYPE_16GTS; i < EQ_PRESET_TYPE_64GTS; i++) {
                 snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
                 if (of_property_present(dev->of_node, name)) {
                         temp = devm_kzalloc(dev, sizeof(u8) * 
num_lanes, GFP_KERNEL);
                         if (!temp)
                                 return -ENOMEM;

                         ret = of_property_read_u8_array(dev->of_node, name,
                                                         temp, num_lanes);
                         if (ret) {
                                 dev_err(dev, "Error %s %d\n", name, ret);
                                 return ret;
                         }

                         switch (i) {
                                 case EQ_PRESET_TYPE_16GTS:
                                         presets->eq_presets_16gts = temp;
                                         break;
                                 case EQ_PRESET_TYPE_32GTS:
                                         presets->eq_presets_32gts = temp;
                                         break;
                                 case EQ_PRESET_TYPE_64GTS:
                                         presets->eq_presets_64gts = temp;
                                         break;
                         }
                 }
         }
- Krishna Chaitanya.

>

Dmitry Baryshkov Dec. 24, 2024, 9:55 a.m. UTC | #7

On Tue, Dec 24, 2024 at 02:47:00PM +0530, Krishna Chaitanya Chundru wrote:
> 
> 
> On 12/24/2024 12:00 AM, Dmitry Baryshkov wrote:
> > On Mon, Dec 23, 2024 at 10:13:29PM +0530, Krishna Chaitanya Chundru wrote:
> > > 
> > > 
> > > On 12/23/2024 8:56 PM, Dmitry Baryshkov wrote:
> > > > On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
> > > > > 
> > > > > 
> > > > > On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
> > > > > > On Mon, Dec 23, 2024 at 12:21:15PM +0530, Krishna Chaitanya Chundru wrote:
> > > > > > > PCIe equalization presets are predefined settings used to optimize
> > > > > > > signal integrity by compensating for signal loss and distortion in
> > > > > > > high-speed data transmission.
> > > > > > > 
> > > > > > > As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
> > > > > > > of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
> > > > > > > configure lane equalization presets for each lane to enhance the PCIe
> > > > > > > link reliability. Each preset value represents a different combination
> > > > > > > of pre-shoot and de-emphasis values. For each data rate, different
> > > > > > > registers are defined: for 8.0 GT/s, registers are defined in section
> > > > > > > 7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
> > > > > > > an extra receiver preset hint, requiring 16 bits per lane, while the
> > > > > > > remaining data rates use 8 bits per lane.
> > > > > > > 
> > > > > > > Based on the number of lanes and the supported data rate, this function
> > > > > > > reads the device tree property and stores in the presets structure.
> > > > > > > 
> > > > > > > Signed-off-by: Krishna Chaitanya Chundru <[e-mail]>
> > > > > > > ---
> > > > > > >     drivers/pci/of.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > > > > > >     drivers/pci/pci.h | 17 +++++++++++++++--
> > > > > > >     2 files changed, 60 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > > > > > > index dacea3fc5128..99e0e7ae12e9 100644
> > > > > > > --- a/drivers/pci/of.c
> > > > > > > +++ b/drivers/pci/of.c
> > > > > > > @@ -826,3 +826,48 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
> > > > > > >     	return slot_power_limit_mw;
> > > > > > >     }
> > > > > > >     EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
> > > > > > > +
> > > > > > 
> > > > > > kerneldoc? Define who should free the memory and how.
> > > > > > 
> > > > > I will update this in next series.
> > > > > as we are allocating using devm_kzalloc it should be freed on driver
> > > > > detach, as no special freeing is required.
> > > > > > > +int of_pci_get_equalization_presets(struct device *dev,
> > > > > > > +				    struct pci_eq_presets *presets,
> > > > > > > +				    int num_lanes)
> > > > > > > +{
> > > > > > > +	char name[20];
> > > > > > > +	void **preset;
> > > > > > > +	void *temp;
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	if (of_property_present(dev->of_node, "eq-presets-8gts")) {
> > > > > > > +		presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
> > > > > > > +		if (!presets->eq_presets_8gts)
> > > > > > > +			return -ENOMEM;
> > > > > > > +
> > > > > > > +		ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
> > > > > > > +						 presets->eq_presets_8gts, num_lanes);
> > > > > > > +		if (ret) {
> > > > > > > +			dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
> > > > > > > +			return ret;
> > > > > > > +		}
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
> > > > > > > +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
> > > > > > > +		if (of_property_present(dev->of_node, name)) {
> > > > > > > +			temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
> > > > > > > +			if (!temp)
> > > > > > > +				return -ENOMEM;
> > > > > > > +
> > > > > > > +			ret = of_property_read_u8_array(dev->of_node, name,
> > > > > > > +							temp, num_lanes);
> > > > > > > +			if (ret) {
> > > > > > > +				dev_err(dev, "Error %s %d\n", name, ret);
> > > > > > > +				return ret;
> > > > > > > +			}
> > > > > > > +
> > > > > > > +			preset = (void **)((u8 *)presets + i * sizeof(void *));
> > > > > > 
> > > > > > Ugh.
> > > > > > 
> > > > > I was trying iterate over each element on the structure as presets holds the
> > > > > starting address of the structure and to that we are adding size of the void
> > > > > * point to go to each element. I did this way to reduce the
> > > > > redundant code to read all the gts which has same way of storing the data
> > > > > from the device tree. I will add comments here in the next series.
> > > > 
> > > > Please rewrite this in a cleaner way. The code shouldn't raise
> > > > questions.
> > > > 
> > > > > > > +			*preset = temp;
> > > > > > > +		}
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
> > > > > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > > > > > index 14d00ce45bfa..82362d58bedc 100644
> > > > > > > --- a/drivers/pci/pci.h
> > > > > > > +++ b/drivers/pci/pci.h
> > > > > > > @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
> > > > > > >     }
> > > > > > >     struct device_node;
> > > > > > > -
> > > > > > > +struct pci_eq_presets {
> > > > > > > +	void *eq_presets_8gts;
> > > > > > > +	void *eq_presets_16gts;
> > > > > > > +	void *eq_presets_32gts;
> > > > > > > +	void *eq_presets_64gts;
> > > > > > 
> > > > > > Why are all of those void*? 8gts is u16*, all other are u8*.
> > > > > > 
> > > > > To have common parsing logic I moved them to void*, as these are pointers
> > > > > actual memory is allocated by of_pci_get_equalization_presets()
> > > > > based upon the gts these should not give any issues.
> > > > 
> > > > Please, don't. They have types. void pointers are for the opaque data.
> > > > 
> > > ok.
> > > 
> > > I think then better to use v1 patch
> > > https://lore.kernel.org/all/[e-mail]/
> > > 
> > > konrad, any objection on using v1 as that will be cleaner way even if we
> > > have some repetitive code.
> > 
> > Konrad had a nice suggestion about using the array of values. Please use
> > such an array for 16gts and above. This removes most of repetitive code.
> > 
> I don't feel having array in the preset structure looks good, I have
> come up with this logic if you feel it is not so good I will go to the
> suggested way by having array for 16gts and above.
> 
>        if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>                 presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) *
> num_lanes, GFP_KERNEL);
>                 if (!presets->eq_presets_8gts)
>                         return -ENOMEM;
> 
>                 ret = of_property_read_u16_array(dev->of_node,
> "eq-presets-8gts",
> 
> presets->eq_presets_8gts, num_lanes);
>                 if (ret) {
>                         dev_err(dev, "Error reading eq-presets-8gts %d\n",
> ret);
>                         return ret;
>                 }
>         }
> 
>         for (int i = EQ_PRESET_TYPE_16GTS; i < EQ_PRESET_TYPE_64GTS; i++) {
>                 snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>                 if (of_property_present(dev->of_node, name)) {
>                         temp = devm_kzalloc(dev, sizeof(u8) * num_lanes,
> GFP_KERNEL);
>                         if (!temp)
>                                 return -ENOMEM;
> 
>                         ret = of_property_read_u8_array(dev->of_node, name,
>                                                         temp, num_lanes);
>                         if (ret) {
>                                 dev_err(dev, "Error %s %d\n", name, ret);
>                                 return ret;
>                         }
> 
>                         switch (i) {
>                                 case EQ_PRESET_TYPE_16GTS:
>                                         presets->eq_presets_16gts = temp;
>                                         break;
>                                 case EQ_PRESET_TYPE_32GTS:
>                                         presets->eq_presets_32gts = temp;
>                                         break;
>                                 case EQ_PRESET_TYPE_64GTS:
>                                         presets->eq_presets_64gts = temp;
>                                         break;
>                         }

This looks like 'presets->eq_presets[i] = temp;', but I won't insist on
that.

Also, a strange thought came to my mind: we know that there won't be
more than 16 lanes. Can we have the following structure instead:

#define MAX_LANES 16
enum pcie_gts {
	PCIE_GTS_16GTS,
	PCIE_GTS_32GTS,
	PCIE_GTS_64GTS,
	PCIE_GTS_MAX,
};
struct pci_eq_presets {
	u16 eq_presets_8gts[MAX_LANES];
	u8 eq_presets_Ngts[PCIE_GTS_MAX][MAX_LANES];
};

This should allow you to drop the of_property_present() and
devm_kzalloc(). Just read DT data into a corresponding array.

>                 }
>         }
> - Krishna Chaitanya.
> 
> >

Krishna Chaitanya Chundru Dec. 24, 2024, 10:35 a.m. UTC | #8

On 12/24/2024 3:25 PM, Dmitry Baryshkov wrote:
> On Tue, Dec 24, 2024 at 02:47:00PM +0530, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 12/24/2024 12:00 AM, Dmitry Baryshkov wrote:
>>> On Mon, Dec 23, 2024 at 10:13:29PM +0530, Krishna Chaitanya Chundru wrote:
>>>>
>>>>
>>>> On 12/23/2024 8:56 PM, Dmitry Baryshkov wrote:
>>>>> On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>
>>>>>>
>>>>>> On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
>>>>>>> On Mon, Dec 23, 2024 at 12:21:15PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>>> PCIe equalization presets are predefined settings used to optimize
>>>>>>>> signal integrity by compensating for signal loss and distortion in
>>>>>>>> high-speed data transmission.
>>>>>>>>
>>>>>>>> As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
>>>>>>>> of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
>>>>>>>> configure lane equalization presets for each lane to enhance the PCIe
>>>>>>>> link reliability. Each preset value represents a different combination
>>>>>>>> of pre-shoot and de-emphasis values. For each data rate, different
>>>>>>>> registers are defined: for 8.0 GT/s, registers are defined in section
>>>>>>>> 7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
>>>>>>>> an extra receiver preset hint, requiring 16 bits per lane, while the
>>>>>>>> remaining data rates use 8 bits per lane.
>>>>>>>>
>>>>>>>> Based on the number of lanes and the supported data rate, this function
>>>>>>>> reads the device tree property and stores in the presets structure.
>>>>>>>>
>>>>>>>> Signed-off-by: Krishna Chaitanya Chundru <[e-mail]>
>>>>>>>> ---
>>>>>>>>      drivers/pci/of.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>      drivers/pci/pci.h | 17 +++++++++++++++--
>>>>>>>>      2 files changed, 60 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>>>>>>>> index dacea3fc5128..99e0e7ae12e9 100644
>>>>>>>> --- a/drivers/pci/of.c
>>>>>>>> +++ b/drivers/pci/of.c
>>>>>>>> @@ -826,3 +826,48 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
>>>>>>>>      	return slot_power_limit_mw;
>>>>>>>>      }
>>>>>>>>      EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
>>>>>>>> +
>>>>>>>
>>>>>>> kerneldoc? Define who should free the memory and how.
>>>>>>>
>>>>>> I will update this in next series.
>>>>>> as we are allocating using devm_kzalloc it should be freed on driver
>>>>>> detach, as no special freeing is required.
>>>>>>>> +int of_pci_get_equalization_presets(struct device *dev,
>>>>>>>> +				    struct pci_eq_presets *presets,
>>>>>>>> +				    int num_lanes)
>>>>>>>> +{
>>>>>>>> +	char name[20];
>>>>>>>> +	void **preset;
>>>>>>>> +	void *temp;
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>>>>>>> +		presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
>>>>>>>> +		if (!presets->eq_presets_8gts)
>>>>>>>> +			return -ENOMEM;
>>>>>>>> +
>>>>>>>> +		ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
>>>>>>>> +						 presets->eq_presets_8gts, num_lanes);
>>>>>>>> +		if (ret) {
>>>>>>>> +			dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
>>>>>>>> +			return ret;
>>>>>>>> +		}
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
>>>>>>>> +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>>>>>>> +		if (of_property_present(dev->of_node, name)) {
>>>>>>>> +			temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
>>>>>>>> +			if (!temp)
>>>>>>>> +				return -ENOMEM;
>>>>>>>> +
>>>>>>>> +			ret = of_property_read_u8_array(dev->of_node, name,
>>>>>>>> +							temp, num_lanes);
>>>>>>>> +			if (ret) {
>>>>>>>> +				dev_err(dev, "Error %s %d\n", name, ret);
>>>>>>>> +				return ret;
>>>>>>>> +			}
>>>>>>>> +
>>>>>>>> +			preset = (void **)((u8 *)presets + i * sizeof(void *));
>>>>>>>
>>>>>>> Ugh.
>>>>>>>
>>>>>> I was trying iterate over each element on the structure as presets holds the
>>>>>> starting address of the structure and to that we are adding size of the void
>>>>>> * point to go to each element. I did this way to reduce the
>>>>>> redundant code to read all the gts which has same way of storing the data
>>>>>> from the device tree. I will add comments here in the next series.
>>>>>
>>>>> Please rewrite this in a cleaner way. The code shouldn't raise
>>>>> questions.
>>>>>
>>>>>>>> +			*preset = temp;
>>>>>>>> +		}
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	return 0;
>>>>>>>> +}
>>>>>>>> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
>>>>>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>>>>>> index 14d00ce45bfa..82362d58bedc 100644
>>>>>>>> --- a/drivers/pci/pci.h
>>>>>>>> +++ b/drivers/pci/pci.h
>>>>>>>> @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
>>>>>>>>      }
>>>>>>>>      struct device_node;
>>>>>>>> -
>>>>>>>> +struct pci_eq_presets {
>>>>>>>> +	void *eq_presets_8gts;
>>>>>>>> +	void *eq_presets_16gts;
>>>>>>>> +	void *eq_presets_32gts;
>>>>>>>> +	void *eq_presets_64gts;
>>>>>>>
>>>>>>> Why are all of those void*? 8gts is u16*, all other are u8*.
>>>>>>>
>>>>>> To have common parsing logic I moved them to void*, as these are pointers
>>>>>> actual memory is allocated by of_pci_get_equalization_presets()
>>>>>> based upon the gts these should not give any issues.
>>>>>
>>>>> Please, don't. They have types. void pointers are for the opaque data.
>>>>>
>>>> ok.
>>>>
>>>> I think then better to use v1 patch
>>>> https://lore.kernel.org/all/[e-mail]/
>>>>
>>>> konrad, any objection on using v1 as that will be cleaner way even if we
>>>> have some repetitive code.
>>>
>>> Konrad had a nice suggestion about using the array of values. Please use
>>> such an array for 16gts and above. This removes most of repetitive code.
>>>
>> I don't feel having array in the preset structure looks good, I have
>> come up with this logic if you feel it is not so good I will go to the
>> suggested way by having array for 16gts and above.
>>
>>         if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>                  presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) *
>> num_lanes, GFP_KERNEL);
>>                  if (!presets->eq_presets_8gts)
>>                          return -ENOMEM;
>>
>>                  ret = of_property_read_u16_array(dev->of_node,
>> "eq-presets-8gts",
>>
>> presets->eq_presets_8gts, num_lanes);
>>                  if (ret) {
>>                          dev_err(dev, "Error reading eq-presets-8gts %d\n",
>> ret);
>>                          return ret;
>>                  }
>>          }
>>
>>          for (int i = EQ_PRESET_TYPE_16GTS; i < EQ_PRESET_TYPE_64GTS; i++) {
>>                  snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>                  if (of_property_present(dev->of_node, name)) {
>>                          temp = devm_kzalloc(dev, sizeof(u8) * num_lanes,
>> GFP_KERNEL);
>>                          if (!temp)
>>                                  return -ENOMEM;
>>
>>                          ret = of_property_read_u8_array(dev->of_node, name,
>>                                                          temp, num_lanes);
>>                          if (ret) {
>>                                  dev_err(dev, "Error %s %d\n", name, ret);
>>                                  return ret;
>>                          }
>>
>>                          switch (i) {
>>                                  case EQ_PRESET_TYPE_16GTS:
>>                                          presets->eq_presets_16gts = temp;
>>                                          break;
>>                                  case EQ_PRESET_TYPE_32GTS:
>>                                          presets->eq_presets_32gts = temp;
>>                                          break;
>>                                  case EQ_PRESET_TYPE_64GTS:
>>                                          presets->eq_presets_64gts = temp;
>>                                          break;
>>                          }
> 
> This looks like 'presets->eq_presets[i] = temp;', but I won't insist on
> that.
> 
> Also, a strange thought came to my mind: we know that there won't be
> more than 16 lanes. Can we have the following structure instead:
> 
> #define MAX_LANES 16
> enum pcie_gts {
> 	PCIE_GTS_16GTS,
> 	PCIE_GTS_32GTS,
> 	PCIE_GTS_64GTS,
> 	PCIE_GTS_MAX,
> };
> struct pci_eq_presets {
> 	u16 eq_presets_8gts[MAX_LANES];
> 	u8 eq_presets_Ngts[PCIE_GTS_MAX][MAX_LANES];
> };
> 
> This should allow you to drop the of_property_present() and
> devm_kzalloc(). Just read DT data into a corresponding array.
> 
in the dwc driver patch I was using pointers and memory allocation
to known if the property is present or not. If I use this way I might
end up reading dt property again. I think better to switch to have a
array for above 16gts.

- Krishna Chaitanya.
>>                  }
>>          }
>> - Krishna Chaitanya.
>>
>>>
>

Dmitry Baryshkov Dec. 24, 2024, 10:57 a.m. UTC | #9

On Tue, 24 Dec 2024 at 12:36, Krishna Chaitanya Chundru
<[e-mail]> wrote:
>
>
>
> On 12/24/2024 3:25 PM, Dmitry Baryshkov wrote:
> > On Tue, Dec 24, 2024 at 02:47:00PM +0530, Krishna Chaitanya Chundru wrote:
> >>
> >>
> >> On 12/24/2024 12:00 AM, Dmitry Baryshkov wrote:
> >>> On Mon, Dec 23, 2024 at 10:13:29PM +0530, Krishna Chaitanya Chundru wrote:
> >>>>
> >>>>
> >>>> On 12/23/2024 8:56 PM, Dmitry Baryshkov wrote:
> >>>>> On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
> >>>>>>> On Mon, Dec 23, 2024 at 12:21:15PM +0530, Krishna Chaitanya Chundru wrote:
> >>>>>>>> PCIe equalization presets are predefined settings used to optimize
> >>>>>>>> signal integrity by compensating for signal loss and distortion in
> >>>>>>>> high-speed data transmission.
> >>>>>>>>
> >>>>>>>> As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
> >>>>>>>> of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
> >>>>>>>> configure lane equalization presets for each lane to enhance the PCIe
> >>>>>>>> link reliability. Each preset value represents a different combination
> >>>>>>>> of pre-shoot and de-emphasis values. For each data rate, different
> >>>>>>>> registers are defined: for 8.0 GT/s, registers are defined in section
> >>>>>>>> 7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
> >>>>>>>> an extra receiver preset hint, requiring 16 bits per lane, while the
> >>>>>>>> remaining data rates use 8 bits per lane.
> >>>>>>>>
> >>>>>>>> Based on the number of lanes and the supported data rate, this function
> >>>>>>>> reads the device tree property and stores in the presets structure.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Krishna Chaitanya Chundru <[e-mail]>
> >>>>>>>> ---
> >>>>>>>>      drivers/pci/of.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>>      drivers/pci/pci.h | 17 +++++++++++++++--
> >>>>>>>>      2 files changed, 60 insertions(+), 2 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> >>>>>>>> index dacea3fc5128..99e0e7ae12e9 100644
> >>>>>>>> --- a/drivers/pci/of.c
> >>>>>>>> +++ b/drivers/pci/of.c
> >>>>>>>> @@ -826,3 +826,48 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
> >>>>>>>>        return slot_power_limit_mw;
> >>>>>>>>      }
> >>>>>>>>      EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
> >>>>>>>> +
> >>>>>>>
> >>>>>>> kerneldoc? Define who should free the memory and how.
> >>>>>>>
> >>>>>> I will update this in next series.
> >>>>>> as we are allocating using devm_kzalloc it should be freed on driver
> >>>>>> detach, as no special freeing is required.
> >>>>>>>> +int of_pci_get_equalization_presets(struct device *dev,
> >>>>>>>> +                                  struct pci_eq_presets *presets,
> >>>>>>>> +                                  int num_lanes)
> >>>>>>>> +{
> >>>>>>>> +      char name[20];
> >>>>>>>> +      void **preset;
> >>>>>>>> +      void *temp;
> >>>>>>>> +      int ret;
> >>>>>>>> +
> >>>>>>>> +      if (of_property_present(dev->of_node, "eq-presets-8gts")) {
> >>>>>>>> +              presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
> >>>>>>>> +              if (!presets->eq_presets_8gts)
> >>>>>>>> +                      return -ENOMEM;
> >>>>>>>> +
> >>>>>>>> +              ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
> >>>>>>>> +                                               presets->eq_presets_8gts, num_lanes);
> >>>>>>>> +              if (ret) {
> >>>>>>>> +                      dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
> >>>>>>>> +                      return ret;
> >>>>>>>> +              }
> >>>>>>>> +      }
> >>>>>>>> +
> >>>>>>>> +      for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
> >>>>>>>> +              snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
> >>>>>>>> +              if (of_property_present(dev->of_node, name)) {
> >>>>>>>> +                      temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
> >>>>>>>> +                      if (!temp)
> >>>>>>>> +                              return -ENOMEM;
> >>>>>>>> +
> >>>>>>>> +                      ret = of_property_read_u8_array(dev->of_node, name,
> >>>>>>>> +                                                      temp, num_lanes);
> >>>>>>>> +                      if (ret) {
> >>>>>>>> +                              dev_err(dev, "Error %s %d\n", name, ret);
> >>>>>>>> +                              return ret;
> >>>>>>>> +                      }
> >>>>>>>> +
> >>>>>>>> +                      preset = (void **)((u8 *)presets + i * sizeof(void *));
> >>>>>>>
> >>>>>>> Ugh.
> >>>>>>>
> >>>>>> I was trying iterate over each element on the structure as presets holds the
> >>>>>> starting address of the structure and to that we are adding size of the void
> >>>>>> * point to go to each element. I did this way to reduce the
> >>>>>> redundant code to read all the gts which has same way of storing the data
> >>>>>> from the device tree. I will add comments here in the next series.
> >>>>>
> >>>>> Please rewrite this in a cleaner way. The code shouldn't raise
> >>>>> questions.
> >>>>>
> >>>>>>>> +                      *preset = temp;
> >>>>>>>> +              }
> >>>>>>>> +      }
> >>>>>>>> +
> >>>>>>>> +      return 0;
> >>>>>>>> +}
> >>>>>>>> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
> >>>>>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> >>>>>>>> index 14d00ce45bfa..82362d58bedc 100644
> >>>>>>>> --- a/drivers/pci/pci.h
> >>>>>>>> +++ b/drivers/pci/pci.h
> >>>>>>>> @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
> >>>>>>>>      }
> >>>>>>>>      struct device_node;
> >>>>>>>> -
> >>>>>>>> +struct pci_eq_presets {
> >>>>>>>> +      void *eq_presets_8gts;
> >>>>>>>> +      void *eq_presets_16gts;
> >>>>>>>> +      void *eq_presets_32gts;
> >>>>>>>> +      void *eq_presets_64gts;
> >>>>>>>
> >>>>>>> Why are all of those void*? 8gts is u16*, all other are u8*.
> >>>>>>>
> >>>>>> To have common parsing logic I moved them to void*, as these are pointers
> >>>>>> actual memory is allocated by of_pci_get_equalization_presets()
> >>>>>> based upon the gts these should not give any issues.
> >>>>>
> >>>>> Please, don't. They have types. void pointers are for the opaque data.
> >>>>>
> >>>> ok.
> >>>>
> >>>> I think then better to use v1 patch
> >>>> https://lore.kernel.org/all/[e-mail]/
> >>>>
> >>>> konrad, any objection on using v1 as that will be cleaner way even if we
> >>>> have some repetitive code.
> >>>
> >>> Konrad had a nice suggestion about using the array of values. Please use
> >>> such an array for 16gts and above. This removes most of repetitive code.
> >>>
> >> I don't feel having array in the preset structure looks good, I have
> >> come up with this logic if you feel it is not so good I will go to the
> >> suggested way by having array for 16gts and above.
> >>
> >>         if (of_property_present(dev->of_node, "eq-presets-8gts")) {
> >>                  presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) *
> >> num_lanes, GFP_KERNEL);
> >>                  if (!presets->eq_presets_8gts)
> >>                          return -ENOMEM;
> >>
> >>                  ret = of_property_read_u16_array(dev->of_node,
> >> "eq-presets-8gts",
> >>
> >> presets->eq_presets_8gts, num_lanes);
> >>                  if (ret) {
> >>                          dev_err(dev, "Error reading eq-presets-8gts %d\n",
> >> ret);
> >>                          return ret;
> >>                  }
> >>          }
> >>
> >>          for (int i = EQ_PRESET_TYPE_16GTS; i < EQ_PRESET_TYPE_64GTS; i++) {
> >>                  snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
> >>                  if (of_property_present(dev->of_node, name)) {
> >>                          temp = devm_kzalloc(dev, sizeof(u8) * num_lanes,
> >> GFP_KERNEL);
> >>                          if (!temp)
> >>                                  return -ENOMEM;
> >>
> >>                          ret = of_property_read_u8_array(dev->of_node, name,
> >>                                                          temp, num_lanes);
> >>                          if (ret) {
> >>                                  dev_err(dev, "Error %s %d\n", name, ret);
> >>                                  return ret;
> >>                          }
> >>
> >>                          switch (i) {
> >>                                  case EQ_PRESET_TYPE_16GTS:
> >>                                          presets->eq_presets_16gts = temp;
> >>                                          break;
> >>                                  case EQ_PRESET_TYPE_32GTS:
> >>                                          presets->eq_presets_32gts = temp;
> >>                                          break;
> >>                                  case EQ_PRESET_TYPE_64GTS:
> >>                                          presets->eq_presets_64gts = temp;
> >>                                          break;
> >>                          }
> >
> > This looks like 'presets->eq_presets[i] = temp;', but I won't insist on
> > that.
> >
> > Also, a strange thought came to my mind: we know that there won't be
> > more than 16 lanes. Can we have the following structure instead:
> >
> > #define MAX_LANES 16
> > enum pcie_gts {
> >       PCIE_GTS_16GTS,
> >       PCIE_GTS_32GTS,
> >       PCIE_GTS_64GTS,
> >       PCIE_GTS_MAX,
> > };
> > struct pci_eq_presets {
> >       u16 eq_presets_8gts[MAX_LANES];
> >       u8 eq_presets_Ngts[PCIE_GTS_MAX][MAX_LANES];
> > };
> >
> > This should allow you to drop the of_property_present() and
> > devm_kzalloc(). Just read DT data into a corresponding array.
> >
> in the dwc driver patch I was using pointers and memory allocation
> to known if the property is present or not. If I use this way I might
> end up reading dt property again.

Add foo_valid flags to the struct.

>  I think better to switch to have a
> array for above 16gts.

Whichever way works for you.

>
> - Krishna Chaitanya.
> >>                  }
> >>          }
> >> - Krishna Chaitanya.
> >>
> >>>
> >

Konrad Dybcio Dec. 30, 2024, 1:41 p.m. UTC | #10

On 24.12.2024 11:57 AM, Dmitry Baryshkov wrote:
> On Tue, 24 Dec 2024 at 12:36, Krishna Chaitanya Chundru
> <[e-mail]> wrote:
>>
>>
>>
>> On 12/24/2024 3:25 PM, Dmitry Baryshkov wrote:
>>> On Tue, Dec 24, 2024 at 02:47:00PM +0530, Krishna Chaitanya Chundru wrote:
>>>>
>>>>
>>>> On 12/24/2024 12:00 AM, Dmitry Baryshkov wrote:
>>>>> On Mon, Dec 23, 2024 at 10:13:29PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>
>>>>>>
>>>>>> On 12/23/2024 8:56 PM, Dmitry Baryshkov wrote:
>>>>>>> On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On Mon, Dec 23, 2024 at 12:21:15PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>>>>> PCIe equalization presets are predefined settings used to optimize
>>>>>>>>>> signal integrity by compensating for signal loss and distortion in
>>>>>>>>>> high-speed data transmission.
>>>>>>>>>>
>>>>>>>>>> As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
>>>>>>>>>> of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
>>>>>>>>>> configure lane equalization presets for each lane to enhance the PCIe
>>>>>>>>>> link reliability. Each preset value represents a different combination
>>>>>>>>>> of pre-shoot and de-emphasis values. For each data rate, different
>>>>>>>>>> registers are defined: for 8.0 GT/s, registers are defined in section
>>>>>>>>>> 7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
>>>>>>>>>> an extra receiver preset hint, requiring 16 bits per lane, while the
>>>>>>>>>> remaining data rates use 8 bits per lane.
>>>>>>>>>>
>>>>>>>>>> Based on the number of lanes and the supported data rate, this function
>>>>>>>>>> reads the device tree property and stores in the presets structure.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Krishna Chaitanya Chundru <[e-mail]>
>>>>>>>>>> ---
>>>>>>>>>>      drivers/pci/of.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>      drivers/pci/pci.h | 17 +++++++++++++++--
>>>>>>>>>>      2 files changed, 60 insertions(+), 2 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>>>>>>>>>> index dacea3fc5128..99e0e7ae12e9 100644
>>>>>>>>>> --- a/drivers/pci/of.c
>>>>>>>>>> +++ b/drivers/pci/of.c
>>>>>>>>>> @@ -826,3 +826,48 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
>>>>>>>>>>        return slot_power_limit_mw;
>>>>>>>>>>      }
>>>>>>>>>>      EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> kerneldoc? Define who should free the memory and how.
>>>>>>>>>
>>>>>>>> I will update this in next series.
>>>>>>>> as we are allocating using devm_kzalloc it should be freed on driver
>>>>>>>> detach, as no special freeing is required.
>>>>>>>>>> +int of_pci_get_equalization_presets(struct device *dev,
>>>>>>>>>> +                                  struct pci_eq_presets *presets,
>>>>>>>>>> +                                  int num_lanes)
>>>>>>>>>> +{
>>>>>>>>>> +      char name[20];
>>>>>>>>>> +      void **preset;
>>>>>>>>>> +      void *temp;
>>>>>>>>>> +      int ret;
>>>>>>>>>> +
>>>>>>>>>> +      if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>>>>>>>>> +              presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
>>>>>>>>>> +              if (!presets->eq_presets_8gts)
>>>>>>>>>> +                      return -ENOMEM;
>>>>>>>>>> +
>>>>>>>>>> +              ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
>>>>>>>>>> +                                               presets->eq_presets_8gts, num_lanes);
>>>>>>>>>> +              if (ret) {
>>>>>>>>>> +                      dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
>>>>>>>>>> +                      return ret;
>>>>>>>>>> +              }
>>>>>>>>>> +      }
>>>>>>>>>> +
>>>>>>>>>> +      for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
>>>>>>>>>> +              snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>>>>>>>>> +              if (of_property_present(dev->of_node, name)) {
>>>>>>>>>> +                      temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
>>>>>>>>>> +                      if (!temp)
>>>>>>>>>> +                              return -ENOMEM;
>>>>>>>>>> +
>>>>>>>>>> +                      ret = of_property_read_u8_array(dev->of_node, name,
>>>>>>>>>> +                                                      temp, num_lanes);
>>>>>>>>>> +                      if (ret) {
>>>>>>>>>> +                              dev_err(dev, "Error %s %d\n", name, ret);
>>>>>>>>>> +                              return ret;
>>>>>>>>>> +                      }
>>>>>>>>>> +
>>>>>>>>>> +                      preset = (void **)((u8 *)presets + i * sizeof(void *));
>>>>>>>>>
>>>>>>>>> Ugh.
>>>>>>>>>
>>>>>>>> I was trying iterate over each element on the structure as presets holds the
>>>>>>>> starting address of the structure and to that we are adding size of the void
>>>>>>>> * point to go to each element. I did this way to reduce the
>>>>>>>> redundant code to read all the gts which has same way of storing the data
>>>>>>>> from the device tree. I will add comments here in the next series.
>>>>>>>
>>>>>>> Please rewrite this in a cleaner way. The code shouldn't raise
>>>>>>> questions.
>>>>>>>
>>>>>>>>>> +                      *preset = temp;
>>>>>>>>>> +              }
>>>>>>>>>> +      }
>>>>>>>>>> +
>>>>>>>>>> +      return 0;
>>>>>>>>>> +}
>>>>>>>>>> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
>>>>>>>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>>>>>>>> index 14d00ce45bfa..82362d58bedc 100644
>>>>>>>>>> --- a/drivers/pci/pci.h
>>>>>>>>>> +++ b/drivers/pci/pci.h
>>>>>>>>>> @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
>>>>>>>>>>      }
>>>>>>>>>>      struct device_node;
>>>>>>>>>> -
>>>>>>>>>> +struct pci_eq_presets {
>>>>>>>>>> +      void *eq_presets_8gts;
>>>>>>>>>> +      void *eq_presets_16gts;
>>>>>>>>>> +      void *eq_presets_32gts;
>>>>>>>>>> +      void *eq_presets_64gts;
>>>>>>>>>
>>>>>>>>> Why are all of those void*? 8gts is u16*, all other are u8*.
>>>>>>>>>
>>>>>>>> To have common parsing logic I moved them to void*, as these are pointers
>>>>>>>> actual memory is allocated by of_pci_get_equalization_presets()
>>>>>>>> based upon the gts these should not give any issues.
>>>>>>>
>>>>>>> Please, don't. They have types. void pointers are for the opaque data.
>>>>>>>
>>>>>> ok.
>>>>>>
>>>>>> I think then better to use v1 patch
>>>>>> https://lore.kernel.org/all/[e-mail]/
>>>>>>
>>>>>> konrad, any objection on using v1 as that will be cleaner way even if we
>>>>>> have some repetitive code.
>>>>>
>>>>> Konrad had a nice suggestion about using the array of values. Please use
>>>>> such an array for 16gts and above. This removes most of repetitive code.
>>>>>
>>>> I don't feel having array in the preset structure looks good, I have
>>>> come up with this logic if you feel it is not so good I will go to the
>>>> suggested way by having array for 16gts and above.
>>>>
>>>>         if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>>>                  presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) *
>>>> num_lanes, GFP_KERNEL);
>>>>                  if (!presets->eq_presets_8gts)
>>>>                          return -ENOMEM;
>>>>
>>>>                  ret = of_property_read_u16_array(dev->of_node,
>>>> "eq-presets-8gts",
>>>>
>>>> presets->eq_presets_8gts, num_lanes);
>>>>                  if (ret) {
>>>>                          dev_err(dev, "Error reading eq-presets-8gts %d\n",
>>>> ret);
>>>>                          return ret;
>>>>                  }
>>>>          }
>>>>
>>>>          for (int i = EQ_PRESET_TYPE_16GTS; i < EQ_PRESET_TYPE_64GTS; i++) {
>>>>                  snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>>>                  if (of_property_present(dev->of_node, name)) {
>>>>                          temp = devm_kzalloc(dev, sizeof(u8) * num_lanes,
>>>> GFP_KERNEL);
>>>>                          if (!temp)
>>>>                                  return -ENOMEM;
>>>>
>>>>                          ret = of_property_read_u8_array(dev->of_node, name,
>>>>                                                          temp, num_lanes);
>>>>                          if (ret) {
>>>>                                  dev_err(dev, "Error %s %d\n", name, ret);
>>>>                                  return ret;
>>>>                          }
>>>>
>>>>                          switch (i) {
>>>>                                  case EQ_PRESET_TYPE_16GTS:
>>>>                                          presets->eq_presets_16gts = temp;
>>>>                                          break;
>>>>                                  case EQ_PRESET_TYPE_32GTS:
>>>>                                          presets->eq_presets_32gts = temp;
>>>>                                          break;
>>>>                                  case EQ_PRESET_TYPE_64GTS:
>>>>                                          presets->eq_presets_64gts = temp;
>>>>                                          break;
>>>>                          }
>>>
>>> This looks like 'presets->eq_presets[i] = temp;', but I won't insist on
>>> that.
>>>
>>> Also, a strange thought came to my mind: we know that there won't be
>>> more than 16 lanes. Can we have the following structure instead:
>>>
>>> #define MAX_LANES 16
>>> enum pcie_gts {
>>>       PCIE_GTS_16GTS,
>>>       PCIE_GTS_32GTS,
>>>       PCIE_GTS_64GTS,
>>>       PCIE_GTS_MAX,
>>> };
>>> struct pci_eq_presets {
>>>       u16 eq_presets_8gts[MAX_LANES];
>>>       u8 eq_presets_Ngts[PCIE_GTS_MAX][MAX_LANES];
>>> };
>>>
>>> This should allow you to drop the of_property_present() and
>>> devm_kzalloc(). Just read DT data into a corresponding array.
>>>
>> in the dwc driver patch I was using pointers and memory allocation
>> to known if the property is present or not. If I use this way I might
>> end up reading dt property again.
> 
> Add foo_valid flags to the struct.

Some(u8)/None would be fitting, but we're not there yet :(

Are all 0x00-0xff(ff) values valid for these presets?

>>  I think better to switch to have a
>> array for above 16gts.
> 
> Whichever way works for you.

Sorta-answering the earlier email, I have no concerns either

Konrad

Krishna Chaitanya Chundru Dec. 31, 2024, 4:38 a.m. UTC | #11

On 12/30/2024 7:11 PM, Konrad Dybcio wrote:
> On 24.12.2024 11:57 AM, Dmitry Baryshkov wrote:
>> On Tue, 24 Dec 2024 at 12:36, Krishna Chaitanya Chundru
>> <[e-mail]> wrote:
>>>
>>>
>>>
>>> On 12/24/2024 3:25 PM, Dmitry Baryshkov wrote:
>>>> On Tue, Dec 24, 2024 at 02:47:00PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>
>>>>>
>>>>> On 12/24/2024 12:00 AM, Dmitry Baryshkov wrote:
>>>>>> On Mon, Dec 23, 2024 at 10:13:29PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 12/23/2024 8:56 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
>>>>>>>>>> On Mon, Dec 23, 2024 at 12:21:15PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>>>>>> PCIe equalization presets are predefined settings used to optimize
>>>>>>>>>>> signal integrity by compensating for signal loss and distortion in
>>>>>>>>>>> high-speed data transmission.
>>>>>>>>>>>
>>>>>>>>>>> As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
>>>>>>>>>>> of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
>>>>>>>>>>> configure lane equalization presets for each lane to enhance the PCIe
>>>>>>>>>>> link reliability. Each preset value represents a different combination
>>>>>>>>>>> of pre-shoot and de-emphasis values. For each data rate, different
>>>>>>>>>>> registers are defined: for 8.0 GT/s, registers are defined in section
>>>>>>>>>>> 7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
>>>>>>>>>>> an extra receiver preset hint, requiring 16 bits per lane, while the
>>>>>>>>>>> remaining data rates use 8 bits per lane.
>>>>>>>>>>>
>>>>>>>>>>> Based on the number of lanes and the supported data rate, this function
>>>>>>>>>>> reads the device tree property and stores in the presets structure.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Krishna Chaitanya Chundru <[e-mail]>
>>>>>>>>>>> ---
>>>>>>>>>>>       drivers/pci/of.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>       drivers/pci/pci.h | 17 +++++++++++++++--
>>>>>>>>>>>       2 files changed, 60 insertions(+), 2 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>>>>>>>>>>> index dacea3fc5128..99e0e7ae12e9 100644
>>>>>>>>>>> --- a/drivers/pci/of.c
>>>>>>>>>>> +++ b/drivers/pci/of.c
>>>>>>>>>>> @@ -826,3 +826,48 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
>>>>>>>>>>>         return slot_power_limit_mw;
>>>>>>>>>>>       }
>>>>>>>>>>>       EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
>>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>> kerneldoc? Define who should free the memory and how.
>>>>>>>>>>
>>>>>>>>> I will update this in next series.
>>>>>>>>> as we are allocating using devm_kzalloc it should be freed on driver
>>>>>>>>> detach, as no special freeing is required.
>>>>>>>>>>> +int of_pci_get_equalization_presets(struct device *dev,
>>>>>>>>>>> +                                  struct pci_eq_presets *presets,
>>>>>>>>>>> +                                  int num_lanes)
>>>>>>>>>>> +{
>>>>>>>>>>> +      char name[20];
>>>>>>>>>>> +      void **preset;
>>>>>>>>>>> +      void *temp;
>>>>>>>>>>> +      int ret;
>>>>>>>>>>> +
>>>>>>>>>>> +      if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>>>>>>>>>> +              presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
>>>>>>>>>>> +              if (!presets->eq_presets_8gts)
>>>>>>>>>>> +                      return -ENOMEM;
>>>>>>>>>>> +
>>>>>>>>>>> +              ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
>>>>>>>>>>> +                                               presets->eq_presets_8gts, num_lanes);
>>>>>>>>>>> +              if (ret) {
>>>>>>>>>>> +                      dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
>>>>>>>>>>> +                      return ret;
>>>>>>>>>>> +              }
>>>>>>>>>>> +      }
>>>>>>>>>>> +
>>>>>>>>>>> +      for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
>>>>>>>>>>> +              snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>>>>>>>>>> +              if (of_property_present(dev->of_node, name)) {
>>>>>>>>>>> +                      temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
>>>>>>>>>>> +                      if (!temp)
>>>>>>>>>>> +                              return -ENOMEM;
>>>>>>>>>>> +
>>>>>>>>>>> +                      ret = of_property_read_u8_array(dev->of_node, name,
>>>>>>>>>>> +                                                      temp, num_lanes);
>>>>>>>>>>> +                      if (ret) {
>>>>>>>>>>> +                              dev_err(dev, "Error %s %d\n", name, ret);
>>>>>>>>>>> +                              return ret;
>>>>>>>>>>> +                      }
>>>>>>>>>>> +
>>>>>>>>>>> +                      preset = (void **)((u8 *)presets + i * sizeof(void *));
>>>>>>>>>>
>>>>>>>>>> Ugh.
>>>>>>>>>>
>>>>>>>>> I was trying iterate over each element on the structure as presets holds the
>>>>>>>>> starting address of the structure and to that we are adding size of the void
>>>>>>>>> * point to go to each element. I did this way to reduce the
>>>>>>>>> redundant code to read all the gts which has same way of storing the data
>>>>>>>>> from the device tree. I will add comments here in the next series.
>>>>>>>>
>>>>>>>> Please rewrite this in a cleaner way. The code shouldn't raise
>>>>>>>> questions.
>>>>>>>>
>>>>>>>>>>> +                      *preset = temp;
>>>>>>>>>>> +              }
>>>>>>>>>>> +      }
>>>>>>>>>>> +
>>>>>>>>>>> +      return 0;
>>>>>>>>>>> +}
>>>>>>>>>>> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
>>>>>>>>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>>>>>>>>> index 14d00ce45bfa..82362d58bedc 100644
>>>>>>>>>>> --- a/drivers/pci/pci.h
>>>>>>>>>>> +++ b/drivers/pci/pci.h
>>>>>>>>>>> @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
>>>>>>>>>>>       }
>>>>>>>>>>>       struct device_node;
>>>>>>>>>>> -
>>>>>>>>>>> +struct pci_eq_presets {
>>>>>>>>>>> +      void *eq_presets_8gts;
>>>>>>>>>>> +      void *eq_presets_16gts;
>>>>>>>>>>> +      void *eq_presets_32gts;
>>>>>>>>>>> +      void *eq_presets_64gts;
>>>>>>>>>>
>>>>>>>>>> Why are all of those void*? 8gts is u16*, all other are u8*.
>>>>>>>>>>
>>>>>>>>> To have common parsing logic I moved them to void*, as these are pointers
>>>>>>>>> actual memory is allocated by of_pci_get_equalization_presets()
>>>>>>>>> based upon the gts these should not give any issues.
>>>>>>>>
>>>>>>>> Please, don't. They have types. void pointers are for the opaque data.
>>>>>>>>
>>>>>>> ok.
>>>>>>>
>>>>>>> I think then better to use v1 patch
>>>>>>> https://lore.kernel.org/all/[e-mail]/
>>>>>>>
>>>>>>> konrad, any objection on using v1 as that will be cleaner way even if we
>>>>>>> have some repetitive code.
>>>>>>
>>>>>> Konrad had a nice suggestion about using the array of values. Please use
>>>>>> such an array for 16gts and above. This removes most of repetitive code.
>>>>>>
>>>>> I don't feel having array in the preset structure looks good, I have
>>>>> come up with this logic if you feel it is not so good I will go to the
>>>>> suggested way by having array for 16gts and above.
>>>>>
>>>>>          if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>>>>                   presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) *
>>>>> num_lanes, GFP_KERNEL);
>>>>>                   if (!presets->eq_presets_8gts)
>>>>>                           return -ENOMEM;
>>>>>
>>>>>                   ret = of_property_read_u16_array(dev->of_node,
>>>>> "eq-presets-8gts",
>>>>>
>>>>> presets->eq_presets_8gts, num_lanes);
>>>>>                   if (ret) {
>>>>>                           dev_err(dev, "Error reading eq-presets-8gts %d\n",
>>>>> ret);
>>>>>                           return ret;
>>>>>                   }
>>>>>           }
>>>>>
>>>>>           for (int i = EQ_PRESET_TYPE_16GTS; i < EQ_PRESET_TYPE_64GTS; i++) {
>>>>>                   snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>>>>                   if (of_property_present(dev->of_node, name)) {
>>>>>                           temp = devm_kzalloc(dev, sizeof(u8) * num_lanes,
>>>>> GFP_KERNEL);
>>>>>                           if (!temp)
>>>>>                                   return -ENOMEM;
>>>>>
>>>>>                           ret = of_property_read_u8_array(dev->of_node, name,
>>>>>                                                           temp, num_lanes);
>>>>>                           if (ret) {
>>>>>                                   dev_err(dev, "Error %s %d\n", name, ret);
>>>>>                                   return ret;
>>>>>                           }
>>>>>
>>>>>                           switch (i) {
>>>>>                                   case EQ_PRESET_TYPE_16GTS:
>>>>>                                           presets->eq_presets_16gts = temp;
>>>>>                                           break;
>>>>>                                   case EQ_PRESET_TYPE_32GTS:
>>>>>                                           presets->eq_presets_32gts = temp;
>>>>>                                           break;
>>>>>                                   case EQ_PRESET_TYPE_64GTS:
>>>>>                                           presets->eq_presets_64gts = temp;
>>>>>                                           break;
>>>>>                           }
>>>>
>>>> This looks like 'presets->eq_presets[i] = temp;', but I won't insist on
>>>> that.
>>>>
>>>> Also, a strange thought came to my mind: we know that there won't be
>>>> more than 16 lanes. Can we have the following structure instead:
>>>>
>>>> #define MAX_LANES 16
>>>> enum pcie_gts {
>>>>        PCIE_GTS_16GTS,
>>>>        PCIE_GTS_32GTS,
>>>>        PCIE_GTS_64GTS,
>>>>        PCIE_GTS_MAX,
>>>> };
>>>> struct pci_eq_presets {
>>>>        u16 eq_presets_8gts[MAX_LANES];
>>>>        u8 eq_presets_Ngts[PCIE_GTS_MAX][MAX_LANES];
>>>> };
>>>>
>>>> This should allow you to drop the of_property_present() and
>>>> devm_kzalloc(). Just read DT data into a corresponding array.
>>>>
>>> in the dwc driver patch I was using pointers and memory allocation
>>> to known if the property is present or not. If I use this way I might
>>> end up reading dt property again.
>>
>> Add foo_valid flags to the struct.
> 
> Some(u8)/None would be fitting, but we're not there yet :(
> 
> Are all 0x00-0xff(ff) values valid for these presets?
> 
currently 0xff are reserved not sure in future PCIe spec data rates
can use it or not.

- Krishna Chaitanya.
>>>   I think better to switch to have a
>>> array for above 16gts.
>>
>> Whichever way works for you.
> 
> Sorta-answering the earlier email, I have no concerns either
> 
> Konrad

Konrad Dybcio Jan. 3, 2025, 11:43 a.m. UTC | #12

On 31.12.2024 5:38 AM, Krishna Chaitanya Chundru wrote:
> 
> 
> On 12/30/2024 7:11 PM, Konrad Dybcio wrote:
>> On 24.12.2024 11:57 AM, Dmitry Baryshkov wrote:
>>> On Tue, 24 Dec 2024 at 12:36, Krishna Chaitanya Chundru
>>> <[e-mail]> wrote:
>>>>
>>>>
>>>>
>>>> On 12/24/2024 3:25 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, Dec 24, 2024 at 02:47:00PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>
>>>>>>
>>>>>> On 12/24/2024 12:00 AM, Dmitry Baryshkov wrote:
>>>>>>> On Mon, Dec 23, 2024 at 10:13:29PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 12/23/2024 8:56 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
>>>>>>>>>>> On Mon, Dec 23, 2024 at 12:21:15PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>>>>>>> PCIe equalization presets are predefined settings used to optimize
>>>>>>>>>>>> signal integrity by compensating for signal loss and distortion in
>>>>>>>>>>>> high-speed data transmission.
>>>>>>>>>>>>
>>>>>>>>>>>> As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
>>>>>>>>>>>> of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
>>>>>>>>>>>> configure lane equalization presets for each lane to enhance the PCIe
>>>>>>>>>>>> link reliability. Each preset value represents a different combination
>>>>>>>>>>>> of pre-shoot and de-emphasis values. For each data rate, different
>>>>>>>>>>>> registers are defined: for 8.0 GT/s, registers are defined in section
>>>>>>>>>>>> 7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
>>>>>>>>>>>> an extra receiver preset hint, requiring 16 bits per lane, while the
>>>>>>>>>>>> remaining data rates use 8 bits per lane.
>>>>>>>>>>>>
>>>>>>>>>>>> Based on the number of lanes and the supported data rate, this function
>>>>>>>>>>>> reads the device tree property and stores in the presets structure.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Krishna Chaitanya Chundru <[e-mail]>
>>>>>>>>>>>> ---
>>>>>>>>>>>>       drivers/pci/of.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>       drivers/pci/pci.h | 17 +++++++++++++++--
>>>>>>>>>>>>       2 files changed, 60 insertions(+), 2 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>>>>>>>>>>>> index dacea3fc5128..99e0e7ae12e9 100644
>>>>>>>>>>>> --- a/drivers/pci/of.c
>>>>>>>>>>>> +++ b/drivers/pci/of.c
>>>>>>>>>>>> @@ -826,3 +826,48 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
>>>>>>>>>>>>         return slot_power_limit_mw;
>>>>>>>>>>>>       }
>>>>>>>>>>>>       EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
>>>>>>>>>>>> +
>>>>>>>>>>>
>>>>>>>>>>> kerneldoc? Define who should free the memory and how.
>>>>>>>>>>>
>>>>>>>>>> I will update this in next series.
>>>>>>>>>> as we are allocating using devm_kzalloc it should be freed on driver
>>>>>>>>>> detach, as no special freeing is required.
>>>>>>>>>>>> +int of_pci_get_equalization_presets(struct device *dev,
>>>>>>>>>>>> +                                  struct pci_eq_presets *presets,
>>>>>>>>>>>> +                                  int num_lanes)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +      char name[20];
>>>>>>>>>>>> +      void **preset;
>>>>>>>>>>>> +      void *temp;
>>>>>>>>>>>> +      int ret;
>>>>>>>>>>>> +
>>>>>>>>>>>> +      if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>>>>>>>>>>> +              presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
>>>>>>>>>>>> +              if (!presets->eq_presets_8gts)
>>>>>>>>>>>> +                      return -ENOMEM;
>>>>>>>>>>>> +
>>>>>>>>>>>> +              ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
>>>>>>>>>>>> +                                               presets->eq_presets_8gts, num_lanes);
>>>>>>>>>>>> +              if (ret) {
>>>>>>>>>>>> +                      dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
>>>>>>>>>>>> +                      return ret;
>>>>>>>>>>>> +              }
>>>>>>>>>>>> +      }
>>>>>>>>>>>> +
>>>>>>>>>>>> +      for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
>>>>>>>>>>>> +              snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>>>>>>>>>>> +              if (of_property_present(dev->of_node, name)) {
>>>>>>>>>>>> +                      temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
>>>>>>>>>>>> +                      if (!temp)
>>>>>>>>>>>> +                              return -ENOMEM;
>>>>>>>>>>>> +
>>>>>>>>>>>> +                      ret = of_property_read_u8_array(dev->of_node, name,
>>>>>>>>>>>> +                                                      temp, num_lanes);
>>>>>>>>>>>> +                      if (ret) {
>>>>>>>>>>>> +                              dev_err(dev, "Error %s %d\n", name, ret);
>>>>>>>>>>>> +                              return ret;
>>>>>>>>>>>> +                      }
>>>>>>>>>>>> +
>>>>>>>>>>>> +                      preset = (void **)((u8 *)presets + i * sizeof(void *));
>>>>>>>>>>>
>>>>>>>>>>> Ugh.
>>>>>>>>>>>
>>>>>>>>>> I was trying iterate over each element on the structure as presets holds the
>>>>>>>>>> starting address of the structure and to that we are adding size of the void
>>>>>>>>>> * point to go to each element. I did this way to reduce the
>>>>>>>>>> redundant code to read all the gts which has same way of storing the data
>>>>>>>>>> from the device tree. I will add comments here in the next series.
>>>>>>>>>
>>>>>>>>> Please rewrite this in a cleaner way. The code shouldn't raise
>>>>>>>>> questions.
>>>>>>>>>
>>>>>>>>>>>> +                      *preset = temp;
>>>>>>>>>>>> +              }
>>>>>>>>>>>> +      }
>>>>>>>>>>>> +
>>>>>>>>>>>> +      return 0;
>>>>>>>>>>>> +}
>>>>>>>>>>>> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
>>>>>>>>>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>>>>>>>>>> index 14d00ce45bfa..82362d58bedc 100644
>>>>>>>>>>>> --- a/drivers/pci/pci.h
>>>>>>>>>>>> +++ b/drivers/pci/pci.h
>>>>>>>>>>>> @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
>>>>>>>>>>>>       }
>>>>>>>>>>>>       struct device_node;
>>>>>>>>>>>> -
>>>>>>>>>>>> +struct pci_eq_presets {
>>>>>>>>>>>> +      void *eq_presets_8gts;
>>>>>>>>>>>> +      void *eq_presets_16gts;
>>>>>>>>>>>> +      void *eq_presets_32gts;
>>>>>>>>>>>> +      void *eq_presets_64gts;
>>>>>>>>>>>
>>>>>>>>>>> Why are all of those void*? 8gts is u16*, all other are u8*.
>>>>>>>>>>>
>>>>>>>>>> To have common parsing logic I moved them to void*, as these are pointers
>>>>>>>>>> actual memory is allocated by of_pci_get_equalization_presets()
>>>>>>>>>> based upon the gts these should not give any issues.
>>>>>>>>>
>>>>>>>>> Please, don't. They have types. void pointers are for the opaque data.
>>>>>>>>>
>>>>>>>> ok.
>>>>>>>>
>>>>>>>> I think then better to use v1 patch
>>>>>>>> https://lore.kernel.org/all/[e-mail]/
>>>>>>>>
>>>>>>>> konrad, any objection on using v1 as that will be cleaner way even if we
>>>>>>>> have some repetitive code.
>>>>>>>
>>>>>>> Konrad had a nice suggestion about using the array of values. Please use
>>>>>>> such an array for 16gts and above. This removes most of repetitive code.
>>>>>>>
>>>>>> I don't feel having array in the preset structure looks good, I have
>>>>>> come up with this logic if you feel it is not so good I will go to the
>>>>>> suggested way by having array for 16gts and above.
>>>>>>
>>>>>>          if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>>>>>                   presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) *
>>>>>> num_lanes, GFP_KERNEL);
>>>>>>                   if (!presets->eq_presets_8gts)
>>>>>>                           return -ENOMEM;
>>>>>>
>>>>>>                   ret = of_property_read_u16_array(dev->of_node,
>>>>>> "eq-presets-8gts",
>>>>>>
>>>>>> presets->eq_presets_8gts, num_lanes);
>>>>>>                   if (ret) {
>>>>>>                           dev_err(dev, "Error reading eq-presets-8gts %d\n",
>>>>>> ret);
>>>>>>                           return ret;
>>>>>>                   }
>>>>>>           }
>>>>>>
>>>>>>           for (int i = EQ_PRESET_TYPE_16GTS; i < EQ_PRESET_TYPE_64GTS; i++) {
>>>>>>                   snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>>>>>                   if (of_property_present(dev->of_node, name)) {
>>>>>>                           temp = devm_kzalloc(dev, sizeof(u8) * num_lanes,
>>>>>> GFP_KERNEL);
>>>>>>                           if (!temp)
>>>>>>                                   return -ENOMEM;
>>>>>>
>>>>>>                           ret = of_property_read_u8_array(dev->of_node, name,
>>>>>>                                                           temp, num_lanes);
>>>>>>                           if (ret) {
>>>>>>                                   dev_err(dev, "Error %s %d\n", name, ret);
>>>>>>                                   return ret;
>>>>>>                           }
>>>>>>
>>>>>>                           switch (i) {
>>>>>>                                   case EQ_PRESET_TYPE_16GTS:
>>>>>>                                           presets->eq_presets_16gts = temp;
>>>>>>                                           break;
>>>>>>                                   case EQ_PRESET_TYPE_32GTS:
>>>>>>                                           presets->eq_presets_32gts = temp;
>>>>>>                                           break;
>>>>>>                                   case EQ_PRESET_TYPE_64GTS:
>>>>>>                                           presets->eq_presets_64gts = temp;
>>>>>>                                           break;
>>>>>>                           }
>>>>>
>>>>> This looks like 'presets->eq_presets[i] = temp;', but I won't insist on
>>>>> that.
>>>>>
>>>>> Also, a strange thought came to my mind: we know that there won't be
>>>>> more than 16 lanes. Can we have the following structure instead:
>>>>>
>>>>> #define MAX_LANES 16
>>>>> enum pcie_gts {
>>>>>        PCIE_GTS_16GTS,
>>>>>        PCIE_GTS_32GTS,
>>>>>        PCIE_GTS_64GTS,
>>>>>        PCIE_GTS_MAX,
>>>>> };
>>>>> struct pci_eq_presets {
>>>>>        u16 eq_presets_8gts[MAX_LANES];
>>>>>        u8 eq_presets_Ngts[PCIE_GTS_MAX][MAX_LANES];
>>>>> };
>>>>>
>>>>> This should allow you to drop the of_property_present() and
>>>>> devm_kzalloc(). Just read DT data into a corresponding array.
>>>>>
>>>> in the dwc driver patch I was using pointers and memory allocation
>>>> to known if the property is present or not. If I use this way I might
>>>> end up reading dt property again.
>>>
>>> Add foo_valid flags to the struct.
>>
>> Some(u8)/None would be fitting, but we're not there yet :(
>>
>> Are all 0x00-0xff(ff) values valid for these presets?
>>
> currently 0xff are reserved not sure in future PCIe spec data rates
> can use it or not.

Maybe we could use it as a #define-d placeholder then, and kick the
can down the road. Opinions?

Konrad

Patch

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index dacea3fc5128..99e0e7ae12e9 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -826,3 +826,48 @@  u32 of_pci_get_slot_power_limit(struct device_node *node,
 	return slot_power_limit_mw;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
+
+int of_pci_get_equalization_presets(struct device *dev,
+				    struct pci_eq_presets *presets,
+				    int num_lanes)
+{
+	char name[20];
+	void **preset;
+	void *temp;
+	int ret;
+
+	if (of_property_present(dev->of_node, "eq-presets-8gts")) {
+		presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
+		if (!presets->eq_presets_8gts)
+			return -ENOMEM;
+
+		ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
+						 presets->eq_presets_8gts, num_lanes);
+		if (ret) {
+			dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
+			return ret;
+		}
+	}
+
+	for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
+		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
+		if (of_property_present(dev->of_node, name)) {
+			temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
+			if (!temp)
+				return -ENOMEM;
+
+			ret = of_property_read_u8_array(dev->of_node, name,
+							temp, num_lanes);
+			if (ret) {
+				dev_err(dev, "Error %s %d\n", name, ret);
+				return ret;
+			}
+
+			preset = (void **)((u8 *)presets + i * sizeof(void *));
+			*preset = temp;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 14d00ce45bfa..82362d58bedc 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -731,7 +731,12 @@  static inline u64 pci_rebar_size_to_bytes(int size)
 }
 
 struct device_node;
-
+struct pci_eq_presets {
+	void *eq_presets_8gts;
+	void *eq_presets_16gts;
+	void *eq_presets_32gts;
+	void *eq_presets_64gts;
+};
 #ifdef CONFIG_OF
 int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
 int of_get_pci_domain_nr(struct device_node *node);
@@ -746,7 +751,9 @@  void pci_set_bus_of_node(struct pci_bus *bus);
 void pci_release_bus_of_node(struct pci_bus *bus);
 
 int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge);
-
+int of_pci_get_equalization_presets(struct device *dev,
+				    struct pci_eq_presets *presets,
+				    int num_lanes);
 #else
 static inline int
 of_pci_parse_bus_range(struct device_node *node, struct resource *res)
@@ -793,6 +800,12 @@  static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br
 	return 0;
 }
 
+static inline int of_pci_get_equalization_presets(struct device *dev,
+						  struct pci_eq_presets *presets,
+						  int num_lanes)
+{
+	return 0;
+}
 #endif /* CONFIG_OF */
 
 struct of_changeset;