| Message ID | 1584105134-13583-8-git-send-email-akashast@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add interconnect support to QSPI and QUP drivers | expand |
Hi, On Fri, Mar 13, 2020 at 06:42:13PM +0530, Akash Asthana wrote: > Get the interconnect paths for QSPI device and vote according to the > current bus speed of the driver. > > Signed-off-by: Akash Asthana <akashast@codeaurora.org> > --- > - As per Bjorn's comment, introduced and using devm_of_icc_get API for getting > path handle > - As per Matthias comment, added error handling for icc_set_bw call > > drivers/spi/spi-qcom-qspi.c | 46 ++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 45 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c > index 3c4f83b..ad48f43 100644 > --- a/drivers/spi/spi-qcom-qspi.c > +++ b/drivers/spi/spi-qcom-qspi.c > @@ -2,6 +2,7 @@ > // Copyright (c) 2017-2018, The Linux foundation. All rights reserved. > > #include <linux/clk.h> > +#include <linux/interconnect.h> > #include <linux/interrupt.h> > #include <linux/io.h> > #include <linux/module.h> > @@ -139,7 +140,10 @@ struct qcom_qspi { > struct device *dev; > struct clk_bulk_data *clks; > struct qspi_xfer xfer; > - /* Lock to protect xfer and IRQ accessed registers */ > + struct icc_path *icc_path_cpu_to_qspi; > + unsigned int avg_bw_cpu; > + unsigned int peak_bw_cpu; This triplet is a recurring pattern, and is probably not limited to geni SE/QSPI. On https://patchwork.kernel.org/patch/11436889/#23221925 I suggested the creation of a geni SE specific struct, however adding a generic convenience struct to 'linux/interconnect.h' might be the better solution: struct icc_client { struct icc_path *path; unsigned int avg_bw; unsigned int peak_bw; }; I'm sure there are better names for it, but this would be the idea.
Hi Matthias, On 3/14/2020 6:28 AM, Matthias Kaehlcke wrote: > Hi, > > On Fri, Mar 13, 2020 at 06:42:13PM +0530, Akash Asthana wrote: >> Get the interconnect paths for QSPI device and vote according to the >> current bus speed of the driver. >> >> Signed-off-by: Akash Asthana <akashast@codeaurora.org> >> --- >> - As per Bjorn's comment, introduced and using devm_of_icc_get API for getting >> path handle >> - As per Matthias comment, added error handling for icc_set_bw call >> >> drivers/spi/spi-qcom-qspi.c | 46 ++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 45 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c >> index 3c4f83b..ad48f43 100644 >> --- a/drivers/spi/spi-qcom-qspi.c >> +++ b/drivers/spi/spi-qcom-qspi.c >> @@ -2,6 +2,7 @@ >> // Copyright (c) 2017-2018, The Linux foundation. All rights reserved. >> >> #include <linux/clk.h> >> +#include <linux/interconnect.h> >> #include <linux/interrupt.h> >> #include <linux/io.h> >> #include <linux/module.h> >> @@ -139,7 +140,10 @@ struct qcom_qspi { >> struct device *dev; >> struct clk_bulk_data *clks; >> struct qspi_xfer xfer; >> - /* Lock to protect xfer and IRQ accessed registers */ >> + struct icc_path *icc_path_cpu_to_qspi; >> + unsigned int avg_bw_cpu; >> + unsigned int peak_bw_cpu; > This triplet is a recurring pattern, and is probably not limited to geni SE/QSPI. > On https://patchwork.kernel.org/patch/11436889/#23221925 I suggested the creation > of a geni SE specific struct, however adding a generic convenience struct to > 'linux/interconnect.h' might be the better solution: > > struct icc_client { > struct icc_path *path; > unsigned int avg_bw; > unsigned int peak_bw; > }; > > I'm sure there are better names for it, but this would be the idea. Yeah, I think introducing this to ICC header would be better solution. Regards, Akash
On Tue, Mar 17, 2020 at 5:13 AM Akash Asthana <akashast@codeaurora.org> wrote: > > Hi Matthias, > > On 3/14/2020 6:28 AM, Matthias Kaehlcke wrote: > > Hi, > > > > On Fri, Mar 13, 2020 at 06:42:13PM +0530, Akash Asthana wrote: > >> Get the interconnect paths for QSPI device and vote according to the > >> current bus speed of the driver. > >> > >> Signed-off-by: Akash Asthana <akashast@codeaurora.org> > >> --- > >> - As per Bjorn's comment, introduced and using devm_of_icc_get API for getting > >> path handle > >> - As per Matthias comment, added error handling for icc_set_bw call > >> > >> drivers/spi/spi-qcom-qspi.c | 46 ++++++++++++++++++++++++++++++++++++++++++++- > >> 1 file changed, 45 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c > >> index 3c4f83b..ad48f43 100644 > >> --- a/drivers/spi/spi-qcom-qspi.c > >> +++ b/drivers/spi/spi-qcom-qspi.c > >> @@ -2,6 +2,7 @@ > >> // Copyright (c) 2017-2018, The Linux foundation. All rights reserved. > >> > >> #include <linux/clk.h> > >> +#include <linux/interconnect.h> > >> #include <linux/interrupt.h> > >> #include <linux/io.h> > >> #include <linux/module.h> > >> @@ -139,7 +140,10 @@ struct qcom_qspi { > >> struct device *dev; > >> struct clk_bulk_data *clks; > >> struct qspi_xfer xfer; > >> - /* Lock to protect xfer and IRQ accessed registers */ > >> + struct icc_path *icc_path_cpu_to_qspi; > >> + unsigned int avg_bw_cpu; > >> + unsigned int peak_bw_cpu; > > This triplet is a recurring pattern, and is probably not limited to geni SE/QSPI. > > On https://patchwork.kernel.org/patch/11436889/#23221925 I suggested the creation > > of a geni SE specific struct, however adding a generic convenience struct to > > 'linux/interconnect.h' might be the better solution: > > > > struct icc_client { > > struct icc_path *path; > > unsigned int avg_bw; > > unsigned int peak_bw; > > }; > > > > I'm sure there are better names for it, but this would be the idea. > > Yeah, I think introducing this to ICC header would be better solution. +Georgi I'm not as convinced this structure is generally useful and belongs in the interconnect core. The thing that strikes me as weird with putting it in the core is now we're saving these values both inside and outside the interconnect core. In the GENI case here, we only really need them to undo the 0 votes we cast during suspend. If "vote for 0 in suspend and whatever it was before at resume" is a recurring theme, maybe the core should give us path_disable() and path_enable() calls instead. I'm thinking out loud, maybe Georgi has some thoughts. Akash, for now if you want to avoid wading into a larger discussion maybe just refactor to a common structure local to GENI. -Evan
Hi Evan, On 3/18/2020 12:38 AM, Evan Green wrote: > On Tue, Mar 17, 2020 at 5:13 AM Akash Asthana <akashast@codeaurora.org> wrote: >> Hi Matthias, >> >> On 3/14/2020 6:28 AM, Matthias Kaehlcke wrote: >>> Hi, >>> >>> On Fri, Mar 13, 2020 at 06:42:13PM +0530, Akash Asthana wrote: >>>> Get the interconnect paths for QSPI device and vote according to the >>>> current bus speed of the driver. >>>> >>>> Signed-off-by: Akash Asthana <akashast@codeaurora.org> >>>> --- >>>> - As per Bjorn's comment, introduced and using devm_of_icc_get API for getting >>>> path handle >>>> - As per Matthias comment, added error handling for icc_set_bw call >>>> >>>> drivers/spi/spi-qcom-qspi.c | 46 ++++++++++++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 45 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c >>>> index 3c4f83b..ad48f43 100644 >>>> --- a/drivers/spi/spi-qcom-qspi.c >>>> +++ b/drivers/spi/spi-qcom-qspi.c >>>> @@ -2,6 +2,7 @@ >>>> // Copyright (c) 2017-2018, The Linux foundation. All rights reserved. >>>> >>>> #include <linux/clk.h> >>>> +#include <linux/interconnect.h> >>>> #include <linux/interrupt.h> >>>> #include <linux/io.h> >>>> #include <linux/module.h> >>>> @@ -139,7 +140,10 @@ struct qcom_qspi { >>>> struct device *dev; >>>> struct clk_bulk_data *clks; >>>> struct qspi_xfer xfer; >>>> - /* Lock to protect xfer and IRQ accessed registers */ >>>> + struct icc_path *icc_path_cpu_to_qspi; >>>> + unsigned int avg_bw_cpu; >>>> + unsigned int peak_bw_cpu; >>> This triplet is a recurring pattern, and is probably not limited to geni SE/QSPI. >>> On https://patchwork.kernel.org/patch/11436889/#23221925 I suggested the creation >>> of a geni SE specific struct, however adding a generic convenience struct to >>> 'linux/interconnect.h' might be the better solution: >>> >>> struct icc_client { >>> struct icc_path *path; >>> unsigned int avg_bw; >>> unsigned int peak_bw; >>> }; >>> >>> I'm sure there are better names for it, but this would be the idea. >> Yeah, I think introducing this to ICC header would be better solution. > +Georgi > > I'm not as convinced this structure is generally useful and belongs in > the interconnect core. The thing that strikes me as weird with putting > it in the core is now we're saving these values both inside and > outside the interconnect core. IIUC, you meant to say struct icc_req(inside icc_path) will be saving avg_bw and peak_bw so no need to save it outside icc_path? > In the GENI case here, we only really > need them to undo the 0 votes we cast during suspend. If "vote for 0 > in suspend and whatever it was before at resume" is a recurring theme, > maybe the core should give us path_disable() and path_enable() calls > instead. I'm thinking out loud, maybe Georgi has some thoughts. > > Akash, for now if you want to avoid wading into a larger discussion > maybe just refactor to a common structure local to GENI. Ok Thanks, Akash > > > -Evan
On Wed, Mar 18, 2020 at 6:48 AM Akash Asthana <akashast@codeaurora.org> wrote: > > Hi Evan, > > On 3/18/2020 12:38 AM, Evan Green wrote: > > On Tue, Mar 17, 2020 at 5:13 AM Akash Asthana <akashast@codeaurora.org> wrote: > >> Hi Matthias, > >> > >> On 3/14/2020 6:28 AM, Matthias Kaehlcke wrote: > >>> Hi, > >>> > >>> On Fri, Mar 13, 2020 at 06:42:13PM +0530, Akash Asthana wrote: > >>>> Get the interconnect paths for QSPI device and vote according to the > >>>> current bus speed of the driver. > >>>> > >>>> Signed-off-by: Akash Asthana <akashast@codeaurora.org> > >>>> --- > >>>> - As per Bjorn's comment, introduced and using devm_of_icc_get API for getting > >>>> path handle > >>>> - As per Matthias comment, added error handling for icc_set_bw call > >>>> > >>>> drivers/spi/spi-qcom-qspi.c | 46 ++++++++++++++++++++++++++++++++++++++++++++- > >>>> 1 file changed, 45 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c > >>>> index 3c4f83b..ad48f43 100644 > >>>> --- a/drivers/spi/spi-qcom-qspi.c > >>>> +++ b/drivers/spi/spi-qcom-qspi.c > >>>> @@ -2,6 +2,7 @@ > >>>> // Copyright (c) 2017-2018, The Linux foundation. All rights reserved. > >>>> > >>>> #include <linux/clk.h> > >>>> +#include <linux/interconnect.h> > >>>> #include <linux/interrupt.h> > >>>> #include <linux/io.h> > >>>> #include <linux/module.h> > >>>> @@ -139,7 +140,10 @@ struct qcom_qspi { > >>>> struct device *dev; > >>>> struct clk_bulk_data *clks; > >>>> struct qspi_xfer xfer; > >>>> - /* Lock to protect xfer and IRQ accessed registers */ > >>>> + struct icc_path *icc_path_cpu_to_qspi; > >>>> + unsigned int avg_bw_cpu; > >>>> + unsigned int peak_bw_cpu; > >>> This triplet is a recurring pattern, and is probably not limited to geni SE/QSPI. > >>> On https://patchwork.kernel.org/patch/11436889/#23221925 I suggested the creation > >>> of a geni SE specific struct, however adding a generic convenience struct to > >>> 'linux/interconnect.h' might be the better solution: > >>> > >>> struct icc_client { > >>> struct icc_path *path; > >>> unsigned int avg_bw; > >>> unsigned int peak_bw; > >>> }; > >>> > >>> I'm sure there are better names for it, but this would be the idea. > >> Yeah, I think introducing this to ICC header would be better solution. > > +Georgi > > > > I'm not as convinced this structure is generally useful and belongs in > > the interconnect core. The thing that strikes me as weird with putting > > it in the core is now we're saving these values both inside and > > outside the interconnect core. > IIUC, you meant to say struct icc_req(inside icc_path) will be saving > avg_bw and peak_bw so no need to save it outside icc_path? Correct, it seems silly to store the same set of values twice in the framework, but with different semantics about who's watching it. -Evan
Hi Evan, >> IIUC, you meant to say struct icc_req(inside icc_path) will be saving >> avg_bw and peak_bw so no need to save it outside icc_path? > Correct, it seems silly to store the same set of values twice in the > framework, but with different semantics about who's watching it. > -Evan Thanks for clarification! Yeah make sense not to introduce the structure in ICC framework Regards, Akash
diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c index 3c4f83b..ad48f43 100644 --- a/drivers/spi/spi-qcom-qspi.c +++ b/drivers/spi/spi-qcom-qspi.c @@ -2,6 +2,7 @@ // Copyright (c) 2017-2018, The Linux foundation. All rights reserved. #include <linux/clk.h> +#include <linux/interconnect.h> #include <linux/interrupt.h> #include <linux/io.h> #include <linux/module.h> @@ -139,7 +140,10 @@ struct qcom_qspi { struct device *dev; struct clk_bulk_data *clks; struct qspi_xfer xfer; - /* Lock to protect xfer and IRQ accessed registers */ + struct icc_path *icc_path_cpu_to_qspi; + unsigned int avg_bw_cpu; + unsigned int peak_bw_cpu; + /* Lock to protect data accessed by IRQs */ spinlock_t lock; }; @@ -241,6 +245,20 @@ static int qcom_qspi_transfer_one(struct spi_master *master, return ret; } + /* + * Set BW quota for CPU as driver supports FIFO mode only. + * Assume peak bw as twice of avg bw. + */ + ctrl->avg_bw_cpu = Bps_to_icc(speed_hz); + ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz); + ret = icc_set_bw(ctrl->icc_path_cpu_to_qspi, ctrl->avg_bw_cpu, + ctrl->peak_bw_cpu); + if (ret) { + dev_err(ctrl->dev, "%s: ICC BW voting failed for cpu\n", + __func__); + return ret; + } + spin_lock_irqsave(&ctrl->lock, flags); /* We are half duplex, so either rx or tx will be set */ @@ -458,6 +476,15 @@ static int qcom_qspi_probe(struct platform_device *pdev) if (ret) goto exit_probe_master_put; + ctrl->icc_path_cpu_to_qspi = devm_of_icc_get(dev, "qspi-config"); + if (IS_ERR(ctrl->icc_path_cpu_to_qspi)) { + ret = PTR_ERR(ctrl->icc_path_cpu_to_qspi); + goto exit_probe_master_put; + } + /* Put BW vote on CPU path for register access */ + ctrl->avg_bw_cpu = Bps_to_icc(1000); + ctrl->peak_bw_cpu = Bps_to_icc(1000); + ret = platform_get_irq(pdev, 0); if (ret < 0) goto exit_probe_master_put; @@ -511,9 +538,17 @@ static int __maybe_unused qcom_qspi_runtime_suspend(struct device *dev) { struct spi_master *master = dev_get_drvdata(dev); struct qcom_qspi *ctrl = spi_master_get_devdata(master); + int ret; clk_bulk_disable_unprepare(QSPI_NUM_CLKS, ctrl->clks); + ret = icc_set_bw(ctrl->icc_path_cpu_to_qspi, 0, 0); + if (ret) { + dev_err_ratelimited(ctrl->dev, "%s: ICC BW remove failed for cpu\n", + __func__); + return ret; + } + return 0; } @@ -521,6 +556,15 @@ static int __maybe_unused qcom_qspi_runtime_resume(struct device *dev) { struct spi_master *master = dev_get_drvdata(dev); struct qcom_qspi *ctrl = spi_master_get_devdata(master); + int ret; + + ret = icc_set_bw(ctrl->icc_path_cpu_to_qspi, ctrl->avg_bw_cpu, + ctrl->peak_bw_cpu); + if (ret) { + dev_err_ratelimited(ctrl->dev, "%s: ICC BW voting failed for cpu\n", + __func__); + return ret; + } return clk_bulk_prepare_enable(QSPI_NUM_CLKS, ctrl->clks); }
Get the interconnect paths for QSPI device and vote according to the current bus speed of the driver. Signed-off-by: Akash Asthana <akashast@codeaurora.org> --- - As per Bjorn's comment, introduced and using devm_of_icc_get API for getting path handle - As per Matthias comment, added error handling for icc_set_bw call drivers/spi/spi-qcom-qspi.c | 46 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-)