| Message ID | 20250212-bam-dma-fixes-v1-7-f560889e65d8@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | dmaengine: qcom: bam_dma: Fix DT error handling for num-channels/ees | expand |
On 12.02.2025 6:03 PM, Stephan Gerhold wrote: > num-channels and qcom,num-ees are required when there are no clocks > specified in the device tree, because we have no reliable way to read them > from the hardware registers if we cannot ensure the BAM hardware is up when > the device is being probed. > > This has often been forgotten when adding new SoC device trees, so make > this clear by describing this requirement in the schema. > > Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> > --- > Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > index 3ad0d9b1fbc5e4f83dd316d1ad79773c288748ba..5f7e7763615578717651014cfd52745ea2132115 100644 > --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > @@ -90,8 +90,12 @@ required: > anyOf: > - required: > - qcom,powered-remotely > + - num-channels > + - qcom,num-ees > - required: > - qcom,controlled-remotely > + - num-channels > + - qcom,num-ees I think I'd rather see these deprecated and add the clock everywhere.. Do we know which one we need to add on newer platforms? Or maybe it's been transformed into an icc path? Reading back things from this piece of HW only to add it to DT to avoid reading it later is a really messy solution. Konrad > - required: > - clocks > - clock-names >
On Wed, Feb 12, 2025 at 10:01:59PM +0100, Konrad Dybcio wrote: > On 12.02.2025 6:03 PM, Stephan Gerhold wrote: > > num-channels and qcom,num-ees are required when there are no clocks > > specified in the device tree, because we have no reliable way to read them > > from the hardware registers if we cannot ensure the BAM hardware is up when > > the device is being probed. > > > > This has often been forgotten when adding new SoC device trees, so make > > this clear by describing this requirement in the schema. > > > > Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> > > --- > > Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > > index 3ad0d9b1fbc5e4f83dd316d1ad79773c288748ba..5f7e7763615578717651014cfd52745ea2132115 100644 > > --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > > +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > > @@ -90,8 +90,12 @@ required: > > anyOf: > > - required: > > - qcom,powered-remotely > > + - num-channels > > + - qcom,num-ees > > - required: > > - qcom,controlled-remotely > > + - num-channels > > + - qcom,num-ees > > I think I'd rather see these deprecated and add the clock everywhere.. > Do we know which one we need to add on newer platforms? Or maybe it's > been transformed into an icc path? This isn't feasible, there are too many different setups. Also often the BAM power management is tightly integrated into the consumer interface. To give a short excerpt (I'm sure there are even more obscure uses): - BLSP BAM (UART, I2C, SPI on older SoCs): 1. Enable GCC_BLSP_AHB_CLK -> This is what the bam_dma driver supports currently. - Crypto BAM: Either OR 1. Vote for single RPM clock OR 1. Enable 3 separate clocks (CE, CE_AHB, CE_AXI) OR 1. Vote dummy bandwidth for interconnect - BAM DMUX (WWAN on older SoCs): 1. Start modem firmware 2. Wait for BAM DMUX service to be up 3. Vote for power up via the BAM-DMUX-specific SMEM state 4. Hope the firmware agrees and brings up the BAM - SLIMbus BAM (audio on some SoCs): 1. Start ADSP firmware 2. Wait for QMI SLIMBUS service to be up via QRTR 3. Vote for power up via SLIMbus-specific QMI messages 4. Hope the firmware agrees and brings up the BAM Especially for the last two, we can't implement support for those consumer-specific interfaces in the BAM driver. Implementing support for the 3 variants of the Crypto BAM would be possible, but it's honestly the least interesting use case of all these. It's not really clear why we are bothing with the crypto engine on newer SoCs at all, see e.g. [1]. [1]: https://lore.kernel.org/linux-arm-msm/20250118080604.GA721573@sol.localdomain/ > Reading back things from this piece of HW only to add it to DT to avoid > reading it later is a really messy solution. In retrospect, it could have been cleaner to avoid describing the BAM as device node independent of the consumer. We wouldn't have this problem if the BAM driver would only probe when the consumer is already ready. But I think specifying num-channels in the device tree is the cleanest way out of this mess. I have a second patch series ready that drops qcom,num-ees and validates the num-channels once it's safe reading from the BAM registers. That way, you just need one boot test to ensure the device tree description is really correct. Thanks, Stephan
On 13.02.2025 10:13 AM, Stephan Gerhold wrote: > On Wed, Feb 12, 2025 at 10:01:59PM +0100, Konrad Dybcio wrote: >> On 12.02.2025 6:03 PM, Stephan Gerhold wrote: >>> num-channels and qcom,num-ees are required when there are no clocks >>> specified in the device tree, because we have no reliable way to read them >>> from the hardware registers if we cannot ensure the BAM hardware is up when >>> the device is being probed. >>> >>> This has often been forgotten when adding new SoC device trees, so make >>> this clear by describing this requirement in the schema. >>> >>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> >>> --- >>> Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >>> index 3ad0d9b1fbc5e4f83dd316d1ad79773c288748ba..5f7e7763615578717651014cfd52745ea2132115 100644 >>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >>> @@ -90,8 +90,12 @@ required: >>> anyOf: >>> - required: >>> - qcom,powered-remotely >>> + - num-channels >>> + - qcom,num-ees >>> - required: >>> - qcom,controlled-remotely >>> + - num-channels >>> + - qcom,num-ees >> >> I think I'd rather see these deprecated and add the clock everywhere.. >> Do we know which one we need to add on newer platforms? Or maybe it's >> been transformed into an icc path? > > This isn't feasible, there are too many different setups. Also often the > BAM power management is tightly integrated into the consumer interface. > To give a short excerpt (I'm sure there are even more obscure uses): > > - BLSP BAM (UART, I2C, SPI on older SoCs): > 1. Enable GCC_BLSP_AHB_CLK > -> This is what the bam_dma driver supports currently. > > - Crypto BAM: Either > OR 1. Vote for single RPM clock > OR 1. Enable 3 separate clocks (CE, CE_AHB, CE_AXI) > OR 1. Vote dummy bandwidth for interconnect > > - BAM DMUX (WWAN on older SoCs): > 1. Start modem firmware > 2. Wait for BAM DMUX service to be up > 3. Vote for power up via the BAM-DMUX-specific SMEM state > 4. Hope the firmware agrees and brings up the BAM > > - SLIMbus BAM (audio on some SoCs): > 1. Start ADSP firmware > 2. Wait for QMI SLIMBUS service to be up via QRTR > 3. Vote for power up via SLIMbus-specific QMI messages > 4. Hope the firmware agrees and brings up the BAM > > Especially for the last two, we can't implement support for those > consumer-specific interfaces in the BAM driver. Implementing support for > the 3 variants of the Crypto BAM would be possible, but it's honestly > the least interesting use case of all these. It's not really clear why > we are bothing with the crypto engine on newer SoCs at all, see e.g. [1]. > > [1]: https://lore.kernel.org/linux-arm-msm/20250118080604.GA721573@sol.localdomain/ > >> Reading back things from this piece of HW only to add it to DT to avoid >> reading it later is a really messy solution. > > In retrospect, it could have been cleaner to avoid describing the BAM as > device node independent of the consumer. We wouldn't have this problem > if the BAM driver would only probe when the consumer is already ready. > > But I think specifying num-channels in the device tree is the cleanest > way out of this mess. I have a second patch series ready that drops > qcom,num-ees and validates the num-channels once it's safe reading from > the BAM registers. That way, you just need one boot test to ensure the > device tree description is really correct. Thanks for the detailed explanation! Do you think it could maybe make sense to expose a clock/power-domain from the modem/adsp rproc and feed it to the DMUX / SLIM instances when an appropriate ping arrives? This way we'd also defer probing the drivers until the device is actually accessible. Konrad
On Thu, Feb 13, 2025 at 03:00:00PM +0100, Konrad Dybcio wrote: > On 13.02.2025 10:13 AM, Stephan Gerhold wrote: > > On Wed, Feb 12, 2025 at 10:01:59PM +0100, Konrad Dybcio wrote: > >> On 12.02.2025 6:03 PM, Stephan Gerhold wrote: > >>> num-channels and qcom,num-ees are required when there are no clocks > >>> specified in the device tree, because we have no reliable way to read them > >>> from the hardware registers if we cannot ensure the BAM hardware is up when > >>> the device is being probed. > >>> > >>> This has often been forgotten when adding new SoC device trees, so make > >>> this clear by describing this requirement in the schema. > >>> > >>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> > >>> --- > >>> Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 4 ++++ > >>> 1 file changed, 4 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > >>> index 3ad0d9b1fbc5e4f83dd316d1ad79773c288748ba..5f7e7763615578717651014cfd52745ea2132115 100644 > >>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > >>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > >>> @@ -90,8 +90,12 @@ required: > >>> anyOf: > >>> - required: > >>> - qcom,powered-remotely > >>> + - num-channels > >>> + - qcom,num-ees > >>> - required: > >>> - qcom,controlled-remotely > >>> + - num-channels > >>> + - qcom,num-ees > >> > >> I think I'd rather see these deprecated and add the clock everywhere.. > >> Do we know which one we need to add on newer platforms? Or maybe it's > >> been transformed into an icc path? > > > > This isn't feasible, there are too many different setups. Also often the > > BAM power management is tightly integrated into the consumer interface. > > To give a short excerpt (I'm sure there are even more obscure uses): > > > > - BLSP BAM (UART, I2C, SPI on older SoCs): > > 1. Enable GCC_BLSP_AHB_CLK > > -> This is what the bam_dma driver supports currently. > > > > - Crypto BAM: Either > > OR 1. Vote for single RPM clock > > OR 1. Enable 3 separate clocks (CE, CE_AHB, CE_AXI) > > OR 1. Vote dummy bandwidth for interconnect > > > > - BAM DMUX (WWAN on older SoCs): > > 1. Start modem firmware > > 2. Wait for BAM DMUX service to be up > > 3. Vote for power up via the BAM-DMUX-specific SMEM state > > 4. Hope the firmware agrees and brings up the BAM > > > > - SLIMbus BAM (audio on some SoCs): > > 1. Start ADSP firmware > > 2. Wait for QMI SLIMBUS service to be up via QRTR > > 3. Vote for power up via SLIMbus-specific QMI messages > > 4. Hope the firmware agrees and brings up the BAM > > > > Especially for the last two, we can't implement support for those > > consumer-specific interfaces in the BAM driver. Implementing support for > > the 3 variants of the Crypto BAM would be possible, but it's honestly > > the least interesting use case of all these. It's not really clear why > > we are bothing with the crypto engine on newer SoCs at all, see e.g. [1]. > > > > [1]: https://lore.kernel.org/linux-arm-msm/20250118080604.GA721573@sol.localdomain/ > > > >> Reading back things from this piece of HW only to add it to DT to avoid > >> reading it later is a really messy solution. > > > > In retrospect, it could have been cleaner to avoid describing the BAM as > > device node independent of the consumer. We wouldn't have this problem > > if the BAM driver would only probe when the consumer is already ready. > > > > But I think specifying num-channels in the device tree is the cleanest > > way out of this mess. I have a second patch series ready that drops > > qcom,num-ees and validates the num-channels once it's safe reading from > > the BAM registers. That way, you just need one boot test to ensure the > > device tree description is really correct. > > Thanks for the detailed explanation! > > Do you think it could maybe make sense to expose a clock/power-domain > from the modem/adsp rproc and feed it to the DMUX / SLIM instances when > an appropriate ping arrives? This way we'd also defer probing the drivers > until the device is actually accessible. > Maybe, but that would result in a cyclic dependency between the DMA provider and consumer. E.g. bam_dmux_dma: dma-controller@ { #dma-cells = <1>; power-domains = <&bam_dmux>; }; remoteproc@ { /* ... */ bam_dmux: bam-dmux { dmas = <&bam_dmux_dma 4>, <&bam_dmux_dma 5>; dma-names = "tx", "rx"; }; }; fw_devlink will likely get confused by that. At the end my thought process here is the following: 1. BAM-DMA is a legacy block at this point, it doesn't look like there are any new use cases being added on new SoCs 2. We need to preserve compatibility with the old bindings anyway 3. I trimmed it down to having to specify just "num-channels" 4. Everything else is read from the hardware registers, and num-channels gets validated when the first DMA channel is requested I think it's the best we can do here at this point. Thanks, Stephan
On 13.02.2025 4:22 PM, Stephan Gerhold wrote: > On Thu, Feb 13, 2025 at 03:00:00PM +0100, Konrad Dybcio wrote: >> On 13.02.2025 10:13 AM, Stephan Gerhold wrote: >>> On Wed, Feb 12, 2025 at 10:01:59PM +0100, Konrad Dybcio wrote: >>>> On 12.02.2025 6:03 PM, Stephan Gerhold wrote: >>>>> num-channels and qcom,num-ees are required when there are no clocks >>>>> specified in the device tree, because we have no reliable way to read them >>>>> from the hardware registers if we cannot ensure the BAM hardware is up when >>>>> the device is being probed. >>>>> >>>>> This has often been forgotten when adding new SoC device trees, so make >>>>> this clear by describing this requirement in the schema. >>>>> >>>>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> >>>>> --- >>>>> Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >>>>> index 3ad0d9b1fbc5e4f83dd316d1ad79773c288748ba..5f7e7763615578717651014cfd52745ea2132115 100644 >>>>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >>>>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >>>>> @@ -90,8 +90,12 @@ required: >>>>> anyOf: >>>>> - required: >>>>> - qcom,powered-remotely >>>>> + - num-channels >>>>> + - qcom,num-ees >>>>> - required: >>>>> - qcom,controlled-remotely >>>>> + - num-channels >>>>> + - qcom,num-ees >>>> >>>> I think I'd rather see these deprecated and add the clock everywhere.. >>>> Do we know which one we need to add on newer platforms? Or maybe it's >>>> been transformed into an icc path? >>> >>> This isn't feasible, there are too many different setups. Also often the >>> BAM power management is tightly integrated into the consumer interface. >>> To give a short excerpt (I'm sure there are even more obscure uses): >>> >>> - BLSP BAM (UART, I2C, SPI on older SoCs): >>> 1. Enable GCC_BLSP_AHB_CLK >>> -> This is what the bam_dma driver supports currently. >>> >>> - Crypto BAM: Either >>> OR 1. Vote for single RPM clock >>> OR 1. Enable 3 separate clocks (CE, CE_AHB, CE_AXI) >>> OR 1. Vote dummy bandwidth for interconnect >>> >>> - BAM DMUX (WWAN on older SoCs): >>> 1. Start modem firmware >>> 2. Wait for BAM DMUX service to be up >>> 3. Vote for power up via the BAM-DMUX-specific SMEM state >>> 4. Hope the firmware agrees and brings up the BAM >>> >>> - SLIMbus BAM (audio on some SoCs): >>> 1. Start ADSP firmware >>> 2. Wait for QMI SLIMBUS service to be up via QRTR >>> 3. Vote for power up via SLIMbus-specific QMI messages >>> 4. Hope the firmware agrees and brings up the BAM >>> >>> Especially for the last two, we can't implement support for those >>> consumer-specific interfaces in the BAM driver. Implementing support for >>> the 3 variants of the Crypto BAM would be possible, but it's honestly >>> the least interesting use case of all these. It's not really clear why >>> we are bothing with the crypto engine on newer SoCs at all, see e.g. [1]. >>> >>> [1]: https://lore.kernel.org/linux-arm-msm/20250118080604.GA721573@sol.localdomain/ >>> >>>> Reading back things from this piece of HW only to add it to DT to avoid >>>> reading it later is a really messy solution. >>> >>> In retrospect, it could have been cleaner to avoid describing the BAM as >>> device node independent of the consumer. We wouldn't have this problem >>> if the BAM driver would only probe when the consumer is already ready. >>> >>> But I think specifying num-channels in the device tree is the cleanest >>> way out of this mess. I have a second patch series ready that drops >>> qcom,num-ees and validates the num-channels once it's safe reading from >>> the BAM registers. That way, you just need one boot test to ensure the >>> device tree description is really correct. >> >> Thanks for the detailed explanation! >> >> Do you think it could maybe make sense to expose a clock/power-domain >> from the modem/adsp rproc and feed it to the DMUX / SLIM instances when >> an appropriate ping arrives? This way we'd also defer probing the drivers >> until the device is actually accessible. >> > > Maybe, but that would result in a cyclic dependency between the DMA > provider and consumer. E.g. > > bam_dmux_dma: dma-controller@ { > #dma-cells = <1>; > power-domains = <&bam_dmux>; > }; > > remoteproc@ { > /* ... */ > > bam_dmux: bam-dmux { > dmas = <&bam_dmux_dma 4>, <&bam_dmux_dma 5>; > dma-names = "tx", "rx"; > }; > }; > > fw_devlink will likely get confused by that. > > At the end my thought process here is the following: > > 1. BAM-DMA is a legacy block at this point, it doesn't look like there > are any new use cases being added on new SoCs > 2. We need to preserve compatibility with the old bindings anyway > 3. I trimmed it down to having to specify just "num-channels" > 4. Everything else is read from the hardware registers, and > num-channels gets validated when the first DMA channel is requested > > I think it's the best we can do here at this point. Alright, let's go this route then. Konrad
On Thu, Feb 13, 2025 at 04:22:17PM +0100, Stephan Gerhold wrote: > On Thu, Feb 13, 2025 at 03:00:00PM +0100, Konrad Dybcio wrote: > > On 13.02.2025 10:13 AM, Stephan Gerhold wrote: > > > On Wed, Feb 12, 2025 at 10:01:59PM +0100, Konrad Dybcio wrote: > > >> On 12.02.2025 6:03 PM, Stephan Gerhold wrote: > > >>> num-channels and qcom,num-ees are required when there are no clocks > > >>> specified in the device tree, because we have no reliable way to read them > > >>> from the hardware registers if we cannot ensure the BAM hardware is up when > > >>> the device is being probed. > > >>> > > >>> This has often been forgotten when adding new SoC device trees, so make > > >>> this clear by describing this requirement in the schema. > > >>> > > >>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> > > >>> --- > > >>> Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 4 ++++ > > >>> 1 file changed, 4 insertions(+) > > >>> > > >>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > > >>> index 3ad0d9b1fbc5e4f83dd316d1ad79773c288748ba..5f7e7763615578717651014cfd52745ea2132115 100644 > > >>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > > >>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > > >>> @@ -90,8 +90,12 @@ required: > > >>> anyOf: > > >>> - required: > > >>> - qcom,powered-remotely > > >>> + - num-channels > > >>> + - qcom,num-ees > > >>> - required: > > >>> - qcom,controlled-remotely > > >>> + - num-channels > > >>> + - qcom,num-ees > > >> > > >> I think I'd rather see these deprecated and add the clock everywhere.. > > >> Do we know which one we need to add on newer platforms? Or maybe it's > > >> been transformed into an icc path? > > > > > > This isn't feasible, there are too many different setups. Also often the > > > BAM power management is tightly integrated into the consumer interface. > > > To give a short excerpt (I'm sure there are even more obscure uses): > > > > > > - BLSP BAM (UART, I2C, SPI on older SoCs): > > > 1. Enable GCC_BLSP_AHB_CLK > > > -> This is what the bam_dma driver supports currently. > > > > > > - Crypto BAM: Either > > > OR 1. Vote for single RPM clock > > > OR 1. Enable 3 separate clocks (CE, CE_AHB, CE_AXI) > > > OR 1. Vote dummy bandwidth for interconnect > > > > > > - BAM DMUX (WWAN on older SoCs): > > > 1. Start modem firmware > > > 2. Wait for BAM DMUX service to be up > > > 3. Vote for power up via the BAM-DMUX-specific SMEM state > > > 4. Hope the firmware agrees and brings up the BAM > > > > > > - SLIMbus BAM (audio on some SoCs): > > > 1. Start ADSP firmware > > > 2. Wait for QMI SLIMBUS service to be up via QRTR > > > 3. Vote for power up via SLIMbus-specific QMI messages > > > 4. Hope the firmware agrees and brings up the BAM > > > > > > Especially for the last two, we can't implement support for those > > > consumer-specific interfaces in the BAM driver. Implementing support for > > > the 3 variants of the Crypto BAM would be possible, but it's honestly > > > the least interesting use case of all these. It's not really clear why > > > we are bothing with the crypto engine on newer SoCs at all, see e.g. [1]. > > > > > > [1]: https://lore.kernel.org/linux-arm-msm/20250118080604.GA721573@sol.localdomain/ > > > > > >> Reading back things from this piece of HW only to add it to DT to avoid > > >> reading it later is a really messy solution. > > > > > > In retrospect, it could have been cleaner to avoid describing the BAM as > > > device node independent of the consumer. We wouldn't have this problem > > > if the BAM driver would only probe when the consumer is already ready. > > > > > > But I think specifying num-channels in the device tree is the cleanest > > > way out of this mess. I have a second patch series ready that drops > > > qcom,num-ees and validates the num-channels once it's safe reading from > > > the BAM registers. That way, you just need one boot test to ensure the > > > device tree description is really correct. > > > > Thanks for the detailed explanation! > > > > Do you think it could maybe make sense to expose a clock/power-domain > > from the modem/adsp rproc and feed it to the DMUX / SLIM instances when > > an appropriate ping arrives? This way we'd also defer probing the drivers > > until the device is actually accessible. > > > > Maybe, but that would result in a cyclic dependency between the DMA > provider and consumer. E.g. > > bam_dmux_dma: dma-controller@ { > #dma-cells = <1>; > power-domains = <&bam_dmux>; > }; > > remoteproc@ { > /* ... */ > > bam_dmux: bam-dmux { > dmas = <&bam_dmux_dma 4>, <&bam_dmux_dma 5>; > dma-names = "tx", "rx"; > }; > }; > > fw_devlink will likely get confused by that. Why? We have a property to break cycles: post-init-providers That doesn't work here? Rob
On Wed, Feb 19, 2025 at 04:27:39PM -0600, Rob Herring wrote: > On Thu, Feb 13, 2025 at 04:22:17PM +0100, Stephan Gerhold wrote: > > On Thu, Feb 13, 2025 at 03:00:00PM +0100, Konrad Dybcio wrote: > > > On 13.02.2025 10:13 AM, Stephan Gerhold wrote: > > > > On Wed, Feb 12, 2025 at 10:01:59PM +0100, Konrad Dybcio wrote: > > > >> On 12.02.2025 6:03 PM, Stephan Gerhold wrote: > > > >>> num-channels and qcom,num-ees are required when there are no clocks > > > >>> specified in the device tree, because we have no reliable way to read them > > > >>> from the hardware registers if we cannot ensure the BAM hardware is up when > > > >>> the device is being probed. > > > >>> > > > >>> This has often been forgotten when adding new SoC device trees, so make > > > >>> this clear by describing this requirement in the schema. > > > >>> > > > >>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> > > > >>> --- > > > >>> Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 4 ++++ > > > >>> 1 file changed, 4 insertions(+) > > > >>> > > > >>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > > > >>> index 3ad0d9b1fbc5e4f83dd316d1ad79773c288748ba..5f7e7763615578717651014cfd52745ea2132115 100644 > > > >>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > > > >>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > > > >>> @@ -90,8 +90,12 @@ required: > > > >>> anyOf: > > > >>> - required: > > > >>> - qcom,powered-remotely > > > >>> + - num-channels > > > >>> + - qcom,num-ees > > > >>> - required: > > > >>> - qcom,controlled-remotely > > > >>> + - num-channels > > > >>> + - qcom,num-ees > > > >> > > > >> I think I'd rather see these deprecated and add the clock everywhere.. > > > >> Do we know which one we need to add on newer platforms? Or maybe it's > > > >> been transformed into an icc path? > > > > > > > > This isn't feasible, there are too many different setups. Also often the > > > > BAM power management is tightly integrated into the consumer interface. > > > > To give a short excerpt (I'm sure there are even more obscure uses): > > > > > > > > - BLSP BAM (UART, I2C, SPI on older SoCs): > > > > 1. Enable GCC_BLSP_AHB_CLK > > > > -> This is what the bam_dma driver supports currently. > > > > > > > > - Crypto BAM: Either > > > > OR 1. Vote for single RPM clock > > > > OR 1. Enable 3 separate clocks (CE, CE_AHB, CE_AXI) > > > > OR 1. Vote dummy bandwidth for interconnect > > > > > > > > - BAM DMUX (WWAN on older SoCs): > > > > 1. Start modem firmware > > > > 2. Wait for BAM DMUX service to be up > > > > 3. Vote for power up via the BAM-DMUX-specific SMEM state > > > > 4. Hope the firmware agrees and brings up the BAM > > > > > > > > - SLIMbus BAM (audio on some SoCs): > > > > 1. Start ADSP firmware > > > > 2. Wait for QMI SLIMBUS service to be up via QRTR > > > > 3. Vote for power up via SLIMbus-specific QMI messages > > > > 4. Hope the firmware agrees and brings up the BAM > > > > > > > > Especially for the last two, we can't implement support for those > > > > consumer-specific interfaces in the BAM driver. Implementing support for > > > > the 3 variants of the Crypto BAM would be possible, but it's honestly > > > > the least interesting use case of all these. It's not really clear why > > > > we are bothing with the crypto engine on newer SoCs at all, see e.g. [1]. > > > > > > > > [1]: https://lore.kernel.org/linux-arm-msm/20250118080604.GA721573@sol.localdomain/ > > > > > > > >> Reading back things from this piece of HW only to add it to DT to avoid > > > >> reading it later is a really messy solution. > > > > > > > > In retrospect, it could have been cleaner to avoid describing the BAM as > > > > device node independent of the consumer. We wouldn't have this problem > > > > if the BAM driver would only probe when the consumer is already ready. > > > > > > > > But I think specifying num-channels in the device tree is the cleanest > > > > way out of this mess. I have a second patch series ready that drops > > > > qcom,num-ees and validates the num-channels once it's safe reading from > > > > the BAM registers. That way, you just need one boot test to ensure the > > > > device tree description is really correct. > > > > > > Thanks for the detailed explanation! > > > > > > Do you think it could maybe make sense to expose a clock/power-domain > > > from the modem/adsp rproc and feed it to the DMUX / SLIM instances when > > > an appropriate ping arrives? This way we'd also defer probing the drivers > > > until the device is actually accessible. > > > > > > > Maybe, but that would result in a cyclic dependency between the DMA > > provider and consumer. E.g. > > > > bam_dmux_dma: dma-controller@ { > > #dma-cells = <1>; > > power-domains = <&bam_dmux>; > > }; > > > > remoteproc@ { > > /* ... */ > > > > bam_dmux: bam-dmux { > > dmas = <&bam_dmux_dma 4>, <&bam_dmux_dma 5>; > > dma-names = "tx", "rx"; > > }; > > }; > > > > fw_devlink will likely get confused by that. > > Why? We have a property to break cycles: post-init-providers > > That doesn't work here? > Thanks, I was not aware of that property. This looks quite useful for fixing up some of the other cyclic dependencies we have! Nevertheless, for this specific case, I still think we should not make such large breaking changes at this point. As I pointed out further below in my quoted email, this is a legacy hardware block that will likely not get any major new users in the future. We're essentially discussing to rework several bindings and drivers just to drop a single straightforward "num-channels = <N>" property. A property that we will need to keep support for anyway, to support users with older DTBs. This effort (and risk) is really better spent elsewhere. Thanks, Stephan
diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml index 3ad0d9b1fbc5e4f83dd316d1ad79773c288748ba..5f7e7763615578717651014cfd52745ea2132115 100644 --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml @@ -90,8 +90,12 @@ required: anyOf: - required: - qcom,powered-remotely + - num-channels + - qcom,num-ees - required: - qcom,controlled-remotely + - num-channels + - qcom,num-ees - required: - clocks - clock-names
num-channels and qcom,num-ees are required when there are no clocks specified in the device tree, because we have no reliable way to read them from the hardware registers if we cannot ensure the BAM hardware is up when the device is being probed. This has often been forgotten when adding new SoC device trees, so make this clear by describing this requirement in the schema. Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> --- Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 4 ++++ 1 file changed, 4 insertions(+)