| Message ID | 20211011141733.3999-3-stephan@gerhold.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: wwan: Add Qualcomm BAM-DMUX WWAN network driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Series has a cover letter |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/checkpatch | warning | CHECK: Alignment should match open parenthesis WARNING: 'threshhold' may be misspelled - perhaps 'threshold'? |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
On Mon 11 Oct 07:17 PDT 2021, Stephan Gerhold wrote: > In some configurations, the BAM DMA controller is set up by a remote > processor and the local processor can simply start making use of it > without setting up the BAM. This is already supported using the > "qcom,controlled-remotely" property. > > However, for some reason another possible configuration is that the > remote processor is responsible for powering up the BAM, but we are > still responsible for initializing it (e.g. resetting it etc). > > This configuration is quite challenging to handle properly because > the power control is handled through separate channels > (e.g. device-specific SMSM interrupts / smem-states). Great care > must be taken to ensure the BAM registers are not accessed while > the BAM is powered off since this results in a bus stall. > > Attempt to support this configuration with minimal device-specific > code in the bam_dma driver by tracking the number of requested > channels. Consumers of DMA channels are responsible to only request > DMA channels when the BAM was powered on by the remote processor, > and to release them before the BAM is powered off. > > When the first channel is requested the BAM is initialized (reset) > and it is also put into reset when the last channel was released. > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > --- > Changes since RFC: > - Drop qcom-specific terminology "power collapse", instead rename > "qcom,remote-power-collapse" -> "qcom,powered-remotely" > > NOTE: This is *not* a compile-time requirement for the BAM-DMUX driver > so this could also go through the dmaengine tree. > > See original RFC for a discussion of alternative approaches to handle > this configuration: > https://lore.kernel.org/netdev/20210719145317.79692-3-stephan@gerhold.net/ > --- > drivers/dma/qcom/bam_dma.c | 88 ++++++++++++++++++++++++-------------- > 1 file changed, 56 insertions(+), 32 deletions(-) > > diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c > index c8a77b428b52..1b33a3ebbfec 100644 > --- a/drivers/dma/qcom/bam_dma.c > +++ b/drivers/dma/qcom/bam_dma.c > @@ -388,6 +388,8 @@ struct bam_device { > /* execution environment ID, from DT */ > u32 ee; > bool controlled_remotely; > + bool powered_remotely; > + u32 active_channels; > > const struct reg_offset_data *layout; > > @@ -415,6 +417,44 @@ static inline void __iomem *bam_addr(struct bam_device *bdev, u32 pipe, > r.ee_mult * bdev->ee; > } > > +/** > + * bam_reset - reset and initialize BAM registers Please include a set of () after the function name. > + * @bdev: bam device > + */ > +static void bam_reset(struct bam_device *bdev) > +{ > + u32 val; > + > + /* s/w reset bam */ > + /* after reset all pipes are disabled and idle */ > + val = readl_relaxed(bam_addr(bdev, 0, BAM_CTRL)); > + val |= BAM_SW_RST; > + writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); > + val &= ~BAM_SW_RST; > + writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); Seems odd to me that we assert and deassert the reset in back-to-back writes, without any delay etc. That said, this is unrelated to your patch as you just moved this hunk from below. > + > + /* make sure previous stores are visible before enabling BAM */ > + wmb(); > + > + /* enable bam */ > + val |= BAM_EN; > + writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); > + > + /* set descriptor threshhold, start with 4 bytes */ > + writel_relaxed(DEFAULT_CNT_THRSHLD, > + bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD)); > + > + /* Enable default set of h/w workarounds, ie all except BAM_FULL_PIPE */ > + writel_relaxed(BAM_CNFG_BITS_DEFAULT, bam_addr(bdev, 0, BAM_CNFG_BITS)); > + > + /* enable irqs for errors */ > + writel_relaxed(BAM_ERROR_EN | BAM_HRESP_ERR_EN, > + bam_addr(bdev, 0, BAM_IRQ_EN)); > + > + /* unmask global bam interrupt */ > + writel_relaxed(BAM_IRQ_MSK, bam_addr(bdev, 0, BAM_IRQ_SRCS_MSK_EE)); > +} > + > /** > * bam_reset_channel - Reset individual BAM DMA channel > * @bchan: bam channel > @@ -512,6 +552,9 @@ static int bam_alloc_chan(struct dma_chan *chan) > return -ENOMEM; > } > > + if (bdev->active_channels++ == 0 && bdev->powered_remotely) > + bam_reset(bdev); > + > return 0; > } > > @@ -565,6 +608,13 @@ static void bam_free_chan(struct dma_chan *chan) > /* disable irq */ > writel_relaxed(0, bam_addr(bdev, bchan->id, BAM_P_IRQ_EN)); > > + if (--bdev->active_channels == 0 && bdev->powered_remotely) { > + /* s/w reset bam */ > + val = readl_relaxed(bam_addr(bdev, 0, BAM_CTRL)); > + val |= BAM_SW_RST; > + writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); > + } > + > err: > pm_runtime_mark_last_busy(bdev->dev); > pm_runtime_put_autosuspend(bdev->dev); > @@ -1164,38 +1214,10 @@ static int bam_init(struct bam_device *bdev) > bdev->num_channels = val & BAM_NUM_PIPES_MASK; > } > > - if (bdev->controlled_remotely) > + if (bdev->controlled_remotely || bdev->powered_remotely) > return 0; I think the resulting code would be cleaner if you flipped it around as: /* Reset BAM now if fully controlled locally */ if (!bdev->controlled_remotely && !bdev->powered_remotely) bam_reset(bdev); return 0; Regards, Bjorn > > - /* s/w reset bam */ > - /* after reset all pipes are disabled and idle */ > - val = readl_relaxed(bam_addr(bdev, 0, BAM_CTRL)); > - val |= BAM_SW_RST; > - writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); > - val &= ~BAM_SW_RST; > - writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); > - > - /* make sure previous stores are visible before enabling BAM */ > - wmb(); > - > - /* enable bam */ > - val |= BAM_EN; > - writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); > - > - /* set descriptor threshhold, start with 4 bytes */ > - writel_relaxed(DEFAULT_CNT_THRSHLD, > - bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD)); > - > - /* Enable default set of h/w workarounds, ie all except BAM_FULL_PIPE */ > - writel_relaxed(BAM_CNFG_BITS_DEFAULT, bam_addr(bdev, 0, BAM_CNFG_BITS)); > - > - /* enable irqs for errors */ > - writel_relaxed(BAM_ERROR_EN | BAM_HRESP_ERR_EN, > - bam_addr(bdev, 0, BAM_IRQ_EN)); > - > - /* unmask global bam interrupt */ > - writel_relaxed(BAM_IRQ_MSK, bam_addr(bdev, 0, BAM_IRQ_SRCS_MSK_EE)); > - > + bam_reset(bdev); > return 0; > }
On Mon, Oct 11, 2021 at 10:39:20AM -0700, Bjorn Andersson wrote: > On Mon 11 Oct 07:17 PDT 2021, Stephan Gerhold wrote: > > > In some configurations, the BAM DMA controller is set up by a remote > > processor and the local processor can simply start making use of it > > without setting up the BAM. This is already supported using the > > "qcom,controlled-remotely" property. > > > > However, for some reason another possible configuration is that the > > remote processor is responsible for powering up the BAM, but we are > > still responsible for initializing it (e.g. resetting it etc). > > > > This configuration is quite challenging to handle properly because > > the power control is handled through separate channels > > (e.g. device-specific SMSM interrupts / smem-states). Great care > > must be taken to ensure the BAM registers are not accessed while > > the BAM is powered off since this results in a bus stall. > > > > Attempt to support this configuration with minimal device-specific > > code in the bam_dma driver by tracking the number of requested > > channels. Consumers of DMA channels are responsible to only request > > DMA channels when the BAM was powered on by the remote processor, > > and to release them before the BAM is powered off. > > > > When the first channel is requested the BAM is initialized (reset) > > and it is also put into reset when the last channel was released. > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > > --- > > Changes since RFC: > > - Drop qcom-specific terminology "power collapse", instead rename > > "qcom,remote-power-collapse" -> "qcom,powered-remotely" > > > > NOTE: This is *not* a compile-time requirement for the BAM-DMUX driver > > so this could also go through the dmaengine tree. > > > > See original RFC for a discussion of alternative approaches to handle > > this configuration: > > https://lore.kernel.org/netdev/20210719145317.79692-3-stephan@gerhold.net/ > > --- > > drivers/dma/qcom/bam_dma.c | 88 ++++++++++++++++++++++++-------------- > > 1 file changed, 56 insertions(+), 32 deletions(-) > > > > diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c > > index c8a77b428b52..1b33a3ebbfec 100644 > > --- a/drivers/dma/qcom/bam_dma.c > > +++ b/drivers/dma/qcom/bam_dma.c > > @@ -388,6 +388,8 @@ struct bam_device { > > /* execution environment ID, from DT */ > > u32 ee; > > bool controlled_remotely; > > + bool powered_remotely; > > + u32 active_channels; > > > > const struct reg_offset_data *layout; > > > > @@ -415,6 +417,44 @@ static inline void __iomem *bam_addr(struct bam_device *bdev, u32 pipe, > > r.ee_mult * bdev->ee; > > } > > > > +/** > > + * bam_reset - reset and initialize BAM registers > > Please include a set of () after the function name. > Thanks, will fix this in v3. > > + * @bdev: bam device > > + */ > > +static void bam_reset(struct bam_device *bdev) > > +{ > > + u32 val; > > + > > + /* s/w reset bam */ > > + /* after reset all pipes are disabled and idle */ > > + val = readl_relaxed(bam_addr(bdev, 0, BAM_CTRL)); > > + val |= BAM_SW_RST; > > + writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); > > + val &= ~BAM_SW_RST; > > + writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); > > Seems odd to me that we assert and deassert the reset in back-to-back > writes, without any delay etc. That said, this is unrelated to your > patch as you just moved this hunk from below. > It seems to work fine though. :) I'm pretty sure I checked that this actually resets at some point, but perhaps I was lucky there. But yeah, seems better to adjust this in some future patch instead of here. > > + > > + /* make sure previous stores are visible before enabling BAM */ > > + wmb(); > > + > > + /* enable bam */ > > + val |= BAM_EN; > > + writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); > > + > > + /* set descriptor threshhold, start with 4 bytes */ > > + writel_relaxed(DEFAULT_CNT_THRSHLD, > > + bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD)); > > + > > + /* Enable default set of h/w workarounds, ie all except BAM_FULL_PIPE */ > > + writel_relaxed(BAM_CNFG_BITS_DEFAULT, bam_addr(bdev, 0, BAM_CNFG_BITS)); > > + > > + /* enable irqs for errors */ > > + writel_relaxed(BAM_ERROR_EN | BAM_HRESP_ERR_EN, > > + bam_addr(bdev, 0, BAM_IRQ_EN)); > > + > > + /* unmask global bam interrupt */ > > + writel_relaxed(BAM_IRQ_MSK, bam_addr(bdev, 0, BAM_IRQ_SRCS_MSK_EE)); > > +} > > + > > /** > > * bam_reset_channel - Reset individual BAM DMA channel > > * @bchan: bam channel > > @@ -512,6 +552,9 @@ static int bam_alloc_chan(struct dma_chan *chan) > > return -ENOMEM; > > } > > > > + if (bdev->active_channels++ == 0 && bdev->powered_remotely) > > + bam_reset(bdev); > > + > > return 0; > > } > > > > @@ -565,6 +608,13 @@ static void bam_free_chan(struct dma_chan *chan) > > /* disable irq */ > > writel_relaxed(0, bam_addr(bdev, bchan->id, BAM_P_IRQ_EN)); > > > > + if (--bdev->active_channels == 0 && bdev->powered_remotely) { > > + /* s/w reset bam */ > > + val = readl_relaxed(bam_addr(bdev, 0, BAM_CTRL)); > > + val |= BAM_SW_RST; > > + writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); > > + } > > + > > err: > > pm_runtime_mark_last_busy(bdev->dev); > > pm_runtime_put_autosuspend(bdev->dev); > > @@ -1164,38 +1214,10 @@ static int bam_init(struct bam_device *bdev) > > bdev->num_channels = val & BAM_NUM_PIPES_MASK; > > } > > > > - if (bdev->controlled_remotely) > > + if (bdev->controlled_remotely || bdev->powered_remotely) > > return 0; > > I think the resulting code would be cleaner if you flipped it around as: > > /* Reset BAM now if fully controlled locally */ > if (!bdev->controlled_remotely && !bdev->powered_remotely) > bam_reset(bdev); > > return 0; > Hmm, you are probably right, I can flip it in v3. Thanks! Stephan
diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c index c8a77b428b52..1b33a3ebbfec 100644 --- a/drivers/dma/qcom/bam_dma.c +++ b/drivers/dma/qcom/bam_dma.c @@ -388,6 +388,8 @@ struct bam_device { /* execution environment ID, from DT */ u32 ee; bool controlled_remotely; + bool powered_remotely; + u32 active_channels; const struct reg_offset_data *layout; @@ -415,6 +417,44 @@ static inline void __iomem *bam_addr(struct bam_device *bdev, u32 pipe, r.ee_mult * bdev->ee; } +/** + * bam_reset - reset and initialize BAM registers + * @bdev: bam device + */ +static void bam_reset(struct bam_device *bdev) +{ + u32 val; + + /* s/w reset bam */ + /* after reset all pipes are disabled and idle */ + val = readl_relaxed(bam_addr(bdev, 0, BAM_CTRL)); + val |= BAM_SW_RST; + writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); + val &= ~BAM_SW_RST; + writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); + + /* make sure previous stores are visible before enabling BAM */ + wmb(); + + /* enable bam */ + val |= BAM_EN; + writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); + + /* set descriptor threshhold, start with 4 bytes */ + writel_relaxed(DEFAULT_CNT_THRSHLD, + bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD)); + + /* Enable default set of h/w workarounds, ie all except BAM_FULL_PIPE */ + writel_relaxed(BAM_CNFG_BITS_DEFAULT, bam_addr(bdev, 0, BAM_CNFG_BITS)); + + /* enable irqs for errors */ + writel_relaxed(BAM_ERROR_EN | BAM_HRESP_ERR_EN, + bam_addr(bdev, 0, BAM_IRQ_EN)); + + /* unmask global bam interrupt */ + writel_relaxed(BAM_IRQ_MSK, bam_addr(bdev, 0, BAM_IRQ_SRCS_MSK_EE)); +} + /** * bam_reset_channel - Reset individual BAM DMA channel * @bchan: bam channel @@ -512,6 +552,9 @@ static int bam_alloc_chan(struct dma_chan *chan) return -ENOMEM; } + if (bdev->active_channels++ == 0 && bdev->powered_remotely) + bam_reset(bdev); + return 0; } @@ -565,6 +608,13 @@ static void bam_free_chan(struct dma_chan *chan) /* disable irq */ writel_relaxed(0, bam_addr(bdev, bchan->id, BAM_P_IRQ_EN)); + if (--bdev->active_channels == 0 && bdev->powered_remotely) { + /* s/w reset bam */ + val = readl_relaxed(bam_addr(bdev, 0, BAM_CTRL)); + val |= BAM_SW_RST; + writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); + } + err: pm_runtime_mark_last_busy(bdev->dev); pm_runtime_put_autosuspend(bdev->dev); @@ -1164,38 +1214,10 @@ static int bam_init(struct bam_device *bdev) bdev->num_channels = val & BAM_NUM_PIPES_MASK; } - if (bdev->controlled_remotely) + if (bdev->controlled_remotely || bdev->powered_remotely) return 0; - /* s/w reset bam */ - /* after reset all pipes are disabled and idle */ - val = readl_relaxed(bam_addr(bdev, 0, BAM_CTRL)); - val |= BAM_SW_RST; - writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); - val &= ~BAM_SW_RST; - writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); - - /* make sure previous stores are visible before enabling BAM */ - wmb(); - - /* enable bam */ - val |= BAM_EN; - writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); - - /* set descriptor threshhold, start with 4 bytes */ - writel_relaxed(DEFAULT_CNT_THRSHLD, - bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD)); - - /* Enable default set of h/w workarounds, ie all except BAM_FULL_PIPE */ - writel_relaxed(BAM_CNFG_BITS_DEFAULT, bam_addr(bdev, 0, BAM_CNFG_BITS)); - - /* enable irqs for errors */ - writel_relaxed(BAM_ERROR_EN | BAM_HRESP_ERR_EN, - bam_addr(bdev, 0, BAM_IRQ_EN)); - - /* unmask global bam interrupt */ - writel_relaxed(BAM_IRQ_MSK, bam_addr(bdev, 0, BAM_IRQ_SRCS_MSK_EE)); - + bam_reset(bdev); return 0; } @@ -1257,8 +1279,10 @@ static int bam_dma_probe(struct platform_device *pdev) bdev->controlled_remotely = of_property_read_bool(pdev->dev.of_node, "qcom,controlled-remotely"); + bdev->powered_remotely = of_property_read_bool(pdev->dev.of_node, + "qcom,powered-remotely"); - if (bdev->controlled_remotely) { + if (bdev->controlled_remotely || bdev->powered_remotely) { ret = of_property_read_u32(pdev->dev.of_node, "num-channels", &bdev->num_channels); if (ret) @@ -1270,7 +1294,7 @@ static int bam_dma_probe(struct platform_device *pdev) dev_err(bdev->dev, "num-ees unspecified in dt\n"); } - if (bdev->controlled_remotely) + if (bdev->controlled_remotely || bdev->powered_remotely) bdev->bamclk = devm_clk_get_optional(bdev->dev, "bam_clk"); else bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk");
In some configurations, the BAM DMA controller is set up by a remote processor and the local processor can simply start making use of it without setting up the BAM. This is already supported using the "qcom,controlled-remotely" property. However, for some reason another possible configuration is that the remote processor is responsible for powering up the BAM, but we are still responsible for initializing it (e.g. resetting it etc). This configuration is quite challenging to handle properly because the power control is handled through separate channels (e.g. device-specific SMSM interrupts / smem-states). Great care must be taken to ensure the BAM registers are not accessed while the BAM is powered off since this results in a bus stall. Attempt to support this configuration with minimal device-specific code in the bam_dma driver by tracking the number of requested channels. Consumers of DMA channels are responsible to only request DMA channels when the BAM was powered on by the remote processor, and to release them before the BAM is powered off. When the first channel is requested the BAM is initialized (reset) and it is also put into reset when the last channel was released. Signed-off-by: Stephan Gerhold <stephan@gerhold.net> --- Changes since RFC: - Drop qcom-specific terminology "power collapse", instead rename "qcom,remote-power-collapse" -> "qcom,powered-remotely" NOTE: This is *not* a compile-time requirement for the BAM-DMUX driver so this could also go through the dmaengine tree. See original RFC for a discussion of alternative approaches to handle this configuration: https://lore.kernel.org/netdev/20210719145317.79692-3-stephan@gerhold.net/ --- drivers/dma/qcom/bam_dma.c | 88 ++++++++++++++++++++++++-------------- 1 file changed, 56 insertions(+), 32 deletions(-)