Am 25.11.24 um 02:12 schrieb Damien Le Moal:
> On 11/22/24 12:05 AM, Josua Mayer wrote:
>> So far ahci_platform relied on number of child nodes in firmware to
>> allocate arrays and expected port numbers to start from 0 without holes.
>> This number of ports is then set in private structure for use when
>> configuring phys and regulators.
>>
>> Some platforms may not use every port of an ahci controller.
>> E.g. SolidRUN CN9130 Clearfog uses only port 1 but not port 0, leading
>> to the following errors during boot:
>> [ 1.719476] ahci f2540000.sata: invalid port number 1
>> [ 1.724562] ahci f2540000.sata: No port enabled
>>
>> Remove from ahci_host_priv the property nports which only makes sense
>> when enabled ports are consecutive. It is replaced with AHCI_MAX_PORTS
>> and checks for hpriv->mask_port_map, which indicates each port that is
>> enabled.
>>
>> Update ahci_host_priv properties target_pwrs and phys from dynamically
>> allocated arrays to statically allocated to size AHCI_MAX_PORTS.
>>
>> Update ahci_platform_get_resources to ignore holes in the port numbers
>> and enable ports defined in firmware by their reg property only.
>>
>> When firmware does not define children it is assumed that there is
>> exactly one port, using index 0.
>>
>> I marked this RFC because it was only tested with Linux v6.1, Marvell
>> fork, CN9130 Clearfog Pro which has only port number 1 in device-tree.
>> Further I am not completely sure if it has severe side-effects on
>> other platforms.
>> I plan to submit it again after testing on v6.13-rc1, but do welcome
>> feedback in the meantime, particularly whether this idea of supporting
>> non-consecutive ports is acceptable.
>>
>> Signed-off-by: Josua Mayer <josua@xxxxxxxxxxxxx>
> [...]
>
>
>> @@ -539,41 +544,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>> hpriv->f_rsts = flags & AHCI_PLATFORM_RST_TRIGGER;
>> }
>>
>> - /*
>> - * Too many sub-nodes most likely means having something wrong with
>> - * the firmware.
>> - */
>> child_nodes = of_get_child_count(dev->of_node);
>> - if (child_nodes > AHCI_MAX_PORTS) {
>> - rc = -EINVAL;
>> - goto err_out;
>> - }
> Why remove this check ? Your platform may not need ti, but it is still valid
> for others.
The check is superfluous, since the following loop will print a warning
and ignore any child with port number greater than AHCI_MAX_PORTS.
The check merely protected against dynamically allocating greater than
AHCI_MAX_PORTS.
>
>> -
>> - /*
>> - * If no sub-node was found, we still need to set nports to
>> - * one in order to be able to use the
>> - * ahci_platform_[en|dis]able_[phys|regulators] functions.
>> - */
>> - if (child_nodes)
>> - hpriv->nports = child_nodes;
>> - else
>> - hpriv->nports = 1;
> Same here.
This is already handled in else case of if (child_nodes)
>
>> -
>> - hpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);
>> - if (!hpriv->phys) {
>> - rc = -ENOMEM;
>> - goto err_out;
>> - }
>> - /*
>> - * We cannot use devm_ here, since ahci_platform_put_resources() uses
>> - * target_pwrs after devm_ have freed memory
>> - */
>> - hpriv->target_pwrs = kcalloc(hpriv->nports, sizeof(*hpriv->target_pwrs), GFP_KERNEL);
>> - if (!hpriv->target_pwrs) {
>> - rc = -ENOMEM;
>> - goto err_out;
>> - }
> And for platforms that actually have a valid nports with no ID holes, the above
> is OK and uses less memory...
The port number is being used as index into the target_pwrs and phys arrays,
which is why those arrays must allocate at least to the highest port id.
A better way to save memory is by cleaning out this semantic,
e.g. by dynamically allocating a structure of id, phy and supply for each port.
>
> Why not simply adding code that checks the ID of the child nodes ? If there are
> no ID holes, then nothing need to change. If there are holes, then
> hpriv->nports can be set to the highest ID + 1 and you can set
> hpriv->mask_port_map as you go.
This would make the already complex function more complex and less readable.
I prefer to reduce corner cases rather than adding extras.
> With just that, you should get everything
> working with far less changes than you have here.
>
>> if (child_nodes) {
>> for_each_child_of_node_scoped(dev->of_node, child) {
>> u32 port;
>> @@ -587,7 +558,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>> goto err_out;
>> }
>>
>> - if (port >= hpriv->nports) {
>> + if (port >= AHCI_MAX_PORTS) {
>> dev_warn(dev, "invalid port number %d\n", port);
>> continue;
>> }
>> @@ -625,6 +596,8 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>> * If no sub-node was found, keep this for device tree
>> * compatibility
>> */
>> + hpriv->mask_port_map |= BIT(0);
>> +
>> rc = ahci_platform_get_phy(hpriv, 0, dev, dev->of_node);
>> if (rc)
>> goto err_out;
>>
>> ---
>> base-commit: adc218676eef25575469234709c2d87185ca223a
>> change-id: 20241121-ahci-nonconsecutive-ports-a8911b3255a7
>>
>> Best regards,
>
