| Message ID | 1409678286-28139-1-git-send-email-seanpaul@chromium.org |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 09/02/2014 11:18 AM, Sean Paul wrote: > This patch adds MIPI CSI/DSIB pad control mux register > from the APB misc block to tegra pinctrl. > > Without writing to this register, the dsib pads are > muxed as csi, and cannot be used. > > The register is not yet documented in the TRM, here is > the description: > > 70000820: APB_MISC_GP_MIPI_PAD_CTRL_0 > [31:02] RESERVED > [01:01] DSIB_MODE [CSI=0,DSIB=1] > [00:00] RESERVED That's a very unfortunate HW design, but oh well:-( I slightly wonder whether it's legitimate to even consider that register part of the pinmux controller; I certainly don't see any mention of it in the pinmux spreadsheets. It feels like some unrelated bolt-on feature. Still, I suppose requiring a separate driver for it just because the registers aren't all nicely grouped is a bit silly. At least a quick glance implies there aren't any other missing cases like this, so we shouldn't need to add any more later. I don't suppose there's any chance you could update: git://github.com/NVIDIA/tegra-pinmux-scripts.git with an equivalent change? > diff --git a/drivers/pinctrl/pinctrl-tegra124.c b/drivers/pinctrl/pinctrl-tegra124.c > +#define TEGRA_PIN_CSI_DSIB _PIN(8) Is that actually the name of the pin on the Tegra package? I don't see anything like that the board schematic I have. > #define DRV_PINGROUP_REG_A 0x868 /* bank 0 */ > #define PINGROUP_REG_A 0x3000 /* bank 1 */ > +#define APB_MISC_PINGROUP_REG_A 0x820 /* bank 2 */ In order for that to work, an extra reg entry will be required in DT so that registers in bank 2 can be accessed. I would expect this patch (or series) to contain an addition to Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-pinmux.txt to mention this. I assume you'll send a patch to arch/arm/boot/dts/tegra124.dtsi separately to add that entry. > +#define APB_MISC_PINGROUP(pg_name, r, b, f0, f1, f_safe) \ f_safe isn't present in any of the upstream Tegra pinctrl drivers any more, so that parameter isn't needed any more... > + { \ > + .name = #pg_name, \ > + .pins = pg_name##_pins, \ > + .npins = ARRAY_SIZE(pg_name##_pins), \ > + .funcs = { \ > + TEGRA_MUX_ ## f0, \ > + TEGRA_MUX_ ## f1, \ > + }, \ > + .func_safe = TEGRA_MUX_ ## f_safe, \ ... and I don't think that line will even compile, since that field doesn't exist? All 4 entries in .funcs[] should be initialized too. If two don't make sense, then they should at least be hard-coded to TEGRA_MUX_RSVD3/4. It would be nice if the driver knew that this pin only had two valid mux options, but I suppose updating the code to handle that special case isn't really worth it. > DRV_PINGROUP(ao4, 0x9c8, 2, 3, 4, 12, 7, 20, 7, 28, 2, 30, 2, Y), > + > + /* pg_name, r b f0, f1, f_safe */ > + APB_MISC_PINGROUP( csi_dsib, 0x820, 1, CSI, DSIB, DSIB) > }; Can you make the indentation of the added lines consistent here. The existing code uses a TAB at the start of the line (but the patch uses spaces), and spaces internally (but the patch uses TABs) so columns don't have to waste space being TAB aligned. The pg_name column should be nestled right against the opening (, without any intervening space. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 2, 2014 at 4:31 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 09/02/2014 11:18 AM, Sean Paul wrote: >> >> This patch adds MIPI CSI/DSIB pad control mux register >> from the APB misc block to tegra pinctrl. >> >> Without writing to this register, the dsib pads are >> muxed as csi, and cannot be used. >> >> The register is not yet documented in the TRM, here is >> the description: >> >> 70000820: APB_MISC_GP_MIPI_PAD_CTRL_0 >> [31:02] RESERVED >> [01:01] DSIB_MODE [CSI=0,DSIB=1] >> [00:00] RESERVED > > > That's a very unfortunate HW design, but oh well:-( > > I slightly wonder whether it's legitimate to even consider that register > part of the pinmux controller; I certainly don't see any mention of it in > the pinmux spreadsheets. It feels like some unrelated bolt-on feature. > Still, I suppose requiring a separate driver for it just because the > registers aren't all nicely grouped is a bit silly. At least a quick glance > implies there aren't any other missing cases like this, so we shouldn't need > to add any more later. > Yeah, the hw is unfortunate. It doesn't feel like this solution was Plan A :-) > I don't suppose there's any chance you could update: > git://github.com/NVIDIA/tegra-pinmux-scripts.git > with an equivalent change? > Sure, I can do that. >> diff --git a/drivers/pinctrl/pinctrl-tegra124.c >> b/drivers/pinctrl/pinctrl-tegra124.c > > >> +#define TEGRA_PIN_CSI_DSIB _PIN(8) > > > Is that actually the name of the pin on the Tegra package? I don't see > anything like that the board schematic I have. > Well, there's more than one pin affected by this register. They're named: DSI_B_CLK_P DSI_B_CLK_N DSI_B_D0_P DSI_B_D0_N DSI_B_D1_P DSI_B_D1_N DSI_B_D2_P DSI_B_D2_N DSI_B_D3_P DSI_B_D3_N I'll change this to TEGRA_PIN_DSI_B, does that work for you? > >> #define DRV_PINGROUP_REG_A 0x868 /* bank 0 */ >> #define PINGROUP_REG_A 0x3000 /* bank 1 */ >> +#define APB_MISC_PINGROUP_REG_A 0x820 /* bank 2 */ > > > In order for that to work, an extra reg entry will be required in DT so that > registers in bank 2 can be accessed. I would expect this patch (or series) > to contain an addition to > Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-pinmux.txt to > mention this. I assume you'll send a patch to > arch/arm/boot/dts/tegra124.dtsi separately to add that entry. > Yep, sounds good. > >> +#define APB_MISC_PINGROUP(pg_name, r, b, f0, f1, f_safe) \ > > > f_safe isn't present in any of the upstream Tegra pinctrl drivers any more, > so that parameter isn't needed any more... > > >> + { \ >> + .name = #pg_name, \ >> + .pins = pg_name##_pins, \ >> + .npins = ARRAY_SIZE(pg_name##_pins), \ >> + .funcs = { \ >> + TEGRA_MUX_ ## f0, \ >> + TEGRA_MUX_ ## f1, \ >> + }, \ >> + .func_safe = TEGRA_MUX_ ## f_safe, \ > > > ... and I don't think that line will even compile, since that field doesn't > exist? > Oh my, sorry about that. I mustn't have had my config set up correctly when I built this. > All 4 entries in .funcs[] should be initialized too. If two don't make > sense, then they should at least be hard-coded to TEGRA_MUX_RSVD3/4. It > would be nice if the driver knew that this pin only had two valid mux > options, but I suppose updating the code to handle that special case isn't > really worth it. > > >> DRV_PINGROUP(ao4, 0x9c8, 2, 3, 4, 12, 7, 20, 7, >> 28, 2, 30, 2, Y), >> + >> + /* pg_name, r b f0, >> f1, f_safe */ >> + APB_MISC_PINGROUP( csi_dsib, 0x820, 1, CSI, >> DSIB, DSIB) >> }; > > > Can you make the indentation of the added lines consistent here. The > existing code uses a TAB at the start of the line (but the patch uses > spaces), and spaces internally (but the patch uses TABs) so columns don't > have to waste space being TAB aligned. The pg_name column should be nestled > right against the opening (, without any intervening space. I'll upload a new version shortly. Sean -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/03/2014 09:24 AM, Sean Paul wrote: > On Tue, Sep 2, 2014 at 4:31 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 09/02/2014 11:18 AM, Sean Paul wrote: >>> >>> This patch adds MIPI CSI/DSIB pad control mux register >>> from the APB misc block to tegra pinctrl. >>> >>> Without writing to this register, the dsib pads are >>> muxed as csi, and cannot be used. >>> >>> The register is not yet documented in the TRM, here is >>> the description: >>> >>> 70000820: APB_MISC_GP_MIPI_PAD_CTRL_0 >>> [31:02] RESERVED >>> [01:01] DSIB_MODE [CSI=0,DSIB=1] >>> [00:00] RESERVED >>> diff --git a/drivers/pinctrl/pinctrl-tegra124.c >>> b/drivers/pinctrl/pinctrl-tegra124.c >> >> >>> +#define TEGRA_PIN_CSI_DSIB _PIN(8) >> >> >> Is that actually the name of the pin on the Tegra package? I don't see >> anything like that the board schematic I have. > > Well, there's more than one pin affected by this register. They're named: > > DSI_B_CLK_P > DSI_B_CLK_N > DSI_B_D0_P > DSI_B_D0_N > DSI_B_D1_P > DSI_B_D1_N > DSI_B_D2_P > DSI_B_D2_N > DSI_B_D3_P > DSI_B_D3_N > > I'll change this to TEGRA_PIN_DSI_B, does that work for you? Would it be possible to add a pin entry for each individual pin, and then create a DSI_B group that contains all those pins? Mux selections are made on pin groups rather than individual pins, so this shouldn't affect anything except for a few data tables in the patch. This way, it keeps the PIN macros purely as pins, rather than sometimes using them for groups of pins. As background: On Tegra30+, there's a 1:1 mapping between pins and groups for the regular pinmux registers, but if you look at the Tegra20 HW/driver, you'll see a much smaller set of groups than pins there. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 3, 2014 at 11:34 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 09/03/2014 09:24 AM, Sean Paul wrote: >> >> On Tue, Sep 2, 2014 at 4:31 PM, Stephen Warren <swarren@wwwdotorg.org> >> wrote: >>> >>> On 09/02/2014 11:18 AM, Sean Paul wrote: >>>> >>>> >>>> This patch adds MIPI CSI/DSIB pad control mux register >>>> from the APB misc block to tegra pinctrl. >>>> >>>> Without writing to this register, the dsib pads are >>>> muxed as csi, and cannot be used. >>>> >>>> The register is not yet documented in the TRM, here is >>>> the description: >>>> >>>> 70000820: APB_MISC_GP_MIPI_PAD_CTRL_0 >>>> [31:02] RESERVED >>>> [01:01] DSIB_MODE [CSI=0,DSIB=1] >>>> [00:00] RESERVED > > >>>> diff --git a/drivers/pinctrl/pinctrl-tegra124.c >>>> b/drivers/pinctrl/pinctrl-tegra124.c >>> >>> >>> >>>> +#define TEGRA_PIN_CSI_DSIB _PIN(8) >>> >>> >>> >>> Is that actually the name of the pin on the Tegra package? I don't see >>> anything like that the board schematic I have. >> >> >> Well, there's more than one pin affected by this register. They're named: >> >> DSI_B_CLK_P >> DSI_B_CLK_N >> DSI_B_D0_P >> DSI_B_D0_N >> DSI_B_D1_P >> DSI_B_D1_N >> DSI_B_D2_P >> DSI_B_D2_N >> DSI_B_D3_P >> DSI_B_D3_N >> >> I'll change this to TEGRA_PIN_DSI_B, does that work for you? > > > Would it be possible to add a pin entry for each individual pin, and then > create a DSI_B group that contains all those pins? Sure, sounds good to me. Sean > Mux selections are made > on pin groups rather than individual pins, so this shouldn't affect anything > except for a few data tables in the patch. This way, it keeps the PIN macros > purely as pins, rather than sometimes using them for groups of pins. As > background: On Tegra30+, there's a 1:1 mapping between pins and groups for > the regular pinmux registers, but if you look at the Tegra20 HW/driver, > you'll see a much smaller set of groups than pins there. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/pinctrl-tegra124.c b/drivers/pinctrl/pinctrl-tegra124.c index e80797e..9a3359f 100644 --- a/drivers/pinctrl/pinctrl-tegra124.c +++ b/drivers/pinctrl/pinctrl-tegra124.c @@ -224,6 +224,7 @@ #define TEGRA_PIN_OWR _PIN(5) #define TEGRA_PIN_CLK_32K_IN _PIN(6) #define TEGRA_PIN_JTAG_RTCK _PIN(7) +#define TEGRA_PIN_CSI_DSIB _PIN(8) static const struct pinctrl_pin_desc tegra124_pins[] = { PINCTRL_PIN(TEGRA_PIN_CLK_32K_OUT_PA0, "CLK_32K_OUT PA0"), @@ -417,6 +418,7 @@ static const struct pinctrl_pin_desc tegra124_pins[] = { PINCTRL_PIN(TEGRA_PIN_OWR, "OWR"), PINCTRL_PIN(TEGRA_PIN_CLK_32K_IN, "CLK_32K_IN"), PINCTRL_PIN(TEGRA_PIN_JTAG_RTCK, "JTAG_RTCK"), + PINCTRL_PIN(TEGRA_PIN_CSI_DSIB, "CSI_DSIB"), }; static const unsigned clk_32k_out_pa0_pins[] = { @@ -1495,6 +1497,10 @@ static const unsigned drive_ao4_pins[] = { TEGRA_PIN_JTAG_RTCK, }; +static const unsigned csi_dsib_pins[] = { + TEGRA_PIN_CSI_DSIB, +}; + enum tegra_mux { TEGRA_MUX_BLINK, TEGRA_MUX_CCLA, @@ -1580,6 +1586,16 @@ enum tegra_mux { TEGRA_MUX_VI_ALT3, TEGRA_MUX_VIMCLK2, TEGRA_MUX_VIMCLK2_ALT, + TEGRA_MUX_CSI, + TEGRA_MUX_DSIB, +}; + +static const char * const csi_groups[] = { + "csi_dsib", +}; + +static const char * const dsib_groups[] = { + "csi_dsib", }; #define FUNCTION(fname) \ @@ -1672,10 +1688,13 @@ static struct tegra_function tegra124_functions[] = { FUNCTION(vi_alt3), FUNCTION(vimclk2), FUNCTION(vimclk2_alt), + FUNCTION(csi), + FUNCTION(dsib), }; #define DRV_PINGROUP_REG_A 0x868 /* bank 0 */ #define PINGROUP_REG_A 0x3000 /* bank 1 */ +#define APB_MISC_PINGROUP_REG_A 0x820 /* bank 2 */ #define PINGROUP_REG(r) ((r) - PINGROUP_REG_A) @@ -1744,6 +1763,32 @@ static struct tegra_function tegra124_functions[] = { .drvtype_bit = PINGROUP_BIT_##drvtype(6), \ } +#define APB_MISC_PINGROUP_REG_Y(r) ((r) - APB_MISC_PINGROUP_REG_A) + +#define APB_MISC_PINGROUP(pg_name, r, b, f0, f1, f_safe) \ + { \ + .name = #pg_name, \ + .pins = pg_name##_pins, \ + .npins = ARRAY_SIZE(pg_name##_pins), \ + .funcs = { \ + TEGRA_MUX_ ## f0, \ + TEGRA_MUX_ ## f1, \ + }, \ + .func_safe = TEGRA_MUX_ ## f_safe, \ + .mux_reg = APB_MISC_PINGROUP_REG_Y(r), \ + .mux_bank = 2, \ + .mux_bit = b, \ + .pupd_reg = -1, \ + .tri_reg = -1, \ + .einput_reg = -1, \ + .odrain_reg = -1, \ + .lock_reg = -1, \ + .ioreset_reg = -1, \ + .rcv_sel_reg = -1, \ + .drv_reg = -1, \ + .drvtype_reg = -1, \ + } + static const struct tegra_pingroup tegra124_groups[] = { /* pg_name, f0, f1, f2, f3, r, od, ior, rcv_sel */ PINGROUP(ulpi_data0_po1, SPI3, HSI, UARTA, ULPI, 0x3000, N, N, N), @@ -1979,6 +2024,9 @@ static const struct tegra_pingroup tegra124_groups[] = { DRV_PINGROUP(hv0, 0x9b4, 2, 3, 4, 12, 5, -1, -1, 28, 2, -1, -1, N), DRV_PINGROUP(sdio4, 0x9c4, 2, 3, 4, 12, 5, 20, 5, 28, 2, 30, 2, N), DRV_PINGROUP(ao4, 0x9c8, 2, 3, 4, 12, 7, 20, 7, 28, 2, 30, 2, Y), + + /* pg_name, r b f0, f1, f_safe */ + APB_MISC_PINGROUP( csi_dsib, 0x820, 1, CSI, DSIB, DSIB) }; static const struct tegra_pinctrl_soc_data tegra124_pinctrl = {
This patch adds MIPI CSI/DSIB pad control mux register from the APB misc block to tegra pinctrl. Without writing to this register, the dsib pads are muxed as csi, and cannot be used. The register is not yet documented in the TRM, here is the description: 70000820: APB_MISC_GP_MIPI_PAD_CTRL_0 [31:02] RESERVED [01:01] DSIB_MODE [CSI=0,DSIB=1] [00:00] RESERVED Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/pinctrl/pinctrl-tegra124.c | 48 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)