| Message ID | 1518713524-1500-1-git-send-email-clombard@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V2] cxl: Fix timebase synchronization status on P9 | expand |
Le 15/02/2018 à 17:52, Christophe Lombard a écrit : > The PSL Timebase register is updated by the PSL to maintain the > timebase. > On P9, the Timebase value is only provided by the CAPP as received > the last time a timebase request was performed. > The timebase requests are initiated through the adapter configuration or > application registers. > The specific sysfs entry "/sys/class/cxl/cardxx/psl_timebase_synced" is > now dynamically updated according the content of the PSL Timebase > register. > > Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com> > > --- > Changelog[v2] > - Missing Signed-off-by > - Spaces required around the ':' > --- > drivers/misc/cxl/pci.c | 35 +++++++++++++++++++---------------- > drivers/misc/cxl/sysfs.c | 14 ++++++++++++++ > 2 files changed, 33 insertions(+), 16 deletions(-) > > diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c > index 758842f..270afb5 100644 > --- a/drivers/misc/cxl/pci.c > +++ b/drivers/misc/cxl/pci.c > @@ -612,8 +612,6 @@ static u64 timebase_read_xsl(struct cxl *adapter) > > static void cxl_setup_psl_timebase(struct cxl *adapter, struct pci_dev *dev) > { > - u64 psl_tb; > - int delta; > unsigned int retry = 0; > struct device_node *np; > > @@ -641,20 +639,25 @@ static void cxl_setup_psl_timebase(struct cxl *adapter, struct pci_dev *dev) > cxl_p1_write(adapter, CXL_PSL_Control, 0x0000000000000000); > cxl_p1_write(adapter, CXL_PSL_Control, CXL_PSL_Control_tb); > > - /* Wait until CORE TB and PSL TB difference <= 16usecs */ > - do { > - msleep(1); > - if (retry++ > 5) { > - dev_info(&dev->dev, "PSL timebase can't synchronize\n"); > - return; > - } > - psl_tb = adapter->native->sl_ops->timebase_read(adapter); > - delta = mftb() - psl_tb; > - if (delta < 0) > - delta = -delta; > - } while (tb_to_ns(delta) > 16000); > - > - adapter->psl_timebase_synced = true; > + if (cxl_is_power8()) { > + u64 psl_tb; > + int delta; > + > + /* Wait until CORE TB and PSL TB difference <= 16usecs */ > + do { > + msleep(1); > + if (retry++ > 5) { > + dev_info(&dev->dev, "PSL timebase can't synchronize\n"); > + return; > + } > + psl_tb = adapter->native->sl_ops->timebase_read(adapter); > + delta = mftb() - psl_tb; > + if (delta < 0) > + delta = -delta; > + } while (tb_to_ns(delta) > 16000); > + > + adapter->psl_timebase_synced = true; > + } > return; > } > > diff --git a/drivers/misc/cxl/sysfs.c b/drivers/misc/cxl/sysfs.c > index a8b6d6a..5384c59 100644 > --- a/drivers/misc/cxl/sysfs.c > +++ b/drivers/misc/cxl/sysfs.c > @@ -63,6 +63,20 @@ static ssize_t psl_timebase_synced_show(struct device *device, > { > struct cxl *adapter = to_cxl_adapter(device); > > + /* > + * On P9, the Timebase value is only updated as a result of > + * PSL TimeBase command sent to CAPP. > + */ > + if (cxl_is_power9()) { > + u64 psl_tb; > + int delta; > + > + psl_tb = cxl_p1_read(adapter, CXL_PSL9_Timebase); > + delta = mftb() - psl_tb; > + if (delta < 0) > + delta = -delta; > + adapter->psl_timebase_synced = true ? tb_to_ns(delta) < 16000 : false; I doubt it is what you meant. The syntax is just wrong, though it may actually work ;-) The patch also conflicts with Vaibhav's about write_timebase_ctrl I'm also wondering if it wouldn't be simpler to always update the timebase_synced flag dynamically, even on p8. That way we wouldn't need to have the p8 specific code to check for synchronization in cxl_setup_psl_timebase(). p8 and p9 code would be the same. Fred > + } > return scnprintf(buf, PAGE_SIZE, "%i\n", adapter->psl_timebase_synced); > } >
Frederic Barrat <fbarrat@linux.vnet.ibm.com> writes: > I'm also wondering if it wouldn't be simpler to always update the > timebase_synced flag dynamically, even on p8. That way we wouldn't need > to have the p8 specific code to check for synchronization in > cxl_setup_psl_timebase(). p8 and p9 code would be the same. I am wondering if we can have an implementation like this: static ssize_t psl_timebase_synced_show(struct device *device, { struct cxl *adapter = to_cxl_adapter(device); /* if not already synced than force a resync */ if (!adapter->psl_timebase_synced) /* Choose appropriate PSL implementation */ adapter->native->sl_ops->psl_sync_timebase(adapter); return scnprintf(buf, PAGE_SIZE, "%i\n", adapter->psl_timebase_synced); } In case of PSL9 if timebase request from PSL->CAPP fails then PSL will send an error interrupt. This can be used to set the 'psl_timebase_synced' flag back to false if timebase ever fails.
Frederic Barrat <fbarrat@linux.vnet.ibm.com> writes: > I'm also wondering if it wouldn't be simpler to always update the > timebase_synced flag dynamically, even on p8. That way we wouldn't need > to have the p8 specific code to check for synchronization in > cxl_setup_psl_timebase(). p8 and p9 code would be the same. I am wondering if we can have an implementation like this: static ssize_t psl_timebase_synced_show(struct device *device, { struct cxl *adapter = to_cxl_adapter(device); /* if not already synced than force a resync */ if (!adapter->psl_timebase_synced) /* Choose appropriate PSL implementation */ adapter->native->sl_ops->psl_sync_timebase(adapter); return scnprintf(buf, PAGE_SIZE, "%i\n", adapter->psl_timebase_synced); } In case of PSL9 if timebase request from PSL->CAPP fails then PSL will send an error interrupt. This can be used to set the 'psl_timebase_synced' flag back to false if timebase ever fails.
Le 16/02/2018 à 11:02, Vaibhav Jain a écrit : > Frederic Barrat <fbarrat@linux.vnet.ibm.com> writes: > >> I'm also wondering if it wouldn't be simpler to always update the >> timebase_synced flag dynamically, even on p8. That way we wouldn't need >> to have the p8 specific code to check for synchronization in >> cxl_setup_psl_timebase(). p8 and p9 code would be the same. > > I am wondering if we can have an implementation like this: > > static ssize_t psl_timebase_synced_show(struct device *device, > { > struct cxl *adapter = to_cxl_adapter(device); > > /* if not already synced than force a resync */ > if (!adapter->psl_timebase_synced) > /* Choose appropriate PSL implementation */ > adapter->native->sl_ops->psl_sync_timebase(adapter); > > return scnprintf(buf, PAGE_SIZE, "%i\n", adapter->psl_timebase_synced); > } > > In case of PSL9 if timebase request from PSL->CAPP fails then PSL will > send an error interrupt. This can be used to set the > 'psl_timebase_synced' flag back to false if timebase ever fails. I kind of like recomputing the status with every call. It takes care of outdated status and could be helpful after a reset for example. As to relying on the PSL error interrupt, I wouldn't sweat too much about it. When we get one, the world stops spinning. So the timebase status is the least of our worries. Or did they add something so that we could find out that the PSL error interrupt is *only* due to a timebase sync pb? Note that recomputing it with every read also works in that case and is simpler. Fred
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c index 758842f..270afb5 100644 --- a/drivers/misc/cxl/pci.c +++ b/drivers/misc/cxl/pci.c @@ -612,8 +612,6 @@ static u64 timebase_read_xsl(struct cxl *adapter) static void cxl_setup_psl_timebase(struct cxl *adapter, struct pci_dev *dev) { - u64 psl_tb; - int delta; unsigned int retry = 0; struct device_node *np; @@ -641,20 +639,25 @@ static void cxl_setup_psl_timebase(struct cxl *adapter, struct pci_dev *dev) cxl_p1_write(adapter, CXL_PSL_Control, 0x0000000000000000); cxl_p1_write(adapter, CXL_PSL_Control, CXL_PSL_Control_tb); - /* Wait until CORE TB and PSL TB difference <= 16usecs */ - do { - msleep(1); - if (retry++ > 5) { - dev_info(&dev->dev, "PSL timebase can't synchronize\n"); - return; - } - psl_tb = adapter->native->sl_ops->timebase_read(adapter); - delta = mftb() - psl_tb; - if (delta < 0) - delta = -delta; - } while (tb_to_ns(delta) > 16000); - - adapter->psl_timebase_synced = true; + if (cxl_is_power8()) { + u64 psl_tb; + int delta; + + /* Wait until CORE TB and PSL TB difference <= 16usecs */ + do { + msleep(1); + if (retry++ > 5) { + dev_info(&dev->dev, "PSL timebase can't synchronize\n"); + return; + } + psl_tb = adapter->native->sl_ops->timebase_read(adapter); + delta = mftb() - psl_tb; + if (delta < 0) + delta = -delta; + } while (tb_to_ns(delta) > 16000); + + adapter->psl_timebase_synced = true; + } return; } diff --git a/drivers/misc/cxl/sysfs.c b/drivers/misc/cxl/sysfs.c index a8b6d6a..5384c59 100644 --- a/drivers/misc/cxl/sysfs.c +++ b/drivers/misc/cxl/sysfs.c @@ -63,6 +63,20 @@ static ssize_t psl_timebase_synced_show(struct device *device, { struct cxl *adapter = to_cxl_adapter(device); + /* + * On P9, the Timebase value is only updated as a result of + * PSL TimeBase command sent to CAPP. + */ + if (cxl_is_power9()) { + u64 psl_tb; + int delta; + + psl_tb = cxl_p1_read(adapter, CXL_PSL9_Timebase); + delta = mftb() - psl_tb; + if (delta < 0) + delta = -delta; + adapter->psl_timebase_synced = true ? tb_to_ns(delta) < 16000 : false; + } return scnprintf(buf, PAGE_SIZE, "%i\n", adapter->psl_timebase_synced); }
The PSL Timebase register is updated by the PSL to maintain the timebase. On P9, the Timebase value is only provided by the CAPP as received the last time a timebase request was performed. The timebase requests are initiated through the adapter configuration or application registers. The specific sysfs entry "/sys/class/cxl/cardxx/psl_timebase_synced" is now dynamically updated according the content of the PSL Timebase register. Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com> --- Changelog[v2] - Missing Signed-off-by - Spaces required around the ':' --- drivers/misc/cxl/pci.c | 35 +++++++++++++++++++---------------- drivers/misc/cxl/sysfs.c | 14 ++++++++++++++ 2 files changed, 33 insertions(+), 16 deletions(-)