| Message ID | 20220511231810.4928-18-Sergey.Semin@baikalelectronics.ru |
|---|---|
| State | New |
| Headers | show |
| Series | ata: ahci: Add DWC/Baikal-T1 AHCI SATA support | expand |
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 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
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; + 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); if (hpriv->saved_port_map && port_map != hpriv->saved_port_map) { - dev_info(dev, "forcing port_map 0x%x -> 0x%x\n", + dev_info(dev, "forcing port_map 0x%lx -> 0x%x\n", port_map, hpriv->saved_port_map); port_map = hpriv->saved_port_map; } else { @@ -527,7 +540,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv) } if (hpriv->mask_port_map) { - dev_warn(dev, "masking port_map 0x%x -> 0x%x\n", + dev_warn(dev, "masking port_map 0x%lx -> 0x%lx\n", port_map, port_map & hpriv->mask_port_map); port_map &= hpriv->mask_port_map; @@ -546,7 +559,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv) */ if (map_ports > ahci_nr_ports(cap)) { dev_warn(dev, - "implemented port map (0x%x) contains more ports than nr_ports (%u), using nr_ports\n", + "implemented port map (0x%lx) contains more ports than nr_ports (%u), using nr_ports\n", port_map, ahci_nr_ports(cap)); port_map = 0; } @@ -555,12 +568,26 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv) /* fabricate port_map from cap.nr_ports for < AHCI 1.3 */ if (!port_map && vers < 0x10300) { port_map = (1 << ahci_nr_ports(cap)) - 1; - dev_warn(dev, "forcing PORTS_IMPL to 0x%x\n", port_map); + dev_warn(dev, "forcing PORTS_IMPL to 0x%lx\n", port_map); /* write the fixed up value to the PI register */ hpriv->saved_port_map = port_map; } + /* + * Preserve the ports capabilities defined by the platform. Note there + * is no need in storing the rest of the P#.CMD fields since they are + * volatile. + */ + for_each_set_bit(i, &port_map, AHCI_MAX_PORTS) { + if (hpriv->saved_port_cap[i]) + continue; + + port_mmio = __ahci_port_base(hpriv, i); + hpriv->saved_port_cap[i] = + readl(port_mmio + PORT_CMD) & PORT_CMD_CAP; + } + /* record values to use during operation */ hpriv->cap = cap; hpriv->cap2 = cap2; @@ -590,13 +617,21 @@ EXPORT_SYMBOL_GPL(ahci_save_initial_config); static void ahci_restore_initial_config(struct ata_host *host) { struct ahci_host_priv *hpriv = host->private_data; + unsigned long port_map = hpriv->port_map; void __iomem *mmio = hpriv->mmio; + void __iomem *port_mmio; + int i; writel(hpriv->saved_cap, mmio + HOST_CAP); if (hpriv->saved_cap2) writel(hpriv->saved_cap2, mmio + HOST_CAP2); writel(hpriv->saved_port_map, mmio + HOST_PORTS_IMPL); (void) readl(mmio + HOST_PORTS_IMPL); /* flush */ + + for_each_set_bit(i, &port_map, AHCI_MAX_PORTS) { + port_mmio = __ahci_port_base(hpriv, i); + writel(hpriv->saved_port_cap[i], port_mmio + PORT_CMD); + } } static unsigned ahci_scr_offset(struct ata_port *ap, unsigned int sc_reg) diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c index 56aced7a76cd..dece57364a3d 100644 --- a/drivers/ata/libahci_platform.c +++ b/drivers/ata/libahci_platform.c @@ -23,6 +23,7 @@ #include <linux/pm_runtime.h> #include <linux/of_platform.h> #include <linux/reset.h> + #include "ahci.h" static void ahci_host_stop(struct ata_host *host); @@ -410,6 +411,44 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port, return rc; } +static int ahci_platform_get_firmware(struct ahci_host_priv *hpriv, + struct device *dev) +{ + struct device_node *child; + u32 port; + + of_property_read_u32(dev->of_node, + "ports-implemented", &hpriv->saved_port_map); + + if (of_property_read_bool(dev->of_node, "hba-sss")) + hpriv->saved_cap |= HOST_CAP_SSS; + if (of_property_read_bool(dev->of_node, "hba-smps")) + hpriv->saved_cap |= HOST_CAP_MPS; + + for_each_child_of_node(dev->of_node, child) { + if (!of_device_is_available(child)) + continue; + + if (of_property_read_u32(child, "reg", &port)) { + of_node_put(child); + return -EINVAL; + } + + if (of_property_read_bool(child, "hba-hpcp")) + hpriv->saved_port_cap[port] |= PORT_CMD_HPCP; + if (of_property_read_bool(child, "hba-mpsp")) + hpriv->saved_port_cap[port] |= PORT_CMD_MPSP; + if (of_property_read_bool(child, "hba-cpd")) + hpriv->saved_port_cap[port] |= PORT_CMD_CPD; + if (of_property_read_bool(child, "hba-esp")) + hpriv->saved_port_cap[port] |= PORT_CMD_ESP; + if (of_property_read_bool(child, "hba-fbscp")) + hpriv->saved_port_cap[port] |= PORT_CMD_FBSCP; + } + + return 0; +} + /** * ahci_platform_get_resources - Get platform resources * @pdev: platform device to get resources for @@ -552,9 +591,6 @@ 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->saved_port_map); - if (child_nodes) { for_each_child_of_node(dev->of_node, child) { u32 port; @@ -619,6 +655,15 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, if (rc == -EPROBE_DEFER) goto err_out; } + + /* + * Retrieve firmware-specific flags which then will be used to set + * the HW-init fields of HBA and its ports + */ + rc = ahci_platform_get_firmware(hpriv, dev); + if (rc) + goto err_out; + pm_runtime_enable(dev); pm_runtime_get_sync(dev); hpriv->got_runtime_pm = true;
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(-)