In the case of Hyper DMA (HDMA) present in DWC controllers, there is no way
the drivers can auto detect the number of read/write channels as like its
predecessor embedded DMA (eDMA). So the glue drivers making use of HDMA
have to pass the channels count during probe.
To accommodate that, let's skip finding the channels if the channels count
were already passed by glue drivers. If the channels count passed were
wrong in any form, then the existing sanity check will catch it.
Suggested-by: Serge Semin <[email protected]>
Reviewed-by: Siddharth Vadapalli <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/pci/controller/dwc/pcie-designware.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 193fcd86cf93..ce273c3c5421 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -927,13 +927,15 @@ static int dw_pcie_edma_find_channels(struct dw_pcie *pci)
{
u32 val;
- if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
- val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
- else
- val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
-
- pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
- pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
+ if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) {
+ if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
+ val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
+ else
+ val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
+
+ pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
+ pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
+ }
/* Sanity check the channels count if the mapping was incorrect */
if (!pci->edma.ll_wr_cnt || pci->edma.ll_wr_cnt > EDMA_MAX_WR_CH ||
--
2.25.1
On Mon, Feb 26, 2024 at 05:07:27PM +0530, Manivannan Sadhasivam wrote:
> In the case of Hyper DMA (HDMA) present in DWC controllers, there is no way
> the drivers can auto detect the number of read/write channels as like its
> predecessor embedded DMA (eDMA). So the glue drivers making use of HDMA
> have to pass the channels count during probe.
>
> To accommodate that, let's skip finding the channels if the channels count
> were already passed by glue drivers. If the channels count passed were
> wrong in any form, then the existing sanity check will catch it.
>
> Suggested-by: Serge Semin <[email protected]>
> Reviewed-by: Siddharth Vadapalli <[email protected]>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-designware.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 193fcd86cf93..ce273c3c5421 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -927,13 +927,15 @@ static int dw_pcie_edma_find_channels(struct dw_pcie *pci)
> {
> u32 val;
>
> - if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> - else
> - val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> -
> - pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> - pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) {
Are you sure that the partly initialized case should be considered as
a request for the auto-detection? IMO &&-ing here and letting the
sanity check to fail further would be more correct since thus the
developer would know about improper initialized data.
-Serge(y)
> + if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> + else
> + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> +
> + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> + }
>
> /* Sanity check the channels count if the mapping was incorrect */
> if (!pci->edma.ll_wr_cnt || pci->edma.ll_wr_cnt > EDMA_MAX_WR_CH ||
>
> --
> 2.25.1
>
On Mon, Feb 26, 2024 at 03:53:20PM +0300, Serge Semin wrote:
> On Mon, Feb 26, 2024 at 05:07:27PM +0530, Manivannan Sadhasivam wrote:
> > In the case of Hyper DMA (HDMA) present in DWC controllers, there is no way
> > the drivers can auto detect the number of read/write channels as like its
> > predecessor embedded DMA (eDMA). So the glue drivers making use of HDMA
> > have to pass the channels count during probe.
> >
> > To accommodate that, let's skip finding the channels if the channels count
> > were already passed by glue drivers. If the channels count passed were
> > wrong in any form, then the existing sanity check will catch it.
> >
> > Suggested-by: Serge Semin <[email protected]>
> > Reviewed-by: Siddharth Vadapalli <[email protected]>
> > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > ---
> > drivers/pci/controller/dwc/pcie-designware.c | 16 +++++++++-------
> > 1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 193fcd86cf93..ce273c3c5421 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -927,13 +927,15 @@ static int dw_pcie_edma_find_channels(struct dw_pcie *pci)
> > {
> > u32 val;
> >
> > - if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> > - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > - else
> > - val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > -
> > - pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> > - pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
>
> > + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) {
>
> Are you sure that the partly initialized case should be considered as
> a request for the auto-detection? IMO &&-ing here and letting the
> sanity check to fail further would be more correct since thus the
> developer would know about improper initialized data.
>
We already have the check below. So the partly initialized case will fail
anyway.
- Mani
> -Serge(y)
>
> > + if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > + else
> > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > +
> > + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> > + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> > + }
> >
> > /* Sanity check the channels count if the mapping was incorrect */
> > if (!pci->edma.ll_wr_cnt || pci->edma.ll_wr_cnt > EDMA_MAX_WR_CH ||
> >
> > --
> > 2.25.1
> >
--
மணிவண்ணன் சதாசிவம்
On Mon, Feb 26, 2024 at 05:07:27PM +0530, Manivannan Sadhasivam wrote:
> In the case of Hyper DMA (HDMA) present in DWC controllers, there is no way
> the drivers can auto detect the number of read/write channels as like its
> predecessor embedded DMA (eDMA). So the glue drivers making use of HDMA
> have to pass the channels count during probe.
>
> To accommodate that, let's skip finding the channels if the channels count
> were already passed by glue drivers. If the channels count passed were
> wrong in any form, then the existing sanity check will catch it.
>
> Suggested-by: Serge Semin <[email protected]>
> Reviewed-by: Siddharth Vadapalli <[email protected]>
Reviewed-by: Frank Li <[email protected]>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-designware.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 193fcd86cf93..ce273c3c5421 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -927,13 +927,15 @@ static int dw_pcie_edma_find_channels(struct dw_pcie *pci)
> {
> u32 val;
>
> - if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> - else
> - val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> -
> - pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> - pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) {
> + if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> + else
> + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> +
> + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> + }
>
> /* Sanity check the channels count if the mapping was incorrect */
> if (!pci->edma.ll_wr_cnt || pci->edma.ll_wr_cnt > EDMA_MAX_WR_CH ||
>
> --
> 2.25.1
>
On Mon, Feb 26, 2024 at 09:00:14PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Feb 26, 2024 at 03:53:20PM +0300, Serge Semin wrote:
> > On Mon, Feb 26, 2024 at 05:07:27PM +0530, Manivannan Sadhasivam wrote:
> > > In the case of Hyper DMA (HDMA) present in DWC controllers, there is no way
> > > the drivers can auto detect the number of read/write channels as like its
> > > predecessor embedded DMA (eDMA). So the glue drivers making use of HDMA
> > > have to pass the channels count during probe.
> > >
> > > To accommodate that, let's skip finding the channels if the channels count
> > > were already passed by glue drivers. If the channels count passed were
> > > wrong in any form, then the existing sanity check will catch it.
> > >
> > > Suggested-by: Serge Semin <[email protected]>
> > > Reviewed-by: Siddharth Vadapalli <[email protected]>
> > > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > > ---
> > > drivers/pci/controller/dwc/pcie-designware.c | 16 +++++++++-------
> > > 1 file changed, 9 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > index 193fcd86cf93..ce273c3c5421 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > @@ -927,13 +927,15 @@ static int dw_pcie_edma_find_channels(struct dw_pcie *pci)
> > > {
> > > u32 val;
> > >
> > > - if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> > > - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > > - else
> > > - val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > > -
> > > - pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> > > - pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> >
> > > + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) {
> >
> > Are you sure that the partly initialized case should be considered as
> > a request for the auto-detection? IMO &&-ing here and letting the
> > sanity check to fail further would be more correct since thus the
> > developer would know about improper initialized data.
> >
>
> We already have the check below. So the partly initialized case will fail
> anyway.
Not really. If the partly initialized case activates the
auto-detection procedure it will override both non-initialized and
_initialized_ number of channels with the values retrieved from the
hardware, which the glue driver has been willing not to use. This
prone to undefined behavior depending on the reasons of skipping the
auto-detection procedure. For instance, assume the DMA_CTRL register
reports an invalid number of read channels. A glue driver by mistake
or willingly overwrites the pci->edma.ll_rd_cnt field only. This won't
solve the problem since the auto-detection will be proceeded due to
the pci->edma.ll_wr_cnt field being left uninitialized.
So to speak it would be better to implement a strictly determined case
for activating the auto-detection procedure: both number of channels
aren't initialized; if only one field is initialized then report an
error.
Alternatively we can have the auto-detection executed on the
per-channel basis:
+ if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) {
+ val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
+
+ if (!pci->edma.ll_wr_cnt)
+ pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
+
+ if (!pci->edma.ll_rd_cnt)
+ pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
+ }
-Serge(y)
>
> - Mani
>
> > -Serge(y)
> >
> > > + if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> > > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > > + else
> > > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > > +
> > > + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> > > + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> > > + }
> > >
> > > /* Sanity check the channels count if the mapping was incorrect */
> > > if (!pci->edma.ll_wr_cnt || pci->edma.ll_wr_cnt > EDMA_MAX_WR_CH ||
> > >
> > > --
> > > 2.25.1
> > >
>
> --
> மணிவண்ணன் சதாசிவம்
On Tue, Feb 27, 2024 at 12:32:44AM +0300, Serge Semin wrote:
> On Mon, Feb 26, 2024 at 09:00:14PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Feb 26, 2024 at 03:53:20PM +0300, Serge Semin wrote:
> > > On Mon, Feb 26, 2024 at 05:07:27PM +0530, Manivannan Sadhasivam wrote:
> > > > In the case of Hyper DMA (HDMA) present in DWC controllers, there is no way
> > > > the drivers can auto detect the number of read/write channels as like its
> > > > predecessor embedded DMA (eDMA). So the glue drivers making use of HDMA
> > > > have to pass the channels count during probe.
> > > >
> > > > To accommodate that, let's skip finding the channels if the channels count
> > > > were already passed by glue drivers. If the channels count passed were
> > > > wrong in any form, then the existing sanity check will catch it.
> > > >
> > > > Suggested-by: Serge Semin <[email protected]>
> > > > Reviewed-by: Siddharth Vadapalli <[email protected]>
> > > > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > > > ---
> > > > drivers/pci/controller/dwc/pcie-designware.c | 16 +++++++++-------
> > > > 1 file changed, 9 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > > index 193fcd86cf93..ce273c3c5421 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > @@ -927,13 +927,15 @@ static int dw_pcie_edma_find_channels(struct dw_pcie *pci)
> > > > {
> > > > u32 val;
> > > >
> > > > - if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> > > > - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > > > - else
> > > > - val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > > > -
> > > > - pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> > > > - pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> > >
> > > > + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) {
> > >
> > > Are you sure that the partly initialized case should be considered as
> > > a request for the auto-detection? IMO &&-ing here and letting the
> > > sanity check to fail further would be more correct since thus the
> > > developer would know about improper initialized data.
> > >
> >
> > We already have the check below. So the partly initialized case will fail
> > anyway.
>
> Not really. If the partly initialized case activates the
> auto-detection procedure it will override both non-initialized and
> _initialized_ number of channels with the values retrieved from the
> hardware, which the glue driver has been willing not to use. This
> prone to undefined behavior depending on the reasons of skipping the
> auto-detection procedure. For instance, assume the DMA_CTRL register
> reports an invalid number of read channels. A glue driver by mistake
> or willingly overwrites the pci->edma.ll_rd_cnt field only. This won't
> solve the problem since the auto-detection will be proceeded due to
> the pci->edma.ll_wr_cnt field being left uninitialized.
>
> So to speak it would be better to implement a strictly determined case
> for activating the auto-detection procedure: both number of channels
> aren't initialized; if only one field is initialized then report an
> error.
>
> Alternatively we can have the auto-detection executed on the
> per-channel basis:
>
> + if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) {
> + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> +
> + if (!pci->edma.ll_wr_cnt)
> + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> +
> + if (!pci->edma.ll_rd_cnt)
> + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> + }
>
Hmm, in this case there is no need to check for uninitialized channels count:
/*
* Autodetect the read/write channels count only for non-HDMA platforms.
* HDMA platforms doesn't support autodetect, so the glue drivers should've
* passed the valid count already. If not, the below sanity check will
* catch it.
*/
if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) {
val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
}
/* Sanity check */
- Mani
--
மணிவண்ணன் சதாசிவம்
On Tue, Feb 27, 2024 at 02:12:04PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Feb 27, 2024 at 12:32:44AM +0300, Serge Semin wrote:
> > On Mon, Feb 26, 2024 at 09:00:14PM +0530, Manivannan Sadhasivam wrote:
> > > On Mon, Feb 26, 2024 at 03:53:20PM +0300, Serge Semin wrote:
> > > > On Mon, Feb 26, 2024 at 05:07:27PM +0530, Manivannan Sadhasivam wrote:
> > > > > In the case of Hyper DMA (HDMA) present in DWC controllers, there is no way
> > > > > the drivers can auto detect the number of read/write channels as like its
> > > > > predecessor embedded DMA (eDMA). So the glue drivers making use of HDMA
> > > > > have to pass the channels count during probe.
> > > > >
> > > > > To accommodate that, let's skip finding the channels if the channels count
> > > > > were already passed by glue drivers. If the channels count passed were
> > > > > wrong in any form, then the existing sanity check will catch it.
> > > > >
> > > > > Suggested-by: Serge Semin <[email protected]>
> > > > > Reviewed-by: Siddharth Vadapalli <[email protected]>
> > > > > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > > > > ---
> > > > > drivers/pci/controller/dwc/pcie-designware.c | 16 +++++++++-------
> > > > > 1 file changed, 9 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > index 193fcd86cf93..ce273c3c5421 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > @@ -927,13 +927,15 @@ static int dw_pcie_edma_find_channels(struct dw_pcie *pci)
> > > > > {
> > > > > u32 val;
> > > > >
> > > > > - if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> > > > > - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > > > > - else
> > > > > - val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > > > > -
> > > > > - pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> > > > > - pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> > > >
> > > > > + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) {
> > > >
> > > > Are you sure that the partly initialized case should be considered as
> > > > a request for the auto-detection? IMO &&-ing here and letting the
> > > > sanity check to fail further would be more correct since thus the
> > > > developer would know about improper initialized data.
> > > >
> > >
> > > We already have the check below. So the partly initialized case will fail
> > > anyway.
> >
> > Not really. If the partly initialized case activates the
> > auto-detection procedure it will override both non-initialized and
> > _initialized_ number of channels with the values retrieved from the
> > hardware, which the glue driver has been willing not to use. This
> > prone to undefined behavior depending on the reasons of skipping the
> > auto-detection procedure. For instance, assume the DMA_CTRL register
> > reports an invalid number of read channels. A glue driver by mistake
> > or willingly overwrites the pci->edma.ll_rd_cnt field only. This won't
> > solve the problem since the auto-detection will be proceeded due to
> > the pci->edma.ll_wr_cnt field being left uninitialized.
> >
> > So to speak it would be better to implement a strictly determined case
> > for activating the auto-detection procedure: both number of channels
> > aren't initialized; if only one field is initialized then report an
> > error.
> >
> > Alternatively we can have the auto-detection executed on the
> > per-channel basis:
> >
> > + if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) {
> > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > +
> > + if (!pci->edma.ll_wr_cnt)
> > + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> > +
> > + if (!pci->edma.ll_rd_cnt)
> > + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> > + }
> >
>
> Hmm, in this case there is no need to check for uninitialized channels count:
>
> /*
> * Autodetect the read/write channels count only for non-HDMA platforms.
> * HDMA platforms doesn't support autodetect, so the glue drivers should've
> * passed the valid count already. If not, the below sanity check will
> * catch it.
> */
> if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) {
> val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
>
> pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> }
>
> /* Sanity check */
That is another possible implementation. Let's sum all of them up:
1. Channel fields-base conditional statement:
+ if (!pci->edma.ll_wr_cnt && !pci->edma.ll_rd_cnt) {
+ val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
+
+ pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
+ pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
+ }
pros: NoF channels override support for all IP-cores; simple.
cons: incompatible with HDMA, but can be taken by mistake/bug; no
partial NoF channels pre-initialization.
2. Channel fields-base conditional statement with logical OR operator #1:
+ if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) {
+ val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
+
+ pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
+ pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
+ }
pros: NoF channels override support for all IP-cores; simple.
cons: incompatible with HDMA, but can be taken by mistake/bug; no
partial NoF channels pre-initialization; silently overrides the
partial NoF channels case.
3. Channel fields-base conditional statement with logical OR operator #2:
+ if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) {
+ val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
+
+ if (!pci->edma.ll_wr_cnt)
+ pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
+
+ if (!pci->edma.ll_rd_cnt)
+ pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
+ }
pros: NoF channels override support for all IP-cores; partial NoF
channels pre-initialization support.
cons: incompatible with HDMA, but can be taken by mistake/bug; more
complex (and actually looking a bit clumsy due to two conditional
statements over the same fields).
4. Unconditional auto-detection:
+ val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
+
+ if (!pci->edma.ll_wr_cnt)
+ pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
+
+ if (!pci->edma.ll_rd_cnt)
+ pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
pros: NoF channels override support for all IP-cores; partial NoF
channels pre-initialization support; simple.
cons: incompatible with HDMA, but will be executed for it anyway so
the NoF channels fields will be overridden with the Channel#0.prefetch
CSR data if haven't been pre-initialized;
5. Mapping format-based conditional statement:
+ if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) {
+ val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
+
+ pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
+ pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
+ }
pros: free of being executed for HDMA IP-core, simple
cons: no NoF channels override support for non-HDMA IP-cores.
6. Mapping format-based conditional statement with partial NoF channels override:
+ if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) {
+ val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
+
+ if (!pci->edma.ll_wr_cnt)
+ pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
+
+ if (!pci->edma.ll_rd_cnt)
+ pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
+ }
pros: free of being executed for HDMA IP-core; NoF channels override
support for all IP-cores.
cons: more complex.
Looking at all of that I'd say that options 5 and 6 seems better to me
now since they prohibit the auto-detection for HDMA IP-cores which
have the Channel#0.prefetch CSR at the 0x8 offset. I don't have strong
opinion which of those two to choose. If you think simplicity is
preferable, then option 2 will be enough. If you wish to have the NoF
channels override supported for all IP-cores, then option 3 will work
for it.
-Serge(y)
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்
On Tue, Feb 27, 2024 at 03:21:00PM +0300, Serge Semin wrote:
> On Tue, Feb 27, 2024 at 02:12:04PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Feb 27, 2024 at 12:32:44AM +0300, Serge Semin wrote:
> > > On Mon, Feb 26, 2024 at 09:00:14PM +0530, Manivannan Sadhasivam wrote:
> > > > On Mon, Feb 26, 2024 at 03:53:20PM +0300, Serge Semin wrote:
> > > > > On Mon, Feb 26, 2024 at 05:07:27PM +0530, Manivannan Sadhasivam wrote:
> > > > > > In the case of Hyper DMA (HDMA) present in DWC controllers, there is no way
> > > > > > the drivers can auto detect the number of read/write channels as like its
> > > > > > predecessor embedded DMA (eDMA). So the glue drivers making use of HDMA
> > > > > > have to pass the channels count during probe.
> > > > > >
> > > > > > To accommodate that, let's skip finding the channels if the channels count
> > > > > > were already passed by glue drivers. If the channels count passed were
> > > > > > wrong in any form, then the existing sanity check will catch it.
> > > > > >
> > > > > > Suggested-by: Serge Semin <[email protected]>
> > > > > > Reviewed-by: Siddharth Vadapalli <[email protected]>
> > > > > > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > > > > > ---
> > > > > > drivers/pci/controller/dwc/pcie-designware.c | 16 +++++++++-------
> > > > > > 1 file changed, 9 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > > index 193fcd86cf93..ce273c3c5421 100644
> > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > > @@ -927,13 +927,15 @@ static int dw_pcie_edma_find_channels(struct dw_pcie *pci)
> > > > > > {
> > > > > > u32 val;
> > > > > >
> > > > > > - if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> > > > > > - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > > > > > - else
> > > > > > - val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > > > > > -
> > > > > > - pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> > > > > > - pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> > > > >
> > > > > > + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) {
> > > > >
> > > > > Are you sure that the partly initialized case should be considered as
> > > > > a request for the auto-detection? IMO &&-ing here and letting the
> > > > > sanity check to fail further would be more correct since thus the
> > > > > developer would know about improper initialized data.
> > > > >
> > > >
> > > > We already have the check below. So the partly initialized case will fail
> > > > anyway.
> > >
> > > Not really. If the partly initialized case activates the
> > > auto-detection procedure it will override both non-initialized and
> > > _initialized_ number of channels with the values retrieved from the
> > > hardware, which the glue driver has been willing not to use. This
> > > prone to undefined behavior depending on the reasons of skipping the
> > > auto-detection procedure. For instance, assume the DMA_CTRL register
> > > reports an invalid number of read channels. A glue driver by mistake
> > > or willingly overwrites the pci->edma.ll_rd_cnt field only. This won't
> > > solve the problem since the auto-detection will be proceeded due to
> > > the pci->edma.ll_wr_cnt field being left uninitialized.
> > >
> > > So to speak it would be better to implement a strictly determined case
> > > for activating the auto-detection procedure: both number of channels
> > > aren't initialized; if only one field is initialized then report an
> > > error.
> > >
> > > Alternatively we can have the auto-detection executed on the
> > > per-channel basis:
> > >
> > > + if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) {
> > > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > > +
> > > + if (!pci->edma.ll_wr_cnt)
> > > + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> > > +
> > > + if (!pci->edma.ll_rd_cnt)
> > > + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> > > + }
> > >
> >
> > Hmm, in this case there is no need to check for uninitialized channels count:
> >
> > /*
> > * Autodetect the read/write channels count only for non-HDMA platforms.
> > * HDMA platforms doesn't support autodetect, so the glue drivers should've
> > * passed the valid count already. If not, the below sanity check will
> > * catch it.
> > */
> > if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) {
> > val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> >
> > pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> > pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> > }
> >
> > /* Sanity check */
>
> That is another possible implementation. Let's sum all of them up:
>
> 1. Channel fields-base conditional statement:
> + if (!pci->edma.ll_wr_cnt && !pci->edma.ll_rd_cnt) {
> + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> +
> + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> + }
> pros: NoF channels override support for all IP-cores; simple.
> cons: incompatible with HDMA, but can be taken by mistake/bug; no
> partial NoF channels pre-initialization.
>
> 2. Channel fields-base conditional statement with logical OR operator #1:
> + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) {
> + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> +
> + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> + }
> pros: NoF channels override support for all IP-cores; simple.
> cons: incompatible with HDMA, but can be taken by mistake/bug; no
> partial NoF channels pre-initialization; silently overrides the
> partial NoF channels case.
>
> 3. Channel fields-base conditional statement with logical OR operator #2:
> + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) {
> + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> +
> + if (!pci->edma.ll_wr_cnt)
> + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> +
> + if (!pci->edma.ll_rd_cnt)
> + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> + }
> pros: NoF channels override support for all IP-cores; partial NoF
> channels pre-initialization support.
> cons: incompatible with HDMA, but can be taken by mistake/bug; more
> complex (and actually looking a bit clumsy due to two conditional
> statements over the same fields).
>
> 4. Unconditional auto-detection:
> + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> +
> + if (!pci->edma.ll_wr_cnt)
> + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> +
> + if (!pci->edma.ll_rd_cnt)
> + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> pros: NoF channels override support for all IP-cores; partial NoF
> channels pre-initialization support; simple.
> cons: incompatible with HDMA, but will be executed for it anyway so
> the NoF channels fields will be overridden with the Channel#0.prefetch
> CSR data if haven't been pre-initialized;
>
> 5. Mapping format-based conditional statement:
> + if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) {
> + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> +
> + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> + }
> pros: free of being executed for HDMA IP-core, simple
> cons: no NoF channels override support for non-HDMA IP-cores.
>
Is it possible for the non-HDMA IPs to override the channels count? Atleast any
such IPs supported in mainline now? If not, then I'd like to go with this
approach.
Because, this makes it explicit that override is only supported for HDMA IPs and
also simplifies the logic.
> 6. Mapping format-based conditional statement with partial NoF channels override:
> + if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) {
> + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> +
> + if (!pci->edma.ll_wr_cnt)
> + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> +
> + if (!pci->edma.ll_rd_cnt)
> + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> + }
> pros: free of being executed for HDMA IP-core; NoF channels override
> support for all IP-cores.
> cons: more complex.
>
>
> Looking at all of that I'd say that options 5 and 6 seems better to me
> now since they prohibit the auto-detection for HDMA IP-cores which
> have the Channel#0.prefetch CSR at the 0x8 offset. I don't have strong
> opinion which of those two to choose. If you think simplicity is
> preferable, then option 2 will be enough. If you wish to have the NoF
> channels override supported for all IP-cores, then option 3 will work
> for it.
>
Thanks for such elaborative comparision :)
- Mani
--
மணிவண்ணன் சதாசிவம்