| Message ID | 20220511231810.4928-1-Sergey.Semin@baikalelectronics.ru |
|---|---|
| Headers | show |
| Series | ata: ahci: Add DWC/Baikal-T1 AHCI SATA support | expand |
On 5/12/22 01:17, Serge Semin wrote: > It's better for readability and maintainability to explicitly assign an > error number to the variable used then as a return value from the method > on the cleanup-on-error path. So adding new code in the method we won't > have to think whether the overridden rc-variable is set afterward in case > of an error. Saving one line of code doesn't worth it especially seeing > the rest of the ahci_platform_get_resources() function errors handling > blocks do explicitly write errno to rc. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > Changelog v2: > - Drop rc variable initialization (@Damien) > --- > drivers/ata/libahci_platform.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > index 32495ae96567..f7f9bfcfc164 100644 > --- a/drivers/ata/libahci_platform.c > +++ b/drivers/ata/libahci_platform.c > @@ -389,7 +389,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, > struct ahci_host_priv *hpriv; > struct clk *clk; > struct device_node *child; > - int i, enabled_ports = 0, rc = -ENOMEM, child_nodes; > + int i, enabled_ports = 0, rc, child_nodes; > u32 mask_port_map = 0; > > if (!devres_open_group(dev, NULL, GFP_KERNEL)) > @@ -397,8 +397,10 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, > > hpriv = devres_alloc(ahci_platform_put_resources, sizeof(*hpriv), > GFP_KERNEL); > - if (!hpriv) > + if (!hpriv) { > + rc = -ENOMEM; > goto err_out; > + } > > devres_add(dev, hpriv); > I disagree. As 'rc' is now only initialized within a conditional we're risking 'rc' will be left uninitialized. And in the end, it's a matter of style; this patch doesn't change the flow of events and the benefits are hard to see. Cheers, Hannes
On 5/12/22 01:17, Serge Semin wrote: > Currently the IOMEM AHCI registers space is mapped by means of the > two functions invocation: platform_get_resource() is used to get the very > first memory resource and devm_ioremap_resource() is called to remap that > resource. Device-managed kernel API provides a handy wrapper to perform > the same in single function call: devm_platform_ioremap_resource(). > > While at it seeing many AHCI platform drivers rely on having the AHCI CSR > space marked with "ahci" name let's first try to find and remap the CSR > IO-mem with that name and only if it fails fallback to getting the very > first registers space platform resource. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > Changelog v2: > - Check whether there is "ahhi" reg resource before using the > devm_platform_ioremap_resource_byname() method in order to prevent a > false error message printed in the log (@Damien) > - Slightly update the patch title due to the change above and to be more > specific that the platform device managed methods are utilized. > --- > drivers/ata/libahci_platform.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > index f7f9bfcfc164..8c9fbcc3043b 100644 > --- a/drivers/ata/libahci_platform.c > +++ b/drivers/ata/libahci_platform.c > @@ -404,8 +404,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, > > devres_add(dev, hpriv); > > - hpriv->mmio = devm_ioremap_resource(dev, > - platform_get_resource(pdev, IORESOURCE_MEM, 0)); > + /* > + * If the DT provided an "ahci" named resource, use it. Otherwise, > + * fallback to using the default first resource for the device node. > + */ > + if (platform_get_resource_byname(pdev, IORESOURCE_MEM, "ahci")) > + hpriv->mmio = devm_platform_ioremap_resource_byname(pdev, "ahci"); > + else > + hpriv->mmio = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(hpriv->mmio)) { > rc = PTR_ERR(hpriv->mmio); > goto err_out; Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 5/12/22 01:17, Serge Semin wrote: > In order to simplify the clock-related code there is a way to convert the > current fixed clocks array into using the common bulk clocks kernel API > with dynamic set of the clock handlers and device-managed clock-resource > tracking. It's a bit tricky due to the complication coming from the > requirement to support the platforms (da850, spear13xx) with the > non-OF-based clock source, but still doable. > > Before this modification there are two methods have been used to get the > clocks connected to an AHCI device: clk_get() - to get the very first > clock in the list and of_clk_get() - to get the rest of them. Basically > the platforms with non-OF-based clocks definition could specify only a > single reference clock source. The platforms with OF-hw clocks have been > luckier and could setup up to AHCI_MAX_CLKS clocks. Such semantic can be > retained with using devm_clk_bulk_get_all() to retrieve the clocks defined > via the DT firmware and devm_clk_get_optional() otherwise. In both cases > using the device-managed version of the methods will cause the automatic > resources deallocation on the AHCI device removal event. The only > complicated part in the suggested approach is the explicit allocation and > initialization of the clk_bulk_data structure instance for the non-OF > reference clocks. It's required in order to use the Bulk Clocks API for > the both denoted cases of the clocks definition. > > Note aside with the clock-related code reduction and natural > simplification, there are several bonuses the suggested modification > provides. First of all the limitation of having no greater than > AHCI_MAX_CLKS clocks is now removed, since the devm_clk_bulk_get_all() > method will allocate as many reference clocks data descriptors as there > are clocks specified for the device. Secondly the clock names are > auto-detected. So the LLDD (glue) drivers can make sure that the required > clocks are specified just by checking the clock IDs in the clk_bulk_data > array. Thirdly using the handy Bulk Clocks kernel API improves the > clocks-handling code readability. And the last but not least this > modification implements a true optional clocks support to the > ahci_platform_get_resources() method. Indeed the previous clocks getting > procedure just stopped getting the clocks on any errors (aside from > non-critical -EPROBE_DEFER) in a way so the callee wasn't even informed > about abnormal loop termination. The new implementation lacks of such > problem. The ahci_platform_get_resources() will return an error code if > the corresponding clocks getting method ends execution abnormally. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > Changelog v2: > - Convert to checking the error-case first in the devm_clk_bulk_get_all() > method invocation. (@Damien) > - Fix some grammar mistakes in the comments. > --- > drivers/ata/ahci.h | 4 +- > drivers/ata/libahci_platform.c | 84 ++++++++++++++++------------------ > 2 files changed, 41 insertions(+), 47 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 5/12/22 01:17, Serge Semin wrote: > Since all the clocks are retrieved by the method > ahci_platform_get_resources() there is no need for the LLD (glue) drivers > to be looking for some particular of them in the kernel clocks table > again. Instead we suggest to add a simple method returning a > device-specific clock with passed connection ID if it is managed to be > found. Otherwise the function will return NULL. Thus the glue-drivers > won't need to either manually touching the hpriv->clks array or calling > clk_get()-friends. The AHCI platform drivers will be able to use the new > function right after the ahci_platform_get_resources() method invocation > and up to the device removal. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > Changelog v2: > - Fix some grammar mistakes in the method description. > --- > drivers/ata/libahci_platform.c | 27 +++++++++++++++++++++++++++ > include/linux/ahci_platform.h | 3 +++ > 2 files changed, 30 insertions(+) > > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > index 3cff86c225fd..7ff6626fd569 100644 > --- a/drivers/ata/libahci_platform.c > +++ b/drivers/ata/libahci_platform.c > @@ -94,6 +94,33 @@ void ahci_platform_disable_phys(struct ahci_host_priv *hpriv) > } > EXPORT_SYMBOL_GPL(ahci_platform_disable_phys); > > +/** > + * ahci_platform_find_clk - Find platform clock > + * @hpriv: host private area to store config values > + * @con_id: clock connection ID > + * > + * This function returns a pointer to the clock descriptor of the clock with > + * the passed ID. > + * > + * RETURNS: > + * Pointer to the clock descriptor on success otherwise NULL > + */ > +struct clk *ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id) > +{ > + struct clk *clk = NULL; > + int i; > + > + for (i = 0; i < hpriv->n_clks; i++) { > + if (!strcmp(hpriv->clks[i].id, con_id)) { > + clk = hpriv->clks[i].clk; > + break; > + } > + } > + > + return clk; > +} > +EXPORT_SYMBOL_GPL(ahci_platform_find_clk); > + > /** > * ahci_platform_enable_clks - Enable platform clocks > * @hpriv: host private area to store config values > diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h > index 49e5383d4222..fd964e6a68d6 100644 > --- a/include/linux/ahci_platform.h > +++ b/include/linux/ahci_platform.h > @@ -13,6 +13,7 @@ > > #include <linux/compiler.h> > > +struct clk; > struct device; > struct ata_port_info; > struct ahci_host_priv; > @@ -21,6 +22,8 @@ struct scsi_host_template; > > int ahci_platform_enable_phys(struct ahci_host_priv *hpriv); > void ahci_platform_disable_phys(struct ahci_host_priv *hpriv); > +struct clk * > +ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id); > int ahci_platform_enable_clks(struct ahci_host_priv *hpriv); > void ahci_platform_disable_clks(struct ahci_host_priv *hpriv); > int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv); Where is this function being used? Cheers, Hannes
On 5/12/22 01:17, Serge Semin wrote: > Having greater than (AHCI_MAX_PORTS = 32) ports detected isn't that > critical from the further AHCI-platform initialization point of view since > exceeding the ports upper limit will cause allocating more resources than > will be used afterwards. But detecting too many child DT-nodes doesn't > seem right since it's very unlikely to have it on an ordinary platform. In > accordance with the AHCI specification there can't be more than 32 ports > implemented at least due to having the CAP.NP field of 4 bits wide and the > PI register of dword size. Thus if such situation is found the DTB must > have been corrupted and the data read from it shouldn't be reliable. Let's > consider that as an erroneous situation and halt further resources > allocation. > > Note it's logically more correct to have the nports set only after the > initialization value is checked for being sane. So while at it let's make > sure nports is assigned with a correct value. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > Changelog v2: > - Drop the else word from the child_nodes value checking if-else-if > statement (@Damien) and convert the after-else part into the ternary > operator-based statement. > --- > drivers/ata/libahci_platform.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > index 7ff6626fd569..4e54e19f07b2 100644 > --- a/drivers/ata/libahci_platform.c > +++ b/drivers/ata/libahci_platform.c > @@ -480,15 +480,22 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, > } > } > > - hpriv->nports = child_nodes = of_get_child_count(dev->of_node); > + /* > + * 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; > + } > > /* > * 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 = 1; > + hpriv->nports = child_nodes ?: 1; > > hpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL); > if (!hpriv->phys) { Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 5/12/22 01:17, Serge Semin wrote: > The ports-implemented property is mainly used on the OF-based platforms > with no ports mapping initialized by a bootloader/BIOS firmware. Seeing > the same of_property_read_u32()-based pattern has already been implemented > in the generic AHCI LLDD (glue) driver and in the Mediatek, St AHCI > drivers let's move the property read procedure to the generic > ahci_platform_get_resources() method. Thus we'll have the forced ports > mapping feature supported for each OF-based platform which requires that, > and stop re-implementing the same pattern in there a bit simplifying the > code. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > --- > drivers/ata/ahci_mtk.c | 2 -- > drivers/ata/ahci_platform.c | 3 --- > drivers/ata/ahci_st.c | 3 --- > drivers/ata/libahci_platform.c | 3 +++ > 4 files changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/ata/ahci_mtk.c b/drivers/ata/ahci_mtk.c > index 1f6c85fde983..c056378e3e72 100644 > --- a/drivers/ata/ahci_mtk.c > +++ b/drivers/ata/ahci_mtk.c > @@ -118,8 +118,6 @@ static int mtk_ahci_parse_property(struct ahci_host_priv *hpriv, > SYS_CFG_SATA_EN); > } > > - of_property_read_u32(np, "ports-implemented", &hpriv->force_port_map); > - > return 0; > } > > diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c > index 28a8de5b48b9..9b56490ecbc3 100644 > --- a/drivers/ata/ahci_platform.c > +++ b/drivers/ata/ahci_platform.c > @@ -56,9 +56,6 @@ static int ahci_probe(struct platform_device *pdev) > if (rc) > return rc; > > - of_property_read_u32(dev->of_node, > - "ports-implemented", &hpriv->force_port_map); > - > if (of_device_is_compatible(dev->of_node, "hisilicon,hisi-ahci")) > hpriv->flags |= AHCI_HFLAG_NO_FBS | AHCI_HFLAG_NO_NCQ; > > diff --git a/drivers/ata/ahci_st.c b/drivers/ata/ahci_st.c > index 7526653c843b..068621099c00 100644 > --- a/drivers/ata/ahci_st.c > +++ b/drivers/ata/ahci_st.c > @@ -168,9 +168,6 @@ static int st_ahci_probe(struct platform_device *pdev) > > st_ahci_configure_oob(hpriv->mmio); > > - of_property_read_u32(dev->of_node, > - "ports-implemented", &hpriv->force_port_map); > - > err = ahci_platform_init_host(pdev, hpriv, &st_ahci_port_info, > &ahci_platform_sht); > if (err) { > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > index 4e54e19f07b2..f7f9cac10cb1 100644 > --- a/drivers/ata/libahci_platform.c > +++ b/drivers/ata/libahci_platform.c > @@ -512,6 +512,9 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, > goto err_out; > } > > + of_property_read_u32(dev->of_node, > + "ports-implemented", &hpriv->force_port_map); > + > if (child_nodes) { > for_each_child_of_node(dev->of_node, child) { > u32 port; What happens on the other platforms? Won't they register an error if that property isn't implemented? Cheers, Hannes
On 5/12/22 01:17, Serge Semin wrote: > Currently the ACHI-platform library supports only the assert and deassert > reset signals and ignores the platforms with self-deasserting reset lines. > That prone to having the platforms with self-deasserting reset method > misbehaviour when it comes to resuming from sleep state after the clocks > have been fully disabled. For such cases the controller needs to be fully > reset all over after the reference clocks are enabled and stable, > otherwise the controller state machine might be in an undetermined state. > > The best solution would be to auto-detect which reset method is supported > by the particular platform and use it implicitly in the framework of the > ahci_platform_enable_resources()/ahci_platform_disable_resources() > methods. Alas it can't be implemented due to the AHCI-platform library > already supporting the shared reset control lines. As [1] says in such > case we have to use only one of the next methods: > + reset_control_assert()/reset_control_deassert(); > + reset_control_reset()/reset_control_rearm(). > If the driver had an exclusive control over the reset lines we could have > been able to manipulate the lines with no much limitation and just used > the combination of the methods above to cover all the possible > reset-control cases. Since the shared reset control has already been > advertised and couldn't be changed with no risk to breaking the platforms > relying on it, we have no choice but to make the platform drivers to > determine which reset methods the platform reset system supports. > > In order to implement both types of reset control support we suggest to > introduce the new AHCI-platform flag: AHCI_PLATFORM_RST_TRIGGER, which > when passed to the ahci_platform_get_resources() method together with the > AHCI_PLATFORM_GET_RESETS flag will indicate that the reset lines are > self-deasserting thus the reset_control_reset()/reset_control_rearm() will > be used to control the reset state. Otherwise the > reset_control_deassert()/reset_control_assert() methods will be utilized. > > [1] Documentation/driver-api/reset.rst > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > Changelog v2: > - Convert the ahci_platform_assert_rsts() method to returning int status > (@Damien). > - Fix some grammar mistakes in the ahci_platform_deassert_rsts() doc > (@Damien). > --- > drivers/ata/ahci.h | 1 + > drivers/ata/libahci_platform.c | 50 ++++++++++++++++++++++++++++++---- > include/linux/ahci_platform.h | 5 +++- > 3 files changed, 50 insertions(+), 6 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 5/12/22 01:18, Serge Semin wrote: > Currently not all of the Port-specific capabilities listed in the > PORT_CMD-enumeration. Let's extend that set with the Cold Presence > Detection and Mechanical Presence Switch attached to the Port flags [1] so > to closeup the set of the platform-specific port-capabilities flags. Note > these flags are supposed to be set by the platform firmware if there is > one. Alternatively as we are about to do they can be set by means of the > OF properties. > > While at it replace PORT_IRQ_DEV_ILCK with PORT_IRQ_DEV_MPS and fix the > comment there. In accordance with [2] that IRQ flag is supposed to > indicate the state of the signal coming from the Mechanical Presence > Switch. > > [1] Serial ATA AHCI 1.3.1 Specification, p.27 > [2] Serial ATA AHCI 1.3.1 Specification, p.7 > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > --- > drivers/ata/ahci.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > You might want to adapt the subject line, as this patch doesn't touch libahci at all. Other than that: Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 5/12/22 01:18, Serge Semin wrote: > Currently there are four port-map-related fields declared in the > ahci_host_priv structure and used to setup the HBA ports mapping. First > the ports-mapping is read from the PI register and immediately stored in > the saved_port_map field. If forced_port_map is initialized with non-zero > value then its value will have greater priority over the value read from > PI, thus it will override the saved_port_map field. That value will be > then masked by a non-zero mask_port_map field and after some sanity checks > it will be stored in the ahci_host_priv.port_map field as a final port > mapping. > > As you can see the logic is a bit too complicated for such a simple task. > We can freely get rid from at least one of the fields with no change to > the implemented semantic. The force_port_map field can be replaced with > taking non-zero saved_port_map value into account. So if saved_port_map is > pre-initialized by the low level drivers (platform drivers) then it will > have greater priority over the value read from PI register and will be > used as actual HBA ports mapping later on. Thus the ports map forcing task > will be just transferred from the force_port_map to saved_port_map field. > > This modification will perfectly fit into the feature of having OF-based > initialization of the HW-init HBA CSR fields we are about to introduce in > the next commit. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > --- > drivers/ata/ahci.c | 2 +- > drivers/ata/ahci.h | 1 - > drivers/ata/libahci.c | 10 ++++++---- > drivers/ata/libahci_platform.c | 2 +- > 4 files changed, 8 insertions(+), 7 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 5/12/22 01:18, Serge Semin wrote: > There is no point in reading the AHCI version all over in the tail of the > ahci_save_initial_config() method. That register is RO and doesn't change > its value even after reset. So just reuse the data, which has already been > read from there earlier in the head of the function. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > --- > drivers/ata/libahci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c > index 000a7072614f..1ffaa5f5f21a 100644 > --- a/drivers/ata/libahci.c > +++ b/drivers/ata/libahci.c > @@ -564,7 +564,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv) > /* record values to use during operation */ > hpriv->cap = cap; > hpriv->cap2 = cap2; > - hpriv->version = readl(mmio + HOST_VERSION); > + hpriv->version = vers; > hpriv->port_map = port_map; > > if (!hpriv->start_engine) Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 5/12/22 01:18, Serge Semin wrote: > There are systems with no BIOS or comprehensive embedded firmware which > could be able to properly initialize the SATA AHCI controller > platform-specific capabilities. In that case a good alternative to having > a clever bootloader is to create a device tree node with the properties > well describing all the AHCI-related platform specifics. All the settings > which are normally detected and marked as available in the HBA and its > ports capabilities fields [1] could be defined in the platform DTB by > means of a set of the dedicated properties. Such approach perfectly fits > to the DTB-philosophy - to provide hardware/platform description. > > So here we suggest to extend the SATA AHCI device tree bindings with the > next set of additional DT boolean properties: > 1) hba-sss - Controller supports Staggered Spin-up. > 2) hba-smps - Mechanical Presence Switch is support by controller. > 3) hba-hpcp - Hot Plug Capable Port. > 4) hba-mpsp - Mechanical Presence Switch Attached to Port. > 5) hba-cpd - Cold Presence Detection. > 6) hba-esp - External SATA Port. > 7) hba-fbscp - FIS-based Switching Capable Port. > All of these capabilities require to have a corresponding hardware > configuration. Thus it's ok to have them defined in DTB. > > Even though the driver currently takes into account the state of the ESP > and FBSCP flags state only, there is nothing wrong with having all them > supported by the generic AHCI library in order to have a complete OF-based > platform-capabilities initialization procedure. These properties will be > parsed in the ahci_platform_get_resources() method and their values will > be stored in the saved_* fields of the ahci_host_priv structure, which in > its turn then will be used to restore the H.CAP, H.PI and P#.CMD > capability fields on device init and after HBA reset. > > Please note this modification concerns the HW-init HBA and its ports flags > only, which are by specification [1] are supposed to be initialized by the > BIOS/platform firmware/expansion ROM and which are normally declared in > the one-time-writable-after-reset register fields. Even though these flags > aren't supposed to be cleared after HBA reset some AHCI instances may > violate that rule so we still need to perform the fields resetting after > each reset. Luckily the corresponding functionality has already been > partly implemented in the framework of the ahci_save_initial_config() and > ahci_restore_initial_config() methods. > > [1] Serial ATA AHCI 1.3.1 Specification, p. 103 > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > --- > drivers/ata/ahci.h | 1 + > drivers/ata/libahci.c | 51 ++++++++++++++++++++++++++++------ > drivers/ata/libahci_platform.c | 51 ++++++++++++++++++++++++++++++++-- > 3 files changed, 92 insertions(+), 11 deletions(-) > > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h > index 8b9826533ae5..0de221055961 100644 > --- a/drivers/ata/ahci.h > +++ b/drivers/ata/ahci.h > @@ -337,6 +337,7 @@ struct ahci_host_priv { > u32 saved_cap; /* saved initial cap */ > u32 saved_cap2; /* saved initial cap2 */ > u32 saved_port_map; /* saved initial port_map */ > + u32 saved_port_cap[AHCI_MAX_PORTS]; /* saved port_cap */ > u32 em_loc; /* enclosure management location */ > u32 em_buf_sz; /* EM buffer size in byte */ > u32 em_msg_type; /* EM message type */ > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c > index 1ffaa5f5f21a..763ff1058da6 100644 > --- a/drivers/ata/libahci.c > +++ b/drivers/ata/libahci.c > @@ -16,6 +16,7 @@ > * http://www.intel.com/technology/serialata/pdf/rev1_1.pdf > */ > > +#include <linux/bitops.h> > #include <linux/kernel.h> > #include <linux/gfp.h> > #include <linux/module.h> > @@ -443,16 +444,28 @@ static ssize_t ahci_show_em_supported(struct device *dev, > void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv) > { > void __iomem *mmio = hpriv->mmio; > - u32 cap, cap2, vers, port_map; > + void __iomem *port_mmio; > + unsigned long port_map; Why is this an 'unsigned long' now? I thought we could have only 32 ports? > + u32 cap, cap2, vers; > int i; > > /* make sure AHCI mode is enabled before accessing CAP */ > ahci_enable_ahci(mmio); > > - /* Values prefixed with saved_ are written back to host after > - * reset. Values without are used for driver operation. > + /* > + * Values prefixed with saved_ are written back to the HBA and ports > + * registers after reset. Values without are used for driver operation. > + */ > + > + /* > + * Override HW-init HBA capability fields with platform-specific values. > + * The rest of the HBA capabilities are defined with strictly RO flags > + * and can't be modified in CSR anyway. > */ > - hpriv->saved_cap = cap = readl(mmio + HOST_CAP); > + cap = readl(mmio + HOST_CAP); > + if (hpriv->saved_cap) > + cap = (cap & ~(HOST_CAP_SSS | HOST_CAP_MPS)) | hpriv->saved_cap; > + hpriv->saved_cap = cap; > > /* CAP2 register is only defined for AHCI 1.2 and later */ > vers = readl(mmio + HOST_VERSION); > @@ -519,7 +532,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv) > /* Override the HBA ports mapping if the platform needs it */ > port_map = readl(mmio + HOST_PORTS_IMPL); And we're still using 'readl', so we will only initialize 32 bits ... > if (hpriv->saved_port_map && port_map != hpriv->saved_port_map) { Tsk. And now comparing is to a u32 ... Please change it back to be 32 bits. Cheers, Hannes
On 5/12/22 01:18, Serge Semin wrote: > It may get required to retrieve the port-base address even before the > ata_host instance is initialized and activated, for instance in the > ahci_save_initial_config() method which we about to update (consider this > modification as a preparation for that one). Seeing the __ahci_port_base() > function isn't used much it's the best candidate to provide the required > functionality. So let's convert it to accepting the ahci_host_priv > structure pointer. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > --- > drivers/ata/ahci.c | 2 +- > drivers/ata/ahci.h | 7 ++++--- > 2 files changed, 5 insertions(+), 4 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 5/12/22 01:18, Serge Semin wrote: > Synopsys AHCI SATA controller can work pretty under with the generic > AHCI-platform driver control. But there are vendor-specific peculiarities > which can tune the device performance up and which may need to be fixed up > for proper device functioning. In addition some DWC AHCI-based controllers > may require small platform-specific fixups, so adding them in the generic > AHCI driver would have ruined the code simplicity. Shortly speaking in > order to keep the generic AHCI-platform code clean and have DWC AHCI > SATA-specific features supported we suggest to add a dedicated DWC AHCI > SATA device driver. Aside with the standard AHCI-platform resources > getting, enabling/disabling and the controller registration the new driver > performs the next actions. > > First of all there is a way to verify whether the HBA/ports capabilities > activated in OF are correct. Almost all features availability is reflected > in the vendor-specific parameters registers. So the DWC AHCI driver does > the capabilities sanity check based on the corresponding fields state. > > Secondly if either the Command Completion Coalescing or the Device Sleep > feature is enabled the DWC AHCI-specific internal 1ms timer must be fixed > in accordance with the application clock signal frequency. In particular > the timer value must be set to be Fapp * 1000. Normally the SoC designers > pre-configure the TIMER1MS register to contain a correct value by default. > But the platforms can support the application clock rate change. If that > happens the 1ms timer value must be accordingly updated otherwise the > dependent features won't work as expected. In the DWC AHCI driver we > suggest to rely on the "aclk" reference clock rate to set the timer > interval up. That clock source is supposed to be the AHCI SATA application > clock in accordance with the DT bindings. > > Finally DWC AHCI SATA controller AXI/AHB bus DMA-engine can be tuned up to > transfer up to 1024 * FIFO words at a time by setting the Tx/Rx > transaction size in the DMA control register. The maximum value depends on > the DMA data bus and AXI/AHB bus maximum burst length. In most of the > cases it's better to set the maximum possible value to reach the best AHCI > SATA controller performance. But sometimes in order to improve the system > interconnect responsiveness, transferring in smaller data chunks may be > more preferable. For such cases and for the case when the default value > doesn't provide the best DMA bus performance we suggest to use the new > HBA-port specific DT-properties "snps,{tx,rx}-ts-max" to tune the DMA > transactions size up. > > After all the settings denoted above are handled the DWC AHCI SATA driver > proceeds further with the standard AHCI-platform host initializations. > > Note since DWC AHCI controller is now have a dedicated driver we can > discard the corresponding compatible string from the ahci-platform.c > module. The same concerns "snps,spear-ahci" compatible string, which is > also based on the DWC AHCI IP-core. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > Note there are three more AHCI SATA drivers which have been created for > the devices based on the DWC AHCI SATA IP-core. It's AHCI SunXi, St and > iMX drivers. Mostly they don't support the features implemented in this > driver. So hopefully sometime in future they can be converted to be based > on the generic DWC AHCI SATA driver and just perform some > subvendor-specific setups in their own LLDD (glue) driver code. But for > now let's leave the generic DWC AHCI SATA code as is. Hopefully the new > DWC AHCI-based device drivers will try at least to re-use a part of the > DWC AHCI driver methods if not being able to be integrated in the generic > DWC driver code. > > Changelog v2: > - Change the local objects prefix from 'dwc_ahci_' to 'ahci_dwc_'. > (@Damien) > --- > drivers/ata/Kconfig | 10 + > drivers/ata/Makefile | 1 + > drivers/ata/ahci_dwc.c | 395 ++++++++++++++++++++++++++++++++++++ > drivers/ata/ahci_platform.c | 2 - > 4 files changed, 406 insertions(+), 2 deletions(-) > create mode 100644 drivers/ata/ahci_dwc.c > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 5/12/22 01:18, Serge Semin wrote: > Some DWC AHCI SATA IP-core derivatives require to perform small platform > or IP-core specific setups. They are too small to be placed in a dedicated > driver. It's just much easier to have a set of quirks for them right in > the DWC AHCI driver code. Since we are about to add such platform support, > as a pre-requisite we introduce a platform-data based DWC AHCI quirks API. > The platform data can be used to define the flags passed to the > ahci_platform_get_resources() method, additional AHCI host-flags and a set > of callbacks to initialize, re-initialize and clear the platform settings. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > Changelog v2: > - Change the local objects prefix from 'dwc_ahci_' to 'ahci_dwc_'. > (@Damien) > --- > drivers/ata/ahci_dwc.c | 52 ++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 48 insertions(+), 4 deletions(-) > This really could be merged with patch 19; as you're adding a new driver you might as well fold this patch in to avoid further modifications. Cheers, Hannes
On 5/12/22 01:18, Serge Semin wrote: > It's almost fully compatible DWC AHCI SATA IP-core derivative except the > reference clocks source, which need to be very carefully selected. In > particular the DWC AHCI SATA PHY can be clocked either from the pads > ref_pad_clk_{m,p} or from the internal wires ref_alt_clk_{m,n}. In the > later case the clock signal is generated from the Baikal-T1 CCU SATA PLL. > The clocks source is selected by means of the ref_use_pad wire connected > to the CCU SATA reference clock CSR. > > In normal situation it would be much more handy to use the internal > reference clock source, but alas we haven't managed to make the AHCI > controller working well with it so far. So it's preferable to have the > controller clocked from the external clock generator and fallback to the > internal clock source only as a last resort. Other than that the > controller is full compatible with the DWC AHCI SATA IP-core. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > Changelog v2: > - Rename 'syscon' property to 'baikal,bt1-syscon'. > - Change the local objects prefix from 'dwc_ahci_' to 'ahci_dwc_', > from 'bt1_ahci_' to 'ahci_bt1_'. (@Damien) > --- > drivers/ata/Kconfig | 1 + > drivers/ata/ahci_dwc.c | 87 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 88 insertions(+) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 5/12/22 01:18, Serge Semin wrote: > Add myself as a maintainer of the new DWC AHCI SATA driver and > its DT-bindings schema. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > Changelog v2: > - Use dlemoal/libata.git git tree for the LIBATA SATA AHCI SYNOPSYS > DWC driver (@Damien). > --- > MAINTAINERS | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 40fa1955ca3f..04f470519708 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -11135,6 +11135,15 @@ F: drivers/ata/ahci_platform.c > F: drivers/ata/libahci_platform.c > F: include/linux/ahci_platform.h > > +LIBATA SATA AHCI SYNOPSYS DWC CONTROLLER DRIVER > +M: Serge Semin <fancer.lancer@gmail.com> > +L: linux-ide@vger.kernel.org > +S: Maintained > +T: git git://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/libata.git > +F: Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml > +F: Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml > +F: drivers/ata/ahci_dwc.c > + > LIBATA SATA PROMISE TX2/TX4 CONTROLLER DRIVER > M: Mikael Pettersson <mikpelinux@gmail.com> > L: linux-ide@vger.kernel.org Tsk. One would have thought that you'd be using the same email address for sending patches, and not specifying a different one for the maintainers file. There is this thing about proof of authenticity and all that ... I'm not really comfortable with this. Please use the same email address for both the Maintainers file and for sending patches. Cheers, Hannes
On 5/12/22 2:17 AM, Serge Semin wrote: > Having greater than (AHCI_MAX_PORTS = 32) ports detected isn't that Having greater than AHCI_MAX_PORTS (32) ports detected? > critical from the further AHCI-platform initialization point of view since > exceeding the ports upper limit will cause allocating more resources than > will be used afterwards. But detecting too many child DT-nodes doesn't > seem right since it's very unlikely to have it on an ordinary platform. In > accordance with the AHCI specification there can't be more than 32 ports > implemented at least due to having the CAP.NP field of 4 bits wide and the It's 5 bits wide, actually... > PI register of dword size. Thus if such situation is found the DTB must > have been corrupted and the data read from it shouldn't be reliable. Let's > consider that as an erroneous situation and halt further resources > allocation. > > Note it's logically more correct to have the nports set only after the > initialization value is checked for being sane. So while at it let's make > sure nports is assigned with a correct value. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> [...] MBR, Sergey
On 2022/05/12 8:27, Hannes Reinecke wrote: > On 5/12/22 01:17, Serge Semin wrote: >> It's better for readability and maintainability to explicitly assign an >> error number to the variable used then as a return value from the method >> on the cleanup-on-error path. So adding new code in the method we won't >> have to think whether the overridden rc-variable is set afterward in case >> of an error. Saving one line of code doesn't worth it especially seeing >> the rest of the ahci_platform_get_resources() function errors handling >> blocks do explicitly write errno to rc. >> >> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> >> >> --- >> >> Changelog v2: >> - Drop rc variable initialization (@Damien) >> --- >> drivers/ata/libahci_platform.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c >> index 32495ae96567..f7f9bfcfc164 100644 >> --- a/drivers/ata/libahci_platform.c >> +++ b/drivers/ata/libahci_platform.c >> @@ -389,7 +389,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, >> struct ahci_host_priv *hpriv; >> struct clk *clk; >> struct device_node *child; >> - int i, enabled_ports = 0, rc = -ENOMEM, child_nodes; >> + int i, enabled_ports = 0, rc, child_nodes; >> u32 mask_port_map = 0; >> >> if (!devres_open_group(dev, NULL, GFP_KERNEL)) >> @@ -397,8 +397,10 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, >> >> hpriv = devres_alloc(ahci_platform_put_resources, sizeof(*hpriv), >> GFP_KERNEL); >> - if (!hpriv) >> + if (!hpriv) { >> + rc = -ENOMEM; >> goto err_out; >> + } >> >> devres_add(dev, hpriv); >> > I disagree. > As 'rc' is now only initialized within a conditional we're risking 'rc' > will be left uninitialized. > And in the end, it's a matter of style; this patch doesn't change the > flow of events and the benefits are hard to see. Yes. Let's drop this patch. Not improving anything. > > Cheers, > > Hannes
On Thu, May 12, 2022 at 12:32:42PM +0200, Damien Le Moal wrote: > On 2022/05/12 8:27, Hannes Reinecke wrote: > > On 5/12/22 01:17, Serge Semin wrote: > >> It's better for readability and maintainability to explicitly assign an > >> error number to the variable used then as a return value from the method > >> on the cleanup-on-error path. So adding new code in the method we won't > >> have to think whether the overridden rc-variable is set afterward in case > >> of an error. Saving one line of code doesn't worth it especially seeing > >> the rest of the ahci_platform_get_resources() function errors handling > >> blocks do explicitly write errno to rc. > >> > >> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > >> > >> --- > >> > >> Changelog v2: > >> - Drop rc variable initialization (@Damien) > >> --- > >> drivers/ata/libahci_platform.c | 6 ++++-- > >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > >> index 32495ae96567..f7f9bfcfc164 100644 > >> --- a/drivers/ata/libahci_platform.c > >> +++ b/drivers/ata/libahci_platform.c > >> @@ -389,7 +389,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, > >> struct ahci_host_priv *hpriv; > >> struct clk *clk; > >> struct device_node *child; > >> - int i, enabled_ports = 0, rc = -ENOMEM, child_nodes; > >> + int i, enabled_ports = 0, rc, child_nodes; > >> u32 mask_port_map = 0; > >> > >> if (!devres_open_group(dev, NULL, GFP_KERNEL)) > >> @@ -397,8 +397,10 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, > >> > >> hpriv = devres_alloc(ahci_platform_put_resources, sizeof(*hpriv), > >> GFP_KERNEL); > >> - if (!hpriv) > >> + if (!hpriv) { > >> + rc = -ENOMEM; > >> goto err_out; > >> + } > >> > >> devres_add(dev, hpriv); > >> > > I disagree. > > As 'rc' is now only initialized within a conditional we're risking 'rc' > > will be left uninitialized. That's what I told to @Damien in v1. > > And in the end, it's a matter of style; this patch doesn't change the > > flow of events and the benefits are hard to see. As a first time reader of the module I've saw them. It was hard to comprehend right from the code context whether rc was properly initialized especially seeing there are so many local variables defined. Unified rc-initialization approach makes code easier to read for sure. In this case the rc variable is re-initialized in each error-case. So having it defaulted to a value which is relevant to the code block in the twenty lines below isn't the most optimized design. > > Yes. Let's drop this patch. Not improving anything. It's up to you to decide after all. Even though I disagree with your opinions the patch will be dropped in v4. -Sergey > > > > > Cheers, > > > > Hannes > > > -- > Damien Le Moal > Western Digital Research
On Thu, May 12, 2022 at 08:32:37AM +0200, Hannes Reinecke wrote: > On 5/12/22 01:17, Serge Semin wrote: > > Since all the clocks are retrieved by the method > > ahci_platform_get_resources() there is no need for the LLD (glue) drivers > > to be looking for some particular of them in the kernel clocks table > > again. Instead we suggest to add a simple method returning a > > device-specific clock with passed connection ID if it is managed to be > > found. Otherwise the function will return NULL. Thus the glue-drivers > > won't need to either manually touching the hpriv->clks array or calling > > clk_get()-friends. The AHCI platform drivers will be able to use the new > > function right after the ahci_platform_get_resources() method invocation > > and up to the device removal. > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > --- > > > > Changelog v2: > > - Fix some grammar mistakes in the method description. > > --- > > drivers/ata/libahci_platform.c | 27 +++++++++++++++++++++++++++ > > include/linux/ahci_platform.h | 3 +++ > > 2 files changed, 30 insertions(+) > > > > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > > index 3cff86c225fd..7ff6626fd569 100644 > > --- a/drivers/ata/libahci_platform.c > > +++ b/drivers/ata/libahci_platform.c > > @@ -94,6 +94,33 @@ void ahci_platform_disable_phys(struct ahci_host_priv *hpriv) > > } > > EXPORT_SYMBOL_GPL(ahci_platform_disable_phys); > > +/** > > + * ahci_platform_find_clk - Find platform clock > > + * @hpriv: host private area to store config values > > + * @con_id: clock connection ID > > + * > > + * This function returns a pointer to the clock descriptor of the clock with > > + * the passed ID. > > + * > > + * RETURNS: > > + * Pointer to the clock descriptor on success otherwise NULL > > + */ > > +struct clk *ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id) > > +{ > > + struct clk *clk = NULL; > > + int i; > > + > > + for (i = 0; i < hpriv->n_clks; i++) { > > + if (!strcmp(hpriv->clks[i].id, con_id)) { > > + clk = hpriv->clks[i].clk; > > + break; > > + } > > + } > > + > > + return clk; > > +} > > +EXPORT_SYMBOL_GPL(ahci_platform_find_clk); > > + > > /** > > * ahci_platform_enable_clks - Enable platform clocks > > * @hpriv: host private area to store config values > > diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h > > index 49e5383d4222..fd964e6a68d6 100644 > > --- a/include/linux/ahci_platform.h > > +++ b/include/linux/ahci_platform.h > > @@ -13,6 +13,7 @@ > > #include <linux/compiler.h> > > +struct clk; > > struct device; > > struct ata_port_info; > > struct ahci_host_priv; > > @@ -21,6 +22,8 @@ struct scsi_host_template; > > int ahci_platform_enable_phys(struct ahci_host_priv *hpriv); > > void ahci_platform_disable_phys(struct ahci_host_priv *hpriv); > > +struct clk * > > +ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id); > > int ahci_platform_enable_clks(struct ahci_host_priv *hpriv); > > void ahci_platform_disable_clks(struct ahci_host_priv *hpriv); > > int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv); > > Where is this function being used? It will be used in the DWC AHCI SATA driver and can be utilized in the rest of the drivers to simplify the available clocks access. BTW Damien asked the same question in v1. My response was the same. -Sergey > > Cheers, > > Hannes > -- > Dr. Hannes Reinecke Kernel Storage Architect > hare@suse.de +49 911 74053 688 > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg > HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
On Thu, May 12, 2022 at 08:48:24AM +0200, Hannes Reinecke wrote: > On 5/12/22 01:17, Serge Semin wrote: > > The ports-implemented property is mainly used on the OF-based platforms > > with no ports mapping initialized by a bootloader/BIOS firmware. Seeing > > the same of_property_read_u32()-based pattern has already been implemented > > in the generic AHCI LLDD (glue) driver and in the Mediatek, St AHCI > > drivers let's move the property read procedure to the generic > > ahci_platform_get_resources() method. Thus we'll have the forced ports > > mapping feature supported for each OF-based platform which requires that, > > and stop re-implementing the same pattern in there a bit simplifying the > > code. > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > drivers/ata/ahci_mtk.c | 2 -- > > drivers/ata/ahci_platform.c | 3 --- > > drivers/ata/ahci_st.c | 3 --- > > drivers/ata/libahci_platform.c | 3 +++ > > 4 files changed, 3 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/ata/ahci_mtk.c b/drivers/ata/ahci_mtk.c > > index 1f6c85fde983..c056378e3e72 100644 > > --- a/drivers/ata/ahci_mtk.c > > +++ b/drivers/ata/ahci_mtk.c > > @@ -118,8 +118,6 @@ static int mtk_ahci_parse_property(struct ahci_host_priv *hpriv, > > SYS_CFG_SATA_EN); > > } > > - of_property_read_u32(np, "ports-implemented", &hpriv->force_port_map); > > - > > return 0; > > } > > diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c > > index 28a8de5b48b9..9b56490ecbc3 100644 > > --- a/drivers/ata/ahci_platform.c > > +++ b/drivers/ata/ahci_platform.c > > @@ -56,9 +56,6 @@ static int ahci_probe(struct platform_device *pdev) > > if (rc) > > return rc; > > - of_property_read_u32(dev->of_node, > > - "ports-implemented", &hpriv->force_port_map); > > - > > if (of_device_is_compatible(dev->of_node, "hisilicon,hisi-ahci")) > > hpriv->flags |= AHCI_HFLAG_NO_FBS | AHCI_HFLAG_NO_NCQ; > > diff --git a/drivers/ata/ahci_st.c b/drivers/ata/ahci_st.c > > index 7526653c843b..068621099c00 100644 > > --- a/drivers/ata/ahci_st.c > > +++ b/drivers/ata/ahci_st.c > > @@ -168,9 +168,6 @@ static int st_ahci_probe(struct platform_device *pdev) > > st_ahci_configure_oob(hpriv->mmio); > > - of_property_read_u32(dev->of_node, > > - "ports-implemented", &hpriv->force_port_map); > > - > > err = ahci_platform_init_host(pdev, hpriv, &st_ahci_port_info, > > &ahci_platform_sht); > > if (err) { > > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > > index 4e54e19f07b2..f7f9cac10cb1 100644 > > --- a/drivers/ata/libahci_platform.c > > +++ b/drivers/ata/libahci_platform.c > > @@ -512,6 +512,9 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, > > goto err_out; > > } > > + of_property_read_u32(dev->of_node, > > + "ports-implemented", &hpriv->force_port_map); > > + > > if (child_nodes) { > > for_each_child_of_node(dev->of_node, child) { > > u32 port; > > What happens on the other platforms? > Won't they register an error if that property isn't implemented? No. The force_port_map field will be left unmodified (zero by default) in case if the optional 'ports-implemented' property is unavailable. See the of_property_read_u32_array() method kdoc: https://elixir.bootlin.com/linux/latest/source/include/linux/of.h#L1261 -Sergey > > Cheers, > > Hannes > -- > Dr. Hannes Reinecke Kernel Storage Architect > hare@suse.de +49 911 74053 688 > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg > HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
On Thu, May 12, 2022 at 11:24:22AM +0300, Sergei Shtylyov wrote: > On 5/12/22 2:17 AM, Serge Semin wrote: > > > Having greater than (AHCI_MAX_PORTS = 32) ports detected isn't that > > Having greater than AHCI_MAX_PORTS (32) ports detected? Ok. > > > critical from the further AHCI-platform initialization point of view since > > exceeding the ports upper limit will cause allocating more resources than > > will be used afterwards. But detecting too many child DT-nodes doesn't > > seem right since it's very unlikely to have it on an ordinary platform. In > > accordance with the AHCI specification there can't be more than 32 ports > > implemented at least due to having the CAP.NP field of 4 bits wide and the > > It's 5 bits wide, actually... Right =) The denoted comments will be taken into account in v4. Thanks. -Sergey > > > PI register of dword size. Thus if such situation is found the DTB must > > have been corrupted and the data read from it shouldn't be reliable. Let's > > consider that as an erroneous situation and halt further resources > > allocation. > > > > Note it's logically more correct to have the nports set only after the > > initialization value is checked for being sane. So while at it let's make > > sure nports is assigned with a correct value. > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > [...] > > MBR, Sergey
On Thu, May 12, 2022 at 08:57:55AM +0200, Hannes Reinecke wrote: > On 5/12/22 01:18, Serge Semin wrote: > > Currently not all of the Port-specific capabilities listed in the > > PORT_CMD-enumeration. Let's extend that set with the Cold Presence > > Detection and Mechanical Presence Switch attached to the Port flags [1] so > > to closeup the set of the platform-specific port-capabilities flags. Note > > these flags are supposed to be set by the platform firmware if there is > > one. Alternatively as we are about to do they can be set by means of the > > OF properties. > > > > While at it replace PORT_IRQ_DEV_ILCK with PORT_IRQ_DEV_MPS and fix the > > comment there. In accordance with [2] that IRQ flag is supposed to > > indicate the state of the signal coming from the Mechanical Presence > > Switch. > > > > [1] Serial ATA AHCI 1.3.1 Specification, p.27 > > [2] Serial ATA AHCI 1.3.1 Specification, p.7 > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > drivers/ata/ahci.h | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > You might want to adapt the subject line, as this patch doesn't touch > libahci at all. AFAICS ahci.h is mainly (if not to say only) the header file of the libahci.c module interface. Modifying ahci.h I basically update the libahci interface. So based on that the subject is fully correct. > Other than that: > > Reviewed-by: Hannes Reinecke <hare@suse.de> Thanks. -Sergey > > Cheers, > > Hannes > -- > Dr. Hannes Reinecke Kernel Storage Architect > hare@suse.de +49 911 74053 688 > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg > HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
On Thu, May 12, 2022 at 09:05:01AM +0200, Hannes Reinecke wrote: > On 5/12/22 01:18, Serge Semin wrote: > > There are systems with no BIOS or comprehensive embedded firmware which > > could be able to properly initialize the SATA AHCI controller > > platform-specific capabilities. In that case a good alternative to having > > a clever bootloader is to create a device tree node with the properties > > well describing all the AHCI-related platform specifics. All the settings > > which are normally detected and marked as available in the HBA and its > > ports capabilities fields [1] could be defined in the platform DTB by > > means of a set of the dedicated properties. Such approach perfectly fits > > to the DTB-philosophy - to provide hardware/platform description. > > > > So here we suggest to extend the SATA AHCI device tree bindings with the > > next set of additional DT boolean properties: > > 1) hba-sss - Controller supports Staggered Spin-up. > > 2) hba-smps - Mechanical Presence Switch is support by controller. > > 3) hba-hpcp - Hot Plug Capable Port. > > 4) hba-mpsp - Mechanical Presence Switch Attached to Port. > > 5) hba-cpd - Cold Presence Detection. > > 6) hba-esp - External SATA Port. > > 7) hba-fbscp - FIS-based Switching Capable Port. > > All of these capabilities require to have a corresponding hardware > > configuration. Thus it's ok to have them defined in DTB. > > > > Even though the driver currently takes into account the state of the ESP > > and FBSCP flags state only, there is nothing wrong with having all them > > supported by the generic AHCI library in order to have a complete OF-based > > platform-capabilities initialization procedure. These properties will be > > parsed in the ahci_platform_get_resources() method and their values will > > be stored in the saved_* fields of the ahci_host_priv structure, which in > > its turn then will be used to restore the H.CAP, H.PI and P#.CMD > > capability fields on device init and after HBA reset. > > > > Please note this modification concerns the HW-init HBA and its ports flags > > only, which are by specification [1] are supposed to be initialized by the > > BIOS/platform firmware/expansion ROM and which are normally declared in > > the one-time-writable-after-reset register fields. Even though these flags > > aren't supposed to be cleared after HBA reset some AHCI instances may > > violate that rule so we still need to perform the fields resetting after > > each reset. Luckily the corresponding functionality has already been > > partly implemented in the framework of the ahci_save_initial_config() and > > ahci_restore_initial_config() methods. > > > > [1] Serial ATA AHCI 1.3.1 Specification, p. 103 > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > drivers/ata/ahci.h | 1 + > > drivers/ata/libahci.c | 51 ++++++++++++++++++++++++++++------ > > drivers/ata/libahci_platform.c | 51 ++++++++++++++++++++++++++++++++-- > > 3 files changed, 92 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h > > index 8b9826533ae5..0de221055961 100644 > > --- a/drivers/ata/ahci.h > > +++ b/drivers/ata/ahci.h > > @@ -337,6 +337,7 @@ struct ahci_host_priv { > > u32 saved_cap; /* saved initial cap */ > > u32 saved_cap2; /* saved initial cap2 */ > > u32 saved_port_map; /* saved initial port_map */ > > + u32 saved_port_cap[AHCI_MAX_PORTS]; /* saved port_cap */ > > u32 em_loc; /* enclosure management location */ > > u32 em_buf_sz; /* EM buffer size in byte */ > > u32 em_msg_type; /* EM message type */ > > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c > > index 1ffaa5f5f21a..763ff1058da6 100644 > > --- a/drivers/ata/libahci.c > > +++ b/drivers/ata/libahci.c > > @@ -16,6 +16,7 @@ > > * http://www.intel.com/technology/serialata/pdf/rev1_1.pdf > > */ > > +#include <linux/bitops.h> > > #include <linux/kernel.h> > > #include <linux/gfp.h> > > #include <linux/module.h> > > @@ -443,16 +444,28 @@ static ssize_t ahci_show_em_supported(struct device *dev, > > void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv) > > { > > void __iomem *mmio = hpriv->mmio; > > - u32 cap, cap2, vers, port_map; > > + void __iomem *port_mmio; > > + unsigned long port_map; > > Why is this an 'unsigned long' now? I thought we could have only 32 ports? > > > + u32 cap, cap2, vers; > > int i; > > /* make sure AHCI mode is enabled before accessing CAP */ > > ahci_enable_ahci(mmio); > > - /* Values prefixed with saved_ are written back to host after > > - * reset. Values without are used for driver operation. > > + /* > > + * Values prefixed with saved_ are written back to the HBA and ports > > + * registers after reset. Values without are used for driver operation. > > + */ > > + > > + /* > > + * Override HW-init HBA capability fields with platform-specific values. > > + * The rest of the HBA capabilities are defined with strictly RO flags > > + * and can't be modified in CSR anyway. > > */ > > - hpriv->saved_cap = cap = readl(mmio + HOST_CAP); > > + cap = readl(mmio + HOST_CAP); > > + if (hpriv->saved_cap) > > + cap = (cap & ~(HOST_CAP_SSS | HOST_CAP_MPS)) | hpriv->saved_cap; > > + hpriv->saved_cap = cap; > > /* CAP2 register is only defined for AHCI 1.2 and later */ > > vers = readl(mmio + HOST_VERSION); > > @@ -519,7 +532,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv) > > /* Override the HBA ports mapping if the platform needs it */ > > port_map = readl(mmio + HOST_PORTS_IMPL); > > And we're still using 'readl', so we will only initialize 32 bits ... > > > if (hpriv->saved_port_map && port_map != hpriv->saved_port_map) { > > Tsk. And now comparing is to a u32 ... > > Please change it back to be 32 bits. Your comment made me thinking I was going crazy. =) After getting this patchset submitted and delivered I've switched to and already finished several more kernel patches series in another subsystems. So the AHCI/SATA context has stashed in my subconscious during that time. Indeed what you are saying seems very much reasonable especially seeing not changing the variable type causes much less code alterations. Anyway the reason of having the port_map variable defined as 'unsigned long' is due to using the for_each_set_bit() macro. It has find_next_bit() called which expects a pointer to unsigned long passed. Needless to say that using 'unsigned long' in the port_map definition is very much required due to that. -Sergey > > Cheers, > > Hannes > -- > Dr. Hannes Reinecke Kernel Storage Architect > hare@suse.de +49 911 74053 688 > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg > HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
On Thu, May 12, 2022 at 09:12:46AM +0200, Hannes Reinecke wrote: > On 5/12/22 01:18, Serge Semin wrote: > > Some DWC AHCI SATA IP-core derivatives require to perform small platform > > or IP-core specific setups. They are too small to be placed in a dedicated > > driver. It's just much easier to have a set of quirks for them right in > > the DWC AHCI driver code. Since we are about to add such platform support, > > as a pre-requisite we introduce a platform-data based DWC AHCI quirks API. > > The platform data can be used to define the flags passed to the > > ahci_platform_get_resources() method, additional AHCI host-flags and a set > > of callbacks to initialize, re-initialize and clear the platform settings. > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > --- > > > > Changelog v2: > > - Change the local objects prefix from 'dwc_ahci_' to 'ahci_dwc_'. > > (@Damien) > > --- > > drivers/ata/ahci_dwc.c | 52 ++++++++++++++++++++++++++++++++++++++---- > > 1 file changed, 48 insertions(+), 4 deletions(-) > > > This really could be merged with patch 19; as you're adding a new driver you > might as well fold this patch in to avoid further modifications. That merely depends on the development approach. I saw and reviewed not one kernel patchsets introducing new drivers and adding features one after another. Moreover there is no kernel patches requirement which would constitutes not to do so. In this particular case the modification isn't complicated at all thus can be reviewed as is. -Sergey > > Cheers, > > Hannes > -- > Dr. Hannes Reinecke Kernel Storage Architect > hare@suse.de +49 911 74053 688 > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg > HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
On Thu, May 12, 2022 at 09:16:19AM +0200, Hannes Reinecke wrote: > On 5/12/22 01:18, Serge Semin wrote: > > Add myself as a maintainer of the new DWC AHCI SATA driver and > > its DT-bindings schema. > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > --- > > > > Changelog v2: > > - Use dlemoal/libata.git git tree for the LIBATA SATA AHCI SYNOPSYS > > DWC driver (@Damien). > > --- > > MAINTAINERS | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 40fa1955ca3f..04f470519708 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -11135,6 +11135,15 @@ F: drivers/ata/ahci_platform.c > > F: drivers/ata/libahci_platform.c > > F: include/linux/ahci_platform.h > > +LIBATA SATA AHCI SYNOPSYS DWC CONTROLLER DRIVER > > +M: Serge Semin <fancer.lancer@gmail.com> > > +L: linux-ide@vger.kernel.org > > +S: Maintained > > +T: git git://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/libata.git > > +F: Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml > > +F: Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml > > +F: drivers/ata/ahci_dwc.c > > + > > LIBATA SATA PROMISE TX2/TX4 CONTROLLER DRIVER > > M: Mikael Pettersson <mikpelinux@gmail.com> > > L: linux-ide@vger.kernel.org > Tsk. > > One would have thought that you'd be using the same email address for > sending patches, and not specifying a different one for the maintainers > file. There is this thing about proof of authenticity and all that ... > > I'm not really comfortable with this. Please use the same email address for > both the Maintainers file and for sending patches. I am more comfortable with reviewing and emailing from my private email box rather than from the corporate one. Moreover the corporate email address tends to get changed much more frequent than the private one while the person role still preserves. Keeping more stable address in the MAINTAINERS file gives a benefit of providing better review service and causes less consequent modifications. So to speak I'd stick with keeping the email address as is here. -Sergey > > Cheers, > > Hannes > -- > Dr. Hannes Reinecke Kernel Storage Architect > hare@suse.de +49 911 74053 688 > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg > HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
On 5/12/22 2:18 AM, Serge Semin wrote: > Currently not all of the Port-specific capabilities listed in the > PORT_CMD-enumeration. Let's extend that set with the Cold Presence > Detection and Mechanical Presence Switch attached to the Port flags [1] so > to closeup the set of the platform-specific port-capabilities flags. Note > these flags are supposed to be set by the platform firmware if there is > one. Alternatively as we are about to do they can be set by means of the > OF properties. > > While at it replace PORT_IRQ_DEV_ILCK with PORT_IRQ_DEV_MPS and fix the Your code has PORT_IRQ_DMPS instead... > comment there. In accordance with [2] that IRQ flag is supposed to > indicate the state of the signal coming from the Mechanical Presence > Switch. > > [1] Serial ATA AHCI 1.3.1 Specification, p.27 > [2] Serial ATA AHCI 1.3.1 Specification, p.7 > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > --- > drivers/ata/ahci.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h > index 7d834deefeb9..f501531bd1b3 100644 > --- a/drivers/ata/ahci.h > +++ b/drivers/ata/ahci.h > @@ -138,7 +138,7 @@ enum { > PORT_IRQ_BAD_PMP = (1 << 23), /* incorrect port multiplier */ > > PORT_IRQ_PHYRDY = (1 << 22), /* PhyRdy changed */ > - PORT_IRQ_DEV_ILCK = (1 << 7), /* device interlock */ > + PORT_IRQ_DMPS = (1 << 7), /* mechanical presence status */ > PORT_IRQ_CONNECT = (1 << 6), /* port connect change status */ > PORT_IRQ_SG_DONE = (1 << 5), /* descriptor processed */ > PORT_IRQ_UNK_FIS = (1 << 4), /* unknown FIS rx'd */ [...] MBR, Sergey
On 2022/05/12 16:26, Serge Semin wrote: > On Thu, May 12, 2022 at 08:32:37AM +0200, Hannes Reinecke wrote: >> On 5/12/22 01:17, Serge Semin wrote: >>> Since all the clocks are retrieved by the method >>> ahci_platform_get_resources() there is no need for the LLD (glue) drivers >>> to be looking for some particular of them in the kernel clocks table >>> again. Instead we suggest to add a simple method returning a >>> device-specific clock with passed connection ID if it is managed to be >>> found. Otherwise the function will return NULL. Thus the glue-drivers >>> won't need to either manually touching the hpriv->clks array or calling >>> clk_get()-friends. The AHCI platform drivers will be able to use the new >>> function right after the ahci_platform_get_resources() method invocation >>> and up to the device removal. >>> >>> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> >>> >>> --- >>> >>> Changelog v2: >>> - Fix some grammar mistakes in the method description. >>> --- >>> drivers/ata/libahci_platform.c | 27 +++++++++++++++++++++++++++ >>> include/linux/ahci_platform.h | 3 +++ >>> 2 files changed, 30 insertions(+) >>> >>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c >>> index 3cff86c225fd..7ff6626fd569 100644 >>> --- a/drivers/ata/libahci_platform.c >>> +++ b/drivers/ata/libahci_platform.c >>> @@ -94,6 +94,33 @@ void ahci_platform_disable_phys(struct ahci_host_priv *hpriv) >>> } >>> EXPORT_SYMBOL_GPL(ahci_platform_disable_phys); >>> +/** >>> + * ahci_platform_find_clk - Find platform clock >>> + * @hpriv: host private area to store config values >>> + * @con_id: clock connection ID >>> + * >>> + * This function returns a pointer to the clock descriptor of the clock with >>> + * the passed ID. >>> + * >>> + * RETURNS: >>> + * Pointer to the clock descriptor on success otherwise NULL >>> + */ >>> +struct clk *ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id) >>> +{ >>> + struct clk *clk = NULL; >>> + int i; >>> + >>> + for (i = 0; i < hpriv->n_clks; i++) { >>> + if (!strcmp(hpriv->clks[i].id, con_id)) { >>> + clk = hpriv->clks[i].clk; >>> + break; >>> + } >>> + } >>> + >>> + return clk; >>> +} >>> +EXPORT_SYMBOL_GPL(ahci_platform_find_clk); >>> + >>> /** >>> * ahci_platform_enable_clks - Enable platform clocks >>> * @hpriv: host private area to store config values >>> diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h >>> index 49e5383d4222..fd964e6a68d6 100644 >>> --- a/include/linux/ahci_platform.h >>> +++ b/include/linux/ahci_platform.h >>> @@ -13,6 +13,7 @@ >>> #include <linux/compiler.h> >>> +struct clk; >>> struct device; >>> struct ata_port_info; >>> struct ahci_host_priv; >>> @@ -21,6 +22,8 @@ struct scsi_host_template; >>> int ahci_platform_enable_phys(struct ahci_host_priv *hpriv); >>> void ahci_platform_disable_phys(struct ahci_host_priv *hpriv); >>> +struct clk * >>> +ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id); >>> int ahci_platform_enable_clks(struct ahci_host_priv *hpriv); >>> void ahci_platform_disable_clks(struct ahci_host_priv *hpriv); >>> int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv); >> > >> Where is this function being used? > > It will be used in the DWC AHCI SATA driver and can be utilized in the > rest of the drivers to simplify the available clocks access. > BTW Damien asked the same question in v1. My response was the same. Please squash this patch together with the patch introducing the first use of this function. > > -Sergey > >> >> Cheers, >> >> Hannes >> -- >> Dr. Hannes Reinecke Kernel Storage Architect >> hare@suse.de +49 911 74053 688 >> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg >> HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
On Fri, May 13, 2022 at 11:22:44AM +0300, Sergei Shtylyov wrote: > On 5/12/22 2:18 AM, Serge Semin wrote: > > > Currently not all of the Port-specific capabilities listed in the > > PORT_CMD-enumeration. Let's extend that set with the Cold Presence > > Detection and Mechanical Presence Switch attached to the Port flags [1] so > > to closeup the set of the platform-specific port-capabilities flags. Note > > these flags are supposed to be set by the platform firmware if there is > > one. Alternatively as we are about to do they can be set by means of the > > OF properties. > > > > While at it replace PORT_IRQ_DEV_ILCK with PORT_IRQ_DEV_MPS and fix the > > Your code has PORT_IRQ_DMPS instead... Right. The correct macro name is used in the patch body. I'll fix the log. Thanks. -Sergey > > > comment there. In accordance with [2] that IRQ flag is supposed to > > indicate the state of the signal coming from the Mechanical Presence > > Switch. > > > > [1] Serial ATA AHCI 1.3.1 Specification, p.27 > > [2] Serial ATA AHCI 1.3.1 Specification, p.7 > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > drivers/ata/ahci.h | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h > > index 7d834deefeb9..f501531bd1b3 100644 > > --- a/drivers/ata/ahci.h > > +++ b/drivers/ata/ahci.h > > @@ -138,7 +138,7 @@ enum { > > PORT_IRQ_BAD_PMP = (1 << 23), /* incorrect port multiplier */ > > > > PORT_IRQ_PHYRDY = (1 << 22), /* PhyRdy changed */ > > - PORT_IRQ_DEV_ILCK = (1 << 7), /* device interlock */ > > + PORT_IRQ_DMPS = (1 << 7), /* mechanical presence status */ > > PORT_IRQ_CONNECT = (1 << 6), /* port connect change status */ > > PORT_IRQ_SG_DONE = (1 << 5), /* descriptor processed */ > > PORT_IRQ_UNK_FIS = (1 << 4), /* unknown FIS rx'd */ > [...] > > MBR, Sergey
On Fri, May 13, 2022 at 11:32:09AM +0200, Damien Le Moal wrote: > On 2022/05/12 16:26, Serge Semin wrote: > > On Thu, May 12, 2022 at 08:32:37AM +0200, Hannes Reinecke wrote: > >> On 5/12/22 01:17, Serge Semin wrote: > >>> Since all the clocks are retrieved by the method > >>> ahci_platform_get_resources() there is no need for the LLD (glue) drivers > >>> to be looking for some particular of them in the kernel clocks table > >>> again. Instead we suggest to add a simple method returning a > >>> device-specific clock with passed connection ID if it is managed to be > >>> found. Otherwise the function will return NULL. Thus the glue-drivers > >>> won't need to either manually touching the hpriv->clks array or calling > >>> clk_get()-friends. The AHCI platform drivers will be able to use the new > >>> function right after the ahci_platform_get_resources() method invocation > >>> and up to the device removal. > >>> > >>> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > >>> > >>> --- > >>> > >>> Changelog v2: > >>> - Fix some grammar mistakes in the method description. > >>> --- > >>> drivers/ata/libahci_platform.c | 27 +++++++++++++++++++++++++++ > >>> include/linux/ahci_platform.h | 3 +++ > >>> 2 files changed, 30 insertions(+) > >>> > >>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > >>> index 3cff86c225fd..7ff6626fd569 100644 > >>> --- a/drivers/ata/libahci_platform.c > >>> +++ b/drivers/ata/libahci_platform.c > >>> @@ -94,6 +94,33 @@ void ahci_platform_disable_phys(struct ahci_host_priv *hpriv) > >>> } > >>> EXPORT_SYMBOL_GPL(ahci_platform_disable_phys); > >>> +/** > >>> + * ahci_platform_find_clk - Find platform clock > >>> + * @hpriv: host private area to store config values > >>> + * @con_id: clock connection ID > >>> + * > >>> + * This function returns a pointer to the clock descriptor of the clock with > >>> + * the passed ID. > >>> + * > >>> + * RETURNS: > >>> + * Pointer to the clock descriptor on success otherwise NULL > >>> + */ > >>> +struct clk *ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id) > >>> +{ > >>> + struct clk *clk = NULL; > >>> + int i; > >>> + > >>> + for (i = 0; i < hpriv->n_clks; i++) { > >>> + if (!strcmp(hpriv->clks[i].id, con_id)) { > >>> + clk = hpriv->clks[i].clk; > >>> + break; > >>> + } > >>> + } > >>> + > >>> + return clk; > >>> +} > >>> +EXPORT_SYMBOL_GPL(ahci_platform_find_clk); > >>> + > >>> /** > >>> * ahci_platform_enable_clks - Enable platform clocks > >>> * @hpriv: host private area to store config values > >>> diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h > >>> index 49e5383d4222..fd964e6a68d6 100644 > >>> --- a/include/linux/ahci_platform.h > >>> +++ b/include/linux/ahci_platform.h > >>> @@ -13,6 +13,7 @@ > >>> #include <linux/compiler.h> > >>> +struct clk; > >>> struct device; > >>> struct ata_port_info; > >>> struct ahci_host_priv; > >>> @@ -21,6 +22,8 @@ struct scsi_host_template; > >>> int ahci_platform_enable_phys(struct ahci_host_priv *hpriv); > >>> void ahci_platform_disable_phys(struct ahci_host_priv *hpriv); > >>> +struct clk * > >>> +ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id); > >>> int ahci_platform_enable_clks(struct ahci_host_priv *hpriv); > >>> void ahci_platform_disable_clks(struct ahci_host_priv *hpriv); > >>> int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv); > >> > > > >> Where is this function being used? > > > > It will be used in the DWC AHCI SATA driver and can be utilized in the > > rest of the drivers to simplify the available clocks access. > > BTW Damien asked the same question in v1. My response was the same. > > Please squash this patch together with the patch introducing the first use of > this function. I don't find this required seeing the changes introduced here are coherent and can be considered as preparation to the corresponding patch. This doesn't break any submitting patch procedure and doesn't complicate the review process but simplifies it. -Sergey > > > > > -Sergey > > > >> > >> Cheers, > >> > >> Hannes > >> -- > >> Dr. Hannes Reinecke Kernel Storage Architect > >> hare@suse.de +49 911 74053 688 > >> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg > >> HRB 36809 (AG Nürnberg), GF: Felix Imendörffer > > > -- > Damien Le Moal > Western Digital Research
The main goal of this patchset was to add Baikal-T1 AHCI SATA specifics support into the kernel AHCI subsystem. On the way of doing that we figured out that mainly these specifics are actually DWC AHCI SATA controller features, but still there were some Baikal-T1 SoC platform peculiarities which we had to take into account. So the patchset introduces two AHCI SATA controllers support and one AHCI SATA driver with a series of preparation, optimization and cleanup patches. The series starts used to start with converting the legacy AHCI SATA controllers text-based DT-bindings to the DT-schema. But turned out that has already been done in kernel v5.17. So instead we suggest to improve the bindings usability by splitting up the AHCI DT bindings into two schemas: one common AHCI SATA controller yaml-file, which can be reused by any AHCI-compatible controller utilizing the kernel AHCI library functions, and DT-bindings for the generic AHCI SATA devices indicated by the "generic-ahci" compatible string and implemented in the ahci_platform.c driver. Note after doing that we had to fix the sata-common.yaml file SATA port IDs constraint. Then a series of generic preparations-cleanups goes. First of all it concerns the device-managed methods usage in the framework of the CSR space remapping and the clocks requesting and enabling. Note since the clocks handlers are requested and kept in the generic AHCI library it seemed a good idea to add an AHCI-platform generic method to find and get a particular clock handler from the pool of the requested ones. It was used later in the series in the DWC/Baikal-T1-specific code. Secondly we suggested to at least sanity check the number of SATA ports DT-sub-nodes before using it further. Thirdly the ports-implemented DT-property parsing was moved from the AHCI platform-driver to the AHCI-library so to be used by the non-generic AHCI drivers if required (DT-schema is accordingly fixed too). Finally due to having the shared-reset control support we had to add a new AHCI-resource getter flag - AHCI_PLATFORM_RST_TRIGGER, which indicated using a trigger-like reset control. For such platforms the controller reset will be performed by means of the reset_control_reset() and reset_control_rearm() methods. AHCI-library reset functions encapsulating the way the reset procedure is performed have been also added. After that goes a patches series with the platform-specific AHCI-capabilities initialization. The suggested functionality will be useful for the platforms with no BIOS, comprehensive bootloader/firmware installed. In that case the AHCI-related platform-specifics like drive staggered spin-up, mechanical presence switch attached or FIS-based switching capability usage, etc will be left uninitialized with no generic way to be indicated as available if required. We suggested to use the AHCI device tree node and its ports sub-nodes for that. AHCI-platform library will be responsible fo the corresponding DT-properties parsing and pre-initialization of the internal capability registers cache, which will be then flashed back to the corresponding CSR after HBA reset. Thus a supposed to be firmware-work will be done by means of the AHCI-library and the DT-data. A set of the preparations/cleanups required to be done before introducing the feature. First the DT-properties indicating the corresponding capability availability were described in the common AHCI DT-binding schema. Second we needed to add the enum items with the AHCI Port CMD fields, which hadn't been added so far. Thirdly we suggested to discard one of the port-map internal storage (force_port_map) in favor of re-using another one (save_port_map) in order to simplify the port-map initialization interface a bit by getting rid from a redundant variable. Finally after discarding the double AHCI-version read procedure and changing the __ahci_port_base() method prototype the platform firmware-specific caps initialization functionality was introduced. The main part of the series goes afterwards. A dedicated DWC AHCI SATA controller driver was introduced together with the corresponding DT-binding schema pre-patch. Note the driver built mode is activated synchronously with the generic AHCI-platform driver by default so automatically to be integrated into the kernel for the DWC AHCI-based platforms which relied on activating the generic AHCI SATA controller driver. Aside with the generic resources getting and AHCI-host initialization, the driver implements the DWC-specific setups. In particular it checks whether the platform capabilities activated by the firmware (see the functionality described above) are actually supported by the controller. It's done by means of the vendor-specific registers. Then it makes sure that the embedded 1ms timer interval, which is used for the DevSleep and CCC features, is correctly initialized based on the application clock rate. The last but not least the driver provides a way to tune the DMA-interface performance up by setting the Tx/Rx transactions maximum size up. The required values are specified by means of the "snps,tx-ts-max" and snps,rx-ts-max" DT-properties. Finally we suggest to extend the DWC AHCI SATA controller driver functionality with a way to add the DWC-AHCI-based platform-specific quirks. Indeed there are many DWC AHCI-based controllers and just a few of them are diverged too much to be handled by a dedicated AHCI-driver. The rest of them most likely can work well either with a generic version of the driver or require a simple normally platform-specific quirk to get up and running. Such platforms can define a platform-data in the DWC AHCI driver with a set of the controller-specific flags and initialization functions. Those functions will be called at the corresponding stages of the device probe/resume/remove procedures so to be performing the platform setups/cleanups. After the denoted above functionality is added we can finally introduce the Baikal-T1 AHCI SATA controller support into the DWC AHCI SATA driver. The controller is based on the DWC AHCI SATA IP-core v4.10a and can work well with the generic DWC AHCI driver. The only peculiarity of it is connected with the SATA Ports reference clock source. It can be supplied either from the internal SoC PLL or from the chip pads. Currently we have to prefer selecting the signal coming from the pads if the corresponding clock source is specified because the link doesn't get stably established when the internal clock signal is activated. In addition the platform has trigger-based reset signals so the corresponding flag must be passed to the generic AHCI-resource getter. Link: https://lore.kernel.org/linux-ide/20220324001628.13028-1-Sergey.Semin@baikalelectronics.ru/ Changelog v2: - Rebase from kernel v5.17 to v5.18-rc3. (@Rob) - Rebase onto the already available AHCI DT schema. As a result two more patches have been added. (@Rob) - Rename 'syscon' property to 'baikal,bt1-syscon'. (@Rob) - Replace min/max constraints of the snps,{tx,rx}-ts-max property with enum [ 1, 2, 4, ..., 1024 ]. (@Rob) - Use dlemoal/libata.git git tree for the LIBATA SATA AHCI SYNOPSYS DWC driver (@Damien). - Change the local objects prefix from 'dwc_ahci_' to 'ahci_dwc_', from 'bt1_ahci_' to 'ahci_bt1_'. (@Damien) - Use LLDD term in place of 'glue-driver'. (@Damien) - Convert the ahci_platform_assert_rsts() method to returning int status (@Damien). - Drop the else word from the DT child_nodes value checking if-else-if statement (@Damien) and convert the after-else part into the ternary operator-based statement. - Convert to checking the error-case first in the devm_clk_bulk_get_all() method invocation. (@Damien) - Drop the rc variable initialization in the ahci_platform_get_resources() method. (@Damien) - Add comma and replace "channel" with "SATA port" in the reg property description of the sata-common.yaml schema. (@Damien) Link: https://lore.kernel.org/lkml/20220503200938.18027-1-Sergey.Semin@baikalelectronics.ru/ Changelog v3: - Replace Jens's email address with Damien's one in the list of the common DT schema maintainers. (@Damien) Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru> Cc: Rob Herring <robh+dt@kernel.org> Cc: linux-ide@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: devicetree@vger.kernel.org Serge Semin (23): dt-bindings: ata: ahci-platform: Drop dma-coherent property declaration dt-bindings: ata: ahci-platform: Detach common AHCI bindings dt-bindings: ata: ahci-platform: Clarify common AHCI props constraints dt-bindings: ata: sata: Extend number of SATA ports ata: libahci_platform: Explicitly set rc on devres_alloc failure ata: libahci_platform: Convert to using platform devm-ioremap methods ata: libahci_platform: Convert to using devm bulk clocks API ata: libahci_platform: Add function returning a clock-handle by id ata: libahci_platform: Sanity check the DT child nodes number ata: libahci_platform: Parse ports-implemented property in resources getter ata: libahci_platform: Introduce reset assertion/deassertion methods dt-bindings: ata: ahci: Add platform capability properties ata: libahci: Extend port-cmd flags set with port capabilities ata: libahci: Discard redundant force_port_map parameter ata: libahci: Don't read AHCI version twice in the save-config method ata: ahci: Convert __ahci_port_base to accepting hpriv as arguments ata: ahci: Introduce firmware-specific caps initialization dt-bindings: ata: ahci: Add DWC AHCI SATA controller DT schema ata: ahci: Add DWC AHCI SATA controller support dt-bindings: ata: ahci: Add Baikal-T1 AHCI SATA controller DT schema ata: ahci-dwc: Add platform-specific quirks support ata: ahci-dwc: Add Baikal-T1 AHCI SATA interface support MAINTAINERS: Add maintainers for DWC AHCI SATA driver .../devicetree/bindings/ata/ahci-common.yaml | 188 +++++++ .../bindings/ata/ahci-platform.yaml | 89 +-- .../bindings/ata/baikal,bt1-ahci.yaml | 127 +++++ .../devicetree/bindings/ata/sata-common.yaml | 7 +- .../bindings/ata/snps,dwc-ahci.yaml | 123 ++++ MAINTAINERS | 9 + drivers/ata/Kconfig | 11 + drivers/ata/Makefile | 1 + drivers/ata/ahci.c | 4 +- drivers/ata/ahci.h | 21 +- drivers/ata/ahci_dwc.c | 526 ++++++++++++++++++ drivers/ata/ahci_mtk.c | 2 - drivers/ata/ahci_platform.c | 5 - drivers/ata/ahci_st.c | 3 - drivers/ata/libahci.c | 63 ++- drivers/ata/libahci_platform.c | 236 ++++++-- include/linux/ahci_platform.h | 8 +- 17 files changed, 1258 insertions(+), 165 deletions(-) create mode 100644 Documentation/devicetree/bindings/ata/ahci-common.yaml create mode 100644 Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml create mode 100644 Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml create mode 100644 drivers/ata/ahci_dwc.c