On 11/27/24 20:52, Josua Mayer wrote: > 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. It would not. Simply implement a helper function that scans the OF IDs and return a mask of ports and max ID. With that, the modifications of ahci_platform_get_resources() would be very minimal. >> 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, >> -- Damien Le Moal Western Digital Research