| Message ID | 20230530145601.2592-7-Jonathan.Cameron@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: Enabling use of aspeed-i2c with ACPI | expand |
On Tue, May 30, 2023 at 5:59 PM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > Needs tidying up - hopefully can do clock right using ongoing > work from Niyas > https://linaro.atlassian.net/wiki/spaces/CLIENTPC/pages/28832333867/ACPI+Clock+Management I'm wondering how this will be solved for the cases where the "clock-frequency" property is used, see below. > ACPI does not provide an equivalent reset deassert / assert. _RST > doesn't fit that model, so for now make the reset optional. ... > - Left the clock with the hideous hack to keep it obvious that it is > a hack given no way for us to get the clk rate on ACPI firmware yet > and I don't want to pretend there is. The workaround in some cases is to read the "clock-frequency" property, which is standard de facto in several drivers / subsystems. That said, why not split this patch into two and switch the clock to be optional and use the above property? Okay, one thing is that this can collide probably with the generic I2C bus timings, which this driver uses with a non-standard property name.
On Tue, 30 May 2023 22:59:50 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, May 30, 2023 at 5:59 PM Jonathan Cameron > <Jonathan.Cameron@huawei.com> wrote: > > > > Needs tidying up - hopefully can do clock right using ongoing > > work from Niyas > > https://linaro.atlassian.net/wiki/spaces/CLIENTPC/pages/28832333867/ACPI+Clock+Management > > I'm wondering how this will be solved for the cases where the > "clock-frequency" property is used, see below. > > > ACPI does not provide an equivalent reset deassert / assert. _RST > > doesn't fit that model, so for now make the reset optional. > > ... > > > - Left the clock with the hideous hack to keep it obvious that it is > > a hack given no way for us to get the clk rate on ACPI firmware yet > > and I don't want to pretend there is. > > The workaround in some cases is to read the "clock-frequency" > property, which is standard de facto in several drivers / subsystems. That's the wrong clock frequency. I believe that property is the i2c bus clock frequency Documentation/devicetree/bindings/i2c/i2c.txt The one being used here is the input clock. I'd imagine there is some division done, but probably doesn't make sense to represent that using the clock framework as the i2c bus clock signal isn't directly derived from the input clock (pulse stretching and all sorts of other fun in I2C). Also massive overkill given no one else can use this clock. > > That said, why not split this patch into two and switch the clock to > be optional and use the above property? Okay, one thing is that this > can collide probably with the generic I2C bus timings, which this > driver uses with a non-standard property name. > I'd rather provide the clock from ACPI using Niyas' stuff when ready - it looks like a promising general solution so don't want to put an partial solution in before that. I kept these two changes in the last patch as I suspect they are the ones that can be considered a hack, given we don't actually have a real platform using ACPI with this device. Anyone on aspeed know if anyone cares about ACPI on those BMCs? Jonathan
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index ae8e187f2233..f0b5789aee21 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -986,14 +986,14 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev) if (IS_ERR(bus->base)) return PTR_ERR(bus->base); - parent_clk = devm_clk_get(&pdev->dev, NULL); - if (IS_ERR(parent_clk)) - return PTR_ERR(parent_clk); - bus->parent_clk_frequency = clk_get_rate(parent_clk); + // parent_clk = devm_clk_get(&pdev->dev, NULL); + // if (IS_ERR(parent_clk))// + // return PTR_ERR(parent_clk); + bus->parent_clk_frequency = 1000000;//clk_get_rate(parent_clk); /* We just need the clock rate, we don't actually use the clk object. */ - devm_clk_put(&pdev->dev, parent_clk); + //devm_clk_put(&pdev->dev, parent_clk); - bus->rst = devm_reset_control_get_shared(&pdev->dev, NULL); + bus->rst = devm_reset_control_get_optional_shared(&pdev->dev, NULL); if (IS_ERR(bus->rst)) { dev_err(&pdev->dev, "missing or invalid reset controller device tree entry\n");
Needs tidying up - hopefully can do clock right using ongoing work from Niyas https://linaro.atlassian.net/wiki/spaces/CLIENTPC/pages/28832333867/ACPI+Clock+Management ACPI does not provide an equivalent reset deassert / assert. _RST doesn't fit that model, so for now make the reset optional. Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> --- v2: - Use optional variant for the reset. - Left the clock with the hideous hack to keep it obvious that it is a hack given no way for us to get the clk rate on ACPI firmware yet and I don't want to pretend there is. --- drivers/i2c/busses/i2c-aspeed.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)