| Message ID | 1444380426-17953-2-git-send-email-wangxfdu@gmail.com |
|---|---|
| State | Deferred |
| Headers | show |
On Fri, 2015-10-09 at 16:47 +0800, wangxfdu@gmail.com wrote: > From: Xiang Wang <xiang.a.wang@intel.com> > > 1. Support setting hs_hcnt and hs_lcnt > 2. Get bus speed mode from ACPI companion of the > i2c controller. > > Signed-off-by: Xiang Wang <xiang.a.wang@intel.com> > --- > drivers/i2c/busses/i2c-designware-pcidrv.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c > b/drivers/i2c/busses/i2c-designware-pcidrv.c > index 6643d2d..0f4c0c4 100644 > --- a/drivers/i2c/busses/i2c-designware-pcidrv.c > +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c > @@ -54,8 +54,10 @@ enum dw_pci_ctl_id_t { > struct dw_scl_sda_cfg { > u32 ss_hcnt; > u32 fs_hcnt; > + u32 hs_hcnt; > u32 ss_lcnt; > u32 fs_lcnt; > + u32 hs_lcnt; > u32 sda_hold; > }; > > @@ -237,8 +239,10 @@ static int i2c_dw_pci_probe(struct pci_dev > *pdev, > cfg = controller->scl_sda_cfg; > dev->ss_hcnt = cfg->ss_hcnt; > dev->fs_hcnt = cfg->fs_hcnt; > + dev->hs_hcnt = cfg->hs_hcnt; > dev->ss_lcnt = cfg->ss_lcnt; > dev->fs_lcnt = cfg->fs_lcnt; > + dev->hs_lcnt = cfg->hs_lcnt; > dev->sda_hold_time = cfg->sda_hold; > } > > @@ -246,6 +250,9 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, > > dev->tx_fifo_depth = controller->tx_fifo_depth; > dev->rx_fifo_depth = controller->rx_fifo_depth; > + > + i2c_dw_acpi_setup_speed(&pdev->dev, dev); Don't see a relationship between PCI driver and this ACPI stuff. > + > r = i2c_dw_init(dev); > if (r) > return r;
Hi, Andy Thanks for your comments. [Andy] Don't see a relationship between PCI driver and this ACPI stuff. Although this is a pci driver, we may enumerate the i2c devices from DSDT table while i2c controllers are enumerated via PCI. In this scenario, in DSDT, there are descriptions of i2c devices as well as i2c controllers. The ACPI node of i2c controllers are bond to i2c PCI devices via pci-acpi glue. So if we want to determine the i2c devices' settings (e.g. bus speed), we should leverage ACPI. Above is also the reason why the ACPI stuff is put in i2c-designware-core: i2c_dw_acpi_setup_speed can be used by both plat and pci driver. Thanks 2015-10-09 17:31 GMT+08:00 Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > On Fri, 2015-10-09 at 16:47 +0800, wangxfdu@gmail.com wrote: >> From: Xiang Wang <xiang.a.wang@intel.com> >> >> 1. Support setting hs_hcnt and hs_lcnt >> 2. Get bus speed mode from ACPI companion of the >> i2c controller. >> >> Signed-off-by: Xiang Wang <xiang.a.wang@intel.com> >> --- >> drivers/i2c/busses/i2c-designware-pcidrv.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c >> b/drivers/i2c/busses/i2c-designware-pcidrv.c >> index 6643d2d..0f4c0c4 100644 >> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c >> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c >> @@ -54,8 +54,10 @@ enum dw_pci_ctl_id_t { >> struct dw_scl_sda_cfg { >> u32 ss_hcnt; >> u32 fs_hcnt; >> + u32 hs_hcnt; >> u32 ss_lcnt; >> u32 fs_lcnt; >> + u32 hs_lcnt; >> u32 sda_hold; >> }; >> >> @@ -237,8 +239,10 @@ static int i2c_dw_pci_probe(struct pci_dev >> *pdev, >> cfg = controller->scl_sda_cfg; >> dev->ss_hcnt = cfg->ss_hcnt; >> dev->fs_hcnt = cfg->fs_hcnt; >> + dev->hs_hcnt = cfg->hs_hcnt; >> dev->ss_lcnt = cfg->ss_lcnt; >> dev->fs_lcnt = cfg->fs_lcnt; >> + dev->hs_lcnt = cfg->hs_lcnt; >> dev->sda_hold_time = cfg->sda_hold; >> } >> >> @@ -246,6 +250,9 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, >> >> dev->tx_fifo_depth = controller->tx_fifo_depth; >> dev->rx_fifo_depth = controller->rx_fifo_depth; >> + >> + i2c_dw_acpi_setup_speed(&pdev->dev, dev); > > Don't see a relationship between PCI driver and this ACPI stuff. > >> + >> r = i2c_dw_init(dev); >> if (r) >> return r; > > -- > Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Intel Finland Oy
On Mon, 2015-10-12 at 15:41 +0800, Xiang Wang wrote: > Hi, Andy > Thanks for your comments. > > [Andy] Don't see a relationship between PCI driver and this ACPI > stuff. > Although this is a pci driver, we may enumerate the i2c devices from > DSDT table while i2c controllers are enumerated via PCI. In this > scenario, in DSDT, there are descriptions of i2c devices as well as > i2c controllers. The ACPI node of i2c controllers are bond to i2c PCI > devices via pci-acpi glue. > So if we want to determine the i2c devices' settings (e.g. bus > speed), > we should leverage ACPI. > > Above is also the reason why the ACPI stuff is put in > i2c-designware-core: i2c_dw_acpi_setup_speed can be used by both plat > and pci driver. Thanks Wait, first of all let's divide parameters to two groups: a) to be applied to host driver, and b) to be applied to slave devices. The drivers/i2c/busses/i2c-designware* is about host driver parameters. Thus, PCI driver comes with hardcoded values since it's enumerated via PCI, and platform driver utilizes ACPI values. For slave devices everything is done in i2c-core.c. So, what exactly you are trying to enhance? > > 2015-10-09 17:31 GMT+08:00 Andy Shevchenko < > andriy.shevchenko@linux.intel.com>: > > On Fri, 2015-10-09 at 16:47 +0800, wangxfdu@gmail.com wrote: > > > From: Xiang Wang <xiang.a.wang@intel.com> > > > > > > 1. Support setting hs_hcnt and hs_lcnt > > > 2. Get bus speed mode from ACPI companion of the > > > i2c controller. > > > > > > Signed-off-by: Xiang Wang <xiang.a.wang@intel.com> > > > --- > > > drivers/i2c/busses/i2c-designware-pcidrv.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c > > > b/drivers/i2c/busses/i2c-designware-pcidrv.c > > > index 6643d2d..0f4c0c4 100644 > > > --- a/drivers/i2c/busses/i2c-designware-pcidrv.c > > > +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c > > > @@ -54,8 +54,10 @@ enum dw_pci_ctl_id_t { > > > struct dw_scl_sda_cfg { > > > u32 ss_hcnt; > > > u32 fs_hcnt; > > > + u32 hs_hcnt; > > > u32 ss_lcnt; > > > u32 fs_lcnt; > > > + u32 hs_lcnt; > > > u32 sda_hold; > > > }; > > > > > > @@ -237,8 +239,10 @@ static int i2c_dw_pci_probe(struct pci_dev > > > *pdev, > > > cfg = controller->scl_sda_cfg; > > > dev->ss_hcnt = cfg->ss_hcnt; > > > dev->fs_hcnt = cfg->fs_hcnt; > > > + dev->hs_hcnt = cfg->hs_hcnt; > > > dev->ss_lcnt = cfg->ss_lcnt; > > > dev->fs_lcnt = cfg->fs_lcnt; > > > + dev->hs_lcnt = cfg->hs_lcnt; > > > dev->sda_hold_time = cfg->sda_hold; > > > } > > > > > > @@ -246,6 +250,9 @@ static int i2c_dw_pci_probe(struct pci_dev > > > *pdev, > > > > > > dev->tx_fifo_depth = controller->tx_fifo_depth; > > > dev->rx_fifo_depth = controller->rx_fifo_depth; > > > + > > > + i2c_dw_acpi_setup_speed(&pdev->dev, dev); > > > > Don't see a relationship between PCI driver and this ACPI stuff. > > > > > + > > > r = i2c_dw_init(dev); > > > if (r) > > > return r; > > > > -- > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Intel Finland Oy > > >
2015-10-12 16:31 GMT+08:00 Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > On Mon, 2015-10-12 at 15:41 +0800, Xiang Wang wrote: >> Hi, Andy >> Thanks for your comments. >> >> [Andy] Don't see a relationship between PCI driver and this ACPI >> stuff. >> Although this is a pci driver, we may enumerate the i2c devices from >> DSDT table while i2c controllers are enumerated via PCI. In this >> scenario, in DSDT, there are descriptions of i2c devices as well as >> i2c controllers. The ACPI node of i2c controllers are bond to i2c PCI >> devices via pci-acpi glue. >> So if we want to determine the i2c devices' settings (e.g. bus >> speed), >> we should leverage ACPI. >> >> Above is also the reason why the ACPI stuff is put in >> i2c-designware-core: i2c_dw_acpi_setup_speed can be used by both plat >> and pci driver. Thanks > > Wait, first of all let's divide parameters to two groups: a) to be > applied to host driver, and b) to be applied to slave devices. > > The drivers/i2c/busses/i2c-designware* is about host driver parameters. > > Thus, PCI driver comes with hardcoded values since it's enumerated via > PCI, and platform driver utilizes ACPI values. > > For slave devices everything is done in i2c-core.c. > > So, what exactly you are trying to enhance? Agree on your analysis. The bus speed mode is a "host controller" parameter. We can hardcode it in PCI driver. However, 1. "bus speed mode" is a bit different from other parameters. Actually it can be determined by the speed setting of "i2c devices" in ACPI (I2CSerialBus). E.g. If i2c device uses 3MHz, we use High-speed mode for this i2c bus. 2. If we hardcode speed setting in pci driver, we lose the flexibility. A high-speed device may be connected to this i2c bus on this board, but may be connected to another i2c bus on another board design. In this patch, we enumerate the i2c device in ACPI table to determine the frequency setting. Then we set corresponding speed mode for this i2c controller. The ACPI stuff is common for pci and plat driver. If board design changes, we only change BIOS. In conclusion, we have 2 solutions to set the i2c controller speed mode (pci driver): 1) use hardcode value in pci driver 2) use frequency setting of "i2c device" in ACPI table (more flexible, but looks a bit strange) Do you have any preference/suggestions for above solutions? Thanks > >> >> 2015-10-09 17:31 GMT+08:00 Andy Shevchenko < >> andriy.shevchenko@linux.intel.com>: >> > On Fri, 2015-10-09 at 16:47 +0800, wangxfdu@gmail.com wrote: >> > > From: Xiang Wang <xiang.a.wang@intel.com> >> > > >> > > 1. Support setting hs_hcnt and hs_lcnt >> > > 2. Get bus speed mode from ACPI companion of the >> > > i2c controller. >> > > >> > > Signed-off-by: Xiang Wang <xiang.a.wang@intel.com> >> > > --- >> > > drivers/i2c/busses/i2c-designware-pcidrv.c | 7 +++++++ >> > > 1 file changed, 7 insertions(+) >> > > >> > > diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c >> > > b/drivers/i2c/busses/i2c-designware-pcidrv.c >> > > index 6643d2d..0f4c0c4 100644 >> > > --- a/drivers/i2c/busses/i2c-designware-pcidrv.c >> > > +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c >> > > @@ -54,8 +54,10 @@ enum dw_pci_ctl_id_t { >> > > struct dw_scl_sda_cfg { >> > > u32 ss_hcnt; >> > > u32 fs_hcnt; >> > > + u32 hs_hcnt; >> > > u32 ss_lcnt; >> > > u32 fs_lcnt; >> > > + u32 hs_lcnt; >> > > u32 sda_hold; >> > > }; >> > > >> > > @@ -237,8 +239,10 @@ static int i2c_dw_pci_probe(struct pci_dev >> > > *pdev, >> > > cfg = controller->scl_sda_cfg; >> > > dev->ss_hcnt = cfg->ss_hcnt; >> > > dev->fs_hcnt = cfg->fs_hcnt; >> > > + dev->hs_hcnt = cfg->hs_hcnt; >> > > dev->ss_lcnt = cfg->ss_lcnt; >> > > dev->fs_lcnt = cfg->fs_lcnt; >> > > + dev->hs_lcnt = cfg->hs_lcnt; >> > > dev->sda_hold_time = cfg->sda_hold; >> > > } >> > > >> > > @@ -246,6 +250,9 @@ static int i2c_dw_pci_probe(struct pci_dev >> > > *pdev, >> > > >> > > dev->tx_fifo_depth = controller->tx_fifo_depth; >> > > dev->rx_fifo_depth = controller->rx_fifo_depth; >> > > + >> > > + i2c_dw_acpi_setup_speed(&pdev->dev, dev); >> > >> > Don't see a relationship between PCI driver and this ACPI stuff. >> > >> > > + >> > > r = i2c_dw_init(dev); >> > > if (r) >> > > return r; >> > >> > -- >> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> > Intel Finland Oy >> >> >> > > -- > Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Intel Finland Oy
On 10/15/2015 08:46 AM, Xiang Wang wrote: > 1. "bus speed mode" is a bit different from other parameters. Actually > it can be determined by the speed setting of "i2c devices" in ACPI > (I2CSerialBus). E.g. If i2c device uses 3MHz, we use High-speed mode > for this i2c bus. > 2. If we hardcode speed setting in pci driver, we lose the > flexibility. A high-speed device may be connected to this i2c bus on > this board, but may be connected to another i2c bus on another board > design. > > In this patch, we enumerate the i2c device in ACPI table to determine > the frequency setting. Then we set corresponding speed mode for this > i2c controller. The ACPI stuff is common for pci and plat driver. If > board design changes, we only change BIOS. > > In conclusion, we have 2 solutions to set the i2c controller speed > mode (pci driver): > 1) use hardcode value in pci driver > 2) use frequency setting of "i2c device" in ACPI table (more flexible, > but looks a bit strange) > > Do you have any preference/suggestions for above solutions? Thanks I don't think we can hard code especially the high-speed mode because most typically buses are populated with slower devices. Things are a bit more clear when ACPI provides timing parameters for the bus (for standard and fast speed modes at the moment in i2c-designware-platdrv.c: dw_i2c_acpi_configure()) but still I think the ACPI namespace walk may be needed against potential BIOS misconfigurations. For instance if it provides timing parameters for all speeds but there are devices with lower speed on the same bus. I'd take these timing parameters as configuration data for bus features but actual speed (speed bits in IC_CON register) is defined separately. To me it looks only way to achieve that is to pick slowest device from I2cSerialBus resource descriptors.
On Thu, 2015-10-15 at 11:32 +0300, Jarkko Nikula wrote: > On 10/15/2015 08:46 AM, Xiang Wang wrote: > > > > In conclusion, we have 2 solutions to set the i2c controller speed > > mode (pci driver): > > 1) use hardcode value in pci driver > > 2) use frequency setting of "i2c device" in ACPI table (more > > flexible, > > but looks a bit strange) > > > > Do you have any preference/suggestions for above solutions? Thanks > > I don't think we can hard code especially the high-speed mode because > > most typically buses are populated with slower devices. > > Things are a bit more clear when ACPI provides timing parameters for > the > bus (for standard and fast speed modes at the moment in > i2c-designware-platdrv.c: dw_i2c_acpi_configure()) but still I think > the > ACPI namespace walk may be needed against potential BIOS > misconfigurations. For instance if it provides timing parameters for > all > speeds but there are devices with lower speed on the same bus. > > I'd take these timing parameters as configuration data for bus > features > but actual speed (speed bits in IC_CON register) is defined > separately. > To me it looks only way to achieve that is to pick slowest device > from > I2cSerialBus resource descriptors. Should it (ACPI walk) be done in PCI case as well? If so, then it needs to be done up to i2c-core. There you may adjust the bus speed whenever slave device is enumerated. For PCI case you have still to have hardcoded values and it should be maximum supported by the bus I think. When you have implemented above algorithm you may do this safely. Am I missing something?
2015-10-15 17:40 GMT+08:00 Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > On Thu, 2015-10-15 at 11:32 +0300, Jarkko Nikula wrote: >> On 10/15/2015 08:46 AM, Xiang Wang wrote: >> > >> > In conclusion, we have 2 solutions to set the i2c controller speed >> > mode (pci driver): >> > 1) use hardcode value in pci driver >> > 2) use frequency setting of "i2c device" in ACPI table (more >> > flexible, >> > but looks a bit strange) >> > >> > Do you have any preference/suggestions for above solutions? Thanks >> >> I don't think we can hard code especially the high-speed mode because >> >> most typically buses are populated with slower devices. >> >> Things are a bit more clear when ACPI provides timing parameters for >> the >> bus (for standard and fast speed modes at the moment in >> i2c-designware-platdrv.c: dw_i2c_acpi_configure()) but still I think >> the >> ACPI namespace walk may be needed against potential BIOS >> misconfigurations. For instance if it provides timing parameters for >> all >> speeds but there are devices with lower speed on the same bus. >> >> I'd take these timing parameters as configuration data for bus >> features >> but actual speed (speed bits in IC_CON register) is defined >> separately. >> To me it looks only way to achieve that is to pick slowest device >> from >> I2cSerialBus resource descriptors. > > Should it (ACPI walk) be done in PCI case as well? If so, then it needs > to be done up to i2c-core. There you may adjust the bus speed whenever > slave device is enumerated. > I think the "ACPI walk for bus speed" also works for PCI case. It'll be good to do this in i2c-core. By doing this we can get a minimum device speed for this bus. I2C bus drivers can use this speed to set corresponding mode into i2c controller. Waiting for comments from others. > For PCI case you have still to have hardcoded values and it should be > maximum supported by the bus I think. When you have implemented above > algorithm you may do this safely. Am I missing something? Agree. It'll be safer to set a hardcoded maximum supported speed of the bus for PCI case. I2C bus driver can use MIN(speed_get_by_ACPI_walk, hardcoded_max_supported_speed) for bus speed. > > -- > Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Intel Finland Oy
> > Should it (ACPI walk) be done in PCI case as well? If so, then it needs > > to be done up to i2c-core. There you may adjust the bus speed whenever > > slave device is enumerated. > > > I think the "ACPI walk for bus speed" also works for PCI case. It'll > be good to do this in i2c-core. > By doing this we can get a minimum device speed for this bus. I2C bus > drivers can use this speed to set corresponding mode into i2c > controller. > Waiting for comments from others. > > > For PCI case you have still to have hardcoded values and it should be > > maximum supported by the bus I think. When you have implemented above > > algorithm you may do this safely. Am I missing something? > Agree. It'll be safer to set a hardcoded maximum supported speed of > the bus for PCI case. > I2C bus driver can use MIN(speed_get_by_ACPI_walk, > hardcoded_max_supported_speed) for bus speed. Do I read this correctly that you agreed on an alternative path so this series can be discarded?
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c index 6643d2d..0f4c0c4 100644 --- a/drivers/i2c/busses/i2c-designware-pcidrv.c +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c @@ -54,8 +54,10 @@ enum dw_pci_ctl_id_t { struct dw_scl_sda_cfg { u32 ss_hcnt; u32 fs_hcnt; + u32 hs_hcnt; u32 ss_lcnt; u32 fs_lcnt; + u32 hs_lcnt; u32 sda_hold; }; @@ -237,8 +239,10 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, cfg = controller->scl_sda_cfg; dev->ss_hcnt = cfg->ss_hcnt; dev->fs_hcnt = cfg->fs_hcnt; + dev->hs_hcnt = cfg->hs_hcnt; dev->ss_lcnt = cfg->ss_lcnt; dev->fs_lcnt = cfg->fs_lcnt; + dev->hs_lcnt = cfg->hs_lcnt; dev->sda_hold_time = cfg->sda_hold; } @@ -246,6 +250,9 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, dev->tx_fifo_depth = controller->tx_fifo_depth; dev->rx_fifo_depth = controller->rx_fifo_depth; + + i2c_dw_acpi_setup_speed(&pdev->dev, dev); + r = i2c_dw_init(dev); if (r) return r;