[PATCH v4 05/13] peci: Add peci-aspeed controller driver

[Winiarska, Iwona <iwona.winiarska@intel.com>]

On Mon, 2021-12-06 at 05:27 +0000, Billy Tsai wrote:
> Hi,
> 
> After reviewing the patch, I have some suggestions for you. Please see the
> reply inline.
> 
> On 2021/11/23, 10:11 PM, "openbmc on behalf of Iwona Winiarska"
> <openbmc-bounces+billy_tsai=aspeedtech.com@lists.ozlabs.org?on behalf of
> iwona.winiarska at intel.com> wrote:
> 
> ??? >?? From: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> Add the define for the bus frequency
> ??????? #define ASPEED_PECI_BUS_FREQUENCY_MIN???????????2000
> ??????? #define ASPEED_PECI_BUS_FREQUENCY_DEFAULT???????2000
> ??????? #define ASPEED_PECI_BUS_FREQUENCY_MAX???????????1000000
> 
> ?? >?? +struct aspeed_peci {
> ??? >?? +???????struct peci_controller *controller;
> ??? >?? +???????struct device *dev;
> ??? >?? +???????void __iomem *base;
> ??? >?? +???????struct clk *clk;
> ??? >?? +???????struct reset_control *rst;
> ??? >?? +???????int irq;
> ??? >?? +???????spinlock_t lock; /* to sync completion status handling */
> ??? >?? +???????struct completion xfer_complete;
> ??? >?? +???????u32 status;
> ??? >?? +???????u32 cmd_timeout_ms;
> ??? >?? +???????u32 msg_timing;
> ??? >?? +???????u32 addr_timing;
> ??? >?? +???????u32 rd_sampling_point;
> ??? >?? +???????u32 clk_div;
> 
> Add the paremeter to store the bus_freq from the dts property
> ??????? u32 bus_freq;
> 
> ??? >?? +};
> ??? >?? +
> ??? >?? +static void aspeed_peci_init_regs(struct aspeed_peci *priv)
> ??? >?? +{
> ??? >?? +???????u32 val;
> ??? >?? +
> ??? >?? +???????val = FIELD_PREP(ASPEED_PECI_CTRL_CLK_DIV_MASK,
> ASPEED_PECI_CLK_DIV_DEFAULT);
> ??? >?? +???????val |= ASPEED_PECI_CTRL_PECI_CLK_EN;
> ??? >?? +???????writel(val, priv->base + ASPEED_PECI_CTRL);
> ??? >?? +???????/*
> ??? >?? +??????? * Timing negotiation period setting.
> ??? >?? +??????? * The unit of the programmed value is 4 times of PECI clock
> period.
> ??? >?? +??????? */
> ??? >?? +???????val = FIELD_PREP(ASPEED_PECI_T_NEGO_MSG_MASK, priv-
> >msg_timing);
> ??? >?? +???????val |= FIELD_PREP(ASPEED_PECI_T_NEGO_ADDR_MASK, priv-
> >addr_timing);
> ??? >?? +???????writel(val, priv->base + ASPEED_PECI_TIMING_NEGOTIATION);
> ??? >?? +
> ??? >?? +???????/* Clear interrupts */
> ??? >?? +???????val = readl(priv->base + ASPEED_PECI_INT_STS) |
> ASPEED_PECI_INT_MASK;
> ??? >?? +???????writel(val, priv->base + ASPEED_PECI_INT_STS);
> ??? >?? +
> ??? >?? +???????/* Set timing negotiation mode and enable interrupts */
> ??? >?? +???????val = FIELD_PREP(ASPEED_PECI_TIMING_NEGO_SEL_MASK,
> ASPEED_PECI_1ST_BIT_OF_ADDR_NEGO);
> ??? >?? +???????val |= ASPEED_PECI_INT_MASK;
> ??? >?? +???????writel(val, priv->base + ASPEED_PECI_INT_CTRL);
> ??? >?? +
> ??? >?? +???????val = FIELD_PREP(ASPEED_PECI_CTRL_SAMPLING_MASK, priv-
> >rd_sampling_point);
> ??? >?? +???????val |= FIELD_PREP(ASPEED_PECI_CTRL_CLK_DIV_MASK, priv-
> >clk_div);
> ??? >?? +???????val |= ASPEED_PECI_CTRL_PECI_EN;
> ??? >?? +???????val |= ASPEED_PECI_CTRL_PECI_CLK_EN;
> ??? >?? +???????writel(val, priv->base + ASPEED_PECI_CTRL);
> ??? >?? +}
> ??? >?? +
> ??? >?? +static inline int aspeed_peci_check_idle(struct aspeed_peci *priv)
> ??? >?? +{
> ??? >?? +???????u32 cmd_sts = readl(priv->base + ASPEED_PECI_CMD);
> ??? >?? +
> ??? >?? +???????if (FIELD_GET(ASPEED_PECI_CMD_STS_MASK, cmd_sts) ==
> ASPEED_PECI_CMD_STS_ADDR_T_NEGO)
> ??? >?? +???????????????aspeed_peci_init_regs(priv);
> 
> As long as the state is not idle, the controller need to reinitialize the PECI
> register for solving the PECI controller hang.
> Before ast2600A3, the controller should assert/de-assert the reset for solving
> the hang issue.
> For backward compatible, I suggest to add the reset assert/de-assert in this
> place.
> ??????? if (FIELD_GET(ASPEED_PECI_CMD_STS_MASK, cmd_sts)) {
> ??????????????????????? reset_control_assert(priv->rst);
> ??????????????????????? reset_control_deassert(priv->rst);
> ??????????????????????? aspeed_peci_init_regs(priv);
> ??????? }

Ack.

> 

(...)

> ??? >?? +static void aspeed_peci_property_setup(struct aspeed_peci *priv)
> ??? >?? +{
> ??? >?? +???????aspeed_peci_property_sanitize(priv->dev, "aspeed,clock-
> divider",
> ??? >?? +???????????????????????????????????? 0, ASPEED_PECI_CLK_DIV_MAX,
> ??? >?? +???????????????????????????????????? ASPEED_PECI_CLK_DIV_DEFAULT,
> &priv->clk_div);
> ??? >?? +???????aspeed_peci_property_sanitize(priv->dev, "aspeed,msg-timing",
> ??? >?? +???????????????????????????????????? 0, ASPEED_PECI_MSG_TIMING_MAX,
> ??? >?? +???????????????????????????????????? ASPEED_PECI_MSG_TIMING_DEFAULT,
> &priv->msg_timing);
> ??? >?? +???????aspeed_peci_property_sanitize(priv->dev, "aspeed,addr-timing",
> ??? >?? +???????????????????????????????????? 0, ASPEED_PECI_ADDR_TIMING_MAX,
> ??? >?? +???????????????????????????????????? ASPEED_PECI_ADDR_TIMING_DEFAULT,
> &priv->addr_timing);
> 
> Use the bus-frequency to replace the property above:
> ??????? aspeed_peci_property_sanitize(priv->dev, "bus-frequency",
> ????????????????????????????????ASPEED_PECI_BUS_FREQUENCY_MIN,
> ASPEED_PECI_BUS_FREQUENCY_MAX,
> ????????????????????????????????ASPEED_PECI_BUS_FREQUENCY_DEFAULT, &priv-
> >bus_freq);
> 
> ??? >?? +???????aspeed_peci_property_sanitize(priv->dev, "aspeed,rd-sampling-
> point",
> ??? >?? +???????????????????????????????????? 0,
> ASPEED_PECI_RD_SAMPLING_POINT_MAX,
> ??? >?? +????????????????????????????????????
> ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT,
> ??? >?? +???????????????????????????????????? &priv->rd_sampling_point);
> ??? >?? +???????aspeed_peci_property_sanitize(priv->dev, "cmd-timeout-ms",
> ??? >?? +???????????????????????????????????? 1,
> ASPEED_PECI_CMD_TIMEOUT_MS_MAX,
> ??? >?? +????????????????????????????????????
> ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT, &priv->cmd_timeout_ms);
> ??? >?? +}
> Add the function of automatically calculating the settings to adapt to the bus
> frequency.

If we want to go with exposing "bus-frequency", instead of hardware properties,
I don't think we should opencode it in this way.
Rather, we should expose it as a clock, passing "clock-frequency" property via
DTS and exposing it using struct clk_hw.
We would then just call clock API (clk_round_rate / clk_set_rate etc),
presenting this as what it actually is (a "custom" clock divider).

Thanks
-Iwona

> ??????? static void aspeed_peci_bus_freq_setup(struct aspeed_peci *priv)
> ??????? {
> ??????????????? unsigned long src_clk_rate;
> ??????????????? int delta_value, delta_tmp, clk_divisor, clk_divisor_tmp;
> ??????????????? u32 msg_timing, clk_div;
> 
> ??????????????? src_clk_rate = clk_get_rate(priv->clk);
> 
> ??????????????? /*
> ???????????????? * PECI bus freq? = (source clk rate) / (4 *
> (PECI04[15:8]*4+1) * (1 << PECI00[10:8]))
> ???????????????? * (1 << PECI00[10:8]) * (PECI04[15:8]*4+1) = (source clk
> rate) / (4 * PECI bus freq )
> ???????????????? */
> ??????????????? clk_divisor = src_clk_rate / (4 * priv->bus_freq);
> ??????????????? delta_value = clk_divisor;
> ??????????????? /* Find the closest divisor for bus-frequency */
> ??????????????? for (msg_timing = 1; msg_timing <= 255; msg_timing++)
> ??????????????????????? for (clk_div = 0; clk_div < 7;
> ??????????????????????????????? clk_div++) {
> ??????????????????????????????? clk_divisor_tmp = (1 << clk_div) *
> ??????????????????????????????????????????????? (msg_timing * 4 + 1);
> ??????????????????????????????? delta_tmp = abs(clk_divisor -
> clk_divisor_tmp);
> ??????????????????????????????? if (delta_tmp < delta_value) {
> ??????????????????????????????????????? delta_value = delta_tmp;
> ??????????????????????????????????????? priv->msg_timing = msg_timing;
> ??????????????????????????????????????? priv->addr_timing = priv->msg_timing;
> ??????????????????????????????????????? priv->clk_div = clk_div;
> ??????????????????????????????? }
> ??????????????????????? }
> ??????? }
> 
> ??? >?? +
> ??? >?? +static struct peci_controller_ops aspeed_ops = {
> ??? >?? +???????.xfer = aspeed_peci_xfer,
> ??? >?? +};
> ??? >?? +
> ??? >?? +static void aspeed_peci_reset_control_release(void *data)
> ??? >?? +{
> ??? >?? +???????reset_control_assert(data);
> ??? >?? +}
> ??? >?? +
> ??? >?? +static int devm_aspeed_peci_reset_control_deassert(struct device
> *dev, struct reset_control *rst)
> ??? >?? +{
> ??? >?? +???????int ret;
> ??? >?? +
> ??? >?? +???????ret = reset_control_deassert(rst);
> ??? >?? +???????if (ret)
> ??? >?? +???????????????return ret;
> ??? >?? +
> ??? >?? +???????return devm_add_action_or_reset(dev,
> aspeed_peci_reset_control_release, rst);
> ??? >?? +}
> ??? >?? +
> ??? >?? +static void aspeed_peci_clk_release(void *data)
> ??? >?? +{
> ??? >?? +???????clk_disable_unprepare(data);
> ??? >?? +}
> ??? >?? +
> ??? >?? +static int devm_aspeed_peci_clk_enable(struct device *dev, struct clk
> *clk)
> ??? >?? +{
> ??? >?? +???????int ret;
> ??? >?? +
> ??? >?? +???????ret = clk_prepare_enable(clk);
> ??? >?? +???????if (ret)
> ??? >?? +???????????????return ret;
> ??? >?? +
> ??? >?? +???????return devm_add_action_or_reset(dev, aspeed_peci_clk_release,
> clk);
> ??? >?? +}
> ??? >?? +
> ??? >?? +static int aspeed_peci_probe(struct platform_device *pdev)
> ??? >?? +{
> ??? >?? +???????struct peci_controller *controller;
> ??? >?? +???????struct aspeed_peci *priv;
> ??? >?? +???????int ret;
> ??? >?? +
> ??? >?? +???????priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> ??? >?? +???????if (!priv)
> ??? >?? +???????????????return -ENOMEM;
> ??? >?? +
> ??? >?? +???????priv->dev = &pdev->dev;
> ??? >?? +???????dev_set_drvdata(priv->dev, priv);
> ??? >?? +
> ??? >?? +???????priv->base = devm_platform_ioremap_resource(pdev, 0);
> ??? >?? +???????if (IS_ERR(priv->base))
> ??? >?? +???????????????return PTR_ERR(priv->base);
> ??? >?? +
> ??? >?? +???????priv->irq = platform_get_irq(pdev, 0);
> ??? >?? +???????if (!priv->irq)
> ??? >?? +???????????????return priv->irq;
> ??? >?? +
> ??? >?? +???????ret = devm_request_irq(&pdev->dev, priv->irq,
> aspeed_peci_irq_handler,
> ??? >?? +????????????????????????????? 0, "peci-aspeed", priv);
> ??? >?? +???????if (ret)
> ??? >?? +???????????????return ret;
> ??? >?? +
> ??? >?? +???????init_completion(&priv->xfer_complete);
> ??? >?? +???????spin_lock_init(&priv->lock);
> ??? >?? +
> ??? >?? +???????priv->rst = devm_reset_control_get(&pdev->dev, NULL);
> ??? >?? +???????if (IS_ERR(priv->rst))
> ??? >?? +???????????????return dev_err_probe(priv->dev, PTR_ERR(priv->rst),
> ??? >?? +??????????????????????????????????? "failed to get reset control\n");
> ??? >?? +
> ??? >?? +???????ret = devm_aspeed_peci_reset_control_deassert(priv->dev, priv-
> >rst);
> ??? >?? +???????if (ret)
> ??? >?? +???????????????return dev_err_probe(priv->dev, ret, "cannot deassert
> reset control\n");
> ??? >?? +
> ??? >?? +???????priv->clk = devm_clk_get(priv->dev, NULL);
> ??? >?? +???????if (IS_ERR(priv->clk))
> ??? >?? +???????????????return dev_err_probe(priv->dev, PTR_ERR(priv->clk),
> "failed to get clk\n");
> ??? >?? +
> ??? >?? +???????ret = devm_aspeed_peci_clk_enable(priv->dev, priv->clk);
> ??? >?? +???????if (ret)
> ??? >?? +???????????????return dev_err_probe(priv->dev, ret, "failed to enable
> clock\n");
> ??? >?? +
> ??? >?? +???????aspeed_peci_property_setup(priv);
> 
> ??????? aspeed_peci_bus_freq_setup(priv);
> 
> ??? >?? +
> ??? >?? +???????aspeed_peci_init_regs(priv);
> ??? >?? +
> ??? >?? +???????controller = devm_peci_controller_add(priv->dev, &aspeed_ops);
> ??? >?? +???????if (IS_ERR(controller))
> ??? >?? +???????????????return dev_err_probe(priv->dev, PTR_ERR(controller),
> ??? >?? +??????????????????????????????????? "failed to add aspeed peci
> controller\n");
> ??? >?? +
> ??? >?? +???????priv->controller = controller;
> ??? >?? +
> ??? >?? +???????return 0;
> ??? >?? +}
> ??? >?? +
> ??? >?? +static const struct of_device_id aspeed_peci_of_table[] = {
> ??? >?? +???????{ .compatible = "aspeed,ast2400-peci", },
> ??? >?? +???????{ .compatible = "aspeed,ast2500-peci", },
> ??? >?? +???????{ .compatible = "aspeed,ast2600-peci", },
> ??? >?? +???????{ }
> ??? >?? +};
> ??? >?? +MODULE_DEVICE_TABLE(of, aspeed_peci_of_table);
> ??? >?? +
> ??? >?? +static struct platform_driver aspeed_peci_driver = {
> ??? >?? +???????.probe? = aspeed_peci_probe,
> ??? >?? +???????.driver = {
> ??? >?? +???????????????.name?????????? = "peci-aspeed",
> ??? >?? +???????????????.of_match_table = aspeed_peci_of_table,
> ??? >?? +???????},
> ??? >?? +};
> ??? >?? +module_platform_driver(aspeed_peci_driver);
> ??? >?? +
> ??? >?? +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
> ??? >?? +MODULE_AUTHOR("Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>");
> ??? >?? +MODULE_DESCRIPTION("ASPEED PECI driver");
> ??? >?? +MODULE_LICENSE("GPL");
> ??? >?? +MODULE_IMPORT_NS(PECI);
> ??? >?? -- 
> ??? >?? 2.31.1
> 
> 
>