| Message ID | 20220315032411.2826-1-leilk.liu@mediatek.com |
|---|---|
| Headers | show |
| Series | spi: mediatek: add single/quad mode support | expand |
On Tue, 2022-03-15 at 10:37 +0100, AngeloGioacchino Del Regno wrote: > Il 15/03/22 04:24, Leilk Liu ha scritto: > > this patch adds hclk support. > > > > Signed-off-by: Leilk Liu <leilk.liu@mediatek.com> > > --- > > drivers/spi/spi-mt65xx.c | 85 ++++++++++++++++++++++++++++++++--- > > ----- > > 1 file changed, 69 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c > > index 8958c3fa4fea..d4a602e78aa7 100644 > > --- a/drivers/spi/spi-mt65xx.c > > +++ b/drivers/spi/spi-mt65xx.c > > @@ -130,7 +130,7 @@ struct mtk_spi { > > u32 state; > > int pad_num; > > u32 *pad_sel; > > - struct clk *parent_clk, *sel_clk, *spi_clk; > > + struct clk *parent_clk, *sel_clk, *spi_clk, *spi_hclk; > > struct spi_transfer *cur_transfer; > > u32 xfer_len; > > u32 num_xfered; > > @@ -1252,25 +1252,38 @@ static int mtk_spi_probe(struct > > platform_device *pdev) > > goto err_put_master; > > } > > > > + mdata->spi_hclk = devm_clk_get(&pdev->dev, "hclk"); > > + if (!IS_ERR(mdata->spi_hclk)) { > > What you're doing here can be simplified by using > devm_clk_get_optional() instead. > Please use that. Hi AngeloGioacchino, thanks for your suggestion, I'll fix it. > > > + ret = clk_prepare_enable(mdata->spi_hclk); > > + if (ret < 0) { > > + dev_err(&pdev->dev, "failed to enable hclk > > (%d)\n", ret); > > + goto err_put_master; > > + } > > + } > > + > > ret = clk_prepare_enable(mdata->spi_clk); > > if (ret < 0) { > > dev_err(&pdev->dev, "failed to enable spi_clk (%d)\n", > > ret); > > - goto err_put_master; > > + goto err_disable_spi_hclk; > > } > > > > ret = clk_set_parent(mdata->sel_clk, mdata->parent_clk); > > if (ret < 0) { > > dev_err(&pdev->dev, "failed to clk_set_parent (%d)\n", > > ret); > > - clk_disable_unprepare(mdata->spi_clk); > > - goto err_put_master; > > + goto err_disable_spi_clk; > > } > > > > mdata->spi_clk_hz = clk_get_rate(mdata->spi_clk); > > > > - if (mdata->dev_comp->no_need_unprepare) > > + if (mdata->dev_comp->no_need_unprepare) { > > clk_disable(mdata->spi_clk); > > - else > > + if (!IS_ERR(mdata->spi_hclk)) > > + clk_disable(mdata->spi_hclk); > > + } else { > > clk_disable_unprepare(mdata->spi_clk); > > + if (!IS_ERR(mdata->spi_hclk)) > > + clk_disable_unprepare(mdata->spi_hclk); > > + } > > > > pm_runtime_enable(&pdev->dev); > > > > @@ -1310,6 +1323,11 @@ static int mtk_spi_probe(struct > > platform_device *pdev) > > > > err_disable_runtime_pm: > > pm_runtime_disable(&pdev->dev); > > +err_disable_spi_clk: > > + clk_disable_unprepare(mdata->spi_clk); > > +err_disable_spi_hclk: > > + if (!IS_ERR(mdata->spi_hclk)) > > When using devm_clk_get_optional(), you can simply omit this check, > since it will > be impossible to reach this point with an error pointer stored in > spi_hclk. OK, I'll fix it. > > > + clk_disable_unprepare(mdata->spi_hclk); > > err_put_master: > > spi_master_put(master); > > > > @@ -1325,8 +1343,11 @@ static int mtk_spi_remove(struct > > platform_device *pdev) > > > > mtk_spi_reset(mdata); > > > > - if (mdata->dev_comp->no_need_unprepare) > > + if (mdata->dev_comp->no_need_unprepare) { > > clk_unprepare(mdata->spi_clk); > > + if (!IS_ERR(mdata->spi_hclk)) > > + clk_unprepare(mdata->spi_hclk); > > + } > > > > return 0; > > } > > @@ -1342,8 +1363,11 @@ static int mtk_spi_suspend(struct device > > *dev) > > if (ret) > > return ret; > > > > - if (!pm_runtime_suspended(dev)) > > + if (!pm_runtime_suspended(dev)) { > > clk_disable_unprepare(mdata->spi_clk); > > + if (!IS_ERR(mdata->spi_hclk)) > > + clk_disable_unprepare(mdata->spi_hclk); > > + } > > > > return ret; > > } > > @@ -1360,11 +1384,23 @@ static int mtk_spi_resume(struct device > > *dev) > > dev_err(dev, "failed to enable spi_clk (%d)\n", > > ret); > > return ret; > > } > > + > > + if (!IS_ERR(mdata->spi_hclk)) { > > Since you will be using devm_clk_get_optional(), you can also omit > this check. OK, I'll fix it. > > > + clk_prepare_enable(mdata->spi_hclk); > > There's a typo. ret = clk_prepare_enable.... OK, I'll fix it. > > > + if (ret < 0) { > > + dev_err(dev, "failed to enable spi_hclk > > (%d)\n", ret); > > + clk_disable_unprepare(mdata->spi_clk); > > + return ret; > > + } > > + } > > } > > > > ret = spi_master_resume(master); > > - if (ret < 0) > > + if (ret < 0) { > > clk_disable_unprepare(mdata->spi_clk); > > + if (!IS_ERR(mdata->spi_hclk)) > > Same here and everywhere else, no error check if you set this as > optional clock. OK, I'll fix it. > > > + clk_disable_unprepare(mdata->spi_hclk); > > + } > > > > return ret; > > } > > @@ -1376,10 +1412,15 @@ static int mtk_spi_runtime_suspend(struct > > device *dev) > > struct spi_master *master = dev_get_drvdata(dev); > > struct mtk_spi *mdata = spi_master_get_devdata(master); > > > > - if (mdata->dev_comp->no_need_unprepare) > > + if (mdata->dev_comp->no_need_unprepare) { > > clk_disable(mdata->spi_clk); > > - else > > + if (!IS_ERR(mdata->spi_hclk)) > > + clk_disable(mdata->spi_hclk); > > + } else { > > clk_disable_unprepare(mdata->spi_clk); > > + if (!IS_ERR(mdata->spi_hclk)) > > + clk_disable_unprepare(mdata->spi_hclk); > > + } > > > > return 0; > > } > > @@ -1390,13 +1431,25 @@ static int mtk_spi_runtime_resume(struct > > device *dev) > > struct mtk_spi *mdata = spi_master_get_devdata(master); > > int ret; > > > > - if (mdata->dev_comp->no_need_unprepare) > > + if (mdata->dev_comp->no_need_unprepare) { > > ret = clk_enable(mdata->spi_clk); > > - else > > + if (!IS_ERR(mdata->spi_hclk)) > > + clk_enable(mdata->spi_hclk); > > + } else { > > ret = clk_prepare_enable(mdata->spi_clk); > > - if (ret < 0) { > > - dev_err(dev, "failed to enable spi_clk (%d)\n", ret); > > - return ret; > > + if (ret < 0) { > > + dev_err(dev, "failed to enable spi_clk (%d)\n", > > ret); > > + return ret; > > + } > > + > > + if (!IS_ERR(mdata->spi_hclk)) { > > + ret = clk_prepare_enable(mdata->spi_hclk); > > + if (ret < 0) { > > + dev_err(dev, "failed to enable spi_hclk > > (%d)\n", ret); > > + clk_disable_unprepare(mdata->spi_clk); > > + return ret; > > + } > > + } > > } > > > > return 0; > > > > > Regards, > Angelo > >
On Tue, 2022-03-15 at 10:31 +0100, AngeloGioacchino Del Regno wrote: > Il 15/03/22 04:24, Leilk Liu ha scritto: > > this patch add the support of spi-mem for ipm design. > > > > Signed-off-by: Leilk Liu <leilk.liu@mediatek.com> > > --- > > drivers/spi/spi-mt65xx.c | 349 > > ++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 348 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c > > index 1a0b3208dfca..8958c3fa4fea 100644 > > --- a/drivers/spi/spi-mt65xx.c > > +++ b/drivers/spi/spi-mt65xx.c > > ...snip... > > > + > > +static void of_mtk_spi_parse_dt(struct spi_master *master, struct > > device_node *nc) > > +{ > > + struct mtk_spi *mdata = spi_master_get_devdata(master); > > + u32 value; > > + > > + if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) { > > Hello Leilk, > > thanks for considering my advice about "spi-{tx,rx}-bus-width", but > there's > something that you have misunderstood about it. > > Simply, you don't need this function at all. Whatever you are doing > here is > already being performed in the Linux SPI framework: at the end of the > probe > function, this driver is calling the (legacy) > devm_spi_register_master(), > which calls devm_spi_register_controller(). > > In drivers/spi/spi.c, function spi_register_controller(), will in > turn call > of_register_spi_devices(ctlr) -> of_register_spi_device(ctlr, nc)... > that > will end up finally calling function of_spi_parse_dt(ctlr, spi, nc). > > The last mentioned function already contains the logic and setup to > check > devicetree properties "spi-tx-bus-width" and "spi-rx-bus-width" (and > some > others, as well). > > This means that spi-mt65xx.c already probed these even before your > IPM > implementation, hence ***function of_mtk_spi_parse_dt() is not > needed***. > > Simply drop it and don't check for these properties: that's already > done. > > > Regards, > Angelo > Hi Angelo, Thanks for your advice. There are two spi controllor on MT7986. One supports single/dual mode, the other supports quad mode. Both of them can support spi memory framework(one's tx/rx bus width is 1/2, the other one's tx/rx bus width is 1/2/4). Can I use of_mtk_spi_parse_dt() to parse the information? What's your suggestion? Thanks! > > + switch (value) { > > + case 1: > > + break; > > + case 2: > > + master->mode_bits |= SPI_TX_DUAL; > > + break; > > + case 4: > > + master->mode_bits |= SPI_TX_QUAD; > > + break; > > + default: > > + dev_warn(mdata->dev, > > + "spi-tx-bus-width %d not supported\n", > > + value); > > + break; > > + } > > + } > > + > > + if (!of_property_read_u32(nc, "spi-rx-bus-width", &value)) { > > + switch (value) { > > + case 1: > > + break; > > + case 2: > > + master->mode_bits |= SPI_RX_DUAL; > > + break; > > + case 4: > > + master->mode_bits |= SPI_RX_QUAD; > > + break; > > + case 8: > > + master->mode_bits |= SPI_RX_OCTAL; > > + break; > > + default: > > + dev_warn(mdata->dev, > > + "spi-rx-bus-width %d not supported\n", > > + value); > > + break; > > + } > > + } > > +} > > + > > static int mtk_spi_probe(struct platform_device *pdev) > > { > > struct spi_master *master; > > @@ -830,6 +1170,13 @@ static int mtk_spi_probe(struct > > platform_device *pdev) > > if (mdata->dev_comp->ipm_design) > > master->mode_bits |= SPI_LOOP; > > > > + if (mdata->dev_comp->ipm_design) { > > + mdata->dev = &pdev->dev; > > + master->mem_ops = &mtk_spi_mem_ops; > > + of_mtk_spi_parse_dt(master, pdev->dev.of_node); > > + init_completion(&mdata->spimem_done); > > + } > > + > > if (mdata->dev_comp->need_pad_sel) { > > mdata->pad_num = of_property_count_u32_elems( > > pdev->dev.of_node,
Il 17/03/22 10:27, Leilk Liu ha scritto: > On Tue, 2022-03-15 at 10:31 +0100, AngeloGioacchino Del Regno wrote: >> Il 15/03/22 04:24, Leilk Liu ha scritto: >>> this patch add the support of spi-mem for ipm design. >>> >>> Signed-off-by: Leilk Liu <leilk.liu@mediatek.com> >>> --- >>> drivers/spi/spi-mt65xx.c | 349 >>> ++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 348 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c >>> index 1a0b3208dfca..8958c3fa4fea 100644 >>> --- a/drivers/spi/spi-mt65xx.c >>> +++ b/drivers/spi/spi-mt65xx.c >> >> ...snip... >> >>> + >>> +static void of_mtk_spi_parse_dt(struct spi_master *master, struct >>> device_node *nc) >>> +{ >>> + struct mtk_spi *mdata = spi_master_get_devdata(master); >>> + u32 value; >>> + >>> + if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) { >> >> Hello Leilk, >> >> thanks for considering my advice about "spi-{tx,rx}-bus-width", but >> there's >> something that you have misunderstood about it. >> >> Simply, you don't need this function at all. Whatever you are doing >> here is >> already being performed in the Linux SPI framework: at the end of the >> probe >> function, this driver is calling the (legacy) >> devm_spi_register_master(), >> which calls devm_spi_register_controller(). >> >> In drivers/spi/spi.c, function spi_register_controller(), will in >> turn call >> of_register_spi_devices(ctlr) -> of_register_spi_device(ctlr, nc)... >> that >> will end up finally calling function of_spi_parse_dt(ctlr, spi, nc). >> >> The last mentioned function already contains the logic and setup to >> check >> devicetree properties "spi-tx-bus-width" and "spi-rx-bus-width" (and >> some >> others, as well). >> >> This means that spi-mt65xx.c already probed these even before your >> IPM >> implementation, hence ***function of_mtk_spi_parse_dt() is not >> needed***. >> >> Simply drop it and don't check for these properties: that's already >> done. >> >> >> Regards, >> Angelo >> > Hi Angelo, > > Thanks for your advice. > > There are two spi controllor on MT7986. One supports single/dual mode, > the other supports quad mode. Both of them can support spi memory > framework(one's tx/rx bus width is 1/2, the other one's tx/rx bus width > is 1/2/4). > > Can I use of_mtk_spi_parse_dt() to parse the information? What's your > suggestion? > > Thanks! > As I've already said, this does NOT require any devicetree handling in spi-mt65xx.c, as setting the right mode_bits is already handled in drivers/spi/spi.c - please follow the explaination that I have given before to fully understand the situation. Regards, Angelo > >>> + switch (value) { >>> + case 1: >>> + break; >>> + case 2: >>> + master->mode_bits |= SPI_TX_DUAL; >>> + break; >>> + case 4: >>> + master->mode_bits |= SPI_TX_QUAD; >>> + break; >>> + default: >>> + dev_warn(mdata->dev, >>> + "spi-tx-bus-width %d not supported\n", >>> + value); >>> + break; >>> + } >>> + } >>> + >>> + if (!of_property_read_u32(nc, "spi-rx-bus-width", &value)) { >>> + switch (value) { >>> + case 1:
On Thu, 2022-03-17 at 10:33 +0100, AngeloGioacchino Del Regno wrote: > Il 17/03/22 10:27, Leilk Liu ha scritto: > > On Tue, 2022-03-15 at 10:31 +0100, AngeloGioacchino Del Regno > > wrote: > > > Il 15/03/22 04:24, Leilk Liu ha scritto: > > > > this patch add the support of spi-mem for ipm design. > > > > > > > > Signed-off-by: Leilk Liu <leilk.liu@mediatek.com> > > > > --- > > > > drivers/spi/spi-mt65xx.c | 349 > > > > ++++++++++++++++++++++++++++++++++++++- > > > > 1 file changed, 348 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi- > > > > mt65xx.c > > > > index 1a0b3208dfca..8958c3fa4fea 100644 > > > > --- a/drivers/spi/spi-mt65xx.c > > > > +++ b/drivers/spi/spi-mt65xx.c > > > > > > ...snip... > > > > > > > + > > > > +static void of_mtk_spi_parse_dt(struct spi_master *master, > > > > struct > > > > device_node *nc) > > > > +{ > > > > + struct mtk_spi *mdata = spi_master_get_devdata(master); > > > > + u32 value; > > > > + > > > > + if (!of_property_read_u32(nc, "spi-tx-bus-width", > > > > &value)) { > > > > > > Hello Leilk, > > > > > > thanks for considering my advice about "spi-{tx,rx}-bus-width", > > > but > > > there's > > > something that you have misunderstood about it. > > > > > > Simply, you don't need this function at all. Whatever you are > > > doing > > > here is > > > already being performed in the Linux SPI framework: at the end of > > > the > > > probe > > > function, this driver is calling the (legacy) > > > devm_spi_register_master(), > > > which calls devm_spi_register_controller(). > > > > > > In drivers/spi/spi.c, function spi_register_controller(), will in > > > turn call > > > of_register_spi_devices(ctlr) -> of_register_spi_device(ctlr, > > > nc)... > > > that > > > will end up finally calling function of_spi_parse_dt(ctlr, spi, > > > nc). > > > > > > The last mentioned function already contains the logic and setup > > > to > > > check > > > devicetree properties "spi-tx-bus-width" and "spi-rx-bus-width" > > > (and > > > some > > > others, as well). > > > > > > This means that spi-mt65xx.c already probed these even before > > > your > > > IPM > > > implementation, hence ***function of_mtk_spi_parse_dt() is not > > > needed***. > > > > > > Simply drop it and don't check for these properties: that's > > > already > > > done. > > > > > > > > > Regards, > > > Angelo > > > > > > > Hi Angelo, > > > > Thanks for your advice. > > > > There are two spi controllor on MT7986. One supports single/dual > > mode, > > the other supports quad mode. Both of them can support spi memory > > framework(one's tx/rx bus width is 1/2, the other one's tx/rx bus > > width > > is 1/2/4). > > > > Can I use of_mtk_spi_parse_dt() to parse the information? What's > > your > > suggestion? > > > > Thanks! > > > > As I've already said, this does NOT require any devicetree handling > in > spi-mt65xx.c, as setting the right mode_bits is already handled in > drivers/spi/spi.c - please follow the explaination that I have given > before to fully understand the situation. > > Regards, > Angelo > OK, I'll fix it, thanks. > > > > > > > + switch (value) { > > > > + case 1: > > > > + break; > > > > + case 2: > > > > + master->mode_bits |= SPI_TX_DUAL; > > > > + break; > > > > + case 4: > > > > + master->mode_bits |= SPI_TX_QUAD; > > > > + break; > > > > + default: > > > > + dev_warn(mdata->dev, > > > > + "spi-tx-bus-width %d not > > > > supported\n", > > > > + value); > > > > + break; > > > > + } > > > > + } > > > > + > > > > + if (!of_property_read_u32(nc, "spi-rx-bus-width", > > > > &value)) { > > > > + switch (value) { > > > > + case 1: