| Message ID | 1585652976-17481-1-git-send-email-akashast@codeaurora.org |
|---|---|
| Headers | show |
| Series | Add interconnect support to QSPI and QUP drivers | expand |
On Tue, Mar 31, 2020 at 04:39:35PM +0530, Akash Asthana wrote: > + /* > + * 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); I thought you were going to factor this best guess handling of peak bandwidth out into the core?
On Tue, Mar 31, 2020 at 04:39:33PM +0530, Akash Asthana wrote: > Get the interconnect paths for SPI based Serial Engine device > and vote according to the current bus speed of the driver. > > Signed-off-by: Akash Asthana <akashast@codeaurora.org> > --- > Changes in V2: > - As per Bjorn's comment, removed se == NULL check from geni_spi_icc_get > - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure > - 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 > > Changes in V3: > - As per Matthias's comment, use helper ICC function from geni-se driver. > > drivers/spi/spi-geni-qcom.c | 31 ++++++++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c > index c397242..f1dae2d 100644 > --- a/drivers/spi/spi-geni-qcom.c > +++ b/drivers/spi/spi-geni-qcom.c > @@ -234,6 +234,16 @@ static int setup_fifo_params(struct spi_device *spi_slv, > return ret; > } > > + /* > + * Set BW quota for CPU as driver supports FIFO mode only. > + * Assume peak bw as twice of avg bw. > + */ > + se->from_cpu.avg_bw = Bps_to_icc(mas->cur_speed_hz); > + se->from_cpu.peak_bw = Bps_to_icc(2 * mas->cur_speed_hz); > + ret = geni_icc_vote_on(se); > + if (ret) > + return ret; > + > clk_sel = idx & CLK_SEL_MSK; > m_clk_cfg = (div << CLK_DIV_SHFT) | SER_CLK_EN; > spi_setup_word_len(mas, spi_slv->mode, spi_slv->bits_per_word); > @@ -578,6 +588,15 @@ static int spi_geni_probe(struct platform_device *pdev) > spin_lock_init(&mas->lock); > pm_runtime_enable(dev); > > + ret = geni_icc_get(&mas->se, "qup-core", "qup-config", NULL); > + if (ret) > + goto spi_geni_probe_runtime_disable; This fails without providing any hints why, besides the error code. It might be worth to add error logging to geni_icc_get(). > + /* Set the bus quota to a reasonable value for register access */ > + mas->se.to_core.avg_bw = Bps_to_icc(CORE_2X_50_MHZ); > + mas->se.to_core.peak_bw = Bps_to_icc(CORE_2X_100_MHZ); > + mas->se.from_cpu.avg_bw = GENI_DEFAULT_BW; > + mas->se.from_cpu.peak_bw = GENI_DEFAULT_BW; > + > ret = spi_geni_init(mas); > if (ret) > goto spi_geni_probe_runtime_disable; > @@ -616,14 +635,24 @@ static int __maybe_unused spi_geni_runtime_suspend(struct device *dev) > { > struct spi_master *spi = dev_get_drvdata(dev); > struct spi_geni_master *mas = spi_master_get_devdata(spi); > + int ret; > + > + ret = geni_se_resources_off(&mas->se); > + if (ret) > + return ret; > > - return geni_se_resources_off(&mas->se); > + return geni_icc_vote_off(&mas->se); > } > > static int __maybe_unused spi_geni_runtime_resume(struct device *dev) > { > struct spi_master *spi = dev_get_drvdata(dev); > struct spi_geni_master *mas = spi_master_get_devdata(spi); > + int ret; > + > + ret = geni_icc_vote_on(&mas->se); > + if (ret) > + return ret; > > return geni_se_resources_on(&mas->se); > } Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
On Tue, Mar 31, 2020 at 04:39:35PM +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> > --- > Changes in V2: > - 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 > > Changes in V3: > - No Change. > > 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; > + /* 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); > } Looks good to me besides Mark's concern about the bandwith calculation logic. Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
On Tue, Mar 31, 2020 at 11:24:57AM -0700, Matthias Kaehlcke wrote: > Hi Akash, > > On Tue, Mar 31, 2020 at 04:39:31PM +0530, Akash Asthana wrote: > > QUP core clock is shared among all the SE drivers present on particular > > QUP wrapper, the system will reset(unclocked access) if earlycon used after > > QUP core clock is put to 0 from other SE drivers before real console comes > > up. > > > > As earlycon can't vote for it's QUP core need, to fix this add ICC > > support to common/QUP wrapper driver and put vote for QUP core from > > probe on behalf of earlycon and remove vote during earlycon exit call. > > > > Signed-off-by: Akash Asthana <akashast@codeaurora.org> > > Reported-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > Change is V3: > > - Add geni_remove_earlycon_icc_vote API that will be used by earlycon > > exit function to remove ICC vote for earlyconsole. > > - Remove suspend/resume hook for geni-se driver as we are no longer > > removing earlyconsole ICC vote from system suspend, we are removing > > from earlycon exit. > > > > drivers/soc/qcom/qcom-geni-se.c | 51 +++++++++++++++++++++++++++++++++++ > > drivers/tty/serial/qcom_geni_serial.c | 7 +++++ > > include/linux/qcom-geni-se.h | 2 ++ > > 3 files changed, 60 insertions(+) > > > > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c > > index 9344c14..d30c282 100644 > > --- a/drivers/soc/qcom/qcom-geni-se.c > > +++ b/drivers/soc/qcom/qcom-geni-se.c > > @@ -90,8 +90,11 @@ struct geni_wrapper { > > struct device *dev; > > void __iomem *base; > > struct clk_bulk_data ahb_clks[NUM_AHB_CLKS]; > > + struct geni_icc_path to_core; > > }; > > > > +struct geni_wrapper *earlycon_wrapper; > > should be static > > > + > > #define QUP_HW_VER_REG 0x4 > > > > /* Common SE registers */ > > @@ -818,6 +821,26 @@ int geni_icc_vote_off(struct geni_se *se) > > } > > EXPORT_SYMBOL(geni_icc_vote_off); > > > > +void geni_remove_earlycon_icc_vote(void) > > +{ > > + struct geni_wrapper *wrapper = earlycon_wrapper; > > + struct device_node *parent = of_get_next_parent(wrapper->dev->of_node); > > + struct device_node *child; > > + > > + for_each_child_of_node(parent, child) { > > + if (of_device_is_compatible(child, "qcom,geni-se-qup")) { > > + wrapper = platform_get_drvdata(of_find_device_by_node( > > + child)); > > + icc_put(wrapper->to_core.path); > > + wrapper->to_core.path = NULL; > > + } > > + } > > + of_node_put(parent); > > + > > + earlycon_wrapper = NULL; > > +} > > +EXPORT_SYMBOL(geni_remove_earlycon_icc_vote); > > I didn't know that consoles have an exit handler, this is way nicer than > the miscellaneous triggers we discussed earlier :) No wonder I 'missed' this when looking at the console code for possible triggers, it is brand new and as of now only exists in -next: commit ed31685c96e18f773ca11dd1a637974d62130673 Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: Mon Feb 3 15:31:30 2020 +0200 console: Introduce ->exit() callback sharp timing!
Hi Matthias On 4/2/2020 1:16 AM, Matthias Kaehlcke wrote: > On Tue, Mar 31, 2020 at 11:24:57AM -0700, Matthias Kaehlcke wrote: >> Hi Akash, >> >> On Tue, Mar 31, 2020 at 04:39:31PM +0530, Akash Asthana wrote: >>> QUP core clock is shared among all the SE drivers present on particular >>> QUP wrapper, the system will reset(unclocked access) if earlycon used after >>> QUP core clock is put to 0 from other SE drivers before real console comes >>> up. >>> >>> As earlycon can't vote for it's QUP core need, to fix this add ICC >>> support to common/QUP wrapper driver and put vote for QUP core from >>> probe on behalf of earlycon and remove vote during earlycon exit call. >>> >>> Signed-off-by: Akash Asthana <akashast@codeaurora.org> >>> Reported-by: Matthias Kaehlcke <mka@chromium.org> >>> --- >>> Change is V3: >>> - Add geni_remove_earlycon_icc_vote API that will be used by earlycon >>> exit function to remove ICC vote for earlyconsole. >>> - Remove suspend/resume hook for geni-se driver as we are no longer >>> removing earlyconsole ICC vote from system suspend, we are removing >>> from earlycon exit. >>> >>> drivers/soc/qcom/qcom-geni-se.c | 51 +++++++++++++++++++++++++++++++++++ >>> drivers/tty/serial/qcom_geni_serial.c | 7 +++++ >>> include/linux/qcom-geni-se.h | 2 ++ >>> 3 files changed, 60 insertions(+) >>> >>> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c >>> index 9344c14..d30c282 100644 >>> --- a/drivers/soc/qcom/qcom-geni-se.c >>> +++ b/drivers/soc/qcom/qcom-geni-se.c >>> @@ -90,8 +90,11 @@ struct geni_wrapper { >>> struct device *dev; >>> void __iomem *base; >>> struct clk_bulk_data ahb_clks[NUM_AHB_CLKS]; >>> + struct geni_icc_path to_core; >>> }; >>> >>> +struct geni_wrapper *earlycon_wrapper; >> should be static Yeah ok, I missed it. >> >>> + >>> #define QUP_HW_VER_REG 0x4 >>> >>> /* Common SE registers */ >>> @@ -818,6 +821,26 @@ int geni_icc_vote_off(struct geni_se *se) >>> } >>> EXPORT_SYMBOL(geni_icc_vote_off); >>> >>> +void geni_remove_earlycon_icc_vote(void) >>> +{ >>> + struct geni_wrapper *wrapper = earlycon_wrapper; >>> + struct device_node *parent = of_get_next_parent(wrapper->dev->of_node); >>> + struct device_node *child; >>> + >>> + for_each_child_of_node(parent, child) { >>> + if (of_device_is_compatible(child, "qcom,geni-se-qup")) { >>> + wrapper = platform_get_drvdata(of_find_device_by_node( >>> + child)); >>> + icc_put(wrapper->to_core.path); >>> + wrapper->to_core.path = NULL; >>> + } >>> + } >>> + of_node_put(parent); >>> + >>> + earlycon_wrapper = NULL; >>> +} >>> +EXPORT_SYMBOL(geni_remove_earlycon_icc_vote); >> I didn't know that consoles have an exit handler, this is way nicer than >> the miscellaneous triggers we discussed earlier :) > No wonder I 'missed' this when looking at the console code for possible > triggers, it is brand new and as of now only exists in -next: > > commit ed31685c96e18f773ca11dd1a637974d62130673 > Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Date: Mon Feb 3 15:31:30 2020 +0200 > > console: Introduce ->exit() callback > > > sharp timing! Yeah this is added recently, even I was not aware of it, Bjorn suggested me to use this. Indeed sharp timing! :) Regards, Akash
Hi Matthias, >> >> + ret = geni_icc_get(&mas->se, "qup-core", "qup-config", NULL); >> + if (ret) >> + goto spi_geni_probe_runtime_disable; > This fails without providing any hints why, besides the error code. > It might be worth to add error logging to geni_icc_get(). > As per Bjorn's comment, I will add error logs inside geni_icc_get API incase it returns something other than -EPROBE_DEFER . https://patchwork.kernel.org/patch/11467511/ regards, Akash
Hi Akash, On Tue, Apr 07, 2020 at 05:04:17PM +0530, Akash Asthana wrote: > Hi Matthias, > > > > > static int geni_se_probe(struct platform_device *pdev) > > > { > > > struct device *dev = &pdev->dev; > > > @@ -845,6 +868,34 @@ static int geni_se_probe(struct platform_device *pdev) > > > } > > > } > > > +#ifdef CONFIG_SERIAL_EARLYCON > > > + wrapper->to_core.path = devm_of_icc_get(dev, "qup-core"); > > > + if (IS_ERR(wrapper->to_core.path)) > > > + return PTR_ERR(wrapper->to_core.path); > > > + /* > > > + * Put minmal BW request on core clocks on behalf of early console. > > > + * The vote will be removed earlycon exit function. > > > + * > > > + * Note: We are putting vote on each QUP wrapper instead only to which > > > + * earlycon is connected because QUP core clock of different wrapper > > > + * share same voltage domain. If core1 is put to 0, then core2 will > > > + * also run at 0, if not voted. Default ICC vote will be removed ASA > > > + * we touch any of the core clock. > > > + * core1 = core2 = max(core1, core2) > > > + */ > > I don't really understand this part. According to the comment if we vote > > (let's say) for core2 but not for core1 then: > > > > core1: 0 > > core2: GENI_DEFAULT_BW > > > > core1 = core2 = max(core1, core2) > > or > > core1 = core2 = max(0, GENI_DEFAULT_BW) > > > > hence > > > > core1 = core2 = GENI_DEFAULT_BW > > > > What am I missing, why is it necessary to vote for both/all? > say core1 is for earlycon usecase > > There is common switch to control both the QUP core clock. I guess most > appropriate description would be switch = max(vote_on_core1, > vote_on_core2) + default_vote. > > During early bootup, vote_on_core1 = 0, vote_on_core2 = 0; > > As earlycon was working even without voting it's core need because there was > some default vote present on the core switch by ICC during bootup. > > So if any child(say SPI) of other QUP wrapper resumed and suspended before > earlycon wrapper comes up. This will make core clock to run at zero and will > cause NOC issue because vote_on_core1 = 0, vote_on_core2 = 0; and it seems > default votes from core switch is removed ASA it's voted on any core. Thanks for the explication! You are probably totally right, but for some reason my brain still resists to get it ... With the above my current interpretation is (assuming earlycon only votes on core1): core1 core2 default switch early boot 0 0 1 1 SPI resume (core2) 0 1 0 1 SPI suspend (core2) 0 0 0 0 earlycon init 1 0 0 1 What is wrong in the above table? Thanks for bearing with me :) Matthias
Hi Matthias, On 4/7/2020 10:56 PM, Matthias Kaehlcke wrote: > Hi Akash, > > On Tue, Apr 07, 2020 at 05:04:17PM +0530, Akash Asthana wrote: >> Hi Matthias, >> >> >>>> static int geni_se_probe(struct platform_device *pdev) >>>> { >>>> struct device *dev = &pdev->dev; >>>> @@ -845,6 +868,34 @@ static int geni_se_probe(struct platform_device *pdev) >>>> } >>>> } >>>> +#ifdef CONFIG_SERIAL_EARLYCON >>>> + wrapper->to_core.path = devm_of_icc_get(dev, "qup-core"); >>>> + if (IS_ERR(wrapper->to_core.path)) >>>> + return PTR_ERR(wrapper->to_core.path); >>>> + /* >>>> + * Put minmal BW request on core clocks on behalf of early console. >>>> + * The vote will be removed earlycon exit function. >>>> + * >>>> + * Note: We are putting vote on each QUP wrapper instead only to which >>>> + * earlycon is connected because QUP core clock of different wrapper >>>> + * share same voltage domain. If core1 is put to 0, then core2 will >>>> + * also run at 0, if not voted. Default ICC vote will be removed ASA >>>> + * we touch any of the core clock. >>>> + * core1 = core2 = max(core1, core2) >>>> + */ >>> I don't really understand this part. According to the comment if we vote >>> (let's say) for core2 but not for core1 then: >>> >>> core1: 0 >>> core2: GENI_DEFAULT_BW >>> >>> core1 = core2 = max(core1, core2) >>> or >>> core1 = core2 = max(0, GENI_DEFAULT_BW) >>> >>> hence >>> >>> core1 = core2 = GENI_DEFAULT_BW >>> >>> What am I missing, why is it necessary to vote for both/all? >> say core1 is for earlycon usecase >> >> There is common switch to control both the QUP core clock. I guess most >> appropriate description would be switch = max(vote_on_core1, >> vote_on_core2) + default_vote. >> >> During early bootup, vote_on_core1 = 0, vote_on_core2 = 0; >> >> As earlycon was working even without voting it's core need because there was >> some default vote present on the core switch by ICC during bootup. >> >> So if any child(say SPI) of other QUP wrapper resumed and suspended before >> earlycon wrapper comes up. This will make core clock to run at zero and will >> cause NOC issue because vote_on_core1 = 0, vote_on_core2 = 0; and it seems >> default votes from core switch is removed ASA it's voted on any core. > Thanks for the explication! > > You are probably totally right, but for some reason my brain still resists > to get it ... > > With the above my current interpretation is (assuming earlycon only votes on > core1): > > core1 core2 default switch > early boot 0 0 1 1 > SPI resume (core2) 0 1 0 1 > SPI suspend (core2) 0 0 0 0 > earlycon init 1 0 0 1 > > > What is wrong in the above table? > > Thanks for bearing with me :) NP :) I guess you meant QUP WRAPPER 1 probe by "earlycon init". core1 core2 default switch Time early boot 0 0 1 1 0s SPI resume (core2) 0 1 0 1 3.2s SPI suspend (core2) 0 0 0 0 3.3s QUP WRAPPER 1(probe) 1 0 0 1 5s (say) So switch is at 0 in time interval [3.3, 5] that will make core clock to run at 0. If we use earlycon during this time interval it will reset the board. Did above answered the query? Regards, Akash > > Matthias
Hi Mark, Evan, Georgi, On 4/7/2020 4:25 PM, Mark Brown wrote: > On Tue, Apr 07, 2020 at 03:24:42PM +0530, Akash Asthana wrote: >> On 3/31/2020 4:53 PM, Mark Brown wrote: >>>> + ctrl->avg_bw_cpu = Bps_to_icc(speed_hz); >>>> + ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz); >>> I thought you were going to factor this best guess handling of peak >>> bandwidth out into the core? >> I can centralize this for SPI, I2C and UART in Common driver(QUP wrapper) >> but still for QSPI I have to keep this piece of code as is because It is not >> child of QUP wrapper(it doesn't use common code). > Why not? > >> I am not sure whether I can move this " Assume peak_bw as twice of avg_bw if >> nothing is mentioned explicitly" to ICC core because the factor of 2 is >> chosen randomly by me. > That's the whole point - if this is just a random number then we may as > well at least be consistently random. Can we centralize below logic of peak_bw selection for all the clients to ICC core? "Assume peak_bw requirement as twice of avg_bw, if it is not mentioned explicitly" =========================================================================== int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw) { struct icc_node *node; u32 old_avg, old_peak; size_t i; int ret; if (!path) return 0; if (WARN_ON(IS_ERR(path) || !path->num_nodes)) return -EINVAL; + /* + * Assume peak_bw requirement as twice of avg_bw, if it is not + * mentioned explicitly + */ + peak_bw = peak_bw ? peak_bw : 2 * avg_bw; =========================================================================== In case if some client really don't want to put peak requirement they can pass avg_bw = peak_bw. As peak_bw <= avg_bw is kind of no-ops. Regards, Akash
On Wed, Apr 08, 2020 at 05:08:01PM +0530, Akash Asthana wrote: > Hi Matthias, > > On 4/7/2020 10:56 PM, Matthias Kaehlcke wrote: > > Hi Akash, > > > > On Tue, Apr 07, 2020 at 05:04:17PM +0530, Akash Asthana wrote: > > > Hi Matthias, > > > > > > > > > > > static int geni_se_probe(struct platform_device *pdev) > > > > > { > > > > > struct device *dev = &pdev->dev; > > > > > @@ -845,6 +868,34 @@ static int geni_se_probe(struct platform_device *pdev) > > > > > } > > > > > } > > > > > +#ifdef CONFIG_SERIAL_EARLYCON > > > > > + wrapper->to_core.path = devm_of_icc_get(dev, "qup-core"); > > > > > + if (IS_ERR(wrapper->to_core.path)) > > > > > + return PTR_ERR(wrapper->to_core.path); > > > > > + /* > > > > > + * Put minmal BW request on core clocks on behalf of early console. > > > > > + * The vote will be removed earlycon exit function. > > > > > + * > > > > > + * Note: We are putting vote on each QUP wrapper instead only to which > > > > > + * earlycon is connected because QUP core clock of different wrapper > > > > > + * share same voltage domain. If core1 is put to 0, then core2 will > > > > > + * also run at 0, if not voted. Default ICC vote will be removed ASA > > > > > + * we touch any of the core clock. > > > > > + * core1 = core2 = max(core1, core2) > > > > > + */ > > > > I don't really understand this part. According to the comment if we vote > > > > (let's say) for core2 but not for core1 then: > > > > > > > > core1: 0 > > > > core2: GENI_DEFAULT_BW > > > > > > > > core1 = core2 = max(core1, core2) > > > > or > > > > core1 = core2 = max(0, GENI_DEFAULT_BW) > > > > > > > > hence > > > > > > > > core1 = core2 = GENI_DEFAULT_BW > > > > > > > > What am I missing, why is it necessary to vote for both/all? > > > say core1 is for earlycon usecase > > > > > > There is common switch to control both the QUP core clock. I guess most > > > appropriate description would be switch = max(vote_on_core1, > > > vote_on_core2) + default_vote. > > > > > > During early bootup, vote_on_core1 = 0, vote_on_core2 = 0; > > > > > > As earlycon was working even without voting it's core need because there was > > > some default vote present on the core switch by ICC during bootup. > > > > > > So if any child(say SPI) of other QUP wrapper resumed and suspended before > > > earlycon wrapper comes up. This will make core clock to run at zero and will > > > cause NOC issue because vote_on_core1 = 0, vote_on_core2 = 0; and it seems > > > default votes from core switch is removed ASA it's voted on any core. > > Thanks for the explication! > > > > You are probably totally right, but for some reason my brain still resists > > to get it ... > > > > With the above my current interpretation is (assuming earlycon only votes on > > core1): > > > > core1 core2 default switch > > early boot 0 0 1 1 > > SPI resume (core2) 0 1 0 1 > > SPI suspend (core2) 0 0 0 0 > > earlycon init 1 0 0 1 > > > > > > What is wrong in the above table? > > > > Thanks for bearing with me :) > NP :) > > I guess you meant QUP WRAPPER 1 probe by "earlycon init". > > core1 core2 default switch Time > early boot 0 0 1 1 0s > SPI resume (core2) 0 1 0 1 3.2s > SPI suspend (core2) 0 0 0 0 3.3s > QUP WRAPPER 1(probe) 1 0 0 1 5s (say) > > So switch is at 0 in time interval [3.3, 5] that will make core clock to run at 0. > If we use earlycon during this time interval it will reset the board. > > Did above answered the query? now it finally made 'click', thanks :)
Hi Akash, On 4/8/20 15:17, Akash Asthana wrote: > Hi Mark, Evan, Georgi, > > On 4/7/2020 4:25 PM, Mark Brown wrote: >> On Tue, Apr 07, 2020 at 03:24:42PM +0530, Akash Asthana wrote: >>> On 3/31/2020 4:53 PM, Mark Brown wrote: >>>>> + ctrl->avg_bw_cpu = Bps_to_icc(speed_hz); >>>>> + ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz); >>>> I thought you were going to factor this best guess handling of peak >>>> bandwidth out into the core? >>> I can centralize this for SPI, I2C and UART in Common driver(QUP wrapper) >>> but still for QSPI I have to keep this piece of code as is because It is not >>> child of QUP wrapper(it doesn't use common code). >> Why not? >> >>> I am not sure whether I can move this " Assume peak_bw as twice of avg_bw if >>> nothing is mentioned explicitly" to ICC core because the factor of 2 is >>> chosen randomly by me. >> That's the whole point - if this is just a random number then we may as >> well at least be consistently random. > > Can we centralize below logic of peak_bw selection for all the clients to ICC core? I don't think this is a good idea for now, because this is very hardware specific. A scaling factor that works for one client might not work for another. My questions here is how did you decide on this "multiply by two"? I can imagine that the traffic can be bursty on some interfaces, but is the factor here really a "random number" or is this based on some data patterns or performance analysis? Thanks, Georgi
dt-binding patch for QUP drivers. - https://patchwork.kernel.org/patch/11436621/ [Convert QUP bindings to YAML and add ICC, pin swap doc] dt-binding patch for QSPI. - https://patchwork.kernel.org/patch/11436719/ [Convert QSPI binding to YAML and add interconnect doc] High level design: - QUP wrapper/common driver. Vote for QUP core on behalf of earlycon from probe. Remove BW vote during earlycon exit call - SERIAL driver. Vote only for CPU/CORE path because driver is in FIFO mode only Vote/unvote from qcom_geni_serial_pm func. Bump up the CPU vote from set_termios call based on real time need - I2C driver. Vote for CORE/CPU/DDR path Vote/unvote from runtime resume/suspend callback As bus speed for I2C is fixed from probe itself no need for bump up. - SPI QUP driver. Vote only for CPU/CORE path because driver is in FIFO mode only Vote/unvote from runtime resume/suspend callback Bump up CPU vote based on real time need per transfer. - QSPI driver. Vote only for CPU path Vote/unvote from runtime resume/suspend callback Bump up CPU vote based on real time need per transfer. Changes in V2: - Add devm_of_icc_get() API interconnect core. - Add ICC support to common driver to fix earlyconsole crash. Changes in V3: - Define common ICC APIs in geni-se driver and use it across geni based I2C,SPI and UART driver. Akash Asthana (8): interconnect: Add devm_of_icc_get() as exported API for users soc: qcom: geni: Support for ICC voting soc: qcom-geni-se: Add interconnect support to fix earlycon crash i2c: i2c-qcom-geni: Add interconnect support spi: spi-geni-qcom: Add interconnect support tty: serial: qcom_geni_serial: Add interconnect support spi: spi-qcom-qspi: Add interconnect support arm64: dts: sc7180: Add interconnect for QUP and QSPI arch/arm64/boot/dts/qcom/sc7180.dtsi | 127 +++++++++++++++++++++++++++++ drivers/i2c/busses/i2c-qcom-geni.c | 30 ++++++- drivers/interconnect/core.c | 25 ++++++ drivers/soc/qcom/qcom-geni-se.c | 149 ++++++++++++++++++++++++++++++++++ drivers/spi/spi-geni-qcom.c | 31 ++++++- drivers/spi/spi-qcom-qspi.c | 46 ++++++++++- drivers/tty/serial/qcom_geni_serial.c | 35 +++++++- include/linux/interconnect.h | 7 ++ include/linux/qcom-geni-se.h | 38 +++++++++ 9 files changed, 482 insertions(+), 6 deletions(-)