| Message ID | 20170605123348.26137-2-icenowy@aosc.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 5, 2017 at 8:33 PM, Icenowy Zheng <icenowy@aosc.io> wrote: > From: Icenowy Zheng <icenowy@aosc.xyz> > > Originally we enable a special gate bit when the compatible indicates > A23/33. > > But according to BSP sources and user manuals, more SoCs will need this > gate bit. > > So make it a common quirk configured in the config struct. > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
On Mon, Jun 05, 2017 at 08:33:47PM +0800, Icenowy Zheng wrote: > From: Icenowy Zheng <icenowy@aosc.xyz> > > Originally we enable a special gate bit when the compatible indicates > A23/33. > > But according to BSP sources and user manuals, more SoCs will need this > gate bit. > > So make it a common quirk configured in the config struct. > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> > --- > Changes since original codec patchset v3: > - Refactored comments to cover some words found in official documents. > - Removed the comments when toggling the gate bit. > > drivers/dma/sun6i-dma.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > index a2358780ab2c..252b59c1d1d5 100644 > --- a/drivers/dma/sun6i-dma.c > +++ b/drivers/dma/sun6i-dma.c > @@ -101,6 +101,17 @@ struct sun6i_dma_config { > u32 nr_max_channels; > u32 nr_max_requests; > u32 nr_max_vchans; > + /* > + * In the datasheets/user manuals of newer Allwinner SoCs, a special > + * bit (bit 2 at register 0x20) is present. > + * It's named "DMA MCLK interface circuit auto gating bit" in the > + * documents, and the footnote of this register says that this bit > + * should be set up when initializing the DMA controller. > + * Allwinner A23/A33 user manuals do not have this bit documented, > + * however these SoCs really have and need this bit, as seen in the > + * BSP kernel source code. > + */ > + bool gate_needed; Since this is a hw property, why is this not added as an optional DT property? > }; > > /* > @@ -1009,6 +1020,7 @@ static struct sun6i_dma_config sun8i_a23_dma_cfg = { > .nr_max_channels = 8, > .nr_max_requests = 24, > .nr_max_vchans = 37, > + .gate_needed = true, > }; > > static struct sun6i_dma_config sun8i_a83t_dma_cfg = { > @@ -1174,13 +1186,7 @@ static int sun6i_dma_probe(struct platform_device *pdev) > goto err_dma_unregister; > } > > - /* > - * sun8i variant requires us to toggle a dma gating register, > - * as seen in Allwinner's SDK. This register is not documented > - * in the A23 user manual. > - */ > - if (of_device_is_compatible(pdev->dev.of_node, > - "allwinner,sun8i-a23-dma")) > + if (sdc->cfg->gate_needed) > writel(SUN8I_DMA_GATE_ENABLE, sdc->base + SUN8I_DMA_GATE); > > return 0; > -- > 2.12.2 >
于 2017年6月14日 GMT+08:00 下午4:32:52, Vinod Koul <vinod.koul@intel.com> 写到: >On Mon, Jun 05, 2017 at 08:33:47PM +0800, Icenowy Zheng wrote: >> From: Icenowy Zheng <icenowy@aosc.xyz> >> >> Originally we enable a special gate bit when the compatible indicates >> A23/33. >> >> But according to BSP sources and user manuals, more SoCs will need >this >> gate bit. >> >> So make it a common quirk configured in the config struct. >> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> >> --- >> Changes since original codec patchset v3: >> - Refactored comments to cover some words found in official >documents. >> - Removed the comments when toggling the gate bit. >> >> drivers/dma/sun6i-dma.c | 20 +++++++++++++------- >> 1 file changed, 13 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c >> index a2358780ab2c..252b59c1d1d5 100644 >> --- a/drivers/dma/sun6i-dma.c >> +++ b/drivers/dma/sun6i-dma.c >> @@ -101,6 +101,17 @@ struct sun6i_dma_config { >> u32 nr_max_channels; >> u32 nr_max_requests; >> u32 nr_max_vchans; >> + /* >> + * In the datasheets/user manuals of newer Allwinner SoCs, a >special >> + * bit (bit 2 at register 0x20) is present. >> + * It's named "DMA MCLK interface circuit auto gating bit" in the >> + * documents, and the footnote of this register says that this bit >> + * should be set up when initializing the DMA controller. >> + * Allwinner A23/A33 user manuals do not have this bit documented, >> + * however these SoCs really have and need this bit, as seen in the >> + * BSP kernel source code. >> + */ >> + bool gate_needed; > >Since this is a hw property, why is this not added as an optional DT >property? As it's SoC-specified. Some SoCs need it, and some don't. SoC info is in compatible, so there's no reason to make it a property. > >> }; >> >> /* >> @@ -1009,6 +1020,7 @@ static struct sun6i_dma_config >sun8i_a23_dma_cfg = { >> .nr_max_channels = 8, >> .nr_max_requests = 24, >> .nr_max_vchans = 37, >> + .gate_needed = true, >> }; >> >> static struct sun6i_dma_config sun8i_a83t_dma_cfg = { >> @@ -1174,13 +1186,7 @@ static int sun6i_dma_probe(struct >platform_device *pdev) >> goto err_dma_unregister; >> } >> >> - /* >> - * sun8i variant requires us to toggle a dma gating register, >> - * as seen in Allwinner's SDK. This register is not documented >> - * in the A23 user manual. >> - */ >> - if (of_device_is_compatible(pdev->dev.of_node, >> - "allwinner,sun8i-a23-dma")) >> + if (sdc->cfg->gate_needed) >> writel(SUN8I_DMA_GATE_ENABLE, sdc->base + SUN8I_DMA_GATE); >> >> return 0; >> -- >> 2.12.2 >>
On Wed, Jun 14, 2017 at 04:32:57PM +0800, Icenowy Zheng wrote: > > > 于 2017年6月14日 GMT+08:00 下午4:32:52, Vinod Koul <vinod.koul@intel.com> 写到: > >On Mon, Jun 05, 2017 at 08:33:47PM +0800, Icenowy Zheng wrote: > >> From: Icenowy Zheng <icenowy@aosc.xyz> > >> > >> Originally we enable a special gate bit when the compatible indicates > >> A23/33. > >> > >> But according to BSP sources and user manuals, more SoCs will need > >this > >> gate bit. > >> > >> So make it a common quirk configured in the config struct. > >> > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> > >> --- > >> Changes since original codec patchset v3: > >> - Refactored comments to cover some words found in official > >documents. > >> - Removed the comments when toggling the gate bit. > >> > >> drivers/dma/sun6i-dma.c | 20 +++++++++++++------- > >> 1 file changed, 13 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > >> index a2358780ab2c..252b59c1d1d5 100644 > >> --- a/drivers/dma/sun6i-dma.c > >> +++ b/drivers/dma/sun6i-dma.c > >> @@ -101,6 +101,17 @@ struct sun6i_dma_config { > >> u32 nr_max_channels; > >> u32 nr_max_requests; > >> u32 nr_max_vchans; > >> + /* > >> + * In the datasheets/user manuals of newer Allwinner SoCs, a > >special > >> + * bit (bit 2 at register 0x20) is present. > >> + * It's named "DMA MCLK interface circuit auto gating bit" in the > >> + * documents, and the footnote of this register says that this bit > >> + * should be set up when initializing the DMA controller. > >> + * Allwinner A23/A33 user manuals do not have this bit documented, > >> + * however these SoCs really have and need this bit, as seen in the > >> + * BSP kernel source code. > >> + */ > >> + bool gate_needed; > > > >Since this is a hw property, why is this not added as an optional DT > >property? > > As it's SoC-specified. > > Some SoCs need it, and some don't. and that is the reason it should be a property > > SoC info is in compatible, so there's no reason to make it a property. that's why it would need to be optional for the SoC's that needs these.. > > > > >> }; > >> > >> /* > >> @@ -1009,6 +1020,7 @@ static struct sun6i_dma_config > >sun8i_a23_dma_cfg = { > >> .nr_max_channels = 8, > >> .nr_max_requests = 24, > >> .nr_max_vchans = 37, > >> + .gate_needed = true, > >> }; > >> > >> static struct sun6i_dma_config sun8i_a83t_dma_cfg = { > >> @@ -1174,13 +1186,7 @@ static int sun6i_dma_probe(struct > >platform_device *pdev) > >> goto err_dma_unregister; > >> } > >> > >> - /* > >> - * sun8i variant requires us to toggle a dma gating register, > >> - * as seen in Allwinner's SDK. This register is not documented > >> - * in the A23 user manual. > >> - */ > >> - if (of_device_is_compatible(pdev->dev.of_node, > >> - "allwinner,sun8i-a23-dma")) > >> + if (sdc->cfg->gate_needed) > >> writel(SUN8I_DMA_GATE_ENABLE, sdc->base + SUN8I_DMA_GATE); > >> > >> return 0; > >> -- > >> 2.12.2 > >>
于 2017年6月14日 GMT+08:00 下午4:45:29, Vinod Koul <vinod.koul@intel.com> 写到: >On Wed, Jun 14, 2017 at 04:32:57PM +0800, Icenowy Zheng wrote: >> >> >> 于 2017年6月14日 GMT+08:00 下午4:32:52, Vinod Koul <vinod.koul@intel.com> >写到: >> >On Mon, Jun 05, 2017 at 08:33:47PM +0800, Icenowy Zheng wrote: >> >> From: Icenowy Zheng <icenowy@aosc.xyz> >> >> >> >> Originally we enable a special gate bit when the compatible >indicates >> >> A23/33. >> >> >> >> But according to BSP sources and user manuals, more SoCs will need >> >this >> >> gate bit. >> >> >> >> So make it a common quirk configured in the config struct. >> >> >> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> >> >> --- >> >> Changes since original codec patchset v3: >> >> - Refactored comments to cover some words found in official >> >documents. >> >> - Removed the comments when toggling the gate bit. >> >> >> >> drivers/dma/sun6i-dma.c | 20 +++++++++++++------- >> >> 1 file changed, 13 insertions(+), 7 deletions(-) >> >> >> >> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c >> >> index a2358780ab2c..252b59c1d1d5 100644 >> >> --- a/drivers/dma/sun6i-dma.c >> >> +++ b/drivers/dma/sun6i-dma.c >> >> @@ -101,6 +101,17 @@ struct sun6i_dma_config { >> >> u32 nr_max_channels; >> >> u32 nr_max_requests; >> >> u32 nr_max_vchans; >> >> + /* >> >> + * In the datasheets/user manuals of newer Allwinner SoCs, a >> >special >> >> + * bit (bit 2 at register 0x20) is present. >> >> + * It's named "DMA MCLK interface circuit auto gating bit" in >the >> >> + * documents, and the footnote of this register says that this >bit >> >> + * should be set up when initializing the DMA controller. >> >> + * Allwinner A23/A33 user manuals do not have this bit >documented, >> >> + * however these SoCs really have and need this bit, as seen in >the >> >> + * BSP kernel source code. >> >> + */ >> >> + bool gate_needed; >> > >> >Since this is a hw property, why is this not added as an optional DT >> >property? >> >> As it's SoC-specified. >> >> Some SoCs need it, and some don't. > >and that is the reason it should be a property > >> >> SoC info is in compatible, so there's no reason to make it a >property. > >that's why it would need to be optional for the SoC's that needs >these.. I don't think it proper to add block-specified properties that can be bound to compatible. I added Rob Herring to the recipient list. Rob, do you think this can be added as a property? This is SoC-specific and compatibles are also SoC-specific. > >> >> > >> >> }; >> >> >> >> /* >> >> @@ -1009,6 +1020,7 @@ static struct sun6i_dma_config >> >sun8i_a23_dma_cfg = { >> >> .nr_max_channels = 8, >> >> .nr_max_requests = 24, >> >> .nr_max_vchans = 37, >> >> + .gate_needed = true, >> >> }; >> >> >> >> static struct sun6i_dma_config sun8i_a83t_dma_cfg = { >> >> @@ -1174,13 +1186,7 @@ static int sun6i_dma_probe(struct >> >platform_device *pdev) >> >> goto err_dma_unregister; >> >> } >> >> >> >> - /* >> >> - * sun8i variant requires us to toggle a dma gating register, >> >> - * as seen in Allwinner's SDK. This register is not documented >> >> - * in the A23 user manual. >> >> - */ >> >> - if (of_device_is_compatible(pdev->dev.of_node, >> >> - "allwinner,sun8i-a23-dma")) >> >> + if (sdc->cfg->gate_needed) >> >> writel(SUN8I_DMA_GATE_ENABLE, sdc->base + SUN8I_DMA_GATE); >> >> >> >> return 0; >> >> -- >> >> 2.12.2 >> >>
On Wed, Jun 14, 2017 at 02:15:29PM +0530, Vinod Koul wrote: > > SoC info is in compatible, so there's no reason to make it a property. > > that's why it would need to be optional for the SoC's that needs these.. There's nothing optional about that behaviour, it's mandatory for the SoC that need it, and useless on the SoC that don't. Plus, that would require changing the DT binding, which isn't something we can do. Maxime
于 2017年6月15日 GMT+08:00 上午11:54:08, Vinod Koul <vinod.koul@intel.com> 写到: >On Wed, Jun 14, 2017 at 11:04:39AM +0200, Maxime Ripard wrote: >> On Wed, Jun 14, 2017 at 02:15:29PM +0530, Vinod Koul wrote: >> > > SoC info is in compatible, so there's no reason to make it a >property. >> > >> > that's why it would need to be optional for the SoC's that needs >these.. >> >> There's nothing optional about that behaviour, it's mandatory for the >> SoC that need it, and useless on the SoC that don't. > >And why should kernel put strings for each hw behaviour. I am expecting >DT >to tell me if this SoC is a special case or not and kernel shall handle >accordingly I don't think this kind of behavior should be described in DT. Rob, do you agree? > >> Plus, that would require changing the DT binding, which isn't >> something we can do. > >Any reason why bindings can't change..? I though this was support for >new >SoC... This is a behavior that exists on a SoC that is already supported (A23/A33).
On Wed, Jun 14, 2017 at 11:04:39AM +0200, Maxime Ripard wrote: > On Wed, Jun 14, 2017 at 02:15:29PM +0530, Vinod Koul wrote: > > > SoC info is in compatible, so there's no reason to make it a property. > > > > that's why it would need to be optional for the SoC's that needs these.. > > There's nothing optional about that behaviour, it's mandatory for the > SoC that need it, and useless on the SoC that don't. And why should kernel put strings for each hw behaviour. I am expecting DT to tell me if this SoC is a special case or not and kernel shall handle accordingly > Plus, that would require changing the DT binding, which isn't > something we can do. Any reason why bindings can't change..? I though this was support for new SoC...
On Thu, Jun 15, 2017 at 09:24:08AM +0530, Vinod Koul wrote: > On Wed, Jun 14, 2017 at 11:04:39AM +0200, Maxime Ripard wrote: > > On Wed, Jun 14, 2017 at 02:15:29PM +0530, Vinod Koul wrote: > > > > SoC info is in compatible, so there's no reason to make it a property. > > > > > > that's why it would need to be optional for the SoC's that needs these.. > > > > There's nothing optional about that behaviour, it's mandatory for the > > SoC that need it, and useless on the SoC that don't. > > And why should kernel put strings for each hw behaviour. You will have strings in the kernel for each hw behaviour, disregarding on whether you base the behaviour on the compatible or a set of properties. In fact, you will have *much* more strings in the kernel in the latter case. > I am expecting DT to tell me if this SoC is a special case or not > and kernel shall handle accordingly How is this not the case here? The DT tells you that this SoC is a special case through a compatible already. > > Plus, that would require changing the DT binding, which isn't > > something we can do. > > Any reason why bindings can't change..? I though this was support for new > SoC... No, this is a rework of an existing code to support a new SoC. The code is already there, and the binding too. It has been for 3 years. Maxime
diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c index a2358780ab2c..252b59c1d1d5 100644 --- a/drivers/dma/sun6i-dma.c +++ b/drivers/dma/sun6i-dma.c @@ -101,6 +101,17 @@ struct sun6i_dma_config { u32 nr_max_channels; u32 nr_max_requests; u32 nr_max_vchans; + /* + * In the datasheets/user manuals of newer Allwinner SoCs, a special + * bit (bit 2 at register 0x20) is present. + * It's named "DMA MCLK interface circuit auto gating bit" in the + * documents, and the footnote of this register says that this bit + * should be set up when initializing the DMA controller. + * Allwinner A23/A33 user manuals do not have this bit documented, + * however these SoCs really have and need this bit, as seen in the + * BSP kernel source code. + */ + bool gate_needed; }; /* @@ -1009,6 +1020,7 @@ static struct sun6i_dma_config sun8i_a23_dma_cfg = { .nr_max_channels = 8, .nr_max_requests = 24, .nr_max_vchans = 37, + .gate_needed = true, }; static struct sun6i_dma_config sun8i_a83t_dma_cfg = { @@ -1174,13 +1186,7 @@ static int sun6i_dma_probe(struct platform_device *pdev) goto err_dma_unregister; } - /* - * sun8i variant requires us to toggle a dma gating register, - * as seen in Allwinner's SDK. This register is not documented - * in the A23 user manual. - */ - if (of_device_is_compatible(pdev->dev.of_node, - "allwinner,sun8i-a23-dma")) + if (sdc->cfg->gate_needed) writel(SUN8I_DMA_GATE_ENABLE, sdc->base + SUN8I_DMA_GATE); return 0;